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