[RFC PATCH net-next v6 1/2] net: pppoe: implement GRO/GSO support

[RFC PATCH net-next v6 1/2] net: pppoe: implement GRO/GSO support

[Qingfang Deng]

From: Felix Fietkau <nbd@nbd.name>

Only handles packets where the pppoe header length field matches the exact
packet length. Significantly improves rx throughput.

When running NAT traffic through a MediaTek MT7621 devices from a host
behind PPPoE to a host directly connected via ethernet, the TCP throughput
that the device is able to handle improves from ~130 Mbit/s to ~630 Mbit/s,
using fraglist GRO.

Signed-off-by: Felix Fietkau <nbd@nbd.name>
Signed-off-by: Qingfang Deng <dqfext@gmail.com>
---
v6:
 - avoid phdr->length field overflow 
 - restore skb_is_gso() check
 - do not register GRO if INET=n
 - do not check for PPP_IPV6 if IPV6=n
 - tail call gro_complete
 https://lore.kernel.org/netdev/20260305013852.3769-1-dqfext@gmail.com/

---
 drivers/net/ppp/pppoe.c | 166 +++++++++++++++++++++++++++++++++++++++-
 net/ipv4/af_inet.c      |   2 +
 net/ipv6/ip6_offload.c  |   2 +
 3 files changed, 169 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
index 1ac61c273b28..7522a8ce52fc 100644
--- a/drivers/net/ppp/pppoe.c
+++ b/drivers/net/ppp/pppoe.c
@@ -77,6 +77,7 @@
 #include <net/net_namespace.h>
 #include <net/netns/generic.h>
 #include <net/sock.h>
+#include <net/gro.h>
 
 #include <linux/uaccess.h>
 
@@ -403,7 +404,7 @@ static int pppoe_rcv(struct sk_buff *skb, struct net_device *dev,
 	if (skb->len < len)
 		goto drop;
 
-	if (pskb_trim_rcsum(skb, len))
+	if (!skb_is_gso(skb) && pskb_trim_rcsum(skb, len))
 		goto drop;
 
 	ph = pppoe_hdr(skb);
@@ -1097,6 +1098,165 @@ static struct pernet_operations pppoe_net_ops = {
 	.size = sizeof(struct pppoe_net),
 };
 
+static u16
+compare_pppoe_header(const struct pppoe_hdr *phdr,
+		     const struct pppoe_hdr *phdr2)
+{
+	__be16 proto = *(const __be16 *)(phdr + 1);
+	__be16 proto2 = *(const __be16 *)(phdr2 + 1);
+
+	return (__force u16)((phdr->sid ^ phdr2->sid) | (proto ^ proto2));
+}
+
+static __be16 pppoe_hdr_proto(const struct pppoe_hdr *phdr)
+{
+	__be16 proto = *(const __be16 *)(phdr + 1);
+
+	switch (proto) {
+	case cpu_to_be16(PPP_IP):
+		return cpu_to_be16(ETH_P_IP);
+#if IS_ENABLED(CONFIG_IPV6)
+	case cpu_to_be16(PPP_IPV6):
+		return cpu_to_be16(ETH_P_IPV6);
+#endif
+	default:
+		return 0;
+	}
+}
+
+static struct sk_buff *pppoe_gro_receive(struct list_head *head,
+					 struct sk_buff *skb)
+{
+	const struct packet_offload *ptype;
+	unsigned int hlen, off_pppoe;
+	const struct pppoe_hdr *phdr;
+	struct sk_buff *pp = NULL;
+	struct sk_buff *p;
+	int flush = 1;
+	__be16 type;
+
+	off_pppoe = skb_gro_offset(skb);
+	hlen = off_pppoe + PPPOE_SES_HLEN;
+	phdr = skb_gro_header(skb, hlen, off_pppoe);
+	if (unlikely(!phdr))
+		goto out;
+
+	/* filter for session packets (type:1, ver:1, code:0) */
+	if (*(const __be16 *)phdr != cpu_to_be16(0x1100))
+		goto out;
+
+	/* ignore packets with padding or invalid length */
+	if (skb_gro_len(skb) != be16_to_cpu(phdr->length) + sizeof(*phdr))
+		goto out;
+
+	type = pppoe_hdr_proto(phdr);
+	ptype = gro_find_receive_by_type(type);
+	if (!ptype)
+		goto out;
+
+	flush = 0;
+
+	list_for_each_entry(p, head, list) {
+		const struct pppoe_hdr *phdr2;
+
+		if (!NAPI_GRO_CB(p)->same_flow)
+			continue;
+
+		phdr2 = (const struct pppoe_hdr *)(p->data + off_pppoe);
+		if (compare_pppoe_header(phdr, phdr2))
+			NAPI_GRO_CB(p)->same_flow = 0;
+	}
+
+	skb_gro_pull(skb, PPPOE_SES_HLEN);
+	skb_gro_postpull_rcsum(skb, phdr, PPPOE_SES_HLEN);
+
+	pp = indirect_call_gro_receive_inet(ptype->callbacks.gro_receive,
+					    ipv6_gro_receive, inet_gro_receive,
+					    head, skb);
+
+out:
+	skb_gro_flush_final(skb, pp, flush);
+
+	return pp;
+}
+
+static int pppoe_gro_complete(struct sk_buff *skb, int nhoff)
+{
+	struct pppoe_hdr *phdr = (struct pppoe_hdr *)(skb->data + nhoff);
+	__be16 type = pppoe_hdr_proto(phdr);
+	struct packet_offload *ptype;
+	unsigned int len;
+
+	ptype = gro_find_complete_by_type(type);
+	if (!ptype)
+		return -ENOENT;
+
+	len = skb->len - (nhoff + sizeof(*phdr));
+	len = min(len, 0xFFFFU);
+	phdr->length = cpu_to_be16(len);
+
+	return INDIRECT_CALL_INET(ptype->callbacks.gro_complete,
+				  ipv6_gro_complete, inet_gro_complete,
+				  skb, nhoff + PPPOE_SES_HLEN);
+}
+
+static struct sk_buff *pppoe_gso_segment(struct sk_buff *skb,
+					 netdev_features_t features)
+{
+	unsigned int pppoe_hlen = sizeof(struct pppoe_hdr) + 2;
+	struct sk_buff *segs = ERR_PTR(-EINVAL);
+	u16 mac_offset = skb->mac_header;
+	struct packet_offload *ptype;
+	u16 mac_len = skb->mac_len;
+	struct pppoe_hdr *phdr;
+	__be16 orig_type, type;
+	int len, nhoff;
+
+	skb_reset_network_header(skb);
+	nhoff = skb_network_header(skb) - skb_mac_header(skb);
+
+	if (unlikely(!pskb_may_pull(skb, pppoe_hlen)))
+		goto out;
+
+	phdr = (struct pppoe_hdr *)skb_network_header(skb);
+	type = pppoe_hdr_proto(phdr);
+	ptype = gro_find_complete_by_type(type);
+	if (!ptype)
+		goto out;
+
+	orig_type = skb->protocol;
+	__skb_pull(skb, pppoe_hlen);
+	segs = ptype->callbacks.gso_segment(skb, features);
+	if (IS_ERR_OR_NULL(segs)) {
+		skb_gso_error_unwind(skb, orig_type, pppoe_hlen, mac_offset,
+				     mac_len);
+		goto out;
+	}
+
+	skb = segs;
+	do {
+		phdr = (struct pppoe_hdr *)(skb_mac_header(skb) + nhoff);
+		len = skb->len - (nhoff + sizeof(*phdr));
+		phdr->length = cpu_to_be16(len);
+		skb->network_header = (u8 *)phdr - skb->head;
+		skb->protocol = orig_type;
+		skb_reset_mac_len(skb);
+	} while ((skb = skb->next));
+
+out:
+	return segs;
+}
+
+static struct packet_offload pppoe_packet_offload __read_mostly = {
+	.type = cpu_to_be16(ETH_P_PPP_SES),
+	.priority = 20,
+	.callbacks = {
+		.gro_receive = pppoe_gro_receive,
+		.gro_complete = pppoe_gro_complete,
+		.gso_segment = pppoe_gso_segment,
+	},
+};
+
 static int __init pppoe_init(void)
 {
 	int err;
@@ -1113,6 +1273,8 @@ static int __init pppoe_init(void)
 	if (err)
 		goto out_unregister_pppoe_proto;
 
+	if (IS_ENABLED(CONFIG_INET))
+		dev_add_offload(&pppoe_packet_offload);
 	dev_add_pack(&pppoes_ptype);
 	dev_add_pack(&pppoed_ptype);
 	register_netdevice_notifier(&pppoe_notifier);
@@ -1132,6 +1294,8 @@ static void __exit pppoe_exit(void)
 	unregister_netdevice_notifier(&pppoe_notifier);
 	dev_remove_pack(&pppoed_ptype);
 	dev_remove_pack(&pppoes_ptype);
+	if (IS_ENABLED(CONFIG_INET))
+		dev_remove_offload(&pppoe_packet_offload);
 	unregister_pppox_proto(PX_PROTO_OE);
 	proto_unregister(&pppoe_sk_proto);
 	unregister_pernet_device(&pppoe_net_ops);
diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index f98e46ae3e30..e6a0c18cc189 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -1539,6 +1539,7 @@ struct sk_buff *inet_gro_receive(struct list_head *head, struct sk_buff *skb)
 
 	return pp;
 }
+EXPORT_INDIRECT_CALLABLE(inet_gro_receive);
 
 static struct sk_buff *ipip_gro_receive(struct list_head *head,
 					struct sk_buff *skb)
@@ -1624,6 +1625,7 @@ int inet_gro_complete(struct sk_buff *skb, int nhoff)
 out:
 	return err;
 }
+EXPORT_INDIRECT_CALLABLE(inet_gro_complete);
 
 static int ipip_gro_complete(struct sk_buff *skb, int nhoff)
 {
diff --git a/net/ipv6/ip6_offload.c b/net/ipv6/ip6_offload.c
index bd7f780e37a5..32ba4739cef9 100644
--- a/net/ipv6/ip6_offload.c
+++ b/net/ipv6/ip6_offload.c
@@ -297,6 +297,7 @@ INDIRECT_CALLABLE_SCOPE struct sk_buff *ipv6_gro_receive(struct list_head *head,
 
 	return pp;
 }
+EXPORT_INDIRECT_CALLABLE(ipv6_gro_receive);
 
 static struct sk_buff *sit_ip6ip6_gro_receive(struct list_head *head,
 					      struct sk_buff *skb)
@@ -359,6 +360,7 @@ INDIRECT_CALLABLE_SCOPE int ipv6_gro_complete(struct sk_buff *skb, int nhoff)
 out:
 	return err;
 }
+EXPORT_INDIRECT_CALLABLE(ipv6_gro_complete);
 
 static int sit_gro_complete(struct sk_buff *skb, int nhoff)
 {
-- 
2.43.0

[RFC PATCH net-next v6 2/2] selftests: net: test PPPoE packets in gro.sh

[Qingfang Deng]

Add PPPoE test-cases to the GRO selftest.

Signed-off-by: Qingfang Deng <dqfext@gmail.com>
---
v6: new patch

I'm not sure if I should include all the existing IPv4/v6 tests for
PPPoE. There are tests that hardcode ETH_HLEN as the NH offset, which
are meant to test the L3 protocol, not the underlying protocol, and
changing all of them seems too invasive.

Comments welcome, thanks.
---
 tools/testing/selftests/drivers/net/config |  2 +
 tools/testing/selftests/drivers/net/gro.py | 25 +++++-
 tools/testing/selftests/net/lib/gro.c      | 97 ++++++++++++++++------
 3 files changed, 98 insertions(+), 26 deletions(-)

diff --git a/tools/testing/selftests/drivers/net/config b/tools/testing/selftests/drivers/net/config
index 77ccf83d87e0..caaba209ba3e 100644
--- a/tools/testing/selftests/drivers/net/config
+++ b/tools/testing/selftests/drivers/net/config
@@ -7,4 +7,6 @@ CONFIG_NETCONSOLE=m
 CONFIG_NETCONSOLE_DYNAMIC=y
 CONFIG_NETCONSOLE_EXTENDED_LOG=y
 CONFIG_NETDEVSIM=m
+CONFIG_PPP=y
+CONFIG_PPPOE=y
 CONFIG_XDP_SOCKETS=y
diff --git a/tools/testing/selftests/drivers/net/gro.py b/tools/testing/selftests/drivers/net/gro.py
index 70709bf670c7..eb8306a8f4c4 100755
--- a/tools/testing/selftests/drivers/net/gro.py
+++ b/tools/testing/selftests/drivers/net/gro.py
@@ -186,8 +186,17 @@ def _run_gro_bin(cfg, test_name, protocol=None, num_flows=None,
 
     dmac = _resolve_dmac(cfg, ipver)
 
-    base_args = [
-        f"--{protocol}",
+    if protocol.startswith("pppoe"):
+        base_args = [
+            f"--ipv{protocol[-1]}",
+            "--pppoe",
+        ]
+    else:
+        base_args = [
+            f"--{protocol}",
+        ]
+
+    base_args += [
         f"--dmac {dmac}",
         f"--smac {cfg.remote_dev['address']}",
         f"--daddr {cfg.addr_v[ipver]}",
@@ -322,6 +331,18 @@ def _gro_variants():
                 for test_name in ipv6_tests:
                     yield mode, protocol, test_name
 
+    for mode in ["sw"]:
+        for protocol in ["pppoev4", "pppoev6"]:
+            for test_name in common_tests:
+                yield mode, protocol, test_name
+
+            if protocol == "pppoev4":
+                for test_name in ipv4_tests:
+                    yield mode, protocol, test_name
+            elif protocol == "pppoev6":
+                for test_name in ipv6_tests:
+                    yield mode, protocol, test_name
+
 
 @ksft_variants(_gro_variants())
 def test(cfg, mode, protocol, test_name):
diff --git a/tools/testing/selftests/net/lib/gro.c b/tools/testing/selftests/net/lib/gro.c
index 3e611ae25f61..6148bcdff478 100644
--- a/tools/testing/selftests/net/lib/gro.c
+++ b/tools/testing/selftests/net/lib/gro.c
@@ -64,12 +64,14 @@
 #include <errno.h>
 #include <error.h>
 #include <getopt.h>
+#include <net/ethernet.h>
+#include <net/if.h>
 #include <linux/filter.h>
 #include <linux/if_packet.h>
+#include <linux/if_pppox.h>
 #include <linux/ipv6.h>
 #include <linux/net_tstamp.h>
-#include <net/ethernet.h>
-#include <net/if.h>
+#include <linux/ppp_defs.h>
 #include <netinet/in.h>
 #include <netinet/ip.h>
 #include <netinet/ip6.h>
@@ -92,11 +94,11 @@
 #define START_SEQ 100
 #define START_ACK 100
 #define ETH_P_NONE 0
-#define TOTAL_HDR_LEN (ETH_HLEN + sizeof(struct ipv6hdr) + sizeof(struct tcphdr))
+#define TOTAL_HDR_LEN (ETH_HLEN + PPPOE_SES_HLEN + sizeof(struct ipv6hdr) + sizeof(struct tcphdr))
 #define MSS (4096 - sizeof(struct tcphdr) - sizeof(struct ipv6hdr))
 #define MAX_PAYLOAD (IP_MAXPACKET - sizeof(struct tcphdr) - sizeof(struct ipv6hdr))
 #define NUM_LARGE_PKT (MAX_PAYLOAD / MSS)
-#define MAX_HDR_LEN (ETH_HLEN + sizeof(struct ipv6hdr) + sizeof(struct tcphdr))
+#define MAX_HDR_LEN (ETH_HLEN + PPPOE_SES_HLEN + sizeof(struct ipv6hdr) + sizeof(struct tcphdr))
 #define MIN_EXTHDR_SIZE 8
 #define EXT_PAYLOAD_1 "\x00\x00\x00\x00\x00\x00"
 #define EXT_PAYLOAD_2 "\x11\x11\x11\x11\x11\x11"
@@ -129,6 +131,7 @@ static int tcp_offset = -1;
 static int total_hdr_len = -1;
 static int ethhdr_proto = -1;
 static bool ipip;
+static bool pppoe;
 static uint64_t txtime_ns;
 static int num_flows = 4;
 static bool order_check;
@@ -148,12 +151,37 @@ static void vlog(const char *fmt, ...)
 	}
 }
 
+static void fill_pppoelayer(void *buf, int payload_len)
+{
+	struct pppoe_ppp_hdr {
+		struct pppoe_hdr eh;
+		__be16 proto;
+	} *ph = buf;
+	int ppp_payload_len;
+
+	ph->eh.type = 1;
+	ph->eh.ver = 1;
+	ph->eh.code = 0;
+	ph->eh.sid = htons(0x1234);
+
+	if (proto == PF_INET6) {
+		ph->proto = htons(PPP_IPV6);
+		ppp_payload_len = sizeof(struct ipv6hdr) + sizeof(struct tcphdr) + payload_len;
+	} else {
+		ph->proto = htons(PPP_IP);
+		ppp_payload_len = sizeof(struct iphdr) + sizeof(struct tcphdr) + payload_len;
+	}
+
+	ph->eh.length = htons(ppp_payload_len + sizeof(ph->proto));
+}
+
 static void setup_sock_filter(int fd)
 {
 	const int dport_off = tcp_offset + offsetof(struct tcphdr, dest);
 	const int ethproto_off = offsetof(struct ethhdr, h_proto);
 	int optlen = 0;
 	int ipproto_off, opt_ipproto_off;
+	int head_len = ETH_HLEN + (pppoe ? PPPOE_SES_HLEN : 0);
 	int next_off;
 
 	if (ipip)
@@ -162,7 +190,7 @@ static void setup_sock_filter(int fd)
 		next_off = offsetof(struct iphdr, protocol);
 	else
 		next_off = offsetof(struct ipv6hdr, nexthdr);
-	ipproto_off = ETH_HLEN + next_off;
+	ipproto_off = head_len + next_off;
 
 	/* Overridden later if exthdrs are used: */
 	opt_ipproto_off = ipproto_off;
@@ -178,7 +206,7 @@ static void setup_sock_filter(int fd)
 
 		/* same size for HBH and Fragment extension header types */
 		optlen = MIN_EXTHDR_SIZE;
-		opt_ipproto_off = ETH_HLEN + sizeof(struct ipv6hdr)
+		opt_ipproto_off = head_len + sizeof(struct ipv6hdr)
 			+ offsetof(struct ip6_ext, ip6e_nxt);
 	}
 
@@ -390,6 +418,10 @@ static void create_packet(void *buf, int seq_offset, int ack_offset,
 				  IPPROTO_IPIP);
 		fill_networklayer(buf + ETH_HLEN + sizeof(struct iphdr),
 				  payload_len, IPPROTO_TCP);
+	} else if (pppoe) {
+		fill_pppoelayer(buf + ETH_HLEN, payload_len);
+		fill_networklayer(buf + ETH_HLEN + PPPOE_SES_HLEN,
+				  payload_len, IPPROTO_TCP);
 	} else {
 		fill_networklayer(buf + ETH_HLEN, payload_len, IPPROTO_TCP);
 	}
@@ -501,7 +533,7 @@ static void send_flags(int fd, struct sockaddr_ll *daddr, int psh, int syn,
 static void send_data_pkts(int fd, struct sockaddr_ll *daddr,
 			   int payload_len1, int payload_len2)
 {
-	static char buf[ETH_HLEN + IP_MAXPACKET];
+	static char buf[ETH_HLEN + PPPOE_SES_HLEN + IP_MAXPACKET];
 
 	create_packet(buf, 0, 0, payload_len1, 0);
 	write_packet(fd, buf, total_hdr_len + payload_len1, daddr);
@@ -745,7 +777,7 @@ static void add_ipv4_ts_option(void *buf, void *optpkt)
 	memcpy(optpkt + tcp_offset + optlen, buf + tcp_offset,
 	       sizeof(struct tcphdr) + PAYLOAD_LEN);
 
-	iph = (struct iphdr *)(optpkt + ETH_HLEN);
+	iph = (struct iphdr *)(optpkt + ETH_HLEN + (pppoe ? PPPOE_SES_HLEN : 0));
 	iph->ihl = 5 + (optlen / 4);
 	iph->tot_len = htons(ntohs(iph->tot_len) + optlen);
 	iph->check = 0;
@@ -755,7 +787,7 @@ static void add_ipv4_ts_option(void *buf, void *optpkt)
 static void add_ipv6_exthdr(void *buf, void *optpkt, __u8 exthdr_type, char *ext_payload)
 {
 	struct ipv6_opt_hdr *exthdr = (struct ipv6_opt_hdr *)(optpkt + tcp_offset);
-	struct ipv6hdr *iph = (struct ipv6hdr *)(optpkt + ETH_HLEN);
+	struct ipv6hdr *iph = (struct ipv6hdr *)(optpkt + ETH_HLEN + (pppoe ? PPPOE_SES_HLEN : 0));
 	char *exthdr_payload_start = (char *)(exthdr + 1);
 
 	exthdr->hdrlen = 0;
@@ -908,7 +940,7 @@ static void send_ip_options(int fd, struct sockaddr_ll *daddr)
 static void send_fragment4(int fd, struct sockaddr_ll *daddr)
 {
 	static char buf[IP_MAXPACKET];
-	struct iphdr *iph = (struct iphdr *)(buf + ETH_HLEN);
+	struct iphdr *iph = (struct iphdr *)(buf + ETH_HLEN + (pppoe ? PPPOE_SES_HLEN : 0));
 	int pkt_size = total_hdr_len + PAYLOAD_LEN;
 
 	create_packet(buf, 0, 0, PAYLOAD_LEN, 0);
@@ -920,7 +952,8 @@ static void send_fragment4(int fd, struct sockaddr_ll *daddr)
 	 */
 	memset(buf + total_hdr_len, 'a', PAYLOAD_LEN * 2);
 	fill_transportlayer(buf + tcp_offset, PAYLOAD_LEN, 0, PAYLOAD_LEN * 2, 0);
-	fill_networklayer(buf + ETH_HLEN, PAYLOAD_LEN, IPPROTO_TCP);
+	fill_networklayer(buf + ETH_HLEN + (pppoe ? PPPOE_SES_HLEN : 0),
+			  PAYLOAD_LEN, IPPROTO_TCP);
 	fill_datalinklayer(buf);
 
 	iph->frag_off = htons(0x6000); // DF = 1, MF = 1
@@ -934,7 +967,7 @@ static void send_changed_ttl(int fd, struct sockaddr_ll *daddr)
 {
 	int pkt_size = total_hdr_len + PAYLOAD_LEN;
 	static char buf[MAX_HDR_LEN + PAYLOAD_LEN];
-	struct iphdr *iph = (struct iphdr *)(buf + ETH_HLEN);
+	struct iphdr *iph = (struct iphdr *)(buf + ETH_HLEN + (pppoe ? PPPOE_SES_HLEN : 0));
 
 	create_packet(buf, 0, 0, PAYLOAD_LEN, 0);
 	write_packet(fd, buf, pkt_size, daddr);
@@ -951,8 +984,8 @@ static void send_changed_tos(int fd, struct sockaddr_ll *daddr)
 {
 	int pkt_size = total_hdr_len + PAYLOAD_LEN;
 	static char buf[MAX_HDR_LEN + PAYLOAD_LEN];
-	struct iphdr *iph = (struct iphdr *)(buf + ETH_HLEN);
-	struct ipv6hdr *ip6h = (struct ipv6hdr *)(buf + ETH_HLEN);
+	struct iphdr *iph = (struct iphdr *)(buf + ETH_HLEN + (pppoe ? PPPOE_SES_HLEN : 0));
+	struct ipv6hdr *ip6h = (struct ipv6hdr *)(buf + ETH_HLEN + (pppoe ? PPPOE_SES_HLEN : 0));
 
 	create_packet(buf, 0, 0, PAYLOAD_LEN, 0);
 	write_packet(fd, buf, pkt_size, daddr);
@@ -980,7 +1013,8 @@ static void send_changed_ECN(int fd, struct sockaddr_ll *daddr)
 
 	create_packet(buf, PAYLOAD_LEN, 0, PAYLOAD_LEN, 0);
 	if (proto == PF_INET) {
-		buf[ETH_HLEN + 1] ^= 0x2; // ECN set to 10
+		int ecn_off = ETH_HLEN + (pppoe ? PPPOE_SES_HLEN : 0) + 1;
+		buf[ecn_off] ^= 0x2; // ECN set to 10
 		iph->check = 0;
 		iph->check = checksum_fold(iph, sizeof(struct iphdr), 0);
 	} else {
@@ -995,7 +1029,7 @@ static void send_fragment6(int fd, struct sockaddr_ll *daddr)
 	static char buf[MAX_HDR_LEN + PAYLOAD_LEN];
 	static char extpkt[MAX_HDR_LEN + PAYLOAD_LEN +
 			   sizeof(struct ip6_frag)];
-	struct ipv6hdr *ip6h = (struct ipv6hdr *)(buf + ETH_HLEN);
+	struct ipv6hdr *ip6h = (struct ipv6hdr *)(buf + ETH_HLEN + (pppoe ? PPPOE_SES_HLEN : 0));
 	struct ip6_frag *frag = (void *)(extpkt + tcp_offset);
 	int extlen = sizeof(struct ip6_frag);
 	int bufpkt_len = total_hdr_len + PAYLOAD_LEN;
@@ -1073,9 +1107,10 @@ static void recv_error(int fd, int rcv_errno)
 static void check_recv_pkts(int fd, int *correct_payload,
 			    int correct_num_pkts)
 {
-	static char buffer[IP_MAXPACKET + ETH_HLEN + 1];
-	struct iphdr *iph = (struct iphdr *)(buffer + ETH_HLEN);
-	struct ipv6hdr *ip6h = (struct ipv6hdr *)(buffer + ETH_HLEN);
+	static char buffer[IP_MAXPACKET + ETH_HLEN + PPPOE_SES_HLEN + 1];
+	int nhoff = ETH_HLEN + (pppoe ? PPPOE_SES_HLEN : 0);
+	struct iphdr *iph = (struct iphdr *)(buffer + nhoff);
+	struct ipv6hdr *ip6h = (struct ipv6hdr *)(buffer + nhoff);
 	struct tcphdr *tcph;
 	bool bad_packet = false;
 	int tcp_ext_len = 0;
@@ -1092,7 +1127,7 @@ static void check_recv_pkts(int fd, int *correct_payload,
 
 	while (1) {
 		ip_ext_len = 0;
-		pkt_size = recv(fd, buffer, IP_MAXPACKET + ETH_HLEN + 1, 0);
+		pkt_size = recv(fd, buffer, sizeof(buffer), 0);
 		if (pkt_size < 0)
 			recv_error(fd, errno);
 
@@ -1134,9 +1169,10 @@ static void check_recv_pkts(int fd, int *correct_payload,
 
 static void check_capacity_pkts(int fd)
 {
-	static char buffer[IP_MAXPACKET + ETH_HLEN + 1];
-	struct iphdr *iph = (struct iphdr *)(buffer + ETH_HLEN);
-	struct ipv6hdr *ip6h = (struct ipv6hdr *)(buffer + ETH_HLEN);
+	static char buffer[IP_MAXPACKET + ETH_HLEN + PPPOE_SES_HLEN + 1];
+	int nhoff = ETH_HLEN + (pppoe ? PPPOE_SES_HLEN : 0);
+	struct iphdr *iph = (struct iphdr *)(buffer + nhoff);
+	struct ipv6hdr *ip6h = (struct ipv6hdr *)(buffer + nhoff);
 	int num_pkt = 0, num_coal = 0, pkt_idx;
 	const char *fail_reason = NULL;
 	int flow_order[num_flows * 2];
@@ -1154,7 +1190,7 @@ static void check_capacity_pkts(int fd)
 
 	while (total_data < num_flows * CAPACITY_PAYLOAD_LEN * 2) {
 		ip_ext_len = 0;
-		pkt_size = recv(fd, buffer, IP_MAXPACKET + ETH_HLEN + 1, 0);
+		pkt_size = recv(fd, buffer, sizeof(buffer), 0);
 		if (pkt_size < 0)
 			recv_error(fd, errno);
 
@@ -1645,6 +1681,7 @@ static void parse_args(int argc, char **argv)
 		{ "ipv4", no_argument, NULL, '4' },
 		{ "ipv6", no_argument, NULL, '6' },
 		{ "ipip", no_argument, NULL, 'e' },
+		{ "pppoe", no_argument, NULL, 'p' },
 		{ "num-flows", required_argument, NULL, 'n' },
 		{ "rx", no_argument, NULL, 'r' },
 		{ "saddr", required_argument, NULL, 's' },
@@ -1671,6 +1708,9 @@ static void parse_args(int argc, char **argv)
 			proto = PF_INET;
 			ethhdr_proto = htons(ETH_P_IP);
 			break;
+		case 'p':
+			pppoe = true;
+			break;
 		case 'd':
 			addr4_dst = addr6_dst = optarg;
 			break;
@@ -1715,6 +1755,15 @@ int main(int argc, char **argv)
 	if (ipip) {
 		tcp_offset = ETH_HLEN + sizeof(struct iphdr) * 2;
 		total_hdr_len = tcp_offset + sizeof(struct tcphdr);
+	} else if (pppoe) {
+		if (proto == PF_INET)
+			tcp_offset = ETH_HLEN + PPPOE_SES_HLEN + sizeof(struct iphdr);
+		else if (proto == PF_INET6)
+			tcp_offset = ETH_HLEN + PPPOE_SES_HLEN + sizeof(struct ipv6hdr);
+		else
+			error(1, 0, "Protocol family is not ipv4 or ipv6");
+		total_hdr_len = tcp_offset + sizeof(struct tcphdr);
+		ethhdr_proto = htons(ETH_P_PPP_SES);
 	} else if (proto == PF_INET) {
 		tcp_offset = ETH_HLEN + sizeof(struct iphdr);
 		total_hdr_len = tcp_offset + sizeof(struct tcphdr);
-- 
2.43.0

Re: [RFC PATCH net-next v6 2/2] selftests: net: test PPPoE packets in gro.sh

[Willem de Bruijn]

Qingfang Deng wrote:
> Add PPPoE test-cases to the GRO selftest.
> 
> Signed-off-by: Qingfang Deng <dqfext@gmail.com>
> ---
> v6: new patch
> 
> I'm not sure if I should include all the existing IPv4/v6 tests for
> PPPoE. There are tests that hardcode ETH_HLEN as the NH offset, which
> are meant to test the L3 protocol, not the underlying protocol, and
> changing all of them seems too invasive.

Overall this change looks good to me. But indeed all the churn of

    (pppoe ? PPPOE_SES_HLEN : 0)
 
is a bit annoying. Left a few representative snippets below.
I think that could be avoided similar to MAX_HDR_LEN by having a
global definition and using that everywhere. Still a lot of churn, but
at least no repeated open-coded logic.

Also consider whether running all these tests again in PPPoE adds
value, if not, then indeed trim the tests to those that truly add
code coverage. 

> @@ -745,7 +777,7 @@ static void add_ipv4_ts_option(void *buf, void *optpkt)
>  	memcpy(optpkt + tcp_offset + optlen, buf + tcp_offset,
>  	       sizeof(struct tcphdr) + PAYLOAD_LEN);
>  
> -	iph = (struct iphdr *)(optpkt + ETH_HLEN);
> +	iph = (struct iphdr *)(optpkt + ETH_HLEN + (pppoe ? PPPOE_SES_HLEN : 0));
>  	iph->ihl = 5 + (optlen / 4);
>  	iph->tot_len = htons(ntohs(iph->tot_len) + optlen);
>  	iph->check = 0;
> @@ -755,7 +787,7 @@ static void add_ipv4_ts_option(void *buf, void *optpkt)
>  static void add_ipv6_exthdr(void *buf, void *optpkt, __u8 exthdr_type, char *ext_payload)
>  {
>  	struct ipv6_opt_hdr *exthdr = (struct ipv6_opt_hdr *)(optpkt + tcp_offset);
> -	struct ipv6hdr *iph = (struct ipv6hdr *)(optpkt + ETH_HLEN);
> +	struct ipv6hdr *iph = (struct ipv6hdr *)(optpkt + ETH_HLEN + (pppoe ? PPPOE_SES_HLEN : 0));
>  	char *exthdr_payload_start = (char *)(exthdr + 1);
>  
>  	exthdr->hdrlen = 0;

Re: [RFC PATCH net-next v6 1/2] net: pppoe: implement GRO/GSO support

[Paolo Abeni]

On 3/26/26 9:11 AM, Qingfang Deng wrote:
> +static int pppoe_gro_complete(struct sk_buff *skb, int nhoff)
> +{
> +	struct pppoe_hdr *phdr = (struct pppoe_hdr *)(skb->data + nhoff);
> +	__be16 type = pppoe_hdr_proto(phdr);
> +	struct packet_offload *ptype;
> +	unsigned int len;
> +
> +	ptype = gro_find_complete_by_type(type);
> +	if (!ptype)
> +		return -ENOENT;
> +
> +	len = skb->len - (nhoff + sizeof(*phdr));
> +	len = min(len, 0xFFFFU);

AFAICS, when the computed len is >= 64K, and the above min() will
truncate it, later pppoe_rcv() will drop the packet.

I think you should prevent such case at GRO time, flushing the chain
when it grows too big.

> +	phdr->length = cpu_to_be16(len);
> +
> +	return INDIRECT_CALL_INET(ptype->callbacks.gro_complete,
> +				  ipv6_gro_complete, inet_gro_complete,
> +				  skb, nhoff + PPPOE_SES_HLEN);
> +}
> +
> +static struct sk_buff *pppoe_gso_segment(struct sk_buff *skb,
> +					 netdev_features_t features)
> +{
> +	unsigned int pppoe_hlen = sizeof(struct pppoe_hdr) + 2;
> +	struct sk_buff *segs = ERR_PTR(-EINVAL);
> +	u16 mac_offset = skb->mac_header;
> +	struct packet_offload *ptype;
> +	u16 mac_len = skb->mac_len;
> +	struct pppoe_hdr *phdr;
> +	__be16 orig_type, type;
> +	int len, nhoff;
> +
> +	skb_reset_network_header(skb);
> +	nhoff = skb_network_header(skb) - skb_mac_header(skb);
> +
> +	if (unlikely(!pskb_may_pull(skb, pppoe_hlen)))
> +		goto out;
> +
> +	phdr = (struct pppoe_hdr *)skb_network_header(skb);
> +	type = pppoe_hdr_proto(phdr);
> +	ptype = gro_find_complete_by_type(type);
> +	if (!ptype)
> +		goto out;
> +
> +	orig_type = skb->protocol;
> +	__skb_pull(skb, pppoe_hlen);
> +	segs = ptype->callbacks.gso_segment(skb, features);
> +	if (IS_ERR_OR_NULL(segs)) {
> +		skb_gso_error_unwind(skb, orig_type, pppoe_hlen, mac_offset,
> +				     mac_len);
> +		goto out;
> +	}
> +
> +	skb = segs;
> +	do {
> +		phdr = (struct pppoe_hdr *)(skb_mac_header(skb) + nhoff);
> +		len = skb->len - (nhoff + sizeof(*phdr));
> +		phdr->length = cpu_to_be16(len);
> +		skb->network_header = (u8 *)phdr - skb->head;

I understand is quite late for the following question, but...
The network headers points to the pppoe hdr. Should it point to the
actual IP hdr?

Why not? A comment in the code or in the commit message would be
appreciated.

/P

Re: [RFC PATCH net-next v6 1/2] net: pppoe: implement GRO/GSO support

[Qingfang Deng]

On Thu, Apr 30, 2026 at 5:34 PM Paolo Abeni <pabeni@redhat.com> wrote:
>
> AFAICS, when the computed len is >= 64K, and the above min() will
> truncate it, later pppoe_rcv() will drop the packet.

pppoe_rcv() does _not_ drop such packets.
The drop condition is "skb->len < ntohs(ph->length)", not the other way around.

> > +     skb = segs;
> > +     do {
> > +             phdr = (struct pppoe_hdr *)(skb_mac_header(skb) + nhoff);
> > +             len = skb->len - (nhoff + sizeof(*phdr));
> > +             phdr->length = cpu_to_be16(len);
> > +             skb->network_header = (u8 *)phdr - skb->head;
>
> I understand is quite late for the following question, but...
> The network headers points to the pppoe hdr. Should it point to the
> actual IP hdr?
>
> Why not? A comment in the code or in the commit message would be
> appreciated.

I'm not sure about the GSO stuff. This code is carried over from
Felix's v3 patch unmodified and I haven't noticed any issues. Maybe he
has the answer.

FYI, Pablo Neira Ayuso is adding the "inline PPPoE GSO" to Netfilter
flowtable: https://lore.kernel.org/netfilter-devel/20260430055836.223494-2-pablo@netfilter.org/
to work around missing GSO support in PPPoE driver, prior to this
patch.

Re: [RFC PATCH net-next v6 1/2] net: pppoe: implement GRO/GSO support

[Pablo Neira Ayuso]

Hi Paolo,

On Thu, Apr 30, 2026 at 11:47:57PM +0800, Qingfang Deng wrote:
> On Thu, Apr 30, 2026 at 5:34 PM Paolo Abeni <pabeni@redhat.com> wrote:
> >
> > AFAICS, when the computed len is >= 64K, and the above min() will
> > truncate it, later pppoe_rcv() will drop the packet.
> 
> pppoe_rcv() does _not_ drop such packets.
> The drop condition is "skb->len < ntohs(ph->length)", not the other way around.
> 
> > > +     skb = segs;
> > > +     do {
> > > +             phdr = (struct pppoe_hdr *)(skb_mac_header(skb) + nhoff);
> > > +             len = skb->len - (nhoff + sizeof(*phdr));
> > > +             phdr->length = cpu_to_be16(len);
> > > +             skb->network_header = (u8 *)phdr - skb->head;
> >
> > I understand is quite late for the following question, but...
> > The network headers points to the pppoe hdr. Should it point to the
> > actual IP hdr?

This is the same with double-tagged-vlan, the network header also
points to the inner vlan in the skb payload. Changing this would
require to revisit all users in the tree that are already assuming
this.

> > Why not? A comment in the code or in the commit message would be
> > appreciated.
> 
> I'm not sure about the GSO stuff. This code is carried over from
> Felix's v3 patch unmodified and I haven't noticed any issues. Maybe he
> has the answer.
> 
> FYI, Pablo Neira Ayuso is adding the "inline PPPoE GSO" to Netfilter
> flowtable: https://lore.kernel.org/netfilter-devel/20260430055836.223494-2-pablo@netfilter.org/
> to work around missing GSO support in PPPoE driver, prior to this
> patch.

Re: [RFC PATCH net-next v6 1/2] net: pppoe: implement GRO/GSO support

[Paolo Abeni]

On 4/30/26 5:57 PM, Pablo Neira Ayuso wrote:
> On Thu, Apr 30, 2026 at 11:47:57PM +0800, Qingfang Deng wrote:
>> On Thu, Apr 30, 2026 at 5:34 PM Paolo Abeni <pabeni@redhat.com> wrote:
>>>
>>> AFAICS, when the computed len is >= 64K, and the above min() will
>>> truncate it, later pppoe_rcv() will drop the packet.
>>
>> pppoe_rcv() does _not_ drop such packets.
>> The drop condition is "skb->len < ntohs(ph->length)", not the other way around.
>>
>>>> +     skb = segs;
>>>> +     do {
>>>> +             phdr = (struct pppoe_hdr *)(skb_mac_header(skb) + nhoff);
>>>> +             len = skb->len - (nhoff + sizeof(*phdr));
>>>> +             phdr->length = cpu_to_be16(len);
>>>> +             skb->network_header = (u8 *)phdr - skb->head;
>>>
>>> I understand is quite late for the following question, but...
>>> The network headers points to the pppoe hdr. Should it point to the
>>> actual IP hdr?
> 
> This is the same with double-tagged-vlan, the network header also
> points to the inner vlan in the skb payload. Changing this would
> require to revisit all users in the tree that are already assuming
> this.

Ah right, the relevant GRO stage is running on top of an ethernet device
and the RX path there resets the NH just after the ethernet one.

I got lost in the relevant hooking.

This patch (and v7) LGTM, thanks.

Paolo