RE: [PATCH] ceph: Fix potential undefined behavior in crush_ln() with GCC 11.1.0

[Viacheslav Dubeyko]

On Fri, 2025-09-19 at 10:34 +0800, 陈华昭（Lyican） wrote:
> > 2025年9月19日 02:07，Viacheslav Dubeyko <Slava.Dubeyko@xxxxxxx> 写道：
> > 

<skipped>

> 
> Hi Slava,
> 
> Thank you for reviewing my patch. I apologize for the issues in my original submission.
> 
> You are absolutely right about the patch application failure. The main problem was that I failed to properly specify the Linux kernel version and commit hash I was working with in my original submission. I am indeed working on commit f83ec76bf285bea5727f478a68b894f5543ca76e (Linux 6.17-rc6), which matches exactly what you mentioned.
> 
> I've now regenerated the patch using git format-patch based on the correct commit. I've also refined the fix by simplifying the zero-value check to make it more concise while maintaining the same safety guarantees. Please find the updated patch below and kindly review it again:
> 
> ---
> 
> From 2465d99797764ad45d7315f0a4a0fe0f5e7113a1 Mon Sep 17 00:00:00 2001
> From: Huazhao Chen <lyican53@xxxxxxxxx>
> Date: Fri, 19 Sep 2025 09:34:14 +0800
> Subject: [PATCH] ceph: Fix potential undefined behavior in crush_ln() with GCC
> 11.1.0
> 
> When x & 0x1FFFF equals zero, __builtin_clz() is called with a zero
> argument, which results in undefined behavior. This can happen during
> ceph's consistent hashing calculations and may lead to incorrect
> placement group mappings.
> 
> Fix by storing the masked value in a variable and checking if it's
> non-zero before calling __builtin_clz(). If the masked value is zero,
> use the expected result of 16 directly.
> 
> Signed-off-by: Huazhao Chen <lyican53@xxxxxxxxx>
> ---
> net/ceph/crush/mapper.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/ceph/crush/mapper.c b/net/ceph/crush/mapper.c
> index 3a5bd1cd1..000f7a633 100644
> --- a/net/ceph/crush/mapper.c
> +++ b/net/ceph/crush/mapper.c
> @@ -262,7 +262,7 @@ static __u64 crush_ln(unsigned int xin)
>   * do it in one step instead of iteratively
>   */
>  if (!(x & 0x18000)) {
> - int bits = __builtin_clz(x & 0x1FFFF) - 16;
> + int bits = (x & 0x1FFFF) ? __builtin_clz(x & 0x1FFFF) - 16 : 16;
>  x <<= bits;
>  iexpon = 15 - bits;
>  }

I still have the same issue with the new patch. Your patch is trying to modify
the line 262. However, we have comments on this line [1]:

260	/*
261	 * figure out number of bits we need to shift and
262	 * do it in one step instead of iteratively
263	 */
264	if (!(x & 0x18000)) {
265		int bits = __builtin_clz(x & 0x1FFFF) - 16;
266		x <<= bits;
267		iexpon = 15 - bits;
268	}

Thanks,
Slava.

[1]
https://elixir.bootlin.com/linux/v6.17-rc6/source/net/ceph/crush/mapper.c#L262