* [PATCH net-next v3 1/4] net: dm: check the boundary of skb drop reasons
2022-05-13 3:03 [PATCH net-next v3 0/4] net: skb: check the boundrary of skb drop reason menglong8.dong
@ 2022-05-13 3:03 ` menglong8.dong
2022-05-13 3:03 ` [PATCH net-next v3 2/4] net: skb: check the boundrary of drop reason in kfree_skb_reason() menglong8.dong
` (3 subsequent siblings)
4 siblings, 0 replies; 11+ messages in thread
From: menglong8.dong @ 2022-05-13 3:03 UTC (permalink / raw)
To: kuba
Cc: nhorman, davem, edumazet, pabeni, yoshfuji, dsahern, imagedong,
kafai, talalahmad, keescook, asml.silence, willemb,
vasily.averin, ilias.apalodimas, luiz.von.dentz, linux-kernel,
netdev, Jiang Biao, Hao Peng
From: Menglong Dong <imagedong@tencent.com>
The 'reason' will be set to 'SKB_DROP_REASON_NOT_SPECIFIED' if it not
small that SKB_DROP_REASON_MAX in net_dm_packet_trace_kfree_skb_hit(),
but it can't avoid it to be 0, which is invalid and can cause NULL
pointer in drop_reasons.
Therefore, reset it to SKB_DROP_REASON_NOT_SPECIFIED when 'reason <= 0'.
Reviewed-by: Jiang Biao <benbjiang@tencent.com>
Reviewed-by: Hao Peng <flyingpeng@tencent.com>
Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
net/core/drop_monitor.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c
index b89e3e95bffc..41cac0e4834e 100644
--- a/net/core/drop_monitor.c
+++ b/net/core/drop_monitor.c
@@ -517,7 +517,7 @@ static void net_dm_packet_trace_kfree_skb_hit(void *ignore,
if (!nskb)
return;
- if ((unsigned int)reason >= SKB_DROP_REASON_MAX)
+ if (unlikely(reason >= SKB_DROP_REASON_MAX || reason <= 0))
reason = SKB_DROP_REASON_NOT_SPECIFIED;
cb = NET_DM_SKB_CB(nskb);
cb->reason = reason;
--
2.36.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH net-next v3 2/4] net: skb: check the boundrary of drop reason in kfree_skb_reason()
2022-05-13 3:03 [PATCH net-next v3 0/4] net: skb: check the boundrary of skb drop reason menglong8.dong
2022-05-13 3:03 ` [PATCH net-next v3 1/4] net: dm: check the boundary of skb drop reasons menglong8.dong
@ 2022-05-13 3:03 ` menglong8.dong
2022-05-13 3:03 ` [PATCH net-next v3 3/4] net: skb: change the definition SKB_DR_SET() menglong8.dong
` (2 subsequent siblings)
4 siblings, 0 replies; 11+ messages in thread
From: menglong8.dong @ 2022-05-13 3:03 UTC (permalink / raw)
To: kuba
Cc: nhorman, davem, edumazet, pabeni, yoshfuji, dsahern, imagedong,
kafai, talalahmad, keescook, asml.silence, willemb,
vasily.averin, ilias.apalodimas, luiz.von.dentz, linux-kernel,
netdev, Jiang Biao, Hao Peng
From: Menglong Dong <imagedong@tencent.com>
Sometimes, we may forget to reset skb drop reason to NOT_SPECIFIED after
we make it the return value of the functions with return type of enum
skb_drop_reason, such as tcp_inbound_md5_hash. Therefore, its value can
be SKB_NOT_DROPPED_YET(0), which is invalid for kfree_skb_reason().
So we check the range of drop reason in kfree_skb_reason() with
DEBUG_NET_WARN_ON_ONCE().
Reviewed-by: Jiang Biao <benbjiang@tencent.com>
Reviewed-by: Hao Peng <flyingpeng@tencent.com>
Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
v3:
- don't reset the reason and print the debug warning only (Jakub Kicinski)
---
net/core/skbuff.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 15f7b6f99a8f..fab791b0c59e 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -771,6 +771,8 @@ void kfree_skb_reason(struct sk_buff *skb, enum skb_drop_reason reason)
if (!skb_unref(skb))
return;
+ DEBUG_NET_WARN_ON_ONCE(reason <= 0 || reason >= SKB_DROP_REASON_MAX);
+
trace_kfree_skb(skb, __builtin_return_address(0), reason);
__kfree_skb(skb);
}
--
2.36.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH net-next v3 3/4] net: skb: change the definition SKB_DR_SET()
2022-05-13 3:03 [PATCH net-next v3 0/4] net: skb: check the boundrary of skb drop reason menglong8.dong
2022-05-13 3:03 ` [PATCH net-next v3 1/4] net: dm: check the boundary of skb drop reasons menglong8.dong
2022-05-13 3:03 ` [PATCH net-next v3 2/4] net: skb: check the boundrary of drop reason in kfree_skb_reason() menglong8.dong
@ 2022-05-13 3:03 ` menglong8.dong
2022-05-13 3:03 ` [PATCH net-next v3 4/4] net: tcp: reset 'drop_reason' to NOT_SPCIFIED in tcp_v{4,6}_rcv() menglong8.dong
2022-05-16 10:00 ` [PATCH net-next v3 0/4] net: skb: check the boundrary of skb drop reason patchwork-bot+netdevbpf
4 siblings, 0 replies; 11+ messages in thread
From: menglong8.dong @ 2022-05-13 3:03 UTC (permalink / raw)
To: kuba
Cc: nhorman, davem, edumazet, pabeni, yoshfuji, dsahern, imagedong,
kafai, talalahmad, keescook, asml.silence, willemb,
vasily.averin, ilias.apalodimas, luiz.von.dentz, linux-kernel,
netdev, Jiang Biao, Hao Peng
From: Menglong Dong <imagedong@tencent.com>
The SKB_DR_OR() is used to set the drop reason to a value when it is
not set or specified yet. SKB_NOT_DROPPED_YET should also be considered
as not set.
Reviewed-by: Jiang Biao <benbjiang@tencent.com>
Reviewed-by: Hao Peng <flyingpeng@tencent.com>
Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
include/linux/skbuff.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index b91d225fdc13..4db3f4a33580 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -510,7 +510,8 @@ enum skb_drop_reason {
(name = SKB_DROP_REASON_##reason)
#define SKB_DR_OR(name, reason) \
do { \
- if (name == SKB_DROP_REASON_NOT_SPECIFIED) \
+ if (name == SKB_DROP_REASON_NOT_SPECIFIED || \
+ name == SKB_NOT_DROPPED_YET) \
SKB_DR_SET(name, reason); \
} while (0)
--
2.36.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH net-next v3 4/4] net: tcp: reset 'drop_reason' to NOT_SPCIFIED in tcp_v{4,6}_rcv()
2022-05-13 3:03 [PATCH net-next v3 0/4] net: skb: check the boundrary of skb drop reason menglong8.dong
` (2 preceding siblings ...)
2022-05-13 3:03 ` [PATCH net-next v3 3/4] net: skb: change the definition SKB_DR_SET() menglong8.dong
@ 2022-05-13 3:03 ` menglong8.dong
2022-05-19 15:48 ` Jakub Kicinski
2022-05-16 10:00 ` [PATCH net-next v3 0/4] net: skb: check the boundrary of skb drop reason patchwork-bot+netdevbpf
4 siblings, 1 reply; 11+ messages in thread
From: menglong8.dong @ 2022-05-13 3:03 UTC (permalink / raw)
To: kuba
Cc: nhorman, davem, edumazet, pabeni, yoshfuji, dsahern, imagedong,
kafai, talalahmad, keescook, asml.silence, willemb,
vasily.averin, ilias.apalodimas, luiz.von.dentz, linux-kernel,
netdev, Jiang Biao, Hao Peng
From: Menglong Dong <imagedong@tencent.com>
The 'drop_reason' that passed to kfree_skb_reason() in tcp_v4_rcv()
and tcp_v6_rcv() can be SKB_NOT_DROPPED_YET(0), as it is used as the
return value of tcp_inbound_md5_hash().
And it can panic the kernel with NULL pointer in
net_dm_packet_report_size() if the reason is 0, as drop_reasons[0]
is NULL.
Fixes: 1330b6ef3313 ("skb: make drop reason booleanable")
Reviewed-by: Jiang Biao <benbjiang@tencent.com>
Reviewed-by: Hao Peng <flyingpeng@tencent.com>
Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
v3:
- remove new lines between tags
v2:
- consider tcp_v6_rcv()
---
net/ipv4/tcp_ipv4.c | 1 +
net/ipv6/tcp_ipv6.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 918816ec5dd4..24eb42497a71 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -2101,6 +2101,7 @@ int tcp_v4_rcv(struct sk_buff *skb)
}
discard_it:
+ SKB_DR_OR(drop_reason, NOT_SPECIFIED);
/* Discard frame. */
kfree_skb_reason(skb, drop_reason);
return 0;
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 60bdec257ba7..636ed23d9af0 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1509,6 +1509,7 @@ int tcp_v6_do_rcv(struct sock *sk, struct sk_buff *skb)
discard:
if (opt_skb)
__kfree_skb(opt_skb);
+ SKB_DR_OR(reason, NOT_SPECIFIED);
kfree_skb_reason(skb, reason);
return 0;
csum_err:
--
2.36.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH net-next v3 4/4] net: tcp: reset 'drop_reason' to NOT_SPCIFIED in tcp_v{4,6}_rcv()
2022-05-13 3:03 ` [PATCH net-next v3 4/4] net: tcp: reset 'drop_reason' to NOT_SPCIFIED in tcp_v{4,6}_rcv() menglong8.dong
@ 2022-05-19 15:48 ` Jakub Kicinski
2022-05-20 1:46 ` Menglong Dong
0 siblings, 1 reply; 11+ messages in thread
From: Jakub Kicinski @ 2022-05-19 15:48 UTC (permalink / raw)
To: menglong8.dong
Cc: nhorman, davem, edumazet, pabeni, yoshfuji, dsahern, imagedong,
kafai, talalahmad, keescook, asml.silence, willemb,
vasily.averin, ilias.apalodimas, luiz.von.dentz, linux-kernel,
netdev, Jiang Biao, Hao Peng
On Fri, 13 May 2022 11:03:39 +0800 menglong8.dong@gmail.com wrote:
> From: Menglong Dong <imagedong@tencent.com>
>
> The 'drop_reason' that passed to kfree_skb_reason() in tcp_v4_rcv()
> and tcp_v6_rcv() can be SKB_NOT_DROPPED_YET(0), as it is used as the
> return value of tcp_inbound_md5_hash().
>
> And it can panic the kernel with NULL pointer in
> net_dm_packet_report_size() if the reason is 0, as drop_reasons[0]
> is NULL.
>
> Fixes: 1330b6ef3313 ("skb: make drop reason booleanable")
This patch is in net, should this fix have been targeting net / 5.18?
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next v3 4/4] net: tcp: reset 'drop_reason' to NOT_SPCIFIED in tcp_v{4,6}_rcv()
2022-05-19 15:48 ` Jakub Kicinski
@ 2022-05-20 1:46 ` Menglong Dong
2022-05-20 2:09 ` Jakub Kicinski
0 siblings, 1 reply; 11+ messages in thread
From: Menglong Dong @ 2022-05-20 1:46 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: LKML, netdev
On Thu, May 19, 2022 at 11:48 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri, 13 May 2022 11:03:39 +0800 menglong8.dong@gmail.com wrote:
> > From: Menglong Dong <imagedong@tencent.com>
> >
> > The 'drop_reason' that passed to kfree_skb_reason() in tcp_v4_rcv()
> > and tcp_v6_rcv() can be SKB_NOT_DROPPED_YET(0), as it is used as the
> > return value of tcp_inbound_md5_hash().
> >
> > And it can panic the kernel with NULL pointer in
> > net_dm_packet_report_size() if the reason is 0, as drop_reasons[0]
> > is NULL.
> >
> > Fixes: 1330b6ef3313 ("skb: make drop reason booleanable")
>
> This patch is in net, should this fix have been targeting net / 5.18?
Yeah, I think it should have. What do I need to do? CC someone?
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next v3 4/4] net: tcp: reset 'drop_reason' to NOT_SPCIFIED in tcp_v{4,6}_rcv()
2022-05-20 1:46 ` Menglong Dong
@ 2022-05-20 2:09 ` Jakub Kicinski
2022-05-20 2:18 ` Jakub Kicinski
0 siblings, 1 reply; 11+ messages in thread
From: Jakub Kicinski @ 2022-05-20 2:09 UTC (permalink / raw)
To: Menglong Dong; +Cc: LKML, netdev
On Fri, 20 May 2022 09:46:49 +0800 Menglong Dong wrote:
> > This patch is in net, should this fix have been targeting net / 5.18?
>
> Yeah, I think it should have. What do I need to do? CC someone?
Too late now, I was just double checking. It can make its way to the
current release via stable in a week or two.
BTW I'm about to send a fixup to patch 4, stay tuned.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next v3 4/4] net: tcp: reset 'drop_reason' to NOT_SPCIFIED in tcp_v{4,6}_rcv()
2022-05-20 2:09 ` Jakub Kicinski
@ 2022-05-20 2:18 ` Jakub Kicinski
2022-05-20 2:39 ` Menglong Dong
0 siblings, 1 reply; 11+ messages in thread
From: Jakub Kicinski @ 2022-05-20 2:18 UTC (permalink / raw)
To: Menglong Dong; +Cc: LKML, netdev
On Thu, 19 May 2022 19:09:15 -0700 Jakub Kicinski wrote:
> On Fri, 20 May 2022 09:46:49 +0800 Menglong Dong wrote:
> > > This patch is in net, should this fix have been targeting net / 5.18?
> >
> > Yeah, I think it should have. What do I need to do? CC someone?
>
> Too late now, I was just double checking. It can make its way to the
> current release via stable in a week or two.
Ah, FWIW my initial question was missing "-next" - I meant to say that
the patch is in net-next rather than net. I think you got what I meant..
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next v3 4/4] net: tcp: reset 'drop_reason' to NOT_SPCIFIED in tcp_v{4,6}_rcv()
2022-05-20 2:18 ` Jakub Kicinski
@ 2022-05-20 2:39 ` Menglong Dong
0 siblings, 0 replies; 11+ messages in thread
From: Menglong Dong @ 2022-05-20 2:39 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: LKML, netdev
On Fri, May 20, 2022 at 10:18 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 19 May 2022 19:09:15 -0700 Jakub Kicinski wrote:
> > On Fri, 20 May 2022 09:46:49 +0800 Menglong Dong wrote:
> > > > This patch is in net, should this fix have been targeting net / 5.18?
> > >
> > > Yeah, I think it should have. What do I need to do? CC someone?
> >
> > Too late now, I was just double checking. It can make its way to the
> > current release via stable in a week or two.
>
> Ah, FWIW my initial question was missing "-next" - I meant to say that
> the patch is in net-next rather than net. I think you got what I meant..
Yeah, I get what you mean now. Such bug-fix patches should target 'net'
rather than 'net-next'.
BTW, thanks for your fixup...I am still surprised at my mistake.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next v3 0/4] net: skb: check the boundrary of skb drop reason
2022-05-13 3:03 [PATCH net-next v3 0/4] net: skb: check the boundrary of skb drop reason menglong8.dong
` (3 preceding siblings ...)
2022-05-13 3:03 ` [PATCH net-next v3 4/4] net: tcp: reset 'drop_reason' to NOT_SPCIFIED in tcp_v{4,6}_rcv() menglong8.dong
@ 2022-05-16 10:00 ` patchwork-bot+netdevbpf
4 siblings, 0 replies; 11+ messages in thread
From: patchwork-bot+netdevbpf @ 2022-05-16 10:00 UTC (permalink / raw)
To: Menglong Dong
Cc: kuba, nhorman, davem, edumazet, pabeni, yoshfuji, dsahern,
imagedong, kafai, talalahmad, keescook, asml.silence, willemb,
vasily.averin, ilias.apalodimas, luiz.von.dentz, linux-kernel,
netdev
Hello:
This series was applied to netdev/net-next.git (master)
by David S. Miller <davem@davemloft.net>:
On Fri, 13 May 2022 11:03:35 +0800 you wrote:
> From: Menglong Dong <imagedong@tencent.com>
>
> In the commit 1330b6ef3313 ("skb: make drop reason booleanable"),
> SKB_NOT_DROPPED_YET is added to the enum skb_drop_reason, which makes
> the invalid drop reason SKB_NOT_DROPPED_YET can leak to the kfree_skb
> tracepoint. Once this happen (it happened, as 4th patch says), it can
> cause NULL pointer in drop monitor and result in kernel panic.
>
> [...]
Here is the summary with links:
- [net-next,v3,1/4] net: dm: check the boundary of skb drop reasons
https://git.kernel.org/netdev/net-next/c/a3af33abd921
- [net-next,v3,2/4] net: skb: check the boundrary of drop reason in kfree_skb_reason()
https://git.kernel.org/netdev/net-next/c/20bbcd0a94c6
- [net-next,v3,3/4] net: skb: change the definition SKB_DR_SET()
https://git.kernel.org/netdev/net-next/c/7ebd3f3ee51a
- [net-next,v3,4/4] net: tcp: reset 'drop_reason' to NOT_SPCIFIED in tcp_v{4,6}_rcv()
https://git.kernel.org/netdev/net-next/c/f8319dfd1b3b
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 11+ messages in thread