Re: [PATCH net] net: gro: set the last skb->next to NULL when it get merged

On 2021/10/27 16:56, Jason Xing wrote:

On Wed, Oct 27, 2021 at 4:07 PM Jason Xing [off-list ref] wrote:

quoted

On Wed, Oct 27, 2021 at 3:23 PM Jason Xing [off-list ref] wrote:

quoted

On Tue, Oct 26, 2021 at 9:19 PM [off-list ref] wrote:

quoted

From: Jason Xing <redacted>

Setting the @next of the last skb to NULL to prevent the panic in future
when someone does something to the last of the gro list but its @next is
invalid.

For example, without the fix (commit: ece23711dd95), a panic could happen
with the clsact loaded when skb is redirected and then validated in
validate_xmit_skb_list() which could access the error addr of the @next
of the last skb. Thus, "general protection fault" would appear after that.

Signed-off-by: Jason Xing <redacted>
---
 net/core/skbuff.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 2170bea..7b248f1 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c

@@ -4396,6 +4396,7 @@ int skb_gro_receive(struct sk_buff *p, struct sk_buff *skb)
                skb_shinfo(p)->frag_list = skb;
        else
                NAPI_GRO_CB(p)->last->next = skb;
+       skb->next = NULL;
        NAPI_GRO_CB(p)->last = skb;

Besides, I'm a little bit confused that this operation inserts the
newest skb into the tail of the flow, so the tail of flow is the
newest, head oldest. The patch (commit: 600adc18) introduces the flush
of the oldest when the flow is full to lower the latency, but actually
it fetches the tail of the flow. Do I get something wrong here? I feel

I have to update this part. The commit 600adc18 evicts and flushes the
oldest flow. But for the current kernel, when
"napi->gro_hash[hash].count >= MAX_GRO_SKBS" happens, the
gro_flush_oldest() flushes the oldest skb of one certain flow,
actually it is the newest skb because it is at the end of the list.

it seems the below is more matched with the gro_flush_oldest() instead
of the above code block:
https://elixir.bootlin.com/linux/v5.15-rc3/source/net/core/dev.c#L6118

I just submitted another patch to explain how it happens, please help
me review both patches.

Link: https://lore.kernel.org/lkml/20211027084944.4508-1-kerneljasonxing@gmail.com/ (local)

Thanks again,
Jason

quoted

quoted

it is really odd.

Thanks,
Jason

quoted

        __skb_header_release(skb);
        lp = p;
--
1.8.3.1

.