net: remove the bogus overflow debug check in pskb_may_pull()

PATCH In review Cong Wang 7 participants 10 messages Jun 6, 2024 – Jun 14, 2024

Subsystems (4)

Networking Network drivers BPF BPF networking

lore

Share & tools

lore

Cong Wang

author

2024-06-06 22:15 UTC #0 lore raw

PATCH 1/1 net: remove the bogus overflow debug check in pskb_may_pull()

From: Cong Wang <[email protected]>

Commit 219eee9c0d16 ("net: skbuff: add overflow debug check to pull/push helpers") introduced an overflow debug check for pull/push helpers. For __skb_pull() this makes sense because its callers rarely check its return value. But for pskb_may_pull() it does not make sense, since its return value is properly taken care of. Remove the one in pskb_may_pull(), we can continue rely on its return value.

Fixes 219eee9c0d16 ("net: skbuff: add overflow debug check to pull/push helpers")

Reported-by [email protected]

Cc Florian Westphal <[email protected]>

Signed-off-by Cong Wang <[email protected]>

---
 include/linux/skbuff.h | 2 --
 1 file changed, 2 deletions(-)

include/linux/skbuff.h +0 -2

index 1c2902eaebd3..9fd49bab6595 100644
@@ -2735,8 +2735,6 @@ void *__pskb_pull_tail(struct sk_buff *skb, int delta);
 static inline enum skb_drop_reason
 pskb_may_pull_reason(struct sk_buff *skb, unsigned int len)
 {
-	DEBUG_NET_WARN_ON_ONCE(len > INT_MAX);
-
 	if (likely(len <= skb_headlen(skb)))
 		return SKB_NOT_DROPPED_YET;

Signature · 1 line

2.34.1

Florian Westphal

2024-06-06 23:27 UTC #1 lore raw

Quoted 8 lines · Cong Wang

From: Cong Wang <[email protected]>
Commit 219eee9c0d16 ("net: skbuff: add overflow debug check to pull/push helpers") introduced an overflow debug check for pull/push helpers. For __skb_pull() this makes sense because its callers rarely check its return value. But for pskb_may_pull() it does not make sense, since its return value is properly taken care of. Remove the one in pskb_may_pull(), we can continue rely on its return value.

See 025f8ad20f2e3264d11683aa9cbbf0083eefbdcd which would not exist without this check, I would not give up yet.

bpf_try_make_writable() could do an explicit check vs. skb->len.

If anyone needs it, splat is at
https://syzkaller.appspot.com/bug?extid=0c4150bff9fff3bf023c

Cong Wang

author

2024-06-07 16:14 UTC #2 lore raw

Quoted 10 lines · Florian Westphal

Quoted 8 lines · Cong Wang
From: Cong Wang <[email protected]>
Commit 219eee9c0d16 ("net: skbuff: add overflow debug check to pull/push helpers") introduced an overflow debug check for pull/push helpers. For __skb_pull() this makes sense because its callers rarely check its return value. But for pskb_may_pull() it does not make sense, since its return value is properly taken care of. Remove the one in pskb_may_pull(), we can continue rely on its return value.

Quoted 2 lines

See 025f8ad20f2e3264d11683aa9cbbf0083eefbdcd which would not exist without this check, I would not give up yet.

What's the point of that commit?

The "fix" (I doubt it fixes anything) you had is merely exiting a few lines earlier than pskb_may_pull():

30 if (!skb_inner_network_header_was_set(skb))
31 goto out;
32

 33         skb_reset_network_header(skb);
 34         mpls_hlen = skb_inner_network_header(skb) - skb_network_header(skb);
 35         if (unlikely(!mpls_hlen || mpls_hlen % MPLS_HLEN))

36 goto out;
37 if (unlikely(!pskb_may_pull(skb, mpls_hlen)))
38 goto out;

Before your "fix", we exit on line 37. After your "fix", we exit on line
30. I don't see any difference here, it returns -EINVAL anyway.

Quoted 2 lines

bpf_try_make_writable() could do an explicit check vs. skb->len.

But why? I don't see the point of its existence. pskb_may_pull() already checks it very well:

2741 if (unlikely(len > skb->len))
2742 return SKB_DROP_REASON_PKT_TOO_SMALL;

Thanks.

Kuniyuki Iwashima

2024-06-07 21:32 UTC #3 lore raw

From: Cong Wang <[email protected]>
Date: Fri, 7 Jun 2024 09:14:04 -0700

Quoted 11 lines · Florian Westphal

Quoted 10 lines
Quoted 8 lines · Cong Wang
From: Cong Wang <[email protected]>
Commit 219eee9c0d16 ("net: skbuff: add overflow debug check to pull/push helpers") introduced an overflow debug check for pull/push helpers. For __skb_pull() this makes sense because its callers rarely check its return value. But for pskb_may_pull() it does not make sense, since its return value is properly taken care of. Remove the one in pskb_may_pull(), we can continue rely on its return value.

Quoted 4 lines

Quoted 2 lines
See 025f8ad20f2e3264d11683aa9cbbf0083eefbdcd which would not exist without this check, I would not give up yet.
What's the point of that commit?

4b911a9690d7 would be better example. The warning actually found a bug in NSH GSO.

Here's splats triggered by syzkaller using NSH over various tunnels.
https://lore.kernel.org/netdev/[email protected]/

Eric Dumazet

2024-06-08 08:01 UTC #4 lore raw

Quoted 12 lines · Kuniyuki Iwashima

From: Cong Wang <[email protected]>
Date: Fri, 7 Jun 2024 09:14:04 -0700
Quoted 10 lines · Florian Westphal
Quoted 9 lines
Quoted 8 lines · Cong Wang
From: Cong Wang <[email protected]>
Commit 219eee9c0d16 ("net: skbuff: add overflow debug check to pull/push helpers") introduced an overflow debug check for pull/push helpers. For __skb_pull() this makes sense because its callers rarely check its return value. But for pskb_may_pull() it does not make sense, since its return value is properly taken care of. Remove the one in pskb_may_pull(), we can continue rely on its return value.

Quoted 8 lines

Quoted 3 lines
Quoted 2 lines
See 025f8ad20f2e3264d11683aa9cbbf0083eefbdcd which would not exist without this check, I would not give up yet.
What's the point of that commit?
4b911a9690d7 would be better example. The warning actually found a bug in NSH GSO.
Here's splats triggered by syzkaller using NSH over various tunnels.
https://lore.kernel.org/netdev/[email protected]/

Right. We discussed this before. I guess I forgot to send the fix.

Florian could you submit the suggestion I made before ?

net/core/filter.c +0 -0

index 358870408a51e61f3cbc552736806e4dfee1ec39..da7aae6fd8ba557c66699d1cfebd47f18f442aa2 100644
@@ -1662,6 +1662,11 @@ static DEFINE_PER_CPU(struct bpf_scratchpad, bpf_sp);

static inline int __bpf_try_make_writable(struct sk_buff *skb, unsigned int write_len)
{
+#if defined(CONFIG_DEBUG_NET)
+    /* Avoid a splat in pskb_may_pull_reason() */
+    if (write_len > INT_MAX)
+        return -EINVAL;
+#endif
     return skb_ensure_writable(skb, write_len);
}

Florian Westphal

2024-06-08 22:24 UTC #5 lore raw

Quoted 32 lines · Eric Dumazet

Quoted 12 lines · Kuniyuki Iwashima
From: Cong Wang <[email protected]>
Date: Fri, 7 Jun 2024 09:14:04 -0700
Quoted 10 lines · Florian Westphal
Quoted 9 lines
Quoted 8 lines · Cong Wang
From: Cong Wang <[email protected]>
Commit 219eee9c0d16 ("net: skbuff: add overflow debug check to pull/push helpers") introduced an overflow debug check for pull/push helpers. For __skb_pull() this makes sense because its callers rarely check its return value. But for pskb_may_pull() it does not make sense, since its return value is properly taken care of. Remove the one in pskb_may_pull(), we can continue rely on its return value.
Quoted 8 lines
Quoted 3 lines
Quoted 2 lines
See 025f8ad20f2e3264d11683aa9cbbf0083eefbdcd which would not exist without this check, I would not give up yet.
What's the point of that commit?
4b911a9690d7 would be better example. The warning actually found a bug in NSH GSO.
Here's splats triggered by syzkaller using NSH over various tunnels.
https://lore.kernel.org/netdev/[email protected]/
Right. We discussed this before. I guess I forgot to send the fix. Florian could you submit the suggestion I made before ?
net/core/filter.c +0 -0
index 358870408a51e61f3cbc552736806e4dfee1ec39..da7aae6fd8ba557c66699d1cfebd47f18f442aa2
100644

net/core/filter.c static DEFINE_PER_CPU(struct bpf_scratchpad, bpf_sp); · line 1671

Quoted 10 lines

net/core/filter.c +5 -0

@@ -1662,6 +1662,11 @@ static DEFINE_PER_CPU(struct bpf_scratchpad, bpf_sp);
  static inline int __bpf_try_make_writable(struct sk_buff *skb,
                        unsigned int write_len)
  {
+#if defined(CONFIG_DEBUG_NET)
+    /* Avoid a splat in pskb_may_pull_reason() */
+    if (write_len > INT_MAX)
+        return -EINVAL;
+#endif
      return skb_ensure_writable(skb, write_len);
  }

Makes sense, I'll probably not get to this before Friday though, so if anyone else wants to do this: go right ahead.

Jason Xing

2024-06-09 02:01 UTC #6 lore raw

In reply to Eric Dumazet

Hello Eric,

Quoted 36 lines · Eric Dumazet

Quoted 12 lines · Kuniyuki Iwashima
From: Cong Wang <[email protected]>
Date: Fri, 7 Jun 2024 09:14:04 -0700
Quoted 10 lines · Florian Westphal
Quoted 9 lines
Quoted 8 lines · Cong Wang
From: Cong Wang <[email protected]>
Commit 219eee9c0d16 ("net: skbuff: add overflow debug check to pull/push helpers") introduced an overflow debug check for pull/push helpers. For __skb_pull() this makes sense because its callers rarely check its return value. But for pskb_may_pull() it does not make sense, since its return value is properly taken care of. Remove the one in pskb_may_pull(), we can continue rely on its return value.
Quoted 8 lines
Quoted 3 lines
Quoted 2 lines
See 025f8ad20f2e3264d11683aa9cbbf0083eefbdcd which would not exist without this check, I would not give up yet.
What's the point of that commit?
4b911a9690d7 would be better example. The warning actually found a bug in NSH GSO.
Here's splats triggered by syzkaller using NSH over various tunnels.
https://lore.kernel.org/netdev/[email protected]/
Right. We discussed this before. I guess I forgot to send the fix.
Florian could you submit the suggestion I made before ?
net/core/filter.c +0 -0
index 358870408a51e61f3cbc552736806e4dfee1ec39..da7aae6fd8ba557c66699d1cfebd47f18f442aa2 100644

net/core/filter.c static DEFINE_PER_CPU(struct bpf_scratchpad, bpf_sp); · line 1665

Quoted 4 lines

net/core/filter.c +1 -0

@@ -1662,6 +1662,11 @@ static DEFINE_PER_CPU(struct bpf_scratchpad, bpf_sp);
  static inline int __bpf_try_make_writable(struct sk_buff *skb,
                        unsigned int write_len)
  {
+#if defined(CONFIG_DEBUG_NET)

I wonder why we want to avoid printing warning information especially when CONFIG_DEBUG_NET is on? Any reasons?

net/core/filter.c static DEFINE_PER_CPU(struct bpf_scratchpad, bpf_sp); · line 1667

Quoted 2 lines

net/core/filter.c +2 -0

+    /* Avoid a splat in pskb_may_pull_reason() */
+    if (write_len > INT_MAX)

I guess: if the purpose is to skip pskb_may_pull_reason and then return a proper value (-EINVAL) to its caller when the above statement is true, can we add the warning here on top of you patch:
DEBUG_NET_WARN_ON_ONCE(len > INT_MAX);
?

After this, we can print useful information and let the caller catch the -EINVAL return value.

Thanks,
Jason

Quoted 7 lines

net/core/filter.c +2 -0

+        return -EINVAL;
+#endif
      return skb_ensure_writable(skb, write_len);
  }

Florian Westphal

2024-06-14 10:41 UTC #7 lore raw

PATCH 1/1 bpf: avoid splat in pskb_pull_reason

In reply to Eric Dumazet

syzkaller builds (CONFIG_DEBUG_NET=y) frequently trigger a debug hint in pskb_may_pull.

We'd like to retain this debug check because it might hint at integer overflows and other issues (kernel code should pull headers, not huge value).

In bpf case, this splat isn't interesting at all: such (nonsensical) bpf programs are typically generated by a fuzzer anyway.

Do what Eric suggested and suppress such warning.

For CONFIG_DEBUG_NET=n we don't need the extra check because pskb_may_pull will do the right thing: return an error without the WARN() backtrace.

Reported-by [email protected]

Closes https://syzkaller.appspot.com/bug?extid=0c4150bff9fff3bf023c

Fixes 219eee9c0d16 ("net: skbuff: add overflow debug check to pull/push helpers")

Link https://lore.kernel.org/netdev/[email protected]/ open here

Signed-off-by Florian Westphal <[email protected]>

---
 net/core/filter.c | 5 +++++
 1 file changed, 5 insertions(+)

net/core/filter.c +5 -0

index 2510464692af..9933851c685e 100644
@@ -1665,6 +1665,11 @@ static DEFINE_PER_CPU(struct bpf_scratchpad, bpf_sp);
 static inline int __bpf_try_make_writable(struct sk_buff *skb,
 					  unsigned int write_len)
 {
+#ifdef CONFIG_DEBUG_NET
+	/* Avoid a splat in pskb_may_pull_reason() */
+	if (write_len > INT_MAX)
+		return -EINVAL;
+#endif
 	return skb_ensure_writable(skb, write_len);
 }

Signature · 1 line

2.44.2

Eric Dumazet

Reviewed-by on v1 1/1

2024-06-14 12:11 UTC #8 lore raw

Re: [PATCH bpf] bpf: avoid splat in pskb_pull_reason

Quoted 17 lines · Florian Westphal

syzkaller builds (CONFIG_DEBUG_NET=y) frequently trigger a debug hint in pskb_may_pull.
We'd like to retain this debug check because it might hint at integer overflows and other issues (kernel code should pull headers, not huge value).
In bpf case, this splat isn't interesting at all: such (nonsensical) bpf programs are typically generated by a fuzzer anyway.
Do what Eric suggested and suppress such warning.
For CONFIG_DEBUG_NET=n we don't need the extra check because pskb_may_pull will do the right thing: return an error without the WARN() backtrace.

Quoted 7 lines

Reported-by [email protected]
Closes https://syzkaller.appspot.com/bug?extid=0c4150bff9fff3bf023c
Fixes 219eee9c0d16 ("net: skbuff: add overflow debug check to pull/push helpers")
Link https://lore.kernel.org/netdev/[email protected]/ open here
Suggested-by Eric Dumazet <[email protected]>
Signed-off-by Florian Westphal <[email protected]>
---

Thanks Florian

Reviewed-by Eric Dumazet <[email protected]>

[email protected]

CI Applied

2024-06-14 15:30 UTC #9 lore raw

Re: [PATCH bpf] bpf: avoid splat in pskb_pull_reason

In reply to Florian Westphal

This patch was applied to bpf/bpf.git (master) by Daniel Borkmann <[email protected]>:

Hello:

This patch was applied to bpf/bpf.git (master)
by Daniel Borkmann <[email protected]>:

Quoted 8 lines · On Fri, 14 Jun 2024 12:17:33 +0200 you wrote:

syzkaller builds (CONFIG_DEBUG_NET=y) frequently trigger a debug hint in pskb_may_pull.
We'd like to retain this debug check because it might hint at integer overflows and other issues (kernel code should pull headers, not huge value).
[...]

Here is the summary with links:
- [bpf] bpf: avoid splat in pskb_pull_reason

    https://git.kernel.org/bpf/bpf/c/2bbe3e5a2f4e

You are awesome, thank you!

Signature · 2 lines

Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html