Re: [RFC][PATCH 22/27] sys_mknodat(): elevate write count for vfs_mknod/create()

[Dave Hansen <haveblue@us.ibm.com>]

On Thu, 2006-06-08 at 13:16 +0200, Herbert Poetzl wrote:
> On Wed, Jun 07, 2006 at 05:10:35PM -0700, Dave Hansen wrote:
> > This takes care of all of the direct callers of vfs_mknod().
> > Since a few of these cases also handle normal file creation
> > as well, this also covers some calls to vfs_create().
> > 
> > Signed-off-by: Dave Hansen <haveblue@us.ibm.com>
> > ---
> > 
> >  ipc/mqueue.c                |    0 
> >  lxc-dave/fs/namei.c         |   48 ++++++++++++++++++++++++++++----------------
> >  lxc-dave/fs/nfsd/vfs.c      |    4 +++
> >  lxc-dave/net/unix/af_unix.c |    4 +++
> >  4 files changed, 39 insertions(+), 17 deletions(-)
> > 
> > diff -puN fs/namei.c~elevate-writers-vfs_mknod-try2 fs/namei.c
> > --- lxc/fs/namei.c~elevate-writers-vfs_mknod-try2	2006-06-07 16:53:25.000000000 -0700
> > +++ lxc-dave/fs/namei.c	2006-06-07 16:53:25.000000000 -0700
> > @@ -1852,26 +1852,40 @@ asmlinkage long sys_mknodat(int dfd, con
> >  
> >  	if (!IS_POSIXACL(nd.dentry->d_inode))
> >  		mode &= ~current->fs->umask;
> > -	if (!IS_ERR(dentry)) {
> > -		switch (mode & S_IFMT) {
> > -		case 0: case S_IFREG:
> > -			error = vfs_create(nd.dentry->d_inode,dentry,mode,&nd);
> > -			break;
> > -		case S_IFCHR: case S_IFBLK:
> > -			error = vfs_mknod(nd.dentry->d_inode,dentry,mode,
> > -					new_decode_dev(dev));
> > -			break;
> > -		case S_IFIFO: case S_IFSOCK:
> > -			error = vfs_mknod(nd.dentry->d_inode,dentry,mode,0);
> > +	if (IS_ERR(dentry))
> > +		goto out_unlock;
> > +	error = mnt_want_write(nd.mnt);
> > +	if (error)
> > +		goto out_dput;
> > +	switch (mode & S_IFMT) {
> > +	case 0: case S_IFREG:
> > +		error = vfs_create(nd.dentry->d_inode,dentry,mode,&nd);
> > +		break;
> > +	case S_IFCHR: case S_IFBLK:
> 
> hmm, could you outline some cases where the following 
> fails, but the previous (above) check succeeded?
> (except for dubious locking issues)

Are you asking about the vfs_create() call, or the second
mnt_want_write() call?

I appear to have munged two patches together here.  One which elevates
the count over the entire switch(), and the other which does the
elevation only around the actual vfs_*() calls.  It should only have the
lower-level calls.

> > +		error = mnt_want_write(nd.mnt);
> > +		if (error)
> >  			break;
> > -		case S_IFDIR:
> > -			error = -EPERM;
> > +		error = vfs_mknod(nd.dentry->d_inode,dentry,mode,
> > +				new_decode_dev(dev));
> > +		mnt_drop_write(nd.mnt);
> > +		break;
> > +	case S_IFIFO: case S_IFSOCK:
> 
> same here
> 
> > +		error = mnt_want_write(nd.mnt);
> > +		if (error)
> >  			break;
> > -		default:
> > -			error = -EINVAL;
> > -		}
> > -		dput(dentry);
> > +		error = vfs_mknod(nd.dentry->d_inode,dentry,mode,0);
> > +		mnt_drop_write(nd.mnt);
> > +		break;
> > +	case S_IFDIR:
> > +		error = -EPERM;
> 
> this will give -EPERM on a r/w filesystem, but
> -EROFS on r/o, is that consistant with the current
> behaviour?

Nope.  That's a bug, I think.  I'll fix it up.

> > +		break;
> > +	default:
> > +		error = -EINVAL;
> 
> same here for -EINVAL and -EROFS, maybe the 
> mnt_want_write() wants to go into vfs_mknod()?

I think I tried that once, and it didn't quite work out.  In this case,
I think just giving vfs_mknod() the same treatment that I gave
vfs_create() should do for now.

-- Dave