linux-kernel - Re: [PATCH] fs: dodge an atomic in putname if ref == 1

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: <naalmvyvolpfkwxoztkskhz2kyoznjjhm5y4zmfd44tyf5d24k@2jap6ty4nkwz>
Date: Wed, 12 Mar 2025 16:51:22 +0100
From: Jan Kara <jack@...e.cz>
To: Mateusz Guzik <mjguzik@...il.com>
Cc: brauner@...nel.org, viro@...iv.linux.org.uk, jack@...e.cz, 
	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org, io-uring@...r.kernel.org, 
	audit@...r.kernel.org, axboe@...nel.dk
Subject: Re: [PATCH] fs: dodge an atomic in putname if ref == 1

On Tue 11-03-25 19:18:04, Mateusz Guzik wrote:
> While the structure is refcounted, the only consumer incrementing it is
> audit and even then the atomic operation is only needed when it
> interacts with io_uring.
> 
> If putname spots a count of 1, there is no legitimate way for anyone to
> bump it.
> 
> If audit is disabled, the count is guaranteed to be 1, which
> consistently elides the atomic for all path lookups. If audit is
> enabled, it still manages to elide the last decrement.
> 
> Note the patch does not do anything to prevent audit from suffering
> atomics. See [1] and [2] for a different approach.
> 
> Benchmarked on Sapphire Rapids issuing access() (ops/s):
> before: 5106246
> after:  5269678 (+3%)
> 
> Link 1:	https://lore.kernel.org/linux-fsdevel/20250307161155.760949-1-mjguzik@gmail.com/
> Link 2: https://lore.kernel.org/linux-fsdevel/20250307164216.GI2023217@ZenIV/
> Signed-off-by: Mateusz Guzik <mjguzik@...il.com>

Yeah, I like this much more than the original. Feel free to add:

Reviewed-by: Jan Kara <jack@...e.cz>

								Honza

> This is an alternative to the patch I linked above.
> 
> I think the improved commit message should also cover the feedback
> Christian previously shared concerning it.
> 
> This is a trivial win until the atomic issue gets resolved, I don't see
> any reason to NOT include it. At the same time I don't have that much
> interest arguing about it either.
> 
> That is to say, here is a different take, if you don't like it, I'm
> dropping the subject.
> 
> cheers
> 
>  fs/namei.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index 06765d320e7e..add90981cfcd 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -275,14 +275,19 @@ EXPORT_SYMBOL(getname_kernel);
>  
>  void putname(struct filename *name)
>  {
> +	int refcnt;
> +
>  	if (IS_ERR_OR_NULL(name))
>  		return;
>  
> -	if (WARN_ON_ONCE(!atomic_read(&name->refcnt)))
> -		return;
> +	refcnt = atomic_read(&name->refcnt);
> +	if (refcnt != 1) {
> +		if (WARN_ON_ONCE(!refcnt))
> +			return;
>  
> -	if (!atomic_dec_and_test(&name->refcnt))
> -		return;
> +		if (!atomic_dec_and_test(&name->refcnt))
> +			return;
> +	}
>  
>  	if (name->name != name->iname) {
>  		__putname(name->name);
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR