more virtio-net csum hardening
Subsystems (9)
Michael S. Tsirkin
maintainercover more virtio-net csum hardening
Allowing untrusted hardware to specify csum start/offset for CHECKSUM_PARTIAL was not a good idea, because, if it edits the packet outside the actual checksum, and depending on the setup, the networking stack could be looking at the packet both before and after the edit, and seeing different results.
Previously, commit 6785011f8b16 ("ipv4: validate checksum_start before completing checksum") validated all known instances where this causes the most grief, but the real fix is to make sure that either only actual checksum fields are ever edited,
or the checksum is completed before the net stack looks at it, so it is always consistent.
This basically does this: checksum at ingress, except in a narrow list of specified known good cases.
Michael S. Tsirkin (4):
net: add ip4_parse()
net: use ip4_parse() in receive validation
net: resolve uncommon partial checksums at ingress
net: account for ingress-resolved virtio net hdr state
drivers/net/tap.c | 4 +- drivers/net/tun.c | 44 +++++++- drivers/net/virtio_net.c | 2 +- include/linux/ip.h | 24 +++++ include/linux/virtio_net.h | 161 ++++++++++++++++++++++++---- net/bridge/br_netfilter_hooks.c | 15 +-- net/ipv4/ip_input.c | 13 +-- net/ipv4/netfilter/nf_reject_ipv4.c | 13 +-- net/packet/af_packet.c | 4 +- 9 files changed, 228 insertions(+), 52 deletions(-)
Signature · 1 line
MST
Michael S. Tsirkin
maintainerPATCH 1/4 net: add ip4_parse()
Multiple places in the net stack open-code the same minimal IPv4 header
validation: pull the fixed header, check version 4 and IHL >= 5 (where 5 is the fixed header size in 32-bit units) - and locate the transport header at IHL * 4 past the network header.
Add a helper for doing that: it returns the validated header and the transport header offset, or NULL if truncated or malformed.
--- include/linux/ip.h | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
include/linux/ip.h +24 -0
@@ -56,4 +56,28 @@ static inline void iph_set_totlen(struct iphdr *iph, unsigned int len) { iph->tot_len = len <=IP_MAX_MTU ?htons (len) :0 ; } + +/* +* Pull and minimally validate the IPv4 header at @nhoff. +* Returns the header and stores the transport header offset in +* @thoff, or NULL if truncated or malformed. Only the fixed header +* is guaranteed linear; callers that need the options or the +* transport header must pull further. +*/ +static inline struct iphdr *ip4_parse (struct sk_buff *skb, +unsigned int nhoff, +unsigned int *thoff) +{ +struct iphdr *iph; + +if (!pskb_may_pull (skb, nhoff +sizeof (struct iphdr ))) +return NULL ; + + iph = (struct iphdr *)(skb->data + nhoff); +if (iph->version !=4 || iph->ihl <sizeof (struct iphdr ) /sizeof (u32)) +return NULL ; + + *thoff = nhoff + iph->ihl *sizeof (u32); +return iph; +}#endif /* _LINUX_IP_H */
Signature · 1 line
MST
[email protected]
AI reviewRe: [PATCH net-next 1/4] net: add ip4_parse()
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 9 lines
Multiple places in the net stack open-code the same minimal IPv4 header
validation: pull the fixed header, check version 4 and IHL >= 5 (where 5 is the fixed header size in 32-bit units) - and locate the transport header at IHL * 4 past the network header.Add a helper for doing that: it returns the validated header and the transport header offset, or NULL if truncated or malformed.
Signed-off-by Michael S. Tsirkin <[email protected]>
Sashiko has reviewed this patch and found no issues. It looks great!
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
[email protected]
AI reviewRe: [PATCH net-next 1/4] net: add ip4_parse()
In reply to Michael S. TsirkinThank you for your contribution! Sashiko AI review found more severe issue(s) in other patches of this series. This mail is only an FYI, in case you want to address the nit picks below when you…
Thank you for your contribution! Sashiko AI review found more severe issue(s) in other patches of this series. This mail is only an FYI, in case you want to address the nit picks below when you respin.
Low priority / nit picks for your consideration:
- [Low] The doc comment on ip4_parse() (include/linux/ip.h) promises more than the code checks.
- [Low] API hazard: nhoff is unsigned int and nhoff + sizeof(struct iphdr) is not checked for overflow.
--
net: add ip4_parse()
This commit adds an inline helper, ip4_parse(), to include/linux/ip.h. It pulls the fixed IPv4 header at a given offset, checks for version 4 and IHL >= 5, and returns the header together with the transport header offset. It is meant to replace several open-coded copies of the same minimal validation in the net stack.
Quoted 6 lines
include/linux/ip.h +0
-0@@ -56,4 +56,28 @@ static inline void iph_set_totlen(struct iphdr *iph, unsigned int len) {
include/linux/ip.h static inline void iph_set_totlen(struct iphdr *iph, unsigned int len) · line 66
Quoted 10 lines
include/linux/ip.h +8
-0iph->tot_len = len <=IP_MAX_MTU ?htons (len) :0 ; } + +/* +* Pull and minimally validate the IPv4 header at @nhoff. +* Returns the header and stores the transport header offset in +* @thoff, or NULL if truncated or malformed. Only the fixed header +* is guaranteed linear; callers that need the options or the +* transport header must pull further. +*/
[Severity: Low]
Is "NULL if truncated or malformed" accurate here? ip4_parse() only does three checks: a pskb_may_pull() of the 20-byte fixed header, version == 4, and ihl >= 5.
Two kinds of bad header still get a non-NULL iph back:
- A header cut off inside its options, for example ihl = 15 with only 20 bytes in the skb.
- A header whose tot_len is smaller than ihl * 4.
In both cases thoff is stored without being checked against skb->len or ntohs(iph->tot_len):
*thoff = nhoff + iph->ihl * sizeof(u32);
The next sentence of the comment says the options may not be linear. It does not say that thoff can point past the end of the packet or the datagram.
The callers added later in this series do their own checks on thoff, so none of them overruns today. Those callers are ip_rcv_core(), br_validate_ipv4(), nf_reject_iphdr_validate() and virtio_net_csum_partial_ok_at_offset().
Could the comment say that only the fixed header is validated, and that callers must check thoff against what they can pull and against tot_len?
include/linux/ip.h static inline void iph_set_totlen(struct iphdr *iph, unsigned int len) · line 76
Quoted 10 lines
include/linux/ip.h +10
-0+static inline struct iphdr *ip4_parse (struct sk_buff *skb, +unsigned int nhoff, +unsigned int *thoff) +{ +struct iphdr *iph; + +if (!pskb_may_pull (skb, nhoff +sizeof (struct iphdr ))) +return NULL ; + + iph = (struct iphdr *)(skb->data + nhoff);
[Severity: Low]
This isn't a bug in any current caller, but nhoff is an unsigned int and the sum nhoff + sizeof(struct iphdr) is not checked for overflow.
Suppose a caller passed a negative offset, such as a negative skb_network_offset(), or a very large one. Could the length passed to pskb_may_pull() then wrap to a small value, while skb->data + nhoff still uses the original offset?
No caller in the series passes such an offset:
- ip_rcv_core(), br_validate_ipv4() and nf_reject_iphdr_validate()
pass 0.
- virtio_net_csum_partial_ok() passes ETH_HLEN, a VLAN depth, or a non-negative skb_network_offset().
The IPv6 branch next to the ip4_parse() call open-codes the same pskb_may_pull(skb, nhoff + sizeof(...)) pattern.
Would it be worth documenting that nhoff must be in range, or rejecting an nhoff that wraps?
[ ... ]
Signature · 1 line
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1791887415.git.mst%40redhat.com
Michael S. Tsirkin
maintainerPATCH 2/4 net: use ip4_parse() in receive validation
In reply to Michael S. TsirkinConvert the sites that pull and do version and ihl checks on the ipv4 header and validate the packet transport offset to use ip4_parse.
Sites that validate a read-only copy instead of pulling - the nft and nft_inner header_pointer validators, ip_check_defrag()'s skb_copy_bits, the flow dissector - stay open-coded for now.
--- net/bridge/br_netfilter_hooks.c | 15 +++++---------- net/ipv4/ip_input.c | 13 +++++-------- net/ipv4/netfilter/nf_reject_ipv4.c | 13 +++++-------- 3 files changed, 15 insertions(+), 26 deletions(-)
net/bridge/br_netfilter_hooks.c +5 -10
@@ -206,18 +206,13 @@ static inline void nf_bridge_pull_encap_header_rcsum(struct sk_buff *skb)static int br_validate_ipv4 (struct net *net,struct sk_buff *skb) {const struct iphdr *iph; -u32 len; +u32 len, thoff; -if (!pskb_may_pull (skb,sizeof (struct iphdr ))) + iph =ip4_parse (skb,0 , &thoff); +if (!iph) goto inhdr_error; - iph =ip_hdr (skb); - -/* Basic sanity checks */ -if (iph->ihl <5 || iph->version !=4 ) - goto inhdr_error; - -if (!pskb_may_pull (skb, iph->ihl*4 )) +if (!pskb_may_pull (skb, thoff)) goto inhdr_error; iph =ip_hdr (skb);
@@ -228,7 +223,7 @@ static int br_validate_ipv4(struct net *net, struct sk_buff *skb)if (skb->len < len) {__IP_INC_STATS (net,IPSTATS_MIB_INTRUNCATEDPKTS ); goto drop; - }else if (len < (iph->ihl*4 )) + }else if (len < thoff) goto inhdr_error;if (pskb_trim_rcsum (skb, len)) {
net/ipv4/ip_input.c +5 -8
@@ -500,7 +500,7 @@ static struct sk_buff *ip_rcv_core(struct sk_buff *skb, struct net *net) {const struct iphdr *iph;int drop_reason; -u32 len; +u32 len, thoff;/* When the interface is in promisc. mode, drop all the crap * that it receives, do not try to analyse it.
@@ -520,10 +520,6 @@ static struct sk_buff *ip_rcv_core(struct sk_buff *skb, struct net *net) } drop_reason =SKB_DROP_REASON_NOT_SPECIFIED ; -if (!pskb_may_pull (skb,sizeof (struct iphdr ))) - goto inhdr_error; - - iph =ip_hdr (skb);/* * RFC1122: 3.2.1.2 MUST silently discard any IP frame that fails the checksum.
@@ -536,7 +532,8 @@ static struct sk_buff *ip_rcv_core(struct sk_buff *skb, struct net *net)* 4. Doesn't have a bogus length */ -if (iph->ihl <5 || iph->version !=4 ) + iph =ip4_parse (skb,0 , &thoff); +if (!iph) goto inhdr_error;BUILD_BUG_ON (IPSTATS_MIB_ECT1PKTS !=IPSTATS_MIB_NOECTPKTS +INET_ECN_ECT_1 );
@@ -546,7 +543,7 @@ static struct sk_buff *ip_rcv_core(struct sk_buff *skb, struct net *net)IPSTATS_MIB_NOECTPKTS + (iph->tos &INET_ECN_MASK ), max_t(unsigned short, 1, skb_shinfo(skb)->gso_segs)); -if (!pskb_may_pull (skb, iph->ihl*4 )) +if (!pskb_may_pull (skb, thoff)) goto inhdr_error; iph =ip_hdr (skb);
@@ -559,7 +556,7 @@ static struct sk_buff *ip_rcv_core(struct sk_buff *skb, struct net *net) drop_reason =SKB_DROP_REASON_PKT_TOO_SMALL ;__IP_INC_STATS (net,IPSTATS_MIB_INTRUNCATEDPKTS ); goto drop; - }else if (len < (iph->ihl*4 )) + }else if (len < thoff) goto inhdr_error;/* Our transport medium may have padded the buffer out. Now we know it
net/ipv4/netfilter/nf_reject_ipv4.c +5 -8
@@ -25,22 +25,19 @@ nf_reject_ip_tcphdr_get(struct sk_buff *oldskb,static int nf_reject_iphdr_validate (struct sk_buff *skb) {struct iphdr *iph; -u32 len; +u32 len, thoff; -if (!pskb_may_pull (skb,sizeof (struct iphdr ))) -return 0 ; - - iph =ip_hdr (skb); -if (iph->ihl <5 || iph->version !=4 ) + iph =ip4_parse (skb,0 , &thoff); +if (!iph)return 0 ; len =ntohs (iph->tot_len);if (skb->len < len)return 0 ; -else if (len < (iph->ihl*4 )) +else if (len < thoff)return 0 ; -if (!pskb_may_pull (skb, iph->ihl*4 )) +if (!pskb_may_pull (skb, thoff))return 0 ;return 1 ;
Signature · 1 line
MST
[email protected]
AI reviewRe: [PATCH net-next 2/4] net: use ip4_parse() in receive validation
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 8 lines
Convert the sites that pull and do version and ihl checks on the ipv4 header and validate the packet transport offset to use
ip4_parse.Sites that validate a read-only copy instead of pulling - the nft and
nft_innerheader_pointervalidators, ip_check_defrag()'sskb_copy_bits, the flow dissector - stay open-coded for now.Signed-off-by Michael S. Tsirkin <[email protected]>
Sashiko has reviewed this patch and found no issues. It looks great!
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
[email protected]
AI reviewRe: [PATCH net-next 2/4] net: use ip4_parse() in receive validation
In reply to Michael S. TsirkinThank you for your contribution! Sashiko AI review found more severe issue(s) in other patches of this series. This mail is only an FYI, in case you want to address the nit picks below when you…
Thank you for your contribution! Sashiko AI review found more severe issue(s) in other patches of this series. This mail is only an FYI, in case you want to address the nit picks below when you respin.
Low priority / nit picks for your consideration:
- [Low] The commit message overstates how many sites were converted.
--
net: use ip4_parse() in receive validation
This patch converts br_validate_ipv4(), ip_rcv_core() and nf_reject_iphdr_validate() to use ip4_parse(). They previously did an open-coded pskb_may_pull() followed by version and ihl checks. The returned thoff is then used in the later length checks.
Quoted 6 lines
Convert the sites that pull and do version and ihl checks on the ipv4 header and validate the packet transport offset to use
ip4_parse.Sites that validate a read-only copy instead of pulling - the nft and
nft_innerheader_pointervalidators, ip_check_defrag()'sskb_copy_bits, the flow dissector - stay open-coded for now.
[Severity: Low]
This isn't a bug, but is this description complete? A few sites still call pskb_may_pull() and then open-code the version and ihl checks. The read-only copy exclusion list above does not cover them.
nft_set_pktinfo_ipv4_ingress() in include/net/netfilter/nf_tables_ipv4.h:
if (!pskb_may_pull(pkt->skb, sizeof(*iph))) return -1; iph = ip_hdr(pkt->skb); if (iph->ihl < 5 || iph->version != 4) goto inhdr_error;
This has the same pattern as br_validate_ipv4(). The header_pointer validator in that file is __nft_set_pktinfo_ipv4_validate(), not this function.
ip_mc_check_iphdr() in net/ipv4/igmp.c:
if (!pskb_may_pull(skb, offset)) return -EINVAL; iph = ip_hdr(skb); if (iph->version != 4 || ip_hdrlen(skb) < sizeof(*iph)) return -EINVAL; offset += ip_hdrlen(skb) - sizeof(*iph); if (!pskb_may_pull(skb, offset)) return -EINVAL;
This looks like it maps directly onto ip4_parse(skb, skb_network_offset(skb), &thoff).
nf_ct_bridge_pre() in net/bridge/netfilter/nf_conntrack_bridge.c calls pskb_may_pull(skb, sizeof(struct iphdr)). nf_ct_br_ip_check() then checks:
if (iph->ihl < 5 || iph->version != 4) return -1;
All three sites are still open-coded at the end of the series, after "net: account for ingress-resolved virtio net hdr state". At that point the only ip4_parse() callers are br_validate_ipv4(), ip_rcv_core(), nf_reject_iphdr_validate() and include/linux/virtio_net.h.
A smaller point: ip_rcv_core() still sets the transport header from ihl, even though thoff is now in scope:
skb->transport_header = skb->network_header + iph->ihl*4;
This has no functional effect.
Should these sites be converted, or should the commit message list them as staying open-coded?
Signature · 1 line
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1791887415.git.mst%40redhat.com
Michael S. Tsirkin
maintainerPATCH 3/4 net: resolve uncommon partial checksums at ingress
In reply to Michael S. TsirkinA virtio net header can place CHECKSUM_PARTIAL at any in-packet offset. If that state is retained, later checksum completion can modify the packet after protocol processing has made decisions from it.
Let's fix that: let's validate csum_start and csum_offset and only keep CHECKSUM_PARTIAL when csum_start and csum_offset match a parsed TCP or UDP header, while handling VLAN headers, IPv4 options and IPv6 extension headers. Complete other non-GSO checksums immediately, before any protocol processing runs.
We can't easily "complete the checksum" for GSO we don't recognize - GSO assumes knowledge of the protocol. We could segment ignoring the checksum then do whatever csum_start and csum_offset say, but that seems like an overkill for something that should happen.
UDP tunnel headers get the same handling on the inner header.
Packets without VIRTIO_NET_HDR_F_NEEDS_CSUM are unaffected.
--- include/linux/virtio_net.h | 151 ++++++++++++++++++++++++++++++++----- 1 file changed, 133 insertions(+), 18 deletions(-)
include/linux/virtio_net.h +133 -18
@@ -5,7 +5,11 @@#include <linux/if_vlan.h> #include <linux/ip.h> #include <linux/ipv6.h> +#include <linux/netdevice.h> #include <linux/udp.h> +#include <net/gso.h> +#include <net/ip.h> +#include <net/ipv6.h> #include <net/tcp.h> #include <uapi/linux/tcp.h> #include <uapi/linux/virtio_net.h>
@@ -48,15 +52,111 @@ static inline int virtio_net_hdr_set_proto(struct sk_buff *skb,return 0 ; } +/* Does checksum metadata match a known TCP or UDP header? */ +static inline bool +virtio_net_csum_partial_ok_at_offset (struct sk_buff *skb,u8 hdr_gso_type, +unsigned int nhoff,__be16 proto) +{ +unsigned int thoff; +int ip_proto; + +if (proto ==htons (ETH_P_IP )) { +const struct iphdr *iph =ip4_parse (skb, nhoff, &thoff); + +if (!iph ||ip_is_fragment (iph)) +return false; + ip_proto = iph->protocol; + }else if (proto ==htons (ETH_P_IPV6 )) { +const struct ipv6hdr *iph; +int flags =0 ; + +if (!pskb_may_pull (skb, nhoff +sizeof (*iph))) +return false; + iph = (const void *)(skb->data + nhoff); +if (iph->version !=6 ) +return false; + thoff = nhoff +sizeof (*iph); + ip_proto = iph->nexthdr; +if (ip_proto !=IPPROTO_TCP && ip_proto !=IPPROTO_UDP ) { + thoff = nhoff; + ip_proto =ipv6_find_hdr (skb, &thoff,-1 ,NULL , + &flags); +if (ip_proto <0 || flags &IP6_FH_F_FRAG ) +return false; + } + }else { +return false; + } + +if (skb_checksum_start_offset (skb) != thoff) +return false; + +switch (ip_proto) { +case IPPROTO_TCP : +if (skb->csum_offset != offsetof(struct tcphdr , check) || + !pskb_may_pull (skb, thoff +sizeof (struct tcphdr ))) +return false; +break ; +case IPPROTO_UDP : +if (skb->csum_offset != offsetof(struct udphdr , check) || + !pskb_may_pull (skb, thoff +sizeof (struct udphdr ))) +return false; +break ; +default : +return false; + } + +switch (hdr_gso_type & ~VIRTIO_NET_HDR_GSO_ECN ) { +case VIRTIO_NET_HDR_GSO_NONE : +return true; +case VIRTIO_NET_HDR_GSO_TCPV4 : +return proto ==htons (ETH_P_IP ) && ip_proto ==IPPROTO_TCP ; +case VIRTIO_NET_HDR_GSO_TCPV6 : +return proto ==htons (ETH_P_IPV6 ) && ip_proto ==IPPROTO_TCP ; +case VIRTIO_NET_HDR_GSO_UDP : +case VIRTIO_NET_HDR_GSO_UDP_L4 : +return ip_proto ==IPPROTO_UDP ; +default : +return false; + } +} + +static inline bool +virtio_net_csum_partial_ok (struct sk_buff *skb,u8 hdr_gso_type) +{ +unsigned int nhoff; +__be16 proto; + +if (skb->dev->type ==ARPHRD_ETHER ) { +if (!pskb_may_pull (skb,ETH_HLEN )) +return false; + nhoff =ETH_HLEN ; + proto =eth_hdr (skb)->h_proto; +if (eth_type_vlan (proto)) { + proto =__vlan_get_protocol (skb, proto, &nhoff); +if (!proto) +return false; + } + }else { + nhoff =skb_network_offset (skb); + proto = skb->protocol; + } + +return virtio_net_csum_partial_ok_at_offset (skb, hdr_gso_type, nhoff, + proto); +} +static inline int __virtio_net_hdr_to_skb (struct sk_buff *skb,const struct virtio_net_hdr *hdr, -bool little_endian,u8 hdr_gso_type) +bool little_endian,u8 hdr_gso_type, +bool need_csum_check) { -unsigned int nh_min_len =sizeof (struct iphdr ); + typeof(skb->transport_header) old_transport = skb->transport_header;unsigned int gso_type =0 ;unsigned int thlen =0 ;unsigned int p_off =0 ;unsigned int ip_proto; +bool csum_partial_ok = true;if (hdr_gso_type !=VIRTIO_NET_HDR_GSO_NONE ) {switch (hdr_gso_type & ~VIRTIO_NET_HDR_GSO_ECN ) {
@@ -69,7 +169,6 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb, gso_type =SKB_GSO_TCPV6 ; ip_proto =IPPROTO_TCP ; thlen =sizeof (struct tcphdr ); - nh_min_len =sizeof (struct ipv6hdr );break ;case VIRTIO_NET_HDR_GSO_UDP : gso_type =SKB_GSO_UDP ;
@@ -104,11 +203,11 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,if (!skb_partial_csum_set (skb, start, off))return -EINVAL ; -if (skb_transport_offset (skb) < nh_min_len) -return -EINVAL ; +if (need_csum_check) + csum_partial_ok = +virtio_net_csum_partial_ok (skb, hdr_gso_type); - nh_min_len =skb_transport_offset (skb); - p_off = nh_min_len + thlen; + p_off =skb_transport_offset (skb) + thlen;if (!pskb_may_pull (skb, p_off))return -EINVAL ; }else if (gso_type) {
@@ -158,8 +257,6 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,case SKB_GSO_UDP_L4 :if (!(hdr->flags &VIRTIO_NET_HDR_F_NEEDS_CSUM ))return -EINVAL ; -if (skb->csum_offset != offsetof(struct udphdr , check)) -return -EINVAL ;if (skb->len - p_off > gso_size *UDP_MAX_SEGMENTS )return -EINVAL ;if (gso_type !=SKB_GSO_UDP_L4 )
@@ -167,10 +264,6 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,break ;case SKB_GSO_TCPV4 :case SKB_GSO_TCPV6 : -if (skb->ip_summed ==CHECKSUM_PARTIAL && - skb->csum_offset != offsetof(struct tcphdr , check)) -return -EINVAL ; -BUILD_BUG_ON (TCP_MIN_GSO_SIZE *GSO_MAX_SEGS <GSO_MAX_SIZE ); gso_size =max (gso_size,TCP_MIN_GSO_SIZE );break ;
@@ -191,6 +284,17 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb, } } +if ((hdr->flags &VIRTIO_NET_HDR_F_NEEDS_CSUM ) && !csum_partial_ok) { +int err; + +if (skb_is_gso (skb)) +return -EINVAL ; + + err =skb_checksum_help (skb); + skb->transport_header = old_transport; +return err; + } +return 0 ; }
@@ -198,7 +302,8 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,const struct virtio_net_hdr *hdr, bool little_endian) { - return __virtio_net_hdr_to_skb(skb, hdr, little_endian, hdr->gso_type); + return __virtio_net_hdr_to_skb(skb, hdr, little_endian, + hdr->gso_type, true); }/* This function must be called after virtio_net_hdr_from_skb(). */
@@ -293,6 +398,7 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,unsigned int inner_nh, outer_th, inner_th;unsigned int inner_l3min, outer_l3min;u8 gso_inner_type, gso_tunnel_type; +__be16 inner_proto;bool outer_isv6, inner_isv6;int ret;
@@ -334,7 +440,7 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,/* Let the basic parsing deal with plain GSO features. */ ret =__virtio_net_hdr_to_skb (skb, hdr, true, - hdr->gso_type & ~gso_tunnel_type); + hdr->gso_type & ~gso_tunnel_type, false);if (ret)return ret;
@@ -352,8 +458,8 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,return -EINVAL ; } -skb_set_inner_protocol (skb, inner_isv6 ?htons (ETH_P_IPV6 ) : -htons (ETH_P_IP )); + inner_proto = inner_isv6 ?htons (ETH_P_IPV6 ) :htons (ETH_P_IP ); +skb_set_inner_protocol (skb, inner_proto);if (hdr->flags &VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM ) {if (!tnl_csum_negotiated)return -EINVAL ;
@@ -368,7 +474,16 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb, skb->inner_mac_header = inner_nh +skb_headroom (skb); skb->transport_header = outer_th +skb_headroom (skb); skb->encapsulation =1 ; -return 0 ; + +if (virtio_net_csum_partial_ok_at_offset (skb, gso_inner_type, inner_nh, + inner_proto)) +return 0 ; + +if (skb_is_gso (skb)) +return -EINVAL ; + + ret =skb_checksum_help (skb); +return ret ?:1 ; }/* Checksum-related fields validation for the driver */
Signature · 1 line
MST
Michael S. Tsirkin
maintainerRe: [PATCH net-next 3/4] net: resolve uncommon partial checksums at ingress
Quoted 15 lines · Michael S. Tsirkin
A virtio net header can place
CHECKSUM_PARTIALat any in-packet offset. If that state is retained, later checksum completion can modify the packet after protocol processing has made decisions from it.Let's fix that: let's validate
csum_startandcsum_offsetand only keepCHECKSUM_PARTIALwhencsum_startandcsum_offsetmatch a parsed TCP or UDP header, while handling VLAN headers, IPv4 options and IPv6 extension headers. Complete other non-GSO checksums immediately, before any protocol processing runs.We can't easily "complete the checksum" for GSO we don't recognize - GSO assumes knowledge of the protocol. We could segment ignoring the checksum then do whatever
csum_startandcsum_offsetsay, but that seems like an overkill for something that should happen.
Quoted 7 lines
UDP tunnel headers get the same handling on the inner header.
Packets without
VIRTIO_NET_HDR_F_NEEDS_CSUMare unaffected.Suggested-by Willem de Bruijn <[email protected]>Signed-off-by Michael S. Tsirkin <[email protected]>Assisted-by LLM
I forgot reported-by tags:
Quoted 255 lines
--- include/linux/virtio_net.h | 151 ++++++++++++++++++++++++++++++++----- 1 file changed, 133 insertions(+), 18 deletions(-)include/linux/virtio_net.h +133
-18@@ -5,7 +5,11 @@#include <linux/if_vlan.h> #include <linux/ip.h> #include <linux/ipv6.h> +#include <linux/netdevice.h> #include <linux/udp.h> +#include <net/gso.h> +#include <net/ip.h> +#include <net/ipv6.h> #include <net/tcp.h> #include <uapi/linux/tcp.h> #include <uapi/linux/virtio_net.h> @@ -48,15 +52,111 @@ static inline int virtio_net_hdr_set_proto(struct sk_buff *skb,return 0 ; } +/* Does checksum metadata match a known TCP or UDP header? */ +static inline bool +virtio_net_csum_partial_ok_at_offset (struct sk_buff *skb,u8 hdr_gso_type, +unsigned int nhoff,__be16 proto) +{ +unsigned int thoff; +int ip_proto; + +if (proto ==htons (ETH_P_IP )) { +const struct iphdr *iph =ip4_parse (skb, nhoff, &thoff); + +if (!iph ||ip_is_fragment (iph)) +return false; + ip_proto = iph->protocol; + }else if (proto ==htons (ETH_P_IPV6 )) { +const struct ipv6hdr *iph; +int flags =0 ; + +if (!pskb_may_pull (skb, nhoff +sizeof (*iph))) +return false; + iph = (const void *)(skb->data + nhoff); +if (iph->version !=6 ) +return false; + thoff = nhoff +sizeof (*iph); + ip_proto = iph->nexthdr; +if (ip_proto !=IPPROTO_TCP && ip_proto !=IPPROTO_UDP ) { + thoff = nhoff; + ip_proto =ipv6_find_hdr (skb, &thoff,-1 ,NULL , + &flags); +if (ip_proto <0 || flags &IP6_FH_F_FRAG ) +return false; + } + }else { +return false; + } + +if (skb_checksum_start_offset (skb) != thoff) +return false; + +switch (ip_proto) { +case IPPROTO_TCP : +if (skb->csum_offset != offsetof(struct tcphdr , check) || + !pskb_may_pull (skb, thoff +sizeof (struct tcphdr ))) +return false; +break ; +case IPPROTO_UDP : +if (skb->csum_offset != offsetof(struct udphdr , check) || + !pskb_may_pull (skb, thoff +sizeof (struct udphdr ))) +return false; +break ; +default : +return false; + } + +switch (hdr_gso_type & ~VIRTIO_NET_HDR_GSO_ECN ) { +case VIRTIO_NET_HDR_GSO_NONE : +return true; +case VIRTIO_NET_HDR_GSO_TCPV4 : +return proto ==htons (ETH_P_IP ) && ip_proto ==IPPROTO_TCP ; +case VIRTIO_NET_HDR_GSO_TCPV6 : +return proto ==htons (ETH_P_IPV6 ) && ip_proto ==IPPROTO_TCP ; +case VIRTIO_NET_HDR_GSO_UDP : +case VIRTIO_NET_HDR_GSO_UDP_L4 : +return ip_proto ==IPPROTO_UDP ; +default : +return false; + } +} + +static inline bool +virtio_net_csum_partial_ok (struct sk_buff *skb,u8 hdr_gso_type) +{ +unsigned int nhoff; +__be16 proto; + +if (skb->dev->type ==ARPHRD_ETHER ) { +if (!pskb_may_pull (skb,ETH_HLEN )) +return false; + nhoff =ETH_HLEN ; + proto =eth_hdr (skb)->h_proto; +if (eth_type_vlan (proto)) { + proto =__vlan_get_protocol (skb, proto, &nhoff); +if (!proto) +return false; + } + }else { + nhoff =skb_network_offset (skb); + proto = skb->protocol; + } + +return virtio_net_csum_partial_ok_at_offset (skb, hdr_gso_type, nhoff, + proto); +} +static inline int __virtio_net_hdr_to_skb (struct sk_buff *skb,const struct virtio_net_hdr *hdr, -bool little_endian,u8 hdr_gso_type) +bool little_endian,u8 hdr_gso_type, +bool need_csum_check) { -unsigned int nh_min_len =sizeof (struct iphdr ); + typeof(skb->transport_header) old_transport = skb->transport_header;unsigned int gso_type =0 ;unsigned int thlen =0 ;unsigned int p_off =0 ;unsigned int ip_proto; +bool csum_partial_ok = true;if (hdr_gso_type !=VIRTIO_NET_HDR_GSO_NONE ) {switch (hdr_gso_type & ~VIRTIO_NET_HDR_GSO_ECN ) {@@ -69,7 +169,6 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb, gso_type =SKB_GSO_TCPV6 ; ip_proto =IPPROTO_TCP ; thlen =sizeof (struct tcphdr ); - nh_min_len =sizeof (struct ipv6hdr );break ;case VIRTIO_NET_HDR_GSO_UDP : gso_type =SKB_GSO_UDP ;@@ -104,11 +203,11 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,if (!skb_partial_csum_set (skb, start, off))return -EINVAL ; -if (skb_transport_offset (skb) < nh_min_len) -return -EINVAL ; +if (need_csum_check) + csum_partial_ok = +virtio_net_csum_partial_ok (skb, hdr_gso_type); - nh_min_len =skb_transport_offset (skb); - p_off = nh_min_len + thlen; + p_off =skb_transport_offset (skb) + thlen;if (!pskb_may_pull (skb, p_off))return -EINVAL ; }else if (gso_type) {@@ -158,8 +257,6 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,case SKB_GSO_UDP_L4 :if (!(hdr->flags &VIRTIO_NET_HDR_F_NEEDS_CSUM ))return -EINVAL ; -if (skb->csum_offset != offsetof(struct udphdr , check)) -return -EINVAL ;if (skb->len - p_off > gso_size *UDP_MAX_SEGMENTS )return -EINVAL ;if (gso_type !=SKB_GSO_UDP_L4 )@@ -167,10 +264,6 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,break ;case SKB_GSO_TCPV4 :case SKB_GSO_TCPV6 : -if (skb->ip_summed ==CHECKSUM_PARTIAL && - skb->csum_offset != offsetof(struct tcphdr , check)) -return -EINVAL ; -BUILD_BUG_ON (TCP_MIN_GSO_SIZE *GSO_MAX_SEGS <GSO_MAX_SIZE ); gso_size =max (gso_size,TCP_MIN_GSO_SIZE );break ;@@ -191,6 +284,17 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb, } } +if ((hdr->flags &VIRTIO_NET_HDR_F_NEEDS_CSUM ) && !csum_partial_ok) { +int err; + +if (skb_is_gso (skb)) +return -EINVAL ; + + err =skb_checksum_help (skb); + skb->transport_header = old_transport; +return err; + } +return 0 ; }@@ -198,7 +302,8 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,const struct virtio_net_hdr *hdr, bool little_endian) { - return __virtio_net_hdr_to_skb(skb, hdr, little_endian, hdr->gso_type); + return __virtio_net_hdr_to_skb(skb, hdr, little_endian, + hdr->gso_type, true); }/* This function must be called after virtio_net_hdr_from_skb(). */ @@ -293,6 +398,7 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,unsigned int inner_nh, outer_th, inner_th;unsigned int inner_l3min, outer_l3min;u8 gso_inner_type, gso_tunnel_type; +__be16 inner_proto;bool outer_isv6, inner_isv6;int ret;@@ -334,7 +440,7 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,/* Let the basic parsing deal with plain GSO features. */ ret =__virtio_net_hdr_to_skb (skb, hdr, true, - hdr->gso_type & ~gso_tunnel_type); + hdr->gso_type & ~gso_tunnel_type, false);if (ret)return ret;@@ -352,8 +458,8 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,return -EINVAL ; } -skb_set_inner_protocol (skb, inner_isv6 ?htons (ETH_P_IPV6 ) : -htons (ETH_P_IP )); + inner_proto = inner_isv6 ?htons (ETH_P_IPV6 ) :htons (ETH_P_IP ); +skb_set_inner_protocol (skb, inner_proto);if (hdr->flags &VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM ) {if (!tnl_csum_negotiated)return -EINVAL ;@@ -368,7 +474,16 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb, skb->inner_mac_header = inner_nh +skb_headroom (skb); skb->transport_header = outer_th +skb_headroom (skb); skb->encapsulation =1 ; -return 0 ; + +if (virtio_net_csum_partial_ok_at_offset (skb, gso_inner_type, inner_nh, + inner_proto)) +return 0 ; + +if (skb_is_gso (skb)) +return -EINVAL ; + + ret =skb_checksum_help (skb); +return ret ?:1 ; }/* Checksum-related fields validation for the driver */ Signature · 1 line
MST
Willem de Bruijn
maintainerRe: [PATCH net-next 3/4] net: resolve uncommon partial checksums at ingress
Quoted 77 lines · Michael S. Tsirkin
Quoted 15 lines · Michael S. Tsirkin
A virtio net header can place
CHECKSUM_PARTIALat any in-packet offset. If that state is retained, later checksum completion can modify the packet after protocol processing has made decisions from it.Let's fix that: let's validate
csum_startandcsum_offsetand only keepCHECKSUM_PARTIALwhencsum_startandcsum_offsetmatch a parsed TCP or UDP header, while handling VLAN headers, IPv4 options and IPv6 extension headers. Complete other non-GSO checksums immediately, before any protocol processing runs.We can't easily "complete the checksum" for GSO we don't recognize - GSO assumes knowledge of the protocol. We could segment ignoring the checksum then do whatever
csum_startandcsum_offsetsay, but that seems like an overkill for something that should happen.Quoted 7 lines
UDP tunnel headers get the same handling on the inner header.
Packets without
VIRTIO_NET_HDR_F_NEEDS_CSUMare unaffected.Suggested-by Willem de Bruijn <[email protected]>Signed-off-by Michael S. Tsirkin <[email protected]>Assisted-by LLMI forgot reported-by tags:
Reported-by Paulos Yibelo <[email protected]>Reported-by Chris Arges <[email protected]>Quoted 45 lines
--- include/linux/virtio_net.h | 151 ++++++++++++++++++++++++++++++++----- 1 file changed, 133 insertions(+), 18 deletions(-)include/linux/virtio_net.h +25
-0@@ -5,7 +5,11 @@#include <linux/if_vlan.h> #include <linux/ip.h> #include <linux/ipv6.h> +#include <linux/netdevice.h> #include <linux/udp.h> +#include <net/gso.h> +#include <net/ip.h> +#include <net/ipv6.h> #include <net/tcp.h> #include <uapi/linux/tcp.h> #include <uapi/linux/virtio_net.h> @@ -48,15 +52,111 @@ static inline int virtio_net_hdr_set_proto(struct sk_buff *skb,return 0 ; } +/* Does checksum metadata match a known TCP or UDP header? */ +static inline bool +virtio_net_csum_partial_ok_at_offset (struct sk_buff *skb,u8 hdr_gso_type, +unsigned int nhoff,__be16 proto) +{ +unsigned int thoff; +int ip_proto; + +if (proto ==htons (ETH_P_IP )) { +const struct iphdr *iph =ip4_parse (skb, nhoff, &thoff); + +if (!iph ||ip_is_fragment (iph)) +return false; + ip_proto = iph->protocol; + }else if (proto ==htons (ETH_P_IPV6 )) { +const struct ipv6hdr *iph; +int flags =0 ; + +if (!pskb_may_pull (skb, nhoff +sizeof (*iph))) +return false; + iph = (const void *)(skb->data + nhoff);
Quoted 8 lines
include/linux/virtio_net.h static inline int virtio_net_hdr_set_proto(struct sk_buff *skb, · line 83
Quoted 8 lines
include/linux/virtio_net.h +8
-0+if (iph->version !=6 ) +return false; + thoff = nhoff +sizeof (*iph); + ip_proto = iph->nexthdr; +if (ip_proto !=IPPROTO_TCP && ip_proto !=IPPROTO_UDP ) { + thoff = nhoff; + ip_proto =ipv6_find_hdr (skb, &thoff,-1 ,NULL , + &flags);
Skip this rare case of extension headers?
Quoted 72 lines
Quoted 72 lines
include/linux/virtio_net.h +15
-13+if (ip_proto <0 || flags &IP6_FH_F_FRAG ) +return false; + } + }else { +return false; + } +static inline int __virtio_net_hdr_to_skb (struct sk_buff *skb,const struct virtio_net_hdr *hdr, -bool little_endian,u8 hdr_gso_type) +bool little_endian,u8 hdr_gso_type, +bool need_csum_check) { -unsigned int nh_min_len =sizeof (struct iphdr ); + typeof(skb->transport_header) old_transport = skb->transport_header;unsigned int gso_type =0 ;unsigned int thlen =0 ;unsigned int p_off =0 ;unsigned int ip_proto; +bool csum_partial_ok = true;if (hdr_gso_type !=VIRTIO_NET_HDR_GSO_NONE ) {switch (hdr_gso_type & ~VIRTIO_NET_HDR_GSO_ECN ) {@@ -69,7 +169,6 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb, gso_type =SKB_GSO_TCPV6 ; ip_proto =IPPROTO_TCP ; thlen =sizeof (struct tcphdr ); - nh_min_len =sizeof (struct ipv6hdr );break ;case VIRTIO_NET_HDR_GSO_UDP : gso_type =SKB_GSO_UDP ;@@ -104,11 +203,11 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,if (!skb_partial_csum_set (skb, start, off))return -EINVAL ; -if (skb_transport_offset (skb) < nh_min_len) -return -EINVAL ; +if (need_csum_check) + csum_partial_ok = +virtio_net_csum_partial_ok (skb, hdr_gso_type); - nh_min_len =skb_transport_offset (skb); - p_off = nh_min_len + thlen; + p_off =skb_transport_offset (skb) + thlen;if (!pskb_may_pull (skb, p_off))return -EINVAL ; }else if (gso_type) {@@ -158,8 +257,6 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,case SKB_GSO_UDP_L4 :if (!(hdr->flags &VIRTIO_NET_HDR_F_NEEDS_CSUM ))return -EINVAL ; -if (skb->csum_offset != offsetof(struct udphdr , check)) -return -EINVAL ;if (skb->len - p_off > gso_size *UDP_MAX_SEGMENTS )return -EINVAL ;if (gso_type !=SKB_GSO_UDP_L4 )@@ -167,10 +264,6 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,break ;case SKB_GSO_TCPV4 :case SKB_GSO_TCPV6 : -if (skb->ip_summed ==CHECKSUM_PARTIAL && - skb->csum_offset != offsetof(struct tcphdr , check)) -return -EINVAL ; -BUILD_BUG_ON (TCP_MIN_GSO_SIZE *GSO_MAX_SEGS <GSO_MAX_SIZE ); gso_size =max (gso_size,TCP_MIN_GSO_SIZE );break ;@@ -191,6 +284,17 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb, } }
Quoted 7 lines
include/linux/virtio_net.h static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb, · line 293
Quoted 7 lines
include/linux/virtio_net.h +7
-0+if ((hdr->flags &VIRTIO_NET_HDR_F_NEEDS_CSUM ) && !csum_partial_ok) { +int err; + +if (skb_is_gso (skb)) +return -EINVAL ; + + err =skb_checksum_help (skb);
Instead of this variable csum_partial_ok and this branch, could virtio_net_csum_partial_ok just call skb_checksum_help directly and drop VIRTIO_NET_HDR_F_NEEDS_CSUM?
Quoted 7 lines
Quoted 7 lines
include/linux/virtio_net.h +4
-0+ skb->transport_header = old_transport; +return err; + } +return 0 ; }
Quoted 7 lines
include/linux/virtio_net.h static inline int virtio_net_hdr_to_skb(struct sk_buff *skb, · line 306
Quoted 7 lines
include/linux/virtio_net.h +2
-1@@ -198,7 +302,8 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,const struct virtio_net_hdr *hdr, bool little_endian) { - return __virtio_net_hdr_to_skb(skb, hdr, little_endian, hdr->gso_type); + return __virtio_net_hdr_to_skb(skb, hdr, little_endian, + hdr->gso_type, true);
Would be great if we can avoid the extra arg and having to update the callers.
Quoted 20 lines
Quoted 20 lines
include/linux/virtio_net.h +2
-1}/* This function must be called after virtio_net_hdr_from_skb(). */ @@ -293,6 +398,7 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,unsigned int inner_nh, outer_th, inner_th;unsigned int inner_l3min, outer_l3min;u8 gso_inner_type, gso_tunnel_type; +__be16 inner_proto;bool outer_isv6, inner_isv6;int ret;@@ -334,7 +440,7 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,/* Let the basic parsing deal with plain GSO features. */ ret =__virtio_net_hdr_to_skb (skb, hdr, true, - hdr->gso_type & ~gso_tunnel_type); + hdr->gso_type & ~gso_tunnel_type, false);if (ret)return ret;
Quoted 8 lines
include/linux/virtio_net.h virtio_net_hdr_tnl_to_skb(struct sk_buff *skb, · line 462
Quoted 8 lines
include/linux/virtio_net.h +2
-2@@ -352,8 +458,8 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,return -EINVAL ; } -skb_set_inner_protocol (skb, inner_isv6 ?htons (ETH_P_IPV6 ) : -htons (ETH_P_IP )); + inner_proto = inner_isv6 ?htons (ETH_P_IPV6 ) :htons (ETH_P_IP ); +skb_set_inner_protocol (skb, inner_proto);
Can drop
Quoted 7 lines
Quoted 7 lines
include/linux/virtio_net.h +0
-0if (hdr->flags &VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM ) {if (!tnl_csum_negotiated)return -EINVAL ;@@ -368,7 +474,16 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb, skb->inner_mac_header = inner_nh +skb_headroom (skb); skb->transport_header = outer_th +skb_headroom (skb); skb->encapsulation =1 ;
Quoted 8 lines
include/linux/virtio_net.h virtio_net_hdr_tnl_to_skb(struct sk_buff *skb, · line 483
Quoted 8 lines
include/linux/virtio_net.h +7
-1-return 0 ; + +if (virtio_net_csum_partial_ok_at_offset (skb, gso_inner_type, inner_nh, + inner_proto)) +return 0 ; + +if (skb_is_gso (skb)) +return -EINVAL ;
This is a risk the commit message will have to call out.
I don't see a simple way around it. We discussed segmenting here too, but that requires intrusive changes to all callers too.
Alternatively,
1. call segmentation, but only to see it remove the SKB_GSO_DODGY and thus verify that the TSO/USO packet is valid. Still drop otherwise. This also does not add overhead, because it would be called later anyway.
2. a failsafe off-by-default sysctl + static_branch that skips the drop when set. Messy, but so is the situation we're dealing with.
Michael S. Tsirkin
maintainerRe: [PATCH net-next 3/4] net: resolve uncommon partial checksums at ingress
Quoted 230 lines · Willem de Bruijn
Quoted 77 lines · Michael S. Tsirkin
Quoted 15 lines · Michael S. Tsirkin
A virtio net header can place
CHECKSUM_PARTIALat any in-packet offset. If that state is retained, later checksum completion can modify the packet after protocol processing has made decisions from it.Let's fix that: let's validate
csum_startandcsum_offsetand only keepCHECKSUM_PARTIALwhencsum_startandcsum_offsetmatch a parsed TCP or UDP header, while handling VLAN headers, IPv4 options and IPv6 extension headers. Complete other non-GSO checksums immediately, before any protocol processing runs.We can't easily "complete the checksum" for GSO we don't recognize - GSO assumes knowledge of the protocol. We could segment ignoring the checksum then do whatever
csum_startandcsum_offsetsay, but that seems like an overkill for something that should happen.Quoted 7 lines
UDP tunnel headers get the same handling on the inner header.
Packets without
VIRTIO_NET_HDR_F_NEEDS_CSUMare unaffected.Suggested-by Willem de Bruijn <[email protected]>Signed-off-by Michael S. Tsirkin <[email protected]>Assisted-by LLMI forgot reported-by tags:
Reported-by Paulos Yibelo <[email protected]>Reported-by Chris Arges <[email protected]>Quoted 45 lines
--- include/linux/virtio_net.h | 151 ++++++++++++++++++++++++++++++++----- 1 file changed, 133 insertions(+), 18 deletions(-)include/linux/virtio_net.h +25
-0@@ -5,7 +5,11 @@#include <linux/if_vlan.h> #include <linux/ip.h> #include <linux/ipv6.h> +#include <linux/netdevice.h> #include <linux/udp.h> +#include <net/gso.h> +#include <net/ip.h> +#include <net/ipv6.h> #include <net/tcp.h> #include <uapi/linux/tcp.h> #include <uapi/linux/virtio_net.h> @@ -48,15 +52,111 @@ static inline int virtio_net_hdr_set_proto(struct sk_buff *skb,return 0 ; } +/* Does checksum metadata match a known TCP or UDP header? */ +static inline bool +virtio_net_csum_partial_ok_at_offset (struct sk_buff *skb,u8 hdr_gso_type, +unsigned int nhoff,__be16 proto) +{ +unsigned int thoff; +int ip_proto; + +if (proto ==htons (ETH_P_IP )) { +const struct iphdr *iph =ip4_parse (skb, nhoff, &thoff); + +if (!iph ||ip_is_fragment (iph)) +return false; + ip_proto = iph->protocol; + }else if (proto ==htons (ETH_P_IPV6 )) { +const struct ipv6hdr *iph; +int flags =0 ; + +if (!pskb_may_pull (skb, nhoff +sizeof (*iph))) +return false; + iph = (const void *)(skb->data + nhoff);Quoted 8 lines
include/linux/virtio_net.h static inline int virtio_net_hdr_set_proto(struct sk_buff *skb, · line 83
Quoted 8 lines
include/linux/virtio_net.h +8
-0+if (iph->version !=6 ) +return false; + thoff = nhoff +sizeof (*iph); + ip_proto = iph->nexthdr; +if (ip_proto !=IPPROTO_TCP && ip_proto !=IPPROTO_UDP ) { + thoff = nhoff; + ip_proto =ipv6_find_hdr (skb, &thoff,-1 ,NULL , + &flags);Skip this rare case of extension headers?
Quoted 72 lines
Quoted 72 lines
include/linux/virtio_net.h +15
-13+if (ip_proto <0 || flags &IP6_FH_F_FRAG ) +return false; + } + }else { +return false; + } +static inline int __virtio_net_hdr_to_skb (struct sk_buff *skb,const struct virtio_net_hdr *hdr, -bool little_endian,u8 hdr_gso_type) +bool little_endian,u8 hdr_gso_type, +bool need_csum_check) { -unsigned int nh_min_len =sizeof (struct iphdr ); + typeof(skb->transport_header) old_transport = skb->transport_header;unsigned int gso_type =0 ;unsigned int thlen =0 ;unsigned int p_off =0 ;unsigned int ip_proto; +bool csum_partial_ok = true;if (hdr_gso_type !=VIRTIO_NET_HDR_GSO_NONE ) {switch (hdr_gso_type & ~VIRTIO_NET_HDR_GSO_ECN ) {@@ -69,7 +169,6 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb, gso_type =SKB_GSO_TCPV6 ; ip_proto =IPPROTO_TCP ; thlen =sizeof (struct tcphdr ); - nh_min_len =sizeof (struct ipv6hdr );break ;case VIRTIO_NET_HDR_GSO_UDP : gso_type =SKB_GSO_UDP ;@@ -104,11 +203,11 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,if (!skb_partial_csum_set (skb, start, off))return -EINVAL ; -if (skb_transport_offset (skb) < nh_min_len) -return -EINVAL ; +if (need_csum_check) + csum_partial_ok = +virtio_net_csum_partial_ok (skb, hdr_gso_type); - nh_min_len =skb_transport_offset (skb); - p_off = nh_min_len + thlen; + p_off =skb_transport_offset (skb) + thlen;if (!pskb_may_pull (skb, p_off))return -EINVAL ; }else if (gso_type) {@@ -158,8 +257,6 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,case SKB_GSO_UDP_L4 :if (!(hdr->flags &VIRTIO_NET_HDR_F_NEEDS_CSUM ))return -EINVAL ; -if (skb->csum_offset != offsetof(struct udphdr , check)) -return -EINVAL ;if (skb->len - p_off > gso_size *UDP_MAX_SEGMENTS )return -EINVAL ;if (gso_type !=SKB_GSO_UDP_L4 )@@ -167,10 +264,6 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,break ;case SKB_GSO_TCPV4 :case SKB_GSO_TCPV6 : -if (skb->ip_summed ==CHECKSUM_PARTIAL && - skb->csum_offset != offsetof(struct tcphdr , check)) -return -EINVAL ; -BUILD_BUG_ON (TCP_MIN_GSO_SIZE *GSO_MAX_SEGS <GSO_MAX_SIZE ); gso_size =max (gso_size,TCP_MIN_GSO_SIZE );break ;@@ -191,6 +284,17 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb, } }Quoted 7 lines
include/linux/virtio_net.h static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb, · line 293
Quoted 7 lines
include/linux/virtio_net.h +7
-0+if ((hdr->flags &VIRTIO_NET_HDR_F_NEEDS_CSUM ) && !csum_partial_ok) { +int err; + +if (skb_is_gso (skb)) +return -EINVAL ; + + err =skb_checksum_help (skb);Instead of this variable
csum_partial_okand this branch, couldvirtio_net_csum_partial_okjust callskb_checksum_helpdirectly and dropVIRTIO_NET_HDR_F_NEEDS_CSUM?Quoted 7 lines
Quoted 7 lines
include/linux/virtio_net.h +4
-0+ skb->transport_header = old_transport; +return err; + } +return 0 ; }Quoted 7 lines
include/linux/virtio_net.h static inline int virtio_net_hdr_to_skb(struct sk_buff *skb, · line 306
Quoted 7 lines
include/linux/virtio_net.h +2
-1@@ -198,7 +302,8 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,const struct virtio_net_hdr *hdr, bool little_endian) { - return __virtio_net_hdr_to_skb(skb, hdr, little_endian, hdr->gso_type); + return __virtio_net_hdr_to_skb(skb, hdr, little_endian, + hdr->gso_type, true);Would be great if we can avoid the extra arg and having to update the callers.
Quoted 20 lines
Quoted 20 lines
include/linux/virtio_net.h +2
-1}/* This function must be called after virtio_net_hdr_from_skb(). */ @@ -293,6 +398,7 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,unsigned int inner_nh, outer_th, inner_th;unsigned int inner_l3min, outer_l3min;u8 gso_inner_type, gso_tunnel_type; +__be16 inner_proto;bool outer_isv6, inner_isv6;int ret;@@ -334,7 +440,7 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,/* Let the basic parsing deal with plain GSO features. */ ret =__virtio_net_hdr_to_skb (skb, hdr, true, - hdr->gso_type & ~gso_tunnel_type); + hdr->gso_type & ~gso_tunnel_type, false);if (ret)return ret;Quoted 8 lines
include/linux/virtio_net.h virtio_net_hdr_tnl_to_skb(struct sk_buff *skb, · line 462
Quoted 8 lines
include/linux/virtio_net.h +2
-2@@ -352,8 +458,8 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,return -EINVAL ; } -skb_set_inner_protocol (skb, inner_isv6 ?htons (ETH_P_IPV6 ) : -htons (ETH_P_IP )); + inner_proto = inner_isv6 ?htons (ETH_P_IPV6 ) :htons (ETH_P_IP ); +skb_set_inner_protocol (skb, inner_proto);Can drop
Quoted 9 lines
Quoted 9 lines
include/linux/virtio_net.h +1
-1if (hdr->flags &VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM ) {if (!tnl_csum_negotiated)return -EINVAL ;@@ -368,7 +474,16 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb, skb->inner_mac_header = inner_nh +skb_headroom (skb); skb->transport_header = outer_th +skb_headroom (skb); skb->encapsulation =1 ; -return 0 ; +
Quoted 8 lines
Quoted 6 lines
include/linux/virtio_net.h virtio_net_hdr_tnl_to_skb(struct sk_buff *skb, · line 483
Quoted 6 lines
include/linux/virtio_net.h +6
-0+if (virtio_net_csum_partial_ok_at_offset (skb, gso_inner_type, inner_nh, + inner_proto)) +return 0 ; + +if (skb_is_gso (skb)) +return -EINVAL ;This is a risk the commit message will have to call out.
I tried with this:
> > > We can't easily "complete the checksum" for GSO we don't recognize - GSO > > > assumes knowledge of the protocol. We could segment ignoring the > > > checksum then do whatever csum_start and csum_offset say, but that > > > seems like an overkill for something that should happen.
I guess I can add a more detailed explanation about what the risk is?
Quoted 11 lines
I don't see a simple way around it. We discussed segmenting here too, but that requires intrusive changes to all callers too. Alternatively,
1. call segmentation, but only to see it remove the
SKB_GSO_DODGYand thus verify that the TSO/USO packet is valid. Still drop otherwise. This also does not add overhead, because it would be called later anyway.2. a failsafe off-by-default sysctl +
static_branchthat skips the drop when set. Messy, but so is the situation we're dealing with.
Tack this on top or do you feel it's needed straight away.
Willem de Bruijn
maintainerRe: [PATCH net-next 3/4] net: resolve uncommon partial checksums at ingress
Quoted 10 lines
Quoted 9 lines
Quoted 7 lines
include/linux/virtio_net.h virtio_net_hdr_tnl_to_skb(struct sk_buff *skb, · line 483
Quoted 7 lines
include/linux/virtio_net.h +7
-0+ +if (virtio_net_csum_partial_ok_at_offset (skb, gso_inner_type, inner_nh, + inner_proto)) +return 0 ; + +if (skb_is_gso (skb)) +return -EINVAL ;This is a risk the commit message will have to call out.
Quoted 8 lines
I tried with this:
> > > We can't easily "complete the checksum" for GSO we don't recognize - GSO > > > assumes knowledge of the protocol. We could segment ignoring the > > > checksum then do whatever
csum_startandcsum_offsetsay, but that > > > seems like an overkill for something that should happen.I guess I can add a more detailed explanation about what the risk is?
Let's just explicitly mention the consequence: non-standard GSO packets that previous would be sent may now fail.
Quoted 10 lines
Quoted 10 lines
I don't see a simple way around it. We discussed segmenting here too, but that requires intrusive changes to all callers too. Alternatively,
1. call segmentation, but only to see it remove the
SKB_GSO_DODGYand thus verify that the TSO/USO packet is valid. Still drop otherwise. This also does not add overhead, because it would be called later anyway.
Quoted 4 lines
Quoted 2 lines
2. a failsafe off-by-default sysctl +
static_branchthat skips the drop when set. Messy, but so is the situation we're dealing with.Tack this on top or do you feel it's needed straight away.
Not urgent. Can even wait until a report appears. But worth taking a look sooner if either of us gets around to it.
Michael S. Tsirkin
maintainerRe: [PATCH net-next 3/4] net: resolve uncommon partial checksums at ingress
Quoted 32 lines · Willem de Bruijn
Quoted 10 lines
Quoted 9 lines
Quoted 7 lines
include/linux/virtio_net.h virtio_net_hdr_tnl_to_skb(struct sk_buff *skb, · line 483
Quoted 7 lines
include/linux/virtio_net.h +7
-0+ +if (virtio_net_csum_partial_ok_at_offset (skb, gso_inner_type, inner_nh, + inner_proto)) +return 0 ; + +if (skb_is_gso (skb)) +return -EINVAL ;This is a risk the commit message will have to call out.
Quoted 8 lines
I tried with this:
> > > We can't easily "complete the checksum" for GSO we don't recognize - GSO > > > assumes knowledge of the protocol. We could segment ignoring the > > > checksum then do whatever
csum_startandcsum_offsetsay, but that > > > seems like an overkill for something that should happen.I guess I can add a more detailed explanation about what the risk is?
Let's just explicitly mention the consequence: non-standard GSO packets that previous would be sent may now fail.
Quoted 10 lines
Quoted 10 lines
I don't see a simple way around it. We discussed segmenting here too, but that requires intrusive changes to all callers too. Alternatively,
1. call segmentation, but only to see it remove the
SKB_GSO_DODGYand thus verify that the TSO/USO packet is valid. Still drop otherwise. This also does not add overhead, because it would be called later anyway.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
2. a failsafe off-by-default sysctl +
static_branchthat skips the drop when set. Messy, but so is the situation we're dealing with.Tack this on top or do you feel it's needed straight away.
Not urgent. Can even wait until a report appears. But worth taking a look sooner if either of us gets around to it.
Maybe if people intend all this for stable, I'll feel safer with this in there.
Just to make sure, the branch would just short-circuit all checks, yes?
Signature · 1 line
MST
Willem de Bruijn
maintainerRe: [PATCH net-next 3/4] net: resolve uncommon partial checksums at ingress
Quoted 39 lines · Michael S. Tsirkin
Quoted 37 lines · Willem de Bruijn
Quoted 10 lines
Quoted 9 lines
Quoted 7 lines
include/linux/virtio_net.h virtio_net_hdr_tnl_to_skb(struct sk_buff *skb, · line 483
Quoted 7 lines
include/linux/virtio_net.h +7
-0+ +if (virtio_net_csum_partial_ok_at_offset (skb, gso_inner_type, inner_nh, + inner_proto)) +return 0 ; + +if (skb_is_gso (skb)) +return -EINVAL ;This is a risk the commit message will have to call out.
Quoted 8 lines
I tried with this:
> > > We can't easily "complete the checksum" for GSO we don't recognize - GSO > > > assumes knowledge of the protocol. We could segment ignoring the > > > checksum then do whatever
csum_startandcsum_offsetsay, but that > > > seems like an overkill for something that should happen.I guess I can add a more detailed explanation about what the risk is?
Let's just explicitly mention the consequence: non-standard GSO packets that previous would be sent may now fail.
Quoted 14 lines
Quoted 5 lines
I don't see a simple way around it. We discussed segmenting here too, but that requires intrusive changes to all callers too. Alternatively,
Quoted 7 lines
1. call segmentation, but only to see it remove the
SKB_GSO_DODGYand thus verify that the TSO/USO packet is valid. Still drop otherwise. This also does not add overhead, because it would be called later anyway.2. a failsafe off-by-default sysctl +
static_branchthat skips the drop when set. Messy, but so is the situation we're dealing with.Tack this on top or do you feel it's needed straight away.
Quoted 7 lines
Quoted 2 lines
Not urgent. Can even wait until a report appears. But worth taking a look sooner if either of us gets around to it.
Maybe if people intend all this for stable, I'll feel safer with this in there.
Just to make sure, the branch would just short-circuit all checks, yes?
Ah option 2, the failsafe? At least any new failures (GSO) that could cause regression reports. Operating entirely as before this series sounds fine too.
Michael S. Tsirkin
maintainerRe: [PATCH net-next 3/4] net: resolve uncommon partial checksums at ingress
Quoted 43 lines · Willem de Bruijn
Quoted 42 lines · Michael S. Tsirkin
Quoted 39 lines · Willem de Bruijn
Quoted 10 lines
Quoted 9 lines
Quoted 7 lines
Review
Quoted 7 lines
(quoted diff) +7
-0+ + if (virtio_net_csum_partial_ok_at_offset(skb, gso_inner_type, inner_nh, + inner_proto)) + return 0; + + if (skb_is_gso(skb)) + return -EINVAL;This is a risk the commit message will have to call out.
Quoted 8 lines
I tried with this:
> > > We can't easily "complete the checksum" for GSO we don't recognize - GSO > > > assumes knowledge of the protocol. We could segment ignoring the > > > checksum then do whatever
csum_startandcsum_offsetsay, but that > > > seems like an overkill for something that should happen.I guess I can add a more detailed explanation about what the risk is?
Let's just explicitly mention the consequence: non-standard GSO packets that previous would be sent may now fail.
Quoted 10 lines
Quoted 10 lines
I don't see a simple way around it. We discussed segmenting here too, but that requires intrusive changes to all callers too. Alternatively,
1. call segmentation, but only to see it remove the
SKB_GSO_DODGYand thus verify that the TSO/USO packet is valid. Still drop otherwise. This also does not add overhead, because it would be called later anyway.Quoted 4 lines
Quoted 2 lines
2. a failsafe off-by-default sysctl +
static_branchthat skips the drop when set. Messy, but so is the situation we're dealing with.Tack this on top or do you feel it's needed straight away.
Not urgent. Can even wait until a report appears. But worth taking a look sooner if either of us gets around to it.
Quoted 8 lines
Quoted 4 lines
Maybe if people intend all this for stable, I'll feel safer with this in there.
Just to make sure, the branch would just short-circuit all checks, yes?
Ah option 2, the failsafe? At least any new failures (GSO) that could cause regression reports. Operating entirely as before this series sounds fine too.
Right. I feel 1 is just as likely to cause regressions. At least 2 gives us exactly legacy behaviour back.
Signature · 1 line
MST
Michael S. Tsirkin
maintainerRe: [PATCH net-next 3/4] net: resolve uncommon partial checksums at ingress
In reply to Willem de BruijnQuoted 80 lines · Willem de Bruijn
Quoted 79 lines · Michael S. Tsirkin
Quoted 15 lines · Michael S. Tsirkin
A virtio net header can place
CHECKSUM_PARTIALat any in-packet offset. If that state is retained, later checksum completion can modify the packet after protocol processing has made decisions from it.Let's fix that: let's validate
csum_startandcsum_offsetand only keepCHECKSUM_PARTIALwhencsum_startandcsum_offsetmatch a parsed TCP or UDP header, while handling VLAN headers, IPv4 options and IPv6 extension headers. Complete other non-GSO checksums immediately, before any protocol processing runs.We can't easily "complete the checksum" for GSO we don't recognize - GSO assumes knowledge of the protocol. We could segment ignoring the checksum then do whatever
csum_startandcsum_offsetsay, but that seems like an overkill for something that should happen.Quoted 7 lines
UDP tunnel headers get the same handling on the inner header.
Packets without
VIRTIO_NET_HDR_F_NEEDS_CSUMare unaffected.Suggested-by Willem de Bruijn <[email protected]>Signed-off-by Michael S. Tsirkin <[email protected]>Assisted-by LLMI forgot reported-by tags:
Reported-by Paulos Yibelo <[email protected]>Reported-by Chris Arges <[email protected]>Quoted 47 lines
--- include/linux/virtio_net.h | 151 ++++++++++++++++++++++++++++++++----- 1 file changed, 133 insertions(+), 18 deletions(-)include/linux/virtio_net.h +27
-0@@ -5,7 +5,11 @@#include <linux/if_vlan.h> #include <linux/ip.h> #include <linux/ipv6.h> +#include <linux/netdevice.h> #include <linux/udp.h> +#include <net/gso.h> +#include <net/ip.h> +#include <net/ipv6.h> #include <net/tcp.h> #include <uapi/linux/tcp.h> #include <uapi/linux/virtio_net.h> @@ -48,15 +52,111 @@ static inline int virtio_net_hdr_set_proto(struct sk_buff *skb,return 0 ; } +/* Does checksum metadata match a known TCP or UDP header? */ +static inline bool +virtio_net_csum_partial_ok_at_offset (struct sk_buff *skb,u8 hdr_gso_type, +unsigned int nhoff,__be16 proto) +{ +unsigned int thoff; +int ip_proto; + +if (proto ==htons (ETH_P_IP )) { +const struct iphdr *iph =ip4_parse (skb, nhoff, &thoff); + +if (!iph ||ip_is_fragment (iph)) +return false; + ip_proto = iph->protocol; + }else if (proto ==htons (ETH_P_IPV6 )) { +const struct ipv6hdr *iph; +int flags =0 ; + +if (!pskb_may_pull (skb, nhoff +sizeof (*iph))) +return false; + iph = (const void *)(skb->data + nhoff); +if (iph->version !=6 ) +return false;
Quoted 8 lines
Quoted 6 lines
include/linux/virtio_net.h static inline int virtio_net_hdr_set_proto(struct sk_buff *skb, · line 83
Quoted 6 lines
include/linux/virtio_net.h +6
-0+ thoff = nhoff +sizeof (*iph); + ip_proto = iph->nexthdr; +if (ip_proto !=IPPROTO_TCP && ip_proto !=IPPROTO_UDP ) { + thoff = nhoff; + ip_proto =ipv6_find_hdr (skb, &thoff,-1 ,NULL , + &flags);Skip this rare case of extension headers?
but then it will be dropped if it's gso. so we can't.
Quoted 72 lines
Quoted 72 lines
Quoted 72 lines
include/linux/virtio_net.h +15
-13+if (ip_proto <0 || flags &IP6_FH_F_FRAG ) +return false; + } + }else { +return false; + } +static inline int __virtio_net_hdr_to_skb (struct sk_buff *skb,const struct virtio_net_hdr *hdr, -bool little_endian,u8 hdr_gso_type) +bool little_endian,u8 hdr_gso_type, +bool need_csum_check) { -unsigned int nh_min_len =sizeof (struct iphdr ); + typeof(skb->transport_header) old_transport = skb->transport_header;unsigned int gso_type =0 ;unsigned int thlen =0 ;unsigned int p_off =0 ;unsigned int ip_proto; +bool csum_partial_ok = true;if (hdr_gso_type !=VIRTIO_NET_HDR_GSO_NONE ) {switch (hdr_gso_type & ~VIRTIO_NET_HDR_GSO_ECN ) {@@ -69,7 +169,6 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb, gso_type =SKB_GSO_TCPV6 ; ip_proto =IPPROTO_TCP ; thlen =sizeof (struct tcphdr ); - nh_min_len =sizeof (struct ipv6hdr );break ;case VIRTIO_NET_HDR_GSO_UDP : gso_type =SKB_GSO_UDP ;@@ -104,11 +203,11 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,if (!skb_partial_csum_set (skb, start, off))return -EINVAL ; -if (skb_transport_offset (skb) < nh_min_len) -return -EINVAL ; +if (need_csum_check) + csum_partial_ok = +virtio_net_csum_partial_ok (skb, hdr_gso_type); - nh_min_len =skb_transport_offset (skb); - p_off = nh_min_len + thlen; + p_off =skb_transport_offset (skb) + thlen;if (!pskb_may_pull (skb, p_off))return -EINVAL ; }else if (gso_type) {@@ -158,8 +257,6 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,case SKB_GSO_UDP_L4 :if (!(hdr->flags &VIRTIO_NET_HDR_F_NEEDS_CSUM ))return -EINVAL ; -if (skb->csum_offset != offsetof(struct udphdr , check)) -return -EINVAL ;if (skb->len - p_off > gso_size *UDP_MAX_SEGMENTS )return -EINVAL ;if (gso_type !=SKB_GSO_UDP_L4 )@@ -167,10 +264,6 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,break ;case SKB_GSO_TCPV4 :case SKB_GSO_TCPV6 : -if (skb->ip_summed ==CHECKSUM_PARTIAL && - skb->csum_offset != offsetof(struct tcphdr , check)) -return -EINVAL ; -BUILD_BUG_ON (TCP_MIN_GSO_SIZE *GSO_MAX_SEGS <GSO_MAX_SIZE ); gso_size =max (gso_size,TCP_MIN_GSO_SIZE );break ;@@ -191,6 +284,17 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb, } }
Quoted 11 lines
Quoted 7 lines
include/linux/virtio_net.h static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb, · line 293
Quoted 7 lines
include/linux/virtio_net.h +7
-0+if ((hdr->flags &VIRTIO_NET_HDR_F_NEEDS_CSUM ) && !csum_partial_ok) { +int err; + +if (skb_is_gso (skb)) +return -EINVAL ; + + err =skb_checksum_help (skb);Instead of this variable
csum_partial_okand this branch, couldvirtio_net_csum_partial_okjust callskb_checksum_helpdirectly and dropVIRTIO_NET_HDR_F_NEEDS_CSUM?
no, hdr is const. and I think I like it that way. we could copy flags I guess - I'll play with it a bit.
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
include/linux/virtio_net.h +4
-0+ skb->transport_header = old_transport; +return err; + } +return 0 ; }
Quoted 9 lines
Quoted 7 lines
include/linux/virtio_net.h static inline int virtio_net_hdr_to_skb(struct sk_buff *skb, · line 306
Quoted 7 lines
include/linux/virtio_net.h +2
-1@@ -198,7 +302,8 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,const struct virtio_net_hdr *hdr, bool little_endian) { - return __virtio_net_hdr_to_skb(skb, hdr, little_endian, hdr->gso_type); + return __virtio_net_hdr_to_skb(skb, hdr, little_endian, + hdr->gso_type, true);Would be great if we can avoid the extra arg and having to update the callers.
There are 2 callers why is this a concern?
But yes the extra bool is kinda ugly. i'll look into refactoring this but i do not think the diff will be smaller - likely larger.
Quoted 24 lines
Quoted 24 lines
Quoted 24 lines
include/linux/virtio_net.h +2
-1}/* This function must be called after virtio_net_hdr_from_skb(). */ @@ -293,6 +398,7 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,unsigned int inner_nh, outer_th, inner_th;unsigned int inner_l3min, outer_l3min;u8 gso_inner_type, gso_tunnel_type; +__be16 inner_proto;bool outer_isv6, inner_isv6;int ret;@@ -334,7 +440,7 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,/* Let the basic parsing deal with plain GSO features. */ ret =__virtio_net_hdr_to_skb (skb, hdr, true, - hdr->gso_type & ~gso_tunnel_type); + hdr->gso_type & ~gso_tunnel_type, false);if (ret)return ret;@@ -352,8 +458,8 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,return -EINVAL ; }
Quoted 6 lines
Quoted 4 lines
include/linux/virtio_net.h virtio_net_hdr_tnl_to_skb(struct sk_buff *skb, · line 462
Quoted 4 lines
include/linux/virtio_net.h +2
-2-skb_set_inner_protocol (skb, inner_isv6 ?htons (ETH_P_IPV6 ) : -htons (ETH_P_IP )); + inner_proto = inner_isv6 ?htons (ETH_P_IPV6 ) :htons (ETH_P_IP ); +skb_set_inner_protocol (skb, inner_proto);Can drop
It is here because inner_proto is used below, seems silly to re-compute when we have it.
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
include/linux/virtio_net.h +1
-1if (hdr->flags &VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM ) {if (!tnl_csum_negotiated)return -EINVAL ;@@ -368,7 +474,16 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb, skb->inner_mac_header = inner_nh +skb_headroom (skb); skb->transport_header = outer_th +skb_headroom (skb); skb->encapsulation =1 ; -return 0 ; +
Quoted 8 lines
Quoted 6 lines
include/linux/virtio_net.h virtio_net_hdr_tnl_to_skb(struct sk_buff *skb, · line 483
Quoted 6 lines
include/linux/virtio_net.h +6
-0+if (virtio_net_csum_partial_ok_at_offset (skb, gso_inner_type, inner_nh, + inner_proto)) +return 0 ; + +if (skb_is_gso (skb)) +return -EINVAL ;This is a risk the commit message will have to call out.
I tried but good point, not enough. Will do.
Quoted 9 lines
I don't see a simple way around it. We discussed segmenting here too, but that requires intrusive changes to all callers too.
Alternatively,
1. call segmentation, but only to see it remove the
SKB_GSO_DODGYand thus verify that the TSO/USO packet is valid. Still drop otherwise. This also does not add overhead, because it would be called later anyway.
Yes, I implemented this originally. It's an even more messy code but the big issue is that it's not clear it is worth it. So we get gso with a bad checksum. Currently it goes out on the wire corrupted. Ideally we would do exactly that, but that would mean much more work that SKB_GSO_DODGY play. Conversely we could just ignore the checksum hints. Then the packet would go on wire because we "fixed" it but it just has a chance to cause even more problems. So... shrug. I opted for the simpler way.
So... shrug. I opted for the simpler way. If a real workload shows up, the counters in the last patch will tell us and we can revisit.
I will add this to the cover letter I feel, not commit log material, right?
Quoted 2 lines
2. a failsafe off-by-default sysctl +
static_branchthat skips the drop when set. Messy, but so is the situation we're dealing with.
good idea, I'll add that.
Signature · 1 line
MST
[email protected]
AI reviewRe: [PATCH net-next 3/4] net: resolve uncommon partial checksums at ingress
In reply to Michael S. TsirkinSashiko has reviewed this patch and found no issues. It looks great!
Quoted 15 lines
A virtio net header can place
CHECKSUM_PARTIALat any in-packet offset. If that state is retained, later checksum completion can modify the packet after protocol processing has made decisions from it.Let's fix that: let's validate
csum_startandcsum_offsetand only keepCHECKSUM_PARTIALwhencsum_startandcsum_offsetmatch a parsed TCP or UDP header, while handling VLAN headers, IPv4 options and IPv6 extension headers. Complete other non-GSO checksums immediately, before any protocol processing runs.We can't easily "complete the checksum" for GSO we don't recognize - GSO assumes knowledge of the protocol. We could segment ignoring the checksum then do whatever
csum_startandcsum_offsetsay, but that seems like an overkill for something that should happen.
Quoted 6 lines
UDP tunnel headers get the same handling on the inner header.
Packets without
VIRTIO_NET_HDR_F_NEEDS_CSUMare unaffected.Suggested-by Willem de Bruijn <[email protected]>Signed-off-by Michael S. Tsirkin <[email protected]>
Sashiko has reviewed this patch and found no issues. It looks great!
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
[email protected]
AI reviewRe: [PATCH net-next 3/4] net: resolve uncommon partial checksums at ingress
In reply to Michael S. TsirkinThank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider.
Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider.
Critical: 0 · High: 1 · Medium: 5 · Low: 2
If you believe none of the issues are legitimate, please reply to each issue of Medium severity or higher and explain why the AI is wrong. If any of them are legitimate and you plan to respin, please let patchwork know by including "pw-bot: cr" as a separate line at the end of your reply (one such reply per series is enough).
- [High] On the UDP-tunnel path, virtio_net_hdr_tnl_to_skb() checks CHECKSUM_PARTIAL only against the 'inner' IP/TCP/UDP header at…
- [Medium] With this patch applied, virtio_net_hdr_tnl_to_skb() in include/linux/virtio_net.h ends with `ret = skb_checksum_help(skb);…
- [Medium] On UML, the new skb_checksum_help() fallback in __virtio_net_hdr_to_skb() checksums the whole preallocated receive…
- [Medium] On UML, the patch's new rejection of GSO packets with unrecognised checksum metadata never takes effect.
- [Medium] On any device whose type is not ARPHRD_ETHER, virtio_net_csum_partial_ok() takes the L3 protocol only from…
- [Medium] For non-ARPHRD_ETHER devices that still carry Ethernet framing, chiefly loopback (ARPHRD_LOOPBACK, hard_header_len =…
- [Low] The commit "net: resolve uncommon partial checksums at ingress" is the actual fix for user-reachable checksum-metadata abuse, but it…
- [Low] The commit message does not state the user-visible accept/reject changes precisely, and it contains a typo that inverts its meaning.
--
net: resolve uncommon partial checksums at ingress
This commit validates the virtio net header csum_start and csum_offset at ingress. CHECKSUM_PARTIAL is kept only when they match a parsed TCP or UDP header behind Ethernet/VLAN and IPv4/IPv6. Other non-GSO partial checksums are completed at once with skb_checksum_help(), unrecognized GSO is rejected, and the UDP tunnel path checks the inner header the same way.
Quoted 4 lines
We can't easily "complete the checksum" for GSO we don't recognize - GSO assumes knowledge of the protocol. We could segment ignoring the checksum then do whatever
csum_startandcsum_offsetsay, but that seems like an overkill for something that should happen.
[Severity: Low]
This isn't a bug, but should "something that should happen" read "something that shouldn't happen"?
Could the commit message also spell out the accept/reject changes?
This patch removes two checks from __virtio_net_hdr_to_skb(): the skb_transport_offset(skb) < nh_min_len check and the per-GSO-type csum_offset checks. Non-GSO packets that used to fail with -EINVAL (and were counted in tun's rx_frame_errors) are now accepted after skb_checksum_help().
In the other direction, GSO packets are now rejected unless csum_start matches a transport offset the kernel can parse. On non-ARPHRD_ETHER devices that parse depends on skb->protocol and skb_network_offset(). The old code never used either of them.
Quoted 4 lines
include/linux/virtio_net.h +0
-0
[ ... ]
Quoted 1 line
include/linux/virtio_net.h +0
-0@@ -48,15 +52,111 @@ static inline int virtio_net_hdr_set_proto(struct sk_buff *skb,
[ ... ]
Quoted 10 lines
include/linux/virtio_net.h +10
-0+static inline bool +virtio_net_csum_partial_ok (struct sk_buff *skb,u8 hdr_gso_type) +{ +unsigned int nhoff; +__be16 proto; + +if (skb->dev->type ==ARPHRD_ETHER ) { +if (!pskb_may_pull (skb,ETH_HLEN )) +return false; + nhoff =ETH_HLEN ;
include/linux/virtio_net.h static inline int virtio_net_hdr_set_proto(struct sk_buff *skb, · line 143
Quoted 10 lines
include/linux/virtio_net.h +10
-0+ proto =eth_hdr (skb)->h_proto; +if (eth_type_vlan (proto)) { + proto =__vlan_get_protocol (skb, proto, &nhoff); +if (!proto) +return false; + } + }else { + nhoff =skb_network_offset (skb); + proto = skb->protocol; + }
[Severity: Medium]
Can this reject valid NEEDS_CSUM GSO packets sent through AF_PACKET on devices that aren't ARPHRD_ETHER? Examples are loopback, tun in IFF_TUN mode, ipip/gre/sit and wireguard.
In packet_snd(), skb->protocol is only set when the socket protocol isn't ETH_P_ALL, so it stays 0 for protocol-0 sockets. The protocol is only derived from the headers after the vnet header has been converted:
net/packet/af_packet.c:packet_snd() {
...
if (proto != htons(ETH_P_ALL))
skb->protocol = proto;
...
if (vnet_hdr_sz) {
err = virtio_net_hdr_to_skb(skb, &vnet_hdr, vio_le());
if (err)
goto out_free;
len += vnet_hdr_sz;
}
packet_parse_headers(skb, sock);
if (vnet_hdr_sz)
virtio_net_hdr_set_proto(skb, &vnet_hdr);
...
}tpacket_snd() uses the same order.
When skb->protocol is 0, virtio_net_csum_partial_ok_at_offset() falls into its final "else return false", even for a well-formed IPv4/TCP packet. __virtio_net_hdr_to_skb() then returns -EINVAL for any GSO packet, so sendmsg() rejects TSO packets that it used to accept. Non-GSO packets always get a software checksum.
The non-NEEDS_CSUM GSO branch of __virtio_net_hdr_to_skb() already handles a zero protocol:
if (!skb->protocol) {
skb->protocol = dev_parse_header_protocol(skb);
if (!skb->protocol)
virtio_net_hdr_set_proto(skb, hdr);
}Should the new check have a similar fallback?
[Severity: Medium]
A related question: is skb->protocol a reliable source for devices that use Ethernet framing but aren't ARPHRD_ETHER? The main case is loopback, which is ARPHRD_LOOPBACK with hard_header_len == ETH_HLEN.
In packet_snd(), skb->protocol comes from the user's sll_protocol. loopback_xmit() then recomputes it from the frame bytes:
drivers/net/loopback.c:loopback_xmit() {
...
skb->protocol = eth_type_trans(skb, dev);
...
}A CAP_NET_RAW sender, possibly inside a user namespace, could set sll_protocol to ETH_P_IP while h_proto names a different encapsulation such as MPLS. This check would then accept CHECKSUM_PARTIAL against a fake IPv4/TCP header at offset 14. The receive path would parse a different layout, so csum_start could end up inside the real transport header.
The ARPHRD_ETHER branch avoids this by parsing h_proto itself. Nobody has shown this can be exploited, so it is speculative. Would parsing the frame be safer for Ethernet-framed devices too?
[ ... ]
include/linux/virtio_net.h static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb, · line 208
Quoted 9 lines
include/linux/virtio_net.h +3
-2@@ -104,11 +203,11 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,if (!skb_partial_csum_set (skb, start, off))return -EINVAL ; -if (skb_transport_offset (skb) < nh_min_len) -return -EINVAL ; +if (need_csum_check) + csum_partial_ok = +virtio_net_csum_partial_ok (skb, hdr_gso_type);
[Severity: Low]
This replaces the weak skb_transport_offset(skb) < nh_min_len check. Should the commit carry a Fixes: tag and Cc: stable?
Before this patch, csum_offset was only checked inside the GSO switch. A non-GSO NEEDS_CSUM packet from a tun/tap user, a guest or an AF_PACKET sender could therefore put the deferred checksum write almost anywhere past byte 20, including in IP header fields.
The only Fixes: tag in the series is on the later commit "net: account for ingress-resolved virtio net hdr state". That commit adds a counter and does not contain this fix.
[ ... ]
include/linux/virtio_net.h static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb, · line 291
Quoted 9 lines
include/linux/virtio_net.h +5
-0@@ -191,6 +284,17 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb, } } +if ((hdr->flags &VIRTIO_NET_HDR_F_NEEDS_CSUM ) && !csum_partial_ok) { +int err; + +if (skb_is_gso (skb)) +return -EINVAL ;
[Severity: Medium]
Does this rejection take effect for the UML vector driver? raw_verify_header() ignores the return value:
arch/um/drivers/vector_transports.c:raw_verify_header() {
...
skb_set_network_header(skb, ETH_HLEN);
virtio_net_hdr_to_skb(skb, vheader, virtio_legacy_is_little_endian());
return 0;
}By the time -EINVAL is returned here, skb_partial_csum_set() has already set CHECKSUM_PARTIAL at the offset from the header. The GSO block has also set gso_size and gso_type. This happens more often on UML because skb->len is still the untrimmed buffer length.
vector_legacy_rx() and vector_mmsg_rx() only drop the packet when verify_header() returns a negative value:
arch/um/drivers/vector_kern.c:vector_legacy_rx() {
...
if (header_check < 0) {
dev_kfree_skb_irq(skb);
...
}So the skb still reaches eth_type_trans() and napi_gro_receive() with CHECKSUM_PARTIAL at an arbitrary offset. The later patch in the series doesn't change this caller.
Should raw_verify_header() propagate the error?
include/linux/virtio_net.h static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb, · line 296
Quoted 5 lines
include/linux/virtio_net.h +5
-0+ + err =skb_checksum_help (skb); + skb->transport_header = old_transport; +return err; + }
[Severity: Medium]
Is skb->len the real packet length here for every caller? On UML, prep_skb() sizes the skb to the whole receive buffer:
arch/um/drivers/vector_kern.c:prep_skb() {
...
skb_put(result, vp->max_packet);
result->data_len = len - vp->max_packet;
result->len += len - vp->max_packet;
...
}and vector_legacy_rx() only trims the skb after verify_header():
arch/um/drivers/vector_kern.c:vector_legacy_rx() {
...
header_check = vp->verify_header(
vp->header_rxbuffer, skb, vp);
...
pskb_trim(skb, pkt_len - vp->rx_header_size);
...
}vector_mmsg_rx() has the same order.
Take a non-GSO NEEDS_CSUM frame that virtio_net_csum_partial_ok() doesn't recognize, for example tunneled traffic with an inner partial checksum. skb_checksum_help() sums skb->len - offset bytes, which includes stale bytes at the end of the buffer. It writes the folded result into the packet and sets CHECKSUM_NONE. The later pskb_trim() doesn't correct the checksum.
Before this patch these packets kept CHECKSUM_PARTIAL and were trimmed correctly. Could they now be dropped with a bad L4 checksum, or forwarded with a value computed from stale memory? The later patch in the series doesn't change arch/um.
[ ... ]
Quoted 9 lines
include/linux/virtio_net.h +1
-1@@ -334,7 +440,7 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,/* Let the basic parsing deal with plain GSO features. */ ret =__virtio_net_hdr_to_skb (skb, hdr, true, - hdr->gso_type & ~gso_tunnel_type); + hdr->gso_type & ~gso_tunnel_type, false);if (ret)return ret;
[ ... ]
include/linux/virtio_net.h virtio_net_hdr_tnl_to_skb(struct sk_buff *skb, · line 480
Quoted 9 lines
include/linux/virtio_net.h +4
-1@@ -368,7 +474,16 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb, skb->inner_mac_header = inner_nh +skb_headroom (skb); skb->transport_header = outer_th +skb_headroom (skb); skb->encapsulation =1 ; -return 0 ; + +if (virtio_net_csum_partial_ok_at_offset (skb, gso_inner_type, inner_nh, + inner_proto)) +return 0 ;
[Severity: High]
Is it enough to anchor the check at inner_nh? inner_nh is taken straight from vhdr->inner_nh_offset. Because need_csum_check is false in the call above, the Ethernet/VLAN/IP parse from the start of the frame is skipped. The outer offsets are only checked against each other:
include/linux/virtio_net.h:virtio_net_hdr_tnl_to_skb() {
...
if (outer_th < outer_l3min ||
inner_nh < outer_th + sizeof(struct udphdr) ||
inner_th < inner_nh + inner_l3min)
return -EINVAL;
...
}Nothing checks that the real outer IP header is followed by UDP at outer_th. Nothing checks that VLAN tags, IPv4 options or IPv6 extension headers don't move the real transport header either.
Two senders can control these offsets:
- a tun user with the tunnel vnet header, through tun_get_user() or
tun_xdp_one() via tun_vnet_hdr_tnl_to_skb()
- a host feeding a virtio_net guest with rx_tnl negotiated
Either one could put a fake IPv4/IPv6 plus TCP/UDP header at any inner_nh and keep CHECKSUM_PARTIAL there. That includes inside IPv4 options, inside IPv6 extension headers, or in the outer L4 payload.
For small packets gso_size is never set, so a non-GSO skb with CHECKSUM_PARTIAL and encapsulation set is accepted. A later skb_checksum_help() in validate_xmit_skb(), or NIC offload after forwarding, would then write 2 bytes at a place that need not match the headers the stack parsed. That could be the real outer L4 port.
Doesn't this leave open the case the commit message describes ("later checksum completion can modify the packet after protocol processing has made decisions from it")? It also seems to fall short of "UDP tunnel headers get the same handling on the inner header".
Would the outer headers need to be parsed from the start of the frame as well?
include/linux/virtio_net.h virtio_net_hdr_tnl_to_skb(struct sk_buff *skb, · line 486
Quoted 6 lines
include/linux/virtio_net.h +6
-0+ +if (skb_is_gso (skb)) +return -EINVAL ; + + ret =skb_checksum_help (skb); +return ret ?:1 ;
[Severity: Medium]
What do callers do with the 1 returned here after skb_checksum_help() succeeds? At this revision every caller treats any non-zero value as an error. For example, in virtnet_receive_done():
drivers/net/virtio_net.c:virtnet_receive_done() {
...
if (virtio_net_hdr_tnl_to_skb(skb, &hdr->tnl_hdr, vi->rx_tnl,
vi->rx_tnl_csum,
virtio_is_little_endian(vi->vdev))) {
...
goto frame_err;
}
...
}tun_get_user() and tun_xdp_one() do the same through tun_vnet_hdr_tnl_to_skb():
drivers/net/tun.c:tun_get_user() {
...
if (tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, &hdr)) {
atomic_long_inc(&tun->rx_frame_errors);
err = -EINVAL;
goto free_skb;
}
...
}As a result, non-GSO tunnel packets whose inner checksum metadata isn't recognized are dropped and counted as frame errors instead of being accepted.
The later commit "net: account for ingress-resolved virtio net hdr state" in this series changes these callers to test for < 0 and documents the return value of 1. Could that change be folded into this patch so the series stays bisectable?
Signature · 1 line
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1791887415.git.mst%40redhat.com
Michael S. Tsirkin
maintainerPATCH 4/4 net: account for ingress-resolved virtio net hdr state
In reply to Michael S. TsirkinIngress resolution of uncommon virtio net hdr checksum metadata (software checksum completion at ingress) adds performance overhead but is currently invisible: no counter tracks it, so a hypothetical workload relying on uncommon metadata cannot be distinguished from none at all, and packets quietly pay the host-CPU checksum overhead instead of offload.
Have virtio_net_hdr_to_skb() return 1 when it completes an uncommon checksum in software, and flip every converter call site to test for negative errno. Have tun count these events per device as rx_csum_sw and expose the counter via ethtool -S.
--- drivers/net/tap.c | 4 ++-- drivers/net/tun.c | 44 ++++++++++++++++++++++++++++++++++++-- drivers/net/virtio_net.c | 2 +- include/linux/virtio_net.h | 12 +++++++++-- net/packet/af_packet.c | 4 ++-- 5 files changed, 57 insertions(+), 9 deletions(-)
drivers/net/tap.c +2 -2
@@ -649,7 +649,7 @@ static ssize_t tap_get_user(struct tap_queue *q, void *msg_control,if (vnet_hdr_len) { err =tun_vnet_hdr_to_skb (q->flags, skb, &vnet_hdr); -if (err) { +if (err <0 ) {rcu_read_unlock (); drop_reason =SKB_DROP_REASON_DEV_HDR ; goto err_kfree;
@@ -1084,7 +1084,7 @@ static int tap_get_user_xdp(struct tap_queue *q, struct xdp_buff *xdp)if (vnet_hdr_len) { err =tun_vnet_hdr_to_skb (q->flags, skb, gso); -if (err) { +if (err <0 ) {rcu_read_unlock (); goto err_kfree; }
drivers/net/tun.c +42 -2
78 lines not on this page · raw message
drivers/net/virtio_net.c +1 -1
@@ -2519,7 +2519,7 @@ static void virtnet_receive_done(struct virtnet_info *vi, struct receive_queue *skb_set_network_header (skb,ETH_HLEN );if (virtio_net_hdr_tnl_to_skb (skb, &hdr->tnl_hdr, vi->rx_tnl, vi->rx_tnl_csum, -virtio_is_little_endian (vi->vdev))) { +virtio_is_little_endian (vi->vdev)) <0 ) { net_warn_ratelimited("%s: bad gso: type: %x, size: %u, flags %x tunnel %d tnl csum %d\n", dev->name, hdr->hdr.gso_type, hdr->hdr.gso_size, hdr->hdr.flags,
include/linux/virtio_net.h +10 -2
@@ -14,6 +14,14 @@#include <uapi/linux/tcp.h> #include <uapi/linux/virtio_net.h> +/* +* virtio_net_hdr_to_skb() and virtio_net_hdr_tnl_to_skb() return 0 on +* success and a negative errno on failure. On success they may also +* return 1, indicating that an uncommon checksum was completed in +* software at ingress rather than deferred to the egress path; +* drivers can use this to account for such packets. +*/ +static inline bool virtio_net_hdr_match_proto (__be16 protocol,__u8 gso_type) {switch (gso_type & ~VIRTIO_NET_HDR_GSO_ECN ) {
@@ -292,7 +300,7 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb, err =skb_checksum_help (skb); skb->transport_header = old_transport; -return err; +return err ?:1 ; }return 0 ;
@@ -441,7 +449,7 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,/* Let the basic parsing deal with plain GSO features. */ ret =__virtio_net_hdr_to_skb (skb, hdr, true, hdr->gso_type & ~gso_tunnel_type, false); -if (ret) +if (ret <0 )return ret;/* In case of USO, the inner protocol is still unknown and
net/packet/af_packet.c +2 -2
@@ -2913,7 +2913,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg) }if (has_vnet_hdr) { -if (virtio_net_hdr_to_skb (skb, &vnet_hdr,vio_le ())) { +if (virtio_net_hdr_to_skb (skb, &vnet_hdr,vio_le ()) <0 ) { tp_len = -EINVAL ; goto tpacket_error; }
@@ -3136,7 +3136,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)if (vnet_hdr_sz) { err =virtio_net_hdr_to_skb (skb, &vnet_hdr,vio_le ()); -if (err) +if (err <0 ) goto out_free; len += vnet_hdr_sz; }
Signature · 1 line
MST
[email protected]
AI reviewRe: [PATCH net-next 4/4] net: account for ingress-resolved virtio net hdr state
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 6 lines
Ingress resolution of uncommon virtio net hdr checksum metadata (software checksum completion at ingress) adds performance overhead but is currently invisible: no counter tracks it, so a hypothetical workload relying on uncommon metadata cannot be distinguished from none at all, and packets quietly pay the host-CPU checksum overhead instead of offload.
Quoted 6 lines
Have
virtio_net_hdr_to_skb()return 1 when it completes an uncommon checksum in software, and flip every converter call site to test for negative errno. Have tun count these events per device asrx_csum_swand expose the counter via ethtool -S.Signed-off-by Michael S. Tsirkin <[email protected]>
Sashiko has reviewed this patch and found no issues. It looks great!
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
Re: [PATCH net-next 4/4] net: account for ingress-resolved virtio net hdr state
In reply to Michael S. TsirkinQuoted 186 lines · Michael S. Tsirkin
Ingress resolution of uncommon virtio net hdr checksum metadata (software checksum completion at ingress) adds performance overhead but is currently invisible: no counter tracks it, so a hypothetical workload relying on uncommon metadata cannot be distinguished from none at all, and packets quietly pay the host-CPU checksum overhead instead of offload.
Have
virtio_net_hdr_to_skb()return 1 when it completes an uncommon checksum in software, and flip every converter call site to test for negative errno. Have tun count these events per device asrx_csum_swand expose the counter via ethtool -S.Signed-off-by Michael S. Tsirkin <[email protected]>Assisted-by LLMFixes f43798c27684 ("tun: Allow GSO using virtio_net_hdr")--- drivers/net/tap.c | 4 ++-- drivers/net/tun.c | 44 ++++++++++++++++++++++++++++++++++++-- drivers/net/virtio_net.c | 2 +- include/linux/virtio_net.h | 12 +++++++++-- net/packet/af_packet.c | 4 ++-- 5 files changed, 57 insertions(+), 9 deletions(-)drivers/net/tap.c +2
-2@@ -649,7 +649,7 @@ static ssize_t tap_get_user(struct tap_queue *q, void *msg_control,if (vnet_hdr_len) { err =tun_vnet_hdr_to_skb (q->flags, skb, &vnet_hdr); -if (err) { +if (err <0 ) {rcu_read_unlock (); drop_reason =SKB_DROP_REASON_DEV_HDR ; goto err_kfree;@@ -1084,7 +1084,7 @@ static int tap_get_user_xdp(struct tap_queue *q, struct xdp_buff *xdp)if (vnet_hdr_len) { err =tun_vnet_hdr_to_skb (q->flags, skb, gso); -if (err) { +if (err <0 ) {rcu_read_unlock (); goto err_kfree; }drivers/net/tun.c +42
-2@@ -210,6 +210,7 @@ struct tun_struct {u32 flow_count;u32 rx_batched;atomic_long_t rx_frame_errors; +atomic_long_t rx_csum_sw;struct bpf_prog __rcu *xdp_prog;struct tun_prog __rcu *steering_prog;struct tun_prog __rcu *filter_prog;@@ -1935,11 +1936,19 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,break ; } -if (tun_vnet_hdr_tnl_to_skb (tun->flags, features, skb, &hdr)) { + err =tun_vnet_hdr_tnl_to_skb (tun->flags, features, skb, &hdr); +if (err <0 ) {atomic_long_inc (&tun->rx_frame_errors); err = -EINVAL ; goto free_skb; } +if (err >0 ) +atomic_long_inc (&tun->rx_csum_sw); +/* The status is consumed: keep err clear so later exit paths +* (XDP drop via unlock_frags) do not return it to write() as +* a short count. +*/ + err =0 ;if ((tun->flags &TUN_TYPE_MASK ) ==IFF_TAP ) skb->protocol =eth_type_trans (skb, tun->dev);@@ -2605,12 +2614,15 @@ static int tun_xdp_one(struct tun_struct *tun, tnl_hdr = (struct virtio_net_hdr_v1_hash_tunnel *)gso; skb->dev = tun->dev;skb_set_network_header (skb,ETH_HLEN ); -if (tun_vnet_hdr_tnl_to_skb (tun->flags, features, skb, tnl_hdr)) { + ret =tun_vnet_hdr_tnl_to_skb (tun->flags, features, skb, tnl_hdr); +if (ret <0 ) {atomic_long_inc (&tun->rx_frame_errors);kfree_skb (skb); ret = -EINVAL ; goto out; } +if (ret >0 ) +atomic_long_inc (&tun->rx_csum_sw); skb->protocol =eth_type_trans (skb, tun->dev);skb_reset_network_header (skb);@@ -3739,7 +3751,35 @@ static void tun_get_channels(struct net_device *dev, channels->max_combined = tun->flags &IFF_MULTI_QUEUE ?MAX_TAP_QUEUES :1 ; } +#define TUN_STATS_LEN 1 +static const char tun_ethtool_strings[TUN_STATS_LEN ][ETH_GSTRING_LEN ] = { +"rx_csum_sw" , +}; + +static int tun_get_sset_count (struct net_device *dev,int sset) +{ +return sset ==ETH_SS_STATS ?TUN_STATS_LEN : -EOPNOTSUPP ; +} + +static void tun_get_strings (struct net_device *dev,u32 stringset,u8 *data) +{ +if (stringset ==ETH_SS_STATS ) +memcpy (data, tun_ethtool_strings, +sizeof (tun_ethtool_strings)); +} + +static void tun_get_ethtool_stats (struct net_device *dev, +struct ethtool_stats *stats,u64 *data) +{ +struct tun_struct *tun =netdev_priv (dev); + + data[0 ] =atomic_long_read (&tun->rx_csum_sw); +} +static const struct ethtool_ops tun_ethtool_ops = { + .get_sset_count = tun_get_sset_count, + .get_strings = tun_get_strings, + .get_ethtool_stats = tun_get_ethtool_stats, .supported_coalesce_params =ETHTOOL_COALESCE_RX_MAX_FRAMES , .get_drvinfo = tun_get_drvinfo, .get_msglevel = tun_get_msglevel,drivers/net/virtio_net.c +1
-1@@ -2519,7 +2519,7 @@ static void virtnet_receive_done(struct virtnet_info *vi, struct receive_queue *skb_set_network_header (skb,ETH_HLEN );if (virtio_net_hdr_tnl_to_skb (skb, &hdr->tnl_hdr, vi->rx_tnl, vi->rx_tnl_csum, -virtio_is_little_endian (vi->vdev))) { +virtio_is_little_endian (vi->vdev)) <0 ) { net_warn_ratelimited("%s: bad gso: type: %x, size: %u, flags %x tunnel %d tnl csum %d\n", dev->name, hdr->hdr.gso_type, hdr->hdr.gso_size, hdr->hdr.flags,include/linux/virtio_net.h +10
-2@@ -14,6 +14,14 @@#include <uapi/linux/tcp.h> #include <uapi/linux/virtio_net.h> +/* +* virtio_net_hdr_to_skb() and virtio_net_hdr_tnl_to_skb() return 0 on +* success and a negative errno on failure. On success they may also +* return 1, indicating that an uncommon checksum was completed in +* software at ingress rather than deferred to the egress path; +* drivers can use this to account for such packets. +*/ +static inline bool virtio_net_hdr_match_proto (__be16 protocol,__u8 gso_type) {switch (gso_type & ~VIRTIO_NET_HDR_GSO_ECN ) {@@ -292,7 +300,7 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb, err =skb_checksum_help (skb); skb->transport_header = old_transport; -return err; +return err ?:1 ; }return 0 ;@@ -441,7 +449,7 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,/* Let the basic parsing deal with plain GSO features. */ ret =__virtio_net_hdr_to_skb (skb, hdr, true, hdr->gso_type & ~gso_tunnel_type, false); -if (ret) +if (ret <0 )return ret;/* In case of USO, the inner protocol is still unknown and net/packet/af_packet.c +0
-0
net/packet/af_packet.c static int tpacket_snd(struct packet_sock *po, struct msghdr *msg) · line 2916
Quoted 5 lines
net/packet/af_packet.c +1
-1@@ -2913,7 +2913,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg) }if (has_vnet_hdr) { -if (virtio_net_hdr_to_skb (skb, &vnet_hdr,vio_le ())) { +if (virtio_net_hdr_to_skb (skb, &vnet_hdr,vio_le ()) <0 ) {
nit: I don't know if we care, but this sort of breaks bisectability. Previous patch does return ret ?: 1, but it's this patch that does the if-statement fixup. (Same for tun/tap... ^^^)
Björn
Michael S. Tsirkin
maintainerRe: [PATCH net-next 4/4] net: account for ingress-resolved virtio net hdr state
Quoted 190 lines · Björn Töpel
Quoted 188 lines · Michael S. Tsirkin
Ingress resolution of uncommon virtio net hdr checksum metadata (software checksum completion at ingress) adds performance overhead but is currently invisible: no counter tracks it, so a hypothetical workload relying on uncommon metadata cannot be distinguished from none at all, and packets quietly pay the host-CPU checksum overhead instead of offload.
Have
virtio_net_hdr_to_skb()return 1 when it completes an uncommon checksum in software, and flip every converter call site to test for negative errno. Have tun count these events per device asrx_csum_swand expose the counter via ethtool -S.Signed-off-by Michael S. Tsirkin <[email protected]>Assisted-by LLMFixes f43798c27684 ("tun: Allow GSO using virtio_net_hdr")--- drivers/net/tap.c | 4 ++-- drivers/net/tun.c | 44 ++++++++++++++++++++++++++++++++++++-- drivers/net/virtio_net.c | 2 +- include/linux/virtio_net.h | 12 +++++++++-- net/packet/af_packet.c | 4 ++-- 5 files changed, 57 insertions(+), 9 deletions(-)drivers/net/tap.c +2
-2@@ -649,7 +649,7 @@ static ssize_t tap_get_user(struct tap_queue *q, void *msg_control,if (vnet_hdr_len) { err =tun_vnet_hdr_to_skb (q->flags, skb, &vnet_hdr); -if (err) { +if (err <0 ) {rcu_read_unlock (); drop_reason =SKB_DROP_REASON_DEV_HDR ; goto err_kfree;@@ -1084,7 +1084,7 @@ static int tap_get_user_xdp(struct tap_queue *q, struct xdp_buff *xdp)if (vnet_hdr_len) { err =tun_vnet_hdr_to_skb (q->flags, skb, gso); -if (err) { +if (err <0 ) {rcu_read_unlock (); goto err_kfree; }drivers/net/tun.c +42
-2@@ -210,6 +210,7 @@ struct tun_struct {u32 flow_count;u32 rx_batched;atomic_long_t rx_frame_errors; +atomic_long_t rx_csum_sw;struct bpf_prog __rcu *xdp_prog;struct tun_prog __rcu *steering_prog;struct tun_prog __rcu *filter_prog;@@ -1935,11 +1936,19 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,break ; } -if (tun_vnet_hdr_tnl_to_skb (tun->flags, features, skb, &hdr)) { + err =tun_vnet_hdr_tnl_to_skb (tun->flags, features, skb, &hdr); +if (err <0 ) {atomic_long_inc (&tun->rx_frame_errors); err = -EINVAL ; goto free_skb; } +if (err >0 ) +atomic_long_inc (&tun->rx_csum_sw); +/* The status is consumed: keep err clear so later exit paths +* (XDP drop via unlock_frags) do not return it to write() as +* a short count. +*/ + err =0 ;if ((tun->flags &TUN_TYPE_MASK ) ==IFF_TAP ) skb->protocol =eth_type_trans (skb, tun->dev);@@ -2605,12 +2614,15 @@ static int tun_xdp_one(struct tun_struct *tun, tnl_hdr = (struct virtio_net_hdr_v1_hash_tunnel *)gso; skb->dev = tun->dev;skb_set_network_header (skb,ETH_HLEN ); -if (tun_vnet_hdr_tnl_to_skb (tun->flags, features, skb, tnl_hdr)) { + ret =tun_vnet_hdr_tnl_to_skb (tun->flags, features, skb, tnl_hdr); +if (ret <0 ) {atomic_long_inc (&tun->rx_frame_errors);kfree_skb (skb); ret = -EINVAL ; goto out; } +if (ret >0 ) +atomic_long_inc (&tun->rx_csum_sw); skb->protocol =eth_type_trans (skb, tun->dev);skb_reset_network_header (skb);@@ -3739,7 +3751,35 @@ static void tun_get_channels(struct net_device *dev, channels->max_combined = tun->flags &IFF_MULTI_QUEUE ?MAX_TAP_QUEUES :1 ; } +#define TUN_STATS_LEN 1 +static const char tun_ethtool_strings[TUN_STATS_LEN ][ETH_GSTRING_LEN ] = { +"rx_csum_sw" , +}; + +static int tun_get_sset_count (struct net_device *dev,int sset) +{ +return sset ==ETH_SS_STATS ?TUN_STATS_LEN : -EOPNOTSUPP ; +} + +static void tun_get_strings (struct net_device *dev,u32 stringset,u8 *data) +{ +if (stringset ==ETH_SS_STATS ) +memcpy (data, tun_ethtool_strings, +sizeof (tun_ethtool_strings)); +} + +static void tun_get_ethtool_stats (struct net_device *dev, +struct ethtool_stats *stats,u64 *data) +{ +struct tun_struct *tun =netdev_priv (dev); + + data[0 ] =atomic_long_read (&tun->rx_csum_sw); +} +static const struct ethtool_ops tun_ethtool_ops = { + .get_sset_count = tun_get_sset_count, + .get_strings = tun_get_strings, + .get_ethtool_stats = tun_get_ethtool_stats, .supported_coalesce_params =ETHTOOL_COALESCE_RX_MAX_FRAMES , .get_drvinfo = tun_get_drvinfo, .get_msglevel = tun_get_msglevel,drivers/net/virtio_net.c +1
-1@@ -2519,7 +2519,7 @@ static void virtnet_receive_done(struct virtnet_info *vi, struct receive_queue *skb_set_network_header (skb,ETH_HLEN );if (virtio_net_hdr_tnl_to_skb (skb, &hdr->tnl_hdr, vi->rx_tnl, vi->rx_tnl_csum, -virtio_is_little_endian (vi->vdev))) { +virtio_is_little_endian (vi->vdev)) <0 ) { net_warn_ratelimited("%s: bad gso: type: %x, size: %u, flags %x tunnel %d tnl csum %d\n", dev->name, hdr->hdr.gso_type, hdr->hdr.gso_size, hdr->hdr.flags,include/linux/virtio_net.h +10
-2@@ -14,6 +14,14 @@#include <uapi/linux/tcp.h> #include <uapi/linux/virtio_net.h> +/* +* virtio_net_hdr_to_skb() and virtio_net_hdr_tnl_to_skb() return 0 on +* success and a negative errno on failure. On success they may also +* return 1, indicating that an uncommon checksum was completed in +* software at ingress rather than deferred to the egress path; +* drivers can use this to account for such packets. +*/ +static inline bool virtio_net_hdr_match_proto (__be16 protocol,__u8 gso_type) {switch (gso_type & ~VIRTIO_NET_HDR_GSO_ECN ) {@@ -292,7 +300,7 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb, err =skb_checksum_help (skb); skb->transport_header = old_transport; -return err; +return err ?:1 ; }return 0 ;@@ -441,7 +449,7 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,/* Let the basic parsing deal with plain GSO features. */ ret =__virtio_net_hdr_to_skb (skb, hdr, true, hdr->gso_type & ~gso_tunnel_type, false); -if (ret) +if (ret <0 )return ret;/* In case of USO, the inner protocol is still unknown and net/packet/af_packet.c +0
-0@@ -2913,7 +2913,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg) }
Quoted 8 lines
net/packet/af_packet.c static int tpacket_snd(struct packet_sock *po, struct msghdr *msg) · line 2916
Quoted 3 lines
net/packet/af_packet.c +1
-1if (has_vnet_hdr) { -if (virtio_net_hdr_to_skb (skb, &vnet_hdr,vio_le ())) { +if (virtio_net_hdr_to_skb (skb, &vnet_hdr,vio_le ()) <0 ) {nit: I don't know if we care, but this sort of breaks bisectability. Previous patch does return ret ?: 1, but it's this patch that does the if-statement fixup. (Same for tun/tap... ^^^)
Oh ugh. You are right, I kept rebasing to tweak things and moved it by chance. I'll fix it up in v2.
Quoted 1 line
Björn
[email protected]
AI reviewRe: [PATCH net-next 4/4] net: account for ingress-resolved virtio net hdr state
In reply to Michael S. TsirkinThank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider.
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider.
Critical: 0 · High: 0 · Medium: 2 · Low: 1
If you believe none of the issues are legitimate, please reply to each issue of Medium severity or higher and explain why the AI is wrong. If any of them are legitimate and you plan to respin, please let patchwork know by including "pw-bot: cr" as a separate line at the end of your reply (one such reply per series is enough).
- [Medium] This patch silently fixes a packet-drop regression introduced by the previous commit in the series, "net: resolve uncommon partial…
- [Medium] tun_get_ethtool_stats() assigns data[0] = atomic_long_read(&tun->rx_csum_sw); straight into the u64 ethtool…
- [Low] The trailer Fixes: f43798c27684 ("tun: Allow GSO using virtio_net_hdr") names a July 2008 commit by Rusty Russell (verified…
--
net: account for ingress-resolved virtio net hdr state
After this commit, virtio_net_hdr_to_skb() returns 1 when it completes an uncommon checksum in software at ingress. The converter call sites in tap, tun, virtio_net and af_packet now test for a negative errno. tun counts these events per device as rx_csum_sw and exposes the counter through ethtool -S.
Quoted 1 line
Fixes f43798c27684 ("tun: Allow GSO using virtio_net_hdr")
[Severity: Low]
Is this the right Fixes: target? f43798c27684 is the 2008 commit that added GSO via virtio_net_hdr to tun.
Software checksum completion in the converter first appears in the previous patch in this series, "net: resolve uncommon partial checksums at ingress". So does the positive return value this patch accounts for. At the baseline there is no skb_checksum_help() call in include/linux/virtio_net.h.
Stable tooling that follows this tag could offer the patch to every stable branch since 2.6.27. On those branches the "return err ?: 1" hunk has nothing to apply to, and rx_csum_sw would always read 0.
Should the tag be dropped? If it is meant to cover the call site fix below, should it point at the previous commit in this series instead?
Quoted 4 lines
drivers/net/tun.c +0
-0
[ ... ]
drivers/net/tun.c static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile, · line 1944
Quoted 11 lines
drivers/net/tun.c +2
-1@@ -1935,11 +1936,19 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,break ; } -if (tun_vnet_hdr_tnl_to_skb (tun->flags, features, skb, &hdr)) { + err =tun_vnet_hdr_tnl_to_skb (tun->flags, features, skb, &hdr); +if (err <0 ) {atomic_long_inc (&tun->rx_frame_errors); err = -EINVAL ; goto free_skb; }
[Severity: Medium]
Does this change also fix a regression from the previous patch in the series, "net: resolve uncommon partial checksums at ingress"?
That commit already made virtio_net_hdr_tnl_to_skb() return 1 on the UDP tunnel path after a successful software checksum:
include/linux/virtio_net.h:virtio_net_hdr_tnl_to_skb() {
...
ret = skb_checksum_help(skb);
return ret ?: 1;
}At that commit, the tunnel converter callers still treated any nonzero return as a failure:
tun_get_user():
if (tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, &hdr)) {
atomic_long_inc(&tun->rx_frame_errors);
err = -EINVAL;
goto free_skb;
}tun_xdp_one() had the same pattern. virtnet_receive_done() went to frame_err and printed a "bad gso" warning.
Take a UDP tunnel header with these properties:
- NEEDS_CSUM is set.
- The payload is no larger than gso_size, so skb_is_gso() is false.
- The inner csum offsets fail virtio_net_csum_partial_ok_at_offset().
With only the previous commit applied, such a packet would have its checksum completed and then be dropped as a frame error. tun write() would return -EINVAL for it.
This commit message describes the "< 0" changes only as accounting plumbing. To keep the series bisectable, could the call site changes be folded into "net: resolve uncommon partial checksums at ingress"? Alternatively, could this commit message say that it fixes that commit?
Quoted 2 lines
drivers/net/tun.c +2
-0+if (err >0 ) +atomic_long_inc (&tun->rx_csum_sw);
[ ... ]
Quoted 1 line
drivers/net/tun.c +0
-0@@ -3739,7 +3751,35 @@ static void tun_get_channels(struct net_device *dev,
[ ... ]
drivers/net/tun.c static void tun_get_channels(struct net_device *dev, · line 3776
Quoted 6 lines
drivers/net/tun.c +6
-0+static void tun_get_ethtool_stats (struct net_device *dev, +struct ethtool_stats *stats,u64 *data) +{ +struct tun_struct *tun =netdev_priv (dev); + + data[0 ] =atomic_long_read (&tun->rx_csum_sw);
[Severity: Medium]
On 32-bit builds, atomic_long_t is an atomic_t and atomic_long_read() returns a signed long. Once rx_csum_sw passes 2^31, can the implicit conversion to u64 sign-extend the value? ethtool -S would then report something like 0xffffffff80000000.
tun_net_get_stats64() in the same file avoids this for the sibling counter:
stats->rx_frame_errors += (unsigned long)atomic_long_read(&tun->rx_frame_errors);
Should this read use the same (unsigned long) cast?
Quoted 1 line
+}
[ ... ]
Signature · 1 line
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1791887415.git.mst%40redhat.com
Michael S. Tsirkin
maintainerQuoted 13 lines · Michael S. Tsirkin
Allowing untrusted hardware to specify csum start/offset for
CHECKSUM_PARTIALwas not a good idea, because, if it edits the packet outside the actual checksum, and depending on the setup, the networking stack could be looking at the packet both before and after the edit, and seeing different results.Previously, commit
6785011f8b16("ipv4: validatechecksum_startbefore completing checksum") validated all known instances where this causes the most grief, but the real fix is to make sure that either only actual checksum fields are ever edited,
or the checksum is completed before the net stack looks at it, so it is always consistent.
Quoted 4 lines
This basically does this: checksum at ingress, except in a narrow list of specified known good cases.
Fixes f43798c27684 ("tun: Allow GSO using virtio_net_hdr")
I forgot.
Quoted 20 lines
Michael S. Tsirkin (4):
net: addip4_parse()
net: useip4_parse()in receive validation
net: resolve uncommon partial checksums at ingress
net: account for ingress-resolved virtio net hdr statedrivers/net/tap.c | 4 +- drivers/net/tun.c | 44 +++++++- drivers/net/virtio_net.c | 2 +- include/linux/ip.h | 24 +++++ include/linux/virtio_net.h | 161 ++++++++++++++++++++++++---- net/bridge/br_netfilter_hooks.c | 15 +-- net/ipv4/ip_input.c | 13 +-- net/ipv4/netfilter/nf_reject_ipv4.c | 13 +-- net/packet/af_packet.c | 4 +- 9 files changed, 228 insertions(+), 52 deletions(-)Signature · 1 line
MST
Chris Arges
Tested-byQuoted 17 lines · Michael S. Tsirkin
Quoted 16 lines · Michael S. Tsirkin
Allowing untrusted hardware to specify csum start/offset for
CHECKSUM_PARTIALwas not a good idea, because, if it edits the packet outside the actual checksum, and depending on the setup, the networking stack could be looking at the packet both before and after the edit, and seeing different results.Previously, commit
6785011f8b16("ipv4: validatechecksum_startbefore completing checksum") validated all known instances where this causes the most grief, but the real fix is to make sure that either only actual checksum fields are ever edited,
or the checksum is completed before the net stack looks at it, so it is always consistent.This basically does this: checksum at ingress, except in a narrow list of specified known good cases.
Quoted 9 lines
Quoted 1 line
Fixes f43798c27684 ("tun: Allow GSO using virtio_net_hdr")I forgot.
Reported-by Paulos Yibelo <[email protected]>Reported-by Chris Arges <[email protected]>
I did some testing of this patch-set and it mitigates my cases in VM testing.
Quoted 21 lines
Quoted 20 lines
Michael S. Tsirkin (4):
net: addip4_parse()
net: useip4_parse()in receive validation
net: resolve uncommon partial checksums at ingress
net: account for ingress-resolved virtio net hdr statedrivers/net/tap.c | 4 +- drivers/net/tun.c | 44 +++++++- drivers/net/virtio_net.c | 2 +- include/linux/ip.h | 24 +++++ include/linux/virtio_net.h | 161 ++++++++++++++++++++++++---- net/bridge/br_netfilter_hooks.c | 15 +-- net/ipv4/ip_input.c | 13 +-- net/ipv4/netfilter/nf_reject_ipv4.c | 13 +-- net/packet/af_packet.c | 4 +- 9 files changed, 228 insertions(+), 52 deletions(-)Signature · 1 line
MST
Willem de Bruijn
maintainerQuoted 7 lines · Michael S. Tsirkin
Allowing untrusted hardware to specify csum start/offset for
CHECKSUM_PARTIALwas not a good idea, because, if it edits the packet outside the actual checksum, and depending on the setup, the networking stack could be looking at the packet both before and after the edit, and seeing different results.
Quoted 6 lines
Previously, commit
6785011f8b16("ipv4: validatechecksum_startbefore completing checksum") validated all known instances where this causes the most grief, but the real fix is to make sure that either only actual checksum fields are ever edited,
or the checksum is completed before the net stack looks at it, so it is always consistent.
+1 on the general approach of performing the checksum offload at input, unless the input can easily be validated to be one of a few common protocols. That greatly reduces the risk vs my previous attempt at trying to drop.
Since we do not know the full extent of grief, I think this too should go to net and stable. That would mean initially a single patch that touches as few files as possible. The helper function in include/linux/ip.h and conversion of existing open coded parsers to use that can follow up later in net-next.
Quoted 10 lines
This basically does this: checksum at ingress, except in a narrow list of specified known good cases.
Fixes f43798c27684 ("tun: Allow GSO using virtio_net_hdr")Michael S. Tsirkin (4):
net: addip4_parse()
net: useip4_parse()in receive validation
net: resolve uncommon partial checksums at ingress
net: account for ingress-resolved virtio net hdr state
Michael S. Tsirkin
maintainerQuoted 15 lines · Willem de Bruijn
Quoted 7 lines · Michael S. Tsirkin
Allowing untrusted hardware to specify csum start/offset for
CHECKSUM_PARTIALwas not a good idea, because, if it edits the packet outside the actual checksum, and depending on the setup, the networking stack could be looking at the packet both before and after the edit, and seeing different results.Quoted 6 lines
Previously, commit
6785011f8b16("ipv4: validatechecksum_startbefore completing checksum") validated all known instances where this causes the most grief, but the real fix is to make sure that either only actual checksum fields are ever edited,
or the checksum is completed before the net stack looks at it, so it is always consistent.
Quoted 7 lines
+1 on the general approach of performing the checksum offload at input, unless the input can easily be validated to be one of a few common protocols. That greatly reduces the risk vs my previous attempt at trying to drop.
Since we do not know the full extent of grief, I think this too should go to net and stable.
I think it needs a bit more testing to be frank, I do not see it wait months out of the tree. So if this lands in next before the merge window, we are good I think. If we miss that I will target net.
Quoted 4 lines
That would mean initially a single patch that
touches as few files as possible. The helper function ininclude/linux/ip.hand conversion of existing open coded parsers to use that can follow up later in net-next.
I don't see the value to be frank.
What moving code between files accomplishes, I donnu. Squashing things surely makes review harder? Does your backporting system really struggle with trivial dependencies like this?
I can drop patch 2 and will if I am targeting net.
Quoted 11 lines
Quoted 11 lines
This basically does this: checksum at ingress, except in a narrow list of specified known good cases.
Fixes f43798c27684 ("tun: Allow GSO using virtio_net_hdr")Michael S. Tsirkin (4):
net: addip4_parse()
net: useip4_parse()in receive validation
net: resolve uncommon partial checksums at ingress
net: account for ingress-resolved virtio net hdr state
Willem de Bruijn
maintainerQuoted 35 lines · Michael S. Tsirkin
Quoted 15 lines · Willem de Bruijn
Quoted 7 lines · Michael S. Tsirkin
Allowing untrusted hardware to specify csum start/offset for
CHECKSUM_PARTIALwas not a good idea, because, if it edits the packet outside the actual checksum, and depending on the setup, the networking stack could be looking at the packet both before and after the edit, and seeing different results.Quoted 6 lines
Previously, commit
6785011f8b16("ipv4: validatechecksum_startbefore completing checksum") validated all known instances where this causes the most grief, but the real fix is to make sure that either only actual checksum fields are ever edited,
or the checksum is completed before the net stack looks at it, so it is always consistent.Quoted 7 lines
+1 on the general approach of performing the checksum offload at input, unless the input can easily be validated to be one of a few common protocols. That greatly reduces the risk vs my previous attempt at trying to drop.
Since we do not know the full extent of grief, I think this too should go to net and stable.
I think it needs a bit more testing to be frank, I do not see it wait months out of the tree. So if this lands in next before the merge window, we are good I think. If we miss that I will target net.
Quoted 4 lines
That would mean initially a single patch that
touches as few files as possible. The helper function ininclude/linux/ip.hand conversion of existing open coded parsers to use that can follow up later in net-next.
Quoted 7 lines
I don't see the value to be frank.
What moving code between files accomplishes, I donnu. Squashing things surely makes review harder? Does your backporting system really struggle with trivial dependencies like this?I can drop patch 2 and will if I am targeting net.
Good. This is not about "my" backporting, but upstream stable. Refactor patches do not belong there.
Since patch 3 might have to be backported in the future if a serious abuse surfaces, even if you now land it in net-next, it is better to not depend on other patches.
Quoted 12 lines
Quoted 11 lines
Quoted 11 lines
This basically does this: checksum at ingress, except in a narrow list of specified known good cases.
Fixes f43798c27684 ("tun: Allow GSO using virtio_net_hdr")Michael S. Tsirkin (4):
net: addip4_parse()
net: useip4_parse()in receive validation
net: resolve uncommon partial checksums at ingress
net: account for ingress-resolved virtio net hdr state
Michael S. Tsirkin
maintainerQuoted 42 lines · Willem de Bruijn
Quoted 41 lines · Michael S. Tsirkin
Quoted 15 lines · Willem de Bruijn
Quoted 7 lines · Michael S. Tsirkin
Allowing untrusted hardware to specify csum start/offset for
CHECKSUM_PARTIALwas not a good idea, because, if it edits the packet outside the actual checksum, and depending on the setup, the networking stack could be looking at the packet both before and after the edit, and seeing different results.Quoted 6 lines
Previously, commit
6785011f8b16("ipv4: validatechecksum_startbefore completing checksum") validated all known instances where this causes the most grief, but the real fix is to make sure that either only actual checksum fields are ever edited,
or the checksum is completed before the net stack looks at it, so it is always consistent.Quoted 7 lines
+1 on the general approach of performing the checksum offload at input, unless the input can easily be validated to be one of a few common protocols. That greatly reduces the risk vs my previous attempt at trying to drop.
Since we do not know the full extent of grief, I think this too should go to net and stable.
I think it needs a bit more testing to be frank, I do not see it wait months out of the tree. So if this lands in next before the merge window, we are good I think. If we miss that I will target net.
Quoted 4 lines
That would mean initially a single patch that
touches as few files as possible. The helper function ininclude/linux/ip.hand conversion of existing open coded parsers to use that can follow up later in net-next.I don't see the value to be frank.
What moving code between files accomplishes, I donnu. Squashing things surely makes review harder? Does your backporting system really struggle with trivial dependencies like this?
Quoted 8 lines
Quoted 1 line
I can drop patch 2 and will if I am targeting net.
Good. This is not about "my" backporting, but upstream stable. Refactor patches do not belong there.
Since patch 3 might have to be backported in the future if a serious abuse surfaces, even if you now land it in net-next, it is better to not depend on other patches.
Right. While this is targeting next, I will reorder it last in series so it's obvious for people it can be dropped.
Thanks a lot for the review!
Quoted 12 lines
Quoted 12 lines
Quoted 11 lines
Quoted 11 lines
This basically does this: checksum at ingress, except in a narrow list of specified known good cases.
Fixes f43798c27684 ("tun: Allow GSO using virtio_net_hdr")Michael S. Tsirkin (4):
net: addip4_parse()
net: useip4_parse()in receive validation
net: resolve uncommon partial checksums at ingress
net: account for ingress-resolved virtio net hdr state
Willem de Bruijn
maintainerQuoted 49 lines · Michael S. Tsirkin
Quoted 47 lines · Willem de Bruijn
Quoted 35 lines · Michael S. Tsirkin
Quoted 15 lines · Willem de Bruijn
Quoted 7 lines · Michael S. Tsirkin
Allowing untrusted hardware to specify csum start/offset for
CHECKSUM_PARTIALwas not a good idea, because, if it edits the packet outside the actual checksum, and depending on the setup, the networking stack could be looking at the packet both before and after the edit, and seeing different results.Quoted 6 lines
Previously, commit
6785011f8b16("ipv4: validatechecksum_startbefore completing checksum") validated all known instances where this causes the most grief, but the real fix is to make sure that either only actual checksum fields are ever edited,
or the checksum is completed before the net stack looks at it, so it is always consistent.Quoted 7 lines
+1 on the general approach of performing the checksum offload at input, unless the input can easily be validated to be one of a few common protocols. That greatly reduces the risk vs my previous attempt at trying to drop.
Since we do not know the full extent of grief, I think this too should go to net and stable.
I think it needs a bit more testing to be frank, I do not see it wait months out of the tree. So if this lands in next before the merge window, we are good I think. If we miss that I will target net.
Quoted 4 lines
That would mean initially a single patch that
touches as few files as possible. The helper function ininclude/linux/ip.hand conversion of existing open coded parsers to use that can follow up later in net-next.Quoted 7 lines
I don't see the value to be frank.
What moving code between files accomplishes, I donnu. Squashing things surely makes review harder? Does your backporting system really struggle with trivial dependencies like this?I can drop patch 2 and will if I am targeting net.
Good. This is not about "my" backporting, but upstream stable. Refactor patches do not belong there.
Quoted 8 lines
Quoted 3 lines
Since patch 3 might have to be backported in the future if a serious abuse surfaces, even if you now land it in net-next, it is better to not depend on other patches.
Right. While this is targeting next, I will reorder it last in series so it's obvious for people it can be dropped.
Thanks a lot for the review!
Of course. Thanks for the fixes!
I would open-code ip4_parse directly in "net: resolve uncommon partial checksums at ingress". It's not a lot of code. Then that patch stands on its own.
And then optionally squash patches 1 and 2 and place them at the end of this series as one dedup patch, or send that separately later. Personally I would drop that entirely and leave the existing code as is. But either way.
Quoted 14 lines
Quoted 13 lines
Quoted 13 lines
Quoted 12 lines
Quoted 12 lines
This basically does this: checksum at ingress, except in a narrow list of specified known good cases.
Fixes f43798c27684 ("tun: Allow GSO using virtio_net_hdr")Michael S. Tsirkin (4):
net: addip4_parse()
net: useip4_parse()in receive validation
net: resolve uncommon partial checksums at ingress
net: account for ingress-resolved virtio net hdr state
Michael S. Tsirkin
maintainerQuoted 57 lines · Willem de Bruijn
Quoted 56 lines · Michael S. Tsirkin
Quoted 42 lines · Willem de Bruijn
Quoted 41 lines · Michael S. Tsirkin
Quoted 15 lines · Willem de Bruijn
Quoted 7 lines · Michael S. Tsirkin
Allowing untrusted hardware to specify csum start/offset for
CHECKSUM_PARTIALwas not a good idea, because, if it edits the packet outside the actual checksum, and depending on the setup, the networking stack could be looking at the packet both before and after the edit, and seeing different results.Quoted 6 lines
Previously, commit
6785011f8b16("ipv4: validatechecksum_startbefore completing checksum") validated all known instances where this causes the most grief, but the real fix is to make sure that either only actual checksum fields are ever edited,
or the checksum is completed before the net stack looks at it, so it is always consistent.Quoted 7 lines
+1 on the general approach of performing the checksum offload at input, unless the input can easily be validated to be one of a few common protocols. That greatly reduces the risk vs my previous attempt at trying to drop.
Since we do not know the full extent of grief, I think this too should go to net and stable.
I think it needs a bit more testing to be frank, I do not see it wait months out of the tree. So if this lands in next before the merge window, we are good I think. If we miss that I will target net.
Quoted 4 lines
That would mean initially a single patch that
touches as few files as possible. The helper function ininclude/linux/ip.hand conversion of existing open coded parsers to use that can follow up later in net-next.I don't see the value to be frank.
What moving code between files accomplishes, I donnu. Squashing things surely makes review harder? Does your backporting system really struggle with trivial dependencies like this?Quoted 8 lines
Quoted 1 line
I can drop patch 2 and will if I am targeting net.
Good. This is not about "my" backporting, but upstream stable. Refactor patches do not belong there.
Since patch 3 might have to be backported in the future if a serious abuse surfaces, even if you now land it in net-next, it is better to not depend on other patches.
Right. While this is targeting next, I will reorder it last in series so it's obvious for people it can be dropped.
Quoted 7 lines
Quoted 1 line
Thanks a lot for the review!
Of course. Thanks for the fixes!
I would open-code
ip4_parsedirectly in "net: resolve uncommon partial checksums at ingress". It's not a lot of code. Then that patch stands on its own.
Ok if you prefer, except I'll make it a small inline in the same file. Easier to then dedup at the end.
Quoted 19 lines
And then optionally squash patches 1 and 2 and place them at the end of this series as one dedup patch, or send that separately later. Personally I would drop that entirely and leave the existing code as is. But either way.
Quoted 14 lines
Quoted 13 lines
Quoted 13 lines
Quoted 12 lines
Quoted 12 lines
This basically does this: checksum at ingress, except in a narrow list of specified known good cases.
Fixes f43798c27684 ("tun: Allow GSO using virtio_net_hdr")Michael S. Tsirkin (4):
net: addip4_parse()
net: useip4_parse()in receive validation
net: resolve uncommon partial checksums at ingress
net: account for ingress-resolved virtio net hdr state