Re: [PATCH 03/16] io_uring: make io_buffer_select() return the user address directly

[Jens Axboe <axboe@kernel.dk>]

On 5/9/22 6:43 AM, Dylan Yudaken wrote:
> On Mon, 2022-05-09 at 06:28 -0600, Jens Axboe wrote:
>> On 5/9/22 6:12 AM, Dylan Yudaken wrote:
>>> On Mon, 2022-05-09 at 12:06 +0000, Dylan Yudaken wrote:
>>>> On Sun, 2022-05-01 at 14:56 -0600, Jens Axboe wrote:
>>>>> There's no point in having callers provide a kbuf, we're just
>>>>> returning
>>>>> the address anyway.
>>>>>
>>>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>>>> ---
>>>>>  fs/io_uring.c | 42 ++++++++++++++++++------------------------
>>>>>  1 file changed, 18 insertions(+), 24 deletions(-)
>>>>>
>>>>
>>>> ...
>>>>
>>>>> @@ -6013,10 +6006,11 @@ static int io_recv(struct io_kiocb
>>>>> *req,
>>>>> unsigned int issue_flags)
>>>>>                 return -ENOTSOCK;
>>>>>  
>>>>>         if (req->flags & REQ_F_BUFFER_SELECT) {
>>>>> -               kbuf = io_buffer_select(req, &sr->len, sr-
>>>>>> bgid,
>>>>> issue_flags);
>>>>> -               if (IS_ERR(kbuf))
>>>>> -                       return PTR_ERR(kbuf);
>>>>> -               buf = u64_to_user_ptr(kbuf->addr);
>>>>> +               void __user *buf;
>>>>
>>>> this now shadows the outer buf, and so does not work at all as
>>>> the buf
>>>> value is lost.
>>>> A bit surprised this did not show up in any tests.
>>>>
>>>>> +
>>>>> +               buf = io_buffer_select(req, &sr->len, sr->bgid,
>>>>> issue_flags);
>>>>> +               if (IS_ERR(buf))
>>>>> +                       return PTR_ERR(buf);
>>>>>         }
>>>>>  
>>>>>         ret = import_single_range(READ, buf, sr->len, &iov,
>>>>> &msg.msg_iter);
>>>>
>>>
>>> The following seems to fix it for me. I can submit it separately if
>>> you
>>> like.
>>
>> I think you want something like this:
>>
>>
>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>> index 19dd3ba92486..2b87c89d2375 100644
>> --- a/fs/io_uring.c
>> +++ b/fs/io_uring.c
>> @@ -5599,7 +5599,6 @@ static int io_recv(struct io_kiocb *req,
>> unsigned int issue_flags)
>>  {
>>         struct io_sr_msg *sr = &req->sr_msg;
>>         struct msghdr msg;
>> -       void __user *buf = sr->buf;
>>         struct socket *sock;
>>         struct iovec iov;
>>         unsigned flags;
>> @@ -5620,9 +5619,10 @@ static int io_recv(struct io_kiocb *req,
>> unsigned int issue_flags)
>>                 buf = io_buffer_select(req, &sr->len, sr->bgid,
>> issue_flags);
>>                 if (IS_ERR(buf))
>>                         return PTR_ERR(buf);
>> +               sr->buf = buf;
> 
> this line I think was added later on anyway in "io_uring: never call
> io_buffer_select() for a buffer re-select"

OK good that makes sense for why the end result was ok, but it should be
added here to avoid breakage in the middle.

>> -       ret = import_single_range(READ, buf, sr->len, &iov,
>> &msg.msg_iter);
>> +       ret = import_single_range(READ, sr->buf, sr->len, &iov,
>> &msg.msg_iter);
>>         if (unlikely(ret))
>>                 goto out_free;
>>  
>>
> 
> I'll send a patch now.

I decided to just fold in the patch to avoid having a broken point in
the middle.

-- 
Jens Axboe