| Message ID | 20190605003838.13101-1-lsahlber@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cifs: add spinlock for the openFileList to cifsInodeInfo | expand |
вт, 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
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
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
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;
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(-)