[PATCH 000 of 9] knfsd: NFSv4 ACL improvements and a couple of bug fixes.

[PATCH 000 of 9] knfsd: NFSv4 ACL improvements and a couple of bug fixes.

[NeilBrown]

following are 9 patches from Bruce Fields for the NFSv4 server,
mostly ACL related.
Suitable for 2.6.21.

Thanks,
NeilBrown


 [PATCH 001 of 9] knfsd: nfsd4: fix non-terminated string
 [PATCH 002 of 9] knfsd: nfsd4: relax checking of ACL inheritance bits
 [PATCH 003 of 9] knfsd: nfsd4: simplify nfsv4->posix translation
 [PATCH 004 of 9] knfsd: nfsd4: represent nfsv4 acl with array instead of linked list
 [PATCH 005 of 9] knfsd: nfsd4: fix memory leak on kmalloc failure in savemem
 [PATCH 006 of 9] knfsd: nfsd4: fix error return on unsupported acl
 [PATCH 007 of 9] knfsd: nfsd4: acls: don't return explicit mask
 [PATCH 008 of 9] knfsd: nfsd4: acls: avoid unnecessary denies
 [PATCH 009 of 9] knfsd: nfsd4: fix handling of directories without default ACLs

[PATCH 001 of 9] knfsd: nfsd4: fix non-terminated string

[NeilBrown]

From: J. Bruce Fields <bfields@citi.umich.edu>
The server name is expected to be a null-terminated string, so we can't
pass in the raw client identifier.

What's more, the client identifier is just a binary, not necessarily
printable, blob.  Let's just use the ip address instead.  The server
name appears to exist just to help debugging by making some printk's
more informative.

Note that the string is copies into the rpc client structure, so
the pointer to the local variable does not outlive the function call.

Signed-off-by: "J. Bruce Fields" <bfields@citi.umich.edu>
Signed-off-by: Neil Brown <neilb@suse.de>

### Diffstat output
 ./fs/nfsd/nfs4callback.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff .prev/fs/nfsd/nfs4callback.c ./fs/nfsd/nfs4callback.c
--- .prev/fs/nfsd/nfs4callback.c	2007-02-13 09:50:26.000000000 +1100
+++ ./fs/nfsd/nfs4callback.c	2007-02-13 10:00:59.000000000 +1100
@@ -387,7 +387,6 @@ nfsd4_probe_callback(struct nfs4_client 
 		.address	= (struct sockaddr *)&addr,
 		.addrsize	= sizeof(addr),
 		.timeout	= &timeparms,
-		.servername	= clp->cl_name.data,
 		.program	= program,
 		.version	= nfs_cb_version[1]->number,
 		.authflavor	= RPC_AUTH_UNIX,	/* XXX: need AUTH_GSS... */
@@ -397,6 +396,7 @@ nfsd4_probe_callback(struct nfs4_client 
 		.rpc_proc       = &nfs4_cb_procedures[NFSPROC4_CLNT_CB_NULL],
 		.rpc_argp       = clp,
 	};
+	char clientname[16];
 	int status;
 
 	if (atomic_read(&cb->cb_set))
@@ -419,6 +419,11 @@ nfsd4_probe_callback(struct nfs4_client 
 	memset(program->stats, 0, sizeof(cb->cb_stat));
 	program->stats->program = program;
 
+	/* Just here to make some printk's more useful: */
+	snprintf(clientname, sizeof(clientname),
+		"%u.%u.%u.%u", NIPQUAD(addr.sin_addr));
+	args.servername = clientname;
+
 	/* Create RPC client */
 	cb->cb_client = rpc_create(&args);
 	if (IS_ERR(cb->cb_client)) {

Re: [NFS] [PATCH 001 of 9] knfsd: nfsd4: fix non-terminated string

[Ming Zhang]

On Tue, 2007-02-13 at 10:44 +1100, NeilBrown wrote:
> From: J. Bruce Fields <bfields@citi.umich.edu>
> The server name is expected to be a null-terminated string, so we can't
> pass in the raw client identifier.
> 
> What's more, the client identifier is just a binary, not necessarily
> printable, blob.  Let's just use the ip address instead.  The server
> name appears to exist just to help debugging by making some printk's
> more informative.
> 
> Note that the string is copies into the rpc client structure, so
> the pointer to the local variable does not outlive the function call.
> 
> Signed-off-by: "J. Bruce Fields" <bfields@citi.umich.edu>
> Signed-off-by: Neil Brown <neilb@suse.de>
> 
> ### Diffstat output
>  ./fs/nfsd/nfs4callback.c |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff .prev/fs/nfsd/nfs4callback.c ./fs/nfsd/nfs4callback.c
> --- .prev/fs/nfsd/nfs4callback.c	2007-02-13 09:50:26.000000000 +1100
> +++ ./fs/nfsd/nfs4callback.c	2007-02-13 10:00:59.000000000 +1100
> @@ -387,7 +387,6 @@ nfsd4_probe_callback(struct nfs4_client 
>  		.address	= (struct sockaddr *)&addr,
>  		.addrsize	= sizeof(addr),
>  		.timeout	= &timeparms,
> -		.servername	= clp->cl_name.data,
>  		.program	= program,
>  		.version	= nfs_cb_version[1]->number,
>  		.authflavor	= RPC_AUTH_UNIX,	/* XXX: need AUTH_GSS... */
> @@ -397,6 +396,7 @@ nfsd4_probe_callback(struct nfs4_client 
>  		.rpc_proc       = &nfs4_cb_procedures[NFSPROC4_CLNT_CB_NULL],
>  		.rpc_argp       = clp,
>  	};
> +	char clientname[16];
>  	int status;
>  
>  	if (atomic_read(&cb->cb_set))
> @@ -419,6 +419,11 @@ nfsd4_probe_callback(struct nfs4_client 
>  	memset(program->stats, 0, sizeof(cb->cb_stat));
>  	program->stats->program = program;
>  
> +	/* Just here to make some printk's more useful: */
> +	snprintf(clientname, sizeof(clientname),
> +		"%u.%u.%u.%u", NIPQUAD(addr.sin_addr));

can use NIPQUAD_FMT here instead of "%u.%u.%u.%u".

btw, will the ip address here possibly be an ipv6 address?

> +	args.servername = clientname;
> +
>  	/* Create RPC client */
>  	cb->cb_client = rpc_create(&args);
>  	if (IS_ERR(cb->cb_client)) {
> 
> -------------------------------------------------------------------------
> Using Tomcat but need to do more? Need to support web services, security?
> Get stuff done quickly with pre-integrated technology to make your job easier.
> Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
> http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
> _______________________________________________
> NFS maillist  -  NFS@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/nfs
-- 
http://blackmagic02881.wordpress.com/

Re: [NFS] [PATCH 001 of 9] knfsd: nfsd4: fix non-terminated string

[Chuck Lever]

[-- Attachment #1: Type: text/plain, Size: 2213 bytes --]

Ming Zhang wrote:
> On Tue, 2007-02-13 at 10:44 +1100, NeilBrown wrote:
>> From: J. Bruce Fields <bfields@citi.umich.edu>
>> The server name is expected to be a null-terminated string, so we can't
>> pass in the raw client identifier.
>>
>> What's more, the client identifier is just a binary, not necessarily
>> printable, blob.  Let's just use the ip address instead.  The server
>> name appears to exist just to help debugging by making some printk's
>> more informative.
>>
>> Note that the string is copies into the rpc client structure, so
>> the pointer to the local variable does not outlive the function call.
>>
>> Signed-off-by: "J. Bruce Fields" <bfields@citi.umich.edu>
>> Signed-off-by: Neil Brown <neilb@suse.de>
>>
>> ### Diffstat output
>>  ./fs/nfsd/nfs4callback.c |    7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff .prev/fs/nfsd/nfs4callback.c ./fs/nfsd/nfs4callback.c
>> --- .prev/fs/nfsd/nfs4callback.c	2007-02-13 09:50:26.000000000 +1100
>> +++ ./fs/nfsd/nfs4callback.c	2007-02-13 10:00:59.000000000 +1100
>> @@ -387,7 +387,6 @@ nfsd4_probe_callback(struct nfs4_client 
>>  		.address	= (struct sockaddr *)&addr,
>>  		.addrsize	= sizeof(addr),
>>  		.timeout	= &timeparms,
>> -		.servername	= clp->cl_name.data,
>>  		.program	= program,
>>  		.version	= nfs_cb_version[1]->number,
>>  		.authflavor	= RPC_AUTH_UNIX,	/* XXX: need AUTH_GSS... */
>> @@ -397,6 +396,7 @@ nfsd4_probe_callback(struct nfs4_client 
>>  		.rpc_proc       = &nfs4_cb_procedures[NFSPROC4_CLNT_CB_NULL],
>>  		.rpc_argp       = clp,
>>  	};
>> +	char clientname[16];
>>  	int status;
>>  
>>  	if (atomic_read(&cb->cb_set))
>> @@ -419,6 +419,11 @@ nfsd4_probe_callback(struct nfs4_client 
>>  	memset(program->stats, 0, sizeof(cb->cb_stat));
>>  	program->stats->program = program;
>>  
>> +	/* Just here to make some printk's more useful: */
>> +	snprintf(clientname, sizeof(clientname),
>> +		"%u.%u.%u.%u", NIPQUAD(addr.sin_addr));
> 
> can use NIPQUAD_FMT here instead of "%u.%u.%u.%u".
> 
> btw, will the ip address here possibly be an ipv6 address?

Some patches are in the works to build in IPv6 support.  See the patch 
series at http://oss.oracle.com/~cel/linux-2.6/2.6.19/patches/

[-- Attachment #2: chuck.lever.vcf --]
[-- Type: text/x-vcard, Size: 265 bytes --]

begin:vcard
fn:Chuck Lever
n:Lever;Chuck
org:Oracle Corporation;Corporate Architecture Linux Projects Group
email;internet:chuck dot lever at nospam oracle dot com
title:Principal Member of Staff
tel;work:+1 248 614 5091
x-mozilla-html:FALSE
version:2.1
end:vcard

Re: [NFS] [PATCH 001 of 9] knfsd: nfsd4: fix non-terminated string

[Ming Zhang]

On Wed, 2007-02-14 at 09:55 -0800, Chuck Lever wrote:
> Ming Zhang wrote:
> > On Tue, 2007-02-13 at 10:44 +1100, NeilBrown wrote:
> >> From: J. Bruce Fields <bfields@citi.umich.edu>
> >> The server name is expected to be a null-terminated string, so we can't
> >> pass in the raw client identifier.
> >>
> >> What's more, the client identifier is just a binary, not necessarily
> >> printable, blob.  Let's just use the ip address instead.  The server
> >> name appears to exist just to help debugging by making some printk's
> >> more informative.
> >>
> >> Note that the string is copies into the rpc client structure, so
> >> the pointer to the local variable does not outlive the function call.
> >>
> >> Signed-off-by: "J. Bruce Fields" <bfields@citi.umich.edu>
> >> Signed-off-by: Neil Brown <neilb@suse.de>
> >>
> >> ### Diffstat output
> >>  ./fs/nfsd/nfs4callback.c |    7 ++++++-
> >>  1 file changed, 6 insertions(+), 1 deletion(-)
> >>
> >> diff .prev/fs/nfsd/nfs4callback.c ./fs/nfsd/nfs4callback.c
> >> --- .prev/fs/nfsd/nfs4callback.c	2007-02-13 09:50:26.000000000 +1100
> >> +++ ./fs/nfsd/nfs4callback.c	2007-02-13 10:00:59.000000000 +1100
> >> @@ -387,7 +387,6 @@ nfsd4_probe_callback(struct nfs4_client 
> >>  		.address	= (struct sockaddr *)&addr,
> >>  		.addrsize	= sizeof(addr),
> >>  		.timeout	= &timeparms,
> >> -		.servername	= clp->cl_name.data,
> >>  		.program	= program,
> >>  		.version	= nfs_cb_version[1]->number,
> >>  		.authflavor	= RPC_AUTH_UNIX,	/* XXX: need AUTH_GSS... */
> >> @@ -397,6 +396,7 @@ nfsd4_probe_callback(struct nfs4_client 
> >>  		.rpc_proc       = &nfs4_cb_procedures[NFSPROC4_CLNT_CB_NULL],
> >>  		.rpc_argp       = clp,
> >>  	};
> >> +	char clientname[16];
> >>  	int status;
> >>  
> >>  	if (atomic_read(&cb->cb_set))
> >> @@ -419,6 +419,11 @@ nfsd4_probe_callback(struct nfs4_client 
> >>  	memset(program->stats, 0, sizeof(cb->cb_stat));
> >>  	program->stats->program = program;
> >>  
> >> +	/* Just here to make some printk's more useful: */
> >> +	snprintf(clientname, sizeof(clientname),
> >> +		"%u.%u.%u.%u", NIPQUAD(addr.sin_addr));
> > 
> > can use NIPQUAD_FMT here instead of "%u.%u.%u.%u".
> > 
> > btw, will the ip address here possibly be an ipv6 address?
> 
> Some patches are in the works to build in IPv6 support.  See the patch 
> series at http://oss.oracle.com/~cel/linux-2.6/2.6.19/patches/

thanks for the info.

-- 
http://blackmagic02881.wordpress.com/

[PATCH 002 of 9] knfsd: nfsd4: relax checking of ACL inheritance bits

[NeilBrown]

From: J. Bruce Fields <bfields@citi.umich.edu>
The rfc allows us to be more permissive about the ACL inheritance bits we
accept:

	"If the server supports a single "inherit ACE" flag that applies to
	both files and directories, the server may reject the request
	(i.e., requiring the client to set both the file and directory
	inheritance flags). The server may also accept the request and
	silently turn on the ACE4_DIRECTORY_INHERIT_ACE flag."

Let's take the latter option--the ACL is a complex attribute that could be
rejected for a wide variety of reasons, and the protocol gives us little
ability to explain the reason for the rejection, so erroring out is a
user-unfriendly last resort.

Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
Signed-off-by: Neil Brown <neilb@suse.de>

### Diffstat output
 ./fs/nfsd/nfs4acl.c |   23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff .prev/fs/nfsd/nfs4acl.c ./fs/nfsd/nfs4acl.c
--- .prev/fs/nfsd/nfs4acl.c	2007-02-13 09:50:26.000000000 +1100
+++ ./fs/nfsd/nfs4acl.c	2007-02-13 10:01:42.000000000 +1100
@@ -61,9 +61,11 @@
 
 /* flags used to simulate posix default ACLs */
 #define NFS4_INHERITANCE_FLAGS (NFS4_ACE_FILE_INHERIT_ACE \
-		| NFS4_ACE_DIRECTORY_INHERIT_ACE | NFS4_ACE_INHERIT_ONLY_ACE)
+		| NFS4_ACE_DIRECTORY_INHERIT_ACE)
 
-#define NFS4_SUPPORTED_FLAGS (NFS4_INHERITANCE_FLAGS | NFS4_ACE_IDENTIFIER_GROUP)
+#define NFS4_SUPPORTED_FLAGS (NFS4_INHERITANCE_FLAGS \
+		| NFS4_ACE_INHERIT_ONLY_ACE \
+		| NFS4_ACE_IDENTIFIER_GROUP)
 
 #define MASK_EQUAL(mask1, mask2) \
 	( ((mask1) & NFS4_ACE_MASK_ALL) == ((mask2) & NFS4_ACE_MASK_ALL) )
@@ -707,11 +709,16 @@ nfs4_acl_split(struct nfs4_acl *acl, str
 		if (ace->flag & ~NFS4_SUPPORTED_FLAGS)
 			return -EINVAL;
 
-		switch (ace->flag & NFS4_INHERITANCE_FLAGS) {
-		case 0:
+		if ((ace->flag & NFS4_INHERITANCE_FLAGS) == 0) {
 			/* Leave this ace in the effective acl: */
 			continue;
-		case NFS4_INHERITANCE_FLAGS:
+		}
+		/*
+		 * Note that when only one of FILE_INHERIT or DIRECTORY_INHERIT
+		 * is set, we're effectively turning on the other.  That's OK,
+		 * according to rfc 3530.
+		 */
+		if (ace->flag & NFS4_ACE_INHERIT_ONLY_ACE) {
 			/* Add this ace to the default acl and remove it
 			 * from the effective acl: */
 			error = nfs4_acl_add_ace(dacl, ace->type, ace->flag,
@@ -721,17 +728,13 @@ nfs4_acl_split(struct nfs4_acl *acl, str
 			list_del(h);
 			kfree(ace);
 			acl->naces--;
-			break;
-		case NFS4_INHERITANCE_FLAGS & ~NFS4_ACE_INHERIT_ONLY_ACE:
+		} else {
 			/* Add this ace to the default, but leave it in
 			 * the effective acl as well: */
 			error = nfs4_acl_add_ace(dacl, ace->type, ace->flag,
 				ace->access_mask, ace->whotype, ace->who);
 			if (error)
 				return error;
-			break;
-		default:
-			return -EINVAL;
 		}
 	}
 	return 0;

[PATCH 003 of 9] knfsd: nfsd4: simplify nfsv4->posix translation

[NeilBrown]

From: J. Bruce Fields <bfields@citi.umich.edu>
The code that splits an incoming nfsv4 ACL into inheritable and effective parts
can be combined with the the code that translates each to a posix acl,
resulting in simpler code that requires one less pass through the ACL.

Signed-off-by: "J. Bruce Fields" <bfields@citi.umich.edu>
Signed-off-by: Neil Brown <neilb@suse.de>

### Diffstat output
 ./fs/nfsd/nfs4acl.c |  133 ++++++++++++++--------------------------------------
 1 file changed, 38 insertions(+), 95 deletions(-)

diff .prev/fs/nfsd/nfs4acl.c ./fs/nfsd/nfs4acl.c
--- .prev/fs/nfsd/nfs4acl.c	2007-02-13 10:01:42.000000000 +1100
+++ ./fs/nfsd/nfs4acl.c	2007-02-13 10:04:17.000000000 +1100
@@ -129,9 +129,7 @@ struct ace_container {
 
 static short ace2type(struct nfs4_ace *);
 static int _posix_to_nfsv4_one(struct posix_acl *, struct nfs4_acl *, unsigned int);
-static struct posix_acl *_nfsv4_to_posix_one(struct nfs4_acl *, unsigned int);
 int nfs4_acl_add_ace(struct nfs4_acl *, u32, u32, u32, int, uid_t);
-static int nfs4_acl_split(struct nfs4_acl *, struct nfs4_acl *);
 
 struct nfs4_acl *
 nfs4_acl_posix_to_nfsv4(struct posix_acl *pacl, struct posix_acl *dpacl,
@@ -344,46 +342,6 @@ sort_pacl(struct posix_acl *pacl)
 	return;
 }
 
-int
-nfs4_acl_nfsv4_to_posix(struct nfs4_acl *acl, struct posix_acl **pacl,
-		struct posix_acl **dpacl, unsigned int flags)
-{
-	struct nfs4_acl *dacl;
-	int error = -ENOMEM;
-
-	*pacl = NULL;
-	*dpacl = NULL;
-
-	dacl = nfs4_acl_new();
-	if (dacl == NULL)
-		goto out;
-
-	error = nfs4_acl_split(acl, dacl);
-	if (error)
-		goto out_acl;
-
-	*pacl = _nfsv4_to_posix_one(acl, flags);
-	if (IS_ERR(*pacl)) {
-		error = PTR_ERR(*pacl);
-		*pacl = NULL;
-		goto out_acl;
-	}
-
-	*dpacl = _nfsv4_to_posix_one(dacl, flags);
-	if (IS_ERR(*dpacl)) {
-		error = PTR_ERR(*dpacl);
-		*dpacl = NULL;
-	}
-out_acl:
-	if (error) {
-		posix_acl_release(*pacl);
-		*pacl = NULL;
-	}
-	nfs4_acl_free(dacl);
-out:
-	return error;
-}
-
 /*
  * While processing the NFSv4 ACE, this maintains bitmasks representing
  * which permission bits have been allowed and which denied to a given
@@ -668,76 +626,61 @@ static void process_one_v4_ace(struct po
 	}
 }
 
-static struct posix_acl *
-_nfsv4_to_posix_one(struct nfs4_acl *n4acl, unsigned int flags)
+int nfs4_acl_nfsv4_to_posix(struct nfs4_acl *acl, struct posix_acl **pacl,
+			    struct posix_acl **dpacl, unsigned int flags)
 {
-	struct posix_acl_state state;
-	struct posix_acl *pacl;
+	struct posix_acl_state effective_acl_state, default_acl_state;
 	struct nfs4_ace *ace;
 	int ret;
 
-	ret = init_state(&state, n4acl->naces);
+	ret = init_state(&effective_acl_state, acl->naces);
 	if (ret)
-		return ERR_PTR(ret);
-
-	list_for_each_entry(ace, &n4acl->ace_head, l_ace)
-		process_one_v4_ace(&state, ace);
-
-	pacl = posix_state_to_acl(&state, flags);
-
-	free_state(&state);
-
-	if (!IS_ERR(pacl))
-		sort_pacl(pacl);
-	return pacl;
-}
-
-static int
-nfs4_acl_split(struct nfs4_acl *acl, struct nfs4_acl *dacl)
-{
-	struct list_head *h, *n;
-	struct nfs4_ace *ace;
-	int error = 0;
-
-	list_for_each_safe(h, n, &acl->ace_head) {
-		ace = list_entry(h, struct nfs4_ace, l_ace);
-
+		return ret;
+	ret = init_state(&default_acl_state, acl->naces);
+	if (ret)
+		goto out_estate;
+	ret = -EINVAL;
+	list_for_each_entry(ace, &acl->ace_head, l_ace) {
 		if (ace->type != NFS4_ACE_ACCESS_ALLOWED_ACE_TYPE &&
 		    ace->type != NFS4_ACE_ACCESS_DENIED_ACE_TYPE)
-			return -EINVAL;
-
+			goto out_dstate;
 		if (ace->flag & ~NFS4_SUPPORTED_FLAGS)
-			return -EINVAL;
-
+			goto out_dstate;
 		if ((ace->flag & NFS4_INHERITANCE_FLAGS) == 0) {
-			/* Leave this ace in the effective acl: */
+			process_one_v4_ace(&effective_acl_state, ace);
 			continue;
 		}
+		if (!(flags & NFS4_ACL_DIR))
+			goto out_dstate;
 		/*
 		 * Note that when only one of FILE_INHERIT or DIRECTORY_INHERIT
 		 * is set, we're effectively turning on the other.  That's OK,
 		 * according to rfc 3530.
 		 */
-		if (ace->flag & NFS4_ACE_INHERIT_ONLY_ACE) {
-			/* Add this ace to the default acl and remove it
-			 * from the effective acl: */
-			error = nfs4_acl_add_ace(dacl, ace->type, ace->flag,
-				ace->access_mask, ace->whotype, ace->who);
-			if (error)
-				return error;
-			list_del(h);
-			kfree(ace);
-			acl->naces--;
-		} else {
-			/* Add this ace to the default, but leave it in
-			 * the effective acl as well: */
-			error = nfs4_acl_add_ace(dacl, ace->type, ace->flag,
-				ace->access_mask, ace->whotype, ace->who);
-			if (error)
-				return error;
-		}
+		process_one_v4_ace(&default_acl_state, ace);
+
+		if (!(ace->flag & NFS4_ACE_INHERIT_ONLY_ACE))
+			process_one_v4_ace(&effective_acl_state, ace);
 	}
-	return 0;
+	*pacl = posix_state_to_acl(&effective_acl_state, flags);
+	if (IS_ERR(*pacl)) {
+		ret = PTR_ERR(*pacl);
+		goto out_dstate;
+	}
+	*dpacl = posix_state_to_acl(&default_acl_state, flags);
+	if (IS_ERR(*dpacl)) {
+		ret = PTR_ERR(*dpacl);
+		posix_acl_release(*pacl);
+		goto out_dstate;
+	}
+	sort_pacl(*pacl);
+	sort_pacl(*dpacl);
+	ret = 0;
+out_dstate:
+	free_state(&default_acl_state);
+out_estate:
+	free_state(&effective_acl_state);
+	return ret;
 }
 
 static short

[PATCH 004 of 9] knfsd: nfsd4: represent nfsv4 acl with array instead of linked list

[NeilBrown]

From: J. Bruce Fields <bfields@citi.umich.edu>
Simplify the memory management and code a bit by representing acls with an
array instead of a linked list.

Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
Signed-off-by: Neil Brown <neilb@suse.de>

### Diffstat output
 ./fs/nfsd/nfs4acl.c        |  153 ++++++++++++---------------------------------
 ./fs/nfsd/nfs4xdr.c        |   43 +++++-------
 ./include/linux/nfs4.h     |    3 
 ./include/linux/nfs4_acl.h |    9 +-
 4 files changed, 69 insertions(+), 139 deletions(-)

diff .prev/fs/nfsd/nfs4acl.c ./fs/nfsd/nfs4acl.c
--- .prev/fs/nfsd/nfs4acl.c	2007-02-13 10:04:17.000000000 +1100
+++ ./fs/nfsd/nfs4acl.c	2007-02-13 10:23:49.000000000 +1100
@@ -128,74 +128,58 @@ struct ace_container {
 };
 
 static short ace2type(struct nfs4_ace *);
-static int _posix_to_nfsv4_one(struct posix_acl *, struct nfs4_acl *, unsigned int);
-int nfs4_acl_add_ace(struct nfs4_acl *, u32, u32, u32, int, uid_t);
+static void _posix_to_nfsv4_one(struct posix_acl *, struct nfs4_acl *,
+				unsigned int);
+void nfs4_acl_add_ace(struct nfs4_acl *, u32, u32, u32, int, uid_t);
 
 struct nfs4_acl *
 nfs4_acl_posix_to_nfsv4(struct posix_acl *pacl, struct posix_acl *dpacl,
 			unsigned int flags)
 {
 	struct nfs4_acl *acl;
-	int error = -EINVAL;
+	int size = 0;
 
-	if ((pacl != NULL &&
-		(posix_acl_valid(pacl) < 0 || pacl->a_count == 0)) ||
-	    (dpacl != NULL &&
-		(posix_acl_valid(dpacl) < 0 || dpacl->a_count == 0)))
-		goto out_err;
-
-	acl = nfs4_acl_new();
-	if (acl == NULL) {
-		error = -ENOMEM;
-		goto out_err;
-	}
+	if (pacl) {
+		if (posix_acl_valid(pacl) < 0)
+			return ERR_PTR(-EINVAL);
+		size += 2*pacl->a_count;
+	}
+	if (dpacl) {
+		if (posix_acl_valid(dpacl) < 0)
+			return ERR_PTR(-EINVAL);
+		size += 2*dpacl->a_count;
+	}
+
+	/* Allocate for worst case: one (deny, allow) pair each: */
+	acl = nfs4_acl_new(size);
+	if (acl == NULL)
+		return ERR_PTR(-ENOMEM);
 
-	if (pacl != NULL) {
-		error = _posix_to_nfsv4_one(pacl, acl,
-						flags & ~NFS4_ACL_TYPE_DEFAULT);
-		if (error < 0)
-			goto out_acl;
-	}
+	if (pacl)
+		_posix_to_nfsv4_one(pacl, acl, flags & ~NFS4_ACL_TYPE_DEFAULT);
 
-	if (dpacl != NULL) {
-		error = _posix_to_nfsv4_one(dpacl, acl,
-						flags | NFS4_ACL_TYPE_DEFAULT);
-		if (error < 0)
-			goto out_acl;
-	}
-
-	return acl;
-
-out_acl:
-	nfs4_acl_free(acl);
-out_err:
-	acl = ERR_PTR(error);
+	if (dpacl)
+		_posix_to_nfsv4_one(dpacl, acl, flags | NFS4_ACL_TYPE_DEFAULT);
 
 	return acl;
 }
 
-static int
+static void
 nfs4_acl_add_pair(struct nfs4_acl *acl, int eflag, u32 mask, int whotype,
 		uid_t owner, unsigned int flags)
 {
-	int error;
-
-	error = nfs4_acl_add_ace(acl, NFS4_ACE_ACCESS_ALLOWED_ACE_TYPE,
+	nfs4_acl_add_ace(acl, NFS4_ACE_ACCESS_ALLOWED_ACE_TYPE,
 				 eflag, mask, whotype, owner);
-	if (error < 0)
-		return error;
-	error = nfs4_acl_add_ace(acl, NFS4_ACE_ACCESS_DENIED_ACE_TYPE,
+	nfs4_acl_add_ace(acl, NFS4_ACE_ACCESS_DENIED_ACE_TYPE,
 				eflag, deny_mask(mask, flags), whotype, owner);
-	return error;
 }
 
 /* We assume the acl has been verified with posix_acl_valid. */
-static int
+static void
 _posix_to_nfsv4_one(struct posix_acl *pacl, struct nfs4_acl *acl,
 						unsigned int flags)
 {
 	struct posix_acl_entry *pa, *pe, *group_owner_entry;
-	int error = -EINVAL;
 	u32 mask, mask_mask;
 	int eflag = ((flags & NFS4_ACL_TYPE_DEFAULT) ?
 					NFS4_INHERITANCE_FLAGS : 0);
@@ -211,23 +195,16 @@ _posix_to_nfsv4_one(struct posix_acl *pa
 	pa = pacl->a_entries;
 	BUG_ON(pa->e_tag != ACL_USER_OBJ);
 	mask = mask_from_posix(pa->e_perm, flags | NFS4_ACL_OWNER);
-	error = nfs4_acl_add_pair(acl, eflag, mask, NFS4_ACL_WHO_OWNER, 0, flags);
-	if (error < 0)
-		goto out;
+	nfs4_acl_add_pair(acl, eflag, mask, NFS4_ACL_WHO_OWNER, 0, flags);
 	pa++;
 
 	while (pa->e_tag == ACL_USER) {
 		mask = mask_from_posix(pa->e_perm, flags);
-		error = nfs4_acl_add_ace(acl, NFS4_ACE_ACCESS_DENIED_ACE_TYPE,
+		nfs4_acl_add_ace(acl, NFS4_ACE_ACCESS_DENIED_ACE_TYPE,
 				eflag,  mask_mask, NFS4_ACL_WHO_NAMED, pa->e_id);
-		if (error < 0)
-			goto out;
-
 
-		error = nfs4_acl_add_pair(acl, eflag, mask,
+		nfs4_acl_add_pair(acl, eflag, mask,
 				NFS4_ACL_WHO_NAMED, pa->e_id, flags);
-		if (error < 0)
-			goto out;
 		pa++;
 	}
 
@@ -238,34 +215,25 @@ _posix_to_nfsv4_one(struct posix_acl *pa
 
 	if (pacl->a_count > 3) {
 		BUG_ON(pa->e_tag != ACL_GROUP_OBJ);
-		error = nfs4_acl_add_ace(acl, NFS4_ACE_ACCESS_DENIED_ACE_TYPE,
+		nfs4_acl_add_ace(acl, NFS4_ACE_ACCESS_DENIED_ACE_TYPE,
 				NFS4_ACE_IDENTIFIER_GROUP | eflag, mask_mask,
 				NFS4_ACL_WHO_GROUP, 0);
-		if (error < 0)
-			goto out;
 	}
 	group_owner_entry = pa;
 	mask = mask_from_posix(pa->e_perm, flags);
-	error = nfs4_acl_add_ace(acl, NFS4_ACE_ACCESS_ALLOWED_ACE_TYPE,
+	nfs4_acl_add_ace(acl, NFS4_ACE_ACCESS_ALLOWED_ACE_TYPE,
 			NFS4_ACE_IDENTIFIER_GROUP | eflag, mask,
 			NFS4_ACL_WHO_GROUP, 0);
-	if (error < 0)
-		goto out;
 	pa++;
 
 	while (pa->e_tag == ACL_GROUP) {
 		mask = mask_from_posix(pa->e_perm, flags);
-		error = nfs4_acl_add_ace(acl, NFS4_ACE_ACCESS_DENIED_ACE_TYPE,
+		nfs4_acl_add_ace(acl, NFS4_ACE_ACCESS_DENIED_ACE_TYPE,
 				NFS4_ACE_IDENTIFIER_GROUP | eflag, mask_mask,
 				NFS4_ACL_WHO_NAMED, pa->e_id);
-		if (error < 0)
-			goto out;
-
-		error = nfs4_acl_add_ace(acl, NFS4_ACE_ACCESS_ALLOWED_ACE_TYPE,
+		nfs4_acl_add_ace(acl, NFS4_ACE_ACCESS_ALLOWED_ACE_TYPE,
 		    		NFS4_ACE_IDENTIFIER_GROUP | eflag, mask,
 		    		NFS4_ACL_WHO_NAMED, pa->e_id);
-		if (error < 0)
-			goto out;
 		pa++;
 	}
 
@@ -273,19 +241,15 @@ _posix_to_nfsv4_one(struct posix_acl *pa
 
 	pa = group_owner_entry;
 	mask = mask_from_posix(pa->e_perm, flags);
-	error = nfs4_acl_add_ace(acl, NFS4_ACE_ACCESS_DENIED_ACE_TYPE,
+	nfs4_acl_add_ace(acl, NFS4_ACE_ACCESS_DENIED_ACE_TYPE,
 			NFS4_ACE_IDENTIFIER_GROUP | eflag,
 			deny_mask(mask, flags), NFS4_ACL_WHO_GROUP, 0);
-	if (error < 0)
-		goto out;
 	pa++;
 	while (pa->e_tag == ACL_GROUP) {
 		mask = mask_from_posix(pa->e_perm, flags);
-		error = nfs4_acl_add_ace(acl, NFS4_ACE_ACCESS_DENIED_ACE_TYPE,
+		nfs4_acl_add_ace(acl, NFS4_ACE_ACCESS_DENIED_ACE_TYPE,
 		    		NFS4_ACE_IDENTIFIER_GROUP | eflag,
 		    		deny_mask(mask, flags), NFS4_ACL_WHO_NAMED, pa->e_id);
-		if (error < 0)
-			goto out;
 		pa++;
 	}
 
@@ -293,10 +257,7 @@ _posix_to_nfsv4_one(struct posix_acl *pa
 		pa++;
 	BUG_ON(pa->e_tag != ACL_OTHER);
 	mask = mask_from_posix(pa->e_perm, flags);
-	error = nfs4_acl_add_pair(acl, eflag, mask, NFS4_ACL_WHO_EVERYONE, 0, flags);
-
-out:
-	return error;
+	nfs4_acl_add_pair(acl, eflag, mask, NFS4_ACL_WHO_EVERYONE, 0, flags);
 }
 
 static void
@@ -640,7 +601,7 @@ int nfs4_acl_nfsv4_to_posix(struct nfs4_
 	if (ret)
 		goto out_estate;
 	ret = -EINVAL;
-	list_for_each_entry(ace, &acl->ace_head, l_ace) {
+	for (ace = acl->aces; ace < acl->aces + acl->naces; ace++) {
 		if (ace->type != NFS4_ACE_ACCESS_ALLOWED_ACE_TYPE &&
 		    ace->type != NFS4_ACE_ACCESS_DENIED_ACE_TYPE)
 			goto out_dstate;
@@ -705,48 +666,22 @@ EXPORT_SYMBOL(nfs4_acl_posix_to_nfsv4);
 EXPORT_SYMBOL(nfs4_acl_nfsv4_to_posix);
 
 struct nfs4_acl *
-nfs4_acl_new(void)
+nfs4_acl_new(int n)
 {
 	struct nfs4_acl *acl;
 
-	if ((acl = kmalloc(sizeof(*acl), GFP_KERNEL)) == NULL)
+	acl = kmalloc(sizeof(*acl) + n*sizeof(struct nfs4_ace), GFP_KERNEL);
+	if (acl == NULL)
 		return NULL;
-
 	acl->naces = 0;
-	INIT_LIST_HEAD(&acl->ace_head);
-
 	return acl;
 }
 
 void
-nfs4_acl_free(struct nfs4_acl *acl)
-{
-	struct list_head *h;
-	struct nfs4_ace *ace;
-
-	if (!acl)
-		return;
-
-	while (!list_empty(&acl->ace_head)) {
-		h = acl->ace_head.next;
-		list_del(h);
-		ace = list_entry(h, struct nfs4_ace, l_ace);
-		kfree(ace);
-	}
-
-	kfree(acl);
-
-	return;
-}
-
-int
 nfs4_acl_add_ace(struct nfs4_acl *acl, u32 type, u32 flag, u32 access_mask,
 		int whotype, uid_t who)
 {
-	struct nfs4_ace *ace;
-
-	if ((ace = kmalloc(sizeof(*ace), GFP_KERNEL)) == NULL)
-		return -ENOMEM;
+	struct nfs4_ace *ace = acl->aces + acl->naces;
 
 	ace->type = type;
 	ace->flag = flag;
@@ -754,10 +689,7 @@ nfs4_acl_add_ace(struct nfs4_acl *acl, u
 	ace->whotype = whotype;
 	ace->who = who;
 
-	list_add_tail(&ace->l_ace, &acl->ace_head);
 	acl->naces++;
-
-	return 0;
 }
 
 static struct {
@@ -811,7 +743,6 @@ nfs4_acl_write_who(int who, char *p)
 }
 
 EXPORT_SYMBOL(nfs4_acl_new);
-EXPORT_SYMBOL(nfs4_acl_free);
 EXPORT_SYMBOL(nfs4_acl_add_ace);
 EXPORT_SYMBOL(nfs4_acl_get_whotype);
 EXPORT_SYMBOL(nfs4_acl_write_who);

diff .prev/fs/nfsd/nfs4xdr.c ./fs/nfsd/nfs4xdr.c
--- .prev/fs/nfsd/nfs4xdr.c	2007-02-13 09:50:25.000000000 +1100
+++ ./fs/nfsd/nfs4xdr.c	2007-02-13 10:23:49.000000000 +1100
@@ -273,42 +273,42 @@ nfsd4_decode_fattr(struct nfsd4_compound
 		iattr->ia_valid |= ATTR_SIZE;
 	}
 	if (bmval[0] & FATTR4_WORD0_ACL) {
-		int nace, i;
-		struct nfs4_ace ace;
+		int nace;
+		struct nfs4_ace *ace;
 
 		READ_BUF(4); len += 4;
 		READ32(nace);
 
-		*acl = nfs4_acl_new();
+		if (nace > NFS4_ACL_MAX)
+			return nfserr_resource;
+
+		*acl = nfs4_acl_new(nace);
 		if (*acl == NULL) {
 			host_err = -ENOMEM;
 			goto out_nfserr;
 		}
-		defer_free(argp, (void (*)(const void *))nfs4_acl_free, *acl);
+		defer_free(argp, kfree, *acl);
 
-		for (i = 0; i < nace; i++) {
+		(*acl)->naces = nace;
+		for (ace = (*acl)->aces; ace < (*acl)->aces + nace; ace++) {
 			READ_BUF(16); len += 16;
-			READ32(ace.type);
-			READ32(ace.flag);
-			READ32(ace.access_mask);
+			READ32(ace->type);
+			READ32(ace->flag);
+			READ32(ace->access_mask);
 			READ32(dummy32);
 			READ_BUF(dummy32);
 			len += XDR_QUADLEN(dummy32) << 2;
 			READMEM(buf, dummy32);
-			ace.whotype = nfs4_acl_get_whotype(buf, dummy32);
+			ace->whotype = nfs4_acl_get_whotype(buf, dummy32);
 			host_err = 0;
-			if (ace.whotype != NFS4_ACL_WHO_NAMED)
-				ace.who = 0;
-			else if (ace.flag & NFS4_ACE_IDENTIFIER_GROUP)
+			if (ace->whotype != NFS4_ACL_WHO_NAMED)
+				ace->who = 0;
+			else if (ace->flag & NFS4_ACE_IDENTIFIER_GROUP)
 				host_err = nfsd_map_name_to_gid(argp->rqstp,
-						buf, dummy32, &ace.who);
+						buf, dummy32, &ace->who);
 			else
 				host_err = nfsd_map_name_to_uid(argp->rqstp,
-						buf, dummy32, &ace.who);
-			if (host_err)
-				goto out_nfserr;
-			host_err = nfs4_acl_add_ace(*acl, ace.type, ace.flag,
-				 ace.access_mask, ace.whotype, ace.who);
+						buf, dummy32, &ace->who);
 			if (host_err)
 				goto out_nfserr;
 		}
@@ -1596,7 +1596,6 @@ nfsd4_encode_fattr(struct svc_fh *fhp, s
 	}
 	if (bmval0 & FATTR4_WORD0_ACL) {
 		struct nfs4_ace *ace;
-		struct list_head *h;
 
 		if (acl == NULL) {
 			if ((buflen -= 4) < 0)
@@ -1609,9 +1608,7 @@ nfsd4_encode_fattr(struct svc_fh *fhp, s
 			goto out_resource;
 		WRITE32(acl->naces);
 
-		list_for_each(h, &acl->ace_head) {
-			ace = list_entry(h, struct nfs4_ace, l_ace);
-
+		for (ace = acl->aces; ace < acl->aces + acl->naces; ace++) {
 			if ((buflen -= 4*3) < 0)
 				goto out_resource;
 			WRITE32(ace->type);
@@ -1821,7 +1818,7 @@ out_acl:
 	status = nfs_ok;
 
 out:
-	nfs4_acl_free(acl);
+	kfree(acl);
 	if (fhp == &tempfh)
 		fh_put(&tempfh);
 	return status;

diff .prev/include/linux/nfs4_acl.h ./include/linux/nfs4_acl.h
--- .prev/include/linux/nfs4_acl.h	2007-02-13 09:50:25.000000000 +1100
+++ ./include/linux/nfs4_acl.h	2007-02-13 10:23:49.000000000 +1100
@@ -39,9 +39,12 @@
 
 #include <linux/posix_acl.h>
 
-struct nfs4_acl *nfs4_acl_new(void);
-void nfs4_acl_free(struct nfs4_acl *);
-int nfs4_acl_add_ace(struct nfs4_acl *, u32, u32, u32, int, uid_t);
+/* Maximum ACL we'll accept from client; chosen (somewhat arbitrarily) to
+ * fit in a page: */
+#define NFS4_ACL_MAX 170
+
+struct nfs4_acl *nfs4_acl_new(int);
+void nfs4_acl_add_ace(struct nfs4_acl *, u32, u32, u32, int, uid_t);
 int nfs4_acl_get_whotype(char *, u32);
 int nfs4_acl_write_who(int who, char *p);
 int nfs4_acl_permission(struct nfs4_acl *acl, uid_t owner, gid_t group,

diff .prev/include/linux/nfs4.h ./include/linux/nfs4.h
--- .prev/include/linux/nfs4.h	2007-02-13 09:50:25.000000000 +1100
+++ ./include/linux/nfs4.h	2007-02-13 10:23:49.000000000 +1100
@@ -105,12 +105,11 @@ struct nfs4_ace {
 	uint32_t	access_mask;
 	int		whotype;
 	uid_t		who;
-	struct list_head l_ace;
 };
 
 struct nfs4_acl {
 	uint32_t	naces;
-	struct list_head ace_head;
+	struct nfs4_ace	aces[0];
 };
 
 typedef struct { char data[NFS4_VERIFIER_SIZE]; } nfs4_verifier;

[PATCH 005 of 9] knfsd: nfsd4: fix memory leak on kmalloc failure in savemem

[NeilBrown]

From: J. Bruce Fields <bfields@snoopy.citi.umich.edu>
The wrong pointer is being kfree'd in savemem() when defer_free
returns with an error.

Signed-off-by: Benny Halevy <bhalevy@panasas.com>
Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
Signed-off-by: Neil Brown <neilb@suse.de>

### Diffstat output
 ./fs/nfsd/nfs4xdr.c |    8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff .prev/fs/nfsd/nfs4xdr.c ./fs/nfsd/nfs4xdr.c
--- .prev/fs/nfsd/nfs4xdr.c	2007-02-13 10:23:49.000000000 +1100
+++ ./fs/nfsd/nfs4xdr.c	2007-02-13 10:36:40.000000000 +1100
@@ -199,18 +199,16 @@ defer_free(struct nfsd4_compoundargs *ar
 
 static char *savemem(struct nfsd4_compoundargs *argp, __be32 *p, int nbytes)
 {
-	void *new = NULL;
 	if (p == argp->tmp) {
-		new = kmalloc(nbytes, GFP_KERNEL);
-		if (!new) return NULL;
-		p = new;
+		p = kmalloc(nbytes, GFP_KERNEL);
+		if (!p) return NULL;
 		memcpy(p, argp->tmp, nbytes);
 	} else {
 		BUG_ON(p != argp->tmpp);
 		argp->tmpp = NULL;
 	}
 	if (defer_free(argp, kfree, p)) {
-		kfree(new);
+		kfree(p);
 		return NULL;
 	} else
 		return (char *)p;

[PATCH 006 of 9] knfsd: nfsd4: fix error return on unsupported acl

[NeilBrown]

From: J. Bruce Fields <bfields@citi.umich.edu>
We should be returning ATTRNOTSUPP, not NOTSUPP, when acls are unsupported.

Also fix a comment.

Signed-off-by: "J. Bruce Fields" <bfields@citi.umich.edu>
Signed-off-by: Neil Brown <neilb@suse.de>

### Diffstat output
 ./fs/nfsd/nfs4xdr.c |    2 +-
 ./fs/nfsd/vfs.c     |    5 ++++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff .prev/fs/nfsd/nfs4xdr.c ./fs/nfsd/nfs4xdr.c
--- .prev/fs/nfsd/nfs4xdr.c	2007-02-13 10:36:40.000000000 +1100
+++ ./fs/nfsd/nfs4xdr.c	2007-02-13 10:37:00.000000000 +1100
@@ -253,7 +253,7 @@ nfsd4_decode_fattr(struct nfsd4_compound
 		return status;
 
 	/*
-	 * According to spec, unsupported attributes return ERR_NOTSUPP;
+	 * According to spec, unsupported attributes return ERR_ATTRNOTSUPP;
 	 * read-only attributes return ERR_INVAL.
 	 */
 	if ((bmval[0] & ~NFSD_SUPPORTED_ATTRS_WORD0) || (bmval[1] & ~NFSD_SUPPORTED_ATTRS_WORD1))

diff .prev/fs/nfsd/vfs.c ./fs/nfsd/vfs.c
--- .prev/fs/nfsd/vfs.c	2007-02-13 09:50:24.000000000 +1100
+++ ./fs/nfsd/vfs.c	2007-02-13 10:37:00.000000000 +1100
@@ -466,7 +466,10 @@ out:
 	posix_acl_release(dpacl);
 	return (error);
 out_nfserr:
-	error = nfserrno(host_error);
+	if (host_error == -EOPNOTSUPP)
+		error = nfserr_attrnotsupp;
+	else
+		error = nfserrno(host_error);
 	goto out;
 }
 

[PATCH 007 of 9] knfsd: nfsd4: acls: don't return explicit mask

[NeilBrown]

From: J. Bruce Fields <bfields@snoopy.citi.umich.edu>
Return just the effective permissions, and forget about the mask.  It isn't
worth the complexity.

WARNING: This breaks backwards compatibility with overly-picky nfsv4->posix acl
translation, as may has been included in some patched versions of libacl.  To
our knowledge no such version was every distributed by anyone outside citi.

Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
Signed-off-by: Neil Brown <neilb@suse.de>

### Diffstat output
 ./fs/nfsd/nfs4acl.c |   25 +++++++------------------
 1 file changed, 7 insertions(+), 18 deletions(-)

diff .prev/fs/nfsd/nfs4acl.c ./fs/nfsd/nfs4acl.c
--- .prev/fs/nfsd/nfs4acl.c	2007-02-13 10:23:49.000000000 +1100
+++ ./fs/nfsd/nfs4acl.c	2007-02-13 10:37:27.000000000 +1100
@@ -180,7 +180,8 @@ _posix_to_nfsv4_one(struct posix_acl *pa
 						unsigned int flags)
 {
 	struct posix_acl_entry *pa, *pe, *group_owner_entry;
-	u32 mask, mask_mask;
+	u32 mask;
+	unsigned short mask_mask;
 	int eflag = ((flags & NFS4_ACL_TYPE_DEFAULT) ?
 					NFS4_INHERITANCE_FLAGS : 0);
 
@@ -188,9 +189,9 @@ _posix_to_nfsv4_one(struct posix_acl *pa
 	pe = pacl->a_entries + pacl->a_count;
 	pa = pe - 2; /* if mask entry exists, it's second from the last. */
 	if (pa->e_tag == ACL_MASK)
-		mask_mask = deny_mask(mask_from_posix(pa->e_perm, flags), flags);
+		mask_mask = pa->e_perm;
 	else
-		mask_mask = 0;
+		mask_mask = S_IRWXO;
 
 	pa = pacl->a_entries;
 	BUG_ON(pa->e_tag != ACL_USER_OBJ);
@@ -199,10 +200,7 @@ _posix_to_nfsv4_one(struct posix_acl *pa
 	pa++;
 
 	while (pa->e_tag == ACL_USER) {
-		mask = mask_from_posix(pa->e_perm, flags);
-		nfs4_acl_add_ace(acl, NFS4_ACE_ACCESS_DENIED_ACE_TYPE,
-				eflag,  mask_mask, NFS4_ACL_WHO_NAMED, pa->e_id);
-
+		mask = mask_from_posix(pa->e_perm & mask_mask, flags);
 		nfs4_acl_add_pair(acl, eflag, mask,
 				NFS4_ACL_WHO_NAMED, pa->e_id, flags);
 		pa++;
@@ -213,24 +211,15 @@ _posix_to_nfsv4_one(struct posix_acl *pa
 
 	/* allow ACEs */
 
-	if (pacl->a_count > 3) {
-		BUG_ON(pa->e_tag != ACL_GROUP_OBJ);
-		nfs4_acl_add_ace(acl, NFS4_ACE_ACCESS_DENIED_ACE_TYPE,
-				NFS4_ACE_IDENTIFIER_GROUP | eflag, mask_mask,
-				NFS4_ACL_WHO_GROUP, 0);
-	}
 	group_owner_entry = pa;
-	mask = mask_from_posix(pa->e_perm, flags);
+	mask = mask_from_posix(pa->e_perm & mask_mask, flags);
 	nfs4_acl_add_ace(acl, NFS4_ACE_ACCESS_ALLOWED_ACE_TYPE,
 			NFS4_ACE_IDENTIFIER_GROUP | eflag, mask,
 			NFS4_ACL_WHO_GROUP, 0);
 	pa++;
 
 	while (pa->e_tag == ACL_GROUP) {
-		mask = mask_from_posix(pa->e_perm, flags);
-		nfs4_acl_add_ace(acl, NFS4_ACE_ACCESS_DENIED_ACE_TYPE,
-				NFS4_ACE_IDENTIFIER_GROUP | eflag, mask_mask,
-				NFS4_ACL_WHO_NAMED, pa->e_id);
+		mask = mask_from_posix(pa->e_perm & mask_mask, flags);
 		nfs4_acl_add_ace(acl, NFS4_ACE_ACCESS_ALLOWED_ACE_TYPE,
 		    		NFS4_ACE_IDENTIFIER_GROUP | eflag, mask,
 		    		NFS4_ACL_WHO_NAMED, pa->e_id);

[PATCH 008 of 9] knfsd: nfsd4: acls: avoid unnecessary denies

[NeilBrown]

From: J. Bruce Fields <bfields@snoopy.citi.umich.edu>
We're inserting deny's between some ACEs in order to enforce posix draft
acl semantics which prevent permissions from accumulating across entries
in an acl.

That's fine, but we're doing that by inserting a deny after *every* allow,
which is overkill.  We shouldn't be adding them in places where they
actually make no difference.

Also replaced some helper functions for creating acl entries; I prefer
just assigning directly to the struct fields--it takes a few more lines,
but the field names provide some documentation that I think makes the
result easier understand.

Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
Signed-off-by: Neil Brown <neilb@suse.de>

### Diffstat output
 ./fs/nfsd/nfs4acl.c |  190 +++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 145 insertions(+), 45 deletions(-)

diff .prev/fs/nfsd/nfs4acl.c ./fs/nfsd/nfs4acl.c
--- .prev/fs/nfsd/nfs4acl.c	2007-02-13 10:37:27.000000000 +1100
+++ ./fs/nfsd/nfs4acl.c	2007-02-13 10:38:09.000000000 +1100
@@ -89,12 +89,19 @@ mask_from_posix(unsigned short perm, uns
 }
 
 static u32
-deny_mask(u32 allow_mask, unsigned int flags)
+deny_mask_from_posix(unsigned short perm, u32 flags)
 {
-	u32 ret = ~allow_mask & ~NFS4_MASK_UNSUPP;
-	if (!(flags & NFS4_ACL_DIR))
-		ret &= ~NFS4_ACE_DELETE_CHILD;
-	return ret;
+	u32 mask = 0;
+
+	if (perm & ACL_READ)
+		mask |= NFS4_READ_MODE;
+	if (perm & ACL_WRITE)
+		mask |= NFS4_WRITE_MODE;
+	if ((perm & ACL_WRITE) && (flags & NFS4_ACL_DIR))
+		mask |= NFS4_ACE_DELETE_CHILD;
+	if (perm & ACL_EXECUTE)
+		mask |= NFS4_EXECUTE_MODE;
+	return mask;
 }
 
 /* XXX: modify functions to return NFS errors; they're only ever
@@ -164,14 +171,51 @@ nfs4_acl_posix_to_nfsv4(struct posix_acl
 	return acl;
 }
 
+struct posix_acl_summary {
+	unsigned short owner;
+	unsigned short users;
+	unsigned short group;
+	unsigned short groups;
+	unsigned short other;
+	unsigned short mask;
+};
+
 static void
-nfs4_acl_add_pair(struct nfs4_acl *acl, int eflag, u32 mask, int whotype,
-		uid_t owner, unsigned int flags)
+summarize_posix_acl(struct posix_acl *acl, struct posix_acl_summary *pas)
 {
-	nfs4_acl_add_ace(acl, NFS4_ACE_ACCESS_ALLOWED_ACE_TYPE,
-				 eflag, mask, whotype, owner);
-	nfs4_acl_add_ace(acl, NFS4_ACE_ACCESS_DENIED_ACE_TYPE,
-				eflag, deny_mask(mask, flags), whotype, owner);
+	struct posix_acl_entry *pa, *pe;
+	pas->users = 0;
+	pas->groups = 0;
+	pas->mask = 07;
+
+	pe = acl->a_entries + acl->a_count;
+
+	FOREACH_ACL_ENTRY(pa, acl, pe) {
+		switch (pa->e_tag) {
+			case ACL_USER_OBJ:
+				pas->owner = pa->e_perm;
+				break;
+			case ACL_GROUP_OBJ:
+				pas->group = pa->e_perm;
+				break;
+			case ACL_USER:
+				pas->users |= pa->e_perm;
+				break;
+			case ACL_GROUP:
+				pas->groups |= pa->e_perm;
+				break;
+			case ACL_OTHER:
+				pas->other = pa->e_perm;
+				break;
+			case ACL_MASK:
+				pas->mask = pa->e_perm;
+				break;
+		}
+	}
+	/* We'll only care about effective permissions: */
+	pas->users &= pas->mask;
+	pas->group &= pas->mask;
+	pas->groups &= pas->mask;
 }
 
 /* We assume the acl has been verified with posix_acl_valid. */
@@ -179,30 +223,63 @@ static void
 _posix_to_nfsv4_one(struct posix_acl *pacl, struct nfs4_acl *acl,
 						unsigned int flags)
 {
-	struct posix_acl_entry *pa, *pe, *group_owner_entry;
-	u32 mask;
-	unsigned short mask_mask;
+	struct posix_acl_entry *pa, *group_owner_entry;
+	struct nfs4_ace *ace;
+	struct posix_acl_summary pas;
+	unsigned short deny;
 	int eflag = ((flags & NFS4_ACL_TYPE_DEFAULT) ?
 					NFS4_INHERITANCE_FLAGS : 0);
 
 	BUG_ON(pacl->a_count < 3);
-	pe = pacl->a_entries + pacl->a_count;
-	pa = pe - 2; /* if mask entry exists, it's second from the last. */
-	if (pa->e_tag == ACL_MASK)
-		mask_mask = pa->e_perm;
-	else
-		mask_mask = S_IRWXO;
+	summarize_posix_acl(pacl, &pas);
 
 	pa = pacl->a_entries;
-	BUG_ON(pa->e_tag != ACL_USER_OBJ);
-	mask = mask_from_posix(pa->e_perm, flags | NFS4_ACL_OWNER);
-	nfs4_acl_add_pair(acl, eflag, mask, NFS4_ACL_WHO_OWNER, 0, flags);
+	ace = acl->aces + acl->naces;
+
+	/* We could deny everything not granted by the owner: */
+	deny = ~pas.owner;
+	/*
+	 * but it is equivalent (and simpler) to deny only what is not
+	 * granted by later entries:
+	 */
+	deny &= pas.users | pas.group | pas.groups | pas.other;
+	if (deny) {
+		ace->type = NFS4_ACE_ACCESS_DENIED_ACE_TYPE;
+		ace->flag = eflag;
+		ace->access_mask = deny_mask_from_posix(deny, flags);
+		ace->whotype = NFS4_ACL_WHO_OWNER;
+		ace++;
+		acl->naces++;
+	}
+
+	ace->type = NFS4_ACE_ACCESS_ALLOWED_ACE_TYPE;
+	ace->flag = eflag;
+	ace->access_mask = mask_from_posix(pa->e_perm, flags | NFS4_ACL_OWNER);
+	ace->whotype = NFS4_ACL_WHO_OWNER;
+	ace++;
+	acl->naces++;
 	pa++;
 
 	while (pa->e_tag == ACL_USER) {
-		mask = mask_from_posix(pa->e_perm & mask_mask, flags);
-		nfs4_acl_add_pair(acl, eflag, mask,
-				NFS4_ACL_WHO_NAMED, pa->e_id, flags);
+		deny = ~(pa->e_perm & pas.mask);
+		deny &= pas.groups | pas.group | pas.other;
+		if (deny) {
+			ace->type = NFS4_ACE_ACCESS_DENIED_ACE_TYPE;
+			ace->flag = eflag;
+			ace->access_mask = deny_mask_from_posix(deny, flags);
+			ace->whotype = NFS4_ACL_WHO_NAMED;
+			ace->who = pa->e_id;
+			ace++;
+			acl->naces++;
+		}
+		ace->type = NFS4_ACE_ACCESS_ALLOWED_ACE_TYPE;
+		ace->flag = eflag;
+		ace->access_mask = mask_from_posix(pa->e_perm & pas.mask,
+						   flags);
+		ace->whotype = NFS4_ACL_WHO_NAMED;
+		ace->who = pa->e_id;
+		ace++;
+		acl->naces++;
 		pa++;
 	}
 
@@ -212,41 +289,64 @@ _posix_to_nfsv4_one(struct posix_acl *pa
 	/* allow ACEs */
 
 	group_owner_entry = pa;
-	mask = mask_from_posix(pa->e_perm & mask_mask, flags);
-	nfs4_acl_add_ace(acl, NFS4_ACE_ACCESS_ALLOWED_ACE_TYPE,
-			NFS4_ACE_IDENTIFIER_GROUP | eflag, mask,
-			NFS4_ACL_WHO_GROUP, 0);
+
+	ace->type = NFS4_ACE_ACCESS_ALLOWED_ACE_TYPE;
+	ace->flag = eflag;
+	ace->access_mask = mask_from_posix(pas.group, flags);
+	ace->whotype = NFS4_ACL_WHO_GROUP;
+	ace++;
+	acl->naces++;
 	pa++;
 
 	while (pa->e_tag == ACL_GROUP) {
-		mask = mask_from_posix(pa->e_perm & mask_mask, flags);
-		nfs4_acl_add_ace(acl, NFS4_ACE_ACCESS_ALLOWED_ACE_TYPE,
-		    		NFS4_ACE_IDENTIFIER_GROUP | eflag, mask,
-		    		NFS4_ACL_WHO_NAMED, pa->e_id);
+		ace->type = NFS4_ACE_ACCESS_ALLOWED_ACE_TYPE;
+		ace->flag = eflag | NFS4_ACE_IDENTIFIER_GROUP;
+		ace->access_mask = mask_from_posix(pa->e_perm & pas.mask,
+						   flags);
+		ace->whotype = NFS4_ACL_WHO_NAMED;
+		ace->who = pa->e_id;
+		ace++;
+		acl->naces++;
 		pa++;
 	}
 
 	/* deny ACEs */
 
 	pa = group_owner_entry;
-	mask = mask_from_posix(pa->e_perm, flags);
-	nfs4_acl_add_ace(acl, NFS4_ACE_ACCESS_DENIED_ACE_TYPE,
-			NFS4_ACE_IDENTIFIER_GROUP | eflag,
-			deny_mask(mask, flags), NFS4_ACL_WHO_GROUP, 0);
+
+	deny = ~pas.group & pas.other;
+	if (deny) {
+		ace->type = NFS4_ACE_ACCESS_DENIED_ACE_TYPE;
+		ace->flag = eflag | NFS4_ACE_IDENTIFIER_GROUP;
+		ace->access_mask = deny_mask_from_posix(deny, flags);
+		ace->whotype = NFS4_ACL_WHO_GROUP;
+		ace++;
+		acl->naces++;
+	}
 	pa++;
+
 	while (pa->e_tag == ACL_GROUP) {
-		mask = mask_from_posix(pa->e_perm, flags);
-		nfs4_acl_add_ace(acl, NFS4_ACE_ACCESS_DENIED_ACE_TYPE,
-		    		NFS4_ACE_IDENTIFIER_GROUP | eflag,
-		    		deny_mask(mask, flags), NFS4_ACL_WHO_NAMED, pa->e_id);
+		deny = ~(pa->e_perm & pas.mask);
+		deny &= pas.other;
+		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->whotype = NFS4_ACL_WHO_NAMED;
+			ace->who = pa->e_id;
+			ace++;
+			acl->naces++;
+		}
 		pa++;
 	}
 
 	if (pa->e_tag == ACL_MASK)
 		pa++;
-	BUG_ON(pa->e_tag != ACL_OTHER);
-	mask = mask_from_posix(pa->e_perm, flags);
-	nfs4_acl_add_pair(acl, eflag, mask, NFS4_ACL_WHO_EVERYONE, 0, flags);
+	ace->type = NFS4_ACE_ACCESS_ALLOWED_ACE_TYPE;
+	ace->flag = eflag;
+	ace->access_mask = mask_from_posix(pa->e_perm, flags);
+	ace->whotype = NFS4_ACL_WHO_EVERYONE;
+	acl->naces++;
 }
 
 static void

[PATCH 009 of 9] knfsd: nfsd4: fix handling of directories without default ACLs

[NeilBrown]

From: J. Bruce Fields <bfields@citi.umich.edu>
When setting an ACL that lacks inheritable ACEs on a directory, we
should set a default ACL of zero length, not a default ACL with all bits
denied.

Signed-off-by: "J. Bruce Fields" <bfields@citi.umich.edu>
Signed-off-by: Neil Brown <neilb@suse.de>

### Diffstat output
 ./fs/nfsd/nfs4acl.c |   21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff .prev/fs/nfsd/nfs4acl.c ./fs/nfsd/nfs4acl.c
--- .prev/fs/nfsd/nfs4acl.c	2007-02-13 10:38:09.000000000 +1100
+++ ./fs/nfsd/nfs4acl.c	2007-02-13 10:38:26.000000000 +1100
@@ -416,6 +416,7 @@ struct posix_ace_state_array {
  * calculated so far: */
 
 struct posix_acl_state {
+	int empty;
 	struct posix_ace_state owner;
 	struct posix_ace_state group;
 	struct posix_ace_state other;
@@ -431,6 +432,7 @@ init_state(struct posix_acl_state *state
 	int alloc;
 
 	memset(state, 0, sizeof(struct posix_acl_state));
+	state->empty = 1;
 	/*
 	 * In the worst case, each individual acl could be for a distinct
 	 * named user or group, but we don't no which, so we allocate
@@ -498,6 +500,20 @@ posix_state_to_acl(struct posix_acl_stat
 	int nace;
 	int i, error = 0;
 
+	/*
+	 * ACLs with no ACEs are treated differently in the inheritable
+	 * and effective cases: when there are no inheritable ACEs, we
+	 * set a zero-length default posix acl:
+	 */
+	if (state->empty && (flags & NFS4_ACL_TYPE_DEFAULT)) {
+		pacl = posix_acl_alloc(0, GFP_KERNEL);
+		return pacl ? pacl : ERR_PTR(-ENOMEM);
+	}
+	/*
+	 * When there are no effective ACEs, the following will end
+	 * up setting a 3-element effective posix ACL with all
+	 * permissions zero.
+	 */
 	nace = 4 + state->users->n + state->groups->n;
 	pacl = posix_acl_alloc(nace, GFP_KERNEL);
 	if (!pacl)
@@ -613,6 +629,8 @@ static void process_one_v4_ace(struct po
 	u32 mask = ace->access_mask;
 	int i;
 
+	state->empty = 0;
+
 	switch (ace2type(ace)) {
 	case ACL_USER_OBJ:
 		if (ace->type == NFS4_ACE_ACCESS_ALLOWED_ACE_TYPE) {
@@ -717,7 +735,8 @@ int nfs4_acl_nfsv4_to_posix(struct nfs4_
 		ret = PTR_ERR(*pacl);
 		goto out_dstate;
 	}
-	*dpacl = posix_state_to_acl(&default_acl_state, flags);
+	*dpacl = posix_state_to_acl(&default_acl_state,
+						flags | NFS4_ACL_TYPE_DEFAULT);
 	if (IS_ERR(*dpacl)) {
 		ret = PTR_ERR(*dpacl);
 		posix_acl_release(*pacl);