diff mbox series

[v2] vhost: fix compilation issue in async path

Message ID 20221005151159.72366-1-maxime.coquelin@redhat.com (mailing list archive)
State Superseded, archived
Delegated to: Maxime Coquelin
Headers
Series [v2] vhost: fix compilation issue in async path |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/iol-mellanox-Performance success Performance Testing PASS
ci/github-robot: build success github build: passed
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-x86_64-unit-testing fail Testing issues
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS

Commit Message

Maxime Coquelin Oct. 5, 2022, 3:11 p.m. UTC 
  This patch fixes a compilation issue met with GCC 12 on
LoongArch64:

In function ‘mbuf_to_desc’,
    inlined from ‘vhost_enqueue_async_packed’ at ../../../dpdk/lib/vhost/virtio_net.c:1822:6,
    inlined from ‘virtio_dev_rx_async_packed’ at ../../../dpdk/lib/vhost/virtio_net.c:1836:6,
    inlined from ‘virtio_dev_rx_async_submit_packed’ at ../../../dpdk/lib/vhost/virtio_net.c:1895:7:
../../../dpdk/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;
      |         ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
../../../dpdk/lib/vhost/virtio_net.c: In function ‘virtio_dev_rx_async_submit_packed’:
../../../dpdk/lib/vhost/virtio_net.c:1834:27: note: ‘buf_vec’ declared here
 1834 |         struct buf_vector buf_vec[BUF_VECTOR_MAX];
      |                           ^~~~~~~

It happens because the compiler assumes that 'size'
variable in vhost_enqueue_async_packed could wrap to 0 since
'size' is uint32_t and pkt->pkt_len too.

In practice, it would never happen since 'pkt->pkt_len' is
unlikely to be close to UINT32_MAX, but let's just change
'size' to uint64_t to make the compiler happy without
having to add runtime checks.

Fixes: 873e8dad6f49 ("vhost: support packed ring in async datapath")
Cc: stable@dpdk.org

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/vhost/virtio_net.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Maxime Coquelin Oct. 5, 2022, 3:36 p.m. UTC | #1 
On 10/5/22 17:11, Maxime Coquelin wrote:
> This patch fixes a compilation issue met with GCC 12 on
> LoongArch64:
> 
> In function ‘mbuf_to_desc’,
>      inlined from ‘vhost_enqueue_async_packed’ at ../../../dpdk/lib/vhost/virtio_net.c:1822:6,
>      inlined from ‘virtio_dev_rx_async_packed’ at ../../../dpdk/lib/vhost/virtio_net.c:1836:6,
>      inlined from ‘virtio_dev_rx_async_submit_packed’ at ../../../dpdk/lib/vhost/virtio_net.c:1895:7:
> ../../../dpdk/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;
>        |         ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../../../dpdk/lib/vhost/virtio_net.c: In function ‘virtio_dev_rx_async_submit_packed’:
> ../../../dpdk/lib/vhost/virtio_net.c:1834:27: note: ‘buf_vec’ declared here
>   1834 |         struct buf_vector buf_vec[BUF_VECTOR_MAX];
>        |                           ^~~~~~~
> 
> It happens because the compiler assumes that 'size'
> variable in vhost_enqueue_async_packed could wrap to 0 since
> 'size' is uint32_t and pkt->pkt_len too.
> 
> In practice, it would never happen since 'pkt->pkt_len' is
> unlikely to be close to UINT32_MAX, but let's just change
> 'size' to uint64_t to make the compiler happy without
> having to add runtime checks.
> 
> Fixes: 873e8dad6f49 ("vhost: support packed ring in async datapath")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Forgot to report the R-by from David on v1:
Reviewed-by: David Marchand <david.marchand@redhat.com>

> ---
>   lib/vhost/virtio_net.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
> index 8f4d0f0502..b86fb26040 100644
> --- a/lib/vhost/virtio_net.c
> +++ b/lib/vhost/virtio_net.c
> @@ -1780,7 +1780,7 @@ vhost_enqueue_async_packed(struct virtio_net *dev,
>   	uint16_t buf_id = 0;
>   	uint32_t len = 0;
>   	uint16_t desc_count = 0;
> -	uint32_t size = pkt->pkt_len + sizeof(struct virtio_net_hdr_mrg_rxbuf);
> +	uint64_t size = pkt->pkt_len + sizeof(struct virtio_net_hdr_mrg_rxbuf);
>   	uint32_t buffer_len[vq->size];
>   	uint16_t buffer_buf_id[vq->size];
>   	uint16_t buffer_desc_count[vq->size];
David Marchand Oct. 5, 2022, 4:34 p.m. UTC | #2 
On Wed, Oct 5, 2022 at 5:36 PM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
> On 10/5/22 17:11, Maxime Coquelin wrote:
> > This patch fixes a compilation issue met with GCC 12 on
> > LoongArch64:
> >
> > In function ‘mbuf_to_desc’,
> >      inlined from ‘vhost_enqueue_async_packed’ at ../../../dpdk/lib/vhost/virtio_net.c:1822:6,
> >      inlined from ‘virtio_dev_rx_async_packed’ at ../../../dpdk/lib/vhost/virtio_net.c:1836:6,
> >      inlined from ‘virtio_dev_rx_async_submit_packed’ at ../../../dpdk/lib/vhost/virtio_net.c:1895:7:
> > ../../../dpdk/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;
> >        |         ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
> > ../../../dpdk/lib/vhost/virtio_net.c: In function ‘virtio_dev_rx_async_submit_packed’:
> > ../../../dpdk/lib/vhost/virtio_net.c:1834:27: note: ‘buf_vec’ declared here
> >   1834 |         struct buf_vector buf_vec[BUF_VECTOR_MAX];
> >        |                           ^~~~~~~
> >
> > It happens because the compiler assumes that 'size'
> > variable in vhost_enqueue_async_packed could wrap to 0 since
> > 'size' is uint32_t and pkt->pkt_len too.
> >
> > In practice, it would never happen since 'pkt->pkt_len' is
> > unlikely to be close to UINT32_MAX, but let's just change
> > 'size' to uint64_t to make the compiler happy without
> > having to add runtime checks.
> >
> > Fixes: 873e8dad6f49 ("vhost: support packed ring in async datapath")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>
> Forgot to report the R-by from David on v1:
> Reviewed-by: David Marchand <david.marchand@redhat.com>

After retesting with other arches, I have an error for cross compiling
ARM, on another part of the code.

C compiler for the host machine: ccache aarch64-linux-gnu-gcc (gcc
12.0.1 "aarch64-linux-gnu-gcc (GCC) 12.0.1 20220205 (experimental)
[master revision f49b8d25b1ff96e9cd09326666cc510b3a3accde]")

Same symptom:

[251/2797] Compiling C object lib/librte_vhost.a.p/vhost_virtio_net.c.o
FAILED: lib/librte_vhost.a.p/vhost_virtio_net.c.o
ccache aarch64-linux-gnu-gcc -Ilib/librte_vhost.a.p -Ilib
-I../../../git/pub/dpdk.org/main/lib -Ilib/vhost
-I../../../git/pub/dpdk.org/main/lib/vhost -I.
-I../../../git/pub/dpdk.org/main -Iconfig
-I../../../git/pub/dpdk.org/main/config -Ilib/eal/include
-I../../../git/pub/dpdk.org/main/lib/eal/include
-Ilib/eal/linux/include
-I../../../git/pub/dpdk.org/main/lib/eal/linux/include
-Ilib/eal/arm/include
-I../../../git/pub/dpdk.org/main/lib/eal/arm/include -Ilib/eal/common
-I../../../git/pub/dpdk.org/main/lib/eal/common -Ilib/eal
-I../../../git/pub/dpdk.org/main/lib/eal -Ilib/kvargs
-I../../../git/pub/dpdk.org/main/lib/kvargs -Ilib/metrics
-I../../../git/pub/dpdk.org/main/lib/metrics -Ilib/telemetry
-I../../../git/pub/dpdk.org/main/lib/telemetry -Ilib/ethdev
-I../../../git/pub/dpdk.org/main/lib/ethdev -Ilib/net
-I../../../git/pub/dpdk.org/main/lib/net -Ilib/mbuf
-I../../../git/pub/dpdk.org/main/lib/mbuf -Ilib/mempool
-I../../../git/pub/dpdk.org/main/lib/mempool -Ilib/ring
-I../../../git/pub/dpdk.org/main/lib/ring -Ilib/meter
-I../../../git/pub/dpdk.org/main/lib/meter -Ilib/cryptodev
-I../../../git/pub/dpdk.org/main/lib/cryptodev -Ilib/rcu
-I../../../git/pub/dpdk.org/main/lib/rcu -Ilib/hash
-I../../../git/pub/dpdk.org/main/lib/hash -Ilib/pci
-I../../../git/pub/dpdk.org/main/lib/pci -Ilib/dmadev
-I../../../git/pub/dpdk.org/main/lib/dmadev -fdiagnostics-color=always
-D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wextra -Werror -O2 -g
-include rte_config.h -Wcast-qual -Wdeprecated -Wformat
-Wformat-nonliteral -Wformat-security -Wmissing-declarations
-Wmissing-prototypes -Wnested-externs -Wold-style-definition
-Wpointer-arith -Wsign-compare -Wstrict-prototypes -Wundef
-Wwrite-strings -Wno-address-of-packed-member -Wno-packed-not-aligned
-Wno-missing-field-initializers -Wno-zero-length-bounds -D_GNU_SOURCE
-fPIC -march=armv8-a+crc -moutline-atomics -DALLOW_EXPERIMENTAL_API
-DALLOW_INTERNAL_API -Wno-format-truncation -DVHOST_GCC_UNROLL_PRAGMA
-fno-strict-aliasing -DRTE_LOG_DEFAULT_LOGTYPE=lib.vhost -MD -MQ
lib/librte_vhost.a.p/vhost_virtio_net.c.o -MF
lib/librte_vhost.a.p/vhost_virtio_net.c.o.d -o
lib/librte_vhost.a.p/vhost_virtio_net.c.o -c
../../../git/pub/dpdk.org/main/lib/vhost/virtio_net.c
In function ‘mbuf_to_desc’,
    inlined from ‘vhost_enqueue_single_packed’ at
../../../git/pub/dpdk.org/main/lib/vhost/virtio_net.c:1321:6,
    inlined from ‘virtio_dev_rx_single_packed’ at
../../../git/pub/dpdk.org/main/lib/vhost/virtio_net.c:1506:6,
    inlined from ‘virtio_dev_rx_packed’ at
../../../git/pub/dpdk.org/main/lib/vhost/virtio_net.c:1540:7:
../../../git/pub/dpdk.org/main/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;
      |         ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
../../../git/pub/dpdk.org/main/lib/vhost/virtio_net.c: In function
‘virtio_dev_rx_packed’:
../../../git/pub/dpdk.org/main/lib/vhost/virtio_net.c:1503:27: note:
‘buf_vec’ declared here
 1503 |         struct buf_vector buf_vec[BUF_VECTOR_MAX];
      |                           ^~~~~~~
In function ‘mbuf_to_desc’,
    inlined from ‘vhost_enqueue_single_packed’ at
../../../git/pub/dpdk.org/main/lib/vhost/virtio_net.c:1321:6,
    inlined from ‘virtio_dev_rx_single_packed’ at
../../../git/pub/dpdk.org/main/lib/vhost/virtio_net.c:1506:6,
    inlined from ‘virtio_dev_rx_packed’ at
../../../git/pub/dpdk.org/main/lib/vhost/virtio_net.c:1540:7:
../../../git/pub/dpdk.org/main/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;
      |         ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
../../../git/pub/dpdk.org/main/lib/vhost/virtio_net.c: In function
‘virtio_dev_rx_packed’:
../../../git/pub/dpdk.org/main/lib/vhost/virtio_net.c:1503:27: note:
‘buf_vec’ declared here
 1503 |         struct buf_vector buf_vec[BUF_VECTOR_MAX];
      |                           ^~~~~~~
In function ‘mbuf_to_desc’,
    inlined from ‘vhost_enqueue_single_packed’ at
../../../git/pub/dpdk.org/main/lib/vhost/virtio_net.c:1321:6,
    inlined from ‘virtio_dev_rx_single_packed’ at
../../../git/pub/dpdk.org/main/lib/vhost/virtio_net.c:1506:6,
    inlined from ‘virtio_dev_rx_packed’ at
../../../git/pub/dpdk.org/main/lib/vhost/virtio_net.c:1540:7:
../../../git/pub/dpdk.org/main/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;
      |                   ~~~~~~~~~~~~~~~~^~~~~~~~
../../../git/pub/dpdk.org/main/lib/vhost/virtio_net.c: In function
‘virtio_dev_rx_packed’:
../../../git/pub/dpdk.org/main/lib/vhost/virtio_net.c:1503:27: note:
‘buf_vec’ declared here
 1503 |         struct buf_vector buf_vec[BUF_VECTOR_MAX];
      |                           ^~~~~~~
cc1: all warnings being treated as errors


And same fix:

@@ -1277,7 +1277,7 @@ vhost_enqueue_single_packed(struct virtio_net *dev,
        uint16_t buf_id = 0;
        uint32_t len = 0;
        uint16_t desc_count;
-       uint32_t size = pkt->pkt_len + sizeof(struct virtio_net_hdr_mrg_rxbuf);
+       uint64_t size = pkt->pkt_len + sizeof(struct virtio_net_hdr_mrg_rxbuf);
        uint16_t num_buffers = 0;


        uint32_t buffer_len[vq->size];
        uint16_t buffer_buf_id[vq->size];


I suspect we may need some code inspection.
I see two other locations with the same pattern.
diff mbox series

Patch

diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
index 8f4d0f0502..b86fb26040 100644
--- a/lib/vhost/virtio_net.c
+++ b/lib/vhost/virtio_net.c
@@ -1780,7 +1780,7 @@  vhost_enqueue_async_packed(struct virtio_net *dev,
 	uint16_t buf_id = 0;
 	uint32_t len = 0;
 	uint16_t desc_count = 0;
-	uint32_t size = pkt->pkt_len + sizeof(struct virtio_net_hdr_mrg_rxbuf);
+	uint64_t size = pkt->pkt_len + sizeof(struct virtio_net_hdr_mrg_rxbuf);
 	uint32_t buffer_len[vq->size];
 	uint16_t buffer_buf_id[vq->size];
 	uint16_t buffer_desc_count[vq->size];