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

[Eric Dumazet]

Le mer. 21 oct. 2026 à 12:27, Qingfang Deng <qingfang.deng@xxxxxxxxx> a écrit :
>
> 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.

Right, I can switch to *(__be16 *)skb->data and htons() constants.

>
> > +       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.

Good point, PPTP and L2TP do not need linear packets.
I will pass NETIF_F_SG: checksums are still computed in software by
skb_segment(), as openvswitch does, and ppp_bridge_xmit() linearizes
segments only for channels without direct_xmit.

>
> > +       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__