[PATCH 1/1] nfsd(v2/v3): fix the failure of creation from HPUX client --revised

[PATCH 1/1] nfsd(v2/v3): fix the failure of creation from HPUX client --revised

[wengang wang]

for descriptions for the problem and solution please see my former post with
Subject "nfsd(v2/v3): fix the failure of creation from HPUX client".

comparing with former post, this patch puts the trick to nfsd_create_setattr().

the patch is based on 2.6.27.10.

Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
--
 vfs.c |   20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)
diff -up ./fs/nfsd/vfs.c.orig ./fs/nfsd/vfs.c
--- ./fs/nfsd/vfs.c.orig	2008-12-23 14:11:14.000000000 +0800
+++ ./fs/nfsd/vfs.c	2009-01-12 19:35:13.000000000 +0800
@@ -1155,7 +1155,7 @@ nfsd_commit(struct svc_rqst *rqstp, stru
 
 static __be32
 nfsd_create_setattr(struct svc_rqst *rqstp, struct svc_fh *resfhp,
-			struct iattr *iap)
+			struct iattr *iap, int newfile)
 {
 	/*
 	 * Mode has already been set earlier in create:
@@ -1168,6 +1168,16 @@ nfsd_create_setattr(struct svc_rqst *rqs
 	 */
 	if (current->fsuid != 0)
 		iap->ia_valid &= ~(ATTR_UID|ATTR_GID);
+	/*
+	 * HPUX client sometimes creates a file in mode 000, and set
+	 * size to 0. setting size to 0 may fail for some spcific
+	 * file systems by the permission checking which requires
+	 * WRITE privilege but the mode is 000.
+	 * we ignore setting size to 0 for the creation, since it's
+	 * just 0 after created.
+	 * */
+	if (newfile && (iap->ia_valid & ATTR_SIZE) && (iap->ia_size == 0))
+		iap->ia_valid &= ~ATTR_SIZE;
 	if (iap->ia_valid)
 		return nfsd_setattr(rqstp, resfhp, iap, 0, (time_t)0);
 	return 0;
@@ -1191,6 +1201,7 @@ nfsd_create(struct svc_rqst *rqstp, stru
 	__be32		err;
 	__be32		err2;
 	int		host_err;
+	int		newfile = 0;
 
 	err = nfserr_perm;
 	if (!flen)
@@ -1268,6 +1279,7 @@ nfsd_create(struct svc_rqst *rqstp, stru
 	switch (type) {
 	case S_IFREG:
 		host_err = vfs_create(dirp, dchild, iap->ia_mode, NULL);
+		newfile = 1;
 		break;
 	case S_IFDIR:
 		host_err = vfs_mkdir(dirp, dchild, iap->ia_mode);
@@ -1289,7 +1301,7 @@ nfsd_create(struct svc_rqst *rqstp, stru
 		write_inode_now(dchild->d_inode, 1);
 	}
 
-	err2 = nfsd_create_setattr(rqstp, resfhp, iap);
+	err2 = nfsd_create_setattr(rqstp, resfhp, iap, newfile);
 	if (err2)
 		err = err2;
 	mnt_drop_write(fhp->fh_export->ex_path.mnt);
@@ -1324,6 +1336,7 @@ nfsd_create_v3(struct svc_rqst *rqstp, s
 	__be32		err2;
 	int		host_err;
 	__u32		v_mtime=0, v_atime=0;
+	int		newfile = 0;
 
 	err = nfserr_perm;
 	if (!flen)
@@ -1415,6 +1428,7 @@ nfsd_create_v3(struct svc_rqst *rqstp, s
 	}
 	if (created)
 		*created = 1;
+	newfile = 1;
 
 	if (EX_ISSYNC(fhp->fh_export)) {
 		err = nfserrno(nfsd_sync_dir(dentry));
@@ -1433,7 +1447,7 @@ nfsd_create_v3(struct svc_rqst *rqstp, s
 	}
 
  set_attr:
-	err2 = nfsd_create_setattr(rqstp, resfhp, iap);
+	err2 = nfsd_create_setattr(rqstp, resfhp, iap, newfile);
 	if (err2)
 		err = err2;
 

Re: [PATCH 1/1] nfsd(v2/v3): fix the failure of creation from HPUX client --revised

[J. Bruce Fields]

On Mon, Jan 12, 2009 at 08:14:46PM +0800, wengang wang wrote:
> for descriptions for the problem and solution please see my former post with
> Subject "nfsd(v2/v3): fix the failure of creation from HPUX client".

Could you just repeat that introduction each time?  I want to commit it
with the patch, and that will save me some work hunting down the old
email.

> comparing with former post, this patch puts the trick to nfsd_create_setattr().
> 
> the patch is based on 2.6.27.10.
> 
> Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
> --
>  vfs.c |   20 +++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
> diff -up ./fs/nfsd/vfs.c.orig ./fs/nfsd/vfs.c
> --- ./fs/nfsd/vfs.c.orig	2008-12-23 14:11:14.000000000 +0800
> +++ ./fs/nfsd/vfs.c	2009-01-12 19:35:13.000000000 +0800
> @@ -1155,7 +1155,7 @@ nfsd_commit(struct svc_rqst *rqstp, stru
>  
>  static __be32
>  nfsd_create_setattr(struct svc_rqst *rqstp, struct svc_fh *resfhp,
> -			struct iattr *iap)
> +			struct iattr *iap, int newfile)
>  {
>  	/*
>  	 * Mode has already been set earlier in create:
> @@ -1168,6 +1168,16 @@ nfsd_create_setattr(struct svc_rqst *rqs
>  	 */
>  	if (current->fsuid != 0)
>  		iap->ia_valid &= ~(ATTR_UID|ATTR_GID);
> +	/*
> +	 * HPUX client sometimes creates a file in mode 000, and set
> +	 * size to 0. setting size to 0 may fail for some spcific
> +	 * file systems by the permission checking which requires
> +	 * WRITE privilege but the mode is 000.
> +	 * we ignore setting size to 0 for the creation, since it's
> +	 * just 0 after created.
> +	 * */
> +	if (newfile && (iap->ia_valid & ATTR_SIZE) && (iap->ia_size == 0))
> +		iap->ia_valid &= ~ATTR_SIZE;

It seems that all newfile does is tell us whether the net thing that was
created is a regular file or not.  Does it even make sense to set size
on other objects?  If not, then why not just make this:

	if (iap->ia_size == 0)
		iap->ia_valid &= ~ATTR_SIZE;

and remove the need for "newfile"?

Also: I'm curious, can you tell where exactly in the code the permission
error is getting set?  From a quick look at nfsd_setattr() it appears
it's doing all permissions checks with NFSD_MAY_OWNER_OVERRIDE, which
should allow the setting of size, even in the case of mode 0, as long as
the caller is the owner of the new file.  Is HPUX also trying to set the
onwer?

--b.

>  	if (iap->ia_valid)
>  		return nfsd_setattr(rqstp, resfhp, iap, 0, (time_t)0);
>  	return 0;
> @@ -1191,6 +1201,7 @@ nfsd_create(struct svc_rqst *rqstp, stru
>  	__be32		err;
>  	__be32		err2;
>  	int		host_err;
> +	int		newfile = 0;
>  
>  	err = nfserr_perm;
>  	if (!flen)
> @@ -1268,6 +1279,7 @@ nfsd_create(struct svc_rqst *rqstp, stru
>  	switch (type) {
>  	case S_IFREG:
>  		host_err = vfs_create(dirp, dchild, iap->ia_mode, NULL);
> +		newfile = 1;
>  		break;
>  	case S_IFDIR:
>  		host_err = vfs_mkdir(dirp, dchild, iap->ia_mode);
> @@ -1289,7 +1301,7 @@ nfsd_create(struct svc_rqst *rqstp, stru
>  		write_inode_now(dchild->d_inode, 1);
>  	}
>  
> -	err2 = nfsd_create_setattr(rqstp, resfhp, iap);
> +	err2 = nfsd_create_setattr(rqstp, resfhp, iap, newfile);
>  	if (err2)
>  		err = err2;
>  	mnt_drop_write(fhp->fh_export->ex_path.mnt);
> @@ -1324,6 +1336,7 @@ nfsd_create_v3(struct svc_rqst *rqstp, s
>  	__be32		err2;
>  	int		host_err;
>  	__u32		v_mtime=0, v_atime=0;
> +	int		newfile = 0;
>  
>  	err = nfserr_perm;
>  	if (!flen)
> @@ -1415,6 +1428,7 @@ nfsd_create_v3(struct svc_rqst *rqstp, s
>  	}
>  	if (created)
>  		*created = 1;
> +	newfile = 1;
>  
>  	if (EX_ISSYNC(fhp->fh_export)) {
>  		err = nfserrno(nfsd_sync_dir(dentry));
> @@ -1433,7 +1447,7 @@ nfsd_create_v3(struct svc_rqst *rqstp, s
>  	}
>  
>   set_attr:
> -	err2 = nfsd_create_setattr(rqstp, resfhp, iap);
> +	err2 = nfsd_create_setattr(rqstp, resfhp, iap, newfile);
>  	if (err2)
>  		err = err2;
>  
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] nfsd(v2/v3): fix the failure of creation from HPUX client --revised

[Wengang Wang]

J. Bruce Fields wrote:
> On Mon, Jan 12, 2009 at 08:14:46PM +0800, wengang wang wrote:
>> for descriptions for the problem and solution please see my former post with
>> Subject "nfsd(v2/v3): fix the failure of creation from HPUX client".
> 
> Could you just repeat that introduction each time?  I want to commit it
> with the patch, and that will save me some work hunting down the old
> email.

Sorry.
OK, will follow that for later patch postings.

>> comparing with former post, this patch puts the trick to nfsd_create_setattr().
>>
>> the patch is based on 2.6.27.10.
>>
>> Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
>> --
>>  vfs.c |   20 +++++++++++++++++---
>>  1 file changed, 17 insertions(+), 3 deletions(-)
>> diff -up ./fs/nfsd/vfs.c.orig ./fs/nfsd/vfs.c
>> --- ./fs/nfsd/vfs.c.orig	2008-12-23 14:11:14.000000000 +0800
>> +++ ./fs/nfsd/vfs.c	2009-01-12 19:35:13.000000000 +0800
>> @@ -1155,7 +1155,7 @@ nfsd_commit(struct svc_rqst *rqstp, stru
>>  
>>  static __be32
>>  nfsd_create_setattr(struct svc_rqst *rqstp, struct svc_fh *resfhp,
>> -			struct iattr *iap)
>> +			struct iattr *iap, int newfile)
>>  {
>>  	/*
>>  	 * Mode has already been set earlier in create:
>> @@ -1168,6 +1168,16 @@ nfsd_create_setattr(struct svc_rqst *rqs
>>  	 */
>>  	if (current->fsuid != 0)
>>  		iap->ia_valid &= ~(ATTR_UID|ATTR_GID);
>> +	/*
>> +	 * HPUX client sometimes creates a file in mode 000, and set
>> +	 * size to 0. setting size to 0 may fail for some spcific
>> +	 * file systems by the permission checking which requires
>> +	 * WRITE privilege but the mode is 000.
>> +	 * we ignore setting size to 0 for the creation, since it's
>> +	 * just 0 after created.
>> +	 * */
>> +	if (newfile && (iap->ia_valid & ATTR_SIZE) && (iap->ia_size == 0))
>> +		iap->ia_valid &= ~ATTR_SIZE;
> 
> It seems that all newfile does is tell us whether the net thing that was
> created is a regular file or not.  Does it even make sense to set size
> on other objects?  If not, then why not just make this:
> 
> 	if (iap->ia_size == 0)
> 		iap->ia_valid &= ~ATTR_SIZE;
> 
> and remove the need for "newfile"?
> 

I want it tell us whether it's new created(vfs_create()ed) file or a 
already existing one to be truncated.
the later is in case of

if (dchild->d_inode) {
	...
	switch (createmode) {
	case NFS3_CREATE_UNCHECKED:
		...
		else {
			...
			goto set_attr;
	
I think we should resize file to zero for the truncating case.
maybe I'm wrong somethere?

> Also: I'm curious, can you tell where exactly in the code the permission
> error is getting set?  

the original call trace for it is like following

@ got the stack that generic_permission() returns -EACCESS
@ generic_permission error -13 

@
@  [<f8ad1930>] gfs_permission+0x61/0x74 [gfs] 

@
@  [<f8ad18cf>] gfs_permission+0x0/0x74 [gfs]
@  [<c047c0f3>] permission+0x78/0xb5
@  [<f8ad19f6>] gfs_setattr+0xb3/0x378 [gfs]
@  [<c0487187>] notify_change+0x138/0x305
@  [<f8b41b0b>] nfsd_setattr+0x337/0x3d5 [nfsd]
@  [<f8b4304d>] nfsd_create_v3+0x30d/0x3c0 [nfsd]
@  [<f8b4759b>] nfsd3_proc_create+0x11c/0x12d [nfsd]
@  [<f8b3e1a4>] <7>Dead loop on netdevice eth0, fix it urgently!
@ nfsd_dispatch+0xbb/0x1a9 [nfsd]
@  [<f8972549>] svc_process+0x3c8/0x633 [sunrpc]
@  [<f8b3e68c>] nfsd+0x17e/0x286 [nfsd]
@  [<f8b3e50e>] nfsd+0x0/0x286 [nfsd]
@  [<c0405c3b>] kernel_thread_helper+0x7/0x10

generic_permission() is called from gfs code, and it returns -EACCESS
because mode is 0.


 From a quick look at nfsd_setattr() it appears
> it's doing all permissions checks with NFSD_MAY_OWNER_OVERRIDE, which
> should allow the setting of size, even in the case of mode 0, as long as
> the caller is the owner of the new file.  

I think you are mentioning the call nfsd_permission() in nfsd_setattr().
if you are, the call is under a condition of
	if (iap->ia_size < inode->i_size) {
in this case iap->ia_size(being 0) equals to inode->i_size, so 
nfsd_permission() is not called.

and from above call trace, it's not within nfsd_permission() where error 
returns.

Is HPUX also trying to set the
> onwer?
> 

No for uid but yes for gid.

thanks
wengang.
> --b.
> 
>>  	if (iap->ia_valid)
>>  		return nfsd_setattr(rqstp, resfhp, iap, 0, (time_t)0);
>>  	return 0;
>> @@ -1191,6 +1201,7 @@ nfsd_create(struct svc_rqst *rqstp, stru
>>  	__be32		err;
>>  	__be32		err2;
>>  	int		host_err;
>> +	int		newfile = 0;
>>  
>>  	err = nfserr_perm;
>>  	if (!flen)
>> @@ -1268,6 +1279,7 @@ nfsd_create(struct svc_rqst *rqstp, stru
>>  	switch (type) {
>>  	case S_IFREG:
>>  		host_err = vfs_create(dirp, dchild, iap->ia_mode, NULL);
>> +		newfile = 1;
>>  		break;
>>  	case S_IFDIR:
>>  		host_err = vfs_mkdir(dirp, dchild, iap->ia_mode);
>> @@ -1289,7 +1301,7 @@ nfsd_create(struct svc_rqst *rqstp, stru
>>  		write_inode_now(dchild->d_inode, 1);
>>  	}
>>  
>> -	err2 = nfsd_create_setattr(rqstp, resfhp, iap);
>> +	err2 = nfsd_create_setattr(rqstp, resfhp, iap, newfile);
>>  	if (err2)
>>  		err = err2;
>>  	mnt_drop_write(fhp->fh_export->ex_path.mnt);
>> @@ -1324,6 +1336,7 @@ nfsd_create_v3(struct svc_rqst *rqstp, s
>>  	__be32		err2;
>>  	int		host_err;
>>  	__u32		v_mtime=0, v_atime=0;
>> +	int		newfile = 0;
>>  
>>  	err = nfserr_perm;
>>  	if (!flen)
>> @@ -1415,6 +1428,7 @@ nfsd_create_v3(struct svc_rqst *rqstp, s
>>  	}
>>  	if (created)
>>  		*created = 1;
>> +	newfile = 1;
>>  
>>  	if (EX_ISSYNC(fhp->fh_export)) {
>>  		err = nfserrno(nfsd_sync_dir(dentry));
>> @@ -1433,7 +1447,7 @@ nfsd_create_v3(struct svc_rqst *rqstp, s
>>  	}
>>  
>>   set_attr:
>> -	err2 = nfsd_create_setattr(rqstp, resfhp, iap);
>> +	err2 = nfsd_create_setattr(rqstp, resfhp, iap, newfile);
>>  	if (err2)
>>  		err = err2;
>>  
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] nfsd(v2/v3): fix the failure of creation from HPUX client --revised

[J. Bruce Fields]

On Fri, Feb 06, 2009 at 10:58:40AM +0800, Wengang Wang wrote:
> J. Bruce Fields wrote:
>> On Mon, Jan 12, 2009 at 08:14:46PM +0800, wengang wang wrote:
>>> for descriptions for the problem and solution please see my former post with
>>> Subject "nfsd(v2/v3): fix the failure of creation from HPUX client".
>>
>> Could you just repeat that introduction each time?  I want to commit it
>> with the patch, and that will save me some work hunting down the old
>> email.
>
> Sorry.
> OK, will follow that for later patch postings.

Thanks.

>> It seems that all newfile does is tell us whether the net thing that was
>> created is a regular file or not.  Does it even make sense to set size
>> on other objects?  If not, then why not just make this:
>>
>> 	if (iap->ia_size == 0)
>> 		iap->ia_valid &= ~ATTR_SIZE;
>>
>> and remove the need for "newfile"?
>>
>
> I want it tell us whether it's new created(vfs_create()ed) file or a  
> already existing one to be truncated.
> the later is in case of
>
> if (dchild->d_inode) {
> 	...
> 	switch (createmode) {
> 	case NFS3_CREATE_UNCHECKED:
> 		...
> 		else {
> 			...
> 			goto set_attr;
> 	
> I think we should resize file to zero for the truncating case.
> maybe I'm wrong somethere?

OK, sorry, you're right, I missed the "goto" in the nfsd_create_v3()
case.  And in other cases we still want the permission check to handle,
I guess.  Fair enough.

> From a quick look at nfsd_setattr() it appears
>> it's doing all permissions checks with NFSD_MAY_OWNER_OVERRIDE, which
>> should allow the setting of size, even in the case of mode 0, as long as
>> the caller is the owner of the new file.  
>
> I think you are mentioning the call nfsd_permission() in nfsd_setattr().
> if you are, the call is under a condition of
> 	if (iap->ia_size < inode->i_size) {
> in this case iap->ia_size(being 0) equals to inode->i_size, so  
> nfsd_permission() is not called.
>
> and from above call trace, it's not within nfsd_permission() where error  
> returns.

Yeah, OK, and OWNER_OVERRIDE isn't really what we want in this case
anyway, since the client doesn't know whether the operation will be a
create and so can't check these permissions for us as it can in the case
of writes.

That being the case, I admit, your first attempt looks simpler.  Could
you just do that, but move the common code (and comment) into a helper
function called from the same two places?

--b.