From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Aneesh Kumar K. V" Subject: Re: [RFC PATCH] Generic name to handle and open by handle syscalls Date: Tue, 23 Feb 2010 14:28:36 +0530 Message-ID: <87ocjgib0j.fsf@linux.vnet.ibm.com> References: <1266558149-11460-1-git-send-email-aneesh.kumar@linux.vnet.ibm.com> <20100222160659.48a91f82@bike.lwn.net> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: hch@infradead.org, viro@zeniv.linux.org.uk, linux-fsdevel@vger.kernel.org To: Jonathan Corbet Return-path: Received: from e28smtp02.in.ibm.com ([122.248.162.2]:43869 "EHLO e28smtp02.in.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751254Ab0BWI6n (ORCPT ); Tue, 23 Feb 2010 03:58:43 -0500 Received: from d28relay05.in.ibm.com (d28relay05.in.ibm.com [9.184.220.62]) by e28smtp02.in.ibm.com (8.14.3/8.13.1) with ESMTP id o1N8wbdu008144 for ; Tue, 23 Feb 2010 14:28:37 +0530 Received: from d28av04.in.ibm.com (d28av04.in.ibm.com [9.184.220.66]) by d28relay05.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id o1N8wb012646264 for ; Tue, 23 Feb 2010 14:28:37 +0530 Received: from d28av04.in.ibm.com (loopback [127.0.0.1]) by d28av04.in.ibm.com (8.14.3/8.13.1/NCO v10.0 AVout) with ESMTP id o1N8wa4m024218 for ; Tue, 23 Feb 2010 19:58:37 +1100 In-Reply-To: <20100222160659.48a91f82@bike.lwn.net> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Mon, 22 Feb 2010 16:06:59 -0700, Jonathan Corbet wrote: > On Fri, 19 Feb 2010 11:12:26 +0530 > "Aneesh Kumar K.V" 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