lkml.org 
[lkml]   [2010]   [May]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 3/6] fat: BKL ioctl pushdown


    On Thu, 6 May 2010, OGAWA Hirofumi wrote:

    > John Kacur <jkacur@redhat.com> writes:
    >
    > > Convert fat_generic_ioctl and fat_dir_ioctl to unlocked_ioctls
    > > and push down the bkl into those functions.
    >
    > I guess this is the part of batch ioctl conversion stuff though, those
    > ioctl of FAT don't need BKL at all. Because all of those should already
    > be protected by inode->i_mutex.
    >
    > Removing BKL and then cleanup after this patch would be almost same with
    > reverting this patch. So, could you just convert to unlocked_ioctl
    > instead?

    That's probably not a good idea, without a little bit more analysis,
    otherwise it's quite easy to introduce subtle bugs.

    >
    > Thanks.
    >
    > > Signed-off-by: John Kacur <jkacur@redhat.com>
    > > ---
    > > fs/fat/dir.c | 36 ++++++++++++++++++++++++------------
    > > fs/fat/fat.h | 3 +--
    > > fs/fat/file.c | 22 ++++++++++++++++------
    > > 3 files changed, 41 insertions(+), 20 deletions(-)
    > >
    > > diff --git a/fs/fat/dir.c b/fs/fat/dir.c
    > > index 530b4ca..1b73e60 100644
    > > --- a/fs/fat/dir.c
    > > +++ b/fs/fat/dir.c
    > > @@ -19,6 +19,7 @@
    > > #include <linux/buffer_head.h>
    > > #include <linux/compat.h>
    > > #include <asm/uaccess.h>
    > > +#include <linux/smp_lock.h>
    > > #include "fat.h"
    > >
    > > /*
    > > @@ -737,10 +738,11 @@ efault: \
    > >
    > > FAT_IOCTL_FILLDIR_FUNC(fat_ioctl_filldir, __fat_dirent)
    > >
    > > -static int fat_ioctl_readdir(struct inode *inode, struct file *filp,
    > > +static long fat_ioctl_readdir(struct file *filp,
    > > void __user *dirent, filldir_t filldir,
    > > int short_only, int both)
    > > {
    > > + struct inode *inode = filp->f_dentry->d_inode;
    > > struct fat_ioctl_filldir_callback buf;
    > > int ret;
    > >
    > > @@ -758,9 +760,10 @@ static int fat_ioctl_readdir(struct inode *inode, struct file *filp,
    > > return ret;
    > > }
    > >
    > > -static int fat_dir_ioctl(struct inode *inode, struct file *filp,
    > > - unsigned int cmd, unsigned long arg)
    > > +static long fat_dir_ioctl(struct file *filp, unsigned int cmd,
    > > + unsigned long arg)
    > > {
    > > + long error;
    > > struct __fat_dirent __user *d1 = (struct __fat_dirent __user *)arg;
    > > int short_only, both;
    > >
    > > @@ -774,21 +777,31 @@ static int fat_dir_ioctl(struct inode *inode, struct file *filp,
    > > both = 1;
    > > break;
    > > default:
    > > - return fat_generic_ioctl(inode, filp, cmd, arg);
    > > + lock_kernel();
    > > + error = fat_generic_ioctl(filp, cmd, arg);
    > > + goto out;
    > > }
    > >
    > > - if (!access_ok(VERIFY_WRITE, d1, sizeof(struct __fat_dirent[2])))
    > > - return -EFAULT;
    > > + lock_kernel();
    > > + if (!access_ok(VERIFY_WRITE, d1, sizeof(struct __fat_dirent[2]))) {
    > > + error = -EFAULT;
    > > + goto out;
    > > + }
    > > /*
    > > * Yes, we don't need this put_user() absolutely. However old
    > > * code didn't return the right value. So, app use this value,
    > > * in order to check whether it is EOF.
    > > */
    > > - if (put_user(0, &d1->d_reclen))
    > > - return -EFAULT;
    > > + if (put_user(0, &d1->d_reclen)) {
    > > + error = -EFAULT;
    > > + goto out;
    > > + }
    > >
    > > - return fat_ioctl_readdir(inode, filp, d1, fat_ioctl_filldir,
    > > + error = fat_ioctl_readdir(filp, d1, fat_ioctl_filldir,
    > > short_only, both);
    > > +out:
    > > + unlock_kernel();
    > > + return error;
    > > }
    > >
    > > #ifdef CONFIG_COMPAT
    > > @@ -800,7 +813,6 @@ FAT_IOCTL_FILLDIR_FUNC(fat_compat_ioctl_filldir, compat_dirent)
    > > static long fat_compat_dir_ioctl(struct file *filp, unsigned cmd,
    > > unsigned long arg)
    > > {
    > > - struct inode *inode = filp->f_path.dentry->d_inode;
    > > struct compat_dirent __user *d1 = compat_ptr(arg);
    > > int short_only, both;
    > >
    > > @@ -827,7 +839,7 @@ static long fat_compat_dir_ioctl(struct file *filp, unsigned cmd,
    > > if (put_user(0, &d1->d_reclen))
    > > return -EFAULT;
    > >
    > > - return fat_ioctl_readdir(inode, filp, d1, fat_compat_ioctl_filldir,
    > > + return fat_ioctl_readdir(filp, d1, fat_compat_ioctl_filldir,
    > > short_only, both);
    > > }
    > > #endif /* CONFIG_COMPAT */
    > > @@ -836,7 +848,7 @@ const struct file_operations fat_dir_operations = {
    > > .llseek = generic_file_llseek,
    > > .read = generic_read_dir,
    > > .readdir = fat_readdir,
    > > - .ioctl = fat_dir_ioctl,
    > > + .unlocked_ioctl = fat_dir_ioctl,
    > > #ifdef CONFIG_COMPAT
    > > .compat_ioctl = fat_compat_dir_ioctl,
    > > #endif
    > > diff --git a/fs/fat/fat.h b/fs/fat/fat.h
    > > index e6efdfa..23f9b2a 100644
    > > --- a/fs/fat/fat.h
    > > +++ b/fs/fat/fat.h
    > > @@ -298,8 +298,7 @@ extern int fat_free_clusters(struct inode *inode, int cluster);
    > > extern int fat_count_free_clusters(struct super_block *sb);
    > >
    > > /* fat/file.c */
    > > -extern int fat_generic_ioctl(struct inode *inode, struct file *filp,
    > > - unsigned int cmd, unsigned long arg);
    > > +extern long fat_generic_ioctl(struct file *filp, unsigned int cmd, unsigned long arg);
    > > extern const struct file_operations fat_file_operations;
    > > extern const struct inode_operations fat_file_inode_operations;
    > > extern int fat_setattr(struct dentry * dentry, struct iattr * attr);
    > > diff --git a/fs/fat/file.c b/fs/fat/file.c
    > > index e8c159d..4f3044f 100644
    > > --- a/fs/fat/file.c
    > > +++ b/fs/fat/file.c
    > > @@ -16,6 +16,7 @@
    > > #include <linux/blkdev.h>
    > > #include <linux/fsnotify.h>
    > > #include <linux/security.h>
    > > +#include <linux/smp_lock.h>
    > > #include "fat.h"
    > >
    > > static int fat_ioctl_get_attributes(struct inode *inode, u32 __user *user_attr)
    > > @@ -114,19 +115,28 @@ out:
    > > return err;
    > > }
    > >
    > > -int fat_generic_ioctl(struct inode *inode, struct file *filp,
    > > - unsigned int cmd, unsigned long arg)
    > > +long fat_generic_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
    > > {
    > > + long error;
    > > + struct inode *inode = filp->f_dentry->d_inode;
    > > u32 __user *user_attr = (u32 __user *)arg;
    > >
    > > + lock_kernel();
    > > +
    > > switch (cmd) {
    > > case FAT_IOCTL_GET_ATTRIBUTES:
    > > - return fat_ioctl_get_attributes(inode, user_attr);
    > > + error = fat_ioctl_get_attributes(inode, user_attr);
    > > + break;
    > > case FAT_IOCTL_SET_ATTRIBUTES:
    > > - return fat_ioctl_set_attributes(filp, user_attr);
    > > + error = fat_ioctl_set_attributes(filp, user_attr);
    > > + break;
    > > default:
    > > - return -ENOTTY; /* Inappropriate ioctl for device */
    > > + error = -ENOTTY; /* Inappropriate ioctl for device */
    > > + break;
    > > }
    > > +
    > > + unlock_kernel();
    > > + return error;
    > > }
    > >
    > > static int fat_file_release(struct inode *inode, struct file *filp)
    > > @@ -159,7 +169,7 @@ const struct file_operations fat_file_operations = {
    > > .aio_write = generic_file_aio_write,
    > > .mmap = generic_file_mmap,
    > > .release = fat_file_release,
    > > - .ioctl = fat_generic_ioctl,
    > > + .unlocked_ioctl = fat_generic_ioctl,
    > > .fsync = fat_file_fsync,
    > > .splice_read = generic_file_splice_read,
    > > };
    >
    > --
    > OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
    > --
    > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    > the body of a message to majordomo@vger.kernel.org
    > More majordomo info at http://vger.kernel.org/majordomo-info.html
    > Please read the FAQ at http://www.tux.org/lkml/
    >


    \
     
     \ /
      Last update: 2010-05-05 19:37    [W:0.035 / U:89.632 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site