On 5/5/22 7:50 AM, Jens Axboe wrote: > On 5/5/22 7:42 AM, Christoph Hellwig wrote: >> On Thu, May 05, 2022 at 07:38:31AM -0600, Jens Axboe wrote: >>>> + req = nvme_alloc_user_request(q, &c, nvme_to_user_ptr(cmd->addr), >>>> + cmd->data_len, nvme_to_user_ptr(cmd->metadata), >>>> + cmd->metadata_len, 0, cmd->timeout_ms ? >>>> + msecs_to_jiffies(cmd->timeout_ms) : 0, 0, rq_flags, >>>> + blk_flags); >>> >>> You need to be careful with reading/re-reading the shared memory. For >>> example, you do: >> >> Uh, yes. With ioucmd->cmd pointing to the user space mapped SQ >> we need to be very careful here. To the point where I'd almost prfer >> to memcpy it out first, altough there might be performance implications. > > Most of it is just copied individually to the on-stack command, so that > part is fine just with READ_ONCE(). Things like timeout don't matter too > much I think, but addr/metadata/metadata_len definitely should be > ensured stable and read/verified just once. > > IOW, I don't think we need the full on-stack copy, as it's just a few > items that are currently an issue. But let's see what the new iteration > looks like of that patch. Maybe we can just shove nvme_uring_cmd > metadata_len and data_len at the end of that struct and make it > identical to nvme_command_command and just make that the memcpy, then > use the copy going forward? The top three patches here have a proposed solution for the 3 issues that I highlighted: https://git.kernel.dk/cgit/linux-block/log/?h=for-5.19/io_uring-passthrough Totally untested... Kanchan, can you take a look and see what you think? They all need folding obviously, I just tried to do them separately. Should also get tested :-) -- Jens Axboe
