* [PATCH net-next v2 1/4] net: dm: check the boundary of skb drop reasons
2022-05-12 12:33 [PATCH net-next v2 0/4] net: skb: check the boundrary of skb drop reason menglong8.dong
@ 2022-05-12 12:33 ` menglong8.dong
2022-05-12 12:33 ` [PATCH net-next v2 2/4] net: skb: check the boundrary of drop reason in kfree_skb_reason() menglong8.dong
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: menglong8.dong @ 2022-05-12 12:33 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
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'.
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] 9+ messages in thread
* [PATCH net-next v2 2/4] net: skb: check the boundrary of drop reason in kfree_skb_reason()
2022-05-12 12:33 [PATCH net-next v2 0/4] net: skb: check the boundrary of skb drop reason menglong8.dong
2022-05-12 12:33 ` [PATCH net-next v2 1/4] net: dm: check the boundary of skb drop reasons menglong8.dong
@ 2022-05-12 12:33 ` menglong8.dong
2022-05-12 16:15 ` Jakub Kicinski
2022-05-12 12:33 ` [PATCH net-next v2 3/4] net: skb: change the definition SKB_DR_SET() menglong8.dong
2022-05-12 12:33 ` [PATCH net-next v2 4/4] net: tcp: reset 'drop_reason' to NOT_SPCIFIED in tcp_v{4,6}_rcv() menglong8.dong
3 siblings, 1 reply; 9+ messages in thread
From: menglong8.dong @ 2022-05-12 12:33 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
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() and reset it
to NOT_SPECIFIED and print a warning with DEBUG_NET_WARN_ON_ONCE() if it
is invalid.
Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
net/core/skbuff.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 15f7b6f99a8f..e49e43d4c34d 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -771,6 +771,11 @@ void kfree_skb_reason(struct sk_buff *skb, enum skb_drop_reason reason)
if (!skb_unref(skb))
return;
+ if (unlikely(reason <= 0 || reason >= SKB_DROP_REASON_MAX)) {
+ DEBUG_NET_WARN_ON_ONCE(1);
+ reason = SKB_DROP_REASON_NOT_SPECIFIED;
+ }
+
trace_kfree_skb(skb, __builtin_return_address(0), reason);
__kfree_skb(skb);
}
--
2.36.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH net-next v2 2/4] net: skb: check the boundrary of drop reason in kfree_skb_reason()
2022-05-12 12:33 ` [PATCH net-next v2 2/4] net: skb: check the boundrary of drop reason in kfree_skb_reason() menglong8.dong
@ 2022-05-12 16:15 ` Jakub Kicinski
2022-05-12 16:16 ` Eric Dumazet
2022-05-13 2:41 ` Menglong Dong
0 siblings, 2 replies; 9+ messages in thread
From: Jakub Kicinski @ 2022-05-12 16:15 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
On Thu, 12 May 2022 20:33:11 +0800 menglong8.dong@gmail.com wrote:
> + if (unlikely(reason <= 0 || reason >= SKB_DROP_REASON_MAX)) {
> + DEBUG_NET_WARN_ON_ONCE(1);
> + reason = SKB_DROP_REASON_NOT_SPECIFIED;
> + }
With drop_monitor fixes sending an invalid reason to the tracepoint
should be a minor bug, right?
Can we just have a:
DEBUG_NET_WARN_ON_ONCE(reason <= 0 || reason >= SKB_DROP_REASON_MAX);
and avoid having this branch on non-debug builds?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next v2 2/4] net: skb: check the boundrary of drop reason in kfree_skb_reason()
2022-05-12 16:15 ` Jakub Kicinski
@ 2022-05-12 16:16 ` Eric Dumazet
2022-05-13 2:41 ` Menglong Dong
1 sibling, 0 replies; 9+ messages in thread
From: Eric Dumazet @ 2022-05-12 16:16 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Menglong Dong, Neil Horman, David Miller, Paolo Abeni,
Hideaki YOSHIFUJI, David Ahern, Menglong Dong, Martin KaFai Lau,
Talal Ahmad, Kees Cook, Pavel Begunkov, Willem de Bruijn,
Vasily Averin, Ilias Apalodimas, Luiz Augusto von Dentz, LKML,
netdev
On Thu, May 12, 2022 at 9:16 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 12 May 2022 20:33:11 +0800 menglong8.dong@gmail.com wrote:
> > + if (unlikely(reason <= 0 || reason >= SKB_DROP_REASON_MAX)) {
> > + DEBUG_NET_WARN_ON_ONCE(1);
> > + reason = SKB_DROP_REASON_NOT_SPECIFIED;
> > + }
>
> With drop_monitor fixes sending an invalid reason to the tracepoint
> should be a minor bug, right?
>
> Can we just have a:
>
> DEBUG_NET_WARN_ON_ONCE(reason <= 0 || reason >= SKB_DROP_REASON_MAX);
>
> and avoid having this branch on non-debug builds?
Exactly what I was going to say.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next v2 2/4] net: skb: check the boundrary of drop reason in kfree_skb_reason()
2022-05-12 16:15 ` Jakub Kicinski
2022-05-12 16:16 ` Eric Dumazet
@ 2022-05-13 2:41 ` Menglong Dong
1 sibling, 0 replies; 9+ messages in thread
From: Menglong Dong @ 2022-05-13 2:41 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Neil Horman, David Miller, Eric Dumazet, Paolo Abeni,
Hideaki YOSHIFUJI, David Ahern, Menglong Dong, Martin Lau,
Talal Ahmad, Kees Cook, asml.silence, Willem de Bruijn,
vasily.averin, Ilias Apalodimas, Luiz Augusto von Dentz, LKML,
netdev
On Fri, May 13, 2022 at 12:16 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 12 May 2022 20:33:11 +0800 menglong8.dong@gmail.com wrote:
> > + if (unlikely(reason <= 0 || reason >= SKB_DROP_REASON_MAX)) {
> > + DEBUG_NET_WARN_ON_ONCE(1);
> > + reason = SKB_DROP_REASON_NOT_SPECIFIED;
> > + }
>
> With drop_monitor fixes sending an invalid reason to the tracepoint
> should be a minor bug, right?
>
> Can we just have a:
>
> DEBUG_NET_WARN_ON_ONCE(reason <= 0 || reason >= SKB_DROP_REASON_MAX);
>
> and avoid having this branch on non-debug builds?
Yeah, it seems this way is more logical. I'll change it in the V3.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH net-next v2 3/4] net: skb: change the definition SKB_DR_SET()
2022-05-12 12:33 [PATCH net-next v2 0/4] net: skb: check the boundrary of skb drop reason menglong8.dong
2022-05-12 12:33 ` [PATCH net-next v2 1/4] net: dm: check the boundary of skb drop reasons menglong8.dong
2022-05-12 12:33 ` [PATCH net-next v2 2/4] net: skb: check the boundrary of drop reason in kfree_skb_reason() menglong8.dong
@ 2022-05-12 12:33 ` menglong8.dong
2022-05-12 12:33 ` [PATCH net-next v2 4/4] net: tcp: reset 'drop_reason' to NOT_SPCIFIED in tcp_v{4,6}_rcv() menglong8.dong
3 siblings, 0 replies; 9+ messages in thread
From: menglong8.dong @ 2022-05-12 12:33 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
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.
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] 9+ messages in thread
* [PATCH net-next v2 4/4] net: tcp: reset 'drop_reason' to NOT_SPCIFIED in tcp_v{4,6}_rcv()
2022-05-12 12:33 [PATCH net-next v2 0/4] net: skb: check the boundrary of skb drop reason menglong8.dong
` (2 preceding siblings ...)
2022-05-12 12:33 ` [PATCH net-next v2 3/4] net: skb: change the definition SKB_DR_SET() menglong8.dong
@ 2022-05-12 12:33 ` menglong8.dong
2022-05-12 16:16 ` Jakub Kicinski
3 siblings, 1 reply; 9+ messages in thread
From: menglong8.dong @ 2022-05-12 12:33 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
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")
Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
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] 9+ messages in thread
* Re: [PATCH net-next v2 4/4] net: tcp: reset 'drop_reason' to NOT_SPCIFIED in tcp_v{4,6}_rcv()
2022-05-12 12:33 ` [PATCH net-next v2 4/4] net: tcp: reset 'drop_reason' to NOT_SPCIFIED in tcp_v{4,6}_rcv() menglong8.dong
@ 2022-05-12 16:16 ` Jakub Kicinski
0 siblings, 0 replies; 9+ messages in thread
From: Jakub Kicinski @ 2022-05-12 16:16 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
On Thu, 12 May 2022 20:33:13 +0800 menglong8.dong@gmail.com wrote:
> Fixes: 1330b6ef3313 ("skb: make drop reason booleanable")
>
> Signed-off-by: Menglong Dong <imagedong@tencent.com>
No new lines between tags
^ permalink raw reply [flat|nested] 9+ messages in thread