On Mon, 26 Apr 2010 23:04:00 +0530
"Aneesh Kumar K.V" <aneesh.kumar@xxxxxxxxxxxxxxxxxx> wrote:
> Acked-by: Serge Hallyn <serue@xxxxxxxxxx>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@xxxxxxxxxxxxxxxxxx>
> ---
> 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 *);
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
