Re: VirtioNet L3 protocol patch advice request.

On Fri, Oct 29, 2021 at 6:51 AM Andrew Melnichenko [off-list ref] wrote:

Hi all,
Recently I've discovered a patch that added an additional check for the
protocol in VirtioNet.
(https://www.spinics.net/lists/kernel/msg3866319.html)
Currently, that patch breaks UFOv6 support and possible USOv6 support in
upcoming patches.
The issue is the code next to the patch expects failure of
skb_flow_dissect_flow_keys_basic()
for IPv6 packets to retry it with protocol IPv6.
I'm not sure about the goals of the patch

A well behaved configuration should not enter that code path to begin
with. GSO packets should also request NEEDS_CSUM, and in normal cases
skb->protocol is set. But packet sockets allow leaving skb->protocol
0, in which case this code tries to infer the protocol from the link
layer header if present and supported, using
dev_parse_header_protocol.

Commit 924a9bc362a5 ("net: check if protocol extracted by
virtio_net_hdr_set_proto is correct") added the
dev_parse_header_protocol check and will drop packets where the GSO
type (e.g., VIRTIO_NET_HDR_GSO_TCPV4) does not match the network
protocol as stores in the link layer header (ETH_P_IPV6, or even
something unrelated like ETH_P_ARP).

You're right that it can drop UFOv6 packets. VIRTIO_NET_HDR_GSO_UDP
has no separate V4 and V6 variants, so we have to accept both
protocols. We need to fix that.

This guess in virtio_net_hdr_set_proto

        case VIRTIO_NET_HDR_GSO_UDP:
                skb->protocol = cpu_to_be16(ETH_P_IP);

might be wrong to assume IPv4 for UFOv6, and then as of that commit
this check will incorrectly drop the packet

                                virtio_net_hdr_set_proto(skb, hdr);
                                if (protocol && protocol != skb->protocol)
                                        return -EINVAL;

and propose the next solution:

static inline int virtio_net_hdr_set_proto(struct sk_buff *skb,

quoted

                     const struct virtio_net_hdr *hdr)
{
    __be16 protocol;

    protocol = dev_parse_header_protocol(skb);
    switch (hdr->gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
    case VIRTIO_NET_HDR_GSO_TCPV4:
        skb->protocol = cpu_to_be16(ETH_P_IP);
        break;
    case VIRTIO_NET_HDR_GSO_TCPV6:
        skb->protocol = cpu_to_be16(ETH_P_IPV6);
        break;
    case VIRTIO_NET_HDR_GSO_UDP:
    case VIRTIO_NET_HDR_GSO_UDP_L4:

Please use diff to show your changes. Also do not mix bug fixes (that
go to net) with new features (that go to net-next).

quoted

        skb->protocol = protocol;

Not exactly, this would just remove the added verification.

We need something like

@@ -89,8 +92,13 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
                                __be16 protocol =

dev_parse_header_protocol(skb);

                                virtio_net_hdr_set_proto(skb, hdr);
-                               if (protocol && protocol != skb->protocol)
-                                       return -EINVAL;
+                               if (protocol && protocol != skb->protocol) {
+                                       if (gso_type ==
VIRTIO_NET_HDR_GSO_UDP &&
+                                           protocol == cpu_to_be16(ETH_P_IPV6))
+                                               skb->protocol = protocol;
+                                       else
+                                               return -EINVAL;
+                               }

But preferably less ugly. Your suggestion of moving the
dev_parse_header_protocol step into virtio_net_hdr_to_skb is cleaner.
But also executes this check in the two other callers that may not
need it. Need to double check whether that is correct.