Received-SPF: pass (google.com: domain of linux-kernel+bounces-7765-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20231205113444.63015-1-linyunsheng@huawei.com>
 <20231205113444.63015-7-linyunsheng@huawei.com> <CACGkMEvVezZnHK-gRWY+MUd_6awnprb024scqPNmMQ05P8rWTQ@mail.gmail.com>
 <424670ab-23d8-663b-10cb-d88906767956@huawei.com> <CACGkMEsMdP1B-9RaqibJYfFsd_qJpB+Kta5BnyD_WXH=W2w_OQ@mail.gmail.com>
 <c5b5d36c-d0ca-c943-5355-343214d92c26@huawei.com> <CACGkMEs8HWq_NFNk=Pp3qxuo7AWBsybXT78LPgC-nKaP_u3LqA@mail.gmail.com>
 <8401fefd-d0da-efb9-78ab-cc4974a35801@huawei.com>
In-Reply-To: <8401fefd-d0da-efb9-78ab-cc4974a35801@huawei.com>
From: Jason Wang <jasowang@redhat.com>
Date: Thu, 21 Dec 2023 10:56:36 +0800
Message-ID: <CACGkMEvKOWpxpBR+YRuiJJ3aEsdxU2q+qVwmFw=L5gS3e7A35w@mail.gmail.com>
Subject: Re: [PATCH net-next 6/6] tools: virtio: introduce vhost_net_test
To: Yunsheng Lin <linyunsheng@huawei.com>
Cc: davem@davemloft.net, kuba@kernel.org, pabeni@redhat.com, 
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org, 
	"Michael S. Tsirkin" <mst@redhat.com>, Xuan Zhuo <xuanzhuo@linux.alibaba.com>, 
	virtualization@lists.linux.dev
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Dec 21, 2023 at 10:48=E2=80=AFAM Yunsheng Lin <linyunsheng@huawei.c=
om> wrote:
>
> On 2023/12/21 10:33, Jason Wang wrote:
> > On Wed, Dec 20, 2023 at 8:45=E2=80=AFPM Yunsheng Lin <linyunsheng@huawe=
i.com> wrote:
> >>
> >> On 2023/12/12 12:35, Jason Wang wrote:>>>> +done:
> >>>>>> +       backend.fd =3D tun_alloc();
> >>>>>> +       assert(backend.fd >=3D 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 enou=
gh
> >>>> with vhost_net and tun yet.
> >>>
> >>> Maybe you can have a packet socket to bind to the tun/tap. Then you c=
an 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 r=
x
> >> queue.
> >>
> >> For vhost_net_build_xdp() called by handle_tx_copy():
> >>
> >> The (gso->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) checking below may caus=
e a
> >> read of uninitialized data if sock_hlen is zero.
> >
> > Which data is uninitialized here?
>
> The 'gso', as the sock_hlen is zero, there is no copying for:
>
>          copied =3D copy_page_from_iter(alloc_frag->page,
>                                       alloc_frag->offset +
>                                       offsetof(struct tun_xdp_hdr, gso),
>                                       sock_hlen, from);

I think you're right. This is something we need to fix.

Or we can drop VHOST_NET_F_VIRTIO_NET_HDR as we managed to survive for year=
s:

https://patchwork.ozlabs.org/project/netdev/patch/1528429842-22835-1-git-se=
nd-email-jasowang@redhat.com/#1930760

>
> >
> >>
> >> And it seems vhost_hdr is skipped in get_tx_bufs():
> >> https://elixir.bootlin.com/linux/latest/source/drivers/vhost/net.c#L61=
6
> >>
> >> static int vhost_net_build_xdp(struct vhost_net_virtqueue *nvq,
> >>                                struct iov_iter *from)
> >> {
> >> ...
> >>         buflen +=3D SKB_DATA_ALIGN(len + pad);
> >>         alloc_frag->offset =3D ALIGN((u64)alloc_frag->offset, SMP_CACH=
E_BYTES);
> >>         if (unlikely(!vhost_net_page_frag_refill(net, buflen,
> >>                                                  alloc_frag, GFP_KERNE=
L)))
> >>                 return -ENOMEM;
> >>
> >>         buf =3D (char *)page_address(alloc_frag->page) + alloc_frag->o=
ffset;
> >>         copied =3D copy_page_from_iter(alloc_frag->page,
> >>                                      alloc_frag->offset +
> >>                                      offsetof(struct tun_xdp_hdr, gso)=
,
> >>                                      sock_hlen, from);
> >>         if (copied !=3D sock_hlen)
> >>                 return -EFAULT;
> >>
> >>         hdr =3D buf;
> >>         gso =3D &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_NE=
T_HDR
> >> case correctly, Or do I miss something obvious here?
> >
> > In get_tx_bufs() we did:
> >
> >         *len =3D init_iov_iter(vq, &msg->msg_iter, nvq->vhost_hlen, *ou=
t);
> >
> > Which covers this case?
>
> It does not seems to cover it, as the vhost_hdr is just skipped without a=
ny
> handling in get_tx_bufs():
> https://elixir.bootlin.com/linux/v6.7-rc6/source/drivers/vhost/net.c#L616

My understanding is that in this case vhost can't do more than this as
the socket doesn't know vnet_hdr.

Let's see if Michael is ok with this.

Thanks

>
> >
> > Thanks
>