пн, 3 июн. 2019 г. в 16:21, ronnie sahlberg <ronniesahlberg@xxxxxxxxx>:
>
> On Tue, Jun 4, 2019 at 3:55 AM Pavel Shilovsky <piastryyy@xxxxxxxxx> wrote:
> >
> > пн, 3 июн. 2019 г. в 01:02, 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.
> > >
> > > RHBZ: 1580165
> > >
> > > Signed-off-by: Ronnie Sahlberg <lsahlber@xxxxxxxxxx>
> > > ---
> > > 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
