On Tue, 27 Apr 2010 11:19:49 +1000, Neil Brown <neilb@xxxxxxx> wrote:
> 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.
Fixed as per you suggestion.
>
>
>
> > + 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.
>
Done. Droped the lname_to_handle syscall
>
> > +
> > + /* 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.
>
Fixed
>
> > +};
> > +
> > +struct file_handle {
> > + int handle_size;
> > + int handle_type;
> > + /* File system identifier */
> > + struct uuid fsid;
> > + /* file identifier */
> > + char f_handle[0];
I also changed this to unsigned 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 ??
Fixed
Thanks for the review
-aneesh
--
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
