[PATCH] cifs: add spinlock for the openFileList to cifsInodeInfo

[PATCH] cifs: add spinlock for the openFileList to cifsInodeInfo

[Ronnie Sahlberg]

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

Re: [PATCH] cifs: add spinlock for the openFileList to cifsInodeInfo

[Pavel Shilovsky]

вт, 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

Re: [PATCH] cifs: add spinlock for the openFileList to cifsInodeInfo

[Steve French]

[-- Attachment #1: Type: text/plain, Size: 4441 bytes --]

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

[-- Attachment #2: 0001-cifs-add-spinlock-for-the-openFileList-to-cifsInodeI.patch --]
[-- Type: text/x-patch, Size: 4006 bytes --]

From b5ea6129c93c49eb4a6d91eddf67982b87adf0f7 Mon Sep 17 00:00:00 2001
From: Ronnie Sahlberg <lsahlber@redhat.com>
Date: Wed, 5 Jun 2019 10:38:38 +1000
Subject: [PATCH] cifs: add spinlock for the openFileList to cifsInodeInfo

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>
Signed-off-by: Steve French <stfrench@microsoft.com>
Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
---
 fs/cifs/cifsfs.c   | 1 +
 fs/cifs/cifsglob.h | 5 +++++
 fs/cifs/file.c     | 8 ++++++--
 3 files changed, 12 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..4777b3c4a92c 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 */
@@ -1780,10 +1781,14 @@ require use of the stronger protocol */
  *  tcp_ses_lock protects:
  *	list operations on tcp and SMB session lists
  *  tcon->open_file_lock protects the list of open files hanging off the tcon
+ *  inode->open_file_lock protects the openFileList hanging off the inode
  *  cfile->file_info_lock protects counters and fields in cifs file struct
  *  f_owner.lock protects certain per file struct operations
  *  mapping->page_lock protects certain per page operations
  *
+ *  Note that the cifs_tcon.open_file_lock should be taken before
+ *  not after the cifsInodeInfo.open_file_lock
+ *
  *  Semaphores
  *  ----------
  *  sesSem     operations on smb session
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.20.1

Re: [PATCH] cifs: add spinlock for the openFileList to cifsInodeInfo

[Pavel Shilovsky]

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