[PATCH 2/2] s390/fpu: Fix kmsan in fpu_vstl function

Aleksei Nikiforov posted 2 patches 1 month, 1 week ago
[PATCH 2/2] s390/fpu: Fix kmsan in fpu_vstl function
Posted by Aleksei Nikiforov 1 month, 1 week ago
clang generates call to __msan_instrument_asm_store with 1 byte as size.
Manually call kmsan helper to indicate correct amount of bytes written.

If function fpu_vstl is called with argument 'index' > 0,
it writes at least 2 bytes, but kmsan only marks first byte as written.

This change fixes following kmsan reports:

[   36.563119] =====================================================
[   36.563594] BUG: KMSAN: uninit-value in virtqueue_add+0x35c6/0x7c70
[   36.563852]  virtqueue_add+0x35c6/0x7c70
[   36.564016]  virtqueue_add_outbuf+0xa0/0xb0
[   36.564266]  start_xmit+0x288c/0x4a20
[   36.564460]  dev_hard_start_xmit+0x302/0x900
[   36.564649]  sch_direct_xmit+0x340/0xea0
[   36.564894]  __dev_queue_xmit+0x2e94/0x59b0
[   36.565058]  neigh_resolve_output+0x936/0xb40
[   36.565278]  __neigh_update+0x2f66/0x3a60
[   36.565499]  neigh_update+0x52/0x60
[   36.565683]  arp_process+0x1588/0x2de0
[   36.565916]  NF_HOOK+0x1da/0x240
[   36.566087]  arp_rcv+0x3e4/0x6e0
[   36.566306]  __netif_receive_skb_list_core+0x1374/0x15a0
[   36.566527]  netif_receive_skb_list_internal+0x1116/0x17d0
[   36.566710]  napi_complete_done+0x376/0x740
[   36.566918]  virtnet_poll+0x1bae/0x2910
[   36.567130]  __napi_poll+0xf4/0x830
[   36.567294]  net_rx_action+0x97c/0x1ed0
[   36.567556]  handle_softirqs+0x306/0xe10
[   36.567731]  irq_exit_rcu+0x14c/0x2e0
[   36.567910]  do_io_irq+0xd4/0x120
[   36.568139]  io_int_handler+0xc2/0xe8
[   36.568299]  arch_cpu_idle+0xb0/0xc0
[   36.568540]  arch_cpu_idle+0x76/0xc0
[   36.568726]  default_idle_call+0x40/0x70
[   36.568953]  do_idle+0x1d6/0x390
[   36.569486]  cpu_startup_entry+0x9a/0xb0
[   36.569745]  rest_init+0x1ea/0x290
[   36.570029]  start_kernel+0x95e/0xb90
[   36.570348]  startup_continue+0x2e/0x40
[   36.570703]
[   36.570798] Uninit was created at:
[   36.571002]  kmem_cache_alloc_node_noprof+0x9e8/0x10e0
[   36.571261]  kmalloc_reserve+0x12a/0x470
[   36.571553]  __alloc_skb+0x310/0x860
[   36.571844]  __ip_append_data+0x483e/0x6a30
[   36.572170]  ip_append_data+0x11c/0x1e0
[   36.572477]  raw_sendmsg+0x1c8c/0x2180
[   36.572818]  inet_sendmsg+0xe6/0x190
[   36.573142]  __sys_sendto+0x55e/0x8e0
[   36.573392]  __s390x_sys_socketcall+0x19ae/0x2ba0
[   36.573571]  __do_syscall+0x12e/0x240
[   36.573823]  system_call+0x6e/0x90
[   36.573976]
[   36.574017] Byte 35 of 98 is uninitialized
[   36.574082] Memory access of size 98 starts at 0000000007aa0012
[   36.574218]
[   36.574325] CPU: 0 UID: 0 PID: 0 Comm: swapper/0 Tainted: G    B            N  6.17.0-dirty #16 NONE
[   36.574541] Tainted: [B]=BAD_PAGE, [N]=TEST
[   36.574617] Hardware name: IBM 3931 A01 703 (KVM/Linux)
[   36.574755] =====================================================

[   63.532541] =====================================================
[   63.533639] BUG: KMSAN: uninit-value in virtqueue_add+0x35c6/0x7c70
[   63.533989]  virtqueue_add+0x35c6/0x7c70
[   63.534940]  virtqueue_add_outbuf+0xa0/0xb0
[   63.535861]  start_xmit+0x288c/0x4a20
[   63.536708]  dev_hard_start_xmit+0x302/0x900
[   63.537020]  sch_direct_xmit+0x340/0xea0
[   63.537997]  __dev_queue_xmit+0x2e94/0x59b0
[   63.538819]  neigh_resolve_output+0x936/0xb40
[   63.539793]  ip_finish_output2+0x1ee2/0x2200
[   63.540784]  __ip_finish_output+0x272/0x7a0
[   63.541765]  ip_finish_output+0x4e/0x5e0
[   63.542791]  ip_output+0x166/0x410
[   63.543771]  ip_push_pending_frames+0x1a2/0x470
[   63.544753]  raw_sendmsg+0x1f06/0x2180
[   63.545033]  inet_sendmsg+0xe6/0x190
[   63.546006]  __sys_sendto+0x55e/0x8e0
[   63.546859]  __s390x_sys_socketcall+0x19ae/0x2ba0
[   63.547730]  __do_syscall+0x12e/0x240
[   63.548019]  system_call+0x6e/0x90
[   63.548989]
[   63.549779] Uninit was created at:
[   63.550691]  kmem_cache_alloc_node_noprof+0x9e8/0x10e0
[   63.550975]  kmalloc_reserve+0x12a/0x470
[   63.551969]  __alloc_skb+0x310/0x860
[   63.552949]  __ip_append_data+0x483e/0x6a30
[   63.553902]  ip_append_data+0x11c/0x1e0
[   63.554912]  raw_sendmsg+0x1c8c/0x2180
[   63.556719]  inet_sendmsg+0xe6/0x190
[   63.557534]  __sys_sendto+0x55e/0x8e0
[   63.557875]  __s390x_sys_socketcall+0x19ae/0x2ba0
[   63.558869]  __do_syscall+0x12e/0x240
[   63.559832]  system_call+0x6e/0x90
[   63.560780]
[   63.560972] Byte 35 of 98 is uninitialized
[   63.561741] Memory access of size 98 starts at 0000000005704312
[   63.561950]
[   63.562824] CPU: 3 UID: 0 PID: 192 Comm: ping Tainted: G    B            N  6.17.0-dirty #16 NONE
[   63.563868] Tainted: [B]=BAD_PAGE, [N]=TEST
[   63.564751] Hardware name: IBM 3931 A01 703 (KVM/Linux)
[   63.564986] =====================================================

Fixes: dcd3e1de9d17 ("s390/checksum: provide csum_partial_copy_nocheck()")
Reviewed-by: Heiko Carstens <hca@linux.ibm.com>
Signed-off-by: Aleksei Nikiforov <aleksei.nikiforov@linux.ibm.com>
---
 arch/s390/include/asm/fpu-insn.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/s390/include/asm/fpu-insn.h b/arch/s390/include/asm/fpu-insn.h
index 135bb89c0a89..151b17e22923 100644
--- a/arch/s390/include/asm/fpu-insn.h
+++ b/arch/s390/include/asm/fpu-insn.h
@@ -393,6 +393,7 @@ static __always_inline void fpu_vstl(u8 v1, u32 index, const void *vxr)
 		     : [vxr] "=Q" (*(u8 *)vxr)
 		     : [index] "d" (index), [v1] "I" (v1)
 		     : "memory");
+	instrument_write_after(vxr, size);
 }
 
 #else /* CONFIG_CC_HAS_ASM_AOR_FORMAT_FLAGS */
@@ -409,6 +410,7 @@ static __always_inline void fpu_vstl(u8 v1, u32 index, const void *vxr)
 		: [vxr] "=R" (*(u8 *)vxr)
 		: [index] "d" (index), [v1] "I" (v1)
 		: "memory", "1");
+	instrument_write_after(vxr, size);
 }
 
 #endif /* CONFIG_CC_HAS_ASM_AOR_FORMAT_FLAGS */
-- 
2.43.7
Re: [PATCH 2/2] s390/fpu: Fix kmsan in fpu_vstl function
Posted by Alexander Potapenko 1 month, 1 week ago
On Thu, Nov 6, 2025 at 5:09 PM Aleksei Nikiforov
<aleksei.nikiforov@linux.ibm.com> wrote:
>
> clang generates call to __msan_instrument_asm_store with 1 byte as size.
> Manually call kmsan helper to indicate correct amount of bytes written.
>
> If function fpu_vstl is called with argument 'index' > 0,
> it writes at least 2 bytes, but kmsan only marks first byte as written.
>
> This change fixes following kmsan reports:
>
> [   36.563119] =====================================================
> [   36.563594] BUG: KMSAN: uninit-value in virtqueue_add+0x35c6/0x7c70
> [   36.563852]  virtqueue_add+0x35c6/0x7c70
> [   36.564016]  virtqueue_add_outbuf+0xa0/0xb0
> [   36.564266]  start_xmit+0x288c/0x4a20
> [   36.564460]  dev_hard_start_xmit+0x302/0x900
> [   36.564649]  sch_direct_xmit+0x340/0xea0
> [   36.564894]  __dev_queue_xmit+0x2e94/0x59b0
> [   36.565058]  neigh_resolve_output+0x936/0xb40
> [   36.565278]  __neigh_update+0x2f66/0x3a60
> [   36.565499]  neigh_update+0x52/0x60
> [   36.565683]  arp_process+0x1588/0x2de0
> [   36.565916]  NF_HOOK+0x1da/0x240
> [   36.566087]  arp_rcv+0x3e4/0x6e0
> [   36.566306]  __netif_receive_skb_list_core+0x1374/0x15a0
> [   36.566527]  netif_receive_skb_list_internal+0x1116/0x17d0
> [   36.566710]  napi_complete_done+0x376/0x740
> [   36.566918]  virtnet_poll+0x1bae/0x2910
> [   36.567130]  __napi_poll+0xf4/0x830
> [   36.567294]  net_rx_action+0x97c/0x1ed0
> [   36.567556]  handle_softirqs+0x306/0xe10
> [   36.567731]  irq_exit_rcu+0x14c/0x2e0
> [   36.567910]  do_io_irq+0xd4/0x120
> [   36.568139]  io_int_handler+0xc2/0xe8
> [   36.568299]  arch_cpu_idle+0xb0/0xc0
> [   36.568540]  arch_cpu_idle+0x76/0xc0
> [   36.568726]  default_idle_call+0x40/0x70
> [   36.568953]  do_idle+0x1d6/0x390
> [   36.569486]  cpu_startup_entry+0x9a/0xb0
> [   36.569745]  rest_init+0x1ea/0x290
> [   36.570029]  start_kernel+0x95e/0xb90
> [   36.570348]  startup_continue+0x2e/0x40
> [   36.570703]
> [   36.570798] Uninit was created at:
> [   36.571002]  kmem_cache_alloc_node_noprof+0x9e8/0x10e0
> [   36.571261]  kmalloc_reserve+0x12a/0x470
> [   36.571553]  __alloc_skb+0x310/0x860
> [   36.571844]  __ip_append_data+0x483e/0x6a30
> [   36.572170]  ip_append_data+0x11c/0x1e0
> [   36.572477]  raw_sendmsg+0x1c8c/0x2180
> [   36.572818]  inet_sendmsg+0xe6/0x190
> [   36.573142]  __sys_sendto+0x55e/0x8e0
> [   36.573392]  __s390x_sys_socketcall+0x19ae/0x2ba0
> [   36.573571]  __do_syscall+0x12e/0x240
> [   36.573823]  system_call+0x6e/0x90
> [   36.573976]
> [   36.574017] Byte 35 of 98 is uninitialized
> [   36.574082] Memory access of size 98 starts at 0000000007aa0012
> [   36.574218]
> [   36.574325] CPU: 0 UID: 0 PID: 0 Comm: swapper/0 Tainted: G    B            N  6.17.0-dirty #16 NONE
> [   36.574541] Tainted: [B]=BAD_PAGE, [N]=TEST
> [   36.574617] Hardware name: IBM 3931 A01 703 (KVM/Linux)
> [   36.574755] =====================================================
>
> [   63.532541] =====================================================
> [   63.533639] BUG: KMSAN: uninit-value in virtqueue_add+0x35c6/0x7c70
> [   63.533989]  virtqueue_add+0x35c6/0x7c70
> [   63.534940]  virtqueue_add_outbuf+0xa0/0xb0
> [   63.535861]  start_xmit+0x288c/0x4a20
> [   63.536708]  dev_hard_start_xmit+0x302/0x900
> [   63.537020]  sch_direct_xmit+0x340/0xea0
> [   63.537997]  __dev_queue_xmit+0x2e94/0x59b0
> [   63.538819]  neigh_resolve_output+0x936/0xb40
> [   63.539793]  ip_finish_output2+0x1ee2/0x2200
> [   63.540784]  __ip_finish_output+0x272/0x7a0
> [   63.541765]  ip_finish_output+0x4e/0x5e0
> [   63.542791]  ip_output+0x166/0x410
> [   63.543771]  ip_push_pending_frames+0x1a2/0x470
> [   63.544753]  raw_sendmsg+0x1f06/0x2180
> [   63.545033]  inet_sendmsg+0xe6/0x190
> [   63.546006]  __sys_sendto+0x55e/0x8e0
> [   63.546859]  __s390x_sys_socketcall+0x19ae/0x2ba0
> [   63.547730]  __do_syscall+0x12e/0x240
> [   63.548019]  system_call+0x6e/0x90
> [   63.548989]
> [   63.549779] Uninit was created at:
> [   63.550691]  kmem_cache_alloc_node_noprof+0x9e8/0x10e0
> [   63.550975]  kmalloc_reserve+0x12a/0x470
> [   63.551969]  __alloc_skb+0x310/0x860
> [   63.552949]  __ip_append_data+0x483e/0x6a30
> [   63.553902]  ip_append_data+0x11c/0x1e0
> [   63.554912]  raw_sendmsg+0x1c8c/0x2180
> [   63.556719]  inet_sendmsg+0xe6/0x190
> [   63.557534]  __sys_sendto+0x55e/0x8e0
> [   63.557875]  __s390x_sys_socketcall+0x19ae/0x2ba0
> [   63.558869]  __do_syscall+0x12e/0x240
> [   63.559832]  system_call+0x6e/0x90
> [   63.560780]
> [   63.560972] Byte 35 of 98 is uninitialized
> [   63.561741] Memory access of size 98 starts at 0000000005704312
> [   63.561950]
> [   63.562824] CPU: 3 UID: 0 PID: 192 Comm: ping Tainted: G    B            N  6.17.0-dirty #16 NONE
> [   63.563868] Tainted: [B]=BAD_PAGE, [N]=TEST
> [   63.564751] Hardware name: IBM 3931 A01 703 (KVM/Linux)
> [   63.564986] =====================================================
>
> Fixes: dcd3e1de9d17 ("s390/checksum: provide csum_partial_copy_nocheck()")
> Reviewed-by: Heiko Carstens <hca@linux.ibm.com>
> Signed-off-by: Aleksei Nikiforov <aleksei.nikiforov@linux.ibm.com>
> ---
>  arch/s390/include/asm/fpu-insn.h | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/arch/s390/include/asm/fpu-insn.h b/arch/s390/include/asm/fpu-insn.h
> index 135bb89c0a89..151b17e22923 100644
> --- a/arch/s390/include/asm/fpu-insn.h
> +++ b/arch/s390/include/asm/fpu-insn.h
> @@ -393,6 +393,7 @@ static __always_inline void fpu_vstl(u8 v1, u32 index, const void *vxr)
>                      : [vxr] "=Q" (*(u8 *)vxr)
>                      : [index] "d" (index), [v1] "I" (v1)
>                      : "memory");
> +       instrument_write_after(vxr, size);
>  }
>
>  #else /* CONFIG_CC_HAS_ASM_AOR_FORMAT_FLAGS */
> @@ -409,6 +410,7 @@ static __always_inline void fpu_vstl(u8 v1, u32 index, const void *vxr)
>                 : [vxr] "=R" (*(u8 *)vxr)
>                 : [index] "d" (index), [v1] "I" (v1)
>                 : "memory", "1");
> +       instrument_write_after(vxr, size);
>  }

Wouldn't it be easier to just call kmsan_unpoison_memory() here directly?
Re: [PATCH 2/2] s390/fpu: Fix kmsan in fpu_vstl function
Posted by Heiko Carstens 1 month, 1 week ago
On Fri, Nov 07, 2025 at 11:26:50AM +0100, Alexander Potapenko wrote:
> On Thu, Nov 6, 2025 at 5:09 PM Aleksei Nikiforov
> <aleksei.nikiforov@linux.ibm.com> wrote:
> > @@ -409,6 +410,7 @@ static __always_inline void fpu_vstl(u8 v1, u32 index, const void *vxr)
> >                 : [vxr] "=R" (*(u8 *)vxr)
> >                 : [index] "d" (index), [v1] "I" (v1)
> >                 : "memory", "1");
> > +       instrument_write_after(vxr, size);
> >  }
> 
> Wouldn't it be easier to just call kmsan_unpoison_memory() here directly?

I guess that's your call. Looks like we have already a couple of
kmsan_unpoison_memory() behind inline assemblies.

So I guess we should either continue using kmsan_unpoison_memory()
directly, or convert all of them to such a new helper. Both works of
course. What do you prefer?
Re: [PATCH 2/2] s390/fpu: Fix kmsan in fpu_vstl function
Posted by Alexander Potapenko 1 month, 1 week ago
On Fri, Nov 7, 2025 at 11:49 AM Heiko Carstens <hca@linux.ibm.com> wrote:
>
> On Fri, Nov 07, 2025 at 11:26:50AM +0100, Alexander Potapenko wrote:
> > On Thu, Nov 6, 2025 at 5:09 PM Aleksei Nikiforov
> > <aleksei.nikiforov@linux.ibm.com> wrote:
> > > @@ -409,6 +410,7 @@ static __always_inline void fpu_vstl(u8 v1, u32 index, const void *vxr)
> > >                 : [vxr] "=R" (*(u8 *)vxr)
> > >                 : [index] "d" (index), [v1] "I" (v1)
> > >                 : "memory", "1");
> > > +       instrument_write_after(vxr, size);
> > >  }
> >
> > Wouldn't it be easier to just call kmsan_unpoison_memory() here directly?
>
> I guess that's your call. Looks like we have already a couple of
> kmsan_unpoison_memory() behind inline assemblies.
>
> So I guess we should either continue using kmsan_unpoison_memory()
> directly, or convert all of them to such a new helper. Both works of
> course. What do you prefer?

Upon reflection, I think adding instrument_write_after() is not the best idea.
For tools like KASAN and KCSAN, every write has the same semantics,
and the instrumentation just notifies the tool that the write
occurred.
For KMSAN, however, writes may affect metadata differently, requiring
us to either poison or unpoison the destination.
In certain special cases, like instrument_get_user() or
instrument_copy_from_user() the semantics are always fixed, but this
is not true for arbitrary writes.

We could make the new annotation's name more verbose, but it will just
become a synonym of kmsan_unpoison_memory().
So I suggest sticking with kmsan_unpoison_memory() for now.


-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Liana Sebastian
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Re: [PATCH 2/2] s390/fpu: Fix kmsan in fpu_vstl function
Posted by Aleksei Nikiforov 1 month, 1 week ago
On 11/7/25 14:32, Alexander Potapenko wrote:
> On Fri, Nov 7, 2025 at 11:49 AM Heiko Carstens <hca@linux.ibm.com> wrote:
>>
>> On Fri, Nov 07, 2025 at 11:26:50AM +0100, Alexander Potapenko wrote:
>>> On Thu, Nov 6, 2025 at 5:09 PM Aleksei Nikiforov
>>> <aleksei.nikiforov@linux.ibm.com> wrote:
>>>> @@ -409,6 +410,7 @@ static __always_inline void fpu_vstl(u8 v1, u32 index, const void *vxr)
>>>>                  : [vxr] "=R" (*(u8 *)vxr)
>>>>                  : [index] "d" (index), [v1] "I" (v1)
>>>>                  : "memory", "1");
>>>> +       instrument_write_after(vxr, size);
>>>>   }
>>>
>>> Wouldn't it be easier to just call kmsan_unpoison_memory() here directly?
>>
>> I guess that's your call. Looks like we have already a couple of
>> kmsan_unpoison_memory() behind inline assemblies.
>>
>> So I guess we should either continue using kmsan_unpoison_memory()
>> directly, or convert all of them to such a new helper. Both works of
>> course. What do you prefer?
> 
> Upon reflection, I think adding instrument_write_after() is not the best idea.
> For tools like KASAN and KCSAN, every write has the same semantics,
> and the instrumentation just notifies the tool that the write
> occurred.
> For KMSAN, however, writes may affect metadata differently, requiring
> us to either poison or unpoison the destination.
> In certain special cases, like instrument_get_user() or
> instrument_copy_from_user() the semantics are always fixed, but this
> is not true for arbitrary writes.
> 
> We could make the new annotation's name more verbose, but it will just
> become a synonym of kmsan_unpoison_memory().
> So I suggest sticking with kmsan_unpoison_memory() for now.
> 
> 

I'll rework changes with that suggestion. Thank you.