Re: [PATCH 07/16] ublk: add helper ublk_check_fetch_buf()

[Ming Lei <ming.lei@redhat.com>]

On Tue, Jul 08, 2025 at 08:31:05AM -0400, Caleb Sander Mateos wrote:
> On Wed, Jul 2, 2025 at 12:04 AM Ming Lei <ming.lei@redhat.com> wrote:
> >
> > Add helper ublk_check_fetch_buf() for checking buffer only.
> >
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> 
> The commit message seems a bit sparse. How about something like:
> Add a helper ublk_check_fetch_buf() to validate UBLK_IO_FETCH_REQ's
> addr. This doesn't require access to the ublk_io, so it can be done
> before taking the ublk_device mutex.

OK, looks much better.

> 
> > ---
> >  drivers/block/ublk_drv.c | 32 +++++++++++++++++++-------------
> >  1 file changed, 19 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> > index 6d3aa08eef22..7fd2fa493d6a 100644
> > --- a/drivers/block/ublk_drv.c
> > +++ b/drivers/block/ublk_drv.c
> > @@ -2146,6 +2146,22 @@ static int ublk_unregister_io_buf(struct io_uring_cmd *cmd,
> >         return io_buffer_unregister_bvec(cmd, index, issue_flags);
> >  }
> >
> > +static int ublk_check_fetch_buf(const struct ublk_queue *ubq, __u64 buf_addr)
> > +{
> > +       if (ublk_need_map_io(ubq)) {
> > +               /*
> > +                * FETCH_RQ has to provide IO buffer if NEED GET
> > +                * DATA is not enabled
> > +                */
> > +               if (!buf_addr && !ublk_need_get_data(ubq))
> > +                       return -EINVAL;
> > +       } else if (buf_addr) {
> > +               /* User copy requires addr to be unset */
> > +               return -EINVAL;
> > +       }
> > +       return 0;
> > +}
> > +
> >  static int ublk_fetch(struct io_uring_cmd *cmd, struct ublk_queue *ubq,
> >                       struct ublk_io *io, __u64 buf_addr)
> >  {
> > @@ -2172,19 +2188,6 @@ static int ublk_fetch(struct io_uring_cmd *cmd, struct ublk_queue *ubq,
> >
> >         WARN_ON_ONCE(io->flags & UBLK_IO_FLAG_OWNED_BY_SRV);
> >
> > -       if (ublk_need_map_io(ubq)) {
> > -               /*
> > -                * FETCH_RQ has to provide IO buffer if NEED GET
> > -                * DATA is not enabled
> > -                */
> > -               if (!buf_addr && !ublk_need_get_data(ubq))
> > -                       goto out;
> 
> Was it a bug that this didn't set ret = -EINVAL before jumping to out?
> Looks like ublk_fetch() would incorrectly skip initializing the
> ublk_io and return 0 in this case. So probably this needs a Fixes tag.
> Looks like the bug was introduced by the code movement in b69b8edfb27d
> ("ublk: properly serialize all FETCH_REQs").

Good catch!


Thanks, 
Ming