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

Commit Message

Cong Wang June 6, 2024, 10:15 p.m. UTC

From: Cong Wang <[e-mail]>

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: [e-mail]
Cc: Florian Westphal <[e-mail]>
Signed-off-by: Cong Wang <[e-mail]>
---
 include/linux/skbuff.h | 2 --
 1 file changed, 2 deletions(-)

Comments

Florian Westphal June 6, 2024, 11:27 p.m. UTC | #1

Cong Wang <[e-mail]> wrote:
> From: Cong Wang <[e-mail]>
> 
> 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 June 7, 2024, 4:14 p.m. UTC | #2

On Fri, Jun 07, 2024 at 01:27:47AM +0200, Florian Westphal wrote:
> Cong Wang <[e-mail]> wrote:
> > From: Cong Wang <[e-mail]>
> > 
> > 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.

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.

> 
> 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 June 7, 2024, 9:32 p.m. UTC | #3

From: Cong Wang <[e-mail]>
Date: Fri, 7 Jun 2024 09:14:04 -0700
> On Fri, Jun 07, 2024 at 01:27:47AM +0200, Florian Westphal wrote:
> > Cong Wang <[e-mail]> wrote:
> > > From: Cong Wang <[e-mail]>
> > > 
> > > 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.
> 
> 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/[e-mail]/

Eric Dumazet June 8, 2024, 8:01 a.m. UTC | #4

On 6/7/24 23:32, Kuniyuki Iwashima wrote:
> From: Cong Wang <[e-mail]>
> Date: Fri, 7 Jun 2024 09:14:04 -0700
>> On Fri, Jun 07, 2024 at 01:27:47AM +0200, Florian Westphal wrote:
>>> Cong Wang <[e-mail]> wrote:
>>>> From: Cong Wang <[e-mail]>
>>>>
>>>> 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.
>> 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/[e-mail]/


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

Florian could you submit the suggestion I made before ?

diff --git a/net/core/filter.c b/net/core/filter.c
index 
358870408a51e61f3cbc552736806e4dfee1ec39..da7aae6fd8ba557c66699d1cfebd47f18f442aa2 
100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -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 June 8, 2024, 10:24 p.m. UTC | #5

Eric Dumazet <[e-mail]> wrote:
> On 6/7/24 23:32, Kuniyuki Iwashima wrote:
> > From: Cong Wang <[e-mail]>
> > Date: Fri, 7 Jun 2024 09:14:04 -0700
> > > On Fri, Jun 07, 2024 at 01:27:47AM +0200, Florian Westphal wrote:
> > > > Cong Wang <[e-mail]> wrote:
> > > > > From: Cong Wang <[e-mail]>
> > > > > 
> > > > > 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.
> > > 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/[e-mail]/
> 
> 
> Right. We discussed this before. I guess I forgot to send the fix.
> Florian could you submit the suggestion I made before ?
> 
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 358870408a51e61f3cbc552736806e4dfee1ec39..da7aae6fd8ba557c66699d1cfebd47f18f442aa2
> 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -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 June 9, 2024, 2:01 a.m. UTC | #6

Hello Eric,

On Sat, Jun 8, 2024 at 4:01 PM Eric Dumazet <[e-mail]> wrote:
>
>
> On 6/7/24 23:32, Kuniyuki Iwashima wrote:
> > From: Cong Wang <[e-mail]>
> > Date: Fri, 7 Jun 2024 09:14:04 -0700
> >> On Fri, Jun 07, 2024 at 01:27:47AM +0200, Florian Westphal wrote:
> >>> Cong Wang <[e-mail]> wrote:
> >>>> From: Cong Wang <[e-mail]>
> >>>>
> >>>> 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.
> >> 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/[e-mail]/
>
>
> Right. We discussed this before. I guess I forgot to send the fix.
>
> Florian could you submit the suggestion I made before ?
>
> diff --git a/net/core/filter.c b/net/core/filter.c
> index
> 358870408a51e61f3cbc552736806e4dfee1ec39..da7aae6fd8ba557c66699d1cfebd47f18f442aa2
> 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -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?

> +    /* 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

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

Patch

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 1c2902eaebd3..9fd49bab6595 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -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;