[PATCH] nfsd: Fix sort_pacl in fs/nfsd/nf4acl.c to actually sort groups

[PATCH] nfsd: Fix sort_pacl in fs/nfsd/nf4acl.c to actually sort groups

[Frank Filz]

We have been doing some extensive testing of Linux support for ACLs on
NFDS v4. We have noticed that the server rejects ACLs where the groups
are out of order, for example, the following ACL is rejected:

A::OWNER@:rwaxtTcCy
A::user101@domain:rwaxtcy
A::GROUP@:rwaxtcy
A:g:group102@domain:rwaxtcy
A:g:group101@domain:rwaxtcy
A::EVERYONE@:rwaxtcy

Examining the server code, I found that after converting an NFS v4 ACL
to POSIX, sort_pacl is called to sort the user ACEs and group ACEs.
Unfortunately, a minor bug causes the group sort to be skipped.

Signed-off-by: Frank Filz <ffilzlnx@us.ibm.com>
---
 fs/nfsd/nfs4acl.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/nfsd/nfs4acl.c b/fs/nfsd/nfs4acl.c
index 725d02f..6d9c6aa 100644
--- a/fs/nfsd/nfs4acl.c
+++ b/fs/nfsd/nfs4acl.c
@@ -389,7 +389,7 @@ sort_pacl(struct posix_acl *pacl)
 	sort_pacl_range(pacl, 1, i-1);
 
 	BUG_ON(pacl->a_entries[i].e_tag != ACL_GROUP_OBJ);
-	j = i++;
+	j = ++i;
 	while (pacl->a_entries[j].e_tag == ACL_GROUP)
 		j++;
 	sort_pacl_range(pacl, i, j-1);
-- 
1.5.2.2

Re: [PATCH] nfsd: Fix sort_pacl in fs/nfsd/nf4acl.c to actually sort groups

[J. Bruce Fields]

On Wed, Oct 21, 2009 at 04:45:02PM -0700, Frank Filz wrote:
> We have been doing some extensive testing of Linux support for ACLs on
> NFDS v4. We have noticed that the server rejects ACLs where the groups
> are out of order, for example, the following ACL is rejected:
> 
> A::OWNER@:rwaxtTcCy
> A::user101@domain:rwaxtcy
> A::GROUP@:rwaxtcy
> A:g:group102@domain:rwaxtcy
> A:g:group101@domain:rwaxtcy
> A::EVERYONE@:rwaxtcy
> 
> Examining the server code, I found that after converting an NFS v4 ACL
> to POSIX, sort_pacl is called to sort the user ACEs and group ACEs.
> Unfortunately, a minor bug causes the group sort to be skipped.

Good grief, I'm embarassed--OK, thanks for catching that!  Do you have
any regression tests that'd be easy for someone else to run?

Applied.

--b.

> 
> Signed-off-by: Frank Filz <ffilzlnx@us.ibm.com>
> ---
>  fs/nfsd/nfs4acl.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4acl.c b/fs/nfsd/nfs4acl.c
> index 725d02f..6d9c6aa 100644
> --- a/fs/nfsd/nfs4acl.c
> +++ b/fs/nfsd/nfs4acl.c
> @@ -389,7 +389,7 @@ sort_pacl(struct posix_acl *pacl)
>  	sort_pacl_range(pacl, 1, i-1);
>  
>  	BUG_ON(pacl->a_entries[i].e_tag != ACL_GROUP_OBJ);
> -	j = i++;
> +	j = ++i;
>  	while (pacl->a_entries[j].e_tag == ACL_GROUP)
>  		j++;
>  	sort_pacl_range(pacl, i, j-1);
> -- 
> 1.5.2.2
> 
> 
> 
> _______________________________________________
> NFSv4 mailing list
> NFSv4@linux-nfs.org
> http://linux-nfs.org/cgi-bin/mailman/listinfo/nfsv4

Re: [PATCH] nfsd: Fix sort_pacl in fs/nfsd/nf4acl.c to actually sort groups

[Frank Filz]

On Tue, 2009-10-27 at 19:32 -0400, J. Bruce Fields wrote:
> On Wed, Oct 21, 2009 at 04:45:02PM -0700, Frank Filz wrote:
> > We have been doing some extensive testing of Linux support for ACLs on
> > NFDS v4. We have noticed that the server rejects ACLs where the groups
> > are out of order, for example, the following ACL is rejected:
> > 
> > A::OWNER@:rwaxtTcCy
> > A::user101@domain:rwaxtcy
> > A::GROUP@:rwaxtcy
> > A:g:group102@domain:rwaxtcy
> > A:g:group101@domain:rwaxtcy
> > A::EVERYONE@:rwaxtcy
> > 
> > Examining the server code, I found that after converting an NFS v4 ACL
> > to POSIX, sort_pacl is called to sort the user ACEs and group ACEs.
> > Unfortunately, a minor bug causes the group sort to be skipped.
> 
> Good grief, I'm embarassed--OK, thanks for catching that!  Do you have
> any regression tests that'd be easy for someone else to run?

Using the above ACL (modified with appropriate groups and the
appropriate domain) with nfs4_setfacl will result in an EINVAL/Invalid
argument.

I'm working on a script to test ACL conversion, I will post it when it
is in a better state for public consumption. It does an exhaustive test
(which in full mode takes a day or two to complete), but I could add a
regression test that just hit various spot conditions (it will also have
to get smart about the ACL re-ordering in order to have an automated
regression test for this error).

We actually hit the bug by using nfs4_setfacl -a, discovering that the
command only succeeded if the index was specified to put the new group
ACE in the correct sorted position.

Thanks

Frank