Re: [PATCH 1/1] vhost: fix GCC 13 build error

[Maxime Coquelin <maxime.coquelin@redhat.com>]

On 4/10/24 17:21, Luca Vizzarro wrote:
> This patch resolves a build error with GCC 13 and arm/aarch32 as
> targets:
> 
> In function ‘mbuf_to_desc’,
>      inlined from ‘vhost_enqueue_async_packed’ at
>        ../lib/vhost/virtio_net.c:1828:6,
>      inlined from ‘virtio_dev_rx_async_packed’ at
>        ../lib/vhost/virtio_net.c:1842:6,
>      inlined from ‘virtio_dev_rx_async_submit_packed’ at
>        ../lib/vhost/virtio_net.c:1900:7:
> ../lib/vhost/virtio_net.c:1159:18: error: ‘buf_vec[0].buf_addr’ may
>      be used uninitialized [-Werror=maybe-uninitialized]
>   1159 |         buf_addr = buf_vec[vec_idx].buf_addr;
>        |         ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
> <snip>
> ../lib/vhost/virtio_net.c:1160:18: error: ‘buf_vec[0].buf_iova’ may
>      be used uninitialized [-Werror=maybe-uninitialized]
>   1160 |         buf_iova = buf_vec[vec_idx].buf_iova;
>        |         ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
> <snip>
> ../lib/vhost/virtio_net.c:1161:35: error: ‘buf_vec[0].buf_len’ may
>      be used uninitialized [-Werror=maybe-uninitialized]
>   1161 |         buf_len = buf_vec[vec_idx].buf_len;
>        |                   ~~~~~~~~~~~~~~~~^~~~~~~~
> 
> GCC complains about the possible runtime path where the while loop
> which fills buf_vec (in vhost_enqueue_async_packed) is not run. As a
> consequence it correctly thinks that buf_vec is not initialized while
> being accessed anyways.
> 
> This scenario is actually very unlikely as the only way this can occur
> is if size has overflowed to 0. Meaning that the total packet length
> would be close to UINT64_MAX (or actually UINT32_MAX). At first glance,
> the code suggests that this may never happen as the type of size has
> been changed to 64-bit. For a 32-bit architecture such as arm
> (e.g. armv7-a) and aarch32, this still happens because the operand types
> (pkt->pkt_len and sizeof) are 32-bit wide, performing 32-bit arithmetic
> first (where the overflow can happen) and widening to 64-bit later.
> 
> The proposed fix simply guarantees to the compiler that the scope which
> fills buf_vec is accessed at least once, while not disrupting the actual
> logic. This is based on the assumption that size will always be greater
> than 0, as suggested by the sizeof, and the packet length will never be
> as big as UINT32_MAX, and causing an overflow.
> 
> Fixes: 873e8dad6f49 ("vhost: support packed ring in async datapath")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
> Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
> Reviewed-by: Nick Connolly <nick.connolly@arm.com>
> ---
>   lib/vhost/virtio_net.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime