fat: BKL ioctl pushdown
in [Kernel]
|
Prev: ramzswap: Eliminate stale data in compressed memory
Next: tty: make termios mutex nest under tty_lock
From: OGAWA Hirofumi on 5 May 2010 12:10 John Kacur <jkacur(a)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? Thanks. > Signed-off-by: John Kacur <jkacur(a)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(a)mail.parknet.co.jp> -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: John Kacur on 5 May 2010 13:40 On Thu, 6 May 2010, OGAWA Hirofumi wrote: > John Kacur <jkacur(a)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(a)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(a)mail.parknet.co.jp> > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo(a)vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: OGAWA Hirofumi on 5 May 2010 14:40 John Kacur <jkacur(a)redhat.com> writes: > On Thu, 6 May 2010, OGAWA Hirofumi wrote: > >> John Kacur <jkacur(a)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. What analysis? Who do it? I thought about removing BKL of FAT from several years ago. I was reviewing FAT multiple times, and I'm always testing FAT without BKL. If you are going to do, could you do it instead of this patch? Thanks. -- OGAWA Hirofumi <hirofumi(a)mail.parknet.co.jp> -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: John Kacur on 5 May 2010 16:20
On Thu, 6 May 2010, OGAWA Hirofumi wrote: > John Kacur <jkacur(a)redhat.com> writes: > > > On Thu, 6 May 2010, OGAWA Hirofumi wrote: > > > >> John Kacur <jkacur(a)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. > > What analysis? Who do it? I thought about removing BKL of FAT from > several years ago. I was reviewing FAT multiple times, and I'm always > testing FAT without BKL. This patch just makes explicit the hidden BKLs that FAT is already using related to ioctl. We would like to get this step done in time for the next merge window, because then we can remove that hidden source. This step is preparation for removing the BKL from the individual functions we've pushed it down into. In other words, we'll do the analysis to remove it later if you don't want to. Thanks. > > If you are going to do, could you do it instead of this patch? > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |