netdev - Re: [PATCH net] docs: net: clarify the NAPI rules around XDP Tx

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]

Date: Tue, 25 Jul 2023 11:55:28 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Alexander H Duyck <alexander.duyck@...il.com>
Cc: davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com,
 pabeni@...hat.com, corbet@....net, linux-doc@...r.kernel.org
Subject: Re: [PATCH net] docs: net: clarify the NAPI rules around XDP Tx

On Tue, 25 Jul 2023 10:30:24 -0700 Alexander H Duyck wrote:
> > -In other words, it is recommended to ignore the budget argument when
> > -performing TX buffer reclamation to ensure that the reclamation is not
> > -arbitrarily bounded; however, it is required to honor the budget argument
> > -for RX processing.
> > +In other words for Rx processing the ``budget`` argument limits how many
> > +packets driver can process in a single poll. Rx specific APIs like page
> > +pool or XDP cannot be used at all when ``budget`` is 0.
> > +skb Tx processing should happen regardless of the ``budget``, but if
> > +the argument is 0 driver cannot call any XDP (or page pool) APIs.
> 
> This isn't accurate, and I would say it is somewhat dangerous advice.
> The Tx still needs to be processed regardless of if it is processing
> page_pool pages or XDP pages. I agree the Rx should not be processed,
> but the Tx must be processed using mechanisms that do NOT make use of
> NAPI optimizations when budget is 0.
> 
> So specifically, xdp_return_frame is safe in non-NAPI Tx cleanup. The
> xdp_return_frame_rx_napi is not.
> 
> Likewise there is napi_consume_skb which will use either a NAPI or non-
> NAPI version of things depending on if budget is 0 or not.
> 
> For the page_pool calls there is the "allow_direct" argument that is
> meant to decide between recycling in directly into the page_pool cache
> or not. It should only be used in the Rx handler itself when budget is
> non-zero.
> 
> I realise this was written up in response to a patch on the Mellanox
> driver. Based on the patch in question it looks like they were calling
> page_pool_recycle_direct outside of NAPI context. There is an explicit
> warning above that function about NOT calling it outside of NAPI
> context.

Unless I'm missing something budget=0 can be called from hard IRQ
context. And page pool takes _bh() locks. So unless we "teach it"
not to recycle _anything_ in hard IRQ context, it is not safe to call.

> >  .. warning::
> >  
> > -   The ``budget`` argument may be 0 if core tries to only process Tx completions
> > -   and no Rx packets.
> > +   The ``budget`` argument may be 0 if core tries to only process
> > +   skb Tx completions and no Rx or XDP packets.
> >  
> >  The poll method returns the amount of work done. If the driver still
> >  has outstanding work to do (e.g. ``budget`` was exhausted)  
> 
> We cannot make this distinction if both XDP and skb are processed in
> the same Tx queue. Otherwise you will cause the Tx to stall and break
> netpoll. If the ring is XDP only then yes, it can be skipped like what
> they did in the Mellanox driver, but if it is mixed then the XDP side
> of things needs to use the "safe" versions of the calls.

IDK, a rare delay in sending of a netpoll message is not a major
concern.