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

Subsystems: networking [general], the rest

STALE1777d

9 messages, 3 authors, 2021-10-27 · open the first message on its own page

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

From: <hidden>
Date: 2021-10-26 13:19:47

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;
 	__skb_header_release(skb);
 	lp = p;

-- 
1.8.3.1

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

From: Jason Xing <hidden>
Date: 2021-10-27 07:24:10

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

quoted hunk

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
it is really odd.

Thanks,
Jason

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

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

From: Jason Xing <hidden>
Date: 2021-10-27 08:07:51

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

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 is really odd.

Thanks,
Jason

quoted

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

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

From: Jason Xing <hidden>
Date: 2021-10-27 08:57:08

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

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.

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/

Thanks again,
Jason

quoted

it is really odd.

Thanks,
Jason

quoted

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

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

From: Yunsheng Lin <hidden>
Date: 2021-10-27 12:40:46

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/

Thanks again,
Jason

quoted

it is really odd.

Thanks,
Jason

quoted

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

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

From: Jason Xing <hidden>
Date: 2021-10-27 12:54:50

On Wed, Oct 27, 2021 at 8:40 PM Yunsheng Lin [off-list ref] wrote:

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

quoted

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

What you said is the @skb->list but not the list between skbs which is
connected by skb->next when the new incoming skb needs to get merged.
The @skb->list->next/prev is not the same as @skb->next.

quoted

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/

Thanks again,
Jason

quoted

it is really odd.

Thanks,
Jason

quoted

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

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

From: Jason Xing <hidden>
Date: 2021-10-27 13:58:19

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

On Wed, Oct 27, 2021 at 8:40 PM Yunsheng Lin [off-list ref] wrote:

quoted

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

quoted

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

What you said is the @skb->list but not the list between skbs which is
connected by skb->next when the new incoming skb needs to get merged.
The @skb->list->next/prev is not the same as @skb->next.

quoted

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/

Emm, I think you're right, Yunsheng. The gro_flush_oldest() fetches
the list of @skb->list.
Do you think the tail of skb's next pointer should be set to NULL?

Thanks,
Jason

quoted

Thanks again,
Jason

quoted

it is really odd.

Thanks,
Jason

quoted

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

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

From: Eric Dumazet <hidden>
Date: 2021-10-27 19:20:09


On 10/26/21 6:18 AM, kerneljasonxing@gmail.com wrote:

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.

If this a bug, please provide a Fixes: tag

quoted hunk

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;

Really at this point skb->next must be already NULL.

Please provide a stack trace so that we fix the caller instead
of adding more code in GRO layer.

 	NAPI_GRO_CB(p)->last = skb;
 	__skb_header_release(skb);
 	lp = p;

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

From: Eric Dumazet <hidden>
Date: 2021-10-27 19:25:41


On 10/27/21 1:07 AM, Jason Xing wrote:

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.

GRO only keeps one skb per flow in the main hash/lru.

I think you are not understanding GRO correctly.

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help