Re: [BUG] Possible unsafe page_pool usage in octeontx2

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

On 2023-08-28 13:07:12 [+0200], Alexander Lobakin wrote:
> > Looking again at the driver otx2_txrx.c NAPI code path also calls PP
> > directly in otx2_napi_handler() will call refill_pool_ptrs() ->
> > otx2_refill_pool_ptrs() -> otx2_alloc_buffer() -> __otx2_alloc_rbuf() ->
> > if (pool->page_pool) otx2_alloc_pool_buf() -> page_pool_alloc_frag().
> > 
> > The function otx2_alloc_buffer() can also choose to start a WQ, that
> > also called PP alloc API this time via otx2_alloc_rbuf() that have
> > BH-disable.  Like Sebastian, I don't think this is safe!
> 
> Disabling BH doesn't look correct to me, but I don't see issues in
> having consumer and producer working on different cores, as long as they
> use ptr_ring with locks.

After learning what p.napi is about, may I point out that there are also
users that don't check that and use page_pool_put_full_page() or later?
While I can't comment on the bpf/XDP users, browsing otx2: there is
this:
| otx2_stop()
| -> otx2_free_hw_resources()
|   -> otx2_cleanup_rx_cqes
|      -> otx2_free_bufs
|        -> page_pool_put_full_page(, true)
| -> cancel_delayed_work_sync()

otx2 is "safe" here due to the in_softirq() check in
__page_pool_put_page(). But: the worker could run and fill the lock less
buffer while otx2_free_bufs() is doing clean up/ removing pages and
filling this buffer on another CPU.
The worker is synchronised after the free. The lack of BH-disable in
otx2_stop()'s path safes the day here.
(It looks somehow suspicious that there is first "free mem" followed by
"sync refill worker" and not the other way around)

Sebastian