On Wed, Dec 20, 2023 at 8:45 PM Yunsheng Lin <linyunsh...@huawei.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 > >> > > > > . > > >
