Re: [PATCH -V5 2/8] vfs: Add name to file handle conversion support

[Neil Brown <neilb@suse.de>]

On Mon, 26 Apr 2010 23:04:00 +0530
"Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> wrote:

> Acked-by: Serge Hallyn <serue@us.ibm.com>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> ---
>  fs/open.c          |  100 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/fs.h |   17 +++++++++
>  2 files changed, 117 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/open.c b/fs/open.c
> index 74e5cd9..e9aae5c 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -30,6 +30,8 @@
>  #include <linux/falloc.h>
>  #include <linux/fs_struct.h>
>  #include <linux/ima.h>
> +#include <linux/exportfs.h>
> +#include <linux/mnt_namespace.h>
>  
>  #include "internal.h"
>  
> @@ -1206,3 +1208,101 @@ int nonseekable_open(struct inode *inode, struct file *filp)
>  }
>  
>  EXPORT_SYMBOL(nonseekable_open);
> +
> +/* limit the handle size to some value */
> +#define MAX_HANDLE_SZ 4096
> +static long do_sys_name_to_handle(struct path *path,
> +			struct file_handle __user *ufh)
> +{
> +	int retval;
> +	int handle_size;
> +	struct super_block *sb;
> +	struct uuid *this_fs_id;
> +	struct file_handle f_handle;
> +	struct file_handle *handle = NULL;
> +
> +	if (copy_from_user(&f_handle, ufh, sizeof(struct file_handle)))
> +		return -EFAULT;
> +
> +	if (f_handle.handle_size > MAX_HANDLE_SZ)
> +		return -EINVAL;
> +
> +	handle = kmalloc(sizeof(struct file_handle) + f_handle.handle_size,
> +			GFP_KERNEL);
> +	if (!handle) {
> +		retval = -ENOMEM;
> +		goto err_out;
> +	}

You are mixing your patterns here.
In some cases you do:
   if (something fails)
           return -ERROR
other times you do

   if (something fails) {
            retval = -ERROR;
            goto err_out;
   }

which has exactly the same fix.  Being consistent is best and I would
recommend using the goto option - it is more maintainable.
I would actually prefer:

    retval = -ERROR;
    if (something fails)
          goto err_out;

but leave the choice to you.  Consistency is the important thing.



> +	handle_size = f_handle.handle_size;
> +
> +	/* we ask for a non connected handle */
> +	retval = exportfs_encode_fh(path->dentry,
> +				(struct fid *)handle->f_handle,
> +				&handle_size,  0);
> +	/* convert handle size to bytes */
> +	handle_size *= sizeof(u32);
> +	handle->handle_type = retval;
> +	handle->handle_size = handle_size;
> +	if (handle_size <= f_handle.handle_size) {
> +		/* get the uuid */
> +		sb = path->mnt->mnt_sb;
> +		if (sb->s_op->get_fsid) {
> +			this_fs_id = sb->s_op->get_fsid(sb);
> +			memcpy(handle->fsid.uuid, this_fs_id->uuid,
> +				sizeof(handle->fsid.uuid));
> +		} else
> +			memset(handle->fsid.uuid, 0, sizeof(handle->fsid.uuid));
> +		retval = 0;
> +	} else {
> +		/*
> +		 * set the handle_size to zero so we copy only
> +		 * non variable part of the file_handle
> +		 */
> +		handle_size = 0;
> +		retval = -EOVERFLOW;
> +	}
> +	if (copy_to_user(ufh, handle,
> +				sizeof(struct file_handle) + handle_size))
> +		retval = -EFAULT;
> +
> +	kfree(handle);
> +err_out:
> +	return retval;
> +}
> +
> +SYSCALL_DEFINE2(name_to_handle, const char __user *, name,
> +		struct file_handle __user *, handle)
> +{
> +	long ret;
> +	struct path path;

*Surely* you want name_to_handle_at, or name_at_to_handle here.
i.e. include an int 'dfd' to specify the starting directory.

Then you can make use of AT_SYMLINK_NOFOLLOW and so avoid the need for
2 syscalls.


> +
> +	/* Follow links */
> +	ret = user_path(name, &path);
> +	if (ret)
> +		return ret;
> +
> +	ret = do_sys_name_to_handle(&path, handle);
> +	path_put(&path);
> +
> +	/* avoid REGPARM breakage on x86: */
> +	asmlinkage_protect(2, ret, name, handle);
> +	return ret;
> +}
> +
> +SYSCALL_DEFINE2(lname_to_handle, const char __user *, name,
> +		struct file_handle __user *, handle)
> +{
> +	long ret;
> +	struct path path;
> +
> +	ret = user_lpath(name, &path);
> +	if (ret)
> +		return ret;
> +
> +	ret = do_sys_name_to_handle(&path, handle);
> +	path_put(&path);
> +
> +	/* avoid REGPARM breakage on x86: */
> +	asmlinkage_protect(2, ret, name, handle);
> +	return ret;
> +}
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 39d57bc..da28b80 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -948,6 +948,20 @@ struct file {
>  	unsigned long f_mnt_write_state;
>  #endif
>  };
> +
> +struct uuid {
> +	char uuid[16];

unsigned char??? just a thought.


> +};
> +
> +struct file_handle {
> +	int handle_size;
> +	int handle_type;
> +	/* File system identifier */
> +	struct uuid fsid;
> +	/* file identifier */
> +	char f_handle[0];
> +};
> +
>  extern spinlock_t files_lock;
>  #define file_list_lock() spin_lock(&files_lock);
>  #define file_list_unlock() spin_unlock(&files_lock);
> @@ -1580,6 +1594,7 @@ struct super_operations {
>  	ssize_t (*quota_write)(struct super_block *, int, const char *, size_t, loff_t);
>  #endif
>  	int (*bdev_try_to_free_page)(struct super_block*, struct page*, gfp_t);
> +	struct uuid *(*get_fsid)(struct super_block *);

Assuming this was actually a good idea (which I am not yet sold on),
I think you would do better to pass in the address of a uuid to be filled in:

       int (*get_fsid)(struct super_block *, struct uuid *)

just incase the filesystem stored something in a slightly different format.

And as XFS has a 'nouuid' mount option, it might be best to allow an error
return to say "there is no uuid" - maybe expect -ENOENT ??

NeilBrown

>  };
>  
>  /*
> @@ -2328,6 +2343,8 @@ extern struct super_block *get_super(struct block_device *);
>  extern struct super_block *get_active_super(struct block_device *bdev);
>  extern struct super_block *user_get_super(dev_t);
>  extern void drop_super(struct super_block *sb);
> +extern struct super_block *fs_get_sb(struct uuid *fsid);
> +
>  
>  extern int dcache_dir_open(struct inode *, struct file *);
>  extern int dcache_dir_close(struct inode *, struct file *);