| Message ID | 20250121-uring-sockcmd-fix-v1-1-add742802a29@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | io_uring/uring_cmd: add missing READ_ONCE() on shared memory read | expand |
On Tue, 21 Jan 2025 17:09:59 +0100, Jann Horn wrote: > cmd->sqe seems to point to shared memory here; so values should only be > read from it with READ_ONCE(). To ensure that the compiler won't generate > code that assumes the value in memory will stay constant, add a > READ_ONCE(). > The callees io_uring_cmd_getsockopt() and io_uring_cmd_setsockopt() already > do this correctly. > > [...] Applied, thanks! [1/1] io_uring/uring_cmd: add missing READ_ONCE() on shared memory read commit: 0963dba3dc006b454c54fd019bbbdb931e7a7c70 Best regards,
On 1/22/25 12:38 PM, Jens Axboe wrote: > > On Tue, 21 Jan 2025 17:09:59 +0100, Jann Horn wrote: >> cmd->sqe seems to point to shared memory here; so values should only be >> read from it with READ_ONCE(). To ensure that the compiler won't generate >> code that assumes the value in memory will stay constant, add a >> READ_ONCE(). >> The callees io_uring_cmd_getsockopt() and io_uring_cmd_setsockopt() already >> do this correctly. >> >> [...] > > Applied, thanks! > > [1/1] io_uring/uring_cmd: add missing READ_ONCE() on shared memory read > commit: 0963dba3dc006b454c54fd019bbbdb931e7a7c70 I took a closer look and this isn't necessary. Either ->sqe is a full copy at this point. Should probably be renamed as such... If we want to make this clearer, then we should do: - switch (cmd->sqe->cmd_op) { + switch (cmd->cmd_op) { instead. I'll drop this one.
On Thu, Jan 23, 2025 at 1:18 AM Jens Axboe <axboe@kernel.dk> wrote: > On 1/22/25 12:38 PM, Jens Axboe wrote: > > > > On Tue, 21 Jan 2025 17:09:59 +0100, Jann Horn wrote: > >> cmd->sqe seems to point to shared memory here; so values should only be > >> read from it with READ_ONCE(). To ensure that the compiler won't generate > >> code that assumes the value in memory will stay constant, add a > >> READ_ONCE(). > >> The callees io_uring_cmd_getsockopt() and io_uring_cmd_setsockopt() already > >> do this correctly. > >> > >> [...] > > > > Applied, thanks! > > > > [1/1] io_uring/uring_cmd: add missing READ_ONCE() on shared memory read > > commit: 0963dba3dc006b454c54fd019bbbdb931e7a7c70 > > I took a closer look and this isn't necessary. Either ->sqe is a full > copy at this point. Should probably be renamed as such... If we want to > make this clearer, then we should do: Are you sure? On mainline (at commit 21266b8df522), I applied the attached diff that basically adds some printf debugging and adds this in io_uring_cmd_sock(): pr_warn("%s: [first read] cmd->sqe->cmd_op = 0x%x\n", __func__, READ_ONCE(cmd->sqe->cmd_op)); mdelay(2000); pr_warn("%s: [second read] cmd->sqe->cmd_op = 0x%x\n", __func__, READ_ONCE(cmd->sqe->cmd_op)); Then I ran the attached testcase, which submits a SQE and then modifies the ->cmd_op of the SQE while it is being submitted. Resulting dmesg output, showing that cmd->sqe->cmd_op changes when userspace modifies the SQE: [ 180.415944][ T1110] io_submit_sqes: SQE = ffff888010bcc000 [ 180.418731][ T1110] io_submit_sqe: SQE = ffff888010bcc000 [ 180.421191][ T1110] io_queue_sqe [ 180.422160][ T1110] io_issue_sqe [ 180.423101][ T1110] io_uring_cmd: SQE = ffff888010bcc000 [ 180.424570][ T1110] io_uring_cmd_sock: cmd->sqe = ffff888010bcc000 [ 180.426272][ T1110] io_uring_cmd_sock: [first read] cmd->sqe->cmd_op = 0x1234 [ 182.429036][ T1110] io_uring_cmd_sock: [second read] cmd->sqe->cmd_op = 0x5678
On 1/23/25 7:44 AM, Jann Horn wrote: > On Thu, Jan 23, 2025 at 1:18?AM Jens Axboe <axboe@kernel.dk> wrote: >> On 1/22/25 12:38 PM, Jens Axboe wrote: >>> >>> On Tue, 21 Jan 2025 17:09:59 +0100, Jann Horn wrote: >>>> cmd->sqe seems to point to shared memory here; so values should only be >>>> read from it with READ_ONCE(). To ensure that the compiler won't generate >>>> code that assumes the value in memory will stay constant, add a >>>> READ_ONCE(). >>>> The callees io_uring_cmd_getsockopt() and io_uring_cmd_setsockopt() already >>>> do this correctly. >>>> >>>> [...] >>> >>> Applied, thanks! >>> >>> [1/1] io_uring/uring_cmd: add missing READ_ONCE() on shared memory read >>> commit: 0963dba3dc006b454c54fd019bbbdb931e7a7c70 >> >> I took a closer look and this isn't necessary. Either ->sqe is a full >> copy at this point. Should probably be renamed as such... If we want to >> make this clearer, then we should do: > > Are you sure? On mainline (at commit 21266b8df522), I applied the > attached diff that basically adds some printf debugging and adds this > in io_uring_cmd_sock(): Yeah you are right, braino on my part. If we don't go async, it's not copied. The changed fix is still better, but I'll reword the commit message to be more accurate.
diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c index fc94c465a9850d4ed9df0cd26fcd6523657a2854..f4397bd66283d5939b60e7fa0a12bd7426322b9f 100644 --- a/io_uring/uring_cmd.c +++ b/io_uring/uring_cmd.c @@ -350,7 +350,7 @@ int io_uring_cmd_sock(struct io_uring_cmd *cmd, unsigned int issue_flags) if (!prot || !prot->ioctl) return -EOPNOTSUPP; - switch (cmd->sqe->cmd_op) { + switch (READ_ONCE(cmd->sqe->cmd_op)) { case SOCKET_URING_OP_SIOCINQ: ret = prot->ioctl(sk, SIOCINQ, &arg); if (ret)
cmd->sqe seems to point to shared memory here; so values should only be read from it with READ_ONCE(). To ensure that the compiler won't generate code that assumes the value in memory will stay constant, add a READ_ONCE(). The callees io_uring_cmd_getsockopt() and io_uring_cmd_setsockopt() already do this correctly. Signed-off-by: Jann Horn <jannh@google.com> --- io_uring/uring_cmd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- base-commit: 95ec54a420b8f445e04a7ca0ea8deb72c51fe1d3 change-id: 20250121-uring-sockcmd-fix-75b73e5b9750