[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CACGkMEs8HWq_NFNk=Pp3qxuo7AWBsybXT78LPgC-nKaP_u3LqA@mail.gmail.com>
Date: Thu, 21 Dec 2023 10:33:11 +0800
From: Jason Wang <jasowang@...hat.com>
To: Yunsheng Lin <linyunsheng@...wei.com>
Cc: davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
"Michael S. Tsirkin" <mst@...hat.com>, Xuan Zhuo <xuanzhuo@...ux.alibaba.com>,
virtualization@...ts.linux.dev
Subject: Re: [PATCH net-next 6/6] tools: virtio: introduce vhost_net_test
On Wed, Dec 20, 2023 at 8:45 PM Yunsheng Lin <linyunsheng@...wei.com> wrote:
>
> On 2023/12/12 12:35, Jason Wang wrote:>>>> +done:
> >>>> + backend.fd = tun_alloc();
> >>>> + assert(backend.fd >= 0);
> >>>> + vdev_info_init(&dev, features);
> >>>> + vq_info_add(&dev, 256);
> >>>> + run_test(&dev, &dev.vqs[0], delayed, batch, reset, nbufs);
> >>>
> >>> I'd expect we are testing some basic traffic here. E.g can we use a
> >>> packet socket then we can test both tx and rx?
> >>
> >> Yes, only rx for tun is tested.
> >> Do you have an idea how to test the tx too? As I am not familar enough
> >> with vhost_net and tun yet.
> >
> > Maybe you can have a packet socket to bind to the tun/tap. Then you can test:
> >
> > 1) TAP RX: by write a packet via virtqueue through vhost_net and read
> > it from packet socket
> > 2) TAP TX: by write via packet socket and read it from the virtqueue
> > through vhost_net
>
> When implementing the TAP TX by adding VHOST_NET_F_VIRTIO_NET_HDR,
> I found one possible use of uninitialized data in vhost_net_build_xdp().
>
> And vhost_hlen is set to sizeof(struct virtio_net_hdr_mrg_rxbuf) and
> sock_hlen is set to zero in vhost_net_set_features() for both tx and rx
> queue.
>
> For vhost_net_build_xdp() called by handle_tx_copy():
>
> The (gso->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) checking below may cause a
> read of uninitialized data if sock_hlen is zero.
Which data is uninitialized here?
>
> And it seems vhost_hdr is skipped in get_tx_bufs():
> https://elixir.bootlin.com/linux/latest/source/drivers/vhost/net.c#L616
>
> static int vhost_net_build_xdp(struct vhost_net_virtqueue *nvq,
> struct iov_iter *from)
> {
> ...
> buflen += SKB_DATA_ALIGN(len + pad);
> alloc_frag->offset = ALIGN((u64)alloc_frag->offset, SMP_CACHE_BYTES);
> if (unlikely(!vhost_net_page_frag_refill(net, buflen,
> alloc_frag, GFP_KERNEL)))
> return -ENOMEM;
>
> buf = (char *)page_address(alloc_frag->page) + alloc_frag->offset;
> copied = copy_page_from_iter(alloc_frag->page,
> alloc_frag->offset +
> offsetof(struct tun_xdp_hdr, gso),
> sock_hlen, from);
> if (copied != sock_hlen)
> return -EFAULT;
>
> hdr = buf;
> gso = &hdr->gso;
>
> if ((gso->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) &&
> vhost16_to_cpu(vq, gso->csum_start) +
> vhost16_to_cpu(vq, gso->csum_offset) + 2 >
> vhost16_to_cpu(vq, gso->hdr_len)) {
> ...
> }
>
> I seems the handle_tx_copy() does not handle the VHOST_NET_F_VIRTIO_NET_HDR
> case correctly, Or do I miss something obvious here?
In get_tx_bufs() we did:
*len = init_iov_iter(vq, &msg->msg_iter, nvq->vhost_hlen, *out);
Which covers this case?
Thanks
>
> >
> > Thanks
> >
> >>
> >>>
> >>> Thanks
> >>
> >
> > .
> >
>
Powered by blists - more mailing lists