This series fixes a bug in the complete phase of UDP in GRO, in which
socket lookup fails due to using network_header when parsing encapsulated
packets. The fix is to keep track of both outer and inner offsets.
The last commit leverages the first commit to remove some state from
napi_gro_cb, and stateful code in {ipv6,inet}_gro_receive which may be
unnecessarily complicated due to encapsulation support in GRO.
In addition, udpgro_fwd selftest is adjusted to include the socket lookup
case for vxlan. This selftest will test its supposed functionality once
local bind support is merged (https://lore.kernel.org/netdev/[email protected]/).
Richard Gobert (3):
net: gro: set {inner_,}network_header in receive phase
selftests/net: add local address bind in vxlan selftest
net: gro: move L3 flush checks to tcp_gro_receive
include/net/gro.h | 23 ++++---
net/8021q/vlan_core.c | 3 +
net/core/gro.c | 3 -
net/ipv4/af_inet.c | 44 ++------------
net/ipv4/tcp_offload.c | 73 ++++++++++++++++++-----
net/ipv4/udp_offload.c | 2 +-
net/ipv6/ip6_offload.c | 22 ++-----
net/ipv6/tcpv6_offload.c | 2 +-
net/ipv6/udp_offload.c | 2 +-
tools/testing/selftests/net/udpgro_fwd.sh | 10 +++-
10 files changed, 97 insertions(+), 87 deletions(-)
--
2.36.1
Commits a602456 ("udp: Add GRO functions to UDP socket") and 57c67ff ("udp:
additional GRO support") introduce incorrect usage of {ip,ipv6}_hdr in the
complete phase of gro. The functions always return skb->network_header,
which in the case of encapsulated packets at the gro complete phase, is
always set to the innermost L3 of the packet. That means that calling
{ip,ipv6}_hdr for skbs which completed the GRO receive phase (both in
gro_list and *_gro_complete) when parsing an encapsulated packet's _outer_
L3/L4 may return an unexpected value.
This incorrect usage leads to a bug in GRO's UDP socket lookup.
udp{4,6}_lib_lookup_skb functions use ip_hdr/ipv6_hdr respectively. These
*_hdr functions return network_header which will point to the innermost L3,
resulting in the wrong offset being used in __udp{4,6}_lib_lookup with
encapsulated packets.
Reproduction example:
Endpoint configuration example (fou + local address bind)
# ip fou add port 6666 ipproto 4
# ip link add name tun1 type ipip remote 2.2.2.1 local 2.2.2.2 encap fou encap-dport 5555 encap-sport 6666 mode ipip
# ip link set tun1 up
# ip a add 1.1.1.2/24 dev tun1
Netperf TCP_STREAM result on net-next before patch is applied:
net-next main, GRO enabled:
$ netperf -H 1.1.1.2 -t TCP_STREAM -l 5
Recv Send Send
Socket Socket Message Elapsed
Size Size Size Time Throughput
bytes bytes bytes secs. 10^6bits/sec
131072 16384 16384 5.28 2.37
net-next main, GRO disabled:
$ netperf -H 1.1.1.2 -t TCP_STREAM -l 5
Recv Send Send
Socket Socket Message Elapsed
Size Size Size Time Throughput
bytes bytes bytes secs. 10^6bits/sec
131072 16384 16384 5.01 2745.06
patch applied, GRO enabled:
$ netperf -H 1.1.1.2 -t TCP_STREAM -l 5
Recv Send Send
Socket Socket Message Elapsed
Size Size Size Time Throughput
bytes bytes bytes secs. 10^6bits/sec
131072 16384 16384 5.01 2877.38
This patch fixes this bug and prevents similar future misuse of
network_header by setting network_header and inner_network_header to their
respective values during the receive phase of GRO. This results in
more coherent {inner_,}network_header values for every skb in gro_list,
which also means there's no need to set/fix these values before passing
the packet forward.
network_header is already set in dev_gro_receive and under encapsulation we
set inner_network_header. *_gro_complete functions use a new helper
function - skb_gro_complete_network_header, which returns the
network_header/inner_network_header offset during the GRO complete phase,
depending on skb->encapsulation.
Fixes: 57c67ff4bd92 ("udp: additional GRO support")
Signed-off-by: Richard Gobert <[email protected]>
---
include/net/gro.h | 14 +++++++++++++-
net/8021q/vlan_core.c | 3 +++
net/ipv4/af_inet.c | 8 ++++----
net/ipv4/tcp_offload.c | 2 +-
net/ipv4/udp_offload.c | 2 +-
net/ipv6/ip6_offload.c | 11 +++++------
net/ipv6/tcpv6_offload.c | 2 +-
net/ipv6/udp_offload.c | 2 +-
8 files changed, 29 insertions(+), 15 deletions(-)
diff --git a/include/net/gro.h b/include/net/gro.h
index b435f0ddbf64..89502a7e35ed 100644
--- a/include/net/gro.h
+++ b/include/net/gro.h
@@ -177,10 +177,22 @@ static inline void *skb_gro_header(struct sk_buff *skb,
return ptr;
}
+static inline int skb_gro_network_offset(struct sk_buff *skb)
+{
+ return NAPI_GRO_CB(skb)->encap_mark ? skb_inner_network_offset(skb) :
+ skb_network_offset(skb);
+}
+
static inline void *skb_gro_network_header(struct sk_buff *skb)
{
return (NAPI_GRO_CB(skb)->frag0 ?: skb->data) +
- skb_network_offset(skb);
+ skb_gro_network_offset(skb);
+}
+
+static inline void *skb_gro_complete_network_header(struct sk_buff *skb)
+{
+ return skb->encapsulation ? skb_inner_network_header(skb) :
+ skb_network_header(skb);
}
static inline __wsum inet_gro_compute_pseudo(struct sk_buff *skb, int proto)
diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
index f00158234505..8bc871397e47 100644
--- a/net/8021q/vlan_core.c
+++ b/net/8021q/vlan_core.c
@@ -478,6 +478,9 @@ static struct sk_buff *vlan_gro_receive(struct list_head *head,
if (unlikely(!vhdr))
goto out;
+ if (!NAPI_GRO_CB(skb)->encap_mark)
+ skb_set_network_header(skb, hlen);
+
type = vhdr->h_vlan_encapsulated_proto;
ptype = gro_find_receive_by_type(type);
diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 835f4f9d98d2..c0f3c162bf73 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -1564,7 +1564,9 @@ struct sk_buff *inet_gro_receive(struct list_head *head, struct sk_buff *skb)
NAPI_GRO_CB(skb)->is_atomic = !!(iph->frag_off & htons(IP_DF));
NAPI_GRO_CB(skb)->flush |= flush;
- skb_set_network_header(skb, off);
+ if (NAPI_GRO_CB(skb)->encap_mark)
+ skb_set_inner_network_header(skb, off);
+
/* The above will be needed by the transport layer if there is one
* immediately following this IP hdr.
*/
@@ -1643,10 +1645,8 @@ int inet_gro_complete(struct sk_buff *skb, int nhoff)
int proto = iph->protocol;
int err = -ENOSYS;
- if (skb->encapsulation) {
+ if (skb->encapsulation)
skb_set_inner_protocol(skb, cpu_to_be16(ETH_P_IP));
- skb_set_inner_network_header(skb, nhoff);
- }
iph_set_totlen(iph, skb->len - nhoff);
csum_replace2(&iph->check, totlen, iph->tot_len);
diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c
index 8311c38267b5..8bbcd3f502ac 100644
--- a/net/ipv4/tcp_offload.c
+++ b/net/ipv4/tcp_offload.c
@@ -330,7 +330,7 @@ struct sk_buff *tcp4_gro_receive(struct list_head *head, struct sk_buff *skb)
INDIRECT_CALLABLE_SCOPE int tcp4_gro_complete(struct sk_buff *skb, int thoff)
{
- const struct iphdr *iph = ip_hdr(skb);
+ const struct iphdr *iph = skb_gro_complete_network_header(skb);
struct tcphdr *th = tcp_hdr(skb);
th->check = ~tcp_v4_check(skb->len - thoff, iph->saddr,
diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index 6c95d28d0c4a..7f59cede67f5 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -709,7 +709,7 @@ EXPORT_SYMBOL(udp_gro_complete);
INDIRECT_CALLABLE_SCOPE int udp4_gro_complete(struct sk_buff *skb, int nhoff)
{
- const struct iphdr *iph = ip_hdr(skb);
+ const struct iphdr *iph = skb_gro_complete_network_header(skb);
struct udphdr *uh = (struct udphdr *)(skb->data + nhoff);
/* do fraglist only if there is no outer UDP encap (or we already processed it) */
diff --git a/net/ipv6/ip6_offload.c b/net/ipv6/ip6_offload.c
index cca64c7809be..db7e3db587b9 100644
--- a/net/ipv6/ip6_offload.c
+++ b/net/ipv6/ip6_offload.c
@@ -67,7 +67,7 @@ static int ipv6_gro_pull_exthdrs(struct sk_buff *skb, int off, int proto)
off += len;
}
- skb_gro_pull(skb, off - skb_network_offset(skb));
+ skb_gro_pull(skb, off - skb_gro_network_offset(skb));
return proto;
}
@@ -236,7 +236,8 @@ INDIRECT_CALLABLE_SCOPE struct sk_buff *ipv6_gro_receive(struct list_head *head,
if (unlikely(!iph))
goto out;
- skb_set_network_header(skb, off);
+ if (NAPI_GRO_CB(skb)->encap_mark)
+ skb_set_inner_network_header(skb, off);
flush += ntohs(iph->payload_len) != skb->len - hlen;
@@ -259,7 +260,7 @@ INDIRECT_CALLABLE_SCOPE struct sk_buff *ipv6_gro_receive(struct list_head *head,
NAPI_GRO_CB(skb)->proto = proto;
flush--;
- nlen = skb_network_header_len(skb);
+ nlen = skb_gro_offset(skb) - off;
list_for_each_entry(p, head, list) {
const struct ipv6hdr *iph2;
@@ -353,10 +354,8 @@ INDIRECT_CALLABLE_SCOPE int ipv6_gro_complete(struct sk_buff *skb, int nhoff)
int err = -ENOSYS;
u32 payload_len;
- if (skb->encapsulation) {
+ if (skb->encapsulation)
skb_set_inner_protocol(skb, cpu_to_be16(ETH_P_IPV6));
- skb_set_inner_network_header(skb, nhoff);
- }
payload_len = skb->len - nhoff - sizeof(*iph);
if (unlikely(payload_len > IPV6_MAXPLEN)) {
diff --git a/net/ipv6/tcpv6_offload.c b/net/ipv6/tcpv6_offload.c
index bf0c957e4b5e..79eeaced2834 100644
--- a/net/ipv6/tcpv6_offload.c
+++ b/net/ipv6/tcpv6_offload.c
@@ -29,7 +29,7 @@ struct sk_buff *tcp6_gro_receive(struct list_head *head, struct sk_buff *skb)
INDIRECT_CALLABLE_SCOPE int tcp6_gro_complete(struct sk_buff *skb, int thoff)
{
- const struct ipv6hdr *iph = ipv6_hdr(skb);
+ const struct ipv6hdr *iph = skb_gro_complete_network_header(skb);
struct tcphdr *th = tcp_hdr(skb);
th->check = ~tcp_v6_check(skb->len - thoff, &iph->saddr,
diff --git a/net/ipv6/udp_offload.c b/net/ipv6/udp_offload.c
index 6b95ba241ebe..897caa2e39fb 100644
--- a/net/ipv6/udp_offload.c
+++ b/net/ipv6/udp_offload.c
@@ -164,7 +164,7 @@ struct sk_buff *udp6_gro_receive(struct list_head *head, struct sk_buff *skb)
INDIRECT_CALLABLE_SCOPE int udp6_gro_complete(struct sk_buff *skb, int nhoff)
{
- const struct ipv6hdr *ipv6h = ipv6_hdr(skb);
+ const struct ipv6hdr *ipv6h = skb_gro_complete_network_header(skb);
struct udphdr *uh = (struct udphdr *)(skb->data + nhoff);
/* do fraglist only if there is no outer UDP encap (or we already processed it) */
--
2.36.1
Add local address bind support to existing udpgro_fwd.sh vxlan selftest, to
ensure UDP socket lookup in GRO is working.
Signed-off-by: Richard Gobert <[email protected]>
---
tools/testing/selftests/net/udpgro_fwd.sh | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/net/udpgro_fwd.sh b/tools/testing/selftests/net/udpgro_fwd.sh
index c079565add39..8cc9b6ff3910 100755
--- a/tools/testing/selftests/net/udpgro_fwd.sh
+++ b/tools/testing/selftests/net/udpgro_fwd.sh
@@ -56,11 +56,13 @@ create_vxlan_endpoint() {
local -r bm_rem_addr=$3
local -r vxlan_dev=$4
local -r vxlan_id=$5
+ local -r bm_local_addr=$6
local -r vxlan_port=4789
ip -n $netns link set dev $bm_dev up
ip -n $netns link add dev $vxlan_dev type vxlan id $vxlan_id \
- dstport $vxlan_port remote $bm_rem_addr
+ dstport $vxlan_port local $bm_local_addr \
+ remote $bm_rem_addr
ip -n $netns link set dev $vxlan_dev up
}
@@ -71,11 +73,13 @@ create_vxlan_pair() {
for ns in $SRC $DST; do
# note that 3 - $SRC == $DST and 3 - $DST == $SRC
- create_vxlan_endpoint $BASE$ns veth$ns $BM_NET_V4$((3 - $ns)) vxlan$ns 4
+ create_vxlan_endpoint $BASE$ns veth$ns $BM_NET_V4$((3 - $ns)) \
+ vxlan$ns 4 $BM_NET_V4$ns
ip -n $BASE$ns addr add dev vxlan$ns $OL_NET_V4$ns/24
done
for ns in $SRC $DST; do
- create_vxlan_endpoint $BASE$ns veth$ns $BM_NET_V6$((3 - $ns)) vxlan6$ns 6
+ create_vxlan_endpoint $BASE$ns veth$ns $BM_NET_V6$((3 - $ns)) \
+ vxlan6$ns 6 $BM_NET_V6$ns
ip -n $BASE$ns addr add dev vxlan6$ns $OL_NET_V6$ns/24 nodad
done
}
--
2.36.1
{inet,ipv6}_gro_receive functions perform flush checks (ttl, flags,
iph->id, ...) against all packets in a loop. These flush checks are
relevant only to tcp flows, and as such they're used to determine whether
the packets can be merged later in tcp_gro_receive.
These checks are not relevant to UDP packets. Furthermore, they need to be
done only once in tcp_gro_receive and only against the found p skb, since
they only affect flush and not same_flow.
Leveraging the initial commit in the series, correct network header offsets
are saved for both outer and inner network headers - allowing these checks
to be done only once, in tcp_gro_receive. As a result,
NAPI_GRO_CB(p)->flush is not used at all. In addition - flush_id checks are
more declarative and contained in inet_gro_flush_id, thus removing the need
for flush_id in napi_gro_cb.
This results in less parsing code for UDP flows and non-loop flush tests
for TCP flows.
For example, running 64 IP/UDP netperf connections:
/super_netperf.sh 64 -H 1.1.1.2 -t UDP_STREAM -l 120
Running perf top for 90s we can see that relatively less time is spent on
inet_gro_receive:
net-next:
Samples: 330K of event 'cycles:P', 4000 Hz, Event count (approx.): 45945660156 lost: 0/0 drop: 0/0
1.43% [kernel] [k] inet_gro_receive
patch applied:
Samples: 295K of event 'cycles:P', 4000 Hz, Event count (approx.): 44726408099 lost: 0/0 drop: 0/0
1.22% [kernel] [k] inet_gro_receive
Signed-off-by: Richard Gobert <[email protected]>
---
include/net/gro.h | 9 ++----
net/core/gro.c | 3 --
net/ipv4/af_inet.c | 36 ---------------------
net/ipv4/tcp_offload.c | 71 ++++++++++++++++++++++++++++++++++--------
net/ipv6/ip6_offload.c | 11 -------
5 files changed, 61 insertions(+), 69 deletions(-)
diff --git a/include/net/gro.h b/include/net/gro.h
index 89502a7e35ed..e6db591cdc32 100644
--- a/include/net/gro.h
+++ b/include/net/gro.h
@@ -35,15 +35,15 @@ struct napi_gro_cb {
/* This is non-zero if the packet cannot be merged with the new skb. */
u16 flush;
- /* Save the IP ID here and check when we get to the transport layer */
- u16 flush_id;
-
/* Number of segments aggregated. */
u16 count;
/* Used in ipv6_gro_receive() and foo-over-udp and esp-in-udp */
u16 proto;
+ /* used to support CHECKSUM_COMPLETE for tunneling protocols */
+ __wsum csum;
+
/* Used in napi_gro_cb::free */
#define NAPI_GRO_FREE 1
#define NAPI_GRO_FREE_STOLEN_HEAD 2
@@ -83,9 +83,6 @@ struct napi_gro_cb {
/* GRO is done by frag_list pointer chaining. */
u8 is_flist:1;
);
-
- /* used to support CHECKSUM_COMPLETE for tunneling protocols */
- __wsum csum;
};
#define NAPI_GRO_CB(skb) ((struct napi_gro_cb *)(skb)->cb)
diff --git a/net/core/gro.c b/net/core/gro.c
index 0759277dc14e..b7dced5bcef7 100644
--- a/net/core/gro.c
+++ b/net/core/gro.c
@@ -332,8 +332,6 @@ static void gro_list_prepare(const struct list_head *head,
list_for_each_entry(p, head, list) {
unsigned long diffs;
- NAPI_GRO_CB(p)->flush = 0;
-
if (hash != skb_get_hash_raw(p)) {
NAPI_GRO_CB(p)->same_flow = 0;
continue;
@@ -466,7 +464,6 @@ static enum gro_result dev_gro_receive(struct napi_struct *napi, struct sk_buff
sizeof(u32))); /* Avoid slow unaligned acc */
*(u32 *)&NAPI_GRO_CB(skb)->zeroed = 0;
NAPI_GRO_CB(skb)->flush = skb_has_frag_list(skb);
- NAPI_GRO_CB(skb)->is_atomic = 1;
NAPI_GRO_CB(skb)->count = 1;
if (unlikely(skb_is_gso(skb))) {
NAPI_GRO_CB(skb)->count = skb_shinfo(skb)->gso_segs;
diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index c0f3c162bf73..fd7378ecc4e3 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -1509,7 +1509,6 @@ struct sk_buff *inet_gro_receive(struct list_head *head, struct sk_buff *skb)
list_for_each_entry(p, head, list) {
struct iphdr *iph2;
- u16 flush_id;
if (!NAPI_GRO_CB(p)->same_flow)
continue;
@@ -1526,43 +1525,8 @@ struct sk_buff *inet_gro_receive(struct list_head *head, struct sk_buff *skb)
NAPI_GRO_CB(p)->same_flow = 0;
continue;
}
-
- /* All fields must match except length and checksum. */
- NAPI_GRO_CB(p)->flush |=
- (iph->ttl ^ iph2->ttl) |
- (iph->tos ^ iph2->tos) |
- ((iph->frag_off ^ iph2->frag_off) & htons(IP_DF));
-
- NAPI_GRO_CB(p)->flush |= flush;
-
- /* We need to store of the IP ID check to be included later
- * when we can verify that this packet does in fact belong
- * to a given flow.
- */
- flush_id = (u16)(id - ntohs(iph2->id));
-
- /* This bit of code makes it much easier for us to identify
- * the cases where we are doing atomic vs non-atomic IP ID
- * checks. Specifically an atomic check can return IP ID
- * values 0 - 0xFFFF, while a non-atomic check can only
- * return 0 or 0xFFFF.
- */
- if (!NAPI_GRO_CB(p)->is_atomic ||
- !(iph->frag_off & htons(IP_DF))) {
- flush_id ^= NAPI_GRO_CB(p)->count;
- flush_id = flush_id ? 0xFFFF : 0;
- }
-
- /* If the previous IP ID value was based on an atomic
- * datagram we can overwrite the value and ignore it.
- */
- if (NAPI_GRO_CB(skb)->is_atomic)
- NAPI_GRO_CB(p)->flush_id = flush_id;
- else
- NAPI_GRO_CB(p)->flush_id |= flush_id;
}
- NAPI_GRO_CB(skb)->is_atomic = !!(iph->frag_off & htons(IP_DF));
NAPI_GRO_CB(skb)->flush |= flush;
if (NAPI_GRO_CB(skb)->encap_mark)
skb_set_inner_network_header(skb, off);
diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c
index 8bbcd3f502ac..962c1b900e11 100644
--- a/net/ipv4/tcp_offload.c
+++ b/net/ipv4/tcp_offload.c
@@ -178,6 +178,60 @@ struct sk_buff *tcp_gso_segment(struct sk_buff *skb,
return segs;
}
+static inline bool inet_gro_flush(struct iphdr *iph, struct iphdr *iph2)
+{
+ /* All fields must match except length and checksum. */
+ return (iph->ttl ^ iph2->ttl) |
+ (iph->tos ^ iph2->tos) |
+ ((iph->frag_off ^ iph2->frag_off) & htons(IP_DF));
+}
+
+static inline bool ipv6_gro_flush(struct ipv6hdr *iph, struct ipv6hdr *iph2)
+{
+ /* <Version:4><Traffic_Class:8><Flow_Label:20> */
+ __be32 first_word = *(__be32 *)iph ^ *(__be32 *)iph2;
+
+ /* Flush if Traffic Class fields are different. */
+ return (first_word & htonl(0x0FF00000)) |
+ (__force __be32)(iph->hop_limit ^ iph2->hop_limit);
+}
+
+static inline bool inet_gro_flush_id(struct sk_buff *p, struct iphdr *iph,
+ struct iphdr *iph2)
+{
+ u16 flush_id = ntohs(iph->id) - ntohs(iph2->id);
+ u16 is_atomic = !!(iph->frag_off & htons(IP_DF));
+ u16 count_masked;
+
+ /* When we receive our second frame we can make a decision on if we
+ * continue this flow as an atomic flow with a fixed ID or if we use
+ * an incrementing ID.
+ */
+ if (NAPI_GRO_CB(p)->count == 1)
+ NAPI_GRO_CB(p)->is_atomic = flush_id == 0;
+
+ /* Ignore outer IP ID value if based on atomic datagram. */
+ if (NAPI_GRO_CB(p)->encap_mark && is_atomic && iph2 == ip_hdr(p))
+ return false;
+
+ is_atomic &= NAPI_GRO_CB(p)->is_atomic;
+ count_masked = NAPI_GRO_CB(p)->count & ~(-is_atomic | is_atomic);
+
+ return flush_id ^ count_masked;
+}
+
+static bool tcp_gro_network_flush(struct sk_buff *p, struct sk_buff *skb,
+ int offset)
+{
+ void *nh = (NAPI_GRO_CB(skb)->frag0 ?: skb->data) + offset;
+ void *nh2 = p->data + offset;
+
+ if (((struct iphdr *)nh)->version == 6)
+ return ipv6_gro_flush(nh, nh2);
+
+ return inet_gro_flush(nh, nh2) | inet_gro_flush_id(p, nh, nh2);
+}
+
struct sk_buff *tcp_gro_receive(struct list_head *head, struct sk_buff *skb)
{
struct sk_buff *pp = NULL;
@@ -232,8 +286,10 @@ struct sk_buff *tcp_gro_receive(struct list_head *head, struct sk_buff *skb)
goto out_check_final;
found:
- /* Include the IP ID check below from the inner most IP hdr */
- flush = NAPI_GRO_CB(p)->flush;
+ flush = tcp_gro_network_flush(p, skb, skb_network_offset(skb));
+ if (NAPI_GRO_CB(skb)->encap_mark)
+ flush |= tcp_gro_network_flush(p, skb, skb_inner_network_offset(skb));
+
flush |= (__force int)(flags & TCP_FLAG_CWR);
flush |= (__force int)((flags ^ tcp_flag_word(th2)) &
~(TCP_FLAG_CWR | TCP_FLAG_FIN | TCP_FLAG_PSH));
@@ -242,17 +298,6 @@ struct sk_buff *tcp_gro_receive(struct list_head *head, struct sk_buff *skb)
flush |= *(u32 *)((u8 *)th + i) ^
*(u32 *)((u8 *)th2 + i);
- /* When we receive our second frame we can made a decision on if we
- * continue this flow as an atomic flow with a fixed ID or if we use
- * an incrementing ID.
- */
- if (NAPI_GRO_CB(p)->flush_id != 1 ||
- NAPI_GRO_CB(p)->count != 1 ||
- !NAPI_GRO_CB(p)->is_atomic)
- flush |= NAPI_GRO_CB(p)->flush_id;
- else
- NAPI_GRO_CB(p)->is_atomic = false;
-
mss = skb_shinfo(p)->gso_size;
/* If skb is a GRO packet, make sure its gso_size matches prior packet mss.
diff --git a/net/ipv6/ip6_offload.c b/net/ipv6/ip6_offload.c
index db7e3db587b9..8bf93a0e9669 100644
--- a/net/ipv6/ip6_offload.c
+++ b/net/ipv6/ip6_offload.c
@@ -291,19 +291,8 @@ INDIRECT_CALLABLE_SCOPE struct sk_buff *ipv6_gro_receive(struct list_head *head,
nlen - sizeof(struct ipv6hdr)))
goto not_same_flow;
}
- /* flush if Traffic Class fields are different */
- NAPI_GRO_CB(p)->flush |= !!((first_word & htonl(0x0FF00000)) |
- (__force __be32)(iph->hop_limit ^ iph2->hop_limit));
- NAPI_GRO_CB(p)->flush |= flush;
-
- /* If the previous IP ID value was based on an atomic
- * datagram we can overwrite the value and ignore it.
- */
- if (NAPI_GRO_CB(skb)->is_atomic)
- NAPI_GRO_CB(p)->flush_id = 0;
}
- NAPI_GRO_CB(skb)->is_atomic = true;
NAPI_GRO_CB(skb)->flush |= flush;
skb_gro_postpull_rcsum(skb, iph, nlen);
--
2.36.1
Eric Dumazet wrote:
>
> My intuition is that this patch has a high cost for normal GRO processing.
> SW-GRO is already a bottleneck on ARM cores in smart NICS.
>
> I would suggest instead using parameters to give both the nhoff and thoff values
> this would avoid many conditionals in the fast path.
>
> ->
>
> INDIRECT_CALLABLE_SCOPE int udp6_gro_complete(struct sk_buff *skb, int
> nhoff, int thoff)
> {
> const struct ipv6hdr *ipv6h = (const struct ipv6hdr *)(skb->data + nhoff);
> struct udphdr *uh = (struct udphdr *)(skb->data + thoff);
> ...
> }
>
> INDIRECT_CALLABLE_SCOPE int tcp6_gro_complete(struct sk_buff *skb, int
> nhoff, int thoff)
> {
> const struct ipv6hdr *iph = (const struct ipv6hdr *)(skb->data + nhoff);
> struct tcphdr *th = (struct tcphdr *)(skb->data + thoff);
>
> Why storing in skb fields things that really could be propagated more
> efficiently as function parameters ?
Hi Eric,
Thanks for the review!
I agree, the conditionals could be a problem and are actually not needed.
The third commit in this patch series introduces an optimisation for
ipv6/ipv4 using the correct {inner_}network_header. We can remove the
conditionals; I thought about multiple ways to do so. First, remove the
conditional in skb_gro_network_offset:
static inline int skb_gro_network_offset(const struct sk_buff *skb)
{
const u32 mask = NAPI_GRO_CB(skb)->encap_mark - 1;
return (skb_network_offset(skb) & mask) | (skb_inner_network_offset(skb) & ~mask);
}
And for the conditionals in {inet,ipv6}_gro_receive I thought about two
ideas. The first is to move set_inner_network_header to encapsulation gro
functions like ipip_gro_receive, this way there's one less write (in
comparison to main) in these functions:
static struct sk_buff *ipip_gro_receive(struct list_head *head,
struct sk_buff *skb)
{
...
NAPI_GRO_CB(skb)->encap_mark = 1;
skb_set_inner_network_header(skb, skb_gro_offset(skb));
The second way is to always write to inner_network_header:
INDIRECT_CALLABLE_SCOPE struct sk_buff *ipv6_gro_receive(struct list_head *head,
struct sk_buff *skb)
{
...
skb_set_inner_network_header(skb, off);
...
What do you think is better? I think the 1st is more beneficial for the
fast path.
We could then use the {inner_}network_header separation to optimise the
receive path, such as in the 3rd commit in this patch series.
Regards,
Richard
On Thu, Feb 29, 2024 at 2:22 PM Richard Gobert <[email protected]> wrote:
>
>
>
> Eric Dumazet wrote:
> >
> > My intuition is that this patch has a high cost for normal GRO processing.
> > SW-GRO is already a bottleneck on ARM cores in smart NICS.
> >
> > I would suggest instead using parameters to give both the nhoff and thoff values
> > this would avoid many conditionals in the fast path.
> >
> > ->
> >
> > INDIRECT_CALLABLE_SCOPE int udp6_gro_complete(struct sk_buff *skb, int
> > nhoff, int thoff)
> > {
> > const struct ipv6hdr *ipv6h = (const struct ipv6hdr *)(skb->data + nhoff);
> > struct udphdr *uh = (struct udphdr *)(skb->data + thoff);
> > ...
> > }
> >
> > INDIRECT_CALLABLE_SCOPE int tcp6_gro_complete(struct sk_buff *skb, int
> > nhoff, int thoff)
> > {
> > const struct ipv6hdr *iph = (const struct ipv6hdr *)(skb->data + nhoff);
> > struct tcphdr *th = (struct tcphdr *)(skb->data + thoff);
> >
> > Why storing in skb fields things that really could be propagated more
> > efficiently as function parameters ?
>
> Hi Eric,
> Thanks for the review!
>
> I agree, the conditionals could be a problem and are actually not needed.
> The third commit in this patch series introduces an optimisation for
> ipv6/ipv4 using the correct {inner_}network_header. We can remove the
> conditionals; I thought about multiple ways to do so. First, remove the
> conditional in skb_gro_network_offset:
>
> static inline int skb_gro_network_offset(const struct sk_buff *skb)
> {
> const u32 mask = NAPI_GRO_CB(skb)->encap_mark - 1;
> return (skb_network_offset(skb) & mask) | (skb_inner_network_offset(skb) & ~mask);
> }
I was trying to say that we do not need all these helpers, storing
state in NAPI_GRO_CB(skb),
dirtying cache lines...
Ideally, the skb network/transport/... headers could be set at the
last stage, in gro_complete(big_gro_skb),
instead of doing this for each segment.
All the gro_receive() could be much faster by using additional
parameters (nhoff, thoff)
skb_gro_offset() could be replaced by the current offset (nhoff or
other name), passed as a parameter.
Here is a WIP for gro_complete() step, this looks large but this is
only adding a 2nd 'offset' parameter
Prior offset (typically network offset), called p_off
Old argument nhoff, (renamed thoff if that makes sense), pointing to
the current offset.
drivers/net/geneve.c | 6 +++---
drivers/net/vxlan/vxlan_core.c | 11 +++++++----
include/linux/etherdevice.h | 2 +-
include/linux/netdevice.h | 2 +-
include/linux/udp.h | 2 +-
include/net/gro.h | 10 +++++-----
include/net/inet_common.h | 2 +-
include/net/tcp.h | 4 ++--
include/net/udp.h | 8 ++++----
include/net/udp_tunnel.h | 2 +-
net/8021q/vlan_core.c | 4 ++--
net/core/gro.c | 2 +-
net/ethernet/eth.c | 4 ++--
net/ipv4/af_inet.c | 8 ++++----
net/ipv4/fou_core.c | 9 +++++----
net/ipv4/gre_offload.c | 4 ++--
net/ipv4/tcp_offload.c | 6 +++---
net/ipv4/udp.c | 3 ++-
net/ipv4/udp_offload.c | 24 ++++++++++++------------
net/ipv6/ip6_offload.c | 22 ++++++++++++----------
net/ipv6/tcpv6_offload.c | 7 ++++---
net/ipv6/udp.c | 3 ++-
net/ipv6/udp_offload.c | 12 ++++++------
23 files changed, 83 insertions(+), 74 deletions(-)
diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index 6f3f9b446b1d202f6c71a20ce48088691e9120bf..af8dfdd47ffdeb7bdea302c5957e81faf83b19db
100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -546,7 +546,7 @@ static struct sk_buff *geneve_gro_receive(struct sock *sk,
}
static int geneve_gro_complete(struct sock *sk, struct sk_buff *skb,
- int nhoff)
+ int p_off, int nhoff)
{
struct genevehdr *gh;
struct packet_offload *ptype;
@@ -560,11 +560,11 @@ static int geneve_gro_complete(struct sock *sk,
struct sk_buff *skb,
/* since skb->encapsulation is set, eth_gro_complete() sets
the inner mac header */
if (likely(type == htons(ETH_P_TEB)))
- return eth_gro_complete(skb, nhoff + gh_len);
+ return eth_gro_complete(skb, p_off, nhoff + gh_len);
ptype = gro_find_complete_by_type(type);
if (ptype)
- err = ptype->callbacks.gro_complete(skb, nhoff + gh_len);
+ err = ptype->callbacks.gro_complete(skb, p_off, nhoff + gh_len);
skb_set_inner_mac_header(skb, nhoff + gh_len);
diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
index 386cbe4d33272791e80470bd1378731d0c3b4d3b..84c123405b70f986a40b9f531e826807bcfc880b
100644
--- a/drivers/net/vxlan/vxlan_core.c
+++ b/drivers/net/vxlan/vxlan_core.c
@@ -767,15 +767,17 @@ static struct sk_buff
*vxlan_gpe_gro_receive(struct sock *sk,
return pp;
}
-static int vxlan_gro_complete(struct sock *sk, struct sk_buff *skb, int nhoff)
+static int vxlan_gro_complete(struct sock *sk, struct sk_buff *skb,
+ int p_off, int nhoff)
{
/* Sets 'skb->inner_mac_header' since we are always called with
* 'skb->encapsulation' set.
*/
- return eth_gro_complete(skb, nhoff + sizeof(struct vxlanhdr));
+ return eth_gro_complete(skb, p_off, nhoff + sizeof(struct vxlanhdr));
}
-static int vxlan_gpe_gro_complete(struct sock *sk, struct sk_buff
*skb, int nhoff)
+static int vxlan_gpe_gro_complete(struct sock *sk, struct sk_buff *skb,
+ int p_off, int nhoff)
{
struct vxlanhdr *vh = (struct vxlanhdr *)(skb->data + nhoff);
const struct packet_offload *ptype;
@@ -786,7 +788,8 @@ static int vxlan_gpe_gro_complete(struct sock *sk,
struct sk_buff *skb, int nhof
return err;
ptype = gro_find_complete_by_type(protocol);
if (ptype)
- err = ptype->callbacks.gro_complete(skb, nhoff +
sizeof(struct vxlanhdr));
+ err = ptype->callbacks.gro_complete(skb, p_off, nhoff +
+ sizeof(struct vxlanhdr));
return err;
}
diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h
index 224645f17c333b2311573197a28b41701eb35f92..b081b43d9686a1f3b0ddc9d84e66566e297a2d67
100644
--- a/include/linux/etherdevice.h
+++ b/include/linux/etherdevice.h
@@ -64,7 +64,7 @@ struct net_device *devm_alloc_etherdev_mqs(struct
device *dev, int sizeof_priv,
#define devm_alloc_etherdev(dev, sizeof_priv)
devm_alloc_etherdev_mqs(dev, sizeof_priv, 1, 1)
struct sk_buff *eth_gro_receive(struct list_head *head, struct sk_buff *skb);
-int eth_gro_complete(struct sk_buff *skb, int nhoff);
+int eth_gro_complete(struct sk_buff *skb, int p_off, int nhoff);
/* Reserved Ethernet Addresses per IEEE 802.1Q */
static const u8 eth_reserved_addr_base[ETH_ALEN] __aligned(2) =
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 09023e44db4e2c3a2133afc52ba5a335d6030646..b21745f6233a2b56a64adda9f303d4db22019a37
100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2788,7 +2788,7 @@ struct offload_callbacks {
netdev_features_t features);
struct sk_buff *(*gro_receive)(struct list_head *head,
struct sk_buff *skb);
- int (*gro_complete)(struct sk_buff *skb, int nhoff);
+ int (*gro_complete)(struct sk_buff *skb,
int nhoff, int thoff);
};
struct packet_offload {
diff --git a/include/linux/udp.h b/include/linux/udp.h
index 3748e82b627b7044508db66adbf77c54a8e3d612..a04d94a3b42f8f5fb3d85622d2f3e56a94c4ea86
100644
--- a/include/linux/udp.h
+++ b/include/linux/udp.h
@@ -82,7 +82,7 @@ struct udp_sock {
struct sk_buff *skb);
int (*gro_complete)(struct sock *sk,
struct sk_buff *skb,
- int nhoff);
+ int nhoff, int thoff);
/* udp_recvmsg try to use this before splicing sk_receive_queue */
struct sk_buff_head reader_queue ____cacheline_aligned_in_smp;
diff --git a/include/net/gro.h b/include/net/gro.h
index b435f0ddbf64f7bf740b7e479a1b28bcdef122c6..2856b00b84dfbf122871ef9bedab2097a7c823eb
100644
--- a/include/net/gro.h
+++ b/include/net/gro.h
@@ -385,18 +385,18 @@ static inline void
skb_gro_flush_final_remcsum(struct sk_buff *skb,
INDIRECT_CALLABLE_DECLARE(struct sk_buff *ipv6_gro_receive(struct list_head *,
struct sk_buff *));
-INDIRECT_CALLABLE_DECLARE(int ipv6_gro_complete(struct sk_buff *, int));
+INDIRECT_CALLABLE_DECLARE(int ipv6_gro_complete(struct sk_buff *, int, int));
INDIRECT_CALLABLE_DECLARE(struct sk_buff *inet_gro_receive(struct list_head *,
struct sk_buff *));
-INDIRECT_CALLABLE_DECLARE(int inet_gro_complete(struct sk_buff *, int));
+INDIRECT_CALLABLE_DECLARE(int inet_gro_complete(struct sk_buff *, int, int));
INDIRECT_CALLABLE_DECLARE(struct sk_buff *udp4_gro_receive(struct list_head *,
struct sk_buff *));
-INDIRECT_CALLABLE_DECLARE(int udp4_gro_complete(struct sk_buff *, int));
+INDIRECT_CALLABLE_DECLARE(int udp4_gro_complete(struct sk_buff *, int, int));
INDIRECT_CALLABLE_DECLARE(struct sk_buff *udp6_gro_receive(struct list_head *,
struct sk_buff *));
-INDIRECT_CALLABLE_DECLARE(int udp6_gro_complete(struct sk_buff *, int));
+INDIRECT_CALLABLE_DECLARE(int udp6_gro_complete(struct sk_buff *, int, int));
#define indirect_call_gro_receive_inet(cb, f2, f1, head, skb) \
({ \
@@ -407,7 +407,7 @@ INDIRECT_CALLABLE_DECLARE(int
udp6_gro_complete(struct sk_buff *, int));
struct sk_buff *udp_gro_receive(struct list_head *head, struct sk_buff *skb,
struct udphdr *uh, struct sock *sk);
-int udp_gro_complete(struct sk_buff *skb, int nhoff, udp_lookup_t lookup);
+int udp_gro_complete(struct sk_buff *skb, int nhoff, int thoff,
udp_lookup_t lookup);
static inline struct udphdr *udp_gro_udphdr(struct sk_buff *skb)
{
diff --git a/include/net/inet_common.h b/include/net/inet_common.h
index f50a644d87a9871fbed2dfd49d4bde9d3df0fd92..605f917c830c9b1794dfc3bda1bdc9ac24a0def3
100644
--- a/include/net/inet_common.h
+++ b/include/net/inet_common.h
@@ -64,7 +64,7 @@ int inet_recv_error(struct sock *sk, struct msghdr
*msg, int len,
int *addr_len);
struct sk_buff *inet_gro_receive(struct list_head *head, struct sk_buff *skb);
-int inet_gro_complete(struct sk_buff *skb, int nhoff);
+int inet_gro_complete(struct sk_buff *skb, int nhoff, int thoff);
struct sk_buff *inet_gso_segment(struct sk_buff *skb,
netdev_features_t features);
diff --git a/include/net/tcp.h b/include/net/tcp.h
index 6ae35199d3b3c159ba029ff74b109c56a7c7d2fc..ad8e6efc8bd867ada990820b5badce96ee49c3da
100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -2196,9 +2196,9 @@ void tcp_v4_destroy_sock(struct sock *sk);
struct sk_buff *tcp_gso_segment(struct sk_buff *skb,
netdev_features_t features);
struct sk_buff *tcp_gro_receive(struct list_head *head, struct sk_buff *skb);
-INDIRECT_CALLABLE_DECLARE(int tcp4_gro_complete(struct sk_buff *skb,
int thoff));
+INDIRECT_CALLABLE_DECLARE(int tcp4_gro_complete(struct sk_buff *skb,
int nhoff, int thoff));
INDIRECT_CALLABLE_DECLARE(struct sk_buff *tcp4_gro_receive(struct
list_head *head, struct sk_buff *skb));
-INDIRECT_CALLABLE_DECLARE(int tcp6_gro_complete(struct sk_buff *skb,
int thoff));
+INDIRECT_CALLABLE_DECLARE(int tcp6_gro_complete(struct sk_buff *skb,
int nhoff, int thoff));
INDIRECT_CALLABLE_DECLARE(struct sk_buff *tcp6_gro_receive(struct
list_head *head, struct sk_buff *skb));
#ifdef CONFIG_INET
void tcp_gro_complete(struct sk_buff *skb);
diff --git a/include/net/udp.h b/include/net/udp.h
index 488a6d2babccf26edfbaecc525f25e03d86b7d62..601d1c3b677a9acd6b6615e64de3f034f940287d
100644
--- a/include/net/udp.h
+++ b/include/net/udp.h
@@ -166,8 +166,8 @@ static inline void udp_csum_pull_header(struct sk_buff *skb)
UDP_SKB_CB(skb)->cscov -= sizeof(struct udphdr);
}
-typedef struct sock *(*udp_lookup_t)(const struct sk_buff *skb, __be16 sport,
- __be16 dport);
+typedef struct sock *(*udp_lookup_t)(const struct sk_buff *skb, int nhoff,
+ __be16 sport, __be16 dport);
void udp_v6_early_demux(struct sk_buff *skb);
INDIRECT_CALLABLE_DECLARE(int udpv6_rcv(struct sk_buff *));
@@ -301,7 +301,7 @@ struct sock *udp4_lib_lookup(struct net *net,
__be32 saddr, __be16 sport,
struct sock *__udp4_lib_lookup(struct net *net, __be32 saddr, __be16 sport,
__be32 daddr, __be16 dport, int dif, int sdif,
struct udp_table *tbl, struct sk_buff *skb);
-struct sock *udp4_lib_lookup_skb(const struct sk_buff *skb,
+struct sock *udp4_lib_lookup_skb(const struct sk_buff *skb, int nhoff,
__be16 sport, __be16 dport);
struct sock *udp6_lib_lookup(struct net *net,
const struct in6_addr *saddr, __be16 sport,
@@ -312,7 +312,7 @@ struct sock *__udp6_lib_lookup(struct net *net,
const struct in6_addr *daddr, __be16 dport,
int dif, int sdif, struct udp_table *tbl,
struct sk_buff *skb);
-struct sock *udp6_lib_lookup_skb(const struct sk_buff *skb,
+struct sock *udp6_lib_lookup_skb(const struct sk_buff *skb, int nhoff,
__be16 sport, __be16 dport);
int udp_read_skb(struct sock *sk, skb_read_actor_t recv_actor);
diff --git a/include/net/udp_tunnel.h b/include/net/udp_tunnel.h
index d716214fe03df0a56266c22c7e8b42ba650e728b..a641392e70b0aa3d01826430640f3db6557d604b
100644
--- a/include/net/udp_tunnel.h
+++ b/include/net/udp_tunnel.h
@@ -75,7 +75,7 @@ typedef struct sk_buff
*(*udp_tunnel_gro_receive_t)(struct sock *sk,
struct list_head *head,
struct sk_buff *skb);
typedef int (*udp_tunnel_gro_complete_t)(struct sock *sk, struct sk_buff *skb,
- int nhoff);
+ int nhoff, int thoff);
struct udp_tunnel_sock_cfg {
void *sk_user_data; /* user data used by encap_rcv call back */
diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
index f001582345052f8c26e008058ae5f721f8bc224d..247704cf70aff1279c62507a940581e5049175b9
100644
--- a/net/8021q/vlan_core.c
+++ b/net/8021q/vlan_core.c
@@ -510,7 +510,7 @@ static struct sk_buff *vlan_gro_receive(struct
list_head *head,
return pp;
}
-static int vlan_gro_complete(struct sk_buff *skb, int nhoff)
+static int vlan_gro_complete(struct sk_buff *skb, int p_off, int nhoff)
{
struct vlan_hdr *vhdr = (struct vlan_hdr *)(skb->data + nhoff);
__be16 type = vhdr->h_vlan_encapsulated_proto;
@@ -521,7 +521,7 @@ static int vlan_gro_complete(struct sk_buff *skb, int nhoff)
if (ptype)
err = INDIRECT_CALL_INET(ptype->callbacks.gro_complete,
ipv6_gro_complete, inet_gro_complete,
- skb, nhoff + sizeof(*vhdr));
+ skb, p_off, nhoff + sizeof(*vhdr));
return err;
}
diff --git a/net/core/gro.c b/net/core/gro.c
index 0759277dc14ee65d0a5376d48694cc1cccaee959..07768055ecf25d426e7cd01551f32978a70e8379
100644
--- a/net/core/gro.c
+++ b/net/core/gro.c
@@ -254,7 +254,7 @@ static void napi_gro_complete(struct napi_struct
*napi, struct sk_buff *skb)
err = INDIRECT_CALL_INET(ptype->callbacks.gro_complete,
ipv6_gro_complete, inet_gro_complete,
- skb, 0);
+ skb, 0, 0);
break;
}
rcu_read_unlock();
diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index 2edc8b796a4e7326aa44128a0618e15b9aa817de..7515e6bcbb7d1e62fa0af2fd477bdea0284bfe40
100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -453,7 +453,7 @@ struct sk_buff *eth_gro_receive(struct list_head
*head, struct sk_buff *skb)
}
EXPORT_SYMBOL(eth_gro_receive);
-int eth_gro_complete(struct sk_buff *skb, int nhoff)
+int eth_gro_complete(struct sk_buff *skb, int p_off, int nhoff)
{
struct ethhdr *eh = (struct ethhdr *)(skb->data + nhoff);
__be16 type = eh->h_proto;
@@ -467,7 +467,7 @@ int eth_gro_complete(struct sk_buff *skb, int nhoff)
if (ptype != NULL)
err = INDIRECT_CALL_INET(ptype->callbacks.gro_complete,
ipv6_gro_complete, inet_gro_complete,
- skb, nhoff + sizeof(*eh));
+ skb, p_off, nhoff + sizeof(*eh));
return err;
}
diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 5daebdcbca326aa1fc042e1e1ff1e82a18bd283d..bef9b222f3c90487c7d44015efb23c08168cf164
100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -1640,7 +1640,7 @@ int inet_recv_error(struct sock *sk, struct
msghdr *msg, int len, int *addr_len)
}
EXPORT_SYMBOL(inet_recv_error);
-int inet_gro_complete(struct sk_buff *skb, int nhoff)
+int inet_gro_complete(struct sk_buff *skb, int prior_off, int nhoff)
{
struct iphdr *iph = (struct iphdr *)(skb->data + nhoff);
const struct net_offload *ops;
@@ -1666,17 +1666,17 @@ int inet_gro_complete(struct sk_buff *skb, int nhoff)
*/
err = INDIRECT_CALL_2(ops->callbacks.gro_complete,
tcp4_gro_complete, udp4_gro_complete,
- skb, nhoff + sizeof(*iph));
+ skb, nhoff, nhoff + sizeof(*iph));
out:
return err;
}
-static int ipip_gro_complete(struct sk_buff *skb, int nhoff)
+static int ipip_gro_complete(struct sk_buff *skb, int prior_off, int nhoff)
{
skb->encapsulation = 1;
skb_shinfo(skb)->gso_type |= SKB_GSO_IPXIP4;
- return inet_gro_complete(skb, nhoff);
+ return inet_gro_complete(skb, prior_off, nhoff);
}
int inet_ctl_sock_create(struct sock **sk, unsigned short family,
diff --git a/net/ipv4/fou_core.c b/net/ipv4/fou_core.c
index 0c41076e31edadd16f8e55ebc50f84db262a2f0d..ac4a6595d5cdf7e1e4b0eef417173d8d2d4ad76d
100644
--- a/net/ipv4/fou_core.c
+++ b/net/ipv4/fou_core.c
@@ -260,7 +260,7 @@ static struct sk_buff *fou_gro_receive(struct sock *sk,
}
static int fou_gro_complete(struct sock *sk, struct sk_buff *skb,
- int nhoff)
+ int p_off, int nhoff)
{
const struct net_offload __rcu **offloads;
u8 proto = fou_from_sock(sk)->protocol;
@@ -272,7 +272,7 @@ static int fou_gro_complete(struct sock *sk,
struct sk_buff *skb,
if (WARN_ON(!ops || !ops->callbacks.gro_complete))
goto out;
- err = ops->callbacks.gro_complete(skb, nhoff);
+ err = ops->callbacks.gro_complete(skb, p_off, nhoff);
skb_set_inner_mac_header(skb, nhoff);
@@ -445,7 +445,8 @@ static struct sk_buff *gue_gro_receive(struct sock *sk,
return pp;
}
-static int gue_gro_complete(struct sock *sk, struct sk_buff *skb, int nhoff)
+static int gue_gro_complete(struct sock *sk, struct sk_buff *skb,
+ int p_off, int nhoff)
{
struct guehdr *guehdr = (struct guehdr *)(skb->data + nhoff);
const struct net_offload __rcu **offloads;
@@ -480,7 +481,7 @@ static int gue_gro_complete(struct sock *sk,
struct sk_buff *skb, int nhoff)
if (WARN_ON(!ops || !ops->callbacks.gro_complete))
goto out;
- err = ops->callbacks.gro_complete(skb, nhoff + guehlen);
+ err = ops->callbacks.gro_complete(skb, p_off, nhoff + guehlen);
skb_set_inner_mac_header(skb, nhoff + guehlen);
diff --git a/net/ipv4/gre_offload.c b/net/ipv4/gre_offload.c
index 311e70bfce407a2cadaa33fbef9a3976375711f4..803a8498f3030ec80a0ed41313e314021416a207
100644
--- a/net/ipv4/gre_offload.c
+++ b/net/ipv4/gre_offload.c
@@ -233,7 +233,7 @@ static struct sk_buff *gre_gro_receive(struct
list_head *head,
return pp;
}
-static int gre_gro_complete(struct sk_buff *skb, int nhoff)
+static int gre_gro_complete(struct sk_buff *skb, int p_off, int nhoff)
{
struct gre_base_hdr *greh = (struct gre_base_hdr *)(skb->data + nhoff);
struct packet_offload *ptype;
@@ -253,7 +253,7 @@ static int gre_gro_complete(struct sk_buff *skb, int nhoff)
ptype = gro_find_complete_by_type(type);
if (ptype)
- err = ptype->callbacks.gro_complete(skb, nhoff + grehlen);
+ err = ptype->callbacks.gro_complete(skb, p_off, nhoff
+ grehlen);
skb_set_inner_mac_header(skb, nhoff + grehlen);
diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c
index 8311c38267b55ba97e59924c3c1c5b59f133fdcd..6f126f7d806d60e4d884c4f95b53b8e4bd9fbb8a
100644
--- a/net/ipv4/tcp_offload.c
+++ b/net/ipv4/tcp_offload.c
@@ -328,10 +328,10 @@ struct sk_buff *tcp4_gro_receive(struct
list_head *head, struct sk_buff *skb)
return tcp_gro_receive(head, skb);
}
-INDIRECT_CALLABLE_SCOPE int tcp4_gro_complete(struct sk_buff *skb, int thoff)
+INDIRECT_CALLABLE_SCOPE int tcp4_gro_complete(struct sk_buff *skb,
int nhoff, int thoff)
{
- const struct iphdr *iph = ip_hdr(skb);
- struct tcphdr *th = tcp_hdr(skb);
+ const struct iphdr *iph = (const struct iphdr *)(skb->data + nhoff);
+ struct tcphdr *th = (struct tcphdr *)(skb->data + thoff);
th->check = ~tcp_v4_check(skb->len - thoff, iph->saddr,
iph->daddr, 0);
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index a8acea17b4e5344d022ae8f8eb674d1a36f8035a..70a6d174855f76fb5dd289e3ef222a5158890e4f
100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -532,9 +532,10 @@ static inline struct sock
*__udp4_lib_lookup_skb(struct sk_buff *skb,
}
struct sock *udp4_lib_lookup_skb(const struct sk_buff *skb,
+ int nhoff,
__be16 sport, __be16 dport)
{
- const struct iphdr *iph = ip_hdr(skb);
+ const struct iphdr *iph = (const struct iphdr *)(skb->data + nhoff);
struct net *net = dev_net(skb->dev);
int iif, sdif;
diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index 6c95d28d0c4a7e56d587a986113b3711f8de964c..fa82a5a08688086415bae1d425d034f12311b0ab
100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -669,18 +669,18 @@ static int udp_gro_complete_segment(struct sk_buff *skb)
return 0;
}
-int udp_gro_complete(struct sk_buff *skb, int nhoff,
+int udp_gro_complete(struct sk_buff *skb, int nhoff, int thoff,
udp_lookup_t lookup)
{
- __be16 newlen = htons(skb->len - nhoff);
- struct udphdr *uh = (struct udphdr *)(skb->data + nhoff);
+ struct udphdr *uh = (struct udphdr *)(skb->data + thoff);
+ __be16 newlen = htons(skb->len - thoff);
struct sock *sk;
int err;
uh->len = newlen;
sk = INDIRECT_CALL_INET(lookup, udp6_lib_lookup_skb,
- udp4_lib_lookup_skb, skb, uh->source, uh->dest);
+ udp4_lib_lookup_skb, skb, nhoff,
uh->source, uh->dest);
if (sk && udp_sk(sk)->gro_complete) {
skb_shinfo(skb)->gso_type = uh->check ? SKB_GSO_UDP_TUNNEL_CSUM
: SKB_GSO_UDP_TUNNEL;
@@ -694,8 +694,8 @@ int udp_gro_complete(struct sk_buff *skb, int nhoff,
* functions to make them set up the inner offsets.
*/
skb->encapsulation = 1;
- err = udp_sk(sk)->gro_complete(sk, skb,
- nhoff + sizeof(struct udphdr));
+ err = udp_sk(sk)->gro_complete(sk, skb, nhoff,
+ thoff + sizeof(struct udphdr));
} else {
err = udp_gro_complete_segment(skb);
}
@@ -707,14 +707,14 @@ int udp_gro_complete(struct sk_buff *skb, int nhoff,
}
EXPORT_SYMBOL(udp_gro_complete);
-INDIRECT_CALLABLE_SCOPE int udp4_gro_complete(struct sk_buff *skb, int nhoff)
+INDIRECT_CALLABLE_SCOPE int udp4_gro_complete(struct sk_buff *skb,
int nhoff, int thoff)
{
- const struct iphdr *iph = ip_hdr(skb);
- struct udphdr *uh = (struct udphdr *)(skb->data + nhoff);
+ const struct iphdr *iph = (const struct iphdr *)(skb->data + nhoff);
+ struct udphdr *uh = (struct udphdr *)(skb->data + thoff);
/* do fraglist only if there is no outer UDP encap (or we
already processed it) */
if (NAPI_GRO_CB(skb)->is_flist && !NAPI_GRO_CB(skb)->encap_mark) {
- uh->len = htons(skb->len - nhoff);
+ uh->len = htons(skb->len - thoff);
skb_shinfo(skb)->gso_type |= (SKB_GSO_FRAGLIST|SKB_GSO_UDP_L4);
skb_shinfo(skb)->gso_segs = NAPI_GRO_CB(skb)->count;
@@ -731,10 +731,10 @@ INDIRECT_CALLABLE_SCOPE int
udp4_gro_complete(struct sk_buff *skb, int nhoff)
}
if (uh->check)
- uh->check = ~udp_v4_check(skb->len - nhoff, iph->saddr,
+ uh->check = ~udp_v4_check(skb->len - thoff, iph->saddr,
iph->daddr, 0);
- return udp_gro_complete(skb, nhoff, udp4_lib_lookup_skb);
+ return udp_gro_complete(skb, nhoff, thoff, udp4_lib_lookup_skb);
}
static const struct net_offload udpv4_offload = {
diff --git a/net/ipv6/ip6_offload.c b/net/ipv6/ip6_offload.c
index cca64c7809bee9a0360cbfab6a645d3f8d2ffea3..e3a05b84c76ab9d55afafa94a53130b3b0d6c7f7
100644
--- a/net/ipv6/ip6_offload.c
+++ b/net/ipv6/ip6_offload.c
@@ -346,12 +346,14 @@ static struct sk_buff *ip4ip6_gro_receive(struct
list_head *head,
return inet_gro_receive(head, skb);
}
-INDIRECT_CALLABLE_SCOPE int ipv6_gro_complete(struct sk_buff *skb, int nhoff)
+INDIRECT_CALLABLE_SCOPE int ipv6_gro_complete(struct sk_buff *skb,
+ int p_off, int nhoff)
{
const struct net_offload *ops;
struct ipv6hdr *iph;
int err = -ENOSYS;
u32 payload_len;
+ int nhlen;
if (skb->encapsulation) {
skb_set_inner_protocol(skb, cpu_to_be16(ETH_P_IPV6));
@@ -387,36 +389,36 @@ INDIRECT_CALLABLE_SCOPE int
ipv6_gro_complete(struct sk_buff *skb, int nhoff)
iph->payload_len = htons(payload_len);
}
- nhoff += sizeof(*iph) + ipv6_exthdrs_len(iph, &ops);
- if (WARN_ON(!ops || !ops->callbacks.gro_complete))
+ nhlen = sizeof(*iph) + ipv6_exthdrs_len(iph, &ops);
+ if (WARN_ON_ONCE(!ops || !ops->callbacks.gro_complete))
goto out;
err = INDIRECT_CALL_L4(ops->callbacks.gro_complete, tcp6_gro_complete,
- udp6_gro_complete, skb, nhoff);
+ udp6_gro_complete, skb, nhoff, nhoff + nhlen);
out:
return err;
}
-static int sit_gro_complete(struct sk_buff *skb, int nhoff)
+static int sit_gro_complete(struct sk_buff *skb, int p_off, int nhoff)
{
skb->encapsulation = 1;
skb_shinfo(skb)->gso_type |= SKB_GSO_IPXIP4;
- return ipv6_gro_complete(skb, nhoff);
+ return ipv6_gro_complete(skb, p_off, nhoff);
}
-static int ip6ip6_gro_complete(struct sk_buff *skb, int nhoff)
+static int ip6ip6_gro_complete(struct sk_buff *skb, int p_off, int nhoff)
{
skb->encapsulation = 1;
skb_shinfo(skb)->gso_type |= SKB_GSO_IPXIP6;
- return ipv6_gro_complete(skb, nhoff);
+ return ipv6_gro_complete(skb, p_off, nhoff);
}
-static int ip4ip6_gro_complete(struct sk_buff *skb, int nhoff)
+static int ip4ip6_gro_complete(struct sk_buff *skb, int p_off, int nhoff)
{
skb->encapsulation = 1;
skb_shinfo(skb)->gso_type |= SKB_GSO_IPXIP6;
- return inet_gro_complete(skb, nhoff);
+ return inet_gro_complete(skb, p_off, nhoff);
}
static struct packet_offload ipv6_packet_offload __read_mostly = {
diff --git a/net/ipv6/tcpv6_offload.c b/net/ipv6/tcpv6_offload.c
index bf0c957e4b5eaaabc0ac3a7e55c7de6608cec156..5043d2ff34eb8a7f9aeb7db6b4347a0f976b8502
100644
--- a/net/ipv6/tcpv6_offload.c
+++ b/net/ipv6/tcpv6_offload.c
@@ -27,10 +27,11 @@ struct sk_buff *tcp6_gro_receive(struct list_head
*head, struct sk_buff *skb)
return tcp_gro_receive(head, skb);
}
-INDIRECT_CALLABLE_SCOPE int tcp6_gro_complete(struct sk_buff *skb, int thoff)
+INDIRECT_CALLABLE_SCOPE int tcp6_gro_complete(struct sk_buff *skb,
+ int nhoff, int thoff)
{
- const struct ipv6hdr *iph = ipv6_hdr(skb);
- struct tcphdr *th = tcp_hdr(skb);
+ const struct ipv6hdr *iph = (const struct ipv6hdr *)(skb->data + nhoff);
+ struct tcphdr *th = (struct tcphdr *)(skb->data + thoff);
th->check = ~tcp_v6_check(skb->len - thoff, &iph->saddr,
&iph->daddr, 0);
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index 3f2249b4cd5f6a594dd9768e29f20f0d9a57faed..400243c89d8234ff98b3991c049e83237fa6686f
100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -273,9 +273,10 @@ static struct sock *__udp6_lib_lookup_skb(struct
sk_buff *skb,
}
struct sock *udp6_lib_lookup_skb(const struct sk_buff *skb,
+ int nhoff,
__be16 sport, __be16 dport)
{
- const struct ipv6hdr *iph = ipv6_hdr(skb);
+ const struct ipv6hdr *iph = (const struct ipv6hdr *)(skb->data + nhoff);
struct net *net = dev_net(skb->dev);
int iif, sdif;
diff --git a/net/ipv6/udp_offload.c b/net/ipv6/udp_offload.c
index 6b95ba241ebe2af7e5f2760d8a9c1d78f08579c5..7bf0fb609451586ab9fb1de955d1ae2ea2dbee64
100644
--- a/net/ipv6/udp_offload.c
+++ b/net/ipv6/udp_offload.c
@@ -162,14 +162,14 @@ struct sk_buff *udp6_gro_receive(struct
list_head *head, struct sk_buff *skb)
return NULL;
}
-INDIRECT_CALLABLE_SCOPE int udp6_gro_complete(struct sk_buff *skb, int nhoff)
+INDIRECT_CALLABLE_SCOPE int udp6_gro_complete(struct sk_buff *skb,
int nhoff, int thoff)
{
- const struct ipv6hdr *ipv6h = ipv6_hdr(skb);
- struct udphdr *uh = (struct udphdr *)(skb->data + nhoff);
+ const struct ipv6hdr *ipv6h = (const struct ipv6hdr
*)(skb->data + nhoff);
+ struct udphdr *uh = (struct udphdr *)(skb->data + thoff);
/* do fraglist only if there is no outer UDP encap (or we
already processed it) */
if (NAPI_GRO_CB(skb)->is_flist && !NAPI_GRO_CB(skb)->encap_mark) {
- uh->len = htons(skb->len - nhoff);
+ uh->len = htons(skb->len - thoff);
skb_shinfo(skb)->gso_type |= (SKB_GSO_FRAGLIST|SKB_GSO_UDP_L4);
skb_shinfo(skb)->gso_segs = NAPI_GRO_CB(skb)->count;
@@ -186,10 +186,10 @@ INDIRECT_CALLABLE_SCOPE int
udp6_gro_complete(struct sk_buff *skb, int nhoff)
}
if (uh->check)
- uh->check = ~udp_v6_check(skb->len - nhoff, &ipv6h->saddr,
+ uh->check = ~udp_v6_check(skb->len - thoff, &ipv6h->saddr,
&ipv6h->daddr, 0);
- return udp_gro_complete(skb, nhoff, udp6_lib_lookup_skb);
+ return udp_gro_complete(skb, nhoff, thoff, udp6_lib_lookup_skb);
}
static const struct net_offload udpv6_offload = {
On Thu, Feb 22, 2024 at 10:05 PM Richard Gobert
<[email protected]> wrote:
>
> Commits a602456 ("udp: Add GRO functions to UDP socket") and 57c67ff ("udp:
> additional GRO support") introduce incorrect usage of {ip,ipv6}_hdr in the
> complete phase of gro. The functions always return skb->network_header,
> which in the case of encapsulated packets at the gro complete phase, is
> always set to the innermost L3 of the packet. That means that calling
> {ip,ipv6}_hdr for skbs which completed the GRO receive phase (both in
> gro_list and *_gro_complete) when parsing an encapsulated packet's _outer_
> L3/L4 may return an unexpected value.
>
> This incorrect usage leads to a bug in GRO's UDP socket lookup.
> udp{4,6}_lib_lookup_skb functions use ip_hdr/ipv6_hdr respectively. These
> *_hdr functions return network_header which will point to the innermost L3,
> resulting in the wrong offset being used in __udp{4,6}_lib_lookup with
> encapsulated packets.
>
> Reproduction example:
>
> Endpoint configuration example (fou + local address bind)
>
> # ip fou add port 6666 ipproto 4
> # ip link add name tun1 type ipip remote 2.2.2.1 local 2.2.2.2 encap fou encap-dport 5555 encap-sport 6666 mode ipip
> # ip link set tun1 up
> # ip a add 1.1.1.2/24 dev tun1
>
> Netperf TCP_STREAM result on net-next before patch is applied:
>
> net-next main, GRO enabled:
> $ netperf -H 1.1.1.2 -t TCP_STREAM -l 5
> Recv Send Send
> Socket Socket Message Elapsed
> Size Size Size Time Throughput
> bytes bytes bytes secs. 10^6bits/sec
>
> 131072 16384 16384 5.28 2.37
>
> net-next main, GRO disabled:
> $ netperf -H 1.1.1.2 -t TCP_STREAM -l 5
> Recv Send Send
> Socket Socket Message Elapsed
> Size Size Size Time Throughput
> bytes bytes bytes secs. 10^6bits/sec
>
> 131072 16384 16384 5.01 2745.06
>
> patch applied, GRO enabled:
> $ netperf -H 1.1.1.2 -t TCP_STREAM -l 5
> Recv Send Send
> Socket Socket Message Elapsed
> Size Size Size Time Throughput
> bytes bytes bytes secs. 10^6bits/sec
>
> 131072 16384 16384 5.01 2877.38
>
> This patch fixes this bug and prevents similar future misuse of
> network_header by setting network_header and inner_network_header to their
> respective values during the receive phase of GRO. This results in
> more coherent {inner_,}network_header values for every skb in gro_list,
> which also means there's no need to set/fix these values before passing
> the packet forward.
>
> network_header is already set in dev_gro_receive and under encapsulation we
> set inner_network_header. *_gro_complete functions use a new helper
> function - skb_gro_complete_network_header, which returns the
> network_header/inner_network_header offset during the GRO complete phase,
> depending on skb->encapsulation.
>
> Fixes: 57c67ff4bd92 ("udp: additional GRO support")
> Signed-off-by: Richard Gobert <[email protected]>
> ---
> include/net/gro.h | 14 +++++++++++++-
> net/8021q/vlan_core.c | 3 +++
> net/ipv4/af_inet.c | 8 ++++----
> net/ipv4/tcp_offload.c | 2 +-
> net/ipv4/udp_offload.c | 2 +-
> net/ipv6/ip6_offload.c | 11 +++++------
> net/ipv6/tcpv6_offload.c | 2 +-
> net/ipv6/udp_offload.c | 2 +-
> 8 files changed, 29 insertions(+), 15 deletions(-)
>
> diff --git a/include/net/gro.h b/include/net/gro.h
> index b435f0ddbf64..89502a7e35ed 100644
> --- a/include/net/gro.h
> +++ b/include/net/gro.h
> @@ -177,10 +177,22 @@ static inline void *skb_gro_header(struct sk_buff *skb,
> return ptr;
> }
>
> +static inline int skb_gro_network_offset(struct sk_buff *skb)
> +{
> + return NAPI_GRO_CB(skb)->encap_mark ? skb_inner_network_offset(skb) :
> + skb_network_offset(skb);
> +}
> +
> static inline void *skb_gro_network_header(struct sk_buff *skb)
> {
> return (NAPI_GRO_CB(skb)->frag0 ?: skb->data) +
> - skb_network_offset(skb);
> + skb_gro_network_offset(skb);
> +}
> +
> +static inline void *skb_gro_complete_network_header(struct sk_buff *skb)
> +{
> + return skb->encapsulation ? skb_inner_network_header(skb) :
> + skb_network_header(skb);
> }
>
> static inline __wsum inet_gro_compute_pseudo(struct sk_buff *skb, int proto)
> diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
> index f00158234505..8bc871397e47 100644
> --- a/net/8021q/vlan_core.c
> +++ b/net/8021q/vlan_core.c
> @@ -478,6 +478,9 @@ static struct sk_buff *vlan_gro_receive(struct list_head *head,
> if (unlikely(!vhdr))
> goto out;
>
> + if (!NAPI_GRO_CB(skb)->encap_mark)
> + skb_set_network_header(skb, hlen);
> +
> type = vhdr->h_vlan_encapsulated_proto;
>
> ptype = gro_find_receive_by_type(type);
> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
> index 835f4f9d98d2..c0f3c162bf73 100644
> --- a/net/ipv4/af_inet.c
> +++ b/net/ipv4/af_inet.c
> @@ -1564,7 +1564,9 @@ struct sk_buff *inet_gro_receive(struct list_head *head, struct sk_buff *skb)
>
> NAPI_GRO_CB(skb)->is_atomic = !!(iph->frag_off & htons(IP_DF));
> NAPI_GRO_CB(skb)->flush |= flush;
> - skb_set_network_header(skb, off);
> + if (NAPI_GRO_CB(skb)->encap_mark)
> + skb_set_inner_network_header(skb, off);
> +
> /* The above will be needed by the transport layer if there is one
> * immediately following this IP hdr.
> */
> @@ -1643,10 +1645,8 @@ int inet_gro_complete(struct sk_buff *skb, int nhoff)
> int proto = iph->protocol;
> int err = -ENOSYS;
>
> - if (skb->encapsulation) {
> + if (skb->encapsulation)
> skb_set_inner_protocol(skb, cpu_to_be16(ETH_P_IP));
> - skb_set_inner_network_header(skb, nhoff);
> - }
>
> iph_set_totlen(iph, skb->len - nhoff);
> csum_replace2(&iph->check, totlen, iph->tot_len);
> diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c
> index 8311c38267b5..8bbcd3f502ac 100644
> --- a/net/ipv4/tcp_offload.c
> +++ b/net/ipv4/tcp_offload.c
> @@ -330,7 +330,7 @@ struct sk_buff *tcp4_gro_receive(struct list_head *head, struct sk_buff *skb)
>
> INDIRECT_CALLABLE_SCOPE int tcp4_gro_complete(struct sk_buff *skb, int thoff)
> {
> - const struct iphdr *iph = ip_hdr(skb);
> + const struct iphdr *iph = skb_gro_complete_network_header(skb);
> struct tcphdr *th = tcp_hdr(skb);
>
> th->check = ~tcp_v4_check(skb->len - thoff, iph->saddr,
> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> index 6c95d28d0c4a..7f59cede67f5 100644
> --- a/net/ipv4/udp_offload.c
> +++ b/net/ipv4/udp_offload.c
> @@ -709,7 +709,7 @@ EXPORT_SYMBOL(udp_gro_complete);
>
> INDIRECT_CALLABLE_SCOPE int udp4_gro_complete(struct sk_buff *skb, int nhoff)
> {
> - const struct iphdr *iph = ip_hdr(skb);
> + const struct iphdr *iph = skb_gro_complete_network_header(skb);
> struct udphdr *uh = (struct udphdr *)(skb->data + nhoff);
>
> /* do fraglist only if there is no outer UDP encap (or we already processed it) */
> diff --git a/net/ipv6/ip6_offload.c b/net/ipv6/ip6_offload.c
> index cca64c7809be..db7e3db587b9 100644
> --- a/net/ipv6/ip6_offload.c
> +++ b/net/ipv6/ip6_offload.c
> @@ -67,7 +67,7 @@ static int ipv6_gro_pull_exthdrs(struct sk_buff *skb, int off, int proto)
> off += len;
> }
>
> - skb_gro_pull(skb, off - skb_network_offset(skb));
> + skb_gro_pull(skb, off - skb_gro_network_offset(skb));
> return proto;
> }
>
> @@ -236,7 +236,8 @@ INDIRECT_CALLABLE_SCOPE struct sk_buff *ipv6_gro_receive(struct list_head *head,
> if (unlikely(!iph))
> goto out;
>
> - skb_set_network_header(skb, off);
> + if (NAPI_GRO_CB(skb)->encap_mark)
> + skb_set_inner_network_header(skb, off);
>
> flush += ntohs(iph->payload_len) != skb->len - hlen;
>
> @@ -259,7 +260,7 @@ INDIRECT_CALLABLE_SCOPE struct sk_buff *ipv6_gro_receive(struct list_head *head,
> NAPI_GRO_CB(skb)->proto = proto;
>
> flush--;
> - nlen = skb_network_header_len(skb);
> + nlen = skb_gro_offset(skb) - off;
>
> list_for_each_entry(p, head, list) {
> const struct ipv6hdr *iph2;
> @@ -353,10 +354,8 @@ INDIRECT_CALLABLE_SCOPE int ipv6_gro_complete(struct sk_buff *skb, int nhoff)
> int err = -ENOSYS;
> u32 payload_len;
>
> - if (skb->encapsulation) {
> + if (skb->encapsulation)
> skb_set_inner_protocol(skb, cpu_to_be16(ETH_P_IPV6));
> - skb_set_inner_network_header(skb, nhoff);
> - }
>
> payload_len = skb->len - nhoff - sizeof(*iph);
> if (unlikely(payload_len > IPV6_MAXPLEN)) {
> diff --git a/net/ipv6/tcpv6_offload.c b/net/ipv6/tcpv6_offload.c
> index bf0c957e4b5e..79eeaced2834 100644
> --- a/net/ipv6/tcpv6_offload.c
> +++ b/net/ipv6/tcpv6_offload.c
> @@ -29,7 +29,7 @@ struct sk_buff *tcp6_gro_receive(struct list_head *head, struct sk_buff *skb)
>
> INDIRECT_CALLABLE_SCOPE int tcp6_gro_complete(struct sk_buff *skb, int thoff)
> {
> - const struct ipv6hdr *iph = ipv6_hdr(skb);
> + const struct ipv6hdr *iph = skb_gro_complete_network_header(skb);
> struct tcphdr *th = tcp_hdr(skb);
>
> th->check = ~tcp_v6_check(skb->len - thoff, &iph->saddr,
> diff --git a/net/ipv6/udp_offload.c b/net/ipv6/udp_offload.c
> index 6b95ba241ebe..897caa2e39fb 100644
> --- a/net/ipv6/udp_offload.c
> +++ b/net/ipv6/udp_offload.c
> @@ -164,7 +164,7 @@ struct sk_buff *udp6_gro_receive(struct list_head *head, struct sk_buff *skb)
>
> INDIRECT_CALLABLE_SCOPE int udp6_gro_complete(struct sk_buff *skb, int nhoff)
> {
> - const struct ipv6hdr *ipv6h = ipv6_hdr(skb);
> + const struct ipv6hdr *ipv6h = skb_gro_complete_network_header(skb);
> struct udphdr *uh = (struct udphdr *)(skb->data + nhoff);
My intuition is that this patch has a high cost for normal GRO processing.
SW-GRO is already a bottleneck on ARM cores in smart NICS.
I would suggest instead using parameters to give both the nhoff and thoff values
this would avoid many conditionals in the fast path.
->
INDIRECT_CALLABLE_SCOPE int udp6_gro_complete(struct sk_buff *skb, int
nhoff, int thoff)
{
const struct ipv6hdr *ipv6h = (const struct ipv6hdr *)(skb->data + nhoff);
struct udphdr *uh = (struct udphdr *)(skb->data + thoff);
..
}
INDIRECT_CALLABLE_SCOPE int tcp6_gro_complete(struct sk_buff *skb, int
nhoff, int thoff)
{
const struct ipv6hdr *iph = (const struct ipv6hdr *)(skb->data + nhoff);
struct tcphdr *th = (struct tcphdr *)(skb->data + thoff);
Why storing in skb fields things that really could be propagated more
efficiently as function parameters ?
Eric Dumazet wrote:
> On Thu, Feb 29, 2024 at 2:22 PM Richard Gobert <[email protected]> wrote:
>>
>>
>>
>> Eric Dumazet wrote:
>>>
>>> My intuition is that this patch has a high cost for normal GRO processing.
>>> SW-GRO is already a bottleneck on ARM cores in smart NICS.
>>>
>>> I would suggest instead using parameters to give both the nhoff and thoff values
>>> this would avoid many conditionals in the fast path.
>>>
>>> ->
>>>
>>> INDIRECT_CALLABLE_SCOPE int udp6_gro_complete(struct sk_buff *skb, int
>>> nhoff, int thoff)
>>> {
>>> const struct ipv6hdr *ipv6h = (const struct ipv6hdr *)(skb->data + nhoff);
>>> struct udphdr *uh = (struct udphdr *)(skb->data + thoff);
>>> ...
>>> }
>>>
>>> INDIRECT_CALLABLE_SCOPE int tcp6_gro_complete(struct sk_buff *skb, int
>>> nhoff, int thoff)
>>> {
>>> const struct ipv6hdr *iph = (const struct ipv6hdr *)(skb->data + nhoff);
>>> struct tcphdr *th = (struct tcphdr *)(skb->data + thoff);
>>>
>>> Why storing in skb fields things that really could be propagated more
>>> efficiently as function parameters ?
>>
>> Hi Eric,
>> Thanks for the review!
>>
>> I agree, the conditionals could be a problem and are actually not needed.
>> The third commit in this patch series introduces an optimisation for
>> ipv6/ipv4 using the correct {inner_}network_header. We can remove the
>> conditionals; I thought about multiple ways to do so. First, remove the
>> conditional in skb_gro_network_offset:
>>
>> static inline int skb_gro_network_offset(const struct sk_buff *skb)
>> {
>> const u32 mask = NAPI_GRO_CB(skb)->encap_mark - 1;
>> return (skb_network_offset(skb) & mask) | (skb_inner_network_offset(skb) & ~mask);
>> }
>
> I was trying to say that we do not need all these helpers, storing
> state in NAPI_GRO_CB(skb),
> dirtying cache lines...
>
> Ideally, the skb network/transport/... headers could be set at the
> last stage, in gro_complete(big_gro_skb),
> instead of doing this for each segment.
>
> All the gro_receive() could be much faster by using additional
> parameters (nhoff, thoff)
>
> skb_gro_offset() could be replaced by the current offset (nhoff or
> other name), passed as a parameter.
>
> Here is a WIP for gro_complete() step, this looks large but this is
> only adding a 2nd 'offset' parameter
>
> Prior offset (typically network offset), called p_off
> Old argument nhoff, (renamed thoff if that makes sense), pointing to
> the current offset.
>
You're right, it seemed to me like a broad change but it is mainly
cosmetic. I'll finish your version and submit it to fix the bug.
I still believe that setting inner_network_header is a valuable change.
For example, although skb_gro_network_offset is used - setting it in
encapsulation protocol functions (such as ipip_gro_receive) allow us to
remove conditionals from {ipv6,inet}_gro_receive gro_list loop and remove
flush_id from napi_gro_cb as written in the 3rd commit.
What are your thoughts about it as a separate patch?