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

[Qingfang Deng]

On 2026/10/21 17:24, Eric Dumazet wrote:
> OK, but I think GRO/GSO is still in the way (because of 55a5d8fca836
> ("net: pppoe: implement GRO/GSO support")).
> I guess Sashiko will find out.
>
> We should GSO segment first, then linearize the small segments
> (instead of 64KB packets)
>
> I was working on the GSO part yesterday, so what about this:
>
> commit 5e8cbf87e1aea072b9bdb52ca5c7a5f523186934
> Author: Eric Dumazet <edumazet@xxxxxxxxxx>
> Date:   Wed Oct 21 10:04:45 2026 +0200
>
>      ppp: linearize and segment bridged packets when required
>
>      PPP channel bridging is commonly used for PPPoE relaying or forwarding
>      from PPPoE to PPPoL2TP. Both destination channel types handle non-linear
>      packets, but other channels, such as PPP Async, require linear packets.
>
>      Ethernet drivers can pass non-linear packets to PPPoE. If one is bridged
>      to PPP Async, ppp_async_encode() accesses the entire packet through
>      skb->data, assuming that it is linear. This can read beyond the linear
>      buffer and transmit unrelated memory over the serial interface.
>
>      Since commit 55a5d8fca836 ("net: pppoe: implement GRO/GSO support"),
>      these packets can also be GSO packets. Packets can also be in
>      CHECKSUM_PARTIAL state, for instance when injected by a tap device or
>      a packet socket using a virtio_net_hdr. The regular transmit path never
>      sees such packets, because the ppp device does not advertise GSO or
>      checksum offload features. Most PPP channels are not prepared for them:
>
>      - PPP Async and PPP Sync send a GSO packet as a single frame, up to
>        64KB, far above the peer MRU. They also send CHECKSUM_PARTIAL packets
>        with an unfinished L4 checksum.
>      - PPTP pushes GRE and IP headers in front of a TCP GSO packet, which
>        gre_gso_segment() refuses because skb->encapsulation is not set. It
>        also sets CHECKSUM_NONE without finishing a pending checksum.
>      - L2TP pushes UDP and L2TP headers in front of a TCP GSO packet, which
>        udp4_ufo_fragment() refuses. A TSO capable device could also be
>        handed a TCP GSO packet whose transport header is a UDP header.
>
>      Only PPPoE handles them correctly, as pppoe_xmit() uses
>      dev_queue_xmit(), which can segment GSO packets with
>      pppoe_gso_segment() and finish checksums, or offload them to the
>      device.
>
>      Add a xmit_offload flag to struct ppp_channel, telling that the channel
>      accepts GSO and CHECKSUM_PARTIAL packets, and set it for PPPoE.
>
>      For other bridged channels, segment GSO packets in software. The PPP
>      protocol field in front of the IP header is used as the L2 header, and
>      is replicated in front of each segment. Then finish CHECKSUM_PARTIAL
>      checksums with skb_checksum_help(). For channels without direct_xmit,
>      also linearize each packet, as ppp_push() does for the regular transmit
>      path. Packets are dropped if any of these steps fails.
>
>      The bridge path still has no transmit queue, so a channel that is
>      busy (for example a full tty) drops the segments it refuses, as it
>      already did for regular packets.
>
>      Non-linear packets were reproduced in a VM, with a TAP interface using
>      IFF_NAPI_FRAGS feeding PPPoE, bridged to PPP Async on a PTY: the
>      packets sent to the PTY contained garbage data.
>
>      GSO and CHECKSUM_PARTIAL packets were reproduced by bridging a PPPoE
>      channel on a veth device to PPP Async on a PTY. With GRO enabled, a
>      burst of 8 TCP segments was sent as a single 8040 byte frame with a
>      bad TCP checksum, and is now sent as 8 valid frames, for both IPv4 and
>      IPv6. A CHECKSUM_PARTIAL packet sent through a packet socket with
>      PACKET_VNET_HDR was sent with a bad TCP checksum, and now has a valid
>      one. When bridging to a second PPPoE channel, GSO packets are passed
>      unchanged to the PPPoE device, and are correctly segmented by
>      pppoe_gso_segment() when the device does not offload them.
>
>      Fixes: 4cf476ced45d ("ppp: add PPPIOCBRIDGECHAN and
> PPPIOCUNBRIDGECHAN ioctls")
>      Fixes: 55a5d8fca836 ("net: pppoe: implement GRO/GSO support")
>      Reported-by: Sashiko <sashiko-bot@xxxxxxxxxx>
>      Closes: https://sashiko.dev/#/patchset/20260908072135.877364-1-qingfang.deng@xxxxxxxxx
>      Co-developed-by: Qingfang Deng <qingfang.deng@xxxxxxxxx>
>      Signed-off-by: Qingfang Deng <qingfang.deng@xxxxxxxxx>
>      Signed-off-by: Eric Dumazet <edumazet@xxxxxxxxxx>
>      Cc: Felix Fietkau <nbd@xxxxxxxx>
>      Cc: Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>

LGTM, thanks. Nits below:

> diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> index 1a610a18893ba94b5ff3f69f8e25dcd944ef01a8..2b27ec36c8cef829e3bc9238036c68967aceea1e
> 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,58 @@ static bool ppp_decompress_proto(struct sk_buff *skb)
>          return pskb_may_pull(skb, 2);
>   }
>
> +/* Segment a GSO packet (built by PPPoE GRO) 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)
> +{
> +       struct sk_buff *segs;
> +
> +       if (!pskb_may_pull(skb, PPP_PROTO_LEN))
> +               goto drop;
> +
> +       switch (get_unaligned_be16(skb->data)) {

skb->data should be already 2-byte aligned here. You can avoid the 
get_unaligned and runtime byte-swapping.

> +       case PPP_IP:
> +               skb->protocol = htons(ETH_P_IP);
> +               break;
> +       case PPP_IPV6:
> +               skb->protocol = htons(ETH_P_IPV6);
> +               break;
> +       default:
> +               goto drop;
> +       }
> +       skb_set_network_header(skb, PPP_PROTO_LEN);
> +
> +       segs = skb_gso_segment(skb, 0);

Passing zero features to skb_gso_segment will linearize every segment 
prematurely.

> +       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 +2338,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 +2355,17 @@ 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)) {
> +               segs = ppp_bridge_gso_segment(skb);
> +               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 bf7414b46a26068d1773ec79055167956dde7183..f22087d76874ce524f66003ae00d1528a0e0b898
> 100644
> --- a/drivers/net/ppp/pppoe.c
> +++ b/drivers/net/ppp/pppoe.c
> @@ -669,6 +669,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__