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@xxxxxxxxxx> wrote:
> >
> > Add helper ublk_check_fetch_buf() for checking buffer only.
> >
> > Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxx>
>
> 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
