Re: [PATCH] vhost/net: initialize the whole negotiated RX vnet header

[Guikun Yang <yangguikun19@qq.com>]

First, a correction to my previous message: the AF_PACKET injection in
that run never actually worked.  I took the "ifindex" from the ifreq
returned by TUNSETIFF, but TUNSETIFF fills in ifr_flags there (the
values 4098/20482 I printed were BROADCAST|MULTICAST|VNET_HDR flag
bits), so every sendto failed with ENODEV and no packet was ever
injected.  The all-0xAA buffer in that run only shows that no packet
was processed at all, and the "payload starts at offset 24" claim was
wrong.  Sorry for the noise.

I fixed the injection (SIOCGIFINDEX by name) and re-ran both
configurations on 7.3.0-rc7 with a 64-byte broadcast frame injected on
the tap.  With IFF_VNET_HDR enabled (plus TUNSETVNETHDRSZ(24)) the
result is worse than stale bytes: the packet is consumed from the tun
queue and then silently discarded, so the guest sees nothing at all:

  tunnel mode (no IFF_VNET_HDR), sendto 64 bytes ok:
	hdr: 00 00 00 00 00 00 00 00 00 00 01 00
	     aa aa aa aa aa aa aa aa aa aa aa aa   <- stale 12..23
	     ff ff ff ff ff ff 02 02 ...           <- our frame at 24

  tunnel-vnet-hdr mode (IFF_VNET_HDR | TUNSETVNETHDRSZ(24)),
  same 64-byte sendto ok:
	hdr: aa aa aa aa ... (all 32 bytes untouched)

The guest buffer is never touched in the second run.  The packet was
read out of the tun queue and then dropped.

The reason is a peek/read size disagreement:

  - peek_head_len() returns head->len, i.e. the raw skb->len.  The tun
    vnet header is not in the skb; tun_put_user() adds vnet_hdr_sz
    bytes at read time.
  - With VHOST_NET_F_VIRTIO_NET_HDR negotiated, sock_hlen = 0, so
    handle_rx() computes sock_len = peek = skb->len (64) and calls
    recvmsg(sock, &msg, 64, ...) with a msg_iter sized for
    vhost_len = 64 + vhost_hlen(24) = 88.
  - tun_do_read() delivers skb->len + vnet_hdr_sz = 64 + 24 = 88 bytes
    and returns 88.
  - handle_rx() then sees err (88) != sock_len (64) and takes the
    "Discarded rx packet" path: vhost_discard_vq_desc() rolls back the
    descriptor, but the skb has already been dequeued from the tun
    receive queue.  The frame is gone.

So for a tun backend, the three combinations are:

  VHOST_NET_F_VIRTIO_NET_HDR | IFF_VNET_HDR | result
  ---------------------------+--------------+--------------------------
  off                        | on           | ok (classic: tun header
                             |              | passed through)
  on                         | off          | header written by vhost,
                             |              | bytes 12..23 stale (the
                             |              | patch under discussion)
  on                         | on           | every packet consumed and
                             |              | dropped, guest RX blackout

VHOST_NET_F_VIRTIO_NET_HDR means "vhost supplies the header, the socket
data has none", so it is only consistent with a backend whose
vnet_hdr_sz is 0.  When the backend has vnet_hdr_sz > 0 the peek and
the read disagree and handle_rx() eats every frame.

Given that, I don't think disabling "vhost + !IFF_VNET_HDR" is the
right fix: that is exactly the combination VHOST_NET_F_VIRTIO_NET_HDR
is for, and it works apart from the 12 stale bytes my patch addresses.
The combination that is plainly broken is "vhost supplies the header"
together with a backend that supplies one (IFF_VNET_HDR set), which
today results in a silent total RX blackout.

So I'd suggest one of:

  1. Reject VHOST_NET_F_VIRTIO_NET_HDR (or refuse the backend attach)
     when the backend's vnet_hdr_sz is nonzero, so both broken
     combinations become impossible; keep my patch for the stale bytes
     in the remaining valid combination.  The backend header size could
     be queried at attach time the same way TUNSETIFF-derived sockets
     are handled today.

  2. Keep all combinations and fix both symptoms: my patch for the
     stale bytes, plus making the peek path account for the backend
     header so peek and read agree.

Happy to test whichever you prefer.

Best,
Guikun Yang