cifs: add spinlock for the openFileList to cifsInodeInfo
diff mbox series

Message ID 20190605003838.13101-1-lsahlber@redhat.com
State New
Headers show
Series
  • cifs: add spinlock for the openFileList to cifsInodeInfo
Related show

Commit Message

Ronnie Sahlberg June 5, 2019, 12:38 a.m. UTC
We can not depend on the tcon->open_file_lock here since in multiuser mode
we may have the same file/inode open via multiple different tcons.

The current code is race prone and will crash if one user deletes a file
at the same time a different user opens/create the file.

To avoid this we need to have a spinlock attached to the inode and not the tcon.

RHBZ:  1580165

CC: Stable <stable@vger.kernel.org>
Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
 fs/cifs/cifsfs.c   | 1 +
 fs/cifs/cifsglob.h | 1 +
 fs/cifs/file.c     | 8 ++++++--
 3 files changed, 8 insertions(+), 2 deletions(-)

Comments

Pavel Shilovsky June 10, 2019, 9:36 p.m. UTC | #1
вт, 4 июн. 2019 г. в 17:42, Ronnie Sahlberg <lsahlber@redhat.com>:
>
> We can not depend on the tcon->open_file_lock here since in multiuser mode
> we may have the same file/inode open via multiple different tcons.
>
> The current code is race prone and will crash if one user deletes a file
> at the same time a different user opens/create the file.
>
> To avoid this we need to have a spinlock attached to the inode and not the tcon.
>
> RHBZ:  1580165
>
> CC: Stable <stable@vger.kernel.org>
> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> ---
>  fs/cifs/cifsfs.c   | 1 +
>  fs/cifs/cifsglob.h | 1 +
>  fs/cifs/file.c     | 8 ++++++--
>  3 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index f5fcd6360056..65d9771e49f9 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -303,6 +303,7 @@ cifs_alloc_inode(struct super_block *sb)
>         cifs_inode->uniqueid = 0;
>         cifs_inode->createtime = 0;
>         cifs_inode->epoch = 0;
> +       spin_lock_init(&cifs_inode->open_file_lock);
>         generate_random_uuid(cifs_inode->lease_key);
>
>         /*
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index 334ff5f9c3f3..733ddd5fd480 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -1377,6 +1377,7 @@ struct cifsInodeInfo {
>         struct rw_semaphore lock_sem;   /* protect the fields above */
>         /* BB add in lists for dirty pages i.e. write caching info for oplock */
>         struct list_head openFileList;
> +       spinlock_t      open_file_lock; /* protects openFileList */
>         __u32 cifsAttrs; /* e.g. DOS archive bit, sparse, compressed, system */
>         unsigned int oplock;            /* oplock/lease level we have */
>         unsigned int epoch;             /* used to track lease state changes */
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index 06e27ac6d82c..97090693d182 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -338,10 +338,12 @@ cifs_new_fileinfo(struct cifs_fid *fid, struct file *file,
>         atomic_inc(&tcon->num_local_opens);
>
>         /* if readable file instance put first in list*/
> +       spin_lock(&cinode->open_file_lock);
>         if (file->f_mode & FMODE_READ)
>                 list_add(&cfile->flist, &cinode->openFileList);
>         else
>                 list_add_tail(&cfile->flist, &cinode->openFileList);
> +       spin_unlock(&cinode->open_file_lock);
>         spin_unlock(&tcon->open_file_lock);
>
>         if (fid->purge_cache)
> @@ -413,7 +415,9 @@ void _cifsFileInfo_put(struct cifsFileInfo *cifs_file, bool wait_oplock_handler)
>         cifs_add_pending_open_locked(&fid, cifs_file->tlink, &open);
>
>         /* remove it from the lists */
> +       spin_lock(&cifsi->open_file_lock);
>         list_del(&cifs_file->flist);
> +       spin_unlock(&cifsi->open_file_lock);
>         list_del(&cifs_file->tlist);
>         atomic_dec(&tcon->num_local_opens);
>
> @@ -1950,9 +1954,9 @@ cifs_get_writable_file(struct cifsInodeInfo *cifs_inode, bool fsuid_only,
>                         return 0;
>                 }
>
> -               spin_lock(&tcon->open_file_lock);
> +               spin_lock(&cifs_inode->open_file_lock);
>                 list_move_tail(&inv_file->flist, &cifs_inode->openFileList);
> -               spin_unlock(&tcon->open_file_lock);
> +               spin_unlock(&cifs_inode->open_file_lock);
>                 cifsFileInfo_put(inv_file);
>                 ++refind;
>                 inv_file = NULL;
> --
> 2.13.6
>

Thanks for the patch. Looks good to me.

Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>

I would only add a comment telling what an order of the locks should
be: cifs_tcon.open_file_lock and then cifsInodeInfo.open_file_lock.

--
Best regards,
Pavel Shilovsky
Steve French June 10, 2019, 10:20 p.m. UTC | #2
Adding a comment as requested and also mention of the new spinlock in
the list of locks and their purpose in cifsglob.h (then squashed that
change into Ronnie's commit and added your Reviewed-by - see attached)

On Mon, Jun 10, 2019 at 4:36 PM Pavel Shilovsky <piastryyy@gmail.com> wrote:
>
> вт, 4 июн. 2019 г. в 17:42, Ronnie Sahlberg <lsahlber@redhat.com>:
> >
> > We can not depend on the tcon->open_file_lock here since in multiuser mode
> > we may have the same file/inode open via multiple different tcons.
> >
> > The current code is race prone and will crash if one user deletes a file
> > at the same time a different user opens/create the file.
> >
> > To avoid this we need to have a spinlock attached to the inode and not the tcon.
> >
> > RHBZ:  1580165
> >
> > CC: Stable <stable@vger.kernel.org>
> > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > ---
> >  fs/cifs/cifsfs.c   | 1 +
> >  fs/cifs/cifsglob.h | 1 +
> >  fs/cifs/file.c     | 8 ++++++--
> >  3 files changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> > index f5fcd6360056..65d9771e49f9 100644
> > --- a/fs/cifs/cifsfs.c
> > +++ b/fs/cifs/cifsfs.c
> > @@ -303,6 +303,7 @@ cifs_alloc_inode(struct super_block *sb)
> >         cifs_inode->uniqueid = 0;
> >         cifs_inode->createtime = 0;
> >         cifs_inode->epoch = 0;
> > +       spin_lock_init(&cifs_inode->open_file_lock);
> >         generate_random_uuid(cifs_inode->lease_key);
> >
> >         /*
> > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> > index 334ff5f9c3f3..733ddd5fd480 100644
> > --- a/fs/cifs/cifsglob.h
> > +++ b/fs/cifs/cifsglob.h
> > @@ -1377,6 +1377,7 @@ struct cifsInodeInfo {
> >         struct rw_semaphore lock_sem;   /* protect the fields above */
> >         /* BB add in lists for dirty pages i.e. write caching info for oplock */
> >         struct list_head openFileList;
> > +       spinlock_t      open_file_lock; /* protects openFileList */
> >         __u32 cifsAttrs; /* e.g. DOS archive bit, sparse, compressed, system */
> >         unsigned int oplock;            /* oplock/lease level we have */
> >         unsigned int epoch;             /* used to track lease state changes */
> > diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> > index 06e27ac6d82c..97090693d182 100644
> > --- a/fs/cifs/file.c
> > +++ b/fs/cifs/file.c
> > @@ -338,10 +338,12 @@ cifs_new_fileinfo(struct cifs_fid *fid, struct file *file,
> >         atomic_inc(&tcon->num_local_opens);
> >
> >         /* if readable file instance put first in list*/
> > +       spin_lock(&cinode->open_file_lock);
> >         if (file->f_mode & FMODE_READ)
> >                 list_add(&cfile->flist, &cinode->openFileList);
> >         else
> >                 list_add_tail(&cfile->flist, &cinode->openFileList);
> > +       spin_unlock(&cinode->open_file_lock);
> >         spin_unlock(&tcon->open_file_lock);
> >
> >         if (fid->purge_cache)
> > @@ -413,7 +415,9 @@ void _cifsFileInfo_put(struct cifsFileInfo *cifs_file, bool wait_oplock_handler)
> >         cifs_add_pending_open_locked(&fid, cifs_file->tlink, &open);
> >
> >         /* remove it from the lists */
> > +       spin_lock(&cifsi->open_file_lock);
> >         list_del(&cifs_file->flist);
> > +       spin_unlock(&cifsi->open_file_lock);
> >         list_del(&cifs_file->tlist);
> >         atomic_dec(&tcon->num_local_opens);
> >
> > @@ -1950,9 +1954,9 @@ cifs_get_writable_file(struct cifsInodeInfo *cifs_inode, bool fsuid_only,
> >                         return 0;
> >                 }
> >
> > -               spin_lock(&tcon->open_file_lock);
> > +               spin_lock(&cifs_inode->open_file_lock);
> >                 list_move_tail(&inv_file->flist, &cifs_inode->openFileList);
> > -               spin_unlock(&tcon->open_file_lock);
> > +               spin_unlock(&cifs_inode->open_file_lock);
> >                 cifsFileInfo_put(inv_file);
> >                 ++refind;
> >                 inv_file = NULL;
> > --
> > 2.13.6
> >
>
> Thanks for the patch. Looks good to me.
>
> Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
>
> I would only add a comment telling what an order of the locks should
> be: cifs_tcon.open_file_lock and then cifsInodeInfo.open_file_lock.
>
> --
> Best regards,
> Pavel Shilovsky
Pavel Shilovsky June 10, 2019, 10:22 p.m. UTC | #3
Looks good, thanks!

--
Best regards,
Pavel Shilovsky

пн, 10 июн. 2019 г. в 15:20, Steve French <smfrench@gmail.com>:
>
> Adding a comment as requested and also mention of the new spinlock in
> the list of locks and their purpose in cifsglob.h (then squashed that
> change into Ronnie's commit and added your Reviewed-by - see attached)
>
> On Mon, Jun 10, 2019 at 4:36 PM Pavel Shilovsky <piastryyy@gmail.com> wrote:
> >
> > вт, 4 июн. 2019 г. в 17:42, Ronnie Sahlberg <lsahlber@redhat.com>:
> > >
> > > We can not depend on the tcon->open_file_lock here since in multiuser mode
> > > we may have the same file/inode open via multiple different tcons.
> > >
> > > The current code is race prone and will crash if one user deletes a file
> > > at the same time a different user opens/create the file.
> > >
> > > To avoid this we need to have a spinlock attached to the inode and not the tcon.
> > >
> > > RHBZ:  1580165
> > >
> > > CC: Stable <stable@vger.kernel.org>
> > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > > ---
> > >  fs/cifs/cifsfs.c   | 1 +
> > >  fs/cifs/cifsglob.h | 1 +
> > >  fs/cifs/file.c     | 8 ++++++--
> > >  3 files changed, 8 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> > > index f5fcd6360056..65d9771e49f9 100644
> > > --- a/fs/cifs/cifsfs.c
> > > +++ b/fs/cifs/cifsfs.c
> > > @@ -303,6 +303,7 @@ cifs_alloc_inode(struct super_block *sb)
> > >         cifs_inode->uniqueid = 0;
> > >         cifs_inode->createtime = 0;
> > >         cifs_inode->epoch = 0;
> > > +       spin_lock_init(&cifs_inode->open_file_lock);
> > >         generate_random_uuid(cifs_inode->lease_key);
> > >
> > >         /*
> > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> > > index 334ff5f9c3f3..733ddd5fd480 100644
> > > --- a/fs/cifs/cifsglob.h
> > > +++ b/fs/cifs/cifsglob.h
> > > @@ -1377,6 +1377,7 @@ struct cifsInodeInfo {
> > >         struct rw_semaphore lock_sem;   /* protect the fields above */
> > >         /* BB add in lists for dirty pages i.e. write caching info for oplock */
> > >         struct list_head openFileList;
> > > +       spinlock_t      open_file_lock; /* protects openFileList */
> > >         __u32 cifsAttrs; /* e.g. DOS archive bit, sparse, compressed, system */
> > >         unsigned int oplock;            /* oplock/lease level we have */
> > >         unsigned int epoch;             /* used to track lease state changes */
> > > diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> > > index 06e27ac6d82c..97090693d182 100644
> > > --- a/fs/cifs/file.c
> > > +++ b/fs/cifs/file.c
> > > @@ -338,10 +338,12 @@ cifs_new_fileinfo(struct cifs_fid *fid, struct file *file,
> > >         atomic_inc(&tcon->num_local_opens);
> > >
> > >         /* if readable file instance put first in list*/
> > > +       spin_lock(&cinode->open_file_lock);
> > >         if (file->f_mode & FMODE_READ)
> > >                 list_add(&cfile->flist, &cinode->openFileList);
> > >         else
> > >                 list_add_tail(&cfile->flist, &cinode->openFileList);
> > > +       spin_unlock(&cinode->open_file_lock);
> > >         spin_unlock(&tcon->open_file_lock);
> > >
> > >         if (fid->purge_cache)
> > > @@ -413,7 +415,9 @@ void _cifsFileInfo_put(struct cifsFileInfo *cifs_file, bool wait_oplock_handler)
> > >         cifs_add_pending_open_locked(&fid, cifs_file->tlink, &open);
> > >
> > >         /* remove it from the lists */
> > > +       spin_lock(&cifsi->open_file_lock);
> > >         list_del(&cifs_file->flist);
> > > +       spin_unlock(&cifsi->open_file_lock);
> > >         list_del(&cifs_file->tlist);
> > >         atomic_dec(&tcon->num_local_opens);
> > >
> > > @@ -1950,9 +1954,9 @@ cifs_get_writable_file(struct cifsInodeInfo *cifs_inode, bool fsuid_only,
> > >                         return 0;
> > >                 }
> > >
> > > -               spin_lock(&tcon->open_file_lock);
> > > +               spin_lock(&cifs_inode->open_file_lock);
> > >                 list_move_tail(&inv_file->flist, &cifs_inode->openFileList);
> > > -               spin_unlock(&tcon->open_file_lock);
> > > +               spin_unlock(&cifs_inode->open_file_lock);
> > >                 cifsFileInfo_put(inv_file);
> > >                 ++refind;
> > >                 inv_file = NULL;
> > > --
> > > 2.13.6
> > >
> >
> > Thanks for the patch. Looks good to me.
> >
> > Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
> >
> > I would only add a comment telling what an order of the locks should
> > be: cifs_tcon.open_file_lock and then cifsInodeInfo.open_file_lock.
> >
> > --
> > Best regards,
> > Pavel Shilovsky
>
>
>
> --
> Thanks,
>
> Steve

Patch
diff mbox series

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index f5fcd6360056..65d9771e49f9 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -303,6 +303,7 @@  cifs_alloc_inode(struct super_block *sb)
 	cifs_inode->uniqueid = 0;
 	cifs_inode->createtime = 0;
 	cifs_inode->epoch = 0;
+	spin_lock_init(&cifs_inode->open_file_lock);
 	generate_random_uuid(cifs_inode->lease_key);
 
 	/*
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 334ff5f9c3f3..733ddd5fd480 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -1377,6 +1377,7 @@  struct cifsInodeInfo {
 	struct rw_semaphore lock_sem;	/* protect the fields above */
 	/* BB add in lists for dirty pages i.e. write caching info for oplock */
 	struct list_head openFileList;
+	spinlock_t	open_file_lock;	/* protects openFileList */
 	__u32 cifsAttrs; /* e.g. DOS archive bit, sparse, compressed, system */
 	unsigned int oplock;		/* oplock/lease level we have */
 	unsigned int epoch;		/* used to track lease state changes */
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 06e27ac6d82c..97090693d182 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -338,10 +338,12 @@  cifs_new_fileinfo(struct cifs_fid *fid, struct file *file,
 	atomic_inc(&tcon->num_local_opens);
 
 	/* if readable file instance put first in list*/
+	spin_lock(&cinode->open_file_lock);
 	if (file->f_mode & FMODE_READ)
 		list_add(&cfile->flist, &cinode->openFileList);
 	else
 		list_add_tail(&cfile->flist, &cinode->openFileList);
+	spin_unlock(&cinode->open_file_lock);
 	spin_unlock(&tcon->open_file_lock);
 
 	if (fid->purge_cache)
@@ -413,7 +415,9 @@  void _cifsFileInfo_put(struct cifsFileInfo *cifs_file, bool wait_oplock_handler)
 	cifs_add_pending_open_locked(&fid, cifs_file->tlink, &open);
 
 	/* remove it from the lists */
+	spin_lock(&cifsi->open_file_lock);
 	list_del(&cifs_file->flist);
+	spin_unlock(&cifsi->open_file_lock);
 	list_del(&cifs_file->tlist);
 	atomic_dec(&tcon->num_local_opens);
 
@@ -1950,9 +1954,9 @@  cifs_get_writable_file(struct cifsInodeInfo *cifs_inode, bool fsuid_only,
 			return 0;
 		}
 
-		spin_lock(&tcon->open_file_lock);
+		spin_lock(&cifs_inode->open_file_lock);
 		list_move_tail(&inv_file->flist, &cifs_inode->openFileList);
-		spin_unlock(&tcon->open_file_lock);
+		spin_unlock(&cifs_inode->open_file_lock);
 		cifsFileInfo_put(inv_file);
 		++refind;
 		inv_file = NULL;