Don't you need to a spinlock/spinunlock(i_lock) or something similar
(there isn't a spinlock in the file struct unfortunately) around the
reads and writes from f_pos in fs/read_write.c in remote_llseek with
your patch since the reads/writes from that field are not necessarily
atomic and threads could be racing in seek on the same file struct?
On Jan 26, 2008 8:17 PM, Andi Kleen <ak@xxxxxxx> wrote:
>
> - Replace remote_llseek with remote_llseek_unlocked (to force compilation
> failures in all users)
> - Change all users to either use remote_llseek 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.
>
> Cc: Trond.Myklebust@xxxxxxxxxx
> Cc: swhiteho@xxxxxxxxxx
> Cc: sfrench@xxxxxxxxx
> Cc: vandrove@xxxxxxxxxx
>
> Signed-off-by: Andi Kleen <ak@xxxxxxx>
>
> ---
> 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 | 7 +++----
> fs/smbfs/file.c | 11 ++++++++++-
> include/linux/fs.h | 3 ++-
> 7 files changed, 34 insertions(+), 11 deletions(-)
>
> Index: linux/fs/cifs/cifsfs.c
> ===================================================================
> --- linux.orig/fs/cifs/cifsfs.c
> +++ linux/fs/cifs/cifsfs.c
> @@ -549,7 +549,7 @@ static loff_t cifs_llseek(struct file *f
> if (retval < 0)
> return (loff_t)retval;
> }
> - return remote_llseek(file, offset, origin);
> + return remote_llseek_unlocked(file, offset, origin);
> }
>
> static struct file_system_type cifs_fs_type = {
> Index: linux/fs/gfs2/ops_file.c
> ===================================================================
> --- linux.orig/fs/gfs2/ops_file.c
> +++ linux/fs/gfs2/ops_file.c
> @@ -107,11 +107,11 @@ static loff_t gfs2_llseek(struct file *f
> 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 = remote_llseek_unlocked(file, offset, origin);
> gfs2_glock_dq_uninit(&i_gh);
> }
> } else
> - error = remote_llseek(file, offset, origin);
> + error = remote_llseek_unlocked(file, offset, origin);
>
> return error;
> }
> Index: linux/fs/ncpfs/file.c
> ===================================================================
> --- linux.orig/fs/ncpfs/file.c
> +++ linux/fs/ncpfs/file.c
> @@ -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"
> @@ -281,9 +282,18 @@ static int ncp_release(struct inode *ino
> return 0;
> }
>
> +static loff_t ncp_remote_llseek(struct file *file, loff_t offset, int origin)
> +{
> + loff_t ret;
> + lock_kernel();
> + ret = remote_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,
> Index: linux/fs/read_write.c
> ===================================================================
> --- linux.orig/fs/read_write.c
> +++ linux/fs/read_write.c
> @@ -58,11 +58,10 @@ loff_t generic_file_llseek(struct file *
>
> EXPORT_SYMBOL(generic_file_llseek);
>
> -loff_t remote_llseek(struct file *file, loff_t offset, int origin)
> +loff_t remote_llseek_unlocked(struct file *file, loff_t offset, int origin)
> {
> long long retval;
>
> - lock_kernel();
> switch (origin) {
> case SEEK_END:
> offset += i_size_read(file->f_path.dentry->d_inode);
> @@ -73,15 +72,15 @@ loff_t remote_llseek(struct file *file,
> retval = -EINVAL;
> if (offset>=0 && offset<=file->f_path.dentry->d_inode->i_sb->s_maxbytes) {
> if (offset != file->f_pos) {
> + /* AK: do we need a lock for those? */
> file->f_pos = offset;
> file->f_version = 0;
> }
> retval = offset;
> }
> - unlock_kernel();
> return retval;
> }
> -EXPORT_SYMBOL(remote_llseek);
> +EXPORT_SYMBOL(remote_llseek_unlocked);
>
> loff_t no_llseek(struct file *file, loff_t offset, int origin)
> {
> Index: linux/fs/smbfs/file.c
> ===================================================================
> --- linux.orig/fs/smbfs/file.c
> +++ linux/fs/smbfs/file.c
> @@ -422,9 +422,18 @@ smb_file_permission(struct inode *inode,
> return error;
> }
>
> +static loff_t smb_remote_llseek(struct file *file, loff_t offset, int origin)
> +{
> + loff_t ret;
> + lock_kernel();
> + ret = remote_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,
> Index: linux/include/linux/fs.h
> ===================================================================
> --- linux.orig/include/linux/fs.h
> +++ linux/include/linux/fs.h
> @@ -1825,7 +1825,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 remote_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);
>
> Index: linux/fs/nfs/file.c
> ===================================================================
> --- linux.orig/fs/nfs/file.c
> +++ linux/fs/nfs/file.c
> @@ -166,6 +166,7 @@ force_reval:
>
> 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;
> @@ -173,7 +174,10 @@ static loff_t nfs_file_llseek(struct fil
> if (retval < 0)
> return (loff_t)retval;
> }
> - return remote_llseek(filp, offset, origin);
> + lock_kernel(); /* BKL needed? */
> + loff = remote_llseek_unlocked(filp, offset, origin);
> + unlock_kernel();
> + return loff;
> }
>
> /*
>
--
Thanks,
Steve
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
