| Message ID | 20240606221531.255224-1-xiyou.wangcong@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: remove the bogus overflow debug check in pskb_may_pull() | expand |
Cong Wang <xiyou.wangcong@gmail.com> wrote: > From: Cong Wang <cong.wang@bytedance.com> > > 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
On Fri, Jun 07, 2024 at 01:27:47AM +0200, Florian Westphal wrote: > Cong Wang <xiyou.wangcong@gmail.com> wrote: > > From: Cong Wang <cong.wang@bytedance.com> > > > > 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.
From: Cong Wang <xiyou.wangcong@gmail.com> Date: Fri, 7 Jun 2024 09:14:04 -0700 > On Fri, Jun 07, 2024 at 01:27:47AM +0200, Florian Westphal wrote: > > Cong Wang <xiyou.wangcong@gmail.com> wrote: > > > From: Cong Wang <cong.wang@bytedance.com> > > > > > > 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/20240415222041.18537-2-kuniyu@amazon.com/
On 6/7/24 23:32, Kuniyuki Iwashima wrote: > From: Cong Wang <xiyou.wangcong@gmail.com> > Date: Fri, 7 Jun 2024 09:14:04 -0700 >> On Fri, Jun 07, 2024 at 01:27:47AM +0200, Florian Westphal wrote: >>> Cong Wang <xiyou.wangcong@gmail.com> wrote: >>>> From: Cong Wang <cong.wang@bytedance.com> >>>> >>>> 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/20240415222041.18537-2-kuniyu@amazon.com/ 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); }
Eric Dumazet <eric.dumazet@gmail.com> wrote: > On 6/7/24 23:32, Kuniyuki Iwashima wrote: > > From: Cong Wang <xiyou.wangcong@gmail.com> > > Date: Fri, 7 Jun 2024 09:14:04 -0700 > > > On Fri, Jun 07, 2024 at 01:27:47AM +0200, Florian Westphal wrote: > > > > Cong Wang <xiyou.wangcong@gmail.com> wrote: > > > > > From: Cong Wang <cong.wang@bytedance.com> > > > > > > > > > > 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/20240415222041.18537-2-kuniyu@amazon.com/ > > > 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.
Hello Eric, On Sat, Jun 8, 2024 at 4:01 PM Eric Dumazet <eric.dumazet@gmail.com> wrote: > > > On 6/7/24 23:32, Kuniyuki Iwashima wrote: > > From: Cong Wang <xiyou.wangcong@gmail.com> > > Date: Fri, 7 Jun 2024 09:14:04 -0700 > >> On Fri, Jun 07, 2024 at 01:27:47AM +0200, Florian Westphal wrote: > >>> Cong Wang <xiyou.wangcong@gmail.com> wrote: > >>>> From: Cong Wang <cong.wang@bytedance.com> > >>>> > >>>> 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/20240415222041.18537-2-kuniyu@amazon.com/ > > > 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); > } > > >
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;