Re: [PATCH] Fix NFS IRIX compatibility braindamage

Re: [PATCH] Fix NFS IRIX compatibility braindamage

[Dave Jones]

I'm going through the old 2.4 changelogs looking for bits that
have been missed out, the little one liners have been going
direct to Linus/maintainer, but here's the first one I'm
unsure of..

Any reason this is missing in 2.5 ?

		Dave


On Tue, Oct 29, 2002 at 11:09:05AM +0000, Linux Kernel wrote:
 > ChangeSet 1.771, 2002/10/29 09:09:05-02:00, okir@suse.de
 > 
 > 	[PATCH] Fix NFS IRIX compatibility braindamage
 > 	
 > 
 > 
 > # This patch includes the following deltas:
 > #	           ChangeSet	1.770   -> 1.771  
 > #	   fs/nfsd/nfsproc.c	1.9     -> 1.10   
 > #	       fs/nfsd/vfs.c	1.13    -> 1.14   
 > #	include/linux/nfsd/nfsd.h	1.4     -> 1.5    
 > #
 > 
 >  fs/nfsd/nfsproc.c         |    4 ++--
 >  fs/nfsd/vfs.c             |   26 +++++++++++++++-----------
 >  include/linux/nfsd/nfsd.h |    3 ++-
 >  3 files changed, 19 insertions(+), 14 deletions(-)
 > 
 > 
 > diff -Nru a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
 > --- a/fs/nfsd/nfsproc.c	Tue Oct 29 04:08:56 2002
 > +++ b/fs/nfsd/nfsproc.c	Tue Oct 29 04:08:56 2002
 > @@ -264,11 +264,11 @@
 >  					/* this is probably a permission check..
 >  					 * at least IRIX implements perm checking on
 >  					 *   echo thing > device-special-file-or-pipe
 > -					 * by does a CREATE with type==0
 > +					 * by doing a CREATE with type==0
 >  					 */
 >  					nfserr = nfsd_permission(newfhp->fh_export,
 >  								 newfhp->fh_dentry,
 > -								 MAY_WRITE);
 > +								 MAY_WRITE|_NFSD_IRIX_BOGOSITY);
 >  					if (nfserr && nfserr != nfserr_rofs)
 >  						goto out_unlock;
 >  				}
 > diff -Nru a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
 > --- a/fs/nfsd/vfs.c	Tue Oct 29 04:08:56 2002
 > +++ b/fs/nfsd/vfs.c	Tue Oct 29 04:08:56 2002
 > @@ -1493,17 +1493,21 @@
 >  		inode->i_uid, inode->i_gid, current->fsuid, current->fsgid);
 >  #endif
 >  
 > -	/* only care about readonly exports for files and
 > -	 * directories. links don't have meaningful write access,
 > -	 * and all else is local to the client
 > -	 */
 > -	if (S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode)) 
 > -		if (acc & (MAY_WRITE | MAY_SATTR | MAY_TRUNC)) {
 > -			if (EX_RDONLY(exp) || IS_RDONLY(inode))
 > -				return nfserr_rofs;
 > -			if (/* (acc & MAY_WRITE) && */ IS_IMMUTABLE(inode))
 > -				return nfserr_perm;
 > -		}
 > +	/* The following code is here to make IRIX happy, which
 > +	 * does a permission check every time a user does
 > +	 *	echo yaddayadda > special-file
 > +	 * by sending a CREATE request.
 > +	 * The original code would check read-only export status
 > +	 * only for regular files and directories, allowing
 > +	 * clients to chown/chmod device files and fifos even
 > +	 * on volumes exported read-only. */
 > +	if (!(acc & _NFSD_IRIX_BOGOSITY)
 > +	 && (acc & (MAY_WRITE | MAY_SATTR | MAY_TRUNC))) {
 > +		if (EX_RDONLY(exp) || IS_RDONLY(inode))
 > +			return nfserr_rofs;
 > +		if (/* (acc & MAY_WRITE) && */ IS_IMMUTABLE(inode))
 > +			return nfserr_perm;
 > +	}
 >  	if ((acc & MAY_TRUNC) && IS_APPEND(inode))
 >  		return nfserr_perm;
 >  
 > diff -Nru a/include/linux/nfsd/nfsd.h b/include/linux/nfsd/nfsd.h
 > --- a/include/linux/nfsd/nfsd.h	Tue Oct 29 04:08:56 2002
 > +++ b/include/linux/nfsd/nfsd.h	Tue Oct 29 04:08:56 2002
 > @@ -37,7 +37,8 @@
 >  #define MAY_TRUNC		16
 >  #define MAY_LOCK		32
 >  #define MAY_OWNER_OVERRIDE	64
 > -#if (MAY_SATTR | MAY_TRUNC | MAY_LOCK | MAX_OWNER_OVERRIDE) & (MAY_READ | MAY_WRITE | MAY_EXEC | MAY_OWNER_OVERRIDE)
 > +#define _NFSD_IRIX_BOGOSITY	128
 > +#if (MAY_SATTR | MAY_TRUNC | MAY_LOCK | MAY_OWNER_OVERRIDE | _NFSD_IRIX_BOGOSITY) & (MAY_READ | MAY_WRITE | MAY_EXEC)
 >  # error "please use a different value for MAY_SATTR or MAY_TRUNC or MAY_LOCK or MAY_OWNER_OVERRIDE."
 >  #endif
 >  #define MAY_CREATE		(MAY_EXEC|MAY_WRITE)
 > -
 > To unsubscribe from this list: send the line "unsubscribe bk-commits-24" in
 > the body of a message to majordomo@vger.kernel.org
 > More majordomo info at  http://vger.kernel.org/majordomo-info.html
---end quoted text---

-- 
| Dave Jones.        http://www.codemonkey.org.uk
| SuSE Labs

Re: [PATCH] Fix NFS IRIX compatibility braindamage

[Olaf Kirch]

On Mon, Jan 06, 2003 at 07:33:20PM +0000, Dave Jones wrote:
> I'm going through the old 2.4 changelogs looking for bits that
> have been missed out, the little one liners have been going
> direct to Linus/maintainer, but here's the first one I'm
> unsure of..
> 
> Any reason this is missing in 2.5 ?

I think I sent it to the NFS mailing list and forgot about it
afterwards.

The problem this patch tries to address is that the current code allows
diskless clients to chmod device files, even when the directory is
exported read-only.

Olaf
-- 
Olaf Kirch     |  Anyone who has had to work with X.509 has probably
okir@suse.de   |  experienced what can best be described as
---------------+  ISO water torture. -- Peter Gutmann

Re: [PATCH] Fix NFS IRIX compatibility braindamage

[Neil Brown]

On Monday January 6, okir@suse.de wrote:
> On Mon, Jan 06, 2003 at 07:33:20PM +0000, Dave Jones wrote:
> > I'm going through the old 2.4 changelogs looking for bits that
> > have been missed out, the little one liners have been going
> > direct to Linus/maintainer, but here's the first one I'm
> > unsure of..
> > 
> > Any reason this is missing in 2.5 ?
> 
> I think I sent it to the NFS mailing list and forgot about it
> afterwards.
> 
> The problem this patch tries to address is that the current code allows
> diskless clients to chmod device files, even when the directory is
> exported read-only.

This has been on my list of things to follow up in 2.5 .... for quite
a while.  Thanks for the prompting.

I'll commit to getting an appropriate patch to Linus, probably
something like the following.

Thanks,
NeilBrown


 ----------- Diffstat output ------------
 ./fs/nfsd/nfsproc.c         |    4 ++--
 ./fs/nfsd/vfs.c             |    8 ++++----
 ./include/linux/nfsd/nfsd.h |    3 ++-
 3 files changed, 8 insertions(+), 7 deletions(-)

diff ./fs/nfsd/nfsproc.c~current~ ./fs/nfsd/nfsproc.c
--- ./fs/nfsd/nfsproc.c~current~	2003-01-08 11:20:22.000000000 +1100
+++ ./fs/nfsd/nfsproc.c	2003-01-08 11:20:41.000000000 +1100
@@ -250,11 +250,11 @@ nfsd_proc_create(struct svc_rqst *rqstp,
 					/* this is probably a permission check..
 					 * at least IRIX implements perm checking on
 					 *   echo thing > device-special-file-or-pipe
-					 * by does a CREATE with type==0
+					 * by doing a CREATE with type==0
 					 */
 					nfserr = nfsd_permission(newfhp->fh_export,
 								 newfhp->fh_dentry,
-								 MAY_WRITE);
+								 MAY_WRITE|MAY_LOCAL_ACCESS);
 					if (nfserr && nfserr != nfserr_rofs)
 						goto out_unlock;
 				}

diff ./fs/nfsd/vfs.c~current~ ./fs/nfsd/vfs.c
--- ./fs/nfsd/vfs.c~current~	2003-01-08 11:20:51.000000000 +1100
+++ ./fs/nfsd/vfs.c	2003-01-08 11:23:00.000000000 +1100
@@ -1515,11 +1515,11 @@ nfsd_permission(struct svc_export *exp, 
 		inode->i_uid, inode->i_gid, current->fsuid, current->fsgid);
 #endif
 
-	/* only care about readonly exports for files and
-	 * directories. links don't have meaningful write access,
-	 * and all else is local to the client
+	/* Normally we reject any write/sattr etc access on a read-only file
+	 * system.  But if it is IRIX doing check on write-access for a 
+	 * device special file, we ignore rofs.
 	 */
-	if (S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode)) 
+	if (!(acc & MAY_LOCAL_ACCESS))
 		if (acc & (MAY_WRITE | MAY_SATTR | MAY_TRUNC)) {
 			if (EX_RDONLY(exp) || IS_RDONLY(inode))
 				return nfserr_rofs;

diff ./include/linux/nfsd/nfsd.h~current~ ./include/linux/nfsd/nfsd.h
--- ./include/linux/nfsd/nfsd.h~current~	2003-01-08 11:18:25.000000000 +1100
+++ ./include/linux/nfsd/nfsd.h	2003-01-08 11:19:51.000000000 +1100
@@ -38,7 +38,8 @@
 #define MAY_TRUNC		16
 #define MAY_LOCK		32
 #define MAY_OWNER_OVERRIDE	64
-#if (MAY_SATTR | MAY_TRUNC | MAY_LOCK | MAX_OWNER_OVERRIDE) & (MAY_READ | MAY_WRITE | MAY_EXEC | MAY_OWNER_OVERRIDE)
+#define	MAY_LOCAL_ACCESS	128 /* IRIX doing local access check on device special file*/
+#if (MAY_SATTR | MAY_TRUNC | MAY_LOCK | MAX_OWNER_OVERRIDE | MAY_LOCAL_ACCESS) & (MAY_READ | MAY_WRITE | MAY_EXEC | MAY_OWNER_OVERRIDE)
 # error "please use a different value for MAY_SATTR or MAY_TRUNC or MAY_LOCK or MAY_OWNER_OVERRIDE."
 #endif
 #define MAY_CREATE		(MAY_EXEC|MAY_WRITE)