[PATCH net-next 0/9] net: stop calling __pskb_pull_tail() from drivers

[PATCH net-next 0/9] net: stop calling __pskb_pull_tail() from drivers

[Josef Bacik]

__pskb_pull_tail() is the slow path behind pskb_may_pull() and
__skb_linearize(), and drivers shouldn't be calling it directly.  It
takes the number of bytes to pull relative to the current head and does
no bounds checking on it.  Ask for more than the skb holds and it BUG()s
in skb_copy_bits().  Ask for a negative amount, which is what a caller
computing "len - skb_headlen(skb)" gets once the head is already long
enough, and skb_copy_bits() is handed a length of nearly 4GB to copy
into the head.  Under KASAN that shows up as an out-of-bounds read of
size 4294967288, after which __pskb_pull_tail() returns success with the
skb's head and paged lengths no longer matching its frags.

It also returns NULL on failure with nothing making the caller look at
it.  Eight network drivers call it directly and four of them don't
check.  All four are RX paths where a failed pull is followed by
eth_type_trans() or skb_pull(), which BUG() in __skb_pull() once the
head is shorter than what they pull.  As far as I can tell none of the
four can fail today, since the skb is fresh, isn't shared and has room
in the head, but that only holds because of how each driver happens to
allocate.

pskb_may_pull() and __skb_linearize() take the length the head should
end up with, check it against skb->len and fail cleanly, which is what
every one of these callers wants.  Convert all eight drivers to them,
check the result, and drop the packet on failure.

The last patch makes skb_condense() check its pull as well.  It can't
fail there today, but it would undercount truesize if it ever did.

The callers left in aoe and xen-netfront are fixed separately, through
the block and net trees:

  https://lore.kernel.org/r/20261007-b4-aoe-short-packets-v1-1-db5155f7bb9c@toxicpanda.com
  https://lore.kernel.org/r/20261007-b4-xen-netfront-short-head-v1-1-12d7113a7e4e@toxicpanda.com

Once those are in I'd like to stop exporting __pskb_pull_tail() so new
drivers can't pick it up.

Testing: every touched file builds with W=1 on x86_64 allmodconfig
(ftmac100 on i386, it's 32-bit only).  e1000e under QEMU, which hits
the converted TSO workaround on every TSO packet, passes TCP traffic
between two emulated 82574Ls, and with failslab failing 5% of atomic
allocations the failed pulls drop the packet and nothing falls over.
The same setup with jumbo frames and copybreak off makes
skb_condense() pull frag data into the head tens of thousands of times
a run.  xen-netfront ran as a Xen HVM guest under QEMU's KVM Xen
emulation, with the backend changed to spread frames over 18 and 19 RX
slots, which takes the converted pull in xennet_fill_frags() and its
overflow path.  The other drivers are compile tested only.

Thanks,
Josef

---
Josef Bacik (9):
      net: ftmac100: check for failure when pulling in the RX header
      net/mlx5e: check for failure when pulling the Ethernet header after XDP
      net: niu: check for failure when pulling in the RX header
      xen/netfront: check for failure when pulling in xennet_fill_frags()
      e1000: use pskb_may_pull() in the 82544 TSO workaround
      e1000e: use pskb_may_pull() in the 82571/2/3 TSO workaround
      netxen: use pskb_may_pull() to pull excess TX frags into the head
      qlcnic: use pskb_may_pull() to pull excess TX frags into the head
      net: skbuff: don't reset truesize in skb_condense() if the pull fails

 drivers/net/ethernet/faraday/ftmac100.c            | 10 ++++++++--
 drivers/net/ethernet/intel/e1000/e1000_main.c      |  5 ++---
 drivers/net/ethernet/intel/e1000e/netdev.c         |  4 ++--
 drivers/net/ethernet/mellanox/mlx5/core/en_rx.c    |  9 ++++++---
 .../net/ethernet/qlogic/netxen/netxen_nic_main.c   |  2 +-
 drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c     |  2 +-
 drivers/net/ethernet/sun/niu.c                     |  6 +++++-
 drivers/net/xen-netfront.c                         | 22 ++++++++++------------
 net/core/skbuff.c                                  |  3 ++-
 9 files changed, 37 insertions(+), 26 deletions(-)
---
base-commit: a5e7d8e446af9803e37a3b6a4d416fb41178348f
change-id: 20261006-b4-pskb-pull-tail-drivers-ff4fb2964e1a

[PATCH net-next 1/9] net: ftmac100: check for failure when pulling in the RX header

[Josef Bacik]

ftmac100_rx_packet() uses __pskb_pull_tail() to pull either the
Ethernet header or, for small frames, the whole frame into the skb
head, and ignores the return value.  If that pull ever failed,
eth_type_trans() would find less than ETH_HLEN in the head and BUG()
in __skb_pull().

It doesn't fail today because the skb is freshly allocated, isn't
shared and has room in the head, but that's only true because of how
this driver allocates.  Use pskb_may_pull() for the header and
__skb_linearize() for small frames, which is what the two pulls are
doing, and drop the frame if either fails.

Assisted-by: LLM
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 drivers/net/ethernet/faraday/ftmac100.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftmac100.c b/drivers/net/ethernet/faraday/ftmac100.c
index 40ba001d4b3f..a1eb04ead2d8 100644
--- a/drivers/net/ethernet/faraday/ftmac100.c
+++ b/drivers/net/ethernet/faraday/ftmac100.c
@@ -469,15 +469,21 @@ static bool ftmac100_rx_packet(struct ftmac100 *priv, int *processed)
 	if (length > 128) {
 		skb->truesize += PAGE_SIZE;
 		/* We pull the minimum amount into linear part */
-		__pskb_pull_tail(skb, ETH_HLEN);
+		ret = pskb_may_pull(skb, ETH_HLEN);
 	} else {
 		/* Small frames are copied into linear part to free one page */
-		__pskb_pull_tail(skb, length);
+		ret = !__skb_linearize(skb);
 	}
 	ftmac100_alloc_rx_page(priv, rxdes, GFP_ATOMIC);
 
 	ftmac100_rx_pointer_advance(priv);
 
+	if (unlikely(!ret)) {
+		netdev->stats.rx_dropped++;
+		kfree_skb(skb);
+		return true;
+	}
+
 	skb->protocol = eth_type_trans(skb, netdev);
 
 	netdev->stats.rx_packets++;

-- 
2.55.0

[PATCH net-next 2/9] net/mlx5e: check for failure when pulling the Ethernet header after XDP

[Josef Bacik]

When an XDP program leaves less than ETH_HLEN in the linear part of a
multi-buffer packet, mlx5e_skb_from_cqe_mpwrq_nonlinear() pulls the rest
of the Ethernet header in from the frags with __pskb_pull_tail() and
ignores the return value.  If that pull fails, eth_type_trans() later
finds less than ETH_HLEN in the head and BUG()s in __skb_pull().

Use pskb_may_pull() instead and drop the packet if it fails.  Pulling
min(ETH_HLEN, skb->len) keeps today's behaviour for a frame shorter
than an Ethernet header.

Assisted-by: LLM
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_rx.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
index e3f915beebe1..b1e1e30a77e2 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
@@ -2060,9 +2060,12 @@ mlx5e_skb_from_cqe_mpwrq_nonlinear(struct mlx5e_rq *rq, struct mlx5e_mpw_info *w
 				pagep->frags++;
 			while (++pagep < frag_page);
 
-			if (len < ETH_HLEN)
-				__pskb_pull_tail(skb, min(ETH_HLEN - len,
-							  skb->data_len));
+			if (len < ETH_HLEN &&
+			    !pskb_may_pull(skb, min(ETH_HLEN, skb->len))) {
+				rq->stats->buff_alloc_err++;
+				dev_kfree_skb_any(skb);
+				return NULL;
+			}
 		}
 	} else {
 		if (xdp_buff_has_frags(&mxbuf->xdp)) {

-- 
2.55.0

Re: [PATCH net-next 2/9] net/mlx5e: check for failure when pulling the Ethernet header after XDP

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] In mlx5e_skb_from_cqe_mpwrq_nonlinear()
  (drivers/net/ethernet/mellanox/mlx5/core/en_rx.c), the new drop path…

--

net/mlx5e: check for failure when pulling the Ethernet header after XDP

This patch replaces the unchecked __pskb_pull_tail() call in
mlx5e_skb_from_cqe_mpwrq_nonlinear() with pskb_may_pull(). If the pull
fails, the packet is dropped. The goal is to stop eth_type_trans() from
hitting BUG() in __skb_pull() when the linear part holds less than
ETH_HLEN.

> When an XDP program leaves less than ETH_HLEN in the linear part of a
> multi-buffer packet, mlx5e_skb_from_cqe_mpwrq_nonlinear() pulls the rest
> of the Ethernet header in from the frags with __pskb_pull_tail() and
> ignores the return value.  If that pull fails, eth_type_trans() later
> finds less than ETH_HLEN in the head and BUG()s in __skb_pull().

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> index e3f915beebe1b..b1e1e30a77e22 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> @@ -2060,9 +2060,12 @@ mlx5e_skb_from_cqe_mpwrq_nonlinear(struct mlx5e_rq *rq, struct mlx5e_mpw_info *w
>  				pagep->frags++;
>  			while (++pagep < frag_page);
>  
> -			if (len < ETH_HLEN)
> -				__pskb_pull_tail(skb, min(ETH_HLEN - len,
> -							  skb->data_len));
> +			if (len < ETH_HLEN &&
> +			    !pskb_may_pull(skb, min(ETH_HLEN, skb->len))) {
> +				rq->stats->buff_alloc_err++;
> +				dev_kfree_skb_any(skb);
> +				return NULL;
> +			}

[Severity: Low]
Can this new drop path actually be reached? Given how the skb is built
on the XDP path, it doesn't look like the pull can fail.

For an skb with frags and no frag_list, these are the only failure
returns in __pskb_pull_tail():

net/core/skbuff.c:__pskb_pull_tail() {
    ...
	if (!skb_frags_readable(skb))
		return NULL;

	if (eat > 0 || skb_cloned(skb)) {
		if (pskb_expand_head(skb, 0, eat > 0 ? eat + 128 : 0,
				     GFP_ATOMIC))
			return NULL;
	}
    ...
}

The frags are always readable here because XDP and memory providers
can't be used together:

- dev_xdp_attach() returns -EBUSY when dev_get_min_mp_channel_count()
  is non-zero.
- The memory provider binding code in netdev_rx_queue.c refuses when
  dev_xdp_prog_count() is non-zero.

The skb isn't cloned either, since mlx5e_build_linear_skb() has just
built it with napi_build_skb().

There is also always enough tailroom, so eat stays negative. The XDP
setup earlier in this function is:

	linear_hr = XDP_PACKET_HEADROOM;
	linear_data_len = 0;
	linear_frame_sz = MLX5_SKB_FRAG_SZ(linear_hr + MLX5E_RX_MAX_HEAD);

That leaves about 256 bytes past the linear tail, and the pull is never
more than ETH_HLEN bytes.

On the commit message's claim that this happens "When an XDP program
leaves less than ETH_HLEN in the linear part of a multi-buffer packet",
is that accurate?

The driver always gives XDP an empty linear area. So the pull runs for
every multi-buffer XDP_PASS packet whose program doesn't adjust the
head.

An XDP program also can't leave 1 to 13 bytes in the linear part,
because bpf_xdp_adjust_head() rejects it:

	if (unlikely(data < data_start ||
		     data > xdp->data_end - ETH_HLEN))
		return -EINVAL;

When frags are present, bpf_xdp_adjust_tail() only changes the frags,
so the linear data_end stays where it is. That means len is either 0 or
at least ETH_HLEN at this point.

If this change is meant as hardening, for example against later changes
to linear_frame_sz, could the commit message say so?

If there is a real way to make the pull fail, could the message describe
it and add a Fixes: tag for the commit that added the unchecked
__pskb_pull_tail()?

>  		}
>  	} else {
>  		if (xdp_buff_has_frags(&mxbuf->xdp)) {

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261007-b4-pskb-pull-tail-drivers-v1-0-9512b0fb977b%40toxicpanda.com

[PATCH net-next 3/9] net: niu: check for failure when pulling in the RX header

[Josef Bacik]

niu_process_rx_pkt() uses __pskb_pull_tail() to pull the hardware RX
header and the Ethernet header into the skb head, and ignores the
return value.  If that pull failed, the skb_pull() of the RX header
right after it would BUG() in __skb_pull().

It doesn't fail today because the skb is freshly allocated, isn't
shared and has room in the head.  Use pskb_may_pull() anyway and drop
the packet if it fails, rather than depending on that.

Assisted-by: LLM
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 drivers/net/ethernet/sun/niu.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/sun/niu.c b/drivers/net/ethernet/sun/niu.c
index c74a97fe5464..d1c0e868004d 100644
--- a/drivers/net/ethernet/sun/niu.c
+++ b/drivers/net/ethernet/sun/niu.c
@@ -3488,7 +3488,11 @@ static int niu_process_rx_pkt(struct napi_struct *napi, struct niu *np,
 
 	len += sizeof(*rh);
 	len = min_t(int, len, sizeof(*rh) + VLAN_ETH_HLEN);
-	__pskb_pull_tail(skb, len);
+	if (unlikely(!pskb_may_pull(skb, len))) {
+		rp->rx_dropped++;
+		kfree_skb(skb);
+		return num_rcr;
+	}
 
 	rh = (struct rx_pkt_hdr1 *) skb->data;
 	if (np->dev->features & NETIF_F_RXHASH)

-- 
2.55.0

[PATCH net-next 4/9] xen/netfront: check for failure when pulling in xennet_fill_frags()

[Josef Bacik]

When the skb already has MAX_SKB_FRAGS frags, xennet_fill_frags() pulls
the start of the packet into the head to try to free up a frag slot.
It ignores the return value of __pskb_pull_tail(), which works out only
because the nr_frags check right after it drops the packet if no slot
was freed.

Use pskb_may_pull() and take the error path explicitly if it fails.
pskb_may_pull() does nothing if the head already holds pull_to bytes,
so the BUG_ON() for pull_to < skb_headlen(skb), which protected the
subtraction, can go.

Assisted-by: LLM
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 drivers/net/xen-netfront.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index 2ed673649c48..6e01a883cc64 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -1174,18 +1174,11 @@ static int xennet_fill_frags(struct netfront_queue *queue,
 
 		RING_COPY_RESPONSE(&queue->rx, ++cons, &rx);
 
-		if (skb_shinfo(skb)->nr_frags == MAX_SKB_FRAGS) {
-			unsigned int pull_to = NETFRONT_SKB_CB(skb)->pull_to;
-
-			BUG_ON(pull_to < skb_headlen(skb));
-			__pskb_pull_tail(skb, pull_to - skb_headlen(skb));
-		}
-		if (unlikely(skb_shinfo(skb)->nr_frags >= MAX_SKB_FRAGS)) {
-			xennet_set_rx_rsp_cons(queue,
-					       ++cons + skb_queue_len(list));
-			kfree_skb(nskb);
-			return -ENOENT;
-		}
+		if (skb_shinfo(skb)->nr_frags == MAX_SKB_FRAGS &&
+		    unlikely(!pskb_may_pull(skb, NETFRONT_SKB_CB(skb)->pull_to)))
+			goto err;
+		if (unlikely(skb_shinfo(skb)->nr_frags >= MAX_SKB_FRAGS))
+			goto err;
 
 		skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags,
 				skb_frag_page(nfrag),
@@ -1198,6 +1191,11 @@ static int xennet_fill_frags(struct netfront_queue *queue,
 	xennet_set_rx_rsp_cons(queue, cons);
 
 	return 0;
+
+err:
+	xennet_set_rx_rsp_cons(queue, ++cons + skb_queue_len(list));
+	kfree_skb(nskb);
+	return -ENOENT;
 }
 
 static int checksum_setup(struct net_device *dev, struct sk_buff *skb)

-- 
2.55.0

Re: [PATCH net-next 4/9] xen/netfront: check for failure when pulling in xennet_fill_frags()

[netdev-bot+sashiko]

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

Critical: 0 · High: 2 (2) · 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).

- [High] The patch now sends pull failures explicitly to an error path
  that leaks the head skb.
- [High] net: xen-netfront: memory leak of head SKB on
  xennet_fill_frags() error path
- [Medium] In xennet_fill_frags() (drivers/net/xen-netfront.c), replacing
  __pskb_pull_tail(skb, pull_to - skb_headlen(skb)) with…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  xennet_fill_frags() (drivers/net/xen-netfront.c) reads each follow-on…
- [High] This problem wasn't introduced by this patch, but in
  xennet_poll() (drivers/net/xen-netfront.c)…

--

xen/netfront: check for failure when pulling in xennet_fill_frags()

When the skb already has MAX_SKB_FRAGS frags, this patch makes
xennet_fill_frags() call pskb_may_pull() instead of __pskb_pull_tail(), and
it now checks the return value. The BUG_ON() on pull_to is removed. Both
failure exits now share a common err label.

> diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
> index 2ed673649c488..6e01a883cc64a 100644
> --- a/drivers/net/xen-netfront.c
> +++ b/drivers/net/xen-netfront.c
> @@ -1174,18 +1174,11 @@ static int xennet_fill_frags(struct netfront_queue *queue,
>  
>  		RING_COPY_RESPONSE(&queue->rx, ++cons, &rx);
>  
> -		if (skb_shinfo(skb)->nr_frags == MAX_SKB_FRAGS) {
> -			unsigned int pull_to = NETFRONT_SKB_CB(skb)->pull_to;
> -
> -			BUG_ON(pull_to < skb_headlen(skb));
> -			__pskb_pull_tail(skb, pull_to - skb_headlen(skb));
> -		}
> -		if (unlikely(skb_shinfo(skb)->nr_frags >= MAX_SKB_FRAGS)) {
> -			xennet_set_rx_rsp_cons(queue,
> -					       ++cons + skb_queue_len(list));
> -			kfree_skb(nskb);
> -			return -ENOENT;
> -		}
> +		if (skb_shinfo(skb)->nr_frags == MAX_SKB_FRAGS &&
> +		    unlikely(!pskb_may_pull(skb, NETFRONT_SKB_CB(skb)->pull_to)))
> +			goto err;
> +		if (unlikely(skb_shinfo(skb)->nr_frags >= MAX_SKB_FRAGS))
> +			goto err;

[Severity: Medium]
Is pskb_may_pull() really equivalent to the old call when pull_to equals
skb_headlen(skb)?

pskb_may_pull_reason() returns before it reaches __pskb_pull_tail():

    if (likely(len <= skb_headlen(skb)))
        return SKB_NOT_DROPPED_YET;

The old code still called __pskb_pull_tail(skb, 0) in that case. Its
pull_pages loop removes every zero-length frag, even with eat == 0:

    if (size <= eat) {
        skb_frag_unref(skb, i);
        eat -= size;
    }

That lowered nr_frags and freed a slot.

xennet_get_responses() accepts zero-length responses, since it only rejects
status < 0 and offset + status > XEN_PAGE_SIZE. It also allows
XEN_NETIF_NR_SLOTS_MIN + 1 (19) slots when the first response is at most
RX_COPY_THRESHOLD bytes.

Take the default MAX_SKB_FRAGS of 17, no XDP, a 64-byte first slot,
non-empty slots 2-17, a zero-length slot 18 and a non-empty slot 19:

    slot 18: nr_frags == 17, pull 64 bytes, frags[0] consumed -> 16
             zero-length slot 18 added -> 17
    slot 19: nr_frags == 17, pull_to == headlen, pskb_may_pull() no-op
             nr_frags >= MAX_SKB_FRAGS -> goto err

The old code would have dropped the zero-length frag at slot 19 and
accepted the packet. A zero-length first slot gives the same result,
because frags[0] is then zero-length.

Are these packets now dropped and counted in rx_errors, when the old code
assembled them? The commit message says "pskb_may_pull() does nothing if the
head already holds pull_to bytes", but that does not match the old
behaviour in this case. These drops also go through the xennet_poll() err
path discussed below.

[Severity: High]
This is a pre-existing issue, but in xennet_poll() pull_to comes from the
first slot's status. It has an upper cap but no lower bound:

    NETFRONT_SKB_CB(skb)->pull_to = rx->status;
    if (NETFRONT_SKB_CB(skb)->pull_to > RX_COPY_THRESHOLD)
        NETFRONT_SKB_CB(skb)->pull_to = RX_COPY_THRESHOLD;

Suppose the backend sends a first slot with status below ETH_HLEN and
XEN_NETRXF_more_data set, followed by non-empty slots.
handle_incoming_queue() pulls only pull_to bytes and then calls
eth_type_trans():

    handle_incoming_queue()
      __pskb_pull_tail(skb, pull_to - skb_headlen(skb))
      eth_type_trans()
        eth_skb_pull_mac()
          skb_pull_inline(skb, ETH_HLEN)
            __skb_pull()
              skb->len < skb->data_len -> BUG()

For example, a 5-byte first slot followed by a 1000-byte slot gives len =
1005, data_len = 1000 and headlen = 5. Pulling ETH_HLEN leaves len = 991,
which is less than data_len.

Can a backend crash the guest this way? Should pull_to be clamped to at
least ETH_HLEN? With a single short slot, skb_pull_inline() returns NULL,
so this only applies when frags follow.

>  
>  		skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags,
>  				skb_frag_page(nfrag),

[Severity: High]
This isn't a bug introduced by this patch, but xennet_fill_frags() reads
each follow-on response from the shared ring a second time. It then uses
offset and status without checking them again:

    RING_COPY_RESPONSE(&queue->rx, ++cons, &rx);
    ...
    skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags,
                    skb_frag_page(nfrag),
                    rx.offset, rx.status, PAGE_SIZE);

The only bounds check runs in xennet_get_responses(), on a separate local
copy that is then thrown away:

    if (unlikely(rx->status < 0 ||
                 rx->offset + rx->status > XEN_PAGE_SIZE)) {

RING_COPY_() does a volatile load, so this is a fresh read of memory that
the backend can still write.

Could a backend rewrite the slot between the check and this read? A
negative status would lower skb->len and skb->data_len and store a huge
frag size. An offset plus status past the page would create a frag that
runs past the RX page.

Commit 8446066bf8c1 was meant to make netfront read each response only
once. Could the copy already validated in xennet_get_responses() be reused
here?

[ ... ]

> @@ -1198,6 +1191,11 @@ static int xennet_fill_frags(struct netfront_queue *queue,
>  	xennet_set_rx_rsp_cons(queue, cons);
>  
>  	return 0;
> +
> +err:
> +	xennet_set_rx_rsp_cons(queue, ++cons + skb_queue_len(list));
> +	kfree_skb(nskb);
> +	return -ENOENT;
>  }

[Severity: High]
net: xen-netfront: memory leak of head SKB on xennet_fill_frags() error path

1. Consequence: Memory leak of the main/head SKB and its data on the RX error path. Over time, an attacker triggering this error could exhaust system memory (DoS).
2. Triggering path: In xennet_poll(), a head SKB is dequeued from tmpq. It is passed to xennet_fill_frags(), which attempts to merge additional fragments. If pskb_may_pull() fails (e.g., due to memory allocation failure) or if nr_frags still exceeds MAX_SKB_FRAGS, xennet_fill_frags() returns -ENOENT without freeing the head SKB. Back in xennet_poll(), this triggers goto err, which loops over the remaining fragments in tmpq using while ((skb = __skb_dequeue(&tmpq))). This loop overwrites the skb local variable, causing the head SKB pointer to be permanently lost and leaked.
3. Reachability: High. A remote attacker could potentially send highly fragmented packets designed to exceed MAX_SKB_FRAGS to intentionally trigger this error path and exhaust memory.

[Severity: High]
Does this error path leak the head skb?

This exit frees only nskb and leaves the partly built head skb with the
caller. In xennet_poll(), the head skb has already been taken off tmpq:

    skb = __skb_dequeue(&tmpq);
    ...
    if (unlikely(xennet_fill_frags(queue, skb, &tmpq)))
        goto err;

The err label there only drains tmpq, and it overwrites the local skb
pointer:

err:
    while ((skb = __skb_dequeue(&tmpq)))
        __skb_queue_tail(&errq, skb);

The GSO failure path just above calls __skb_queue_head(&tmpq, skb) before
goto err, but this path does not. The head skb is never freed or requeued.

That would leak the linear buffer and the page_pool frag pages attached to
the skb. These include pages moved over from earlier nskbs whose nr_frags
was set to 0.

The leak has existed since commit ad4f15dc2c70 (2018). However, the commit
message assumes this path drops the packet ("take the error path
explicitly"), and the change above sends more packets into it.

Should the head skb be put back on tmpq, or freed, before goto err in
xennet_poll()?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261007-b4-pskb-pull-tail-drivers-v1-0-9512b0fb977b%40toxicpanda.com

[PATCH net-next 5/9] e1000: use pskb_may_pull() in the 82544 TSO workaround

[Josef Bacik]

e1000_xmit_frame() uses __pskb_pull_tail() to pull up to 4 bytes of
payload into the head for the 82544 TSO workaround.  It already checks
the result.  Switch to pskb_may_pull(), which takes the length the head
should end up with and checks it against the skb, so this driver no
longer calls __pskb_pull_tail() directly.

Assisted-by: LLM
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 drivers/net/ethernet/intel/e1000/e1000_main.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
index d7f5c6f16142..3a55b211f5a6 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_main.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
@@ -3155,9 +3155,8 @@ static netdev_tx_t e1000_xmit_frame(struct sk_buff *skb,
 				    & 4)
 					break;
 				pull_size = min((unsigned int)4, skb->data_len);
-				if (!__pskb_pull_tail(skb, pull_size)) {
-					e_err(drv, "__pskb_pull_tail "
-					      "failed.\n");
+				if (!pskb_may_pull(skb, len + pull_size)) {
+					e_err(drv, "pskb_may_pull failed.\n");
 					dev_kfree_skb_any(skb);
 					return NETDEV_TX_OK;
 				}

-- 
2.55.0

[PATCH net-next 6/9] e1000e: use pskb_may_pull() in the 82571/2/3 TSO workaround

[Josef Bacik]

e1000_xmit_frame() uses __pskb_pull_tail() to pull up to 4 bytes of
payload into the head when the head holds only the TSO headers.  It
already checks the result.  Switch to pskb_may_pull(), which takes the
length the head should end up with and checks it against the skb, so
this driver no longer calls __pskb_pull_tail() directly.

Assisted-by: LLM
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 drivers/net/ethernet/intel/e1000e/netdev.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 844f31ab37ad..e216868e15cb 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -5859,8 +5859,8 @@ static netdev_tx_t e1000_xmit_frame(struct sk_buff *skb,
 			unsigned int pull_size;
 
 			pull_size = min_t(unsigned int, 4, skb->data_len);
-			if (!__pskb_pull_tail(skb, pull_size)) {
-				e_err("__pskb_pull_tail failed.\n");
+			if (!pskb_may_pull(skb, len + pull_size)) {
+				e_err("pskb_may_pull failed.\n");
 				dev_kfree_skb_any(skb);
 				return NETDEV_TX_OK;
 			}

-- 
2.55.0

Re: [PATCH net-next 6/9] e1000e: use pskb_may_pull() in the 82571/2/3 TSO workaround

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] After this patch, two unrelated error paths in e1000e log the
  same message.

--

e1000e: use pskb_may_pull() in the 82571/2/3 TSO workaround

This commit changes the 82571/2/3 TSO workaround in e1000_xmit_frame() to
use pskb_may_pull(skb, len + pull_size) in place of a direct
__pskb_pull_tail() call. With this change the driver no longer calls
__pskb_pull_tail() itself. The error message printed on failure is
updated to match.

> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> index 844f31ab37ad4..e216868e15cba 100644
> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> @@ -5859,8 +5859,8 @@ static netdev_tx_t e1000_xmit_frame(struct sk_buff *skb,
>  			unsigned int pull_size;
>  
>  			pull_size = min_t(unsigned int, 4, skb->data_len);
> -			if (!__pskb_pull_tail(skb, pull_size)) {
> -				e_err("__pskb_pull_tail failed.\n");
> +			if (!pskb_may_pull(skb, len + pull_size)) {
> +				e_err("pskb_may_pull failed.\n");

[Severity: Low]
This isn't a bug, but this TX message now matches a message that
already exists on the jumbo RX path in the same file:

drivers/net/ethernet/intel/e1000e/netdev.c:e1000_clean_jumbo_rx_irq() {
    ...
		/* eth type trans needs skb->data to point to something */
		if (!pskb_may_pull(skb, ETH_HLEN)) {
			e_err("pskb_may_pull failed.\n");
			dev_kfree_skb_irq(skb);
			goto next_desc;
		}
    ...
}

e_err() expands to netdev_err(adapter->netdev, ...). The log line
therefore shows only the device name, not the function or the
direction.

Before this patch the TX site printed "__pskb_pull_tail failed.", so the
two failures could be told apart in dmesg. After it, a failed RX
ETH_HLEN pull and a failed pull in the TX TSO workaround print the same
line.

Could the TX site use a TX-specific message instead, for example
"TSO workaround pskb_may_pull failed.\n"?

>  				dev_kfree_skb_any(skb);
>  				return NETDEV_TX_OK;
>  			}

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261007-b4-pskb-pull-tail-drivers-v1-0-9512b0fb977b%40toxicpanda.com

[PATCH net-next 7/9] netxen: use pskb_may_pull() to pull excess TX frags into the head

[Josef Bacik]

netxen_nic_xmit_frame() pulls the frags that don't fit in a TX
descriptor into the head with __pskb_pull_tail().  It already checks
the result.  Switch to pskb_may_pull(), which takes the length the head
should end up with and checks it against the skb, so this driver no
longer calls __pskb_pull_tail() directly.

Assisted-by: LLM
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c b/drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c
index 67d9bf69f8f2..202deb64ddef 100644
--- a/drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c
+++ b/drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c
@@ -2045,7 +2045,7 @@ netxen_nic_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
 			delta += skb_frag_size(frag);
 		}
 
-		if (!__pskb_pull_tail(skb, delta))
+		if (!pskb_may_pull(skb, skb_headlen(skb) + delta))
 			goto drop_packet;
 
 		frag_count = 1 + skb_shinfo(skb)->nr_frags;

-- 
2.55.0

Re: [PATCH net-next 7/9] netxen: use pskb_may_pull() to pull excess TX frags into the head

[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

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

- [Medium] In netxen_nic_xmit_frame()
  (drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c), the new…

--

netxen: use pskb_may_pull() to pull excess TX frags into the head

This patch changes how netxen_nic_xmit_frame() pulls the TX frags that
don't fit in a descriptor into the head. It used to call
__pskb_pull_tail(skb, delta) directly. It now calls
pskb_may_pull(skb, skb_headlen(skb) + delta).

> diff --git a/drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c b/drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c
> index 67d9bf69f8f29..202deb64ddefb 100644
> --- a/drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c
> +++ b/drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c
> @@ -2045,7 +2045,7 @@ netxen_nic_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
>  			delta += skb_frag_size(frag);
>  		}
>  
> -		if (!__pskb_pull_tail(skb, delta))
> +		if (!pskb_may_pull(skb, skb_headlen(skb) + delta))
>  			goto drop_packet;
>  
>  		frag_count = 1 + skb_shinfo(skb)->nr_frags;

[Severity: Medium]
Does this behave the same as the old call when delta is 0?

delta is the sum of the sizes of the leading excess frags. It is 0 when
every frag in frags[0 .. frag_count - NETXEN_MAX_FRAGS_PER_TX - 1] has
size 0.

The old __pskb_pull_tail(skb, 0) still ran the pull_pages loop. With
eat == 0, every zero-size frag matched the size <= eat test:

net/core/skbuff.c:__pskb_pull_tail() {
    ...
	eat = delta;
	k = 0;
	for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
		int size = skb_frag_size(&skb_shinfo(skb)->frags[i]);

		if (size <= eat) {
			skb_frag_unref(skb, i);
			eat -= size;
		} else {
		...
	}
	skb_shinfo(skb)->nr_frags = k;
    ...
}

So those frags were dropped, and the recomputed frag_count was back
within the limit.

With this patch the length passed is exactly skb_headlen(skb). That makes
pskb_may_pull_reason() return early without calling __pskb_pull_tail():

include/linux/skbuff.h:pskb_may_pull_reason() {
    ...
	if (likely(len <= skb_headlen(skb)))
		return SKB_NOT_DROPPED_YET;
    ...
}

The skb is left unchanged, so frag_count stays above
NETXEN_MAX_FRAGS_PER_TX (14), and nothing checks it again. After that:

netxen_nic_xmit_frame()
    netxen_map_tx_skb()        maps every frag, zero-length ones included
    netxen_set_tx_frags_len()  writes the oversized frag count
    descriptor loop            posts buffer_length[k] = 0 entries across
                               extra cmd_desc_type0 descriptors

The comment above this block says only 14 frags are supported for normal
(non-TSO) packets. Could this post a descriptor chain the hardware
doesn't support?

When delta > 0 the two calls are equivalent, so only the delta == 0 case
changes. It isn't clear whether zero-length leading frags can reach a TX
driver today. Nothing on the TX path appears to rule them out, though.

The commit message describes this as a direct swap:

  "It already checks the result.  Switch to pskb_may_pull(), which takes
  the length the head should end up with and checks it against the skb"

Would it make sense to re-check frag_count against
NETXEN_MAX_FRAGS_PER_TX after the pull, and drop or linearize the skb if
it is still too large?

The later patch "qlcnic: use pskb_may_pull() to pull excess TX frags into
the head" makes the same change in qlcnic_xmit_frame(). It looks like it
has the same delta == 0 gap.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261007-b4-pskb-pull-tail-drivers-v1-0-9512b0fb977b%40toxicpanda.com

[PATCH net-next 8/9] qlcnic: use pskb_may_pull() to pull excess TX frags into the head

[Josef Bacik]

qlcnic_xmit_frame() pulls the frags that don't fit in a TX descriptor
into the head with __pskb_pull_tail().  It already checks the result.
Switch to pskb_may_pull(), which takes the length the head should end
up with and checks it against the skb, so this driver no longer calls
__pskb_pull_tail() directly.

Assisted-by: LLM
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c
index 761ef3bc8193..4a81d499e51c 100644
--- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c
+++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c
@@ -682,7 +682,7 @@ netdev_tx_t qlcnic_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
 		for (i = 0; i < (frag_count - QLCNIC_MAX_FRAGS_PER_TX); i++)
 			delta += skb_frag_size(&skb_shinfo(skb)->frags[i]);
 
-		if (!__pskb_pull_tail(skb, delta))
+		if (!pskb_may_pull(skb, skb_headlen(skb) + delta))
 			goto drop_packet;
 
 		frag_count = 1 + skb_shinfo(skb)->nr_frags;

-- 
2.55.0

Re: [PATCH net-next 8/9] qlcnic: use pskb_may_pull() to pull excess TX frags into the head

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] In qlcnic_xmit_frame()
  (drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c:681-688), changing…

--

qlcnic: use pskb_may_pull() to pull excess TX frags into the head

This patch changes qlcnic_xmit_frame() so that it no longer calls
__pskb_pull_tail(skb, delta) directly when a non-GSO skb has more frags
than QLCNIC_MAX_FRAGS_PER_TX. It calls pskb_may_pull(skb,
skb_headlen(skb) + delta) instead. The commit message presents this as
an equivalent API swap.

> diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c
> index 761ef3bc8193a..4a81d499e51c0 100644
> --- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c
> +++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c
> @@ -682,7 +682,7 @@ netdev_tx_t qlcnic_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
>  		for (i = 0; i < (frag_count - QLCNIC_MAX_FRAGS_PER_TX); i++)
>  			delta += skb_frag_size(&skb_shinfo(skb)->frags[i]);
>  
> -		if (!__pskb_pull_tail(skb, delta))
> +		if (!pskb_may_pull(skb, skb_headlen(skb) + delta))
>  			goto drop_packet;
>  
>  		frag_count = 1 + skb_shinfo(skb)->nr_frags;

[Severity: Low]
Is this exactly equivalent when delta is 0?

delta is a byte count. It is 0 if all of the leading excess frags of a
non-GSO skb with more than 14 frags are zero-length.

In that case pskb_may_pull_reason() returns early and never calls
__pskb_pull_tail():

include/linux/skbuff.h:pskb_may_pull_reason() {
    ...
	if (likely(len <= skb_headlen(skb)))
		return SKB_NOT_DROPPED_YET;
    ...
}

The old direct call to __pskb_pull_tail(skb, 0) always reached the
pull_pages loop. With eat == 0, every zero-size frag matches
size <= eat, so it is unreferenced and dropped from frags[]:

net/core/skbuff.c:__pskb_pull_tail() {
    ...
pull_pages:
	eat = delta;
	k = 0;
	for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
		int size = skb_frag_size(&skb_shinfo(skb)->frags[i]);

		if (size <= eat) {
			skb_frag_unref(skb, i);
			eat -= size;
		} else {
    ...
	}
	skb_shinfo(skb)->nr_frags = k;
    ...
}

With the new call, nr_frags does not change. The recomputed frag_count
stays above QLCNIC_MAX_FRAGS_PER_TX, and nothing checks it again.

After that, qlcnic_map_tx_skb() maps every frag, including the
zero-length ones. qlcnic_set_tx_frags_len() then writes this frag_count
into the first descriptor. The descriptor loop programs more than 14
buffers for a non-TSO packet, which the comment above this block says
the hardware does not support:

	/* 14 frags supported for normal packet and
	 * 32 frags supported for TSO packet
	 */

This needs an unusual skb layout, and no in-tree producer of it is
known. Even so, could the frag_count be rechecked after the pull, or the
commit message mention this difference?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261007-b4-pskb-pull-tail-drivers-v1-0-9512b0fb977b%40toxicpanda.com

[PATCH net-next 9/9] net: skbuff: don't reset truesize in skb_condense() if the pull fails

[Josef Bacik]

skb_condense() pulls all of the frag data into the head and then sets
truesize to cover just the head, but it ignores the return value of
__pskb_pull_tail().  If the pull failed, the frags would still be
attached and truesize would undercount them.

It can't fail today.  The caller has checked that the head has room,
that the skb isn't cloned and that the frags are readable, and pulling
all of data_len eats every frag_list skb whole, so nothing is
allocated.  Check the result anyway and leave the skb alone on
failure, so this stays correct if any of that changes.  Use
__skb_linearize(), which is what this pull is.

Assisted-by: LLM
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 net/core/skbuff.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 43ebe61c7fc4..70df607da05e 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -7118,7 +7118,8 @@ void skb_condense(struct sk_buff *skb)
 			return;
 
 		/* Nice, we can free page frag(s) right now */
-		__pskb_pull_tail(skb, skb->data_len);
+		if (__skb_linearize(skb))
+			return;
 	}
 	/* At this point, skb->truesize might be over estimated,
 	 * because skb had a fragment, and fragments do not tell

-- 
2.55.0