чт, 28 мар. 2019 г. в 08:34, Aurelien Aptel <aaptel@xxxxxxxx>:
>
> 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() helper.
> - The ref is decremented at the end of the handler
>
> This bug was triggered by xfstest 464.
>
> Signed-off-by: Aurelien Aptel <aaptel@xxxxxxxx>
> ---
> fs/cifs/cifsacl.c | 2 +-
> fs/cifs/cifsglob.h | 3 ++-
> fs/cifs/cifssmb.c | 2 +-
> fs/cifs/file.c | 36 +++++++++++++++++++++++-------------
> fs/cifs/inode.c | 4 ++--
> fs/cifs/misc.c | 27 ++++++++++++++++++++++++---
> fs/cifs/smb1ops.c | 2 +-
> fs/cifs/smb2misc.c | 6 +++---
> fs/cifs/smb2ops.c | 2 +-
> 9 files changed, 58 insertions(+), 26 deletions(-)
>
> diff --git a/fs/cifs/cifsacl.c b/fs/cifs/cifsacl.c
> index 1d377b7f2860..3c03e3346744 100644
> --- a/fs/cifs/cifsacl.c
> +++ b/fs/cifs/cifsacl.c
> @@ -1069,7 +1069,7 @@ struct cifs_ntsd *get_cifs_acl(struct cifs_sb_info *cifs_sb,
> return get_cifs_acl_by_path(cifs_sb, path, pacllen);
>
> pntsd = get_cifs_acl_by_fid(cifs_sb, &open_file->fid, pacllen);
> - cifsFileInfo_put(open_file);
> + cifsFileInfo_put(open_file, true);
> return pntsd;
> }
>
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index 38feae812b47..87751a9c947a 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -1325,7 +1325,7 @@ cifsFileInfo_get_locked(struct cifsFileInfo *cifs_file)
> }
>
> struct cifsFileInfo *cifsFileInfo_get(struct cifsFileInfo *cifs_file);
> -void cifsFileInfo_put(struct cifsFileInfo *cifs_file);
> +void cifsFileInfo_put(struct cifsFileInfo *cifs_file, bool wait_oplock_hdlr);
>
> #define CIFS_CACHE_READ_FLG 1
> #define CIFS_CACHE_HANDLE_FLG 2
> @@ -1847,6 +1847,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/cifssmb.c b/fs/cifs/cifssmb.c
> index f43747c062a7..d17a697b86f9 100644
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
> @@ -2067,7 +2067,7 @@ cifs_writedata_release(struct kref *refcount)
> #endif
>
> if (wdata->cfile)
> - cifsFileInfo_put(wdata->cfile);
> + cifsFileInfo_put(wdata->cfile, true);
>
> kvfree(wdata->pages);
> kfree(wdata);
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index 89006e044973..3cea7bb5dac9 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -360,12 +360,20 @@ 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
> + *
> + * 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)
> +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);
> @@ -414,7 +422,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;
> @@ -772,7 +781,7 @@ cifs_reopen_file(struct cifsFileInfo *cfile, bool can_flush)
> int cifs_close(struct inode *inode, struct file *file)
> {
> if (file->private_data != NULL) {
> - cifsFileInfo_put(file->private_data);
> + cifsFileInfo_put(file->private_data, true);
> file->private_data = NULL;
> }
>
> @@ -812,7 +821,7 @@ cifs_reopen_persistent_handles(struct cifs_tcon *tcon)
> if (cifs_reopen_file(open_file, false /* do not flush */))
> tcon->need_reopen_files = true;
> list_del_init(&open_file->rlist);
> - cifsFileInfo_put(open_file);
> + cifsFileInfo_put(open_file, true);
> }
> }
>
> @@ -1934,7 +1943,7 @@ cifs_get_writable_file(struct cifsInodeInfo *cifs_inode, bool fsuid_only,
> spin_lock(&tcon->open_file_lock);
> list_move_tail(&inv_file->flist, &cifs_inode->openFileList);
> spin_unlock(&tcon->open_file_lock);
> - cifsFileInfo_put(inv_file);
> + cifsFileInfo_put(inv_file, true);
> ++refind;
> inv_file = NULL;
> spin_lock(&tcon->open_file_lock);
> @@ -1995,7 +2004,7 @@ static int cifs_partialpagewrite(struct page *page, unsigned from, unsigned to)
> if (!rc) {
> bytes_written = cifs_write(open_file, open_file->pid,
> write_data, to - from, &offset);
> - cifsFileInfo_put(open_file);
> + cifsFileInfo_put(open_file, true);
> /* Does mm or vfs already set times? */
> inode->i_atime = inode->i_mtime = current_time(inode);
> if ((bytes_written > 0) && (offset))
> @@ -2182,7 +2191,7 @@ static int cifs_writepages(struct address_space *mapping,
> int get_file_rc = 0;
>
> if (cfile)
> - cifsFileInfo_put(cfile);
> + cifsFileInfo_put(cfile, true);
>
> rc = cifs_get_writable_file(CIFS_I(inode), false, &cfile);
>
> @@ -2296,7 +2305,7 @@ static int cifs_writepages(struct address_space *mapping,
> mapping->writeback_index = index;
>
> if (cfile)
> - cifsFileInfo_put(cfile);
> + cifsFileInfo_put(cfile, true);
> free_xid(xid);
> return rc;
> }
> @@ -3184,7 +3193,7 @@ cifs_readdata_release(struct kref *refcount)
> }
> #endif
> if (rdata->cfile)
> - cifsFileInfo_put(rdata->cfile);
> + cifsFileInfo_put(rdata->cfile, true);
>
> kvfree(rdata->pages);
> kfree(rdata);
> @@ -4603,6 +4612,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/inode.c b/fs/cifs/inode.c
> index 53fdb5df0d2e..713188cda2cd 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -2172,7 +2172,7 @@ cifs_set_file_size(struct inode *inode, struct iattr *attrs,
> attrs->ia_size, false);
> else
> rc = -ENOSYS;
> - cifsFileInfo_put(open_file);
> + cifsFileInfo_put(open_file, true);
> cifs_dbg(FYI, "SetFSize for attrs rc = %d\n", rc);
> } else
> rc = -EINVAL;
> @@ -2319,7 +2319,7 @@ cifs_setattr_unix(struct dentry *direntry, struct iattr *attrs)
> u32 npid = open_file->pid;
> pTcon = tlink_tcon(open_file->tlink);
> rc = CIFSSMBUnixSetFileInfo(xid, pTcon, args, nfid, npid);
> - cifsFileInfo_put(open_file);
> + cifsFileInfo_put(open_file, true);
> } else {
> tlink = cifs_sb_tlink(cifs_sb);
> if (IS_ERR(tlink)) {
> diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
> index bee203055b30..48bcc4459d0f 100644
> --- a/fs/cifs/misc.c
> +++ b/fs/cifs/misc.c
> @@ -501,8 +501,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);
> @@ -607,6 +606,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);
> @@ -785,7 +806,7 @@ cifs_aio_ctx_release(struct kref *refcount)
> struct cifs_aio_ctx *ctx = container_of(refcount,
> struct cifs_aio_ctx, refcount);
>
> - cifsFileInfo_put(ctx->cfile);
> + cifsFileInfo_put(ctx->cfile, true);
> kvfree(ctx->bv);
> kfree(ctx);
> }
> diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
> index c711f1f39bf2..a5edf08644bf 100644
> --- a/fs/cifs/smb1ops.c
> +++ b/fs/cifs/smb1ops.c
> @@ -828,7 +828,7 @@ smb_set_file_info(struct inode *inode, const char *full_path,
> if (open_file == NULL)
> CIFSSMBClose(xid, tcon, fid.netfid);
> else
> - cifsFileInfo_put(open_file);
> + cifsFileInfo_put(open_file, true);
> out:
> if (tlink != NULL)
> cifs_put_tlink(tlink);
> diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c
> index 0e3570e40ff8..e311f58dc1c8 100644
> --- a/fs/cifs/smb2misc.c
> +++ b/fs/cifs/smb2misc.c
> @@ -555,7 +555,7 @@ smb2_tcon_has_lease(struct cifs_tcon *tcon, struct smb2_lease_break *rsp,
> clear_bit(CIFS_INODE_DOWNGRADE_OPLOCK_TO_L2,
> &cinode->flags);
>
> - queue_work(cifsoplockd_wq, &cfile->oplock_break);
> + cifs_queue_oplock_break(cfile);
> kfree(lw);
> return true;
> }
> @@ -712,8 +712,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);
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index e755236af8ff..e496d6287da0 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -2584,7 +2584,7 @@ get_smb2_acl(struct cifs_sb_info *cifs_sb,
> return get_smb2_acl_by_path(cifs_sb, path, pacllen);
>
> pntsd = get_smb2_acl_by_fid(cifs_sb, &open_file->fid, pacllen);
> - cifsFileInfo_put(open_file);
> + cifsFileInfo_put(open_file, true);
> return pntsd;
> }
> #endif
> --
> 2.16.4
>
Thanks for fixing it. The patch involves a lot of changes that will
complicate backporting and may be avoided. How about this:
1) rename the existing cifsFileInfo_put(open_file, bool wait) to
__cifsFileInfo_put(open_file, bool wait) keeping the changes you made
inside,
2) make new cifsFileInfo_put(open_file) call
__cifsFileInfo_put(open_file, wait=true)?
This way we will avoid changing all the places where cifsFileInfo_put
is currently being called and make it easier for further stable
porting.
--
Best regards,
Pavel Shilovsky
