Re: [PATCH v4 4/5] nvme: wire-up uring-cmd support for io-passthru on char-device.

[Jens Axboe <axboe@kernel.dk>]

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?

-- 
Jens Axboe