| Message ID | 20190717200236.14920-2-gpiccoli@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | kernel panic using CIFS share in smb2_push_mandatory_locks() | expand |
On 17.07.19 22:02, Guilherme G. Piccoli wrote: > From: Aurelien Aptel <aaptel@suse.com> > > BugLink: http://bugs.launchpad.net/bugs/1795659 > > In the oplock break handler, writing pending changes from pages puts > the FileInfo handle. If the refcount reaches zero it closes the handle > and waits for any oplock break handler to return, thus causing a deadlock. > > To prevent this situation: > > * We add a wait flag to cifsFileInfo_put() to decide whether we should > wait for running/pending oplock break handlers > > * We keep an additionnal reference of the SMB FileInfo handle so that > for the rest of the handler putting the handle won't close it. > - The ref is bumped everytime we queue the handler via the > cifs_queue_oplock_break() helper. > - The ref is decremented at the end of the handler > > This bug was triggered by xfstest 464. > > Also important fix to address the various reports of > oops in smb2_push_mandatory_locks > > Signed-off-by: Aurelien Aptel <aaptel@suse.com> > Signed-off-by: Steve French <stfrench@microsoft.com> > Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com> > CC: Stable <stable@vger.kernel.org> > (backported from commit b98749cac4a695f084a5ff076f4510b23e353ecd) > [gpiccoli: context adjustment.] > Signed-off-by: Guilherme G. Piccoli <gpiccoli@canonical.com> Acked-by: Stefan Bader <stefan.bader@canonical.com> > ---> fs/cifs/cifsglob.h | 2 ++ > fs/cifs/file.c | 30 +++++++++++++++++++++++++----- > fs/cifs/misc.c | 25 +++++++++++++++++++++++-- > fs/cifs/smb2misc.c | 6 +++--- > 4 files changed, 53 insertions(+), 10 deletions(-) > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h > index 37b5ddf27ff1..edfa9e69fd10 100644 > --- a/fs/cifs/cifsglob.h > +++ b/fs/cifs/cifsglob.h > @@ -1189,6 +1189,7 @@ cifsFileInfo_get_locked(struct cifsFileInfo *cifs_file) > } > > struct cifsFileInfo *cifsFileInfo_get(struct cifsFileInfo *cifs_file); > +void _cifsFileInfo_put(struct cifsFileInfo *cifs_file, bool wait_oplock_hdlr); > void cifsFileInfo_put(struct cifsFileInfo *cifs_file); > > #define CIFS_CACHE_READ_FLG 1 > @@ -1693,6 +1694,7 @@ GLOBAL_EXTERN spinlock_t gidsidlock; > #endif /* CONFIG_CIFS_ACL */ > > void cifs_oplock_break(struct work_struct *work); > +void cifs_queue_oplock_break(struct cifsFileInfo *cfile); > > extern const struct slow_work_ops cifs_oplock_break_ops; > extern struct workqueue_struct *cifsiod_wq; > diff --git a/fs/cifs/file.c b/fs/cifs/file.c > index 3a85df2a9baf..7bb94b269fd2 100644 > --- a/fs/cifs/file.c > +++ b/fs/cifs/file.c > @@ -358,12 +358,30 @@ cifsFileInfo_get(struct cifsFileInfo *cifs_file) > return cifs_file; > } > > -/* > - * Release a reference on the file private data. This may involve closing > - * the filehandle out on the server. Must be called without holding > - * tcon->open_file_lock and cifs_file->file_info_lock. > +/** > + * cifsFileInfo_put - release a reference of file priv data > + * > + * Always potentially wait for oplock handler. See _cifsFileInfo_put(). > */ > void cifsFileInfo_put(struct cifsFileInfo *cifs_file) > +{ > + _cifsFileInfo_put(cifs_file, true); > +} > + > +/** > + * _cifsFileInfo_put - release a reference of file priv data > + * > + * This may involve closing the filehandle @cifs_file out on the > + * server. Must be called without holding tcon->open_file_lock and > + * cifs_file->file_info_lock. > + * > + * If @wait_for_oplock_handler is true and we are releasing the last > + * reference, wait for any running oplock break handler of the file > + * and cancel any pending one. If calling this function from the > + * oplock break handler, you need to pass false. > + * > + */ > +void _cifsFileInfo_put(struct cifsFileInfo *cifs_file, bool wait_oplock_handler) > { > struct inode *inode = d_inode(cifs_file->dentry); > struct cifs_tcon *tcon = tlink_tcon(cifs_file->tlink); > @@ -411,7 +429,8 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_file) > > spin_unlock(&tcon->open_file_lock); > > - oplock_break_cancelled = cancel_work_sync(&cifs_file->oplock_break); > + oplock_break_cancelled = wait_oplock_handler ? > + cancel_work_sync(&cifs_file->oplock_break) : false; > > if (!tcon->need_reconnect && !cifs_file->invalidHandle) { > struct TCP_Server_Info *server = tcon->ses->server; > @@ -4097,6 +4116,7 @@ void cifs_oplock_break(struct work_struct *work) > cinode); > cifs_dbg(FYI, "Oplock release rc = %d\n", rc); > } > + _cifsFileInfo_put(cfile, false /* do not wait for ourself */); > cifs_done_oplock_break(cinode); > } > > diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c > index bcab30d4a6c7..76f1649ab444 100644 > --- a/fs/cifs/misc.c > +++ b/fs/cifs/misc.c > @@ -486,8 +486,7 @@ is_valid_oplock_break(char *buffer, struct TCP_Server_Info *srv) > CIFS_INODE_DOWNGRADE_OPLOCK_TO_L2, > &pCifsInode->flags); > > - queue_work(cifsoplockd_wq, > - &netfile->oplock_break); > + cifs_queue_oplock_break(netfile); > netfile->oplock_break_cancelled = false; > > spin_unlock(&tcon->open_file_lock); > @@ -584,6 +583,28 @@ void cifs_put_writer(struct cifsInodeInfo *cinode) > spin_unlock(&cinode->writers_lock); > } > > +/** > + * cifs_queue_oplock_break - queue the oplock break handler for cfile > + * > + * This function is called from the demultiplex thread when it > + * receives an oplock break for @cfile. > + * > + * Assumes the tcon->open_file_lock is held. > + * Assumes cfile->file_info_lock is NOT held. > + */ > +void cifs_queue_oplock_break(struct cifsFileInfo *cfile) > +{ > + /* > + * Bump the handle refcount now while we hold the > + * open_file_lock to enforce the validity of it for the oplock > + * break handler. The matching put is done at the end of the > + * handler. > + */ > + cifsFileInfo_get(cfile); > + > + queue_work(cifsoplockd_wq, &cfile->oplock_break); > +} > + > void cifs_done_oplock_break(struct cifsInodeInfo *cinode) > { > clear_bit(CIFS_INODE_PENDING_OPLOCK_BREAK, &cinode->flags); > diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c > index efdfdb47a7dd..bba371071ac6 100644 > --- a/fs/cifs/smb2misc.c > +++ b/fs/cifs/smb2misc.c > @@ -506,7 +506,7 @@ smb2_tcon_has_lease(struct cifs_tcon *tcon, struct smb2_lease_break *rsp, > else > cfile->oplock_break_cancelled = true; > > - queue_work(cifsoplockd_wq, &cfile->oplock_break); > + cifs_queue_oplock_break(cfile); > kfree(lw); > return true; > } > @@ -650,8 +650,8 @@ smb2_is_valid_oplock_break(char *buffer, struct TCP_Server_Info *server) > CIFS_INODE_DOWNGRADE_OPLOCK_TO_L2, > &cinode->flags); > spin_unlock(&cfile->file_info_lock); > - queue_work(cifsoplockd_wq, > - &cfile->oplock_break); > + > + cifs_queue_oplock_break(cfile); > > spin_unlock(&tcon->open_file_lock); > spin_unlock(&cifs_tcp_ses_lock); >
Acked-by: Marcelo Henrique Cerri <marcelo.cerri@canonical.com> Is that needed for 4.4 too? On Wed, Jul 17, 2019 at 05:02:36PM -0300, Guilherme G. Piccoli wrote: > From: Aurelien Aptel <aaptel@suse.com> > > BugLink: http://bugs.launchpad.net/bugs/1795659 > > In the oplock break handler, writing pending changes from pages puts > the FileInfo handle. If the refcount reaches zero it closes the handle > and waits for any oplock break handler to return, thus causing a deadlock. > > To prevent this situation: > > * We add a wait flag to cifsFileInfo_put() to decide whether we should > wait for running/pending oplock break handlers > > * We keep an additionnal reference of the SMB FileInfo handle so that > for the rest of the handler putting the handle won't close it. > - The ref is bumped everytime we queue the handler via the > cifs_queue_oplock_break() helper. > - The ref is decremented at the end of the handler > > This bug was triggered by xfstest 464. > > Also important fix to address the various reports of > oops in smb2_push_mandatory_locks > > Signed-off-by: Aurelien Aptel <aaptel@suse.com> > Signed-off-by: Steve French <stfrench@microsoft.com> > Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com> > CC: Stable <stable@vger.kernel.org> > (backported from commit b98749cac4a695f084a5ff076f4510b23e353ecd) > [gpiccoli: context adjustment.] > Signed-off-by: Guilherme G. Piccoli <gpiccoli@canonical.com> > --- > fs/cifs/cifsglob.h | 2 ++ > fs/cifs/file.c | 30 +++++++++++++++++++++++++----- > fs/cifs/misc.c | 25 +++++++++++++++++++++++-- > fs/cifs/smb2misc.c | 6 +++--- > 4 files changed, 53 insertions(+), 10 deletions(-) > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h > index 37b5ddf27ff1..edfa9e69fd10 100644 > --- a/fs/cifs/cifsglob.h > +++ b/fs/cifs/cifsglob.h > @@ -1189,6 +1189,7 @@ cifsFileInfo_get_locked(struct cifsFileInfo *cifs_file) > } > > struct cifsFileInfo *cifsFileInfo_get(struct cifsFileInfo *cifs_file); > +void _cifsFileInfo_put(struct cifsFileInfo *cifs_file, bool wait_oplock_hdlr); > void cifsFileInfo_put(struct cifsFileInfo *cifs_file); > > #define CIFS_CACHE_READ_FLG 1 > @@ -1693,6 +1694,7 @@ GLOBAL_EXTERN spinlock_t gidsidlock; > #endif /* CONFIG_CIFS_ACL */ > > void cifs_oplock_break(struct work_struct *work); > +void cifs_queue_oplock_break(struct cifsFileInfo *cfile); > > extern const struct slow_work_ops cifs_oplock_break_ops; > extern struct workqueue_struct *cifsiod_wq; > diff --git a/fs/cifs/file.c b/fs/cifs/file.c > index 3a85df2a9baf..7bb94b269fd2 100644 > --- a/fs/cifs/file.c > +++ b/fs/cifs/file.c > @@ -358,12 +358,30 @@ cifsFileInfo_get(struct cifsFileInfo *cifs_file) > return cifs_file; > } > > -/* > - * Release a reference on the file private data. This may involve closing > - * the filehandle out on the server. Must be called without holding > - * tcon->open_file_lock and cifs_file->file_info_lock. > +/** > + * cifsFileInfo_put - release a reference of file priv data > + * > + * Always potentially wait for oplock handler. See _cifsFileInfo_put(). > */ > void cifsFileInfo_put(struct cifsFileInfo *cifs_file) > +{ > + _cifsFileInfo_put(cifs_file, true); > +} > + > +/** > + * _cifsFileInfo_put - release a reference of file priv data > + * > + * This may involve closing the filehandle @cifs_file out on the > + * server. Must be called without holding tcon->open_file_lock and > + * cifs_file->file_info_lock. > + * > + * If @wait_for_oplock_handler is true and we are releasing the last > + * reference, wait for any running oplock break handler of the file > + * and cancel any pending one. If calling this function from the > + * oplock break handler, you need to pass false. > + * > + */ > +void _cifsFileInfo_put(struct cifsFileInfo *cifs_file, bool wait_oplock_handler) > { > struct inode *inode = d_inode(cifs_file->dentry); > struct cifs_tcon *tcon = tlink_tcon(cifs_file->tlink); > @@ -411,7 +429,8 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_file) > > spin_unlock(&tcon->open_file_lock); > > - oplock_break_cancelled = cancel_work_sync(&cifs_file->oplock_break); > + oplock_break_cancelled = wait_oplock_handler ? > + cancel_work_sync(&cifs_file->oplock_break) : false; > > if (!tcon->need_reconnect && !cifs_file->invalidHandle) { > struct TCP_Server_Info *server = tcon->ses->server; > @@ -4097,6 +4116,7 @@ void cifs_oplock_break(struct work_struct *work) > cinode); > cifs_dbg(FYI, "Oplock release rc = %d\n", rc); > } > + _cifsFileInfo_put(cfile, false /* do not wait for ourself */); > cifs_done_oplock_break(cinode); > } > > diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c > index bcab30d4a6c7..76f1649ab444 100644 > --- a/fs/cifs/misc.c > +++ b/fs/cifs/misc.c > @@ -486,8 +486,7 @@ is_valid_oplock_break(char *buffer, struct TCP_Server_Info *srv) > CIFS_INODE_DOWNGRADE_OPLOCK_TO_L2, > &pCifsInode->flags); > > - queue_work(cifsoplockd_wq, > - &netfile->oplock_break); > + cifs_queue_oplock_break(netfile); > netfile->oplock_break_cancelled = false; > > spin_unlock(&tcon->open_file_lock); > @@ -584,6 +583,28 @@ void cifs_put_writer(struct cifsInodeInfo *cinode) > spin_unlock(&cinode->writers_lock); > } > > +/** > + * cifs_queue_oplock_break - queue the oplock break handler for cfile > + * > + * This function is called from the demultiplex thread when it > + * receives an oplock break for @cfile. > + * > + * Assumes the tcon->open_file_lock is held. > + * Assumes cfile->file_info_lock is NOT held. > + */ > +void cifs_queue_oplock_break(struct cifsFileInfo *cfile) > +{ > + /* > + * Bump the handle refcount now while we hold the > + * open_file_lock to enforce the validity of it for the oplock > + * break handler. The matching put is done at the end of the > + * handler. > + */ > + cifsFileInfo_get(cfile); > + > + queue_work(cifsoplockd_wq, &cfile->oplock_break); > +} > + > void cifs_done_oplock_break(struct cifsInodeInfo *cinode) > { > clear_bit(CIFS_INODE_PENDING_OPLOCK_BREAK, &cinode->flags); > diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c > index efdfdb47a7dd..bba371071ac6 100644 > --- a/fs/cifs/smb2misc.c > +++ b/fs/cifs/smb2misc.c > @@ -506,7 +506,7 @@ smb2_tcon_has_lease(struct cifs_tcon *tcon, struct smb2_lease_break *rsp, > else > cfile->oplock_break_cancelled = true; > > - queue_work(cifsoplockd_wq, &cfile->oplock_break); > + cifs_queue_oplock_break(cfile); > kfree(lw); > return true; > } > @@ -650,8 +650,8 @@ smb2_is_valid_oplock_break(char *buffer, struct TCP_Server_Info *server) > CIFS_INODE_DOWNGRADE_OPLOCK_TO_L2, > &cinode->flags); > spin_unlock(&cfile->file_info_lock); > - queue_work(cifsoplockd_wq, > - &cfile->oplock_break); > + > + cifs_queue_oplock_break(cfile); > > spin_unlock(&tcon->open_file_lock); > spin_unlock(&cifs_tcp_ses_lock); > -- > 2.22.0 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
On Thu, Jul 18, 2019 at 10:23 AM Marcelo Henrique Cerri <marcelo.cerri@canonical.com> wrote: > > Acked-by: Marcelo Henrique Cerri <marcelo.cerri@canonical.com> > > Is that needed for 4.4 too? > Hi Cerri, thanks for the ack! CIFS maintainers sent this patch to stable, it's on 4.9/4.14/4.19 LTS stable kernels, as well as 5.0.y and newer, but not in 4.4.y. I've checked and seems "backportable" to 4.4, code is quite similar. But since CIFS maintainers and Greg/Sash didn't merged the commit in version 4.4 and we have no reports of this issue in 4.4, I vote for being conservative and keep it on 4.15+, not diverging from stable tree. If you or anybody here disagree with that, I'd be glad to proceed with the SRU to Xenial. Thanks, Guilherme
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h index 37b5ddf27ff1..edfa9e69fd10 100644 --- a/fs/cifs/cifsglob.h +++ b/fs/cifs/cifsglob.h @@ -1189,6 +1189,7 @@ cifsFileInfo_get_locked(struct cifsFileInfo *cifs_file) } struct cifsFileInfo *cifsFileInfo_get(struct cifsFileInfo *cifs_file); +void _cifsFileInfo_put(struct cifsFileInfo *cifs_file, bool wait_oplock_hdlr); void cifsFileInfo_put(struct cifsFileInfo *cifs_file); #define CIFS_CACHE_READ_FLG 1 @@ -1693,6 +1694,7 @@ GLOBAL_EXTERN spinlock_t gidsidlock; #endif /* CONFIG_CIFS_ACL */ void cifs_oplock_break(struct work_struct *work); +void cifs_queue_oplock_break(struct cifsFileInfo *cfile); extern const struct slow_work_ops cifs_oplock_break_ops; extern struct workqueue_struct *cifsiod_wq; diff --git a/fs/cifs/file.c b/fs/cifs/file.c index 3a85df2a9baf..7bb94b269fd2 100644 --- a/fs/cifs/file.c +++ b/fs/cifs/file.c @@ -358,12 +358,30 @@ cifsFileInfo_get(struct cifsFileInfo *cifs_file) return cifs_file; } -/* - * Release a reference on the file private data. This may involve closing - * the filehandle out on the server. Must be called without holding - * tcon->open_file_lock and cifs_file->file_info_lock. +/** + * cifsFileInfo_put - release a reference of file priv data + * + * Always potentially wait for oplock handler. See _cifsFileInfo_put(). */ void cifsFileInfo_put(struct cifsFileInfo *cifs_file) +{ + _cifsFileInfo_put(cifs_file, true); +} + +/** + * _cifsFileInfo_put - release a reference of file priv data + * + * This may involve closing the filehandle @cifs_file out on the + * server. Must be called without holding tcon->open_file_lock and + * cifs_file->file_info_lock. + * + * If @wait_for_oplock_handler is true and we are releasing the last + * reference, wait for any running oplock break handler of the file + * and cancel any pending one. If calling this function from the + * oplock break handler, you need to pass false. + * + */ +void _cifsFileInfo_put(struct cifsFileInfo *cifs_file, bool wait_oplock_handler) { struct inode *inode = d_inode(cifs_file->dentry); struct cifs_tcon *tcon = tlink_tcon(cifs_file->tlink); @@ -411,7 +429,8 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_file) spin_unlock(&tcon->open_file_lock); - oplock_break_cancelled = cancel_work_sync(&cifs_file->oplock_break); + oplock_break_cancelled = wait_oplock_handler ? + cancel_work_sync(&cifs_file->oplock_break) : false; if (!tcon->need_reconnect && !cifs_file->invalidHandle) { struct TCP_Server_Info *server = tcon->ses->server; @@ -4097,6 +4116,7 @@ void cifs_oplock_break(struct work_struct *work) cinode); cifs_dbg(FYI, "Oplock release rc = %d\n", rc); } + _cifsFileInfo_put(cfile, false /* do not wait for ourself */); cifs_done_oplock_break(cinode); } diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c index bcab30d4a6c7..76f1649ab444 100644 --- a/fs/cifs/misc.c +++ b/fs/cifs/misc.c @@ -486,8 +486,7 @@ is_valid_oplock_break(char *buffer, struct TCP_Server_Info *srv) CIFS_INODE_DOWNGRADE_OPLOCK_TO_L2, &pCifsInode->flags); - queue_work(cifsoplockd_wq, - &netfile->oplock_break); + cifs_queue_oplock_break(netfile); netfile->oplock_break_cancelled = false; spin_unlock(&tcon->open_file_lock); @@ -584,6 +583,28 @@ void cifs_put_writer(struct cifsInodeInfo *cinode) spin_unlock(&cinode->writers_lock); } +/** + * cifs_queue_oplock_break - queue the oplock break handler for cfile + * + * This function is called from the demultiplex thread when it + * receives an oplock break for @cfile. + * + * Assumes the tcon->open_file_lock is held. + * Assumes cfile->file_info_lock is NOT held. + */ +void cifs_queue_oplock_break(struct cifsFileInfo *cfile) +{ + /* + * Bump the handle refcount now while we hold the + * open_file_lock to enforce the validity of it for the oplock + * break handler. The matching put is done at the end of the + * handler. + */ + cifsFileInfo_get(cfile); + + queue_work(cifsoplockd_wq, &cfile->oplock_break); +} + void cifs_done_oplock_break(struct cifsInodeInfo *cinode) { clear_bit(CIFS_INODE_PENDING_OPLOCK_BREAK, &cinode->flags); diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c index efdfdb47a7dd..bba371071ac6 100644 --- a/fs/cifs/smb2misc.c +++ b/fs/cifs/smb2misc.c @@ -506,7 +506,7 @@ smb2_tcon_has_lease(struct cifs_tcon *tcon, struct smb2_lease_break *rsp, else cfile->oplock_break_cancelled = true; - queue_work(cifsoplockd_wq, &cfile->oplock_break); + cifs_queue_oplock_break(cfile); kfree(lw); return true; } @@ -650,8 +650,8 @@ smb2_is_valid_oplock_break(char *buffer, struct TCP_Server_Info *server) CIFS_INODE_DOWNGRADE_OPLOCK_TO_L2, &cinode->flags); spin_unlock(&cfile->file_info_lock); - queue_work(cifsoplockd_wq, - &cfile->oplock_break); + + cifs_queue_oplock_break(cfile); spin_unlock(&tcon->open_file_lock); spin_unlock(&cifs_tcp_ses_lock);