netdev - Re: [PATCH net-next] net: save some cycles when doing skb_attempt_defer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAL+tcoAf1FyqEsM-u-shGaE2FUQO_di6e63md_DYmFLWxXe3ew@mail.gmail.com>
Date: Thu, 11 Apr 2024 18:00:09 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Alexander Lobakin <aleksander.lobakin@...el.com>
Cc: Eric Dumazet <edumazet@...gle.com>, pablo@...filter.org, kuba@...nel.org, 
	pabeni@...hat.com, davem@...emloft.net, horms@...nel.org, 
	netdev@...r.kernel.org, Jason Xing <kernelxing@...cent.com>
Subject: Re: [PATCH net-next] net: save some cycles when doing skb_attempt_defer_free()

On Thu, Apr 11, 2024 at 5:13 PM Alexander Lobakin
<aleksander.lobakin@...el.com> wrote:
>
> From: Jason Xing <kerneljasonxing@...il.com>
> Date: Thu, 11 Apr 2024 15:31:23 +0800
>
> > On Thu, Apr 11, 2024 at 3:12 PM Eric Dumazet <edumazet@...gle.com> wrote:
> >>
> >> On Thu, Apr 11, 2024 at 8:33 AM Jason Xing <kerneljasonxing@...il.com> wrote:
> >>>
> >>> On Thu, Apr 11, 2024 at 1:27 PM Eric Dumazet <edumazet@...gle.com> wrote:
> >>>>
> >>>> On Thu, Apr 11, 2024 at 5:25 AM Jason Xing <kerneljasonxing@...il.com> wrote:
> >>>>>
> >>>>> From: Jason Xing <kernelxing@...cent.com>
> >>>>>
> >>>>> Normally, we don't face these two exceptions very often meanwhile
> >>>>> we have some chance to meet the condition where the current cpu id
> >>>>> is the same as skb->alloc_cpu.
> >>>>>
> >>>>> One simple test that can help us see the frequency of this statement
> >>>>> 'cpu == raw_smp_processor_id()':
> >>>>> 1. running iperf -s and iperf -c [ip] -P [MAX CPU]
> >>>>> 2. using BPF to capture skb_attempt_defer_free()
> >>>>>
> >>>>> I can see around 4% chance that happens to satisfy the statement.
> >>>>> So moving this statement at the beginning can save some cycles in
> >>>>> most cases.
> >>>>>
> >>>>> Signed-off-by: Jason Xing <kernelxing@...cent.com>
> >>>>> ---
> >>>>>  net/core/skbuff.c | 4 ++--
> >>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> >>>>> index ab970ded8a7b..b4f252dc91fb 100644
> >>>>> --- a/net/core/skbuff.c
> >>>>> +++ b/net/core/skbuff.c
> >>>>> @@ -7002,9 +7002,9 @@ void skb_attempt_defer_free(struct sk_buff *skb)
> >>>>>         unsigned int defer_max;
> >>>>>         bool kick;
> >>>>>
> >>>>> -       if (WARN_ON_ONCE(cpu >= nr_cpu_ids) ||
> >>>>> +       if (cpu == raw_smp_processor_id() ||
> >>>>>             !cpu_online(cpu) ||
> >>>>> -           cpu == raw_smp_processor_id()) {
> >>>>> +           WARN_ON_ONCE(cpu >= nr_cpu_ids)) {
> >>>>>  nodefer:       kfree_skb_napi_cache(skb);
> >>>>>                 return;
> >>>>>         }
> >>>>
> >>>> Wrong patch.
> >>>>
> >>>> cpu_online(X) is undefined and might crash if X is out of bounds on CONFIG_SMP=y
> >>>
> >>> Even if skb->alloc_cpu is larger than nr_cpu_ids, I don't know why the
> >>> integer test statement could cause crashing the kernel. It's just a
> >>> simple comparison. And if the statement is true,
> >>> raw_smp_processor_id() can guarantee the validation, right?
> >>
> >> Please read again the code you wrote, or run it with skb->alloc_cpu
> >> being set to 45000 on a full DEBUG kernel.
> >>
> >> You are focusing on skb->alloc_cpu == raw_smp_processor_id(), I am
> >> focusing on what happens
> >> when this condition is not true.
> >
> > Sorry. My bad. I put the wrong order of '!cpu_online(cpu)' and 'cpu >=
> > nr_cpu_ids'. I didn't consider the out-of-bound issue. I should have
> > done more checks :(
> >
> > The correct patch should be:
> > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > index ab970ded8a7b..6dc577a3ea6a 100644
> > --- a/net/core/skbuff.c
> > +++ b/net/core/skbuff.c
> > @@ -7002,9 +7002,9 @@ void skb_attempt_defer_free(struct sk_buff *skb)
> >         unsigned int defer_max;
> >         bool kick;
> >
> > -       if (WARN_ON_ONCE(cpu >= nr_cpu_ids) ||
> > -           !cpu_online(cpu) ||
> > -           cpu == raw_smp_processor_id()) {
> > +       if (cpu == raw_smp_processor_id() ||
> > +           WARN_ON_ONCE(cpu >= nr_cpu_ids) ||
> > +           !cpu_online(cpu)) {
>
> This one looks good to me.
> Feel free to add
>
> Reviewed-by: Alexander Lobakin <aleksander.lobakin@...el.com>
>
> To your v2 before sending.

Thanks! I will:)