ACK: [SRU B][PATCH 1/1] CIFS: keep FileInfo handle live during oplock break

Stefan Bader stefan.bader at canonical.com
Thu Jul 18 09:22:45 UTC 2019


On 17.07.19 22:02, Guilherme G. Piccoli wrote:
> From: Aurelien Aptel <aaptel at 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 at suse.com>
> Signed-off-by: Steve French <stfrench at microsoft.com>
> Reviewed-by: Pavel Shilovsky <pshilov at microsoft.com>
> CC: Stable <stable at vger.kernel.org>
> (backported from commit b98749cac4a695f084a5ff076f4510b23e353ecd)
> [gpiccoli: context adjustment.]
> Signed-off-by: Guilherme G. Piccoli <gpiccoli at canonical.com>
Acked-by: Stefan Bader <stefan.bader at 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);
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20190718/fb34f0f1/attachment-0001.sig>


More information about the kernel-team mailing list