Le mer. 4 nov. 2026 à 15:00, [off-list ref] a écrit :
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] The patch adds a fallback in gro_try_pull_from_frag0()
(net/core/gro.c:465): when frag0_len is 0 it calls `pskb_may_pull(skb,…
--
net: gro: invalidate frag0 after a pull
This patch adds skb_gro_frag0_invalidate(), which clears
NAPI_GRO_CB(skb)->frag0_len. It is called before the pulls in
skb_gro_header_slow(), skb_gro_receive_list(), udp_gro_receive_segment(),
esp4_gro_receive() and esp6_gro_receive(). It also changes
gro_try_pull_from_frag0() so it copies from frag0 only while frag0_len is
not zero, and falls back to pskb_may_pull() otherwise.
quoted
diff --git a/net/core/gro.c b/net/core/gro.c
index 1b033855a20db..49e7f64f27756 100644
--- a/net/core/gro.c
+++ b/net/core/gro.c
[ ... ]
quoted
@@ -454,8 +455,14 @@ static void gro_try_pull_from_frag0(struct sk_buff *skb)
{
int grow = skb_gro_offset(skb) - skb_headlen(skb);
- if (grow > 0)
+ if (grow <= 0)
+ return;
+
+ /* frag0 can only be used if no pull happened. */
+ if (likely(NAPI_GRO_CB(skb)->frag0_len))
gro_pull_from_frag0(skb, grow);
+ else
+ pskb_may_pull(skb, skb_gro_offset(skb));
}
[Severity: Medium]
Is it safe to ignore the return value of pskb_may_pull() here?
gro_try_pull_from_frag0() still returns void. Before calling it,
dev_gro_receive() has already incremented gro_list->count (or called
gro_flush_oldest()). After it returns, the skb is held unconditionally:
net/core/gro.c:dev_gro_receive() {
...
if (unlikely(gro_list->count >= MAX_GRO_SKBS))
gro_flush_oldest(gro, &gro_list->list);
else
gro_list->count++;
/* Must be called before setting NAPI_GRO_CB(skb)->{age|last} */
gro_try_pull_from_frag0(skb);
...
list_add(&skb->list, &gro_list->list);
ret = GRO_HELD;
...
}
Before this patch, gro_pull_from_frag0() was the only thing done here,
and it could not fail because frag0_len is bounded by the tailroom.
One way to reach this point with frag0_len == 0 is through
esp4_gro_receive() and esp6_gro_receive(). With this patch they do:
skb_gro_frag0_invalidate(skb);
if (!pskb_pull(skb, offset))
return NULL;
This returns without setting NAPI_GRO_CB(skb)->flush. For an ordinary
unfragmented IPv4 packet, inet_gro_receive() computes flush as 0, so
dev_gro_receive() takes the GRO_HELD path.
offset == skb_gro_offset(skb), so the retry in gro_try_pull_from_frag0()
is the same pull that just failed.
If the held skb's linear area does not reach skb_gro_offset(), later
packets in the same bucket would read stale tailroom bytes as headers:
net/ipv4/af_inet.c:inet_gro_receive() {
...
iph2 = (struct iphdr *)(p->data + off);
...
}
The same applies to ipv6_gro_receive(), udp_gro_receive() and the
tcp_hdr(p)/udp_hdr(p) lookups.
If one of these bogus headers matches and a merge happens,
inet_gro_complete() writes tot_len and check through skb->data + nhoff.
Those writes would go into tailroom rather than the real header, and the
aggregated packet would keep its original single-segment tot_len.
The window looks narrow. The retry can only fail for an skb that entered
GRO with headlen == 0 and frags[0] mapped as frag0, and that skb must
also be either:
- cloned, with pskb_expand_head(..., GFP_ATOMIC) failing, or
- carrying unreadable frags while frags[0] is a normal page.
No concrete in-tree source of such skbs has been identified, but they
don't look impossible either.
Could gro_try_pull_from_frag0() return a bool? On failure,
dev_gro_receive() could undo the count change and take the normal path
instead of holding the skb.
Yes. In v4, gro_try_pull_from_frag0() returns false if the pull fails.
dev_gro_receive() calls it before changing gro_list->count, and takes
the normal path instead of holding the skb.