[PATCH v1] vhost: fix build

[PATCH v1] vhost: fix build

[Min Zhou]

This patch fixes the following build failure seen on CentOS 8
with gcc 12.1 because of uninitialized struct variable:

  [..]
../lib/vhost/virtio_net.c:1159:18: warning: 'buf_vec[0].buf_addr' may be used uninitialized [-Wmaybe-uninitialized]
1159 |         buf_addr = buf_vec[vec_idx].buf_addr;
     |         ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
  [..]

Fixes: 873e8dad6f49 ("vhost: support packed ring in async datapath")

Signed-off-by: Min Zhou <zhoumin@loongson.cn>
---
 lib/vhost/virtio_net.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
index 35fa4670fd..4878bb2d8a 100644
--- a/lib/vhost/virtio_net.c
+++ b/lib/vhost/virtio_net.c
@@ -1837,6 +1837,7 @@ virtio_dev_rx_async_packed(struct virtio_net *dev, struct vhost_virtqueue *vq,
 {
 	struct buf_vector buf_vec[BUF_VECTOR_MAX];
 
+	memset(buf_vec, 0, sizeof(buf_vec));
 	if (unlikely(vhost_enqueue_async_packed(dev, vq, pkt, buf_vec,
 					nr_descs, nr_buffers) < 0)) {
 		VHOST_LOG_DATA(dev->ifname, DEBUG, "failed to get enough desc from vring\n");
-- 
2.31.1

Re: [PATCH v1] vhost: fix build

[zhoumin]

Ping.


On Mon, Aug 22, 2022 at 15:42, Min Zhou wrote:
> This patch fixes the following build failure seen on CentOS 8
> with gcc 12.1 because of uninitialized struct variable:
>
>    [..]
> ../lib/vhost/virtio_net.c:1159:18: warning: 'buf_vec[0].buf_addr' may be used uninitialized [-Wmaybe-uninitialized]
> 1159 |         buf_addr = buf_vec[vec_idx].buf_addr;
>       |         ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
>    [..]
>
> Fixes: 873e8dad6f49 ("vhost: support packed ring in async datapath")
>
> Signed-off-by: Min Zhou <zhoumin@loongson.cn>
> ---
>   lib/vhost/virtio_net.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
> index 35fa4670fd..4878bb2d8a 100644
> --- a/lib/vhost/virtio_net.c
> +++ b/lib/vhost/virtio_net.c
> @@ -1837,6 +1837,7 @@ virtio_dev_rx_async_packed(struct virtio_net *dev, struct vhost_virtqueue *vq,
>   {
>   	struct buf_vector buf_vec[BUF_VECTOR_MAX];
>   
> +	memset(buf_vec, 0, sizeof(buf_vec));
>   	if (unlikely(vhost_enqueue_async_packed(dev, vq, pkt, buf_vec,
>   					nr_descs, nr_buffers) < 0)) {
>   		VHOST_LOG_DATA(dev->ifname, DEBUG, "failed to get enough desc from vring\n");

-- 
Thanks,
Min Zhou

Re: [PATCH v1] vhost: fix build

[David Marchand]

On Mon, Aug 22, 2022 at 9:42 AM Min Zhou <zhoumin@loongson.cn> wrote:
>
> This patch fixes the following build failure seen on CentOS 8
> with gcc 12.1 because of uninitialized struct variable:
>
>   [..]
> ../lib/vhost/virtio_net.c:1159:18: warning: 'buf_vec[0].buf_addr' may be used uninitialized [-Wmaybe-uninitialized]
> 1159 |         buf_addr = buf_vec[vec_idx].buf_addr;
>      |         ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
>   [..]

I don't like setting the whole variable to 0 just to silence a
warning, like pushing something under the rug.
This is all the more suspicious as there is other code in this file
that does almost the same.


I had seen a similar warning during 22.07 when cross compiling but did
not investigate much.
The patch that I had written at the time was:

diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
index 35fa4670fd..9446e33aa7 100644
--- a/lib/vhost/virtio_net.c
+++ b/lib/vhost/virtio_net.c
@@ -1153,7 +1153,7 @@ mbuf_to_desc(struct virtio_net *dev, struct
vhost_virtqueue *vq,
        struct virtio_net_hdr_mrg_rxbuf tmp_hdr, *hdr = NULL;
        struct vhost_async *async = vq->async;

-       if (unlikely(m == NULL))
+       if (unlikely(m == NULL || nr_vec == 0))
                return -1;

        buf_addr = buf_vec[vec_idx].buf_addr;


Could you see if this fixes your issue?

If it is the case, it may be worth better understanding what bothers
the compiler in the current code.


-- 
David Marchand

Re: [PATCH v1] vhost: fix build

[zhoumin]

Hi David,

Thanks for your kind reply.


On Wed, Aug 24, 2022 at 22:09, David Marchand wrote:
> On Mon, Aug 22, 2022 at 9:42 AM Min Zhou <zhoumin@loongson.cn> wrote:
>> This patch fixes the following build failure seen on CentOS 8
>> with gcc 12.1 because of uninitialized struct variable:
>>
>>    [..]
>> ../lib/vhost/virtio_net.c:1159:18: warning: 'buf_vec[0].buf_addr' may be used uninitialized [-Wmaybe-uninitialized]
>> 1159 |         buf_addr = buf_vec[vec_idx].buf_addr;
>>       |         ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
>>    [..]
> I don't like setting the whole variable to 0 just to silence a
> warning, like pushing something under the rug.
> This is all the more suspicious as there is other code in this file
> that does almost the same.
I'm sorry. I did see the same code at eight places in this file. They
are all similar to the following:

     struct buf_vector buf_vec[BUF_VECTOR_MAX];

     [...]
     fill_vec_buf_{split, packed}(.., buf_vec, ..)

     [...]
     mbuf_to_desc(.., buf_vec, ..) or desc_to_mbuf(.., buf_vec, ..)

At these eight places, the array variable `buf_vec` is not explicitly
initialized. Actually, all these `buf_vec` arraies are initialized by
sub-function calls under various conditions.

When I saw the GCC warning at line 1838, I decided to initialize the
`buf_vec` array by calling memset() at all eight places. But when I
just did that for line 1838, the GCC warning was disappeared. I'm not
familiar with vhost, so this made me puzzled and I tried to minimize
the amount of code modification.

I wondered if the uninitialized case at line 1838 is such evident to
GCC that it can be easily found and the other seven uninitialized cases
are hidden by more complicated sub-function call routines which made
GCC more cautious to warn. I am not sure either, but that's what I am
guessing.

Now I think I'm wrong. I sorted out the sub-function calls related to the
`buf_vec` array for all these eight places as follows. **Assignment**
means assigning values to member of the `buf_vec` array. **Using**
means using values from the `buf_vec` array.

1. virtio_dev_rx_split // line 1330
    |__ reserve_avail_buf_split
    |    |__ fill_vec_buf_split
    |        |__ map_one_desc // assign values to buf_vec[vec_id]
    |__ mbuf_to_desc // use values from buf_vec[vec_id]

2. virtio_dev_rx_single_packed // line 1498
    |__ vhost_enqueue_single_packed
         |__ fill_vec_buf_packed
         |    |__ fill_vec_buf_packed_indirect
         |    |    |__ map_one_desc // assignment
         |    |__ map_one_desc // assignment
         |__ mbuf_to_desc // using

3. virtio_dev_rx_async_submit_split // line 1670
    |__ reserve_avail_buf_split
    |    |__ fill_vec_buf_split
    |        |__ map_one_desc // assignment
    |__ mbuf_to_desc // using

4. virtio_dev_rx_async_packed // line 1834
    |__ vhost_enqueue_async_pakced
         |__ fill_vec_buf_packed
         |    |__ fill_vec_buf_packed_indirect
         |    |    |__ map_one_desc // assignment
         |    |__ map_one_desc // assignment
         |__ mbuf_to_desc // **using, but gcc emits warning**

5. virtio_dev_tx_split // line 2878
    |__ fill_vec_buf_split
    |    |__ map_one_desc // assignment
    |__ desc_to_mbuf // using

6. vhost_dequeue_single_packed // line 3105
    |__ fill_vec_buf_packed
    |    |__ fill_vec_buf_packed_indirect
    |    |    |__ map_one_desc // assignment
    |    |__ map_one_desc // assignment
    |__ desc_to_mbuf // using

7. virtio_dev_tx_async_split // line 3397
    |__ fill_vec_buf_split
    |    |__ map_one_desc // assignment
    |__ desc_to_mbuf // using

8. virtio_dev_tx_async_single_packed // line 3571
    |__ fill_vec_buf_packed
    |    |__ fill_vec_buf_packed_indirect
    |    |    |__ map_one_desc // assignment
    |    |__ map_one_desc // assignment
    |__ desc_to_mbuf // using

However, I cannot guess the warning rules of GCC from the above calling
relationships. These eight groups of calling relationships seem to follow
the same pattern and are not significantly different. It's difficult for
me to find the real reasons behind the GCC warning.
>
> I had seen a similar warning during 22.07 when cross compiling but did
> not investigate much.
> The patch that I had written at the time was:
>
> diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
> index 35fa4670fd..9446e33aa7 100644
> --- a/lib/vhost/virtio_net.c
> +++ b/lib/vhost/virtio_net.c
> @@ -1153,7 +1153,7 @@ mbuf_to_desc(struct virtio_net *dev, struct
> vhost_virtqueue *vq,
>          struct virtio_net_hdr_mrg_rxbuf tmp_hdr, *hdr = NULL;
>          struct vhost_async *async = vq->async;
>
> -       if (unlikely(m == NULL))
> +       if (unlikely(m == NULL || nr_vec == 0))
>                  return -1;
>
>          buf_addr = buf_vec[vec_idx].buf_addr;
>
>
> Could you see if this fixes your issue?
>
> If it is the case, it may be worth better understanding what bothers
> the compiler in the current code.
>
>
I have verified that the solution you proposed here is effective. It can
eliminate the GCC warning. But I don't know what this change means for
the compiler.

 From the programmer's point of view, we can know the binding relationship
between the `nr_vec` variable and the `buf_vec` array. we can use
"nr_vec == 0" to determine the validity of the `buf_vec[0]`. But, I'm not
sure if the compiler knows about it. I cannot explain from the GCC's point
of view why this modification can eliminate the warning.

However, in terms of correctness, this modification is very reasonable 
because
the `buf_vec[0]` would be invalid if the `nr_vec` variable equals to 
zero. In
this case, the function should return.

In addition, we can see that the `buf_vec` array are also used in function
desc_to_mbuf() from the above calling relationships. Do you think it is
necessary to add a similar check to this function? like this:

diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
index 9446e33aa7..99233f1759 100644
--- a/lib/vhost/virtio_net.c
+++ b/lib/vhost/virtio_net.c
@@ -2673,6 +2673,9 @@ desc_to_mbuf(struct virtio_net *dev, struct 
vhost_virtqueue *vq,
         struct vhost_async *async = vq->async;
         struct async_inflight_info *pkts_info;

+       if (unlikely(nr_vec == 0))
+               return -1;
+
         buf_addr = buf_vec[vec_idx].buf_addr;
         buf_iova = buf_vec[vec_idx].buf_iova;
         buf_len = buf_vec[vec_idx].buf_len;

I expect this compilation warning can be resolved. Because LoongArch
cross-compile tools must be based on GCC 12.1 and this compilation warning
will cause LoongArch CI job to fail.


Thinks,
Min Zhou

`

Re: [PATCH v1] vhost: fix build

[David Marchand]

Hello,

On Thu, Aug 25, 2022 at 2:37 PM zhoumin <zhoumin@loongson.cn> wrote:
> >
> > I had seen a similar warning during 22.07 when cross compiling but did
> > not investigate much.
> > The patch that I had written at the time was:
> >
> > diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
> > index 35fa4670fd..9446e33aa7 100644
> > --- a/lib/vhost/virtio_net.c
> > +++ b/lib/vhost/virtio_net.c
> > @@ -1153,7 +1153,7 @@ mbuf_to_desc(struct virtio_net *dev, struct
> > vhost_virtqueue *vq,
> >          struct virtio_net_hdr_mrg_rxbuf tmp_hdr, *hdr = NULL;
> >          struct vhost_async *async = vq->async;
> >
> > -       if (unlikely(m == NULL))
> > +       if (unlikely(m == NULL || nr_vec == 0))
> >                  return -1;
> >
> >          buf_addr = buf_vec[vec_idx].buf_addr;
> >
> >
> > Could you see if this fixes your issue?
> >
> > If it is the case, it may be worth better understanding what bothers
> > the compiler in the current code.
> >
> >
> I have verified that the solution you proposed here is effective. It can
> eliminate the GCC warning. But I don't know what this change means for
> the compiler.
>
>  From the programmer's point of view, we can know the binding relationship
> between the `nr_vec` variable and the `buf_vec` array. we can use
> "nr_vec == 0" to determine the validity of the `buf_vec[0]`. But, I'm not
> sure if the compiler knows about it. I cannot explain from the GCC's point
> of view why this modification can eliminate the warning.
>
> However, in terms of correctness, this modification is very reasonable
> because
> the `buf_vec[0]` would be invalid if the `nr_vec` variable equals to
> zero. In
> this case, the function should return.
>
> In addition, we can see that the `buf_vec` array are also used in function
> desc_to_mbuf() from the above calling relationships. Do you think it is
> necessary to add a similar check to this function? like this:

Maxime, Chenbo, can you have a look?
Thanks!

>
> diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
> index 9446e33aa7..99233f1759 100644
> --- a/lib/vhost/virtio_net.c
> +++ b/lib/vhost/virtio_net.c
> @@ -2673,6 +2673,9 @@ desc_to_mbuf(struct virtio_net *dev, struct
> vhost_virtqueue *vq,
>          struct vhost_async *async = vq->async;
>          struct async_inflight_info *pkts_info;
>
> +       if (unlikely(nr_vec == 0))
> +               return -1;
> +
>          buf_addr = buf_vec[vec_idx].buf_addr;
>          buf_iova = buf_vec[vec_idx].buf_iova;
>          buf_len = buf_vec[vec_idx].buf_len;
>
> I expect this compilation warning can be resolved. Because LoongArch
> cross-compile tools must be based on GCC 12.1 and this compilation warning
> will cause LoongArch CI job to fail.

Agreed.


-- 
David Marchand