netdev - Re: [PATCH v14 07/11] io_uring/zcrx: set pp memory provider for an rx queue

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]

Message-ID: <78dba0c0-5cc6-4634-b657-f5575fd37770@gmail.com>
Date: Wed, 19 Feb 2025 09:54:50 +0000
From: Pavel Begunkov <asml.silence@...il.com>
To: David Wei <dw@...idwei.uk>, Kees Bakker <kees@...erbout.nl>,
 io-uring@...r.kernel.org, netdev@...r.kernel.org
Cc: Jens Axboe <axboe@...nel.dk>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Jesper Dangaard Brouer
 <hawk@...nel.org>, David Ahern <dsahern@...nel.org>,
 Mina Almasry <almasrymina@...gle.com>,
 Stanislav Fomichev <stfomichev@...il.com>, Joe Damato <jdamato@...tly.com>,
 Pedro Tammela <pctammela@...atatu.com>, lizetao <lizetao1@...wei.com>
Subject: Re: [PATCH v14 07/11] io_uring/zcrx: set pp memory provider for an rx
 queue

On 2/18/25 22:06, David Wei wrote:
> On 2025-02-18 11:40, Kees Bakker wrote:
>> Op 15-02-2025 om 01:09 schreef David Wei:
>>> Set the page pool memory provider for the rx queue configured for zero
>>> copy to io_uring. Then the rx queue is reset using
>>> netdev_rx_queue_restart() and netdev core + page pool will take care of
>>> filling the rx queue from the io_uring zero copy memory provider.
>>>
>>> For now, there is only one ifq so its destruction happens implicitly
>>> during io_uring cleanup.
>>>
>>> Reviewed-by: Jens Axboe <axboe@...nel.dk>
>>> Signed-off-by: David Wei <dw@...idwei.uk>
>>> ---
>>>    io_uring/zcrx.c | 49 +++++++++++++++++++++++++++++++++++++++++--------
>>>    1 file changed, 41 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/io_uring/zcrx.c b/io_uring/zcrx.c
>>> index 8833879d94ba..7d24fc98b306 100644
>>> --- a/io_uring/zcrx.c
>>> +++ b/io_uring/zcrx.c
>>> [...]
>>> @@ -444,6 +475,8 @@ void io_shutdown_zcrx_ifqs(struct io_ring_ctx *ctx)
>>>          if (ctx->ifq)
>>>            io_zcrx_scrub(ctx->ifq);
>>> +
>>> +    io_close_queue(ctx->ifq);
>> If ctx->ifq is NULL (which seems to be not unlikely given the if statement above)
>> then you'll get a NULL pointer dereference in io_close_queue().
> 
> The only caller of io_shutdown_zcrx_ifqs() is io_ring_exit_work() which
> checks for ctx->ifq first. That does mean the ctx->ifq check is
> redundant in this function though.

And despite the check being outside of the lock, it's even well
synchronised, but in any case we should just make it saner. Thanks
for letting know

-- 
Pavel Begunkov