linux-kernel - Re: [PATCH] ceph: Fix potential undefined behavior in crush

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] [day] [month] [year] [list]

Message-Id: <A246BD33-C009-4C12-94E7-E95CABB94D04@gmail.com>
Date: Sat, 20 Sep 2025 20:06:48 +0800
From: 陈华昭（Lyican） <lyican53@...il.com>
To: Viacheslav Dubeyko <Slava.Dubeyko@....com>
Cc: "ceph-devel@...r.kernel.org" <ceph-devel@...r.kernel.org>,
 "idryomov@...il.com" <idryomov@...il.com>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 Xiubo Li <xiubli@...hat.com>
Subject: Re: [PATCH] ceph: Fix potential undefined behavior in crush_ln() with
 GCC 11.1.0

> 2025年9月20日 02:51，Viacheslav Dubeyko <Slava.Dubeyko@....com> 写道：
> 
> On Fri, 2025-09-19 at 10:34 +0800, 陈华昭（Lyican） wrote:
>>> 2025年9月19日 02:07，Viacheslav Dubeyko <Slava.Dubeyko@....com> 写道：
>>> 
> 
> <skipped>
> 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
Hi Slava,

Thank you for your patience with this patch. I want to clarify the confusion about the line numbering.

The patch header "@@ -262,7 +262,7 @@" was automatically generated by git format-patch - I did not manually specify line 262. This is how git diff format works: it shows context lines starting from line 262, but the actual code modification is on line 265 where the `__builtin_clz()` call is located (exactly as you referenced in [1]).

To be absolutely clear:
- I am NOT trying to modify line 262 (which contains comments)
- I AM modifying line 265: `int bits = __builtin_clz(x & 0x1FFFF) - 16;`
- The "@@ -262,7 +262,7 @@" header is git's standard way of providing context
- Git automatically chooses how many context lines to show and where to start them

The patch content clearly shows the actual change:
```diff
- int bits = __builtin_clz(x & 0x1FFFF) - 16;
+ int bits = (x & 0x1FFFF) ? __builtin_clz(x & 0x1FFFF) - 16 : 16;
```

This line-by-line diff shows exactly what gets modified - line 265 in the official kernel source.

Here is the git-generated patch:

---

From ac3a55a6a18761d613971ef6f78fa39e6d7d2172 Mon Sep 17 00:00:00 2001
From: Huazhao Chen <lyican53@...il.com>
Date: Sat, 20 Sep 2025 19:42:54 +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 checking if the masked value is 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@...il.com>
---
 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;
  }
-- 
2.39.5 (Apple Git-154)

---

To demonstrate that this is git's automatic behavior and not my manual choice, I can provide the same fix with different context formatting. Here's an alternative patch with less context (generated using `git format-patch -U1`):

```diff
@@ -264,3 +264,3 @@ static __u64 crush_ln(unsigned int xin)
  if (!(x & 0x18000)) {
- int bits = __builtin_clz(x & 0x1FFFF) - 16;
+ int bits = (x & 0x1FFFF) ? __builtin_clz(x & 0x1FFFF) - 16 : 16;
  x <<= bits;
```

As you can see, this version shows "@@ -264,3 +264,3 @@" but still modifies the exact same line - line 265 where `__builtin_clz()` is called. The line numbers in the @@ header are just context indicators, not the target of the modification.

Both patches apply successfully to commit f83ec76bf285bea5727f478a68b894f5543ca76e (Linux 6.17-rc6). I've tested both locally with `git am`. The actual code change is identical in both cases - we're fixing the undefined behavior in the `__builtin_clz()` call on line 265.

I hope this clarifies that the git-generated diff headers don't indicate which line I'm trying to modify, but rather where git chooses to show the context for the patch.

Best regards,
Huazhao Chen

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