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

[Dylan Yudaken <dylany@fb.com>]

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"

>         }
>  
> -       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.