Re: VirtioNet L3 protocol patch advice request.

From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Date: 2021-10-31 20:50:10

On Fri, Oct 29, 2021 at 10:19 AM Willem de Bruijn
[off-list ref] wrote:

quoted hunk ↗ jump to hunk

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

quoted

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;

quoted

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.

If the protocol can be inferred from ll_type, that should take
precedence over inferring from gso_type.

For packet sockets, tpacket_snd does call dev_parse_header_protocol
before virtio_net_hdr_to_skb. But packet_snd calls it after.

Does the following solve your bug?

"

@@ -3001,6 +3001,8 @@ static int packet_snd(struct socket *sock,

struct msghdr *msg, size_t len)
        skb->mark = sockc.mark;
        skb->tstamp = sockc.transmit_time;

+       packet_parse_headers(skb, sock);
+
        if (has_vnet_hdr) {
                err = virtio_net_hdr_to_skb(skb, &vnet_hdr, vio_le());
                if (err)

@@ -3009,8 +3011,6 @@ static int packet_snd(struct socket *sock,

struct msghdr *msg, size_t len)
                virtio_net_hdr_set_proto(skb, &vnet_hdr);
        }

-       packet_parse_headers(skb, sock);
-
"

If the protocol is set, virtio_net_hdr_to_skb should not try to
overwrite it, but check that the values match:

+static inline bool virtio_net_hdr_check_gso_proto(struct sk_buff *skb,
+                                                 const struct
virtio_net_hdr *hdr)
+{
+       u8 gso_type = hdr->gso_type & ~VIRTIO_NET_HDR_GSO_ECN;
+
+       switch (skb->protocol) {
+       case htons(ETH_P_IP):
+               return (gso_type == VIRTIO_NET_HDR_GSO_TCPV4 ||
+                       gso_type == VIRTIO_NET_HDR_GSO_UDP);
+       case htons(ETH_P_IPV6):
+               return (gso_type == VIRTIO_NET_HDR_GSO_TCPV6 ||
+                       gso_type == VIRTIO_NET_HDR_GSO_UDP);
+       default:
+               return false;
+       }
+}

This can be called on any packet entering virtio_net_hdr_to_skb.

But such larger change should go to net-next.

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help