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

["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. 

> 
> Is there any sense in allowing anybody to call name_to_handle() if
> open_by_handle() is restricted?
> 

I guess it should still be useful because handle can be passed around
and later given to a process that have enough permission to open by handle.

-aneesh