* [PATCH] nfsd: inherit required unset default acls from effective set
@ 2023-07-19 17:49 Jeff Layton
2023-07-19 19:02 ` Chuck Lever III
2023-07-19 21:22 ` Andreas Grünbacher
0 siblings, 2 replies; 6+ messages in thread
From: Jeff Layton @ 2023-07-19 17:49 UTC (permalink / raw)
To: Chuck Lever, Neil Brown, Olga Kornievskaia, Dai Ngo, Tom Talpey
Cc: linux-nfs, linux-fsdevel, linux-kernel, Ondrej Valousek,
Andreas Gruenbacher, 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>
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] nfsd: inherit required unset default acls from effective set
2023-07-19 17:49 [PATCH] nfsd: inherit required unset default acls from effective set Jeff Layton
@ 2023-07-19 19:02 ` Chuck Lever III
2023-07-19 19:12 ` Jeff Layton
2023-07-19 21:22 ` Andreas Grünbacher
1 sibling, 1 reply; 6+ messages in thread
From: Chuck Lever III @ 2023-07-19 19:02 UTC (permalink / raw)
To: Jeff Layton
Cc: Neil Brown, Olga Kornievskaia, Dai Ngo, Tom Talpey,
Linux NFS Mailing List, linux-fsdevel, open list,
Ondrej Valousek, Andreas Gruenbacher
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nfsd: inherit required unset default acls from effective set
2023-07-19 19:02 ` Chuck Lever III
@ 2023-07-19 19:12 ` Jeff Layton
0 siblings, 0 replies; 6+ messages in thread
From: Jeff Layton @ 2023-07-19 19:12 UTC (permalink / raw)
To: Chuck Lever III
Cc: Neil Brown, Olga Kornievskaia, Dai Ngo, Tom Talpey,
Linux NFS Mailing List, linux-fsdevel, open list,
Ondrej Valousek, Andreas Gruenbacher
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>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nfsd: inherit required unset default acls from effective set
2023-07-19 17:49 [PATCH] nfsd: inherit required unset default acls from effective set Jeff Layton
2023-07-19 19:02 ` Chuck Lever III
@ 2023-07-19 21:22 ` Andreas Grünbacher
2023-07-20 8:43 ` Andreas Grünbacher
1 sibling, 1 reply; 6+ messages in thread
From: Andreas Grünbacher @ 2023-07-19 21:22 UTC (permalink / raw)
To: Jeff Layton
Cc: Chuck Lever, Neil Brown, Olga Kornievskaia, Dai Ngo, Tom Talpey,
linux-nfs, linux-fsdevel, linux-kernel, Ondrej Valousek,
Andreas Gruenbacher
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nfsd: inherit required unset default acls from effective set
2023-07-19 21:22 ` Andreas Grünbacher
@ 2023-07-20 8:43 ` Andreas Grünbacher
2023-07-20 10:38 ` Jeff Layton
0 siblings, 1 reply; 6+ messages in thread
From: Andreas Grünbacher @ 2023-07-20 8:43 UTC (permalink / raw)
To: Jeff Layton
Cc: Chuck Lever, Neil Brown, Olga Kornievskaia, Dai Ngo, Tom Talpey,
linux-nfs, linux-fsdevel, linux-kernel, Ondrej Valousek,
Andreas Gruenbacher
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nfsd: inherit required unset default acls from effective set
2023-07-20 8:43 ` Andreas Grünbacher
@ 2023-07-20 10:38 ` Jeff Layton
0 siblings, 0 replies; 6+ messages in thread
From: Jeff Layton @ 2023-07-20 10:38 UTC (permalink / raw)
To: Andreas Grünbacher
Cc: Chuck Lever, Neil Brown, Olga Kornievskaia, Dai Ngo, Tom Talpey,
linux-nfs, linux-fsdevel, linux-kernel, Ondrej Valousek,
Andreas Gruenbacher
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>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-07-20 10:38 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-19 17:49 [PATCH] nfsd: inherit required unset default acls from effective set Jeff Layton
2023-07-19 19:02 ` Chuck Lever III
2023-07-19 19:12 ` Jeff Layton
2023-07-19 21:22 ` Andreas Grünbacher
2023-07-20 8:43 ` Andreas Grünbacher
2023-07-20 10:38 ` Jeff Layton
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).