| Message ID | 20240820073330.9161-4-xuanzhuo@linux.alibaba.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | virtio-net: support AF_XDP zero copy (tx) | expand |
Hi Xuan,
kernel test robot noticed the following build warnings:
url: https://github.com/intel-lab-lkp/linux/commits/Xuan-Zhuo/virtio_ring-introduce-vring_need_unmap_buffer/20240820-153644
base: net-next/main
patch link: https://lore.kernel.org/r/20240820073330.9161-4-xuanzhuo%40linux.alibaba.com
patch subject: [PATCH net-next 03/13] virtio_ring: packed: harden dma unmap for indirect
config: x86_64-randconfig-161-20240820 (https://download.01.org/0day-ci/archive/20240821/202408210655.dx8v5uRW-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202408210655.dx8v5uRW-lkp@intel.com/
New smatch warnings:
drivers/virtio/virtio_ring.c:1634 detach_buf_packed() error: uninitialized symbol 'desc'.
vim +/desc +1634 drivers/virtio/virtio_ring.c
1ce9e6055fa0a9 Tiwei Bie 2018-11-21 1594 static void detach_buf_packed(struct vring_virtqueue *vq,
1ce9e6055fa0a9 Tiwei Bie 2018-11-21 1595 unsigned int id, void **ctx)
1ce9e6055fa0a9 Tiwei Bie 2018-11-21 1596 {
1ce9e6055fa0a9 Tiwei Bie 2018-11-21 1597 struct vring_desc_state_packed *state = NULL;
1ce9e6055fa0a9 Tiwei Bie 2018-11-21 1598 struct vring_packed_desc *desc;
1ce9e6055fa0a9 Tiwei Bie 2018-11-21 1599 unsigned int i, curr;
1ce9e6055fa0a9 Tiwei Bie 2018-11-21 1600
1ce9e6055fa0a9 Tiwei Bie 2018-11-21 1601 state = &vq->packed.desc_state[id];
1ce9e6055fa0a9 Tiwei Bie 2018-11-21 1602
1ce9e6055fa0a9 Tiwei Bie 2018-11-21 1603 /* Clear data ptr. */
1ce9e6055fa0a9 Tiwei Bie 2018-11-21 1604 state->data = NULL;
1ce9e6055fa0a9 Tiwei Bie 2018-11-21 1605
aeef9b4733c5c2 Jason Wang 2021-06-04 1606 vq->packed.desc_extra[state->last].next = vq->free_head;
1ce9e6055fa0a9 Tiwei Bie 2018-11-21 1607 vq->free_head = id;
1ce9e6055fa0a9 Tiwei Bie 2018-11-21 1608 vq->vq.num_free += state->num;
1ce9e6055fa0a9 Tiwei Bie 2018-11-21 1609
d5c0ed17fea60c Xuan Zhuo 2024-02-23 1610 if (unlikely(vq->use_dma_api)) {
1ce9e6055fa0a9 Tiwei Bie 2018-11-21 1611 curr = id;
1ce9e6055fa0a9 Tiwei Bie 2018-11-21 1612 for (i = 0; i < state->num; i++) {
d80dc15bb6e76a Xuan Zhuo 2022-02-24 1613 vring_unmap_extra_packed(vq,
1ce9e6055fa0a9 Tiwei Bie 2018-11-21 1614 &vq->packed.desc_extra[curr]);
aeef9b4733c5c2 Jason Wang 2021-06-04 1615 curr = vq->packed.desc_extra[curr].next;
1ce9e6055fa0a9 Tiwei Bie 2018-11-21 1616 }
1ce9e6055fa0a9 Tiwei Bie 2018-11-21 1617 }
1ce9e6055fa0a9 Tiwei Bie 2018-11-21 1618
1ce9e6055fa0a9 Tiwei Bie 2018-11-21 1619 if (vq->indirect) {
dfcc54f92ab71c Xuan Zhuo 2024-08-20 1620 struct vring_desc_extra *extra;
1ce9e6055fa0a9 Tiwei Bie 2018-11-21 1621 u32 len;
1ce9e6055fa0a9 Tiwei Bie 2018-11-21 1622
1ce9e6055fa0a9 Tiwei Bie 2018-11-21 1623 /* Free the indirect table, if any, now that it's unmapped. */
dfcc54f92ab71c Xuan Zhuo 2024-08-20 1624 extra = state->indir;
dfcc54f92ab71c Xuan Zhuo 2024-08-20 1625 if (!extra)
1ce9e6055fa0a9 Tiwei Bie 2018-11-21 1626 return;
1ce9e6055fa0a9 Tiwei Bie 2018-11-21 1627
de6a29c4b4c442 Xuan Zhuo 2024-08-20 1628 if (vring_need_unmap_buffer(vq)) {
1ce9e6055fa0a9 Tiwei Bie 2018-11-21 1629 len = vq->packed.desc_extra[id].len;
1ce9e6055fa0a9 Tiwei Bie 2018-11-21 1630 for (i = 0; i < len / sizeof(struct vring_packed_desc);
1ce9e6055fa0a9 Tiwei Bie 2018-11-21 1631 i++)
dfcc54f92ab71c Xuan Zhuo 2024-08-20 1632 vring_unmap_extra_packed(vq, &extra[i]);
1ce9e6055fa0a9 Tiwei Bie 2018-11-21 1633 }
1ce9e6055fa0a9 Tiwei Bie 2018-11-21 @1634 kfree(desc);
^^^^
desc is never initialized/used.
dfcc54f92ab71c Xuan Zhuo 2024-08-20 1635 state->indir = NULL;
1ce9e6055fa0a9 Tiwei Bie 2018-11-21 1636 } else if (ctx) {
dfcc54f92ab71c Xuan Zhuo 2024-08-20 1637 *ctx = state->indir;
1ce9e6055fa0a9 Tiwei Bie 2018-11-21 1638 }
1ce9e6055fa0a9 Tiwei Bie 2018-11-21 1639 }
As gcc luckily noted: On Tue, Aug 20, 2024 at 03:33:20PM +0800, Xuan Zhuo wrote: > @@ -1617,23 +1617,24 @@ static void detach_buf_packed(struct vring_virtqueue *vq, > } > > if (vq->indirect) { > + struct vring_desc_extra *extra; > u32 len; > > /* Free the indirect table, if any, now that it's unmapped. */ > - desc = state->indir_desc; > - if (!desc) desc is no longer initialized here > + extra = state->indir; > + if (!extra) > return; > > if (vring_need_unmap_buffer(vq)) { > len = vq->packed.desc_extra[id].len; > for (i = 0; i < len / sizeof(struct vring_packed_desc); > i++) > - vring_unmap_desc_packed(vq, &desc[i]); > + vring_unmap_extra_packed(vq, &extra[i]); > } > kfree(desc); but freed here > - state->indir_desc = NULL; > + state->indir = NULL; > } else if (ctx) { > - *ctx = state->indir_desc; > + *ctx = state->indir; > } > } It seems unlikely this was always 0 on all paths with even a small amount of stress, so now I question how this was tested. Besides, do not ignore compiler warnings, and do not tweak code to just make compiler shut up - they are your friend. > > -- > 2.32.0.3.g01195cf9f
On Wed, 11 Sep 2024 07:28:36 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote: > As gcc luckily noted: > > On Tue, Aug 20, 2024 at 03:33:20PM +0800, Xuan Zhuo wrote: > > @@ -1617,23 +1617,24 @@ static void detach_buf_packed(struct vring_virtqueue *vq, > > } > > > > if (vq->indirect) { > > + struct vring_desc_extra *extra; > > u32 len; > > > > /* Free the indirect table, if any, now that it's unmapped. */ > > - desc = state->indir_desc; > > - if (!desc) > > desc is no longer initialized here Will fix. > > > + extra = state->indir; > > + if (!extra) > > return; > > > > if (vring_need_unmap_buffer(vq)) { > > len = vq->packed.desc_extra[id].len; > > for (i = 0; i < len / sizeof(struct vring_packed_desc); > > i++) > > - vring_unmap_desc_packed(vq, &desc[i]); > > + vring_unmap_extra_packed(vq, &extra[i]); > > } > > kfree(desc); > > > but freed here > > > - state->indir_desc = NULL; > > + state->indir = NULL; > > } else if (ctx) { > > - *ctx = state->indir_desc; > > + *ctx = state->indir; > > } > > } > > > It seems unlikely this was always 0 on all paths with even > a small amount of stress, so now I question how this was tested. > Besides, do not ignore compiler warnings, and do not tweak code > to just make compiler shut up - they are your friend. I agree. Normally I do this by make W=12, but we have too many message, so I missed this. make W=12 drivers/net/virtio_net.o drivers/virtio/virtio_ring.o If not W=12, then I did not get any warning message. How do you get the message quickly? Thanks. > > > > > -- > > 2.32.0.3.g01195cf9f >
On Thu, Sep 12, 2024 at 02:55:38PM +0800, Xuan Zhuo wrote: > On Wed, 11 Sep 2024 07:28:36 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote: > > As gcc luckily noted: > > > > On Tue, Aug 20, 2024 at 03:33:20PM +0800, Xuan Zhuo wrote: > > > @@ -1617,23 +1617,24 @@ static void detach_buf_packed(struct vring_virtqueue *vq, > > > } > > > > > > if (vq->indirect) { > > > + struct vring_desc_extra *extra; > > > u32 len; > > > > > > /* Free the indirect table, if any, now that it's unmapped. */ > > > - desc = state->indir_desc; > > > - if (!desc) > > > > desc is no longer initialized here > > > Will fix. > > > > > > > + extra = state->indir; > > > + if (!extra) > > > return; > > > > > > if (vring_need_unmap_buffer(vq)) { > > > len = vq->packed.desc_extra[id].len; > > > for (i = 0; i < len / sizeof(struct vring_packed_desc); > > > i++) > > > - vring_unmap_desc_packed(vq, &desc[i]); > > > + vring_unmap_extra_packed(vq, &extra[i]); > > > } > > > kfree(desc); > > > > > > but freed here > > > > > - state->indir_desc = NULL; > > > + state->indir = NULL; > > > } else if (ctx) { > > > - *ctx = state->indir_desc; > > > + *ctx = state->indir; > > > } > > > } > > > > > > It seems unlikely this was always 0 on all paths with even > > a small amount of stress, so now I question how this was tested. > > Besides, do not ignore compiler warnings, and do not tweak code > > to just make compiler shut up - they are your friend. > > I agree. > > Normally I do this by make W=12, but we have too many message, > so I missed this. > > make W=12 drivers/net/virtio_net.o drivers/virtio/virtio_ring.o > > If not W=12, then I did not get any warning message. > How do you get the message quickly? > > Thanks. If you stress test this for a long enough time, and with debug enabled, you will see a crash. > > > > > > > > -- > > > 2.32.0.3.g01195cf9f > >
On Thu, 12 Sep 2024 03:38:12 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Thu, Sep 12, 2024 at 02:55:38PM +0800, Xuan Zhuo wrote: > > On Wed, 11 Sep 2024 07:28:36 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > As gcc luckily noted: > > > > > > On Tue, Aug 20, 2024 at 03:33:20PM +0800, Xuan Zhuo wrote: > > > > @@ -1617,23 +1617,24 @@ static void detach_buf_packed(struct vring_virtqueue *vq, > > > > } > > > > > > > > if (vq->indirect) { > > > > + struct vring_desc_extra *extra; > > > > u32 len; > > > > > > > > /* Free the indirect table, if any, now that it's unmapped. */ > > > > - desc = state->indir_desc; > > > > - if (!desc) > > > > > > desc is no longer initialized here > > > > > > Will fix. > > > > > > > > > > > + extra = state->indir; > > > > + if (!extra) > > > > return; > > > > > > > > if (vring_need_unmap_buffer(vq)) { > > > > len = vq->packed.desc_extra[id].len; > > > > for (i = 0; i < len / sizeof(struct vring_packed_desc); > > > > i++) > > > > - vring_unmap_desc_packed(vq, &desc[i]); > > > > + vring_unmap_extra_packed(vq, &extra[i]); > > > > } > > > > kfree(desc); > > > > > > > > > but freed here > > > > > > > - state->indir_desc = NULL; > > > > + state->indir = NULL; > > > > } else if (ctx) { > > > > - *ctx = state->indir_desc; > > > > + *ctx = state->indir; > > > > } > > > > } > > > > > > > > > It seems unlikely this was always 0 on all paths with even > > > a small amount of stress, so now I question how this was tested. > > > Besides, do not ignore compiler warnings, and do not tweak code > > > to just make compiler shut up - they are your friend. > > > > I agree. > > > > Normally I do this by make W=12, but we have too many message, > > so I missed this. > > > > make W=12 drivers/net/virtio_net.o drivers/virtio/virtio_ring.o > > > > If not W=12, then I did not get any warning message. > > How do you get the message quickly? > > > > Thanks. > > > If you stress test this for a long enough time, and with > debug enabled, you will see a crash. I only stress tested the split ring. For the packed ring, I just performed a simple verification. I will street test for two mode in next version. Thanks. > > > > > > > > > > > > > -- > > > > 2.32.0.3.g01195cf9f > > > >
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index 582d2c05498a..b43eca93015c 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -81,7 +81,7 @@ struct vring_desc_state_split { struct vring_desc_state_packed { void *data; /* Data for callback. */ - struct vring_packed_desc *indir_desc; /* Indirect descriptor, if any. */ + struct vring_desc_extra *indir; /* Indirect descriptor, if any. */ u16 num; /* Descriptor list length. */ u16 last; /* The last desc state in a list. */ }; @@ -1238,27 +1238,13 @@ static void vring_unmap_extra_packed(const struct vring_virtqueue *vq, } } -static void vring_unmap_desc_packed(const struct vring_virtqueue *vq, - const struct vring_packed_desc *desc) -{ - u16 flags; - - if (!vring_need_unmap_buffer(vq)) - return; - - flags = le16_to_cpu(desc->flags); - - dma_unmap_page(vring_dma_dev(vq), - le64_to_cpu(desc->addr), - le32_to_cpu(desc->len), - (flags & VRING_DESC_F_WRITE) ? - DMA_FROM_DEVICE : DMA_TO_DEVICE); -} - static struct vring_packed_desc *alloc_indirect_packed(unsigned int total_sg, + struct vring_desc_extra **pextra, gfp_t gfp) { + struct vring_desc_extra *extra; struct vring_packed_desc *desc; + int i; /* * We require lowmem mappings for the descriptors because @@ -1267,7 +1253,14 @@ static struct vring_packed_desc *alloc_indirect_packed(unsigned int total_sg, */ gfp &= ~__GFP_HIGHMEM; - desc = kmalloc_array(total_sg, sizeof(struct vring_packed_desc), gfp); + extra = kmalloc_array(total_sg, sizeof(*desc) + sizeof(*extra), gfp); + + desc = (struct vring_packed_desc *)&extra[total_sg]; + + for (i = 0; i < total_sg; i++) + extra[i].next = i + 1; + + *pextra = extra; return desc; } @@ -1280,6 +1273,7 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq, void *data, gfp_t gfp) { + struct vring_desc_extra *extra; struct vring_packed_desc *desc; struct scatterlist *sg; unsigned int i, n, err_idx; @@ -1287,7 +1281,7 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq, dma_addr_t addr; head = vq->packed.next_avail_idx; - desc = alloc_indirect_packed(total_sg, gfp); + desc = alloc_indirect_packed(total_sg, &extra, gfp); if (!desc) return -ENOMEM; @@ -1313,6 +1307,12 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq, desc[i].addr = cpu_to_le64(addr); desc[i].len = cpu_to_le32(sg->length); i++; + + if (unlikely(vq->use_dma_api)) { + extra[i].addr = addr; + extra[i].len = sg->length; + extra[i].flags = n < out_sgs ? 0 : VRING_DESC_F_WRITE; + } } } @@ -1367,7 +1367,7 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq, /* Store token and indirect buffer state. */ vq->packed.desc_state[id].num = 1; vq->packed.desc_state[id].data = data; - vq->packed.desc_state[id].indir_desc = desc; + vq->packed.desc_state[id].indir = extra; vq->packed.desc_state[id].last = id; vq->num_added += 1; @@ -1381,7 +1381,7 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq, err_idx = i; for (i = 0; i < err_idx; i++) - vring_unmap_desc_packed(vq, &desc[i]); + vring_unmap_extra_packed(vq, &extra[i]); free_desc: kfree(desc); @@ -1504,7 +1504,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq, /* Store token. */ vq->packed.desc_state[id].num = descs_used; vq->packed.desc_state[id].data = data; - vq->packed.desc_state[id].indir_desc = ctx; + vq->packed.desc_state[id].indir = ctx; vq->packed.desc_state[id].last = prev; /* @@ -1617,23 +1617,24 @@ static void detach_buf_packed(struct vring_virtqueue *vq, } if (vq->indirect) { + struct vring_desc_extra *extra; u32 len; /* Free the indirect table, if any, now that it's unmapped. */ - desc = state->indir_desc; - if (!desc) + extra = state->indir; + if (!extra) return; if (vring_need_unmap_buffer(vq)) { len = vq->packed.desc_extra[id].len; for (i = 0; i < len / sizeof(struct vring_packed_desc); i++) - vring_unmap_desc_packed(vq, &desc[i]); + vring_unmap_extra_packed(vq, &extra[i]); } kfree(desc); - state->indir_desc = NULL; + state->indir = NULL; } else if (ctx) { - *ctx = state->indir_desc; + *ctx = state->indir; } }
1. this commit hardens dma unmap for indirect 2. the subsequent commit uses the struct extra to record whether the buffers need to be unmapped or not. So we need a struct extra for every desc, whatever it is indirect or not. Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> --- drivers/virtio/virtio_ring.c | 57 ++++++++++++++++++------------------ 1 file changed, 29 insertions(+), 28 deletions(-)