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

Date:   Thu, 16 Jun 2022 15:38:23 +0200
From:   Alexander Lobakin <alexandr.lobakin@...el.com>
To:     Sieng Piaw Liew <liew.s.piaw@...il.com>
Cc:     Alexander Lobakin <alexandr.lobakin@...el.com>,
        davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
        netdev@...r.kernel.org
Subject: Re: [PATCH] net: don't check skb_count twice

From: Sieng Piaw Liew <liew.s.piaw@...il.com>
Date: Thu, 16 Jun 2022 10:04:53 +0800

> 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.

Ok, thank you! Then it makes sense. I'll recheck my recent code
whether I did it that way again somewhere :D

> 
> > 
> > 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

Thanks,
Olek