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(-)

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)

---
base-commit: 95ec54a420b8f445e04a7ca0ea8deb72c51fe1d3
change-id: 20250121-uring-sockcmd-fix-75b73e5b9750

-- 
Jann Horn <jannh@google.com>

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,
-- 
Jens Axboe

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.

-- 
Jens Axboe

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.

-- 
Jens Axboe