linux-kernel - Re: [PATCH] vfs: dodge smp_mb in break_lease and break

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b2f081444c0c70603af40942e0776fe1fb298577.camel@kernel.org>
Date: Tue, 06 Aug 2024 14:12:05 -0400
From: Jeff Layton <jlayton@...nel.org>
To: Mateusz Guzik <mjguzik@...il.com>, brauner@...nel.org, 
	viro@...iv.linux.org.uk
Cc: jack@...e.cz, linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH] vfs: dodge smp_mb in break_lease and break_deleg in the
 common case

On Tue, 2024-08-06 at 19:28 +0200, Mateusz Guzik wrote:
> These inlines show up in the fast path (e.g., in do_dentry_open()) and
> induce said full barrier regarding i_flctx access when in most cases the
> pointer is NULL.
> 
> The pointer can be safely checked before issuing the barrier, dodging it
> in most cases as a result.
> 
> It is plausible the consume fence would be sufficient, but I don't want
> to go audit all callers regarding what they before calling here.
> 
> Signed-off-by: Mateusz Guzik <mjguzik@...il.com>
> ---
> 
> the header file has locks_inode_context and i even found users like
> this (lease_get_mtime):
> 
> ctx = locks_inode_context(inode);
> if (ctx && !list_empty_careful(&ctx->flc_lease)) {
> 
> however, without looking further at the code I'm not confident this
> would be sufficient here -- for all I know one consumer needs all stores
> to be visible before looking further after derefing the pointer
> 
> keeping the full fence in place makes this reasonably easy to reason
> about the change i think, but someone(tm) willing to sort this out is
> most welcome to do so
> 

Nod. It would be nice to get rid of that barrier. I'm not sure how to
do that in a provably correct way. I'll need to think about that.

>  include/linux/filelock.h | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/filelock.h b/include/linux/filelock.h
> index daee999d05f3..bb44224c6676 100644
> --- a/include/linux/filelock.h
> +++ b/include/linux/filelock.h
> @@ -420,28 +420,38 @@ static inline int locks_lock_file_wait(struct file *filp, struct file_lock *fl)
>  #ifdef CONFIG_FILE_LOCKING
>  static inline int break_lease(struct inode *inode, unsigned int mode)
>  {
> +	struct file_lock_context *flctx;
> +
>  	/*
>  	 * Since this check is lockless, we must ensure that any refcounts
>  	 * taken are done before checking i_flctx->flc_lease. Otherwise, we
>  	 * could end up racing with tasks trying to set a new lease on this
>  	 * file.
>  	 */
> +	flctx = READ_ONCE(inode->i_flctx);
> +	if (!flctx)
> +		return 0;
>  	smp_mb();
> -	if (inode->i_flctx && !list_empty_careful(&inode->i_flctx->flc_lease))
> +	if (!list_empty_careful(&flctx->flc_lease))
>  		return __break_lease(inode, mode, FL_LEASE);
>  	return 0;
>  }
>  
>  static inline int break_deleg(struct inode *inode, unsigned int mode)
>  {
> +	struct file_lock_context *flctx;
> +
>  	/*
>  	 * Since this check is lockless, we must ensure that any refcounts
>  	 * taken are done before checking i_flctx->flc_lease. Otherwise, we
>  	 * could end up racing with tasks trying to set a new lease on this
>  	 * file.
>  	 */
> +	flctx = READ_ONCE(inode->i_flctx);
> +	if (!flctx)
> +		return 0;
>  	smp_mb();
> -	if (inode->i_flctx && !list_empty_careful(&inode->i_flctx->flc_lease))
> +	if (!list_empty_careful(&flctx->flc_lease))
>  		return __break_lease(inode, mode, FL_DELEG);
>  	return 0;
>  }

This change looks good to me:

Reviewed-by: Jeff Layton <jlayton@...nel.org>