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
