[PATHC] nfsd: Fix a couple issues with POSIX->NFSv4 ACL conversion

[PATHC] nfsd: Fix a couple issues with POSIX->NFSv4 ACL conversion

[Frank Filz]

1. GROUP@ Allow entry doesn't have NFS4_ACE_IDENTIFIER_GROUP, This
appears to have been introduced by accident as part of commit
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=bec50c47aaf6f1f9247f1860547ab394a0802a4c

2. The group deny entries end up denying tcy even though tcy was just
allowed by the allow entry. This appears to be due to:
	ace->access_mask = mask_from_posix(deny, flags);
instead of:
	ace->access_mask = deny_mask_from_posix(deny, flags);

Frank

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

diff --git a/fs/nfsd/nfs4acl.c b/fs/nfsd/nfs4acl.c
index 54b8b41..ae37ec4 100644
--- a/fs/nfsd/nfs4acl.c
+++ b/fs/nfsd/nfs4acl.c
@@ -295,7 +295,7 @@ _posix_to_nfsv4_one(struct posix_acl *pacl, struct nfs4_acl *acl,
 	group_owner_entry = pa;
 
 	ace->type = NFS4_ACE_ACCESS_ALLOWED_ACE_TYPE;
-	ace->flag = eflag;
+	ace->flag = eflag | NFS4_ACE_IDENTIFIER_GROUP;
 	ace->access_mask = mask_from_posix(pas.group, flags);
 	ace->whotype = NFS4_ACL_WHO_GROUP;
 	ace++;
@@ -335,7 +335,7 @@ _posix_to_nfsv4_one(struct posix_acl *pacl, struct nfs4_acl *acl,
 		if (deny) {
 			ace->type = NFS4_ACE_ACCESS_DENIED_ACE_TYPE;
 			ace->flag = eflag | NFS4_ACE_IDENTIFIER_GROUP;
-			ace->access_mask = mask_from_posix(deny, flags);
+			ace->access_mask = deny_mask_from_posix(deny, flags);
 			ace->whotype = NFS4_ACL_WHO_NAMED;
 			ace->who = pa->e_id;
 			ace++;
-- 
1.5.2.2



_______________________________________________
NFSv4 mailing list
NFSv4@linux-nfs.org
http://linux-nfs.org/cgi-bin/mailman/listinfo/nfsv4

Re: [PATHC] nfsd: Fix a couple issues with POSIX->NFSv4 ACL conversion

[J. Bruce Fields]

On Fri, Aug 14, 2009 at 03:02:30PM -0700, Frank Filz wrote:
> 1. GROUP@ Allow entry doesn't have NFS4_ACE_IDENTIFIER_GROUP, This
> appears to have been introduced by accident as part of commit
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=bec50c47aaf6f1f9247f1860547ab394a0802a4c

It's good to flip that bit every now and then just to keep client
implementations on their toes....

	(Slightly more seriously, the 4.1 draft says "The
	ACE4_IDENTIFIER_GROUP flag MUST be ignored on entries with these
	special identifiers.  When encoding entries with these special
	identifiers, the ACE4_IDENTIFIER_GROUP flag SHOULD be set to
	zero."  It really shouldn't matter either way, but the point is
	that this flag is used to distinguish named users from named
	groups (since unix allows a group to have the same name as a
	user), so it doesn't really make sense to use it on a special
	identifier such as this.)

> 2. The group deny entries end up denying tcy even though tcy was just
> allowed by the allow entry. This appears to be due to:
> 	ace->access_mask = mask_from_posix(deny, flags);
> instead of:
> 	ace->access_mask = deny_mask_from_posix(deny, flags);

Yup, good catch, thanks.  I'll apply the second chunk from this patch
(and adjust the comment appropriately).

--b.

> 
> Frank
> 
> Signed-off-by: Frank Filz <ffilzlnx@us.ibm.com>
> ---
>  fs/nfsd/nfs4acl.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4acl.c b/fs/nfsd/nfs4acl.c
> index 54b8b41..ae37ec4 100644
> --- a/fs/nfsd/nfs4acl.c
> +++ b/fs/nfsd/nfs4acl.c
> @@ -295,7 +295,7 @@ _posix_to_nfsv4_one(struct posix_acl *pacl, struct nfs4_acl *acl,
>  	group_owner_entry = pa;
>  
>  	ace->type = NFS4_ACE_ACCESS_ALLOWED_ACE_TYPE;
> -	ace->flag = eflag;
> +	ace->flag = eflag | NFS4_ACE_IDENTIFIER_GROUP;
>  	ace->access_mask = mask_from_posix(pas.group, flags);
>  	ace->whotype = NFS4_ACL_WHO_GROUP;
>  	ace++;
> @@ -335,7 +335,7 @@ _posix_to_nfsv4_one(struct posix_acl *pacl, struct nfs4_acl *acl,
>  		if (deny) {
>  			ace->type = NFS4_ACE_ACCESS_DENIED_ACE_TYPE;
>  			ace->flag = eflag | NFS4_ACE_IDENTIFIER_GROUP;
> -			ace->access_mask = mask_from_posix(deny, flags);
> +			ace->access_mask = deny_mask_from_posix(deny, flags);
>  			ace->whotype = NFS4_ACL_WHO_NAMED;
>  			ace->who = pa->e_id;
>  			ace++;
> -- 
> 1.5.2.2
> 
> 
> 

Re: [PATHC] nfsd: Fix a couple issues with POSIX->NFSv4 ACL conversion

[Frank Filz]

On Mon, 2009-08-24 at 19:59 -0400, J. Bruce Fields wrote:
> On Fri, Aug 14, 2009 at 03:02:30PM -0700, Frank Filz wrote:
> > 1. GROUP@ Allow entry doesn't have NFS4_ACE_IDENTIFIER_GROUP, This
> > appears to have been introduced by accident as part of commit
> > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=bec50c47aaf6f1f9247f1860547ab394a0802a4c
> 
> It's good to flip that bit every now and then just to keep client
> implementations on their toes....
> 
> 	(Slightly more seriously, the 4.1 draft says "The
> 	ACE4_IDENTIFIER_GROUP flag MUST be ignored on entries with these
> 	special identifiers.  When encoding entries with these special
> 	identifiers, the ACE4_IDENTIFIER_GROUP flag SHOULD be set to
> 	zero."  It really shouldn't matter either way, but the point is
> 	that this flag is used to distinguish named users from named
> 	groups (since unix allows a group to have the same name as a
> 	user), so it doesn't really make sense to use it on a special
> 	identifier such as this.)

Ok, that makes sense, in that case, we probably should have this
fragment to remove the flag from the GROUP@ deny entry:

@@ -321,7 +321,7 @@ _posix_to_nfsv4_one(struct posix_acl *pacl, struct nfs4_acl *acl,
 	deny = ~pas.group & pas.other;
 	if (deny) {
 		ace->type = NFS4_ACE_ACCESS_DENIED_ACE_TYPE;
-		ace->flag = eflag | NFS4_ACE_IDENTIFIER_GROUP;
+		ace->flag = eflag;
 		ace->access_mask = deny_mask_from_posix(deny, flags);
 		ace->whotype = NFS4_ACL_WHO_GROUP;
 		ace++;

Re: [PATHC] nfsd: Fix a couple issues with POSIX->NFSv4 ACL conversion

[J. Bruce Fields]

On Thu, Aug 27, 2009 at 10:40:11AM -0700, Frank Filz wrote:
> On Mon, 2009-08-24 at 19:59 -0400, J. Bruce Fields wrote:
> > On Fri, Aug 14, 2009 at 03:02:30PM -0700, Frank Filz wrote:
> > > 1. GROUP@ Allow entry doesn't have NFS4_ACE_IDENTIFIER_GROUP, This
> > > appears to have been introduced by accident as part of commit
> > > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=bec50c47aaf6f1f9247f1860547ab394a0802a4c
> > 
> > It's good to flip that bit every now and then just to keep client
> > implementations on their toes....
> > 
> > 	(Slightly more seriously, the 4.1 draft says "The
> > 	ACE4_IDENTIFIER_GROUP flag MUST be ignored on entries with these
> > 	special identifiers.  When encoding entries with these special
> > 	identifiers, the ACE4_IDENTIFIER_GROUP flag SHOULD be set to
> > 	zero."  It really shouldn't matter either way, but the point is
> > 	that this flag is used to distinguish named users from named
> > 	groups (since unix allows a group to have the same name as a
> > 	user), so it doesn't really make sense to use it on a special
> > 	identifier such as this.)
> 
> Ok, that makes sense, in that case, we probably should have this
> fragment to remove the flag from the GROUP@ deny entry:

Sure.  Applied.

--b.

> 
> @@ -321,7 +321,7 @@ _posix_to_nfsv4_one(struct posix_acl *pacl, struct nfs4_acl *acl,
>  	deny = ~pas.group & pas.other;
>  	if (deny) {
>  		ace->type = NFS4_ACE_ACCESS_DENIED_ACE_TYPE;
> -		ace->flag = eflag | NFS4_ACE_IDENTIFIER_GROUP;
> +		ace->flag = eflag;
>  		ace->access_mask = deny_mask_from_posix(deny, flags);
>  		ace->whotype = NFS4_ACL_WHO_GROUP;
>  		ace++;
> 
>