[PATCH net] ppp: linearize and segment bridged packets when required

[PATCH net] ppp: linearize and segment bridged packets when required

[Eric Dumazet]

PPP channel bridging hands packets received on one channel directly to
the start_xmit() of another. Packets received by PPPoE can be
non-linear, GSO since commit 55a5d8fca836 ("net: pppoe: implement
GRO/GSO support"), or CHECKSUM_PARTIAL when injected with a
virtio_net_hdr. The regular transmit path never sees such packets, and
only PPPoE, which uses dev_queue_xmit(), handles them:

- PPP Async reads the whole packet through skb->data, transmitting
  unrelated memory for non-linear packets.
- PPP Async and Sync send GSO packets as single frames of up to 64KB,
  and CHECKSUM_PARTIAL packets with a bad L4 checksum.
- PPTP and L2TP cannot segment a TCP GSO packet once their GRE or UDP
  header is pushed, and do not finish pending checksums.

Add a xmit_offload flag to struct ppp_channel, set for PPPoE. For other
channels, segment GSO packets in software, using the PPP protocol field
as the L2 header, finish CHECKSUM_PARTIAL checksums, and linearize
packets for channels without direct_xmit, as ppp_push() does. Packets
are dropped if any step fails, or if the channel is busy, as before.

Tested in a VM by bridging PPPoE (on a TAP with IFF_NAPI_FRAGS, or on
veth) to PPP Async on a PTY, PPPoL2TP and PPPoE: non-linear packets no
longer leak garbage, GSO packets (including fraglist GRO) are sent as
valid segments for IPv4 and IPv6, CHECKSUM_PARTIAL packets get a valid
checksum, and PPPoE still receives GSO packets unchanged.

Fixes: 4cf476ced45d ("ppp: add PPPIOCBRIDGECHAN and PPPIOCUNBRIDGECHAN ioctls")
Fixes: 55a5d8fca836 ("net: pppoe: implement GRO/GSO support")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260908072135.877364-1-qingfang.deng@linux.dev
Co-developed-by: Qingfang Deng <qingfang.deng@linux.dev>
Signed-off-by: Qingfang Deng <qingfang.deng@linux.dev>
Signed-off-by: Eric Dumazet <edumazet@kernel.org>
Cc: Felix Fietkau <nbd@nbd.name>
Cc: Pablo Neira Ayuso <pablo@netfilter.org>
---
 drivers/net/ppp/ppp_generic.c | 75 ++++++++++++++++++++++++++++++++++-
 drivers/net/ppp/pppoe.c       |  1 +
 include/linux/ppp_channel.h   |  1 +
 3 files changed, 75 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 1a610a18893ba94b5ff3f69f8e25dcd944ef01a8..68e2ccfb17518af0a94a90e8c7d9bd8acf0b74d7 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -45,6 +45,7 @@
 #include <linux/slab.h>
 #include <linux/file.h>
 #include <linux/unaligned.h>
+#include <net/gso.h>
 #include <net/netdev_lock.h>
 #include <net/slhc_vj.h>
 #include <linux/atomic.h>
@@ -2278,6 +2279,61 @@ static bool ppp_decompress_proto(struct sk_buff *skb)
 	return pskb_may_pull(skb, 2);
 }
 
+/* Segment a GSO packet (built by PPPoE GRO, or injected with a
+ * virtio_net_hdr) before sending it to a bridged channel that does
+ * not accept GSO packets.
+ * skb->data points to the PPP protocol field, which is replicated
+ * in front of each segment.
+ * Returns a list of segments, or NULL if the packet was dropped.
+ */
+static struct sk_buff *ppp_bridge_gso_segment(struct sk_buff *skb,
+					      netdev_features_t features)
+{
+	struct sk_buff *segs;
+
+	if (!pskb_may_pull(skb, PPP_PROTO_LEN))
+		goto drop;
+
+	switch (get_unaligned((__be16 *)skb->data)) {
+	case htons(PPP_IP):
+		skb->protocol = htons(ETH_P_IP);
+		break;
+	case htons(PPP_IPV6):
+		skb->protocol = htons(ETH_P_IPV6);
+		break;
+	default:
+		goto drop;
+	}
+	skb_set_network_header(skb, PPP_PROTO_LEN);
+
+	/* No checksum offload: skb_segment() computes the checksums. */
+	segs = skb_gso_segment(skb, features);
+	if (IS_ERR_OR_NULL(segs))
+		goto drop;
+
+	consume_skb(skb);
+	return segs;
+
+drop:
+	kfree_skb(skb);
+	return NULL;
+}
+
+static void ppp_bridge_xmit(struct ppp_channel *chan, struct sk_buff *skb)
+{
+	if (!chan->direct_xmit && skb_linearize(skb))
+		goto drop;
+
+	if (!chan->xmit_offload && skb->ip_summed == CHECKSUM_PARTIAL &&
+	    skb_checksum_help(skb))
+		goto drop;
+
+	if (chan->ops->start_xmit(chan, skb))
+		return;
+drop:
+	kfree_skb(skb);
+}
+
 /* Attempt to handle a frame via. a bridged channel, if one exists.
  * If the channel is bridged, the frame is consumed by the bridge.
  * If not, the caller must handle the frame by normal recv mechanisms.
@@ -2285,6 +2341,8 @@ static bool ppp_decompress_proto(struct sk_buff *skb)
  */
 static bool ppp_channel_bridge_input(struct channel *pch, struct sk_buff *skb)
 {
+	struct sk_buff *segs, *next;
+	struct ppp_channel *chan;
 	struct channel *pchb;
 
 	rcu_read_lock();
@@ -2300,8 +2358,21 @@ static bool ppp_channel_bridge_input(struct channel *pch, struct sk_buff *skb)
 	}
 
 	skb_scrub_packet(skb, !net_eq(pch->chan_net, pchb->chan_net));
-	if (!pchb->chan->ops->start_xmit(pchb->chan, skb))
-		kfree_skb(skb);
+
+	chan = pchb->chan;
+	if (!chan->xmit_offload && skb_is_gso(skb)) {
+		/* Channels needing linear packets get linear segments,
+		 * others keep references to the original pages.
+		 */
+		segs = ppp_bridge_gso_segment(skb, chan->direct_xmit ?
+						   NETIF_F_SG : 0);
+		skb_list_walk_safe(segs, skb, next) {
+			skb_mark_not_on_list(skb);
+			ppp_bridge_xmit(chan, skb);
+		}
+	} else {
+		ppp_bridge_xmit(chan, skb);
+	}
 
 outl:
 	spin_unlock_bh(&pchb->downl);
diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
index ee0e318d6a2a4f32f35231a412b59921b0cb6921..b54bd713cc096f7a33f07ab6a8371898aa69dd5b 100644
--- a/drivers/net/ppp/pppoe.c
+++ b/drivers/net/ppp/pppoe.c
@@ -664,6 +664,7 @@ static int pppoe_connect(struct socket *sock, struct sockaddr_unsized *uservaddr
 		po->chan.private = sk;
 		po->chan.ops = &pppoe_chan_ops;
 		po->chan.direct_xmit = true;
+		po->chan.xmit_offload = true;
 
 		error = ppp_register_net_channel(dev_net(dev), &po->chan);
 		if (error) {
diff --git a/include/linux/ppp_channel.h b/include/linux/ppp_channel.h
index 2f63e9a6cc88984c075792dcbd6c832ff45acd51..15bc3aa4f1715c265a5808fcc1cc2b93cc44c519 100644
--- a/include/linux/ppp_channel.h
+++ b/include/linux/ppp_channel.h
@@ -43,6 +43,7 @@ struct ppp_channel {
 	void		*ppp;		/* opaque to channel */
 	int		speed;		/* transfer rate (bytes/second) */
 	bool		direct_xmit;	/* no qdisc, xmit directly */
+	bool		xmit_offload;	/* accepts GSO and CHECKSUM_PARTIAL skbs */
 };
 
 #ifdef __KERNEL__
-- 
2.53.0

Re: [PATCH net] ppp: linearize and segment bridged packets when required

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 1 · High: 1 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Critical] ppp_bridge_gso_segment() in drivers/net/ppp/ppp_generic.c
  calls skb_gso_segment() on PPPoE-received GSO skbs whose skb->dev is…
- [High] ppp: WARN_ONCE and packet drop in ppp_bridge_xmit() on negative
  csum_start
- [Medium] ppp_bridge_xmit() in drivers/net/ppp/ppp_generic.c finishes
  every CHECKSUM_PARTIAL skb with skb_checksum_help(), which always…

--

ppp: linearize and segment bridged packets when required

This patch adds an xmit_offload flag to struct ppp_channel and sets it
only for PPPoE. For bridged channels without the flag, the bridge now
segments GSO packets in software and finishes CHECKSUM_PARTIAL checksums.
Packets going to channels without direct_xmit are also linearized.

> diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> index 1a610a18893ba..68e2ccfb17518 100644
> --- a/drivers/net/ppp/ppp_generic.c
> +++ b/drivers/net/ppp/ppp_generic.c

[ ... ]

> @@ -2278,6 +2279,61 @@ static bool ppp_decompress_proto(struct sk_buff *skb)
>  	return pskb_may_pull(skb, 2);
>  }
>  
> +/* Segment a GSO packet (built by PPPoE GRO, or injected with a
> + * virtio_net_hdr) before sending it to a bridged channel that does
> + * not accept GSO packets.
> + * skb->data points to the PPP protocol field, which is replicated
> + * in front of each segment.
> + * Returns a list of segments, or NULL if the packet was dropped.
> + */
> +static struct sk_buff *ppp_bridge_gso_segment(struct sk_buff *skb,
> +					      netdev_features_t features)
> +{
> +	struct sk_buff *segs;
> +
> +	if (!pskb_may_pull(skb, PPP_PROTO_LEN))
> +		goto drop;
> +
> +	switch (get_unaligned((__be16 *)skb->data)) {
> +	case htons(PPP_IP):
> +		skb->protocol = htons(ETH_P_IP);
> +		break;
> +	case htons(PPP_IPV6):
> +		skb->protocol = htons(ETH_P_IPV6);
> +		break;
> +	default:
> +		goto drop;
> +	}
> +	skb_set_network_header(skb, PPP_PROTO_LEN);
> +
> +	/* No checksum offload: skb_segment() computes the checksums. */
> +	segs = skb_gso_segment(skb, features);

[Severity: Critical]

Can skb->dev be NULL at this point?

On the PPPoE receive side, pppoe_rcv() passes the skb to
__sk_receive_skb(), which clears the device before the backlog handler
runs:

net/core/sock.c:__sk_receive_skb() {
    ...
	skb->dev = NULL;
    ...
}

The skb then gets here through:

pppoe_rcv()->__sk_receive_skb()->sk_backlog_rcv()->pppoe_rcv_core()->
  ppp_input()->ppp_channel_bridge_input()->ppp_bridge_gso_segment()

Nothing on that path sets skb->dev again.

For tunnelled GSO packets, the software GSO handlers dereference skb->dev
with no NULL check. For IP-in-IP, ipip_gso_segment() calls the inner
inet_gso_segment(), which does:

net/ipv4/af_inet.c:inet_gso_segment() {
    ...
	if (encap)
		features &= skb->dev->hw_enc_features;
    ...
}

gre_gso_segment(), __skb_udp_tunnel_segment() and ipv6_gso_segment()
dereference skb->dev the same way.

Remote traffic can build such skbs. pppoe_gro_receive() hands off to
inet_gro_receive(), which can stack ipip_gro_receive() without any local
tunnel device. ipip_gro_complete() then sets:

	skb->encapsulation = 1;
	skb_shinfo(skb)->gso_type |= SKB_GSO_IPXIP4;

Before this patch, bridged GSO skbs went unchanged to start_xmit(). Only
PPPoE segmented them, and it sets skb->dev before dev_queue_xmit().

Take a LAC that bridges PPPoE to L2TP or PPTP with PPPIOCBRIDGECHAN.
Could a remote PPPoE peer that sends GRO-merged IPIP-encapsulated TCP
cause a NULL pointer dereference in softirq here, with pchb->downl held?

Should this path either set skb->dev or reject skbs with
skb->encapsulation or tunnel gso_type bits before calling
skb_gso_segment()?

> +	if (IS_ERR_OR_NULL(segs))
> +		goto drop;
> +
> +	consume_skb(skb);
> +	return segs;
> +
> +drop:
> +	kfree_skb(skb);
> +	return NULL;
> +}
> +
> +static void ppp_bridge_xmit(struct ppp_channel *chan, struct sk_buff *skb)
> +{
> +	if (!chan->direct_xmit && skb_linearize(skb))
> +		goto drop;
> +
> +	if (!chan->xmit_offload && skb->ip_summed == CHECKSUM_PARTIAL &&
> +	    skb_checksum_help(skb))
> +		goto drop;

[Severity: High]
Can skb->ip_summed still be CHECKSUM_PARTIAL here while csum_start
points before skb->data?

Consider PPPoL2TP over UDP as the bridge source. Suppose the L2TP packet
was generated locally and is received over veth, loopback or virtio-net
with UDP checksum offload. It then arrives in udp_rcv() or udpv6_rcv()
as CHECKSUM_PARTIAL, with csum_start at the outer UDP header.
udp4_csum_init() and udp6_csum_init() see skb_csum_unnecessary() and
leave ip_summed unchanged.

The skb then gets here through:

udp_queue_rcv_one_skb()->l2tp_udp_encap_recv()->l2tp_recv_common()->
  pppol2tp_recv()->ppp_input()->ppp_channel_bridge_input()->
  ppp_bridge_xmit()

l2tp_udp_encap_recv() and l2tp_recv_common() strip the UDP and L2TP
headers with __skb_pull(). pppol2tp_recv() strips the address and
control bytes with skb_pull(). None of these update the checksum state,
so skb_checksum_start_offset(skb) is negative by the time we get here.

The non-bridged path handles this case in ppp_receive_nonmp_frame()
through skb_pull_rcsum(). Its skb_postpull_rcsum() turns
CHECKSUM_PARTIAL into CHECKSUM_NONE when the start offset goes
negative. The bridge path never reaches that code.

For a destination channel without xmit_offload, skb_checksum_help()
then does:

net/core/dev.c:skb_checksum_help() {
    ...
	offset = skb_checksum_start_offset(skb);
	ret = -EINVAL;
	if (unlikely(offset >= skb_headlen(skb))) {
		DO_ONCE_LITE(skb_dump, KERN_ERR, skb, false);
		WARN_ONCE(true, "offset (%d) >= skb_headlen() (%u)\n",
			  offset, skb_headlen(skb));
		goto out;
	}
    ...
}

The negative int offset is converted to unsigned int in the comparison.
That fires the WARN_ONCE() and skb_dump(), and every such packet is
dropped. With panic_on_warn this is a panic.

Before this patch the skb went to start_xmit() unchanged. So bridging
L2TP to PPP Async, Sync, PPTP or L2TP looks like a new regression
here.

PPPoE as the destination sets xmit_offload and skips this check, but
the stale state remains. pppoe_xmit() then pushes the PPPoE and
Ethernet headers, which can bring the start offset back to zero or
above. Could the output device then write a checksum into the wrong
bytes of the frame?

Should ppp_channel_bridge_input() drop the stale state before the GSO
and ppp_bridge_xmit() paths? It could reset ip_summed to CHECKSUM_NONE
when skb_checksum_start_offset(skb) < 0, as skb_postpull_rcsum() does.
Or should the L2TP receive path use the _rcsum pull helpers instead?

[Severity: Medium]

Are SCTP CRC32c requests handled correctly here?

skb_checksum_help() always writes a 16-bit Internet checksum and then sets
CHECKSUM_NONE. It does not check skb->csum_not_inet. The core dispatcher
handles that case separately:

net/core/dev.c:skb_csum_hwoffload_help() {
    ...
	if (unlikely(skb_csum_is_sctp(skb)))
		return !!(features & NETIF_F_SCTP_CRC) ? 0 :
			skb_crc32c_csum_help(skb);
    ...
}

sctp_packet_pack() sets CHECKSUM_PARTIAL with csum_not_inet = 1 when the
route device has NETIF_F_SCTP_CRC, as veth does. Such a packet can be
wrapped in PPPoE, for example with bpf_skb_change_head() in a tc program,
which keeps ip_summed and csum_start. If PPPoE then receives it on the
peer, neither PPPoE receive nor skb_scrub_packet() clears that state.

If the packet is bridged to a channel without xmit_offload, a 16-bit
Internet checksum is written into the 32-bit SCTP checksum field. The
receiver will then drop the packet.

Take L2TP over IPv4 with the default of no UDP checksum. Before this
patch the skb kept CHECKSUM_PARTIAL and csum_not_inet, and
validate_xmit_skb()->skb_csum_hwoffload_help() on the output device
computed a correct CRC32c. So this looks like a regression for that
setup. It also does not match the commit message, which says the patch
will "finish CHECKSUM_PARTIAL checksums".

Would skb_csum_hwoffload_help(skb, 0) work better here?

> +
> +	if (chan->ops->start_xmit(chan, skb))
> +		return;
> +drop:
> +	kfree_skb(skb);
> +}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261022111039.85112-1-edumazet%40kernel.org

Re: [PATCH net] ppp: linearize and segment bridged packets when required

[Eric Dumazet]

Le lun. 26 oct. 2026 à 03:12, <netdev-bot+sashiko@kernel.org> a écrit :
>
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
>
> Critical: 1 · High: 1 · Medium: 1 · Low: 0
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [Critical] ppp_bridge_gso_segment() in drivers/net/ppp/ppp_generic.c
>   calls skb_gso_segment() on PPPoE-received GSO skbs whose skb->dev is…
> - [High] ppp: WARN_ONCE and packet drop in ppp_bridge_xmit() on negative
>   csum_start
> - [Medium] ppp_bridge_xmit() in drivers/net/ppp/ppp_generic.c finishes
>   every CHECKSUM_PARTIAL skb with skb_checksum_help(), which always…
>
> --
>
> ppp: linearize and segment bridged packets when required
>
> This patch adds an xmit_offload flag to struct ppp_channel and sets it
> only for PPPoE. For bridged channels without the flag, the bridge now
> segments GSO packets in software and finishes CHECKSUM_PARTIAL checksums.
> Packets going to channels without direct_xmit are also linearized.
>
> > diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> > index 1a610a18893ba..68e2ccfb17518 100644
> > --- a/drivers/net/ppp/ppp_generic.c
> > +++ b/drivers/net/ppp/ppp_generic.c
>
> [ ... ]
>
> > @@ -2278,6 +2279,61 @@ static bool ppp_decompress_proto(struct sk_buff *skb)
> >       return pskb_may_pull(skb, 2);
> >  }
> >
> > +/* Segment a GSO packet (built by PPPoE GRO, or injected with a
> > + * virtio_net_hdr) before sending it to a bridged channel that does
> > + * not accept GSO packets.
> > + * skb->data points to the PPP protocol field, which is replicated
> > + * in front of each segment.
> > + * Returns a list of segments, or NULL if the packet was dropped.
> > + */
> > +static struct sk_buff *ppp_bridge_gso_segment(struct sk_buff *skb,
> > +                                           netdev_features_t features)
> > +{
> > +     struct sk_buff *segs;
> > +
> > +     if (!pskb_may_pull(skb, PPP_PROTO_LEN))
> > +             goto drop;
> > +
> > +     switch (get_unaligned((__be16 *)skb->data)) {
> > +     case htons(PPP_IP):
> > +             skb->protocol = htons(ETH_P_IP);
> > +             break;
> > +     case htons(PPP_IPV6):
> > +             skb->protocol = htons(ETH_P_IPV6);
> > +             break;
> > +     default:
> > +             goto drop;
> > +     }
> > +     skb_set_network_header(skb, PPP_PROTO_LEN);
> > +
> > +     /* No checksum offload: skb_segment() computes the checksums. */
> > +     segs = skb_gso_segment(skb, features);
>
> [Severity: Critical]
>
> Can skb->dev be NULL at this point?
>
> On the PPPoE receive side, pppoe_rcv() passes the skb to
> __sk_receive_skb(), which clears the device before the backlog handler
> runs:
>
> net/core/sock.c:__sk_receive_skb() {
>     ...
>         skb->dev = NULL;
>     ...
> }
>
> The skb then gets here through:
>
> pppoe_rcv()->__sk_receive_skb()->sk_backlog_rcv()->pppoe_rcv_core()->
>   ppp_input()->ppp_channel_bridge_input()->ppp_bridge_gso_segment()
>
> Nothing on that path sets skb->dev again.
>
> For tunnelled GSO packets, the software GSO handlers dereference skb->dev
> with no NULL check. For IP-in-IP, ipip_gso_segment() calls the inner
> inet_gso_segment(), which does:
>
> net/ipv4/af_inet.c:inet_gso_segment() {
>     ...
>         if (encap)
>                 features &= skb->dev->hw_enc_features;
>     ...
> }
>
> gre_gso_segment(), __skb_udp_tunnel_segment() and ipv6_gso_segment()
> dereference skb->dev the same way.
>
> Remote traffic can build such skbs. pppoe_gro_receive() hands off to
> inet_gro_receive(), which can stack ipip_gro_receive() without any local
> tunnel device. ipip_gro_complete() then sets:
>
>         skb->encapsulation = 1;
>         skb_shinfo(skb)->gso_type |= SKB_GSO_IPXIP4;
>
> Before this patch, bridged GSO skbs went unchanged to start_xmit(). Only
> PPPoE segmented them, and it sets skb->dev before dev_queue_xmit().
>
> Take a LAC that bridges PPPoE to L2TP or PPTP with PPPIOCBRIDGECHAN.
> Could a remote PPPoE peer that sends GRO-merged IPIP-encapsulated TCP
> cause a NULL pointer dereference in softirq here, with pchb->downl held?
>
> Should this path either set skb->dev or reject skbs with
> skb->encapsulation or tunnel gso_type bits before calling
> skb_gso_segment()?

Right, skb->dev is NULL here. IPv6 UFO has the same issue
(udp6_ufo_fragment() uses dev_net(skb->dev)).

There is no device to use at this point, so v2 drops encapsulated
skbs, and only segments TCP, UDP_L4 and fraglist GSO types.

>
> > +     if (IS_ERR_OR_NULL(segs))
> > +             goto drop;
> > +
> > +     consume_skb(skb);
> > +     return segs;
> > +
> > +drop:
> > +     kfree_skb(skb);
> > +     return NULL;
> > +}
> > +
> > +static void ppp_bridge_xmit(struct ppp_channel *chan, struct sk_buff *skb)
> > +{
> > +     if (!chan->direct_xmit && skb_linearize(skb))
> > +             goto drop;
> > +
> > +     if (!chan->xmit_offload && skb->ip_summed == CHECKSUM_PARTIAL &&
> > +         skb_checksum_help(skb))
> > +             goto drop;
>
> [Severity: High]
> Can skb->ip_summed still be CHECKSUM_PARTIAL here while csum_start
> points before skb->data?
>
> Consider PPPoL2TP over UDP as the bridge source. Suppose the L2TP packet
> was generated locally and is received over veth, loopback or virtio-net
> with UDP checksum offload. It then arrives in udp_rcv() or udpv6_rcv()
> as CHECKSUM_PARTIAL, with csum_start at the outer UDP header.
> udp4_csum_init() and udp6_csum_init() see skb_csum_unnecessary() and
> leave ip_summed unchanged.
>
> The skb then gets here through:
>
> udp_queue_rcv_one_skb()->l2tp_udp_encap_recv()->l2tp_recv_common()->
>   pppol2tp_recv()->ppp_input()->ppp_channel_bridge_input()->
>   ppp_bridge_xmit()
>
> l2tp_udp_encap_recv() and l2tp_recv_common() strip the UDP and L2TP
> headers with __skb_pull(). pppol2tp_recv() strips the address and
> control bytes with skb_pull(). None of these update the checksum state,
> so skb_checksum_start_offset(skb) is negative by the time we get here.
>
> The non-bridged path handles this case in ppp_receive_nonmp_frame()
> through skb_pull_rcsum(). Its skb_postpull_rcsum() turns
> CHECKSUM_PARTIAL into CHECKSUM_NONE when the start offset goes
> negative. The bridge path never reaches that code.
>
> For a destination channel without xmit_offload, skb_checksum_help()
> then does:
>
> net/core/dev.c:skb_checksum_help() {
>     ...
>         offset = skb_checksum_start_offset(skb);
>         ret = -EINVAL;
>         if (unlikely(offset >= skb_headlen(skb))) {
>                 DO_ONCE_LITE(skb_dump, KERN_ERR, skb, false);
>                 WARN_ONCE(true, "offset (%d) >= skb_headlen() (%u)\n",
>                           offset, skb_headlen(skb));
>                 goto out;
>         }
>     ...
> }
>
> The negative int offset is converted to unsigned int in the comparison.
> That fires the WARN_ONCE() and skb_dump(), and every such packet is
> dropped. With panic_on_warn this is a panic.
>
> Before this patch the skb went to start_xmit() unchanged. So bridging
> L2TP to PPP Async, Sync, PPTP or L2TP looks like a new regression
> here.
>
> PPPoE as the destination sets xmit_offload and skips this check, but
> the stale state remains. pppoe_xmit() then pushes the PPPoE and
> Ethernet headers, which can bring the start offset back to zero or
> above. Could the output device then write a checksum into the wrong
> bytes of the frame?
>
> Should ppp_channel_bridge_input() drop the stale state before the GSO
> and ppp_bridge_xmit() paths? It could reset ip_summed to CHECKSUM_NONE
> when skb_checksum_start_offset(skb) < 0, as skb_postpull_rcsum() does.

Yes, v2 does this right after skb_scrub_packet(), which also covers
the PPPoE destination case.

> Or should the L2TP receive path use the _rcsum pull helpers instead?
>
> [Severity: Medium]
>
> Are SCTP CRC32c requests handled correctly here?
>
> skb_checksum_help() always writes a 16-bit Internet checksum and then sets
> CHECKSUM_NONE. It does not check skb->csum_not_inet. The core dispatcher
> handles that case separately:
>
> net/core/dev.c:skb_csum_hwoffload_help() {
>     ...
>         if (unlikely(skb_csum_is_sctp(skb)))
>                 return !!(features & NETIF_F_SCTP_CRC) ? 0 :
>                         skb_crc32c_csum_help(skb);
>     ...
> }
>
> sctp_packet_pack() sets CHECKSUM_PARTIAL with csum_not_inet = 1 when the
> route device has NETIF_F_SCTP_CRC, as veth does. Such a packet can be
> wrapped in PPPoE, for example with bpf_skb_change_head() in a tc program,
> which keeps ip_summed and csum_start. If PPPoE then receives it on the
> peer, neither PPPoE receive nor skb_scrub_packet() clears that state.
>
> If the packet is bridged to a channel without xmit_offload, a 16-bit
> Internet checksum is written into the 32-bit SCTP checksum field. The
> receiver will then drop the packet.
>
> Take L2TP over IPv4 with the default of no UDP checksum. Before this
> patch the skb kept CHECKSUM_PARTIAL and csum_not_inet, and
> validate_xmit_skb()->skb_csum_hwoffload_help() on the output device
> computed a correct CRC32c. So this looks like a regression for that
> setup. It also does not match the commit message, which says the patch
> will "finish CHECKSUM_PARTIAL checksums".
>
> Would skb_csum_hwoffload_help(skb, 0) work better here?


Indeed, v2 uses skb_csum_hwoffload_help(skb, 0).

Thanks!

pw-bot: cr

>
> > +
> > +     if (chan->ops->start_xmit(chan, skb))
> > +             return;
> > +drop:
> > +     kfree_skb(skb);
> > +}
>
> [ ... ]
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261022111039.85112-1-edumazet%40kernel.org