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
| ||
|
Message-ID: <9ecc38df-a185-fc1f-e94e-cf0c1fef865f@redhat.com> Date: Wed, 12 Apr 2023 11:29:08 +0200 From: Jesper Dangaard Brouer <jbrouer@...hat.com> To: Jakub Kicinski <kuba@...nel.org>, davem@...emloft.net Cc: brouer@...hat.com, netdev@...r.kernel.org, edumazet@...gle.com, pabeni@...hat.com, hawk@...nel.org, ilias.apalodimas@...aro.org, linyunsheng@...wei.com, Alexander Duyck <alexander.duyck@...il.com> Subject: Re: [PATCH net-next 1/3] net: skb: plumb napi state thru skb freeing paths On 11/04/2023 22.17, 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 code was changed to "napi_safe", the desc should reflect this ;-) > 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). I do like the code cleanup. It is worth explaining/mentioning that where budget==0 comes from? (Cc. Alex, please correct me.) My understanding is that this is caused by netconsole/netpoll (see net/core/netpoll.c func poll_one_napi()), which is a kernel (net)console debugging facility sending UDP packets via using only TX side of napi_poll. Thus, we are really trying to protect against doing these recycle tricks for when netpoll/netconcole is running (which I guess makes sense as we are likely printing/sending an OOPS msg over UDP). > > Signed-off-by: Jakub Kicinski <kuba@...nel.org> > --- > v1: > - feed the cache in __kfree_skb_defer(), it's in NAPI > - s/in_normal_napi/napi_safe/ > --- > net/core/skbuff.c | 38 ++++++++++++++++++++------------------ > 1 file changed, 20 insertions(+), 18 deletions(-) > (no more comments below, but didn't cut code for Alex to see) > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > index 050a875d09c5..b2092166f7e2 100644 > --- a/net/core/skbuff.c > +++ b/net/core/skbuff.c > @@ -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 napi_safe) > { > if (!IS_ENABLED(CONFIG_PAGE_POOL) || !skb->pp_recycle) > return false; > @@ -856,12 +856,12 @@ static void skb_kfree_head(void *head, unsigned int end_offset) > kfree(head); > } > > -static void skb_free_head(struct sk_buff *skb) > +static void skb_free_head(struct sk_buff *skb, bool napi_safe) > { > unsigned char *head = skb->head; > > if (skb->head_frag) { > - if (skb_pp_recycle(skb, head)) > + if (skb_pp_recycle(skb, head, napi_safe)) > return; > skb_free_frag(head); > } else { > @@ -869,7 +869,8 @@ static void skb_free_head(struct sk_buff *skb) > } > } > > -static void skb_release_data(struct sk_buff *skb, enum skb_drop_reason reason) > +static void skb_release_data(struct sk_buff *skb, enum skb_drop_reason reason, > + bool napi_safe) > { > struct skb_shared_info *shinfo = skb_shinfo(skb); > int i; > @@ -894,7 +895,7 @@ static void skb_release_data(struct sk_buff *skb, enum skb_drop_reason reason) > if (shinfo->frag_list) > kfree_skb_list_reason(shinfo->frag_list, reason); > > - skb_free_head(skb); > + skb_free_head(skb, napi_safe); > exit: > /* When we clone an SKB we copy the reycling bit. The pp_recycle > * bit is only set on the head though, so in order to avoid races > @@ -955,11 +956,12 @@ void skb_release_head_state(struct sk_buff *skb) > } > > /* Free everything but the sk_buff shell. */ > -static void skb_release_all(struct sk_buff *skb, enum skb_drop_reason reason) > +static void skb_release_all(struct sk_buff *skb, enum skb_drop_reason reason, > + bool napi_safe) > { > skb_release_head_state(skb); > if (likely(skb->head)) > - skb_release_data(skb, reason); > + skb_release_data(skb, reason, napi_safe); > } > > /** > @@ -973,7 +975,7 @@ static void skb_release_all(struct sk_buff *skb, enum skb_drop_reason reason) > > void __kfree_skb(struct sk_buff *skb) > { > - skb_release_all(skb, SKB_DROP_REASON_NOT_SPECIFIED); > + skb_release_all(skb, SKB_DROP_REASON_NOT_SPECIFIED, false); > kfree_skbmem(skb); > } > EXPORT_SYMBOL(__kfree_skb); > @@ -1027,7 +1029,7 @@ static void kfree_skb_add_bulk(struct sk_buff *skb, > return; > } > > - skb_release_all(skb, reason); > + skb_release_all(skb, reason, false); > sa->skb_array[sa->skb_count++] = skb; > > if (unlikely(sa->skb_count == KFREE_SKB_BULK_SIZE)) { > @@ -1201,7 +1203,7 @@ EXPORT_SYMBOL(consume_skb); > void __consume_stateless_skb(struct sk_buff *skb) > { > trace_consume_skb(skb, __builtin_return_address(0)); > - skb_release_data(skb, SKB_CONSUMED); > + skb_release_data(skb, SKB_CONSUMED, false); > kfree_skbmem(skb); > } > > @@ -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, true); > napi_skb_cache_put(skb); > } > > @@ -1264,7 +1266,7 @@ void napi_consume_skb(struct sk_buff *skb, int budget) > return; > } > > - skb_release_all(skb, SKB_CONSUMED); > + skb_release_all(skb, SKB_CONSUMED, !!budget); > napi_skb_cache_put(skb); > } > EXPORT_SYMBOL(napi_consume_skb); > @@ -1395,7 +1397,7 @@ EXPORT_SYMBOL_GPL(alloc_skb_for_msg); > */ > struct sk_buff *skb_morph(struct sk_buff *dst, struct sk_buff *src) > { > - skb_release_all(dst, SKB_CONSUMED); > + skb_release_all(dst, SKB_CONSUMED, false); > return __skb_clone(dst, src); > } > EXPORT_SYMBOL_GPL(skb_morph); > @@ -2018,9 +2020,9 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail, > if (skb_has_frag_list(skb)) > skb_clone_fraglist(skb); > > - skb_release_data(skb, SKB_CONSUMED); > + skb_release_data(skb, SKB_CONSUMED, false); > } else { > - skb_free_head(skb); > + skb_free_head(skb, false); > } > off = (data + nhead) - skb->head; > > @@ -6389,12 +6391,12 @@ static int pskb_carve_inside_header(struct sk_buff *skb, const u32 off, > skb_frag_ref(skb, i); > if (skb_has_frag_list(skb)) > skb_clone_fraglist(skb); > - skb_release_data(skb, SKB_CONSUMED); > + skb_release_data(skb, SKB_CONSUMED, false); > } else { > /* we can reuse existing recount- all we did was > * relocate values > */ > - skb_free_head(skb); > + skb_free_head(skb, false); > } > > skb->head = data; > @@ -6529,7 +6531,7 @@ static int pskb_carve_inside_nonlinear(struct sk_buff *skb, const u32 off, > skb_kfree_head(data, size); > return -ENOMEM; > } > - skb_release_data(skb, SKB_CONSUMED); > + skb_release_data(skb, SKB_CONSUMED, false); > > skb->head = data; > skb->head_frag = 0;
Powered by blists - more mailing lists