linux-kernel - Re: [PATCH v2] futex: Rewrite get_inode_sequence

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 for Android: free password hash cracker in your pocket

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

Message-ID: <bb91d63d-c61a-4063-bf14-4cbbb62bec12@igalia.com>
Date: Wed, 9 Oct 2024 16:43:58 -0300
From: André Almeida <andrealmeid@...lia.com>
To: Uros Bizjak <ubizjak@...il.com>
Cc: Thomas Gleixner <tglx@...utronix.de>, linux-kernel@...r.kernel.org,
 Ingo Molnar <mingo@...nel.org>, Peter Zijlstra <peterz@...radead.org>,
 Darren Hart <dvhart@...radead.org>, Davidlohr Bueso <dave@...olabs.net>
Subject: Re: [PATCH v2] futex: Rewrite get_inode_sequence_number() to make
 code simpler

Hi Uros,

Em 04/10/2024 05:52, Uros Bizjak escreveu:
> Rewrite get_inode_sequence_number() to make code simpler:
> 
> a) Rewrite FOR loop to a DO-WHILE loop with returns moved
> out of the loop.
> 
> b) Use atomic64_inc_return() instead of atomic64_add_return().
> 
> c) Use !atomic64_try_cmpxchg_relaxed(*ptr, &old, new) instead of
> atomic64_cmpxchg_relaxed (*ptr, old, new) != old.  x86 CMPXCHG
> instruction returns success in ZF flag, so this change also saves
> a compare instruction after CMPXCHG.

Remember, it's easy to see in the diff that you replace the function, 
but might be not so clear why you did so. I think it would be better to 
understand if you write like:

We are trying to set a value for the i_sequence, that we expect that is 
zero, but if we fail to do so, we are happy to use the current non-zero 
i_sequence value that we found. Instead of using 
atomic64_cmpxchg_relaxed(), use atomic64_try_cmpxchg_relaxed() which 
provides a better semantic for this situation.

> 
> Signed-off-by: Uros Bizjak <ubizjak@...il.com>
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Cc: Ingo Molnar <mingo@...nel.org>
> Cc: Peter Zijlstra <peterz@...radead.org>
> Cc: Darren Hart <dvhart@...radead.org>
> Cc: Davidlohr Bueso <dave@...olabs.net>
> Cc: "André Almeida" <andrealmeid@...lia.com>
> ---
> v2: Explicitly initialize "old" to zero before the call to
> atomic64_try_cmpxchg_relaxed(). Rewrite commit message to
> state the motivation for the patch.
> ---
>   kernel/futex/core.c | 18 ++++++++----------
>   1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/kernel/futex/core.c b/kernel/futex/core.c
> index 136768ae2637..ac650f7ed56c 100644
> --- a/kernel/futex/core.c
> +++ b/kernel/futex/core.c
> @@ -173,23 +173,21 @@ futex_setup_timer(ktime_t *time, struct hrtimer_sleeper *timeout,
>   static u64 get_inode_sequence_number(struct inode *inode)
>   {
>   	static atomic64_t i_seq;
> -	u64 old;
> +	u64 old, new;
>   
>   	/* Does the inode already have a sequence number? */
>   	old = atomic64_read(&inode->i_sequence);
>   	if (likely(old))
>   		return old;
>   
> -	for (;;) {
> -		u64 new = atomic64_add_return(1, &i_seq);
> -		if (WARN_ON_ONCE(!new))
> -			continue;
> +	do {
> +		new = atomic64_inc_return(&i_seq);
> +	} while	(WARN_ON_ONCE(!new));
>   
> -		old = atomic64_cmpxchg_relaxed(&inode->i_sequence, 0, new);
> -		if (old)
> -			return old;
> -		return new;
> -	}
> +	old = 0;

Please initialize it in the variable declaration.

> +	if (!atomic64_try_cmpxchg_relaxed(&inode->i_sequence, &old, new))
> +		return old;
> +	return new;
>   }
>   
>   /**