Re: [PATCH] nfsd: inherit required unset default acls from effective set

[Jeff Layton]

On Thu, 2023-07-20 at 10:43 +0200, Andreas Grünbacher wrote:
> Am Mi., 19. Juli 2023 um 23:22 Uhr schrieb Andreas Grünbacher
> <andreas.gruenbacher@xxxxxxxxx>:
> > 
> > Hi Jeff,
> > 
> > this patch seems useful, thanks.
> > 
> > Am Mi., 19. Juli 2023 um 19:56 Uhr schrieb Jeff Layton <jlayton@xxxxxxxxxx>:
> > > A well-formed NFSv4 ACL will always contain OWNER@/GROUP@/EVERYONE@
> > > ACEs, but there is no requirement for inheritable entries for those
> > > entities. POSIX ACLs must always have owner/group/other entries, even for a
> > > default ACL.
> > 
> > NFSv4 ACLs actually don't *need* to have OWNER@/GROUP@/EVERYONE@
> > entries; that's merely a result of translating POSIX ACLs (or file
> > modes) to NFSv4 ACLs.
> > 

RFC 8881, section 6.4 says:

"The server that supports both mode and ACL must take care to
synchronize the MODE4_*USR, MODE4_*GRP, and MODE4_*OTH bits with the
ACEs that have respective who fields of "OWNER@", "GROUP@", and
"EVERYONE@". This way, the client can see if semantically equivalent
access permissions exist whether the client asks for the owner,
owner_group, and mode attributes or for just the ACL."

...so technically you're correct for a generic NFS server, but in the
Linux nfsd case, we have to have them.

> > > nfsd builds the default ACL from inheritable ACEs, but the current code
> > > just leaves any unspecified ACEs zeroed out. The result is that adding a
> > > default user or group ACE to an inode can leave it with unwanted deny
> > > entries.
> > > 
> > > For instance, a newly created directory with no acl will look something
> > > like this:
> > > 
> > >         # NFSv4 translation by server
> > >         A::OWNER@:rwaDxtTcCy
> > >         A::GROUP@:rxtcy
> > >         A::EVERYONE@:rxtcy
> > > 
> > >         # POSIX ACL of underlying file
> > >         user::rwx
> > >         group::r-x
> > >         other::r-x
> > > 
> > > ...if I then add new v4 ACE:
> > > 
> > >         nfs4_setfacl -a A:fd:1000:rwx /mnt/local/test
> > > 
> > > ...I end up with a result like this today:
> > > 
> > >         user::rwx
> > >         user:1000:rwx
> > >         group::r-x
> > >         mask::rwx
> > >         other::r-x
> > >         default:user::---
> > >         default:user:1000:rwx
> > >         default:group::---
> > >         default:mask::rwx
> > >         default:other::---
> > > 
> > >         A::OWNER@:rwaDxtTcCy
> > >         A::1000:rwaDxtcy
> > >         A::GROUP@:rxtcy
> > >         A::EVERYONE@:rxtcy
> > >         D:fdi:OWNER@:rwaDx
> > >         A:fdi:OWNER@:tTcCy
> > >         A:fdi:1000:rwaDxtcy
> > >         A:fdi:GROUP@:tcy
> > >         A:fdi:EVERYONE@:tcy
> > > 
> > > ...which is not at all expected. Adding a single inheritable allow ACE
> > > should not result in everyone else losing access.
> > > 
> > > The setfacl command solves a silimar issue by copying owner/group/other
> > > entries from the effective ACL when none of them are set:
> > > 
> > >     "If a Default ACL entry is created, and the  Default  ACL  contains  no
> > >      owner,  owning group,  or  others  entry,  a  copy of the ACL owner,
> > >      owning group, or others entry is added to the Default ACL.
> > > 
> > > Having nfsd do the same provides a more sane result (with no deny ACEs
> > > in the resulting set):
> > > 
> > >         user::rwx
> > >         user:1000:rwx
> > >         group::r-x
> > >         mask::rwx
> > >         other::r-x
> > >         default:user::rwx
> > >         default:user:1000:rwx
> > >         default:group::r-x
> > >         default:mask::rwx
> > >         default:other::r-x
> > > 
> > >         A::OWNER@:rwaDxtTcCy
> > >         A::1000:rwaDxtcy
> > >         A::GROUP@:rxtcy
> > >         A::EVERYONE@:rxtcy
> > >         A:fdi:OWNER@:rwaDxtTcCy
> > >         A:fdi:1000:rwaDxtcy
> > >         A:fdi:GROUP@:rxtcy
> > >         A:fdi:EVERYONE@:rxtcy
> > 
> > This resulting NFSv4 ACL is still rather dull; we end up with an
> > inherit-only entry for each effective entry. Those could all be
> > combined, resulting in:
> > 
> >          A:fd:OWNER@:rwaDxtTcCy
> >          A:fd:1000:rwaDxtcy
> >          A:fd:GROUP@:rxtcy
> >          A:fd:EVERYONE@:rxtcy
> > 
> > This will be the common case, so maybe matching entry pairs can either
> > be recombined or not generated in the first place as a further
> > improvement.
> > 

To be clear, this patch fixes the NFSv4->POSIX ACL translation. The
problem you're describing above is more with the POSIX->NFSv4
translation. I don't think we can make the resulting POSIX ACLs more
concise.


> > > Link: https://bugzilla.redhat.com/show_bug.cgi?id=2136452
> > > Reported-by: Ondrej Valousek <ondrej.valousek@xxxxxxxxxxx>
> > > Suggested-by: Andreas Gruenbacher <agruen@xxxxxxxxxx>
> > > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > > ---
> > >  fs/nfsd/nfs4acl.c | 32 +++++++++++++++++++++++++++++---
> > >  1 file changed, 29 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/fs/nfsd/nfs4acl.c b/fs/nfsd/nfs4acl.c
> > > index 518203821790..64e45551d1b6 100644
> > > --- a/fs/nfsd/nfs4acl.c
> > > +++ b/fs/nfsd/nfs4acl.c
> > > @@ -441,7 +441,8 @@ struct posix_ace_state_array {
> > >   * calculated so far: */
> > > 
> > >  struct posix_acl_state {
> > > -       int empty;
> > > +       bool empty;
> > > +       unsigned char valid;
> > 
> > Hmm, "valid" is a bitmask here but it only matters whether it is zero.
> > Shouldn't a bool be good enough? Also, this variable indicates whether
> > special "who" values are present (and which), so the name "valid"
> > probably isn't the best choice.
> > 

Yep, a bool would be fine. This patch went through a bunch of different
revisions and in one, I needed know which individual fields were set. I
kept that here since the storage requirements are still the same, and it
might be more useful for debugging purposes in the future.

> > >         struct posix_ace_state owner;
> > >         struct posix_ace_state group;
> > >         struct posix_ace_state other;
> > > @@ -457,7 +458,7 @@ init_state(struct posix_acl_state *state, int cnt)
> > >         int alloc;
> > > 
> > >         memset(state, 0, sizeof(struct posix_acl_state));
> > > -       state->empty = 1;
> > > +       state->empty = true;
> > >         /*
> > >          * In the worst case, each individual acl could be for a distinct
> > >          * named user or group, but we don't know which, so we allocate
> > > @@ -624,7 +625,7 @@ static void process_one_v4_ace(struct posix_acl_state *state,
> > >         u32 mask = ace->access_mask;
> > >         int i;
> > > 
> > > -       state->empty = 0;
> > > +       state->empty = false;
> > > 
> > >         switch (ace2type(ace)) {
> > >         case ACL_USER_OBJ:
> > > @@ -633,6 +634,7 @@ static void process_one_v4_ace(struct posix_acl_state *state,
> > >                 } else {
> > >                         deny_bits(&state->owner, mask);
> > >                 }
> > > +               state->valid |= ACL_USER_OBJ;
> > >                 break;
> > >         case ACL_USER:
> > >                 i = find_uid(state, ace->who_uid);
> > > @@ -655,6 +657,7 @@ static void process_one_v4_ace(struct posix_acl_state *state,
> > >                         deny_bits_array(state->users, mask);
> > >                         deny_bits_array(state->groups, mask);
> > >                 }
> > > +               state->valid |= ACL_GROUP_OBJ;
> > >                 break;
> > >         case ACL_GROUP:
> > >                 i = find_gid(state, ace->who_gid);
> > > @@ -686,6 +689,7 @@ static void process_one_v4_ace(struct posix_acl_state *state,
> > >                         deny_bits_array(state->users, mask);
> > >                         deny_bits_array(state->groups, mask);
> > >                 }
> > > +               state->valid |= ACL_OTHER;
> > >         }
> > >  }
> > > 
> > > @@ -726,6 +730,28 @@ static int nfs4_acl_nfsv4_to_posix(struct nfs4_acl *acl,
> > >                 if (!(ace->flag & NFS4_ACE_INHERIT_ONLY_ACE))
> > >                         process_one_v4_ace(&effective_acl_state, ace);
> > >         }
> > > +
> > > +       /*
> > > +        * At this point, the default ACL may have zeroed-out entries for owner,
> > > +        * group and other. That usually results in a non-sensical resulting ACL
> > > +        * that denies all access except to any ACE that was explicitly added.
> > > +        *
> > > +        * The setfacl command solves a similar problem with this logic:
> > > +        *
> > > +        * "If  a  Default  ACL  entry is created, and the Default ACL contains
> > > +        *  no owner, owning group, or others entry,  a  copy of  the  ACL
> > > +        *  owner, owning group, or others entry is added to the Default ACL."
> > > +        *
> > > +        * If none of the requisite ACEs were set, and some explicit user or group
> > > +        * ACEs were, copy the requisite entries from the effective set.
> > > +        */
> > > +       if (!default_acl_state.valid &&
> > > +           (default_acl_state.users->n || default_acl_state.groups->n)) {
> > > +               default_acl_state.owner = effective_acl_state.owner;
> > > +               default_acl_state.group = effective_acl_state.group;
> > > +               default_acl_state.other = effective_acl_state.other;
> > > +       }
> > > +
> 
> The other thing I'm wondering about is whether it would make more
> sense to fake up for missing entries individually as setfacl does:
> 
> http://git.savannah.nongnu.org/cgit/acl.git/tree/tools/do_set.c#n368
> 

Oh, I was going by the description in the manpage which seemed to
indicate that if any of those had been set in the effective ACL that we
wouldn't try to fake up anything. I actually had an earlier version that
did what you suggest here. I can change it if you think that's more
correct. It might be good to revise that description in the setfacl
manpage since it's a little unclear.

> > >         *pacl = posix_state_to_acl(&effective_acl_state, flags);
> > >         if (IS_ERR(*pacl)) {
> > >                 ret = PTR_ERR(*pacl);
> > > 
> > > ---
> > > base-commit: 9d985ab8ed33176c3c0380b7de589ea2ae51a48d
> > > change-id: 20230719-nfsd-acl-5ab61537e4e6
> > > 
> > > Best regards,
> > > --
> > > Jeff Layton <jlayton@xxxxxxxxxx>
> 
> Thanks,
> Andreas

-- 
Jeff Layton <jlayton@xxxxxxxxxx>