* VirtioNet L3 protocol patch advice request. @ 2021-10-29 10:51 Andrew Melnichenko 2021-10-29 14:19 ` Willem de Bruijn 0 siblings, 1 reply; 3+ messages in thread From: Andrew Melnichenko @ 2021-10-29 10:51 UTC (permalink / raw) To: davem, willemb, bnemeth, gregkh; +Cc: Yan Vugenfirer, Yuri Benditovich, netdev 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 and propose the next solution: static inline int virtio_net_hdr_set_proto(struct sk_buff *skb, > 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: > skb->protocol = protocol; > default: > return -EINVAL; > } > > return protocol && protocol == skb->protocol ? 0 : -EINVAL; > } > And in virtio_net_hdr_to_skb(): if (!skb->protocol) { > if(virtio_net_hdr_set_proto(skb, hdr)) > return -EINVAL; > } > > if (!skb_flow_dissect_flow_keys_basic(NULL, skb, &keys, > NULL, 0, 0, 0, > 0)) { > return -EINVAL; > } > Would such changes suit the goals of the initial patch? ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: VirtioNet L3 protocol patch advice request. 2021-10-29 10:51 VirtioNet L3 protocol patch advice request Andrew Melnichenko @ 2021-10-29 14:19 ` Willem de Bruijn 2021-10-31 20:49 ` Willem de Bruijn 0 siblings, 1 reply; 3+ messages in thread From: Willem de Bruijn @ 2021-10-29 14:19 UTC (permalink / raw) To: Andrew Melnichenko Cc: davem, bnemeth, gregkh, Yan Vugenfirer, Yuri Benditovich, netdev On Fri, Oct 29, 2021 at 6:51 AM Andrew Melnichenko <andrew@daynix.com> 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, > > 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). > > 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. ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: VirtioNet L3 protocol patch advice request. 2021-10-29 14:19 ` Willem de Bruijn @ 2021-10-31 20:49 ` Willem de Bruijn 0 siblings, 0 replies; 3+ messages in thread From: Willem de Bruijn @ 2021-10-31 20:49 UTC (permalink / raw) To: Willem de Bruijn Cc: Andrew Melnichenko, davem, bnemeth, gregkh, Yan Vugenfirer, Yuri Benditovich, netdev On Fri, Oct 29, 2021 at 10:19 AM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > On Fri, Oct 29, 2021 at 6:51 AM Andrew Melnichenko <andrew@daynix.com> 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, > > > 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). > > > > 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. ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-10-31 20:50 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-29 10:51 VirtioNet L3 protocol patch advice request Andrew Melnichenko 2021-10-29 14:19 ` Willem de Bruijn 2021-10-31 20:49 ` Willem de Bruijn
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).