generic/013 failure to Samba

generic/013 failure to Samba

[Steve French]

Do you see this dmesg when running generic/013 to Samba (with 6.19-rc2)?

21943.198920] UBSAN: array-index-out-of-bounds in
fs/smb/client/smb2ops.c:1912:27
[21943.198930] index 1 is out of range for type 'srv_copychunk [*]'
[21943.198938] CPU: 6 UID: 0 PID: 13663 Comm: fsstress Kdump: loaded
Not tainted 6.19.
0-rc2+ #16 PREEMPT(voluntary)
[21943.198944] Hardware name: LENOVO 21KAS0JB00/21KAS0JB00, BIOS
R2FET63W (1.43 ) 03/2
0/2025
[21943.198947] Call Trace:
[21943.198951]  <TASK>
[21943.198960]  dump_stack_lvl+0x5f/0x90
[21943.198972]  dump_stack+0x10/0x18
[21943.198976]  ubsan_epilogue+0x9/0x39
[21943.198982]  __ubsan_handle_out_of_bounds.cold+0x50/0x55
[21943.198996]  smb2_copychunk_range+0xa2e/0xc50 [cifs]
[21943.199074]  smb3_fallocate+0xaa3/0xf90 [cifs]
[21943.199116]  ? cap_inode_need_killpriv+0x1e/0x40
[21943.199123]  ? srso_alias_return_thunk+0x5/0xfbef5
[21943.199128]  ? security_inode_need_killpriv+0x4f/0x140
[21943.199135]  ? aa_file_perm+0x68/0x5e0
[21943.199141]  ? srso_alias_return_thunk+0x5/0xfbef5
[21943.199146]  cifs_fallocate+0xfe/0x1a0 [cifs]
[21943.199202]  ? srso_alias_return_thunk+0x5/0xfbef5
[21943.199204]  ? common_file_perm+0x5d/0x1e0
[21943.199209]  vfs_fallocate+0x178/0x3c0
[21943.199218]  __x64_sys_fallocate+0x4a/0xc0
[21943.199221]  ? __do_sys_newfstatat+0x57/0x90
[21943.199227]  x64_sys_call+0x163c/0x2360
[21943.199231]  do_syscall_64+0x82/0x4d0
[21943.199243]  ? srso_alias_return_thunk+0x5/0xfbef5
[21943.199245]  ? __x64_sys_newfstatat+0x1c/0x30
[21943.199248]  ? srso_alias_return_thunk+0x5/0xfbef5
[21943.199250]  ? x64_sys_call+0x1510/0x2360
[21943.199252]  ? srso_alias_return_thunk+0x5/0xfbef5
[21943.199255]  ? do_syscall_64+0xbf/0x4d0
[21943.199259]  ? srso_alias_return_thunk+0x5/0xfbef5
[21943.199262]  ? srso_alias_return_thunk+0x5/0xfbef5
[21943.199264]  ? do_syscall_64+0x271/0x4d0
[21943.199268]  ? srso_alias_return_thunk+0x5/0xfbef5
[21943.199271]  ? __x64_sys_newfstatat+0x1c/0x30
[21943.199273]  ? srso_alias_return_thunk+0x5/0xfbef5
[21943.199276]  ? x64_sys_call+0x1510/0x2360
[21943.199278]  ? srso_alias_return_thunk+0x5/0xfbef5
[21943.199280]  ? do_syscall_64+0xbf/0x4d0
[21943.199284]  ? srso_alias_return_thunk+0x5/0xfbef5
[21943.199287]  ? do_user_addr_fault+0x2ee/0x830
[21943.199293]  ? srso_alias_return_thunk+0x5/0xfbef5
[21943.199295]  ? irqentry_exit+0xa5/0x600
[21943.199300]  ? srso_alias_return_thunk+0x5/0xfbef5
[21943.199303]  ? exc_page_fault+0x90/0x1b0
[21943.199306]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
[21943.199308] RIP: 0033:0x74b2186a0186
[21943.199312] Code: 47 ba 04 00 00 00 48 8b 05 87 3c 19 00 64 89 10
48 c7 c2 ff ff ff
 ff c9 48 89 d0 c3 0f 1f 84 00 00 00 00 00 48 8b 45 10 0f 05 <48> 89
c2 48 3d 00 f0 ff
 ff 77 0f c9 48 89 d0 c3 66 2e 0f 1f 84 00
[21943.199314] RSP: 002b:00007ffd142f3610 EFLAGS: 00000202 ORIG_RAX:
000000000000011d
[21943.199318] RAX: ffffffffffffffda RBX: 00007ffd142f3710 RCX: 000074b2186a0186
[21943.199320] RDX: 000000000095c82c RSI: 0000000000000020 RDI: 0000000000000004
[21943.199322] RBP: 00007ffd142f3620 R08: 0000000000000000 R09: 0000000000000000
[21943.199324] R10: 00000000000011e7 R11: 0000000000000202 R12: 0000000000000004
[21943.199325] R13: 00000000000011e7 R14: 0000000000000054 R15: 0000000000000020
[21943.199331]  </TASK>


-- 
Thanks,

Steve

Re: generic/013 failure to Samba

[Henrique Carvalho]

[-- Attachment #1: Type: text/plain, Size: 759 bytes --]

This UBSAN report is consistent with struct copychunk_ioctl_req::Chunks[]
being annotated with __counted_by_le(ChunkCount) while ChunkCount is not
set to the allocated capacity before we start populating the array. This is
the same class of issue described in [1].

A fix would be to set ChunkCount to chunk_count (capacity) at the start
of each iteration before accessing Chunks[]. Proposed patch is attached.

However, if my interpretation is correct, I would expect the first
population to trip as well since ChunkCount starts at 0, which does not
happen.

@Gustavo do you have any insight into why the first access might not
trigger?

[1] https://people.kernel.org/gustavoars/how-to-use-the-new-counted_by-attribute-in-c-and-linux

-- 
Henrique
SUSE Labs

[-- Attachment #2: 0001-smb-client-fix-UBSAN-array-index-out-of-bounds-in-sm.patch --]
[-- Type: text/x-patch, Size: 1420 bytes --]

From 9ebdd134dd01f19219b7f1634e2b577fe5778824 Mon Sep 17 00:00:00 2001
From: Henrique Carvalho <henrique.carvalho@suse.com>
Date: Wed, 24 Dec 2025 01:17:25 -0300
Subject: [PATCH] smb: client: fix UBSAN array-index-out-of-bounds in
 smb2_copychunk_range

struct copychunk_ioctl_req::ChunkCount is annotated with
__counted_by_le() as the number of elements in Chunks[].

smb2_copychunk_range reuses ChunkCount to store the number of chunks
sent in the current iteration. If a later iteration populates more
chunks than a previous one, the stale smaller value trips UBSAN.

Set ChunkCount to chunk_count (allocated capacity) before populating
Chunks[].

Fixes: cc26f593dc19 ("smb: move copychunk definitions to common/smb2pdu.h")
Signed-off-by: Henrique Carvalho <henrique.carvalho@suse.com>
---
 fs/smb/client/smb2ops.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
index a16ded46b5a2..c1aaf77e187b 100644
--- a/fs/smb/client/smb2ops.c
+++ b/fs/smb/client/smb2ops.c
@@ -1905,6 +1905,12 @@ smb2_copychunk_range(const unsigned int xid,
 		src_off_prev = src_off;
 		dst_off_prev = dst_off;
 
+		/*
+		 * __counted_by_le(ChunkCount): set to allocated chunks before
+		 * populating Chunks[]
+		 */
+		cc_req->ChunkCount = cpu_to_le32(chunk_count);
+
 		chunks = 0;
 		copy_bytes = 0;
 		copy_bytes_left = umin(total_bytes_left, tcon->max_bytes_copy);
-- 
2.50.1

Re: generic/013 failure to Samba

[ChenXiaoSong]

Hi Henrique,

I can reproduce this UBSAN error. You need to compile the kernel code 
using the latest version of Clang.

I would like to give special thanks to Tang Youling 
<tangyouling@kylinos.cn> for his guidance on UBSAN and Clang.

Thanks,
ChenXiaoSong <chenxiaosong@kylinos.cn>

On 12/24/25 23:02, Henrique Carvalho wrote:
> This UBSAN report is consistent with struct copychunk_ioctl_req::Chunks[]
> being annotated with __counted_by_le(ChunkCount) while ChunkCount is not
> set to the allocated capacity before we start populating the array. This is
> the same class of issue described in [1].
> 
> A fix would be to set ChunkCount to chunk_count (capacity) at the start
> of each iteration before accessing Chunks[]. Proposed patch is attached.
> 
> However, if my interpretation is correct, I would expect the first
> population to trip as well since ChunkCount starts at 0, which does not
> happen.
> 
> @Gustavo do you have any insight into why the first access might not
> trigger?
> 
> [1] https://people.kernel.org/gustavoars/how-to-use-the-new-counted_by-attribute-in-c-and-linux
> 

Re: generic/013 failure to Samba

[ChenXiaoSong]

Hi Henrique,

The following is the modifications I suggest. If you have a better 
solution, please let me know.

If you agree with my modifications, could you send the v2 patch?

Give special thanks once again to Youling Tang <tangyouling@kylinos.cn> 
for his guidance on UBSAN and Clang.

If you build the kernel code with the latest version Clang (I am using 
version 21.1.7), and `CONFIG_UBSAN_BOUNDS` has been enabled, you will be 
able to see this UBSAN error every time.

It seems that we need to add two Fixes tags:
Fixes: 68d2e2ca1cba ("smb: client: batch SRV_COPYCHUNK entries to cut 
round trips")
Fixes: cc26f593dc19 ("smb: move copychunk definitions to common/smb2pdu.h")

The key modifications are as follows:
```
smb2_copychunk_range()
{
	// remove `chunk_count`, and use only `cc_req->ChunkCount`
	...
	cc_req->ChunkCount = 0;
	while (copy_bytes_left > 0 && cc_req->ChunkCount < chunk_count) {
		cc_req->ChunkCount++;
		chunk = &cc_req->Chunks[cc_req->ChunkCount - 1];
		...
	}
	...
}
```

Thanks,
ChenXiaoSong <chenxiaosong@kylinos.cn>

On 12/26/25 12:36, ChenXiaoSong wrote:
> Hi Henrique,
> 
> I can reproduce this UBSAN error. You need to compile the kernel code 
> using the latest version of Clang.
> 
> I would like to give special thanks to Tang Youling 
> <tangyouling@kylinos.cn> for his guidance on UBSAN and Clang.
> 
> Thanks,
> ChenXiaoSong <chenxiaosong@kylinos.cn>
> 
> On 12/24/25 23:02, Henrique Carvalho wrote:
>> This UBSAN report is consistent with struct copychunk_ioctl_req::Chunks[]
>> being annotated with __counted_by_le(ChunkCount) while ChunkCount is not
>> set to the allocated capacity before we start populating the array. 
>> This is
>> the same class of issue described in [1].
>>
>> A fix would be to set ChunkCount to chunk_count (capacity) at the start
>> of each iteration before accessing Chunks[]. Proposed patch is attached.
>>
>> However, if my interpretation is correct, I would expect the first
>> population to trip as well since ChunkCount starts at 0, which does not
>> happen.
>>
>> @Gustavo do you have any insight into why the first access might not
>> trigger?
>>
>> [1] https://people.kernel.org/gustavoars/how-to-use-the-new- 
>> counted_by-attribute-in-c-and-linux
>>
> 

Re: generic/013 failure to Samba

[ChenXiaoSong]

Sorry, there is a typo. It should be "remove `chunks`", not "remove 
`chunk_count`".

Thanks,
ChenXiaoSong <chenxiaosong@kylinos.cn>

On 12/26/25 14:44, ChenXiaoSong wrote:
>      // remove `chunk_count`, and use only `cc_req->ChunkCount`

Re: generic/013 failure to Samba

[Henrique Carvalho]

Hi ChenXiaoSong,

On Fri, Dec 26, 2025 at 02:44:26PM +0800, ChenXiaoSong wrote:
> Hi Henrique,
> 
> The following is the modifications I suggest. If you have a better solution,
> please let me know.
> 
> If you agree with my modifications, could you send the v2 patch?
> 
> Give special thanks once again to Youling Tang <tangyouling@kylinos.cn> for
> his guidance on UBSAN and Clang.
> 
> If you build the kernel code with the latest version Clang (I am using
> version 21.1.7), and `CONFIG_UBSAN_BOUNDS` has been enabled, you will be
> able to see this UBSAN error every time.
> 
> It seems that we need to add two Fixes tags:
> Fixes: 68d2e2ca1cba ("smb: client: batch SRV_COPYCHUNK entries to cut round
> trips")
> Fixes: cc26f593dc19 ("smb: move copychunk definitions to common/smb2pdu.h")
> 
> The key modifications are as follows:
> ```
> smb2_copychunk_range()
> {
> 	// remove `chunk_count`, and use only `cc_req->ChunkCount`
> 	...
> 	cc_req->ChunkCount = 0;
> 	while (copy_bytes_left > 0 && cc_req->ChunkCount < chunk_count) {
> 		cc_req->ChunkCount++;
> 		chunk = &cc_req->Chunks[cc_req->ChunkCount - 1];
> 		...
> 	}
> 	...
> }
> ```

Thanks for looking into this and for the reproduction details.

I don't agree with the proposed changes for a couple of reasons:

1. ChunkCount is an on-wire __le32. Using it as the loop counter mixes
endianness-annotated storage with CPU-endian arithmetic and would either
require constant cpu_to_le32()/le32_to_cpu() conversions or be
type/endianness misuse. The current CPU-endian u32 chunks counter is
intentional.

2. Re: "Fixes:" tags: the UBSAN report exists because Chunks[] is now
__counted_by_le(ChunkCount). We currently index Chunks[] while
ChunkCount is still zero, so the __counted_by contract is violated. That
points to the commit that introduced the annotation as the Fixes target.
I don't think the batching commit should be tagged unless it introduced
a functional issue independent of the annotation.

Also, my earlier comment about the "first population" was wrong: it does
trigger, but the report appears to be emitted once per call site (subsequent
occurrences suppressed), which matches what I'm seeing.

-- 
Henrique
SUSE Labs

Re: generic/013 failure to Samba

[ChenXiaoSong]

Yes, you are correct. I didn't consider `cpu_to_le32()`.

How about changing it to the following?
```
smb2_copychunk_range()
{
	...
	chunks = 0;
	while (copy_bytes_left > 0 && chunks < chunk_count) {
		cc_req->ChunkCount = cpu_to_le32(++chunks);
		chunk = &cc_req->Chunks[chunks - 1];
		...
	}
	...
}
```

I also agree with your point about the Fixes tag.

Thanks,
ChenXiaoSong.

On 12/26/25 11:45 PM, Henrique Carvalho wrote:
> I don't agree with the proposed changes for a couple of reasons:
> 
> 1. ChunkCount is an on-wire __le32. Using it as the loop counter mixes
> endianness-annotated storage with CPU-endian arithmetic and would either
> require constant cpu_to_le32()/le32_to_cpu() conversions or be
> type/endianness misuse. The current CPU-endian u32 chunks counter is
> intentional.
> 
> 2. Re: "Fixes:" tags: the UBSAN report exists because Chunks[] is now
> __counted_by_le(ChunkCount). We currently index Chunks[] while
> ChunkCount is still zero, so the __counted_by contract is violated. That
> points to the commit that introduced the annotation as the Fixes target.
> I don't think the batching commit should be tagged unless it introduced
> a functional issue independent of the annotation.

Re: generic/013 failure to Samba

[Henrique Carvalho]

On Sat, Dec 27, 2025 at 12:01:18AM +0800, ChenXiaoSong wrote:
> Yes, you are correct. I didn't consider `cpu_to_le32()`.
> 
> How about changing it to the following?
> ```
> smb2_copychunk_range()
> {
> 	...
> 	chunks = 0;
> 	while (copy_bytes_left > 0 && chunks < chunk_count) {
> 		cc_req->ChunkCount = cpu_to_le32(++chunks);
> 		chunk = &cc_req->Chunks[chunks - 1];
> 		...
> 	}
> 	...
> }
> ```
>

Your change does make the code semantically tighter, since ChunkCount would
track initialized elements as we populate the array.

That said, I still slightly prefer setting ChunkCount to the allocated
capacity before we first index Chunks[], and then setting it to the final
chunks value before the IOCTL.

This both satisfies __counted_by_le() during population, it isn't wrong
given the allocation is chunk_count, and avoids an extra ChunkCount
update on every chunk entry (in my build this is not optimized away).
It's cheap either way, but if we can avoid per-iteration overhead, I'd
rather do so.

What do you think? Do you see any correctness or tooling downside with
this approach?

Thanks,

-- 
Henrique
SUSE Labs

Re: generic/013 failure to Samba

[ChenXiaoSong]

I think the comment in your change is easy to understand. Looks good to me.

Steve, what do you think? I see that you have already sent the pull 
request for 6.19-rc3. Should we merge Henrique's patch in rc3?

Thanks,
ChenXiaoSong <chenxiaosong@kylinos.cn>

On 12/27/25 2:05 AM, Henrique Carvalho wrote:
> Your change does make the code semantically tighter, since ChunkCount would
> track initialized elements as we populate the array.
> 
> That said, I still slightly prefer setting ChunkCount to the allocated
> capacity before we first index Chunks[], and then setting it to the final
> chunks value before the IOCTL.
> 
> This both satisfies __counted_by_le() during population, it isn't wrong
> given the allocation is chunk_count, and avoids an extra ChunkCount
> update on every chunk entry (in my build this is not optimized away).
> It's cheap either way, but if we can avoid per-iteration overhead, I'd
> rather do so.
> 
> What do you think? Do you see any correctness or tooling downside with
> this approach?

Re: generic/013 failure to Samba

[ChenXiaoSong]

On 12/24/25 11:02 PM, Henrique Carvalho wrote:
> If a later iteration populates more
> chunks than a previous one, the stale smaller value trips UBSAN.

The commit message may also need to be updated, since the UBSAN error 
can be triggered on the first loop iteration.

Thanks,
ChenXiaoSong <chenxiaosong@kylinos.cn>

On 12/27/25 7:02 AM, ChenXiaoSong wrote:
> Hi Henrique,
> 
> Thanks for your patch.
> 
> You could resend the patch as a standalone email instead of an 
> attachment, and include some additional information, such as links to 
> the relevant mailing list discussions.
> 
> Additionally, please add my colleague's Tested-by: "Tested-by: Youling 
> Tang <tangyouling@kylinos.cn>".
> 
> Thanks,
> ChenXiaoSong <chenxiaosong@kylinos.cn>
> 
> On 12/27/25 6:51 AM, Steve French wrote:
>> It should bake in for-next for at least a few days so can send next to 
>> Linus Tuesday or Wednesday if reviews and tests ok
> 

Re: generic/013 failure to Samba

[Henrique Carvalho]

On Wed, Dec 24, 2025 at 12:02:22PM -0300, Henrique Carvalho wrote:
> This UBSAN report is consistent with struct copychunk_ioctl_req::Chunks[]
> being annotated with __counted_by_le(ChunkCount) while ChunkCount is not
> set to the allocated capacity before we start populating the array. This is
> the same class of issue described in [1].
> 
> A fix would be to set ChunkCount to chunk_count (capacity) at the start
> of each iteration before accessing Chunks[]. Proposed patch is attached.
> 
> However, if my interpretation is correct, I would expect the first
> population to trip as well since ChunkCount starts at 0, which does not
> happen.
> 
> @Gustavo do you have any insight into why the first access might not
> trigger?
> 

I need to correct myself: it's not that the first population "does not
trip", but rather that we only see one UBSAN report. Subsequent
array-out-of-bounds events may still hit the handler but are probably
suppressed after the first report.

So this behavior is actually consistent with my interpretation of the
issue.

-- 
Henrique
SUSE Labs