netdev - Re: [RFC net-next v2 1/3] net: skb: plumb napi state thru skb freeing paths

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: <20230407071402.09fa792f@kernel.org>
Date:   Fri, 7 Apr 2023 07:14:02 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Yunsheng Lin <linyunsheng@...wei.com>
Cc:     <davem@...emloft.net>, <netdev@...r.kernel.org>,
        <edumazet@...gle.com>, <pabeni@...hat.com>, <hawk@...nel.org>,
        <ilias.apalodimas@...aro.org>
Subject: Re: [RFC net-next v2 1/3] net: skb: plumb napi state thru skb
 freeing paths

On Fri, 7 Apr 2023 17:15:15 +0800 Yunsheng Lin wrote:
> On 2023/4/6 7:20, Jakub Kicinski wrote:
> > We maintain a NAPI-local cache of skbs which is fed by napi_consume_skb().
> > Going forward we will also try to cache head and data pages.
> > Plumb the "are we in a normal NAPI context" information thru
> > deeper into the freeing path, up to skb_release_data() and
> > skb_free_head()/skb_pp_recycle().
> > 
> > Use "bool in_normal_napi" rather than bare "int budget",
> > the further we get from NAPI the more confusing the budget
> > argument may seem (particularly whether 0 or MAX is the
> > correct value to pass in when not in NAPI).

> > @@ -839,7 +839,7 @@ static void skb_clone_fraglist(struct sk_buff *skb)
> >  		skb_get(list);
> >  }
> >  
> > -static bool skb_pp_recycle(struct sk_buff *skb, void *data)
> > +static bool skb_pp_recycle(struct sk_buff *skb, void *data, bool in_normal_napi)  
> 
> What does *normal* means in 'in_normal_napi'?
> can we just use in_napi?

Technically netpoll also calls NAPI, that's why I threw in the
"normal". If folks prefer in_napi or some other name I'm more 
than happy to change. Naming is hard.

> > @@ -1226,7 +1228,7 @@ static void napi_skb_cache_put(struct sk_buff *skb)
> >  
> >  void __kfree_skb_defer(struct sk_buff *skb)
> >  {
> > -	skb_release_all(skb, SKB_DROP_REASON_NOT_SPECIFIED);
> > +	skb_release_all(skb, SKB_DROP_REASON_NOT_SPECIFIED, false);
> >  	napi_skb_cache_put(skb);  
> 
> Is there any reson not to call skb_release_all() with in_normal_napi
> being true while napi_skb_cache_put() is called here?

True, __kfree_skb_defer() needs more work also. I'll set in to true 
in the PATCH posting and clean up the function in a follow up.