login
Search
Mostly saves some code size and cleans up the code after Roland's recent changes. Please ack or nack. -Andi --
- Replace remote_llseek with generic_file_llseek_unlocked (to force compilation
failures in all users)
- Change all users to either use generic_file_llseek_unlocked directly or
take the BKL around. I changed the file systems who don't use the BKL
for anything (CIFS, GFS) to call it directly. NCPFS and SMBFS and NFS
take the BKL, but explicitely in their own source now.
I moved them all over in a single patch to avoid unbisectable sections.
Open problem: 32bit kernels can corrupt fpos because its modification
is not atomic, but they can do that anyways because there's other paths who
modify it without BKL.
Do we need a special lock for the pos/f_version = 0 checks?
Trond says the NFS BKL is likely not needed, but keep it for now
until his full audit.
v2: Use generic_file_llseek_unlocked instead of remote_llseek_unlocked
and factor duplicated code (suggested by hch)
Cc: Trond.Myklebust@netapp.com
Cc: swhiteho@redhat.com
Cc: sfrench@samba.org
Cc: vandrove@vc.cvut.cz
Signed-off-by: Andi Kleen <ak@suse.de>
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
fs/cifs/cifsfs.c | 2 +-
fs/gfs2/ops_file.c | 4 ++--
fs/ncpfs/file.c | 12 +++++++++++-
fs/nfs/file.c | 6 +++++-
fs/read_write.c | 38 +++++++++++---------------------------
fs/smbfs/file.c | 11 ++++++++++-
include/linux/fs.h | 3 ++-
7 files changed, 42 insertions(+), 34 deletions(-)
Index: linux/fs/cifs/cifsfs.c
===================================================================
--- linux.orig/fs/cifs/cifsfs.c
+++ linux/fs/cifs/cifsfs.c
@@ -581,7 +581,7 @@ static loff_t cifs_llseek(struct file *f
if (retval < 0)
return (loff_t)retval;
}
- return remote_llseek(file, offset, origin);
+ return generic_file_llseek_unlocked(file, offset, origin);
}
struct file_system_type cifs_fs_type = {
Index: linux/fs/gfs2/ops_file.c
===================================================================
--- linux.orig/fs/gfs2/ops_file.c
+++ ...Doesn't need BKL because it just uses fasync_helper and minor->dev is protected by the file reference count. Cc: airlied@linux.ie Signed-off-by: Andi Kleen <ak@linux.intel.com> --- drivers/char/drm/i810_dma.c | 2 +- drivers/char/drm/i810_drv.c | 2 +- drivers/char/drm/i830_dma.c | 2 +- drivers/char/drm/i830_drv.c | 2 +- drivers/char/drm/i915_drv.c | 2 +- drivers/char/drm/mga_drv.c | 2 +- drivers/char/drm/r128_drv.c | 2 +- drivers/char/drm/radeon_drv.c | 2 +- drivers/char/drm/savage_drv.c | 2 +- drivers/char/drm/sis_drv.c | 2 +- drivers/char/drm/tdfx_drv.c | 2 +- drivers/char/drm/via_drv.c | 2 +- 12 files changed, 12 insertions(+), 12 deletions(-) Index: linux/drivers/char/drm/i810_dma.c =================================================================== --- linux.orig/drivers/char/drm/i810_dma.c +++ linux/drivers/char/drm/i810_dma.c @@ -117,7 +117,7 @@ static const struct file_operations i810 .release = drm_release, .ioctl = drm_ioctl, .mmap = i810_mmap_buffers, - .fasync = drm_fasync, + .unlocked_fasync = drm_fasync, }; static int i810_map_buffer(struct drm_buf * buf, struct drm_file *file_priv) Index: linux/drivers/char/drm/i810_drv.c =================================================================== --- linux.orig/drivers/char/drm/i810_drv.c +++ linux/drivers/char/drm/i810_drv.c @@ -62,7 +62,7 @@ static struct drm_driver driver = { .ioctl = drm_ioctl, .mmap = drm_mmap, .poll = drm_poll, - .fasync = drm_fasync, + .unlocked_fasync = drm_fasync, }, .pci_driver = { Index: linux/drivers/char/drm/i830_dma.c =================================================================== --- linux.orig/drivers/char/drm/i830_dma.c +++ linux/drivers/char/drm/i830_dma.c @@ -119,7 +119,7 @@ static const struct file_operations i830 .release = drm_release, .ioctl = drm_ioctl, .mmap = i830_mmap_buffers, - .fasync = drm_fasync, + .unlocked_fasync = drm_fasync, ...
Acked-by: Dave Airlie <airlied@linux.ie> unless you want me to take it via my tree. --
Just calls fasync_helper and private structure is protected by file reference count. Cc: clemens@ladisch.de Signed-off-by: Andi Kleen <ak@linux.intel.com> --- drivers/char/hpet.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Index: linux/drivers/char/hpet.c =================================================================== --- linux.orig/drivers/char/hpet.c +++ linux/drivers/char/hpet.c @@ -585,7 +585,7 @@ static const struct file_operations hpet .ioctl = hpet_ioctl, .open = hpet_open, .release = hpet_release, - .fasync = hpet_fasync, + .unlocked_fasync = hpet_fasync, .mmap = hpet_mmap, }; --
Acked-by: Clemens Ladisch <clemens@ladisch.de> --
Just calls fasync_helper Cc: hjk@linutronix.de --- drivers/uio/uio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Index: linux/drivers/uio/uio.c =================================================================== --- linux.orig/drivers/uio/uio.c +++ linux/drivers/uio/uio.c @@ -541,7 +541,7 @@ static const struct file_operations uio_ .read = uio_read, .mmap = uio_mmap, .poll = uio_poll, - .fasync = uio_fasync, + .unlocked_fasync = uio_fasync, }; static int uio_major_init(void) --
Signed-off-by: Andi Kleen <ak@suse.de>
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
fs/pipe.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
Index: linux/fs/pipe.c
===================================================================
--- linux.orig/fs/pipe.c
+++ linux/fs/pipe.c
@@ -787,7 +787,7 @@ const struct file_operations read_fifo_f
.unlocked_ioctl = pipe_ioctl,
.open = pipe_read_open,
.release = pipe_read_release,
- .fasync = pipe_read_fasync,
+ .unlocked_fasync = pipe_read_fasync,
};
const struct file_operations write_fifo_fops = {
@@ -799,7 +799,7 @@ const struct file_operations write_fifo_
.unlocked_ioctl = pipe_ioctl,
.open = pipe_write_open,
.release = pipe_write_release,
- .fasync = pipe_write_fasync,
+ .unlocked_fasync = pipe_write_fasync,
};
const struct file_operations rdwr_fifo_fops = {
@@ -812,7 +812,7 @@ const struct file_operations rdwr_fifo_f
.unlocked_ioctl = pipe_ioctl,
.open = pipe_rdwr_open,
.release = pipe_rdwr_release,
- .fasync = pipe_rdwr_fasync,
+ .unlocked_fasync = pipe_rdwr_fasync,
};
static const struct file_operations read_pipe_fops = {
@@ -824,7 +824,7 @@ static const struct file_operations read
.unlocked_ioctl = pipe_ioctl,
.open = pipe_read_open,
.release = pipe_read_release,
- .fasync = pipe_read_fasync,
+ .unlocked_fasync = pipe_read_fasync,
};
static const struct file_operations write_pipe_fops = {
@@ -836,7 +836,7 @@ static const struct file_operations writ
.unlocked_ioctl = pipe_ioctl,
.open = pipe_write_open,
.release = pipe_write_release,
- .fasync = pipe_write_fasync,
+ .unlocked_fasync = pipe_write_fasync,
};
static const struct file_operations rdwr_pipe_fops = {
@@ -849,7 +849,7 @@ static const struct file_operations rdwr
.unlocked_ioctl = pipe_ioctl,
.open = pipe_rdwr_open,
.release = pipe_rdwr_release,
- .fasync = pipe_rdwr_fasync,
+ .unlocked_fasync = pipe_rdwr_fasync,
};
struct pipe_inode_info * ...Add a new fops entry point to allow fasync without BKL. While it's arguably
unclear this entry point is called often enough for it really matters
it was still relatively easy to do. And there are far less async users
in the tree than ioctls so it's likely they can be all converted
eventually and then the non unlocked async entry point could be dropped.
There was still the problem of the actual flags change being
protected against other setters of flags. Instead of using BKL
for this use the i_mutex now.
I also added a mutex_lock against one other flags change
that was lockless and could potentially lose updates.
There are a couple of potential problems I added comments about on.
Signed-off-by: Andi Kleen <ak@suse.de>
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
Documentation/filesystems/vfs.txt | 5 ++++-
fs/fcntl.c | 22 +++++++++++++++-------
fs/ioctl.c | 13 ++++++++++++-
include/linux/fs.h | 1 +
4 files changed, 32 insertions(+), 9 deletions(-)
Index: linux/fs/fcntl.c
===================================================================
--- linux.orig/fs/fcntl.c
+++ linux/fs/fcntl.c
@@ -227,18 +227,26 @@ static int setfl(int fd, struct file * f
if (error)
return error;
- lock_kernel();
+ /* AK: potential race here. Since test is unlocked fasync could
+ be called several times in a row with on. We hope the drivers
+ deal with it. */
if ((arg ^ filp->f_flags) & FASYNC) {
- if (filp->f_op && filp->f_op->fasync) {
- error = filp->f_op->fasync(fd, filp, (arg & FASYNC) != 0);
- if (error < 0)
- goto out;
+ int on = !!(arg & FASYNC);
+ if (filp->f_op && filp->f_op->unlocked_fasync)
+ error = filp->f_op->unlocked_fasync(fd, filp, on);
+ else if (filp->f_op && filp->f_op->fasync) {
+ lock_kernel();
+ error = filp->f_op->fasync(fd, filp, on);
+ unlock_kernel();
}
+ /* AK: no else error = -EINVAL here? */
+ if (error < 0)
+ return error;
...On Tue, 20 May 2008 17:28:43 +0200 (CEST) I (again) really don't like having another entry point for this... it'll stay around forever rather than doing this as one step and move on. --
Yes the goal is for it staying around forever, correct. And ->fasync() will go instead. Advantage is that out of tree drivers will be compile broken which I consider an advantage. Yes I know Linus said earlier that's not important to him, but in this case my standards are higher than his. Also BTW if you're that worried about the audit not getting finished then the result would be just that lots of lock_kernel()s would stay around. Hardly better. But cannot do that many drivers in one step. My goal is to just audit the remaining ones and then remove ->fasync() and unlocked_fasync stays. Will be hopefully not that far away, since fasync is relatively easy. The conversions are mostly for me to keep track which ones I audited. -Andi --
On Tue, 20 May 2008 20:30:06 +0200 I'd say it's just different standards. My concern is that the new API as long lived is ugly and not the right thing. I assume Linus and others have similar concerns, and weigh that over the "some obscure out of tree driver might break". --
What do you find ugly exactly? The prefix "unlocked_" ? Other than that there is no difference. Duplicated entry points are not great, but they will be going away. -Andi --
On Wed, 21 May 2008 01:35:21 +0200 yes exactly --
On Tue, 20 May 2008 08:58:30 -0700 Agreed entirely - all our unlocked_foo methods we got stuck with, all our push down everyone cases we got most cases promptly fixed and the other lock/unlocks show up clearly in grep and help embarass the maintainer/author 8) --
That's the goal actually to "get stuck with it" and ->fasync going away. -Andi --
I guess my one concern with this mirrors what other people have said: might not it be better to just push the BKL down into the fasync() implementations and avoid adding yet another file operation? A quick grep shows 43 drivers defining fasync() functions - and many of those use the same one. fs/ has a few more. Obnoxious but doable, unless there's something I'm missing? Thanks, jon --
See my reply to Arjan. While for complicated stuff pushing down first is better, fasync is not that complicated and I think my strategy with the new entry point, with the old one going away is better in this case. -Andi --
BKL held. so is that BKL must be held by the caller or this function --- ~Randy --
Not that it matters much, but it was easy. Signed-off-by: Andi Kleen <ak@suse.de> Signed-off-by: Andi Kleen <ak@linux.intel.com> --- fs/bad_inode.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Index: linux/fs/bad_inode.c =================================================================== --- linux.orig/fs/bad_inode.c +++ linux/fs/bad_inode.c @@ -174,7 +174,7 @@ static const struct file_operations bad_ .release = bad_file_release, .fsync = bad_file_fsync, .aio_fsync = bad_file_aio_fsync, - .fasync = bad_file_fasync, + .unlocked_fasync = bad_file_fasync, .lock = bad_file_lock, .sendpage = bad_file_sendpage, .get_unmapped_area = bad_file_get_unmapped_area, --
Pretty sure the socket layer has no BKL requirements. Cc: davem@davemloft.net Signed-off-by: Andi Kleen <ak@suse.de> Signed-off-by: Andi Kleen <ak@linux.intel.com> --- net/socket.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Index: linux/net/socket.c =================================================================== --- linux.orig/net/socket.c +++ linux/net/socket.c @@ -134,7 +134,7 @@ static const struct file_operations sock .mmap = sock_mmap, .open = sock_no_open, /* special open code to disallow open via /proc */ .release = sock_close, - .fasync = sock_fasync, + .unlocked_fasync = sock_fasync, .sendpage = sock_sendpage, .splice_write = generic_splice_sendpage, .splice_read = sock_splice_read, --
Just uses fasync_helper which does its own locking Cc: mpm@selenic.com Signed-off-by: Andi Kleen <ak@linux.intel.com> --- drivers/char/random.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) Index: linux/drivers/char/random.c =================================================================== --- linux.orig/drivers/char/random.c +++ linux/drivers/char/random.c @@ -1121,7 +1121,7 @@ const struct file_operations random_fops .write = random_write, .poll = random_poll, .unlocked_ioctl = random_ioctl, - .fasync = random_fasync, + .unlocked_fasync = random_fasync, .release = random_release, }; @@ -1129,7 +1129,7 @@ const struct file_operations urandom_fop .read = urandom_read, .write = random_write, .unlocked_ioctl = random_ioctl, - .fasync = random_fasync, + .unlocked_fasync = random_fasync, .release = random_release, }; --
Cc: miklos@szeredi.hu Signed-off-by: Andi Kleen <ak@suse.de> Signed-off-by: Andi Kleen <ak@linux.intel.com> --- fs/fuse/dev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Index: linux/fs/fuse/dev.c =================================================================== --- linux.orig/fs/fuse/dev.c +++ linux/fs/fuse/dev.c @@ -1082,7 +1082,7 @@ const struct file_operations fuse_dev_op .aio_write = fuse_dev_write, .poll = fuse_dev_poll, .release = fuse_dev_release, - .fasync = fuse_dev_fasync, + .unlocked_fasync = fuse_dev_fasync, }; static struct miscdevice fuse_miscdevice = { --
Just uses fasync_helper
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
drivers/char/rtc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Index: linux/drivers/char/rtc.c
===================================================================
--- linux.orig/drivers/char/rtc.c
+++ linux/drivers/char/rtc.c
@@ -913,7 +913,7 @@ static const struct file_operations rtc_
.ioctl = rtc_ioctl,
.open = rtc_open,
.release = rtc_release,
- .fasync = rtc_fasync,
+ .unlocked_fasync = rtc_fasync,
};
static struct miscdevice rtc_dev = {
--
