Hmm... A drat. I forgot that comments can only be started at the beginning of a line in the config file. (I've been burned by it before).

vfs objects = ;; shadow_copy2 zfsacl full_audit

doesn't do quite what I thought. :-)

Anyway, it seems the culprit is the "zfsacl" vfs module. With it enabled I see this behaviour for non-ACL-protected folders (on a ZFS filesystem) on our FreeBSD-with-ZFS servers.

Created attachment 16478 [details]
Patch that fixes the missing permission bits for FreeBSD/ZFS(unix mode)/Samba

This patch adds a smb.conf option "acl force attrib write", which, if set to yes, will add the Write_Attributes and Write_Names_Attrs ACL permissions for ACE entries where the Write_Data permssions is set.

This is a bit of a hack but it is needed on FreeBSD with data stored on ZFS when the users have set the permssions using Unix "mode bits" instead of ACLs. The problem is that ZFS doesn't know anything about mode bits and the mode bits are mapped to a set of ZFS ACLs permissions, which then when sent to a Windows client via Samba will cause the Windows client to think that it doesn't have permission to write to files that it really does (due to the missing Write Attributes & Write Named Attrs permissions).

A small example (the group bits are the ones of interest), a file set to 0770:

-rwxrwx---  1 peter86  employee-liu.se  0 Mar  1 15:33 f-770

   group@:rwxp--a-R-c--s:-------:allow

acl force attrib write = false:
 ACL:AD\employee-liu.se:ALLOWED/0x0/0x001200af

acl force attrib write = true:
 ACL:AD\employee-liu.se:ALLOWED/0x0/RWX


Without the patch a user in the "employee-liu.se" group that isn't me connecting from a Windows client doesn't like to write to this file. Which can be a pain in shared folders when some users are connecting from Unix clients...

Created attachment 16479 [details]
Sample output for various mode bit combilations vs ACLs via Samba

Thanks Peter, but I'd really like to avoid having another parameter added for this.

In your description you state:

"when the users have set the permissions using Unix "mode bits" instead of ACLs."

Is the comment "well don't do that then" too harsh here ? :-).

Jeremy.

(In reply to Jeremy Allison from comment #4)
Adding Andrew to the loop. Andrew, what's your take on this?

This is very similar to what I have applied in the vfs module we use in freenas for ACLs in specific edge cases (I call the parameter "map_modify" there).

It's only an issue when users have zfsacl + no ACLs set on a file (which I think is on the edge of server misconfiguration). I think correct place to have this logic is in vfs_zfsacl since it's not specifically related to nfsv4 ACLs in general (just a ZFS-specific behavior).

We can also auto-detect in vfs_connect_fn() in zfsacl using acl_is_trivial_np(3) or the FreeBSD case. This subroutine will determine whether an ACL is trivial (can be expressed as POSIX mode without losing info), which can be used to  configure the share to behave differently in this edge case.

I'm not a terribly big fan of granting rights where they're not present in the ZFS ACL. After all, we have native NFSv4 ACLs, they should just be configured correctly (or if NFSv4 ACLs aren't going to be used, then the module shouldn't be enabled).

Having supported a large number of users, I think best practice in the general case for SMB shares on ZFS is to set ACLs, and then set the ZFS aclmode property to "restricted" (which causes chmod(2) to fail with EPERM in case of a non-trivial ACL). Doing otherwise can result in an unsupportable mess of permissions as an inadvertent chmod can really make a mess of things (stipping inheritance bits, removing permissions, etc).

TL;DR, 
1) If a change like this goes in, it should be in zfsacl, not general nfsv4 code.
2) We can potentially have this just auto-configure
3) It's much better to just have people configure ACLs correctly. 

I regularly regret doing (1+2) because it: 
(a) adds yet another alternate behavior that can break
(b) it doesn't actually fix the misconfiguration. SMB clients will still add a ZFS ACL via SMB protocol.
(c) the patch makes it easier for the administrator to be less aware of the details of what's happening permissions-wise on the server. I still almost weekly deal with a handful of tickets / problem reports with root cause being misconfigured ACLs (partially because I was too nice in allowing (1-2)). 

Actually... maybe we can use acl_is_trivial_np() in vfs_connect_fn in vfs_zfsacl to deny access to the share if it doesn't have a proper ACL set on it :)))

"Actually... maybe we can use acl_is_trivial_np() in vfs_connect_fn in vfs_zfsacl to deny access to the share if it doesn't have a proper ACL set on it :)))"

That sounds like a plan ! Push an MR and I'll review :-).

(In reply to Jeremy Allison from comment #4)

> "when the users have set the permissions using Unix "mode bits" instead of ACLs."
> Is the comment "well don't do that then" too harsh here ? :-).

Heh. Yeah, I wish I was so lucky... But with a large set of Windows users expecting ACLs to work, another large set of Linux users used to the "chmod-way" (and with Linux crappy ACL implementation in general I can't really blame them). 

And some that access the files from MacOS - and some from all three systems at the same time). 

Via both SMB and NFSv4 (and SFTP).

And lot's of files migrated from legacy file servers (some Windows, some EMC VNX, NetApp, some Solaris, some Linux - that have been moved between systems too before) it's a big mess...

Someone should invent an "aclck" tool that can check and fix ACLs automatically :-)

So that "hack" was the least painful way to give users workable access to their files in the shared folders. And I don't doubt it could be written in a better way.

And I know it's not perfect but I figured I'll make it available anyway in case other people would run into similar problems.

(Btw, it took some time to figure out why Windows clients would refuse writing to perfectly fine writable documents that all other systems had not problem writing to :-)

(In reply to Andrew Walker from comment #6)

> It's only an issue when users have zfsacl + no ACLs set on a file (which I think is
> on the edge of server misconfiguration). I think correct place to have this logic is
> in vfs_zfsacl since it's not specifically related to nfsv4 ACLs in general (just a
> ZFS-specific behavior).

I agree that vfs_zfsacl is a better place for it.

And I think it's only a problem for systems that emulate mode bits via ZFS ACLs. Ie, had the acl_get() function returned an error instead of the emulated ACL then Samba would have falled back to the code that handles normal mode bits and would have had the right permissions set...

- Peter

(In reply to Peter Eriksson from comment #9)
>Someone should invent an "aclck" tool that can check and fix ACLs automatically :-)

I have a merge request sitting somewhere on FreeBSD's phabricator to add NFSv41 ACL support along with changes to getfacl / setfacl to do NFSv41 inheritance. The algorithm in this case would be roughly to walk down tree and on files / dirs without the ACL flag ACL_FLAG_PROTECTED, strip all entries with ACE_INHERITED_ACE, and add new ones calculated from the parent. This is roughly same as what Windows does.

>And I think it's only a problem for systems that emulate mode bits via ZFS ACLs. Ie, had the acl_get() function returned an error instead of the emulated ACL then Samba would have falled back to the code that handles normal mode bits and would have had the right permissions set...

No. It would still be a problem even if we returned an error here. Consider the following situation with aclmode=passthrough.

----
root@truenas[/mnt/tank]# setfacl -a 0 g:1000:full_set:fd:allow foo 
root@truenas[/mnt/tank]# setfacl -m owner@:full_set:fd:allow,group@:modify_set:fd:allow,everyone@:read_set:fd:allow foo 
root@truenas[/mnt/tank]# getfacl foo
# file: foo
# owner: root
# group: wheel
        group:1000:rwxpDdaARWcCos:fd-----:allow
            owner@:rwxpDdaARWcCos:fd-----:allow
            group@:rwxpDdaARWc--s:fd-----:allow
         everyone@:r-----a-R-c---:fd-----:allow
root@truenas[/mnt/tank]# chmod 755 foo
root@truenas[/mnt/tank]# getfacl foo
# file: foo
# owner: root
# group: wheel
        group:1000:rwxpDdaARWcCos:fd-----:allow
            owner@:rwxpDdaARWcCos:fdi----:allow
            group@:rwxpDdaARWc--s:fdi----:allow
         everyone@:r-----a-R-c---:fdi----:allow
            owner@:rwxp--aARWcCos:-------:allow
            group@:r-x---a-R-c--s:-------:allow
         everyone@:r-x---a-R-c--s:-------:allow
----
original ACL entries are converted to inherit-only (i), and three new entries equivalent to the specified POSIX mode are added to end of ACL. It's not just an issue for the one case where ACL has been entirely stripped (i.e. "setfacl -b"). Hence it's better overall for the administrator to prevent chmod if he's using NFSv4 ACLs. The interplay between chmod and existing ACLs is nuanced and generally not easily maintainable at scale.

Also, it's _much_ better to not have the OS fail on acl_get_* depending on whether the ACL is trivial. This is the reason why we have acl_is_trivial_np() and why ZFS internally has a flag on the ACL to indicate that it's trivial.

(In reply to Andrew Walker from comment #11)

> Hence it's better overall for the administrator to prevent chmod if he's using NFSv4
> ACLs. The interplay between chmod and existing ACLs is nuanced and generally not 
> easily maintainable at scale.

Oh, I agree. Definitely better for me. I really wish it was possible to disable chmod et al and get rid of the "mode bits" stuff completely... But if I do that then I'll have some very angry users at my door.

We have users that set up things like complete file trees complete with unix permissions that they then copy (rsync/tar) to other sites (not using ZFS) and they weren't happy when we forced ACLs on them and all their finely tuned mode bits where garbled...

Sure it might be possible with some care to only use specific ACLs so the "mode bits" look ok but getting the users to understand that... Urgh.

Anyway, I think this is getting a bit off topic now :-)

In comment #10:

> And I think it's only a problem for systems that emulate mode bits via ZFS
> ACLs. Ie, had the acl_get() function returned an error instead of the emulated
> ACL then Samba would have falled back to the code that handles normal mode
> bits and would have had the right permissions set...

So is the solution to call acl_is_trivial_np() inside a ZFS vectored version of acl_get() and deliberately return an error here if it's a trivial ACL ?

(In reply to Jeremy Allison from comment #13)

> So is the solution to call acl_is_trivial_np() inside a ZFS vectored version of 
> acl_get() and deliberately return an error here if it's a trivial ACL ?

Hmm.. Probably not. Well, perhaps. In an ideal world. With just Samba accessing the files. But in (atleast in mine) people have sometimes modified the ACLs, like adding an overide group ACL (using FreeBSD tools directly on the servers, or via NFS from Linux clients), so the nice and pure just owner@/group@/everyone@ "trivial" acls aren't always so "pure" anymore. So a solution like that still would cause problems (for us) - ie denying groups write access to files/directories.

(In reply to Jeremy Allison from comment #13)
> So is the solution to call acl_is_trivial_np() inside a ZFS vectored version of acl_get() and deliberately return an error here if it's a trivial ACL ?

I think that trying to figure out whether an ACL is trivial or not on a per-request basis through get_nt_acl_at / fget_nt_acl in samba is too expensive with the way that this is currently implemented in FreeBSD. I was proposing doing a check on handle->conn->connectpath zfsacl_connect() and setting a share-wide parameter at that time. 

If we want to do this on a per-request basis, changes to FreeBSD's VOP_GETACL and zfs_vnops will be required to expose ZFS_ACL_TRIVIAL from the ZFS znode pflags (I'd have to do this through NFSv41 ACL flags). So perhaps part of getting NFSv41 ACL support, but not realizable in the short term. We're also limited in this place because we're passing through libsunacl as opposed to using the native FreeBSD ACL APIs.

I'll throw a patch on here tomorrow for vfs_zfsacl to show what I'm talking about, though I'm still skeptical that this is the right way to do things. Ideally, the admin should set the ZFS dataset property aclmode=restricted, which prevents chmod if the ACL on a file is non-trivial (neatly skirting around the issue).

Fwiw, GPFS behaves just the same:

[root@bb-centos7 gpfs]# ls -l file
----rw---- 1 localbb localbb 4 Jun 26  2020 file

[root@bb-centos7 gpfs]# mmgetacl /gpfs/file
#NFSv4 ACL
#owner:localbb
#group:localbb
special:owner@:---c:allow
 (-)READ/LIST (-)WRITE/CREATE (-)APPEND/MKDIR (X)SYNCHRONIZE (X)READ_ACL  (X)READ_ATTR  (X)READ_NAMED
 (-)DELETE    (-)DELETE_CHILD (X)CHOWN        (-)EXEC/SEARCH (X)WRITE_ACL (X)WRITE_ATTR (X)WRITE_NAMED

special:group@:rw--:allow
 (X)READ/LIST (X)WRITE/CREATE (X)APPEND/MKDIR (X)SYNCHRONIZE (X)READ_ACL  (X)READ_ATTR  (X)READ_NAMED
 (-)DELETE    (-)DELETE_CHILD (-)CHOWN        (-)EXEC/SEARCH (-)WRITE_ACL (-)WRITE_ATTR (-)WRITE_NAMED

special:everyone@:----:allow
 (-)READ/LIST (-)WRITE/CREATE (-)APPEND/MKDIR (X)SYNCHRONIZE (X)READ_ACL  (X)READ_ATTR  (X)READ_NAMED
 (-)DELETE    (-)DELETE_CHILD (-)CHOWN        (-)EXEC/SEARCH (-)WRITE_ACL (-)WRITE_ATTR (-)WRITE_NAMED

[root@bb-centos7 gpfs]# su - localbb

[localbb@bb-centos7 ~]$ touch /gpfs/file

[localbb@bb-centos7 ~]$ logout

So of course, touch is allowed even though the mapped ACE doesn't have WRITE_ATTR set.

I wonder whether we maybe really should always add the permission bit when mapping special entries from disk to NT ACL in the common NFS4 ACL mapping glue code, regardless if the ACL is trivial or not.

(In reply to Ralph Böhme from comment #16)
>So of course, touch is allowed even though the mapped ACE doesn't have WRITE_ATTR set.
Same is true on FreeBSD. WRITE_DATA implies WRITE_ATTRIBUTES and WRITE_NAMED_ATTRS.

>I wonder whether we maybe really should always add the permission bit when mapping special entries from disk to NT ACL in the common NFS4 ACL mapping glue code, regardless if the ACL is trivial or not.

Perhaps so in that case. New day, new perspective :) 

For entries
1) that have special ids
2) are SMB_ACE4_ACCESS_ALLOWED_ACE_TYPE
3) and do not have inheritance flags

We can map SMB_ACE4_WRITE_DATA to SEC_FILE_WRITE_DATA|SEC_FILE_WRITE_EA|SEC_FILE_WRITE_ATTRIBUTE.

(1) - (3) above should more or less uniquely identify when these entries are set via chmod and WRITE_DATA should be viewed somewhat more generically.

(In reply to Andrew Walker from comment #17)
I guess we should do the mapping for inherited special ACEs as well as an inherited special group ACE with WRITE_DATA implicitly grants WRITE_ATTR just like the non-inherited ACE in the previous example:

# mmgetacl file
#NFSv4 ACL
#owner:localbb
#group:localbb
special:owner@:---c:allow
 (-)READ/LIST (-)WRITE/CREATE (-)APPEND/MKDIR (X)SYNCHRONIZE (X)READ_ACL  (X)READ_ATTR  (X)READ_NAMED
 (-)DELETE    (-)DELETE_CHILD (X)CHOWN        (-)EXEC/SEARCH (X)WRITE_ACL (X)WRITE_ATTR (X)WRITE_NAMED

special:group@:r---:allow
 (X)READ/LIST (-)WRITE/CREATE (-)APPEND/MKDIR (X)SYNCHRONIZE (X)READ_ACL  (X)READ_ATTR  (X)READ_NAMED
 (-)DELETE    (-)DELETE_CHILD (-)CHOWN        (-)EXEC/SEARCH (-)WRITE_ACL (-)WRITE_ATTR (-)WRITE_NAMED

special:group@:rw--:allow:Inherited
 (X)READ/LIST (X)WRITE/CREATE (X)APPEND/MKDIR (X)SYNCHRONIZE (X)READ_ACL  (X)READ_ATTR  (X)READ_NAMED
 (-)DELETE    (-)DELETE_CHILD (-)CHOWN        (-)EXEC/SEARCH (-)WRITE_ACL (-)WRITE_ATTR (-)WRITE_NAMED

special:everyone@:----:allow
 (-)READ/LIST (-)WRITE/CREATE (-)APPEND/MKDIR (X)SYNCHRONIZE (X)READ_ACL  (X)READ_ATTR  (X)READ_NAMED
 (-)DELETE    (-)DELETE_CHILD (-)CHOWN        (-)EXEC/SEARCH (-)WRITE_ACL (-)WRITE_ATTR (-)WRITE_NAMED

# su - localbb
Last login: Tue Mar  2 09:45:17 CET 2021 on pts/1

$

(In reply to Ralph Böhme from comment #18)
I'm not familiar with GPFS. What is result if you set an ACL on a file with owner@, group@, and everyone@ with full_control, then chmod 777 the file? In the case of FreeBSD, the newly-created ACL entries lack inheritance flags (including ACE_INHERITED_ACE). The end result is that you will never really inherit the mode of the parent file. In ZFS if an ACL lacks inheritable special entries, ZFS will auto-generate ones (unless the aclmode is restricted) that lack inheritance flags.

Created attachment 16488 [details]
General nfs4 acls patch

Patch masking write bits in conditions laid out in convo. Patch is against samba 4.12.

(In reply to Andrew Walker from comment #19)
Well, I guess the question if a special group entry with inherited flag set and granting WRITE_DATA does grant WRITE_ATTR or not. Can you verify this?

If it does, then we should add the mapped NT right regardless of inheritance flags.

(In reply to Ralph Böhme from comment #21)
Sorry, I derped that regarding group@. Granting WRITE_ATTRIBUTES for group@ seems incorrect (since it will expand permissions beyond what is granted by POSIX mode). 

Following comments in vnode.h are possibly enlightening regarding VFS behavior in FreeBSD
---
#define VWRITE                  000000000200 /* write permission */
#define VREAD                   000000000400 /* read permission */
#define VAPPEND                 000000040000 /* permission to write/append */
#define VREAD_NAMED_ATTRS       000000200000 /* not used */
#define VWRITE_NAMED_ATTRS      000000400000 /* not used */
#define VDELETE_CHILD           000001000000
#define VREAD_ATTRIBUTES        000002000000 /* permission to stat(2) */
#define VWRITE_ATTRIBUTES       000004000000 /* change {m,c,a}time */
#define VDELETE                 000010000000
#define VREAD_ACL               000020000000 /* read ACL and file mode */
#define VWRITE_ACL              000040000000 /* change ACL and/or file mode */
#define VWRITE_OWNER            000100000000 /* change file owner */

/*
 * Permissions that were traditionally granted only to the file owner.
 */
#define VADMIN_PERMS    (VADMIN | VWRITE_ATTRIBUTES | VWRITE_ACL | \
    VWRITE_OWNER)

/*
 * Permissions that were traditionally granted to everyone.
 */
#define VSTAT_PERMS     (VREAD_ATTRIBUTES | VREAD_ACL)
----

These are the items that explicitly map to NFSv4 ACLs. group@ by default will _not_ have V_WRITE_ATTRIBUTES from chmod. 

VWRITE_NAMED_ATTRS is not enforced and the same is the case for VREAD_NAMED_ATTRS (VWRITE_DATA and VREAD_DATA is used for attributes-related access checks respectively).

I will update patch accordingly for case of group@.

Above is true for all cases for permissions (not just special ids), it's just case  of chmod where we lose some bits that is somewhat problematic.

(In reply to Ralph Böhme from comment #21)
On the other hand, we map "write" from posix acl side to FILE_GENERIC_WRITE, which includes FILE_WRITE_ATTRIBUTES.

I threw the patch onto gitlab to hit CI here: https://gitlab.com/samba-team/samba/-/merge_requests/1820

Looks like some fairly immediate test failures because raw.acls test explicitly sets DACL with owner_sid, SEC_ACE_TYPE_ACCESS_ALLOWED, SEC_FILE_WRITE_DATA, and then checks the result (which is the precise situation in which we're mapping additional rights here). I figured it's easier to discuss implementation details in gitlab.

(In reply to Andrew Walker from comment #22)
Sorry, but I can't follow you there...

The question is: does touch work on a file on ZFS with an inherited @group ACE granting WRITE_DATA?

Afaict this is all we care about for determining if we should implicitly add WRITE_ATTR when mapping.

How does this relate to the chmod stuff you bring up? I fail to see the relation to the task of mapping an ZFS NFS4 ACL to an NT ACL.

(In reply to Peter Eriksson from comment #0)
Do you mind if I ask what could be a stupid question ?

When Peter says 'server' what does he actually mean ?

Every Samba instance is a 'server', it could be a standalone server, a PDC, an AD DC or a Unix domain member, they are all 'servers'.

The default for 'vfs objects' is 'vfs objects =', so if he is just setting setting 'vfs objects' then he should stop, it is syntactically incorrect. If he is setting 'vfs objects =', it should have no effect, unless he is doing this on a DC, and again, he should stop doing this, he is turning off the default vfs objects 'dfs_samba4' & 'acl_xattr'

(In reply to Ralph Böhme from comment #25)
>smbuser@homenas:/mnt/dozer/NFS4TEST % touch testdir/testfile 
Success for updating to current time

>smbuser@homenas:/mnt/dozer/NFS4TEST % touch -d 2020-01-01T01:01:01 >testdir/testfile 
>touch: testdir/testfile: Operation not permitted
Failure to set to arbitrary time.

This is because in kernel WRITE_DATA does not imply WRITE_ATTRIBUTES. I misremembered this point. Which is what I tried to point out above.

>smbuser@homenas:/mnt/dozer/NFS4TEST % getfacl testdir/testfile 
># file: testdir/testfile
># owner: root
># group: SMBGROUP
>            group@:rw----a-R-c---:------I:allow
^^^ single inherited group@ entry from parent directory


>root@homenas[/mnt/dozer/NFS4TEST]# setfacl -b testdir/testfile 
Ran setfacl -b to strip ACL and retest.

>root@homenas[/mnt/dozer/NFS4TEST]# su smbuser                 
>smbuser@homenas:/mnt/dozer/NFS4TEST % touch -d 2020-01-01T01:01:01 >testdir/testfile
>touch: testdir/testfile: Operation not permitted
Still failure

In above tests care was take to avoid using owner of directory / file.

(In reply to Rowland Penny from comment #26)
This is a general issue with how specific ZFS ACL entries (or NFSv4 ACLs) are converted into DACL entries in a NT ACL. Not relevant to server type. FYI, last time I checked, vfs_acl_xattr does not work on FreeBSD on ZFS due to differing available xattr namespaces and a lack of POSIX ACL support.

(In reply to Andrew Walker from comment #27)
Prior to "touch" test in this comment I created a directory without an ACL and then set the following group@ entry
> setfacl -a 0 group@:rwaRc:f:allow testdir  
so that a newly created file would inherit a group@ entry granting WRITE, but not WRITE_ATTRIBUTES

If we are concerned about ZFS behavior in case of lack of inheriting special entries (unless the ZFS aclmode property is set to "restricted" and ACL is non-trivial), then ZFS will append three entries representing requested mode when creating the new filesystem object and determining inherited ACL.

root@homenas[/mnt/dozer/NFS4TEST]# mkdir testdir 
root@homenas[/mnt/dozer/NFS4TEST]# setfacl -a 0 everyone@:full_set::allow testdir
root@homenas[/mnt/dozer/NFS4TEST]# setfacl -x 1 testdir
root@homenas[/mnt/dozer/NFS4TEST]# setfacl -x 1 testdir
root@homenas[/mnt/dozer/NFS4TEST]# setfacl -x 1 testdir
root@homenas[/mnt/dozer/NFS4TEST]# getfacl testdir
# file: testdir
# owner: root
# group: wheel
         everyone@:rwxpDdaARWcCos:-------:allow
root@homenas[/mnt/dozer/NFS4TEST]# touch testdir/testfile
root@homenas[/mnt/dozer/NFS4TEST]# getfacl testdir/testfile 
# file: testdir/testfile
# owner: root
# group: wheel
            owner@:rw-p--aARWcCos:-------:allow
            group@:r-----a-R-c--s:-------:allow
         everyone@:r-----a-R-c--s:-------:allow

(In reply to Andrew Walker from comment #27)
>>smbuser@homenas:/mnt/dozer/NFS4TEST % touch testdir/testfile 
>
> Success for updating to current time
>
>> smbuser@homenas:/mnt/dozer/NFS4TEST % touch -d 2020-01-01T01:01:01 testdir/testfile 
>> touch: testdir/testfile: Operation not permitted
>
> Failure to set to arbitrary time.

Huh? Really? But both try to change the mtime of the file, just to (sligtly) different values. I'm really really stumped as to one succeeds while the other fails, but maybe my brain is EOL at the end of the day... :)

(In reply to Ralph Böhme from comment #30)
It's implementing RFC 5661 Section 6.2.1.3.1:

   ACE4_WRITE_ATTRIBUTES

      Operation(s) affected:

         SETATTR of time_access_set, time_backup,

         time_create, time_modify_set, mimetype, hidden, system

      Discussion:

         Permission to change the times associated with a file or
         directory to an arbitrary value.  Also permission to change the
         mimetype, hidden, and system attributes.  A user having
         ACE4_WRITE_DATA or ACE4_WRITE_ATTRIBUTES will be allowed to set
         the times associated with a file to the current server time.

(In reply to Andrew Walker from comment #31)
Holy moly, I was totally unaware of that nuisance. So ACE4_WRITE_DATA only allows "to set the times associated with a file to the current server time.".

So it looks like this doesn't leave us any simple sane options to improve behaviour, does it? I'm not sure I like the trivial ACL check on the share root at tcon time.

(In reply to Ralph Böhme from comment #32)
>So it looks like this doesn't leave us any simple sane options to improve behaviour, does it? I'm not sure I like the trivial ACL check on the share root at tcon time.

Perhaps it's better to more clearly characterize the problem with simple case. Take this file:
>root@homenas[/mnt/dozer/NFS4TEST/foo]# getfacl bar.txt
># file: bar.txt
># owner: root
># group: builtin_users
>            owner@:rwxpDdaARWcCos:-------:allow
>            group@:rwxpDdaARWcCos:-------:allow
>         everyone@:rwxpDdaARWcCos:-------:allow

Currently all special IDs have full set of permissions. When you open the file in notepad, Windows sends smb2 create with access mask including (SEC_FILE_WRITE_DATA|SEC_FILE_WRITE_ATTRIBUTES|SEC_FILE_WRITE_NAMED_ATTRS). NFSv4 ACL maps to these permissions and smbd_check_access_rights() gives a thumbs up.

User decides he doesn't want everyone to be able to write to bar.txt and decides to chmod 775 bar.txt.
>root@homenas[/mnt/dozer/NFS4TEST/foo]# su smbuser
>smbuser@homenas:/mnt/dozer/NFS4TEST/foo % chmod 775 bar.txt
>smbuser@homenas:/mnt/dozer/NFS4TEST/foo % getfacl bar.txt 
># file: bar.txt
># owner: root
># group: builtin_users
>            owner@:rwxp--aARWcCos:-------:allow
>            group@:rwxp--a-R-c--s:-------:allow
>         everyone@:r-x---a-R-c--s:-------:allow

When we generate the NT ACL it lacks (SEC_FILE_WRITE_ATTRIBUTES|SEC_FILE_WRITE_NAMED_ATTRS). User tries to open file in notepad again and can't save it because smb2 create failed with the normal requested access mask:
>[2021/03/03 11:30:40.984574, 10, pid=41882, effective(1000, 1000), real(0, 0)] ../../source3/smbd/open.c:196(smbd_check_access_rights)
  smbd_check_access_rights: file foo/bar.txt requesting 0x12019f returning 0x110 (NT_STATUS_ACCESS_DENIED)

ACCESS_DENIED is not being returned by OS in this case (it's internal Samba check). Proposal is to basically do something similar to map_canon_ace_perms() in source3/smbd/posix_acls.c and grant roughly FILE_GENERIC_WRITE in case it looks like we inadvertently have a POSIX mode "write" represented in our special entry.

I hope this clarifies somewhat.

Your patch is not doing the right thing. Samba denies access because the client's requested access mask does not match the actual filesystems permissions. If you fake the reported permissions to be more open than they actually are, this might fix notepad in this case but it will fail as soon as an application is actually trying to make use of  the SEC_FILE_WRITE_ATTRIBUTES or SEC_FILE_WRITE_NAMED_ATTRS rights because then the filesystem will deny that access.

There is probably a reason why the filesystem is not setting the "write attributes" and "write named attrs" bits: This is POSIX semantics. Windows has different default semantics here are different (which is why there is the "dos file mode" parameter, but which probably is currently not helping for NFS4 filesystems).