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 <[email protected]> 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-8f2bd9bee...@toxicpanda.com?part=5
