Re: [PATCH v3 1/5] net: add header len parameter to tun_get_socket(), tap_get_socket()

在 2021/6/25 下午4:23, David Woodhouse 写道:

On Fri, 2021-06-25 at 13:00 +0800, Jason Wang wrote:

quoted

在 2021/6/24 下午8:30, David Woodhouse 写道:

quoted

From: David Woodhouse <redacted>

The vhost-net driver was making wild assumptions about the header length
of the underlying tun/tap socket.

It's by design to depend on the userspace to co-ordinate the vnet header
setting with the underlying sockets.

quoted

   Then it was discarding packets if
the number of bytes it got from sock_recvmsg() didn't precisely match
its guess.

Anything that is broken by this? The failure is a hint for the userspace
that something is wrong during the coordination.

I am not a fan of this approach. I firmly believe that for a given
configuration, the kernel should either *work* or it should gracefully
refuse to set it up that way. And the requirements should be clearly
documented.

That works only if all the logic were implemented in the same module but 
not the case in the e.g networking stack that a packet need to iterate 
several modules.

E.g in this case, the vnet header size of the TAP could be changed at 
anytime via TUNSETVNETHDRSZ, and tuntap is unaware of the existence of 
vhost_net. This makes it impossible to do refuse in the case of setup 
(SET_BACKEND).

Having been on the receiving end of this "hint" of which you speak, I
found it distinctly suboptimal as a user interface. I was left
scrabbling around trying to find a set of options which *would* work,
and it was only through debugging the kernel that I managed to work out
that I:

   • MUST set IFF_NO_PI
   • MUST use TUNSETSNDBUF to reduce the sndbuf from INT_MAX
   • MUST use a virtio_net_hdr that I don't want

If my application failed to do any of those things, I got a silent
failure to transport any packets.

Yes, this is because the bug when using vhost_net + PI/TUN. And I guess 
the reason is that nobody tries to use that combination in the past.

I'm not even sure if it's a valid setup since vhost-net is a virtio-net 
kernel server which is not expected to handle L3 packets or PI header 
(which is Linux specific and out of the scope virtio spec).

  The only thing I could do *without*
debugging the kernel was tcpdump on the 'tun0' interface and see if the
TX packets I put into the ring were even making it to the interface,
and what they looked like if they did. (Losing the first 14 bytes and
having the *next* 14 bytes scribbled on by an Ethernet header was a fun
one.)

The tricky part is that, the networking stack thinks the packet is 
successfully received but it was actually dropped by vhost-net.

And there's no obvious userspace API to report such dropping as 
statistics counters or trace-points. Maybe we can tweak the vhost for a 
better logging in this case.

quoted

quoted

Fix it to get the correct information along with the socket itself.

I'm not sure what is fixed by this. It looks to me it tires to let
packet go even if the userspace set the wrong attributes to tap or
vhost. This is even sub-optimal than failing explicitly fail the RX.

I'm OK with explicit failure. But once I'd let it *get* the information
from the underlying socket in order to decide whether it should fail or
not, it turned out to be easy enough just to make those configs work
anyway.

The problem is that this change may make some wrong configuration 
"works" silently at the level of vhost or TAP. When using this for VM, 
it would make the debugging even harder.

The main case where that "easy enough" is stretched a little (IMO) was
when there's a tun_pi header. I have one more of your emails to reply
to after this, and I'll address that there.

quoted

quoted

As a side-effect, this means that tun_get_socket() won't work if the
tun file isn't actually connected to a device, since there's no 'tun'
yet in that case to get the information from.

This may break the existing application. Vhost-net is tied to the socket
instead of the device that the socket is loosely coupled.

Hm. Perhaps the PI and vnet hdr should be considered an option of the
*socket* (which is tied to the tfile), not purely an option of the
underlying device?

Though this is how it is done in macvtap. It's probably too late to 
change tuntap.

Or maybe it's sufficient just to get the flags from *either* tfile->tun
or tfile->detached, so that it works when the queue is detached. I'll
take a look.

I suppose we could even have a fallback that makes stuff up like we do
today. If the user attempts to attach a tun file descriptor to vhost
without ever calling TUNSETIFF on it first, *then* we make the same
assumptions we do today?

Then I would rather keep the using the assumption:

1) the value get from get_socket() might not be correct
2) the complexity or risk for bring a very little improvement of the 
debug-ability (which is still suspicious).

quoted

quoted

--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c

@@ -1143,7 +1143,8 @@ static void handle_rx(struct vhost_net *net)
   
   	vq_log = unlikely(vhost_has_feature(vq, VHOST_F_LOG_ALL)) ?
   		vq->log : NULL;
-	mergeable = vhost_has_feature(vq, VIRTIO_NET_F_MRG_RXBUF);
+	mergeable = vhost_has_feature(vq, VIRTIO_NET_F_MRG_RXBUF) &&
+		(vhost_hlen || sock_hlen >= sizeof(num_buffers));

So this change the behavior. When mergeable buffer is enabled, userspace
expects the vhost to merge buffers. If the feature is disabled silently,
it violates virtio spec.

If anything wrong in the setup, userspace just breaks itself.

E.g if sock_hlen is less that struct virtio_net_hdr_mrg_buf. The packet
header might be overwrote by the vnet header.

This wasn't intended to change the behaviour of any code path that is
already working today. If *either* vhost or the underlying device have
provided a vnet header, we still merge.

If *neither* provide a vnet hdr, there's nowhere to put num_buffers and
we can't merge.

That code path doesn't work at all today, but does after my patches.

It looks to me it's a bug that userspace can keep working in this case. 
After mrg rx buffer is negotiated, userspace should always assumes the 
vhost-net to provide num_buffers.

But you're right, we should explicitly refuse to negotiate
VIRITO_NET_F_MSG_RXBUF in that case.

This would be very hard:

1) VHOST_SET_FEATURES and VHOST_NET_SET_BACKEND are two different ioctls
2) vhost_net is not tightly coupled with tuntap, vnet header size could 
be changed by userspace at any time

quoted

quoted

   
   	do {
   		sock_len = vhost_net_rx_peek_head_len(net, sock->sk,

@@ -1213,9 +1214,10 @@ static void handle_rx(struct vhost_net *net)
   			}
   		} else {
   			/* Header came from socket; we'll need to patch
-			 * ->num_buffers over if VIRTIO_NET_F_MRG_RXBUF
+			 * ->num_buffers over the last two bytes if
+			 * VIRTIO_NET_F_MRG_RXBUF is enabled.
   			 */
-			iov_iter_advance(&fixup, sizeof(hdr));
+			iov_iter_advance(&fixup, sock_hlen - 2);

I'm not sure what did the above code want to fix. It doesn't change
anything if vnet header is set correctly in TUN. It only prevents the
the packet header from being rewrote.

It fixes the case where the virtio_net_hdr isn't at the start of the
tun header, because the tun actually puts the tun_pi struct *first*,
and *then* the virtio_net_hdr.

Right.

The num_buffers field needs to go at the *end* of sock_hlen. Not at a
fixed offset from the *start* of it.

At least, that's true unless we want to just declare that we *only*
support TUN with the IFF_NO_PI flag. (qv).

Yes, that's a good question. This is probably a hint that "vhost-net is 
never designed to work of PI", and even if it's not true, I'm not sure 
if it's too late to fix.

Thanks