netdev - Re: [PATCH] net: don't check skb

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220616020453.GA39@DESKTOP-8REGVGF.localdomain>
Date:   Thu, 16 Jun 2022 10:04:53 +0800
From:   Sieng Piaw Liew <liew.s.piaw@...il.com>
To:     Alexander Lobakin <alexandr.lobakin@...el.com>
Cc:     davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
        netdev@...r.kernel.org
Subject: Re: [PATCH] net: don't check skb_count twice

On Wed, Jun 15, 2022 at 05:35:25PM +0200, Alexander Lobakin wrote:
> From: Sieng Piaw Liew <liew.s.piaw@...il.com>
> Date: Wed, 15 Jun 2022 11:24:26 +0800
> 
> > NAPI cache skb_count is being checked twice without condition. Change to
> > checking the second time only if the first check is run.
> > 
> > Signed-off-by: Sieng Piaw Liew <liew.s.piaw@...il.com>
> > ---
> >  net/core/skbuff.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > index 5b3559cb1d82..c426adff6d96 100644
> > --- a/net/core/skbuff.c
> > +++ b/net/core/skbuff.c
> > @@ -172,13 +172,14 @@ static struct sk_buff *napi_skb_cache_get(void)
> >  	struct napi_alloc_cache *nc = this_cpu_ptr(&napi_alloc_cache);
> >  	struct sk_buff *skb;
> >  
> > -	if (unlikely(!nc->skb_count))
> > +	if (unlikely(!nc->skb_count)) {
> >  		nc->skb_count = kmem_cache_alloc_bulk(skbuff_head_cache,
> >  						      GFP_ATOMIC,
> >  						      NAPI_SKB_CACHE_BULK,
> >  						      nc->skb_cache);
> > -	if (unlikely(!nc->skb_count))
> > -		return NULL;
> > +		if (unlikely(!nc->skb_count))
> > +			return NULL;
> > +	}
> 
> I was sure the compilers are able to see that if the condition is
> false first time, it will be the second as well. Just curious, have
> you consulted objdump/objdiff to look whether anything changed?

I'm a total noob at this. Thanks for the guidance.
Here is the diff I just generated:

< before patch
> after patch

619,620c619,620
<   14: 24630000        addiu   v1,v1,0
<   18: 00021080        sll     v0,v0,0x2
---
>   14: 00021080        sll     v0,v0,0x2
>   18: 24630000        addiu   v1,v1,0
626,635c626,635
<   30: 8e030010        lw      v1,16(s0)
<   34: 1060000b        beqz    v1,64 <napi_skb_cache_get+0x64>
<   38: 3c020000        lui     v0,0x0
<   3c: 24620003        addiu   v0,v1,3
<   40: 2463ffff        addiu   v1,v1,-1
<   44: ae030010        sw      v1,16(s0)
<   48: 8fbf0014        lw      ra,20(sp)
<   4c: 00021080        sll     v0,v0,0x2
<   50: 02028021        addu    s0,s0,v0
<   54: 8e020004        lw      v0,4(s0)
---
>   30: 8e020010        lw      v0,16(s0)
>   34: 1040000b        beqz    v0,64 <napi_skb_cache_get+0x64>
>   38: 26070014        addiu   a3,s0,20
>   3c: 24430003        addiu   v1,v0,3
>   40: 00031880        sll     v1,v1,0x2
>   44: 2442ffff        addiu   v0,v0,-1
>   48: ae020010        sw      v0,16(s0)
>   4c: 02038021        addu    s0,s0,v1
>   50: 8e020004        lw      v0,4(s0)
>   54: 8fbf0014        lw      ra,20(sp)
639,640c639,640
<   64: 8c440000        lw      a0,0(v0)
<   68: 26070014        addiu   a3,s0,20
---
>   64: 3c020000        lui     v0,0x0
>   68: 8c440000        lw      a0,0(v0)
644c644
<   78: 00401825        move    v1,v0
---
>   78: 1440fff0        bnez    v0,3c <napi_skb_cache_get+0x3c>
646c646
<   80: 1460ffee        bnez    v1,3c <napi_skb_cache_get+0x3c>
---
>   80: 1000fff4        b       54 <napi_skb_cache_get+0x54>
648,651d647
<   88: 8fbf0014        lw      ra,20(sp)
<   8c: 8fb00010        lw      s0,16(sp)
<   90: 03e00008        jr      ra
<   94: 27bd0018        addiu   sp,sp,24
1736c1732
<  244: 24050ae8        li      a1,2792
---
>  244: 24050ae9        li      a1,2793

...(More similar li instruction diffs)
I think there are slightly more instructions before patch.

> 
> Also, please use scripts/get_maintainers.pl or at least git blame
> and add the original authors to Ccs next time, so that they won't
> miss your changes and will be able to review them in time. E.g. I
> noticed this patch only when it did hit the net-next tree already,
> as I don't monitor LKML 24/7 (but I do that with my mailbox).
> 

Thanks for the tip.

> >  
> >  	skb = nc->skb_cache[--nc->skb_count];
> >  	kasan_unpoison_object_data(skbuff_head_cache, skb);
> > -- 
> > 2.17.1
> 
> Thanks,
> Olek