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

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

[Jeff Layton]

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.

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

Link: https://bugzilla.redhat.com/show_bug.cgi?id=2136452
Reported-by: Ondrej Valousek <ondrej.valousek@diasemi.com>
Suggested-by: Andreas Gruenbacher <agruen@redhat.com>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 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;
 	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;
+	}
+
 	*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@kernel.org>

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

[Chuck Lever III]

> On Jul 19, 2023, at 1:49 PM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> 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.
> 
> 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
> 
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2136452
> Reported-by: Ondrej Valousek <ondrej.valousek@diasemi.com>
> Suggested-by: Andreas Gruenbacher <agruen@redhat.com>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>

As you pointed out in the bug report, there is not much testing
infrastructure for NFSv4 ACLs. It will be hard to tell in
advance if this change results in a behavior regression.

On the other hand, I'm not sure we have a large cohort of
NFSv4 ACL users on Linux.

I can certainly apply this to nfsd-next at least for a few
weeks to see if anyone yelps.


> ---
> 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;
> 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;
> + }
> +
> *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@kernel.org>
> 

--
Chuck Lever

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

[Jeff Layton]

On Wed, 2023-07-19 at 19:02 +0000, Chuck Lever III wrote:
> 
> > On Jul 19, 2023, at 1:49 PM, Jeff Layton <jlayton@kernel.org> wrote:
> > 
> > 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.
> > 
> > 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
> > 
> > Link: https://bugzilla.redhat.com/show_bug.cgi?id=2136452
> > Reported-by: Ondrej Valousek <ondrej.valousek@diasemi.com>
> > Suggested-by: Andreas Gruenbacher <agruen@redhat.com>
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> 
> As you pointed out in the bug report, there is not much testing
> infrastructure for NFSv4 ACLs. It will be hard to tell in
> advance if this change results in a behavior regression.
> 
> On the other hand, I'm not sure we have a large cohort of
> NFSv4 ACL users on Linux.
> 
> I can certainly apply this to nfsd-next at least for a few
> weeks to see if anyone yelps.
> 

Thanks, that's probably the best we can do, given the state of v4 ACL
test coverage.
-- 
Jeff Layton <jlayton@kernel.org>

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

[Andreas Grünbacher]

Hi Jeff,

this patch seems useful, thanks.

Am Mi., 19. Juli 2023 um 19:56 Uhr schrieb Jeff Layton <jlayton@kernel.org>:
> 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.

> 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.

> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2136452
> Reported-by: Ondrej Valousek <ondrej.valousek@diasemi.com>
> Suggested-by: Andreas Gruenbacher <agruen@redhat.com>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  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.

>         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;
> +       }
> +
>         *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@kernel.org>

Thanks,
Andreas

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

[Andreas Grünbacher]

Am Mi., 19. Juli 2023 um 23:22 Uhr schrieb Andreas Grünbacher
<andreas.gruenbacher@gmail.com>:
>
> Hi Jeff,
>
> this patch seems useful, thanks.
>
> Am Mi., 19. Juli 2023 um 19:56 Uhr schrieb Jeff Layton <jlayton@kernel.org>:
> > 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.
>
> > 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.
>
> > Link: https://bugzilla.redhat.com/show_bug.cgi?id=2136452
> > Reported-by: Ondrej Valousek <ondrej.valousek@diasemi.com>
> > Suggested-by: Andreas Gruenbacher <agruen@redhat.com>
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  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.
>
> >         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

> >         *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@kernel.org>

Thanks,
Andreas

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@gmail.com>:
> > 
> > Hi Jeff,
> > 
> > this patch seems useful, thanks.
> > 
> > Am Mi., 19. Juli 2023 um 19:56 Uhr schrieb Jeff Layton <jlayton@kernel.org>:
> > > 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@diasemi.com>
> > > Suggested-by: Andreas Gruenbacher <agruen@redhat.com>
> > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > ---
> > >  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@kernel.org>
> 
> Thanks,
> Andreas

-- 
Jeff Layton <jlayton@kernel.org>