Looks good, thanks!
--
Best regards,
Pavel Shilovsky
пн, 10 июн. 2019 г. в 15:20, Steve French <smfrench@xxxxxxxxx>:
>
> 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@xxxxxxxxx> wrote:
> >
> > вт, 4 июн. 2019 г. в 17:42, Ronnie Sahlberg <lsahlber@xxxxxxxxxx>:
> > >
> > > 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@xxxxxxxxxxxxxxx>
> > > Signed-off-by: Ronnie Sahlberg <lsahlber@xxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxxxxx>
> >
> > 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
