linux-kernel - Re: [PATCH 3/3] io_uring/uring_cmd: import fixed buffer before going async

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: <68e33a48-8a77-4c49-824f-902187a4aacc@gmail.com>
Date: Sat, 22 Mar 2025 12:18:39 +0000
From: Pavel Begunkov <asml.silence@...il.com>
To: Caleb Sander Mateos <csander@...estorage.com>
Cc: Jens Axboe <axboe@...nel.dk>, Ming Lei <ming.lei@...hat.com>,
 Keith Busch <kbusch@...nel.org>, Christoph Hellwig <hch@....de>,
 Sagi Grimberg <sagi@...mberg.me>, Xinyu Zhang <xizhang@...estorage.com>,
 io-uring@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-nvme@...ts.infradead.org
Subject: Re: [PATCH 3/3] io_uring/uring_cmd: import fixed buffer before going
 async

On 3/21/25 21:38, Caleb Sander Mateos wrote:
> On Fri, Mar 21, 2025 at 1:34 PM Pavel Begunkov <asml.silence@...il.com> wrote:
>>
>> On 3/21/25 18:48, Caleb Sander Mateos wrote:
>>> For uring_cmd operations with fixed buffers, the fixed buffer lookup
>>> happens in io_uring_cmd_import_fixed(), called from the ->uring_cmd()
>>> implementation. A ->uring_cmd() implementation could return -EAGAIN on
>>> the initial issue for any reason before io_uring_cmd_import_fixed().
>>> For example, nvme_uring_cmd_io() calls nvme_alloc_user_request() first,
>>> which can return -EAGAIN if all tags in the tag set are in use.
>>
>> That's up to command when it resolves the buffer, you can just
>> move the call to io_import_reg_buf() earlier in nvme cmd code
>> and not working it around at the io_uring side.
>>
>> In general, it's a step back, it just got cleaned up from the
>> mess where node resolution and buffer imports were separate
>> steps and duplicate by every single request type that used it.
> 
> Yes, I considered just reordering the steps in nvme_uring_cmd_io().
> But it seems easy for a future change to accidentally introduce
> another point where the issue can go async before it has looked up the
> fixed buffer. And I am imagining there will be more uring_cmd fixed
> buffer users added (e.g. btrfs). This seems like a generic problem
> rather than something specific to NVMe passthru.

That's working around the api for ordering requests, that's the
reason you have an ordering problem.

> My other feeling is that the fixed buffer lookup is an io_uring-layer
> detail, whereas the use of the buffer is more a concern of the
> ->uring_cmd() implementation. If only the opcodes were consistent
> about how a fixed buffer is requested, we could do the lookup in the
> generic io_uring code like fixed files already do.

That's one of things I'd hope was done better, and not only
specifically for registered buffers, but it's late for that.

> But I'm open to implementing a different fix here if Jens would prefer.

It's not a fix, the behaviour is well within the constrained
of io_uring.

-- 
Pavel Begunkov