RE: [PATCH 3.16 58/87] cifs: add spinlock for the openFileList to cifsInodeInfo

[Pavel Shilovskiy]

> -----Original Message-----
> From: Ben Hutchings <ben@xxxxxxxxxxxxxxx>
> Sent: Wednesday, October 2, 2019 12:07 PM
> To: linux-kernel@xxxxxxxxxxxxxxx; stable@xxxxxxxxxxxxxxx
> Cc: akpm@xxxxxxxxxxxxxxxxxxxx; Denis Kirjanov <kda@xxxxxxxxxxxxxxxxx>; Ronnie Sahlberg <lsahlber@xxxxxxxxxx>; Steven French <Steven.French@xxxxxxxxxxxxx>; Pavel Shilovskiy <pshilov@xxxxxxxxxxxxx>
> Subject: [PATCH 3.16 58/87] cifs: add spinlock for the openFileList to cifsInodeInfo
>
> 3.16.75-rc1 review patch. ÂIf anyone has any objections, please let me know.
>
> ------------------
>
> From: Ronnie Sahlberg <lsahlber@xxxxxxxxxx>
>
> commit 487317c99477d00f22370625d53be3239febabbe upstream.
>
> 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
>
> Signed-off-by: Ronnie Sahlberg <lsahlber@xxxxxxxxxx>
> Signed-off-by: Steve French <stfrench@xxxxxxxxxxxxx>
> Reviewed-by: Pavel Shilovsky <pshilov@xxxxxxxxxxxxx>
> [bwh: Backported to 3.16: adjust context, indentation]
> Signed-off-by: Ben Hutchings <ben@xxxxxxxxxxxxxxx>
> ---
> Âfs/cifs/cifsfs.c  | 1 +
> Âfs/cifs/cifsglob.h | 5 +++++
> Âfs/cifs/file.c   | 8 ++++++--
> Â3 files changed, 12 insertions(+), 2 deletions(-)
>
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -260,6 +260,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);
> Â#ifdef CONFIG_CIFS_SMB2
> Â Â Â Â generate_random_uuid(cifs_inode->lease_key);
> Â#endif
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -1116,6 +1116,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 */
> @@ -1485,10 +1486,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
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -337,10 +337,12 @@ cifs_new_fileinfo(struct cifs_fid *fid,
> Â Â Â Â list_add(&cfile->tlist, &tcon->openFileList);
>
> Â Â Â Â /* 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)
> @@ -412,7 +414,9 @@ void _cifsFileInfo_put(struct cifsFileIn
> Â Â Â Â 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);
>
> Â Â Â Â if (list_empty(&cifsi->openFileList)) { @@ -1850,10 +1854,10 @@ refind_writable:
> Â Â Â Â Â Â Â Â if (!rc)
> Â Â Â Â Â Â Â Â Â Â Â Â return inv_file;
> Â Â Â Â Â Â Â Â else {
> - Â Â Â Â Â Â Â Â Â Â Â 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;
>

Hi Ben,

We have recently found regressions in this patch that are addressed in the following two patches:

cb248819d209d ("cifs: use cifsInodeInfo->open_file_lock while iterating to avoid a panic")
1a67c41596575 ("CIFS: Fix use after free of file info structures")

So, I would suggest either to apply those two patches above or to revert this one.

--
Best regards,
Pavel Shilovsky