Skip to content

Commit

Permalink
Remove BKL from remote_llseek v2
Browse files Browse the repository at this point in the history
- 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>
Signed-off-by: Jonathan Corbet <corbet@lwn.net>
  • Loading branch information
Andi Kleen authored and Jonathan Corbet committed Jul 2, 2008
1 parent 9c20616 commit 9465efc
Show file tree
Hide file tree
Showing 7 changed files with 42 additions and 34 deletions.
2 changes: 1 addition & 1 deletion fs/cifs/cifsfs.c
Original file line number Diff line number Diff line change
Expand Up @@ -581,7 +581,7 @@ static loff_t cifs_llseek(struct file *file, loff_t offset, int origin)
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 = {
Expand Down
4 changes: 2 additions & 2 deletions fs/gfs2/ops_file.c
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,11 @@ static loff_t gfs2_llseek(struct file *file, loff_t offset, int origin)
error = gfs2_glock_nq_init(ip->i_gl, LM_ST_SHARED, LM_FLAG_ANY,
&i_gh);
if (!error) {
error = remote_llseek(file, offset, origin);
error = generic_file_llseek_unlocked(file, offset, origin);
gfs2_glock_dq_uninit(&i_gh);
}
} else
error = remote_llseek(file, offset, origin);
error = generic_file_llseek_unlocked(file, offset, origin);

return error;
}
Expand Down
12 changes: 11 additions & 1 deletion fs/ncpfs/file.c
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include <linux/slab.h>
#include <linux/vmalloc.h>
#include <linux/sched.h>
#include <linux/smp_lock.h>

#include <linux/ncp_fs.h>
#include "ncplib_kernel.h"
Expand Down Expand Up @@ -281,9 +282,18 @@ static int ncp_release(struct inode *inode, struct file *file) {
return 0;
}

static loff_t ncp_remote_llseek(struct file *file, loff_t offset, int origin)
{
loff_t ret;
lock_kernel();
ret = generic_file_llseek_unlocked(file, offset, origin);
unlock_kernel();
return ret;
}

const struct file_operations ncp_file_operations =
{
.llseek = remote_llseek,
.llseek = ncp_remote_llseek,
.read = ncp_file_read,
.write = ncp_file_write,
.ioctl = ncp_ioctl,
Expand Down
6 changes: 5 additions & 1 deletion fs/nfs/file.c
Original file line number Diff line number Diff line change
Expand Up @@ -170,14 +170,18 @@ static int nfs_revalidate_file_size(struct inode *inode, struct file *filp)

static loff_t nfs_file_llseek(struct file *filp, loff_t offset, int origin)
{
loff_t loff;
/* origin == SEEK_END => we must revalidate the cached file length */
if (origin == SEEK_END) {
struct inode *inode = filp->f_mapping->host;
int retval = nfs_revalidate_file_size(inode, filp);
if (retval < 0)
return (loff_t)retval;
}
return remote_llseek(filp, offset, origin);
lock_kernel(); /* BKL needed? */
loff = generic_file_llseek_unlocked(filp, offset, origin);
unlock_kernel();
return loff;
}

/*
Expand Down
38 changes: 11 additions & 27 deletions fs/read_write.c
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,12 @@ const struct file_operations generic_ro_fops = {

EXPORT_SYMBOL(generic_ro_fops);

loff_t generic_file_llseek(struct file *file, loff_t offset, int origin)
loff_t
generic_file_llseek_unlocked(struct file *file, loff_t offset, int origin)
{
loff_t retval;
struct inode *inode = file->f_mapping->host;

mutex_lock(&inode->i_mutex);
switch (origin) {
case SEEK_END:
offset += inode->i_size;
Expand All @@ -46,42 +46,26 @@ loff_t generic_file_llseek(struct file *file, loff_t offset, int origin)
}
retval = -EINVAL;
if (offset>=0 && offset<=inode->i_sb->s_maxbytes) {
/* Special lock needed here? */
if (offset != file->f_pos) {
file->f_pos = offset;
file->f_version = 0;
}
retval = offset;
}
mutex_unlock(&inode->i_mutex);
return retval;
}
EXPORT_SYMBOL(generic_file_llseek_unlocked);

EXPORT_SYMBOL(generic_file_llseek);

loff_t remote_llseek(struct file *file, loff_t offset, int origin)
loff_t generic_file_llseek(struct file *file, loff_t offset, int origin)
{
loff_t retval;

lock_kernel();
switch (origin) {
case SEEK_END:
offset += i_size_read(file->f_path.dentry->d_inode);
break;
case SEEK_CUR:
offset += file->f_pos;
}
retval = -EINVAL;
if (offset>=0 && offset<=file->f_path.dentry->d_inode->i_sb->s_maxbytes) {
if (offset != file->f_pos) {
file->f_pos = offset;
file->f_version = 0;
}
retval = offset;
}
unlock_kernel();
return retval;
loff_t n;
mutex_lock(&file->f_dentry->d_inode->i_mutex);
n = generic_file_llseek_unlocked(file, offset, origin);
mutex_unlock(&file->f_dentry->d_inode->i_mutex);
return n;
}
EXPORT_SYMBOL(remote_llseek);
EXPORT_SYMBOL(generic_file_llseek);

loff_t no_llseek(struct file *file, loff_t offset, int origin)
{
Expand Down
11 changes: 10 additions & 1 deletion fs/smbfs/file.c
Original file line number Diff line number Diff line change
Expand Up @@ -422,9 +422,18 @@ smb_file_permission(struct inode *inode, int mask, struct nameidata *nd)
return error;
}

static loff_t smb_remote_llseek(struct file *file, loff_t offset, int origin)
{
loff_t ret;
lock_kernel();
ret = generic_file_llseek_unlocked(file, offset, origin);
unlock_kernel();
return ret;
}

const struct file_operations smb_file_operations =
{
.llseek = remote_llseek,
.llseek = smb_remote_llseek,
.read = do_sync_read,
.aio_read = smb_file_aio_read,
.write = do_sync_write,
Expand Down
3 changes: 2 additions & 1 deletion include/linux/fs.h
Original file line number Diff line number Diff line change
Expand Up @@ -1871,7 +1871,8 @@ extern void
file_ra_state_init(struct file_ra_state *ra, struct address_space *mapping);
extern loff_t no_llseek(struct file *file, loff_t offset, int origin);
extern loff_t generic_file_llseek(struct file *file, loff_t offset, int origin);
extern loff_t remote_llseek(struct file *file, loff_t offset, int origin);
extern loff_t generic_file_llseek_unlocked(struct file *file, loff_t offset,
int origin);
extern int generic_file_open(struct inode * inode, struct file * filp);
extern int nonseekable_open(struct inode * inode, struct file * filp);

Expand Down

0 comments on commit 9465efc

Please sign in to comment.