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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] netfront: memory leak of head skb in xennet_poll() when xennet_fill_frags() fails
--

commit f1df1b5218b129f50d5ffc52995e379ca9e3fc59
Author: Josef Bacik <josef@toxicpanda.com>

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

When the skb already has MAX_SKB_FRAGS frags, xennet_fill_frags() calls
__pskb_pull_tail() to free up a frag slot.  That frees slots in two
ways: it pulls the start of the packet into the head, and it releases
empty frags, even when there's nothing left to pull.

Use pskb_may_pull() followed by skb_drop_empty_frags(), and take the
error path explicitly if either 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.

> diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
> index 2ed673649c488..007fa3bbc9e66 100644
> --- a/drivers/net/xen-netfront.c
> +++ b/drivers/net/xen-netfront.c
> @@ -1174,18 +1174,15 @@ 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;
> -		}
> +		/* Out of frag slots: pull the start of the packet into the
> +		 * head and drop empty frags to make room.
> +		 */
> +		if (skb_shinfo(skb)->nr_frags == MAX_SKB_FRAGS &&
> +		    unlikely(!pskb_may_pull(skb, NETFRONT_SKB_CB(skb)->pull_to) ||
> +			     skb_drop_empty_frags(skb, GFP_ATOMIC)))
> +			goto err;
> +		if (unlikely(skb_shinfo(skb)->nr_frags >= MAX_SKB_FRAGS))
> +			goto err;

[Severity: Medium]
When xennet_fill_frags() returns an error here, the execution jumps to the err
label in xennet_poll() where it leaks the head skb:

drivers/net/xen-netfront.c:xennet_poll() {
...
		skb = __skb_dequeue(&tmpq);
...
		if (unlikely(xennet_fill_frags(queue, skb, &tmpq)))
			goto err;
...
err:
			while ((skb = __skb_dequeue(&tmpq)))
				__skb_queue_tail(&errq, skb);
...
}

Because the while loop unconditionally overwrites the head skb variable without
freeing it or queuing it to errq, does this code permanently leak the head
packet?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008-b4-pskb-pull-tail-drivers-v2-0-8f2bd9bee138@toxicpanda.com?part=5