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    [from the cache]
©2003-2011 Jasper Spaans