| Message ID | 20230810123057.43407-5-xuanzhuo@linux.alibaba.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | virtio core prepares for AF_XDP | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
On Thu, 2023-08-10 at 20:30 +0800, Xuan Zhuo wrote: > If the vq is the premapped mode, use the sg_dma_address() directly. > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> > --- > drivers/virtio/virtio_ring.c | 19 +++++++++++++++++-- > 1 file changed, 17 insertions(+), 2 deletions(-) > > diff --git a/drivers/virtio/virtio_ring.c > b/drivers/virtio/virtio_ring.c > index 8e81b01e0735..f9f772e85a38 100644 > --- a/drivers/virtio/virtio_ring.c > +++ b/drivers/virtio/virtio_ring.c > @@ -361,6 +361,11 @@ static struct device *vring_dma_dev(const struct > vring_virtqueue *vq) > static int vring_map_one_sg(const struct vring_virtqueue *vq, struct > scatterlist *sg, > enum dma_data_direction direction, > dma_addr_t *addr) > { > + if (vq->premapped) { > + *addr = sg_dma_address(sg); > + return 0; > + } > + I wonder if something needs to be done for KMSAN here, like it's done by the next block in this function? I'm looking into what seems to be a KMSAN false positive on s390x: BUG: KMSAN: uninit-value in receive_buf+0x45ca/0x6990 receive_buf+0x45ca/0x6990 virtnet_poll+0x17e0/0x3130 net_rx_action+0x832/0x26e0 handle_softirqs+0x330/0x10f0 [...] Uninit was created at: __alloc_pages_noprof+0x62a/0xe60 alloc_pages_noprof+0x392/0x830 skb_page_frag_refill+0x21a/0x5c0 virtnet_rq_alloc+0x50/0x1500 try_fill_recv+0x372/0x54c0 virtnet_open+0x210/0xbe0 __dev_open+0x56e/0x920 __dev_change_flags+0x39c/0x2000 dev_change_flags+0xaa/0x200 do_setlink+0x197a/0x7420 rtnl_setlink+0x77c/0x860 [...] My understanding is that virtnet_rq_alloc() allocates a page for receiving data from a virtio device, which is then wrapped in struct scatterlist by virtnet_rq_init_one_sg(), which is in turn associated with a virtqueue through the virtqueue_add_inbuf_ctx() -> virtqueue_add() -> virtqueue_add_split() -> vring_map_one_sg() call chain. Someone should unpoison this page (since KMSAN doesn't know that the hypervisor writes to it), and today for the non-premapped case this is vring_map_one_sg(). So I tried the following naive fix: if (vq->premapped) { *addr = sg_dma_address(sg); + if (!vq->use_dma_api) { + kmsan_handle_dma(phys_to_page(*addr), sg- >offset, sg->length, direction); + } but it didn't help. I plan to investigate this further, but any hints are much appreciated. > if (!vq->use_dma_api) { > /* > * If DMA is not used, KMSAN doesn't know that the > scatterlist
On Tue, Jun 4, 2024 at 6:07 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote: > > On Thu, 2023-08-10 at 20:30 +0800, Xuan Zhuo wrote: > > If the vq is the premapped mode, use the sg_dma_address() directly. > > > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> > > --- > > drivers/virtio/virtio_ring.c | 19 +++++++++++++++++-- > > 1 file changed, 17 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/virtio/virtio_ring.c > > b/drivers/virtio/virtio_ring.c > > index 8e81b01e0735..f9f772e85a38 100644 > > --- a/drivers/virtio/virtio_ring.c > > +++ b/drivers/virtio/virtio_ring.c > > @@ -361,6 +361,11 @@ static struct device *vring_dma_dev(const struct > > vring_virtqueue *vq) > > static int vring_map_one_sg(const struct vring_virtqueue *vq, struct > > scatterlist *sg, > > enum dma_data_direction direction, > > dma_addr_t *addr) > > { > > + if (vq->premapped) { > > + *addr = sg_dma_address(sg); > > + return 0; > > + } > > + > > I wonder if something needs to be done for KMSAN here, like it's done > by the next block in this function? I'm looking into what seems to be a > KMSAN false positive on s390x: > > BUG: KMSAN: uninit-value in receive_buf+0x45ca/0x6990 > receive_buf+0x45ca/0x6990 > virtnet_poll+0x17e0/0x3130 > net_rx_action+0x832/0x26e0 > handle_softirqs+0x330/0x10f0 > [...] I think there's a similar problem on x86 as well: https://syzkaller.appspot.com/bug?extid=c5336dcd1b741349d27a I was going to look closer this week.
On Tue, 2024-06-04 at 18:17 +0200, Alexander Potapenko wrote: > On Tue, Jun 4, 2024 at 6:07 PM Ilya Leoshkevich <iii@linux.ibm.com> > wrote: > > > > On Thu, 2023-08-10 at 20:30 +0800, Xuan Zhuo wrote: > > > If the vq is the premapped mode, use the sg_dma_address() > > > directly. > > > > > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> > > > --- > > > drivers/virtio/virtio_ring.c | 19 +++++++++++++++++-- > > > 1 file changed, 17 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/virtio/virtio_ring.c > > > b/drivers/virtio/virtio_ring.c > > > index 8e81b01e0735..f9f772e85a38 100644 > > > --- a/drivers/virtio/virtio_ring.c > > > +++ b/drivers/virtio/virtio_ring.c > > > @@ -361,6 +361,11 @@ static struct device *vring_dma_dev(const > > > struct > > > vring_virtqueue *vq) > > > static int vring_map_one_sg(const struct vring_virtqueue *vq, > > > struct > > > scatterlist *sg, > > > enum dma_data_direction direction, > > > dma_addr_t *addr) > > > { > > > + if (vq->premapped) { > > > + *addr = sg_dma_address(sg); > > > + return 0; > > > + } > > > + > > > > I wonder if something needs to be done for KMSAN here, like it's > > done > > by the next block in this function? I'm looking into what seems to > > be a > > KMSAN false positive on s390x: > > > > BUG: KMSAN: uninit-value in receive_buf+0x45ca/0x6990 > > receive_buf+0x45ca/0x6990 > > virtnet_poll+0x17e0/0x3130 > > net_rx_action+0x832/0x26e0 > > handle_softirqs+0x330/0x10f0 > > [...] > > I think there's a similar problem on x86 as well: > https://syzkaller.appspot.com/bug?extid=c5336dcd1b741349d27a > > I was going to look closer this week. Thanks! I bisected it in the meantime and the first failing commit is: commit f9dac92ba9081062a6477ee015bd3b8c5914efc4 Author: Xuan Zhuo <xuanzhuo@linux.alibaba.com> Date: Sat May 11 11:14:01 2024 +0800 virtio_ring: enable premapped mode whatever use_dma_api so it's definitely related.
On Tue, 04 Jun 2024 18:07:44 +0200, Ilya Leoshkevich <iii@linux.ibm.com> wrote: > On Thu, 2023-08-10 at 20:30 +0800, Xuan Zhuo wrote: > > If the vq is the premapped mode, use the sg_dma_address() directly. > > > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> > > --- > > drivers/virtio/virtio_ring.c | 19 +++++++++++++++++-- > > 1 file changed, 17 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/virtio/virtio_ring.c > > b/drivers/virtio/virtio_ring.c > > index 8e81b01e0735..f9f772e85a38 100644 > > --- a/drivers/virtio/virtio_ring.c > > +++ b/drivers/virtio/virtio_ring.c > > @@ -361,6 +361,11 @@ static struct device *vring_dma_dev(const struct > > vring_virtqueue *vq) > > static int vring_map_one_sg(const struct vring_virtqueue *vq, struct > > scatterlist *sg, > > enum dma_data_direction direction, > > dma_addr_t *addr) > > { > > + if (vq->premapped) { > > + *addr = sg_dma_address(sg); > > + return 0; > > + } > > + > > I wonder if something needs to be done for KMSAN here, like it's done > by the next block in this function? I'm looking into what seems to be a > KMSAN false positive on s390x: > > BUG: KMSAN: uninit-value in receive_buf+0x45ca/0x6990 > receive_buf+0x45ca/0x6990 > virtnet_poll+0x17e0/0x3130 > net_rx_action+0x832/0x26e0 > handle_softirqs+0x330/0x10f0 > [...] > > Uninit was created at: > __alloc_pages_noprof+0x62a/0xe60 > alloc_pages_noprof+0x392/0x830 > skb_page_frag_refill+0x21a/0x5c0 > virtnet_rq_alloc+0x50/0x1500 > try_fill_recv+0x372/0x54c0 > virtnet_open+0x210/0xbe0 > __dev_open+0x56e/0x920 > __dev_change_flags+0x39c/0x2000 > dev_change_flags+0xaa/0x200 > do_setlink+0x197a/0x7420 > rtnl_setlink+0x77c/0x860 > [...] > > My understanding is that virtnet_rq_alloc() allocates a page for > receiving data from a virtio device, which is then wrapped in struct > scatterlist by virtnet_rq_init_one_sg(), which is in turn associated > with a virtqueue through the virtqueue_add_inbuf_ctx() -> > virtqueue_add() -> virtqueue_add_split() -> vring_map_one_sg() > call chain. > > Someone should unpoison this page (since KMSAN doesn't know that the > hypervisor writes to it), and today for the non-premapped case this is > vring_map_one_sg(). So I tried the following naive fix: > > if (vq->premapped) { > *addr = sg_dma_address(sg); > + if (!vq->use_dma_api) { > + kmsan_handle_dma(phys_to_page(*addr), sg- > >offset, sg->length, direction); > + } > > but it didn't help. I plan to investigate this further, but any hints > are much appreciated. > > > if (!vq->use_dma_api) { > > /* > > * If DMA is not used, KMSAN doesn't know that the > > scatterlist Could you try this? Thanks. diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index 37c9c5b55864..cb280b66c7a2 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -3119,8 +3119,10 @@ dma_addr_t virtqueue_dma_map_single_attrs(struct virtqueue *_vq, void *ptr, { struct vring_virtqueue *vq = to_vvq(_vq); - if (!vq->use_dma_api) + if (!vq->use_dma_api) { + kmsan_handle_dma(virt_to_page(ptr), offset_in_page(ptr), size, dir); return (dma_addr_t)virt_to_phys(ptr); + } return dma_map_single_attrs(vring_dma_dev(vq), ptr, size, dir, attrs); } @@ -3171,8 +3173,10 @@ dma_addr_t virtqueue_dma_map_page_attrs(struct virtqueue *_vq, struct page *page { struct vring_virtqueue *vq = to_vvq(_vq); - if (!vq->use_dma_api) + if (!vq->use_dma_api) { + kmsan_handle_dma(page, offset, size, dir); return page_to_phys(page) + offset; + } return dma_map_page_attrs(vring_dma_dev(vq), page, offset, size, dir, attrs); }
On Thu, 6 Jun 2024 10:20:21 +0200, Alexander Potapenko <glider@google.com> wrote: > > Could you try this? > > Hi Xuan, > > What kernel revision does this patch apply to? I tried it against > v6.10-rc2, and only the first hunk applied. > However this seems to fix the problem, at least the kernel boots without > warnings now. Sorry, I have some changes locally. If the hunk #1 is applied, then it is ok. Do you think we need more test? Or I post an new patch directly. Thanks. > > > Thanks. > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > index 37c9c5b55864..cb280b66c7a2 100644 > > --- a/drivers/virtio/virtio_ring.c > > +++ b/drivers/virtio/virtio_ring.c > > @@ -3119,8 +3119,10 @@ dma_addr_t virtqueue_dma_map_single_attrs(struct > virtqueue *_vq, void *ptr, > > { > > struct vring_virtqueue *vq = to_vvq(_vq); > > > > - if (!vq->use_dma_api) > > + if (!vq->use_dma_api) { > > + kmsan_handle_dma(virt_to_page(ptr), offset_in_page(ptr), > size, dir); > > return (dma_addr_t)virt_to_phys(ptr); > > + } > > > > return dma_map_single_attrs(vring_dma_dev(vq), ptr, size, dir, > attrs); > > } > > @@ -3171,8 +3173,10 @@ dma_addr_t virtqueue_dma_map_page_attrs(struct > virtqueue *_vq, struct page *page > > { > > struct vring_virtqueue *vq = to_vvq(_vq); > > > > - if (!vq->use_dma_api) > > + if (!vq->use_dma_api) { > > + kmsan_handle_dma(page, offset, size, dir); > > return page_to_phys(page) + offset; > > + } > > > > return dma_map_page_attrs(vring_dma_dev(vq), page, offset, size, > dir, attrs); > > } > > > > -- > 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 >
> > Could you try this? (resending without HTML, sorry for inconvenience). Hi Xuan, What kernel revision does this patch apply to? I tried it against v6.10-rc2, and only the first hunk applied. However this seems to fix the problem, at least the kernel boots without warnings now. > Thanks. > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > index 37c9c5b55864..cb280b66c7a2 100644 > --- a/drivers/virtio/virtio_ring.c > +++ b/drivers/virtio/virtio_ring.c > @@ -3119,8 +3119,10 @@ dma_addr_t virtqueue_dma_map_single_attrs(struct virtqueue *_vq, void *ptr, > { > struct vring_virtqueue *vq = to_vvq(_vq); > > - if (!vq->use_dma_api) > + if (!vq->use_dma_api) { > + kmsan_handle_dma(virt_to_page(ptr), offset_in_page(ptr), size, dir); > return (dma_addr_t)virt_to_phys(ptr); > + } > > return dma_map_single_attrs(vring_dma_dev(vq), ptr, size, dir, attrs); > } > @@ -3171,8 +3173,10 @@ dma_addr_t virtqueue_dma_map_page_attrs(struct virtqueue *_vq, struct page *page > { > struct vring_virtqueue *vq = to_vvq(_vq); > > - if (!vq->use_dma_api) > + if (!vq->use_dma_api) { > + kmsan_handle_dma(page, offset, size, dir); > return page_to_phys(page) + offset; > + } > > return dma_map_page_attrs(vring_dma_dev(vq), page, offset, size, dir, attrs); > }
On Thu, Jun 6, 2024 at 10:27 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote: > > On Thu, 6 Jun 2024 10:20:21 +0200, Alexander Potapenko <glider@google.com> wrote: > > > Could you try this? > > > > Hi Xuan, > > > > What kernel revision does this patch apply to? I tried it against > > v6.10-rc2, and only the first hunk applied. > > However this seems to fix the problem, at least the kernel boots without > > warnings now. > > > Sorry, I have some changes locally. > > If the hunk #1 is applied, then it is ok. > > Do you think we need more test? Or I post an new patch directly. I think we're good to go (just double-checked that). Assuming you're sending the following patch: diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index 2a972752ff1bc..9d3a9942c8c82 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -3121,8 +3121,10 @@ dma_addr_t virtqueue_dma_map_single_attrs(struct virtqueue *_vq, void *ptr, { struct vring_virtqueue *vq = to_vvq(_vq); - if (!vq->use_dma_api) + if (!vq->use_dma_api) { + kmsan_handle_dma(virt_to_page(ptr), offset_in_page(ptr), size, dir); return (dma_addr_t)virt_to_phys(ptr); + } return dma_map_single_attrs(vring_dma_dev(vq), ptr, size, dir, attrs); } , feel free to add: Tested-by: Alexander Potapenko <glider@google.com>
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index 8e81b01e0735..f9f772e85a38 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -361,6 +361,11 @@ static struct device *vring_dma_dev(const struct vring_virtqueue *vq) static int vring_map_one_sg(const struct vring_virtqueue *vq, struct scatterlist *sg, enum dma_data_direction direction, dma_addr_t *addr) { + if (vq->premapped) { + *addr = sg_dma_address(sg); + return 0; + } + if (!vq->use_dma_api) { /* * If DMA is not used, KMSAN doesn't know that the scatterlist @@ -639,8 +644,12 @@ static inline int virtqueue_add_split(struct virtqueue *_vq, dma_addr_t addr = vring_map_single( vq, desc, total_sg * sizeof(struct vring_desc), DMA_TO_DEVICE); - if (vring_mapping_error(vq, addr)) + if (vring_mapping_error(vq, addr)) { + if (vq->premapped) + goto free_indirect; + goto unmap_release; + } virtqueue_add_desc_split(_vq, vq->split.vring.desc, head, addr, @@ -706,6 +715,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq, i = vring_unmap_one_split(vq, i); } +free_indirect: if (indirect) kfree(desc); @@ -1307,8 +1317,12 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq, addr = vring_map_single(vq, desc, total_sg * sizeof(struct vring_packed_desc), DMA_TO_DEVICE); - if (vring_mapping_error(vq, addr)) + if (vring_mapping_error(vq, addr)) { + if (vq->premapped) + goto free_desc; + goto unmap_release; + } vq->packed.vring.desc[head].addr = cpu_to_le64(addr); vq->packed.vring.desc[head].len = cpu_to_le32(total_sg * @@ -1366,6 +1380,7 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq, for (i = 0; i < err_idx; i++) vring_unmap_desc_packed(vq, &desc[i]); +free_desc: kfree(desc); END_USE(vq);
If the vq is the premapped mode, use the sg_dma_address() directly. Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> --- drivers/virtio/virtio_ring.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)