[PATCH net] pppoatm: ensure a writable skb header

[PATCH net] pppoatm: ensure a writable skb header

[Eric Dumazet]

In pppoatm_send(), LLC encapsulation checks whether there is sufficient
headroom for the 4-byte LLC header, but does not ensure that the skb header
is writable.

Normal transmit packets passing through ppp_start_xmit() have their header
unshared via skb_cow_head(). However, packets can also reach pppoatm_send()
via PPP channel bridging (PPPIOCBRIDGECHAN) without going through
ppp_start_xmit().

Use skb_cow_head() to ensure both sufficient headroom and a writable
header before pushing the LLC header.

While at it, this also simplifies the code and avoids allocating and
freeing a temporary skb when the ATM socket buffer is full.

Fixes: 4cf476ced45d ("ppp: add PPPIOCBRIDGECHAN and PPPIOCUNBRIDGECHAN ioctls")
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
Cc: Mitchell Blank Jr <mitch@sfgoth.com>
Cc: Chas Williams <3chas3@gmail.com>
Cc: Qingfang Deng <qingfang.deng@linux.dev>
---
 net/atm/pppoatm.c | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index 6da52d12df68e493b03f57edc46c3b5255956893..4ce7fa6e493e95d64909b72fd123a517c0befb12 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -317,21 +317,12 @@ static int pppoatm_send(struct ppp_channel *chan, struct sk_buff *skb)
 
 	switch (pvcc->encaps) {		/* LLC encapsulation needed */
 	case e_llc:
-		if (skb_headroom(skb) < LLC_LEN) {
-			struct sk_buff *n;
-			n = skb_realloc_headroom(skb, LLC_LEN);
-			if (n != NULL &&
-			    !pppoatm_may_send(pvcc, n->truesize)) {
-				kfree_skb(n);
-				goto nospace;
-			}
-			consume_skb(skb);
-			skb = n;
-			if (skb == NULL) {
-				bh_unlock_sock(sk_atm(vcc));
-				return DROP_PACKET;
-			}
-		} else if (!pppoatm_may_send(pvcc, skb->truesize))
+		if (skb_cow_head(skb, LLC_LEN)) {
+			bh_unlock_sock(sk_atm(vcc));
+			kfree_skb(skb);
+			return DROP_PACKET;
+		}
+		if (!pppoatm_may_send(pvcc, skb->truesize))
 			goto nospace;
 		memcpy(skb_push(skb, LLC_LEN), pppllc, LLC_LEN);
 		break;
-- 
2.55.0.979.g7e5102b832-goog

Re: [PATCH net] pppoatm: ensure a writable skb header

[netdev-bot+sashiko]

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

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

- [Medium] net/atm/pppoatm.c:pppoatm_send() — the SC_COMP_PROT "undo"
  heuristic on the `nospace` path now makes its decision based on…

--

pppoatm: ensure a writable skb header

This replaces the open-coded skb_realloc_headroom() dance in pppoatm_send()
with skb_cow_head(), so the LLC header is pushed into headroom that is both
large enough and writable.  It also removes the temporary skb allocation
that used to happen when the ATM socket buffer was full.

> diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
> index 6da52d12df68e..4ce7fa6e493e9 100644
> --- a/net/atm/pppoatm.c
> +++ b/net/atm/pppoatm.c
> @@ -317,21 +317,12 @@ static int pppoatm_send(struct ppp_channel *chan, struct sk_buff *skb)
>  
>  	switch (pvcc->encaps) {		/* LLC encapsulation needed */
>  	case e_llc:
> -		if (skb_headroom(skb) < LLC_LEN) {
> -			struct sk_buff *n;
> -			n = skb_realloc_headroom(skb, LLC_LEN);
> -			if (n != NULL &&
> -			    !pppoatm_may_send(pvcc, n->truesize)) {
> -				kfree_skb(n);
> -				goto nospace;
> -			}
> -			consume_skb(skb);
> -			skb = n;
> -			if (skb == NULL) {
> -				bh_unlock_sock(sk_atm(vcc));
> -				return DROP_PACKET;
> -			}
> -		} else if (!pppoatm_may_send(pvcc, skb->truesize))
> +		if (skb_cow_head(skb, LLC_LEN)) {
> +			bh_unlock_sock(sk_atm(vcc));
> +			kfree_skb(skb);
> +			return DROP_PACKET;
> +		}
> +		if (!pppoatm_may_send(pvcc, skb->truesize))
>  			goto nospace;

[Severity: Medium]
Does this change how the SC_COMP_PROT undo heuristic behaves at the nospace
label for skbs that arrive with less than LLC_LEN headroom?

The head expansion now happens on the original skb, before the
pppoatm_may_send() admission check, so when admission fails we reach:

net/atm/pppoatm.c:pppoatm_send() {
	...
nospace:
	bh_unlock_sock(sk_atm(vcc));
	if ((pvcc->flags & SC_COMP_PROT) && skb_headroom(skb) > 0 &&
	    skb->data[-1] == '\0')
		(void) skb_push(skb, 1);
	return 0;
}

For an skb that entered with zero headroom, skb_cow_head() goes through
pskb_expand_head(), which allocates with kmalloc_reserve() (no __GFP_ZERO)
and only copies the old data:

net/core/skbuff.c:pskb_expand_head() {
	...
	data = kmalloc_reserve(&size, gfp_mask, NUMA_NO_NODE, NULL);
	if (!data)
		goto nodata;
	size = SKB_WITH_OVERHEAD(size);
	memcpy(data + nhead, skb->head, skb_tail_pointer(skb) - skb->head);
	...
}

The new bytes in front of the copied data are never initialized, so is
skb->data[-1] here reading stale slab content rather than a byte this
function pulled?  If that byte happens to be 0x00, the skb_push(skb, 1)
prepends a spurious zero to a frame from which nothing was ever pulled at:

	if (skb->data[0] == '\0' && (pvcc->flags & SC_COMP_PROT))
		(void) skb_pull(skb, 1);

Before this change the same case took the skb_realloc_headroom() branch,
freed the copy and jumped to nospace with the original zero-headroom skb, so
the skb_headroom(skb) > 0 test suppressed the push.

Frames written to /dev/ppp look like they can hit this: ppp_write() does

drivers/net/ppp/ppp_generic.c:ppp_write() {
	...
	skb = alloc_skb(count + pf->hdrlen, GFP_KERNEL);
	if (!skb)
		goto out;
	skb_reserve(skb, pf->hdrlen);
	...
}

and pppoatm never assigns chan.hdrlen, so pf->hdrlen is 0 and control frames
with a non-zero first protocol byte (LCP 0xc021, IPCP 0x8021, PAP 0xc023)
arrive with no headroom and no entry-time pull.  With the VCC queue full,
pppoatm_may_send() returns 0 and the goto nospace path runs.

Would it be more robust to record whether the entry-time skb_pull(skb, 1)
actually happened in a local bool, and test that at nospace, instead of
inferring it from skb->data[-1]?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260908090519.339696-1-edumazet%40google.com

Re: [PATCH net] pppoatm: ensure a writable skb header

[Eric Dumazet]

On Wed, Sep 9, 2026 at 2:07 PM <netdev-bot+sashiko@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
>
> Critical: 0 · High: 0 · Medium: 1 · Low: 0
>
> - [Medium] net/atm/pppoatm.c:pppoatm_send() — the SC_COMP_PROT "undo"
>   heuristic on the `nospace` path now makes its decision based on…
>
> --
>
> pppoatm: ensure a writable skb header
>
> This replaces the open-coded skb_realloc_headroom() dance in pppoatm_send()
> with skb_cow_head(), so the LLC header is pushed into headroom that is both
> large enough and writable.  It also removes the temporary skb allocation
> that used to happen when the ATM socket buffer was full.
>
> > diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
> > index 6da52d12df68e..4ce7fa6e493e9 100644
> > --- a/net/atm/pppoatm.c
> > +++ b/net/atm/pppoatm.c
> > @@ -317,21 +317,12 @@ static int pppoatm_send(struct ppp_channel *chan, struct sk_buff *skb)
> >
> >       switch (pvcc->encaps) {         /* LLC encapsulation needed */
> >       case e_llc:
> > -             if (skb_headroom(skb) < LLC_LEN) {
> > -                     struct sk_buff *n;
> > -                     n = skb_realloc_headroom(skb, LLC_LEN);
> > -                     if (n != NULL &&
> > -                         !pppoatm_may_send(pvcc, n->truesize)) {
> > -                             kfree_skb(n);
> > -                             goto nospace;
> > -                     }
> > -                     consume_skb(skb);
> > -                     skb = n;
> > -                     if (skb == NULL) {
> > -                             bh_unlock_sock(sk_atm(vcc));
> > -                             return DROP_PACKET;
> > -                     }
> > -             } else if (!pppoatm_may_send(pvcc, skb->truesize))
> > +             if (skb_cow_head(skb, LLC_LEN)) {
> > +                     bh_unlock_sock(sk_atm(vcc));
> > +                     kfree_skb(skb);
> > +                     return DROP_PACKET;
> > +             }
> > +             if (!pppoatm_may_send(pvcc, skb->truesize))
> >                       goto nospace;
>
> [Severity: Medium]
> Does this change how the SC_COMP_PROT undo heuristic behaves at the nospace
> label for skbs that arrive with less than LLC_LEN headroom?
>
> The head expansion now happens on the original skb, before the
> pppoatm_may_send() admission check, so when admission fails we reach:
>
> net/atm/pppoatm.c:pppoatm_send() {
>         ...
> nospace:
>         bh_unlock_sock(sk_atm(vcc));
>         if ((pvcc->flags & SC_COMP_PROT) && skb_headroom(skb) > 0 &&
>             skb->data[-1] == '\0')
>                 (void) skb_push(skb, 1);
>         return 0;
> }
>
> For an skb that entered with zero headroom, skb_cow_head() goes through
> pskb_expand_head(), which allocates with kmalloc_reserve() (no __GFP_ZERO)
> and only copies the old data:
>
> net/core/skbuff.c:pskb_expand_head() {
>         ...
>         data = kmalloc_reserve(&size, gfp_mask, NUMA_NO_NODE, NULL);
>         if (!data)
>                 goto nodata;
>         size = SKB_WITH_OVERHEAD(size);
>         memcpy(data + nhead, skb->head, skb_tail_pointer(skb) - skb->head);
>         ...
> }
>
> The new bytes in front of the copied data are never initialized, so is
> skb->data[-1] here reading stale slab content rather than a byte this
> function pulled?  If that byte happens to be 0x00, the skb_push(skb, 1)
> prepends a spurious zero to a frame from which nothing was ever pulled at:
>
>         if (skb->data[0] == '\0' && (pvcc->flags & SC_COMP_PROT))
>                 (void) skb_pull(skb, 1);
>
> Before this change the same case took the skb_realloc_headroom() branch,
> freed the copy and jumped to nospace with the original zero-headroom skb, so
> the skb_headroom(skb) > 0 test suppressed the push.
>
> Frames written to /dev/ppp look like they can hit this: ppp_write() does
>
> drivers/net/ppp/ppp_generic.c:ppp_write() {
>         ...
>         skb = alloc_skb(count + pf->hdrlen, GFP_KERNEL);
>         if (!skb)
>                 goto out;
>         skb_reserve(skb, pf->hdrlen);
>         ...
> }
>
> and pppoatm never assigns chan.hdrlen, so pf->hdrlen is 0 and control frames
> with a non-zero first protocol byte (LCP 0xc021, IPCP 0x8021, PAP 0xc023)
> arrive with no headroom and no entry-time pull.  With the VCC queue full,
> pppoatm_may_send() returns 0 and the goto nospace path runs.
>
> Would it be more robust to record whether the entry-time skb_pull(skb, 1)
> actually happened in a local bool, and test that at nospace, instead of
> inferring it from skb->data[-1]?

Good idea, I will add this to V2.

pw-bot: cr