2022-07-15 04:41:22

by Jakub Kicinski

Subject: Re: [PATCH v3,bpf-next] bpf: Don't redirect packets with invalid pkt_len

On Fri, 15 Jul 2022 11:22:33 +0800 Zhengchao Shao wrote:
> +#ifdef CONFIG_DEBUG_NET
> + if (unlikely(!skb->len)) {
> + pr_err("%s\n", __func__);
> + skb_dump(KERN_ERR, skb, false);
> + WARN_ON_ONCE(1);
> + }

Is there a reason to open code WARN_ONCE() like that?

#ifdef CONFIG_DEBUG_NET
if (WARN_ONCE(!skb->len, "%s\n", __func__))
skb_dump(KERN_ERR, skb, false);

or

if (IS_ENABLED(CONFIG_DEBUG_NET) &&
WARN_ONCE(!skb->len, "%s\n", __func__))
skb_dump(KERN_ERR, skb, false);

2022-07-15 10:06:11

by Eric Dumazet

Subject: Re: [PATCH v3,bpf-next] bpf: Don't redirect packets with invalid pkt_len

On Fri, Jul 15, 2022 at 6:30 AM Jakub Kicinski <[email protected]> wrote:
>
> On Fri, 15 Jul 2022 11:22:33 +0800 Zhengchao Shao wrote:
> > +#ifdef CONFIG_DEBUG_NET
> > + if (unlikely(!skb->len)) {
> > + pr_err("%s\n", __func__);
> > + skb_dump(KERN_ERR, skb, false);
> > + WARN_ON_ONCE(1);
> > + }
>
> Is there a reason to open code WARN_ONCE() like that?
>
> #ifdef CONFIG_DEBUG_NET
> if (WARN_ONCE(!skb->len, "%s\n", __func__))
> skb_dump(KERN_ERR, skb, false);
>
> or
>
> if (IS_ENABLED(CONFIG_DEBUG_NET) &&
> WARN_ONCE(!skb->len, "%s\n", __func__))
> skb_dump(KERN_ERR, skb, false);


Also the skb_dump() needs to be done once.

DO_ONCE_LITE(skb_dump, KERN_ERR, skb, false);