Re: [RFC PATCH] Generic name to handle and open by handle syscalls

["Serge E. Hallyn" <serue@us.ibm.com>]

Quoting Aneesh Kumar K. V (aneesh.kumar@linux.vnet.ibm.com):
> On Mon, 22 Feb 2010 16:06:59 -0700, Jonathan Corbet <corbet@lwn.net> wrote:
> > On Fri, 19 Feb 2010 11:12:26 +0530
> > "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> wrote:
> > 
> > > The below set of patches implement open by handle support using exportfs
> > > operations.
> > 
> > I have a couple of questions...starting with: what is the use case for
> > this functionality?  There must, clearly, be some kind of application
> > which needs to be able to open by file handle, but I'm not sure what
> > that would be.
> >
> 
> User space NFS server would be one example. Also if we want to NFS
> export another network file system which have a user space server, that
> would be another reason.
> 
> > I agree with Andreas that the handle length looks like a bit of a
> > crapshoot.  How should an application developer know how much memory to
> > dedicate to this?
> > 
> > In do_sys_name_to_handle() you have:
> > 
> > > +	f_handle = kmalloc(handle_size, GFP_KERNEL);
> > 
> > handle_size comes directly from user space.  Perhaps it should be
> > sanity-checked?  (I would also just use handle->handle_size; later
> > handle_size contains the *real* handle size, and I found that confusing.
> > Yes, I'm easily confused, but...)
> 
> I already had a FIXME is another patch to sanity check the handle
> size. I was not sure what should be maximum size. Andreas suggested 4096
> So i will update the patch in the next iteration with that value. I will
> also update the variable names as you suggested above.
> 
> > 
> > > +	handle->handle_type = retval;
> > > +	if (handle_size < handle->handle_size) {
> > > +		if (copy_to_user(handle->f_handle, f_handle,
> > > +					handle_size*sizeof(u32)))
> > > +			retval = -EFAULT;
> > > +		retval = 0;
> > > +	} else
> > > +		retval = -EAGAIN;
> > > +	handle->handle_size = handle_size;
> > 
> > EAGAIN seems like a strange thing to return here.  ENOSPC maybe?
> 
> The reason for me using EAGAIN was to give a hint that if the user
> reissue the call with new returned handle_size we should be ok. But
> Andreas suggested EOVERFLOW. So i will use EOVERFLOW. Do you think
> ENOSPC is he right error value here ?
> 
> > 
> > Are you missing an "else" before the "retval = 0;" line?  You'll never
> > return -EFAULT here.
> > 
> 
> good catch. Will fix in the next iteration
> 
> 
> > In do_sys_open_by_handle():
> > 
> > > +	if (!capable(CAP_SYS_ADMIN))
> > > +		/* Allow open by handle only by sysadmin */
> > > +		return -EPERM;
> > 
> > I assume this is to avoid access to readable files within unreadable
> > directories?   Otherwise you could check the permissions of the target
> > dentry.
> > 
> > CAP_SYS_ADMIN seems like the wrong capability, though.  CAP_DAC_OVERRIDE
> > might make more sense?
> 
> I guess CAP_DAC_OVERRIDE should be enough here. The CAP_SYS_ADMIN
> restriction came from the xfs ioctl version. 

I'd be curious to see the reasons for requiring it in the xfs version.
Do you have any docs about it?  You're still doing a dentry_open, and
you got the filename fd somehow so the name shouldn't be a secret...
An LSM hook - specifically to make sure that selinux still allows you
to read the path (access to file->f_security) - might belong here, but
I dunno about the capable check.

thanks,
-serge