diff mbox series

cifs: add global spinlock for the openFileList

Message ID 20190603075922.27266-1-lsahlber@redhat.com (mailing list archive)
State New, archived
Headers show
Series cifs: add global spinlock for the openFileList | expand

Commit Message

Ronnie Sahlberg June 3, 2019, 7:59 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.

RHBZ:  1580165

Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
 fs/cifs/cifsfs.c   |  1 +
 fs/cifs/cifsglob.h |  5 +++++
 fs/cifs/file.c     | 12 ++++++++++--
 3 files changed, 16 insertions(+), 2 deletions(-)

Comments

Pavel Shilovsky June 3, 2019, 5:29 p.m. UTC | #1
пн, 3 июн. 2019 г. в 01:02, 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.
>
> RHBZ:  1580165
>
> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> ---
>  fs/cifs/cifsfs.c   |  1 +
>  fs/cifs/cifsglob.h |  5 +++++
>  fs/cifs/file.c     | 12 ++++++++++--
>  3 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index f5fcd6360056..20cc4eaa7a49 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -1459,6 +1459,7 @@ init_cifs(void)
>         GlobalTotalActiveXid = 0;
>         GlobalMaxActiveXid = 0;
>         spin_lock_init(&cifs_tcp_ses_lock);
> +       spin_lock_init(&cifs_list_lock);
>         spin_lock_init(&GlobalMid_Lock);
>
>         cifs_lock_secret = get_random_u32();
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index 334ff5f9c3f3..807b7cd7d48d 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -1817,6 +1817,11 @@ GLOBAL_EXTERN struct list_head           cifs_tcp_ses_list;
>   * structure order is cifs_socket-->cifs_ses-->cifs_tcon-->cifs_file
>   */
>  GLOBAL_EXTERN spinlock_t               cifs_tcp_ses_lock;
> +/*
> + * This lock protects the cifsInodeInfo->openFileList as well as
> + * cifsFileInfo->flist|tlist.
> + */
> +GLOBAL_EXTERN spinlock_t               cifs_list_lock;
>
>  #ifdef CONFIG_CIFS_DNOTIFY_EXPERIMENTAL /* unused temporarily */
>  /* Outstanding dir notify requests */
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index 06e27ac6d82c..8e96a5ae83bf 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(&cifs_list_lock);
>         if (file->f_mode & FMODE_READ)
>                 list_add(&cfile->flist, &cinode->openFileList);
>         else
>                 list_add_tail(&cfile->flist, &cinode->openFileList);
> +       spin_unlock(&cifs_list_lock);

You are protecting cinode->openFileList here - this should be a lock
per inode structure.

>         spin_unlock(&tcon->open_file_lock);
>
>         if (fid->purge_cache)
> @@ -413,8 +415,10 @@ 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(&cifs_list_lock);
>         list_del(&cifs_file->flist);

The same here - inode lock.


>         list_del(&cifs_file->tlist);

It is a list per tcon - existing tcon lock is protecting here.

> +       spin_unlock(&cifs_list_lock);
>         atomic_dec(&tcon->num_local_opens);
>
>         if (list_empty(&cifsi->openFileList)) {
> @@ -1459,8 +1463,10 @@ void
>  cifs_move_llist(struct list_head *source, struct list_head *dest)
>  {
>         struct list_head *li, *tmp;
> +       spin_lock(&cifs_list_lock);
>         list_for_each_safe(li, tmp, source)
>                 list_move(li, dest);
> +       spin_unlock(&cifs_list_lock);
>  }
>
>  void
> @@ -1469,7 +1475,9 @@ cifs_free_llist(struct list_head *llist)
>         struct cifsLockInfo *li, *tmp;
>         list_for_each_entry_safe(li, tmp, llist, llist) {
>                 cifs_del_lock_waiters(li);
> +               spin_lock(&cifs_list_lock);
>                 list_del(&li->llist);
> +               spin_unlock(&cifs_list_lock);
>                 kfree(li);
>         }
>  }

Above two functions operate on lists of locks of inode's files - all
protected by cifsi->lock_sem.

> @@ -1950,9 +1958,9 @@ cifs_get_writable_file(struct cifsInodeInfo *cifs_inode, bool fsuid_only,
>                         return 0;
>                 }
>
> -               spin_lock(&tcon->open_file_lock);
> +               spin_lock(&cifs_list_lock);
>                 list_move_tail(&inv_file->flist, &cifs_inode->openFileList);
> -               spin_unlock(&tcon->open_file_lock);
> +               spin_unlock(&cifs_list_lock);

inode lock.

>                 cifsFileInfo_put(inv_file);
>                 ++refind;
>                 inv_file = NULL;
> --
> 2.13.6
>

What is the reasoning under using a global spin lock? Global locking
is always a source of performance problems and should be avoided as
much as possible.

--
Best regards,
Pavel Shilovsky
ronnie sahlberg June 3, 2019, 11:21 p.m. UTC | #2
On Tue, Jun 4, 2019 at 3:55 AM Pavel Shilovsky <piastryyy@gmail.com> wrote:
>
> пн, 3 июн. 2019 г. в 01:02, 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.
> >
> > RHBZ:  1580165
> >
> > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > ---
> >  fs/cifs/cifsfs.c   |  1 +
> >  fs/cifs/cifsglob.h |  5 +++++
> >  fs/cifs/file.c     | 12 ++++++++++--
> >  3 files changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> > index f5fcd6360056..20cc4eaa7a49 100644
> > --- a/fs/cifs/cifsfs.c
> > +++ b/fs/cifs/cifsfs.c
> > @@ -1459,6 +1459,7 @@ init_cifs(void)
> >         GlobalTotalActiveXid = 0;
> >         GlobalMaxActiveXid = 0;
> >         spin_lock_init(&cifs_tcp_ses_lock);
> > +       spin_lock_init(&cifs_list_lock);
> >         spin_lock_init(&GlobalMid_Lock);
> >
> >         cifs_lock_secret = get_random_u32();
> > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> > index 334ff5f9c3f3..807b7cd7d48d 100644
> > --- a/fs/cifs/cifsglob.h
> > +++ b/fs/cifs/cifsglob.h
> > @@ -1817,6 +1817,11 @@ GLOBAL_EXTERN struct list_head           cifs_tcp_ses_list;
> >   * structure order is cifs_socket-->cifs_ses-->cifs_tcon-->cifs_file
> >   */
> >  GLOBAL_EXTERN spinlock_t               cifs_tcp_ses_lock;
> > +/*
> > + * This lock protects the cifsInodeInfo->openFileList as well as
> > + * cifsFileInfo->flist|tlist.
> > + */
> > +GLOBAL_EXTERN spinlock_t               cifs_list_lock;
> >
> >  #ifdef CONFIG_CIFS_DNOTIFY_EXPERIMENTAL /* unused temporarily */
> >  /* Outstanding dir notify requests */
> > diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> > index 06e27ac6d82c..8e96a5ae83bf 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(&cifs_list_lock);
> >         if (file->f_mode & FMODE_READ)
> >                 list_add(&cfile->flist, &cinode->openFileList);
> >         else
> >                 list_add_tail(&cfile->flist, &cinode->openFileList);
> > +       spin_unlock(&cifs_list_lock);
>
> You are protecting cinode->openFileList here - this should be a lock
> per inode structure.
>
> >         spin_unlock(&tcon->open_file_lock);
> >
> >         if (fid->purge_cache)
> > @@ -413,8 +415,10 @@ 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(&cifs_list_lock);
> >         list_del(&cifs_file->flist);
>
> The same here - inode lock.
>
>
> >         list_del(&cifs_file->tlist);
>
> It is a list per tcon - existing tcon lock is protecting here.
>
> > +       spin_unlock(&cifs_list_lock);
> >         atomic_dec(&tcon->num_local_opens);
> >
> >         if (list_empty(&cifsi->openFileList)) {
> > @@ -1459,8 +1463,10 @@ void
> >  cifs_move_llist(struct list_head *source, struct list_head *dest)
> >  {
> >         struct list_head *li, *tmp;
> > +       spin_lock(&cifs_list_lock);
> >         list_for_each_safe(li, tmp, source)
> >                 list_move(li, dest);
> > +       spin_unlock(&cifs_list_lock);
> >  }
> >
> >  void
> > @@ -1469,7 +1475,9 @@ cifs_free_llist(struct list_head *llist)
> >         struct cifsLockInfo *li, *tmp;
> >         list_for_each_entry_safe(li, tmp, llist, llist) {
> >                 cifs_del_lock_waiters(li);
> > +               spin_lock(&cifs_list_lock);
> >                 list_del(&li->llist);
> > +               spin_unlock(&cifs_list_lock);
> >                 kfree(li);
> >         }
> >  }
>
> Above two functions operate on lists of locks of inode's files - all
> protected by cifsi->lock_sem.
>
> > @@ -1950,9 +1958,9 @@ cifs_get_writable_file(struct cifsInodeInfo *cifs_inode, bool fsuid_only,
> >                         return 0;
> >                 }
> >
> > -               spin_lock(&tcon->open_file_lock);
> > +               spin_lock(&cifs_list_lock);
> >                 list_move_tail(&inv_file->flist, &cifs_inode->openFileList);
> > -               spin_unlock(&tcon->open_file_lock);
> > +               spin_unlock(&cifs_list_lock);
>
> inode lock.
>
> >                 cifsFileInfo_put(inv_file);
> >                 ++refind;
> >                 inv_file = NULL;
> > --
> > 2.13.6
> >
>
> What is the reasoning under using a global spin lock? Global locking
> is always a source of performance problems and should be avoided as
> much as possible.

In multiuser each user has their own tcon  so if user A and user B
does a list_add/list_del at the same time
they are not protected against eachother sicne A and B use different
tcon->open_file_list spinlocks :-(

I will do the other changes you suggest later today and re-send
>
> --
> Best regards,
> Pavel Shilovsky
Pavel Shilovsky June 3, 2019, 11:50 p.m. UTC | #3
пн, 3 июн. 2019 г. в 16:21, ronnie sahlberg <ronniesahlberg@gmail.com>:
>
> On Tue, Jun 4, 2019 at 3:55 AM Pavel Shilovsky <piastryyy@gmail.com> wrote:
> >
> > пн, 3 июн. 2019 г. в 01:02, 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.
> > >
> > > RHBZ:  1580165
> > >
> > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > > ---
> > >  fs/cifs/cifsfs.c   |  1 +
> > >  fs/cifs/cifsglob.h |  5 +++++
> > >  fs/cifs/file.c     | 12 ++++++++++--
> > >  3 files changed, 16 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> > > index f5fcd6360056..20cc4eaa7a49 100644
> > > --- a/fs/cifs/cifsfs.c
> > > +++ b/fs/cifs/cifsfs.c
> > > @@ -1459,6 +1459,7 @@ init_cifs(void)
> > >         GlobalTotalActiveXid = 0;
> > >         GlobalMaxActiveXid = 0;
> > >         spin_lock_init(&cifs_tcp_ses_lock);
> > > +       spin_lock_init(&cifs_list_lock);
> > >         spin_lock_init(&GlobalMid_Lock);
> > >
> > >         cifs_lock_secret = get_random_u32();
> > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> > > index 334ff5f9c3f3..807b7cd7d48d 100644
> > > --- a/fs/cifs/cifsglob.h
> > > +++ b/fs/cifs/cifsglob.h
> > > @@ -1817,6 +1817,11 @@ GLOBAL_EXTERN struct list_head           cifs_tcp_ses_list;
> > >   * structure order is cifs_socket-->cifs_ses-->cifs_tcon-->cifs_file
> > >   */
> > >  GLOBAL_EXTERN spinlock_t               cifs_tcp_ses_lock;
> > > +/*
> > > + * This lock protects the cifsInodeInfo->openFileList as well as
> > > + * cifsFileInfo->flist|tlist.
> > > + */
> > > +GLOBAL_EXTERN spinlock_t               cifs_list_lock;
> > >
> > >  #ifdef CONFIG_CIFS_DNOTIFY_EXPERIMENTAL /* unused temporarily */
> > >  /* Outstanding dir notify requests */
> > > diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> > > index 06e27ac6d82c..8e96a5ae83bf 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(&cifs_list_lock);
> > >         if (file->f_mode & FMODE_READ)
> > >                 list_add(&cfile->flist, &cinode->openFileList);
> > >         else
> > >                 list_add_tail(&cfile->flist, &cinode->openFileList);
> > > +       spin_unlock(&cifs_list_lock);
> >
> > You are protecting cinode->openFileList here - this should be a lock
> > per inode structure.
> >
> > >         spin_unlock(&tcon->open_file_lock);
> > >
> > >         if (fid->purge_cache)
> > > @@ -413,8 +415,10 @@ 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(&cifs_list_lock);
> > >         list_del(&cifs_file->flist);
> >
> > The same here - inode lock.
> >
> >
> > >         list_del(&cifs_file->tlist);
> >
> > It is a list per tcon - existing tcon lock is protecting here.
> >
> > > +       spin_unlock(&cifs_list_lock);
> > >         atomic_dec(&tcon->num_local_opens);
> > >
> > >         if (list_empty(&cifsi->openFileList)) {
> > > @@ -1459,8 +1463,10 @@ void
> > >  cifs_move_llist(struct list_head *source, struct list_head *dest)
> > >  {
> > >         struct list_head *li, *tmp;
> > > +       spin_lock(&cifs_list_lock);
> > >         list_for_each_safe(li, tmp, source)
> > >                 list_move(li, dest);
> > > +       spin_unlock(&cifs_list_lock);
> > >  }
> > >
> > >  void
> > > @@ -1469,7 +1475,9 @@ cifs_free_llist(struct list_head *llist)
> > >         struct cifsLockInfo *li, *tmp;
> > >         list_for_each_entry_safe(li, tmp, llist, llist) {
> > >                 cifs_del_lock_waiters(li);
> > > +               spin_lock(&cifs_list_lock);
> > >                 list_del(&li->llist);
> > > +               spin_unlock(&cifs_list_lock);
> > >                 kfree(li);
> > >         }
> > >  }
> >
> > Above two functions operate on lists of locks of inode's files - all
> > protected by cifsi->lock_sem.
> >
> > > @@ -1950,9 +1958,9 @@ cifs_get_writable_file(struct cifsInodeInfo *cifs_inode, bool fsuid_only,
> > >                         return 0;
> > >                 }
> > >
> > > -               spin_lock(&tcon->open_file_lock);
> > > +               spin_lock(&cifs_list_lock);
> > >                 list_move_tail(&inv_file->flist, &cifs_inode->openFileList);
> > > -               spin_unlock(&tcon->open_file_lock);
> > > +               spin_unlock(&cifs_list_lock);
> >
> > inode lock.
> >
> > >                 cifsFileInfo_put(inv_file);
> > >                 ++refind;
> > >                 inv_file = NULL;
> > > --
> > > 2.13.6
> > >
> >
> > What is the reasoning under using a global spin lock? Global locking
> > is always a source of performance problems and should be avoided as
> > much as possible.
>
> In multiuser each user has their own tcon  so if user A and user B
> does a list_add/list_del at the same time
> they are not protected against eachother sicne A and B use different
> tcon->open_file_list spinlocks :-(

In this case both users still have the common thing to lock - inode,
so, locking should be a part of the inode structure to not slow access
to other files on the file system.

We also need to agree on the locking order here - tcon first and inode
second looks fine unless anybody has objections.

>
> I will do the other changes you suggest later today and re-send

Thanks for spotting the problem! It seems that multiuser mounts don't
have that much of usage but *theoretically* this is a right way to
setup SMB authorization on Linux.

--
Best regards,
Pavel Shilovsky
diff mbox series

Patch

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index f5fcd6360056..20cc4eaa7a49 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -1459,6 +1459,7 @@  init_cifs(void)
 	GlobalTotalActiveXid = 0;
 	GlobalMaxActiveXid = 0;
 	spin_lock_init(&cifs_tcp_ses_lock);
+	spin_lock_init(&cifs_list_lock);
 	spin_lock_init(&GlobalMid_Lock);
 
 	cifs_lock_secret = get_random_u32();
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 334ff5f9c3f3..807b7cd7d48d 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -1817,6 +1817,11 @@  GLOBAL_EXTERN struct list_head		cifs_tcp_ses_list;
  * structure order is cifs_socket-->cifs_ses-->cifs_tcon-->cifs_file
  */
 GLOBAL_EXTERN spinlock_t		cifs_tcp_ses_lock;
+/*
+ * This lock protects the cifsInodeInfo->openFileList as well as
+ * cifsFileInfo->flist|tlist.
+ */
+GLOBAL_EXTERN spinlock_t		cifs_list_lock;
 
 #ifdef CONFIG_CIFS_DNOTIFY_EXPERIMENTAL /* unused temporarily */
 /* Outstanding dir notify requests */
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 06e27ac6d82c..8e96a5ae83bf 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(&cifs_list_lock);
 	if (file->f_mode & FMODE_READ)
 		list_add(&cfile->flist, &cinode->openFileList);
 	else
 		list_add_tail(&cfile->flist, &cinode->openFileList);
+	spin_unlock(&cifs_list_lock);
 	spin_unlock(&tcon->open_file_lock);
 
 	if (fid->purge_cache)
@@ -413,8 +415,10 @@  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(&cifs_list_lock);
 	list_del(&cifs_file->flist);
 	list_del(&cifs_file->tlist);
+	spin_unlock(&cifs_list_lock);
 	atomic_dec(&tcon->num_local_opens);
 
 	if (list_empty(&cifsi->openFileList)) {
@@ -1459,8 +1463,10 @@  void
 cifs_move_llist(struct list_head *source, struct list_head *dest)
 {
 	struct list_head *li, *tmp;
+	spin_lock(&cifs_list_lock);
 	list_for_each_safe(li, tmp, source)
 		list_move(li, dest);
+	spin_unlock(&cifs_list_lock);
 }
 
 void
@@ -1469,7 +1475,9 @@  cifs_free_llist(struct list_head *llist)
 	struct cifsLockInfo *li, *tmp;
 	list_for_each_entry_safe(li, tmp, llist, llist) {
 		cifs_del_lock_waiters(li);
+		spin_lock(&cifs_list_lock);
 		list_del(&li->llist);
+		spin_unlock(&cifs_list_lock);
 		kfree(li);
 	}
 }
@@ -1950,9 +1958,9 @@  cifs_get_writable_file(struct cifsInodeInfo *cifs_inode, bool fsuid_only,
 			return 0;
 		}
 
-		spin_lock(&tcon->open_file_lock);
+		spin_lock(&cifs_list_lock);
 		list_move_tail(&inv_file->flist, &cifs_inode->openFileList);
-		spin_unlock(&tcon->open_file_lock);
+		spin_unlock(&cifs_list_lock);
 		cifsFileInfo_put(inv_file);
 		++refind;
 		inv_file = NULL;