On Mon, Jun 9, 2025 at 12:34 AM Ming Lei <ming.lei@xxxxxxxxxx> wrote:
>
> On Fri, Jun 06, 2025 at 03:40:09PM -0600, Caleb Sander Mateos wrote:
> > Currently, UBLK_IO_REGISTER_IO_BUF and UBLK_IO_UNREGISTER_IO_BUF are
> > only permitted on the ublk_io's daemon task. But this restriction is
> > unnecessary. ublk_register_io_buf() calls __ublk_check_and_get_req() to
> > look up the request from the tagset and atomically take a reference on
> > the request without accessing the ublk_io. ublk_unregister_io_buf()
> > doesn't use the q_id or tag at all.
> >
> > So allow these opcodes even on tasks other than io->task.
> >
> > Handle UBLK_IO_UNREGISTER_IO_BUF before obtaining the ubq and io since
> > the buffer index being unregistered is not necessarily related to the
> > specified q_id and tag.
> >
> > Add a feature flag UBLK_F_BUF_REG_OFF_DAEMON that userspace can use to
> > determine whether the kernel supports off-daemon buffer registration.
> >
> > Suggested-by: Ming Lei <ming.lei@xxxxxxxxxx>
> > Signed-off-by: Caleb Sander Mateos <csander@xxxxxxxxxxxxxxx>
> > ---
> > drivers/block/ublk_drv.c | 37 +++++++++++++++++++++--------------
> > include/uapi/linux/ublk_cmd.h | 8 ++++++++
> > 2 files changed, 30 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> > index a8030818f74a..2084bbdd2cbb 100644
> > --- a/drivers/block/ublk_drv.c
> > +++ b/drivers/block/ublk_drv.c
> > @@ -68,11 +68,12 @@
> > | UBLK_F_ZONED \
> > | UBLK_F_USER_RECOVERY_FAIL_IO \
> > | UBLK_F_UPDATE_SIZE \
> > | UBLK_F_AUTO_BUF_REG \
> > | UBLK_F_QUIESCE \
> > - | UBLK_F_PER_IO_DAEMON)
> > + | UBLK_F_PER_IO_DAEMON \
> > + | UBLK_F_BUF_REG_OFF_DAEMON)
> >
> > #define UBLK_F_ALL_RECOVERY_FLAGS (UBLK_F_USER_RECOVERY \
> > | UBLK_F_USER_RECOVERY_REISSUE \
> > | UBLK_F_USER_RECOVERY_FAIL_IO)
> >
> > @@ -2018,20 +2019,10 @@ static int ublk_register_io_buf(struct io_uring_cmd *cmd,
> > }
> >
> > return 0;
> > }
> >
> > -static int ublk_unregister_io_buf(struct io_uring_cmd *cmd,
> > - const struct ublk_queue *ubq,
> > - unsigned int index, unsigned int issue_flags)
> > -{
> > - if (!ublk_support_zero_copy(ubq))
> > - return -EINVAL;
> > -
> > - return io_buffer_unregister_bvec(cmd, index, issue_flags);
> > -}
> > -
> > static int ublk_fetch(struct io_uring_cmd *cmd, struct ublk_queue *ubq,
> > struct ublk_io *io, __u64 buf_addr)
> > {
> > struct ublk_device *ub = ubq->dev;
> > int ret = 0;
> > @@ -2184,10 +2175,18 @@ static int __ublk_ch_uring_cmd(struct io_uring_cmd *cmd,
> >
> > ret = ublk_check_cmd_op(cmd_op);
> > if (ret)
> > goto out;
> >
> > + /*
> > + * io_buffer_unregister_bvec() doesn't access the ubq or io,
> > + * so no need to validate the q_id, tag, or task
> > + */
> > + if (_IOC_NR(cmd_op) == UBLK_IO_UNREGISTER_IO_BUF)
> > + return io_buffer_unregister_bvec(cmd, ub_cmd->addr,
> > + issue_flags);
> > +
>
> Yeah, the behavior looks correct, but I'd suggest to validate the q_id
> too for making code more robust.
What value do you see in validating the q_id parameter? It's not used,
only the addr parameter is. I would rather document that the ublk
server doesn't need to provide q_id nor tag for a
UBLK_IO_UNREGISTER_IO_BUF command.
>
> Also you removed ublk_support_zero_copy() check for unregistering io buffer
> command, which isn't expected for this patch.
I can move that change into its own patch. I don't think the check
adds much value, though. There's nothing that requires the registered
buffer index passed in addr to belong to this q_id (or even this ublk
device). If you do want to provide a sanity check for the ublk server
that the ublk device supports zero-copy, I would rather just check the
flags on the ublk_device so the ublk server doesn't have to provide a
q_id.
>
> > ret = -EINVAL;
> > if (ub_cmd->q_id >= ub->dev_info.nr_hw_queues)
> > goto out;
> >
> > ubq = ublk_get_queue(ub, ub_cmd->q_id);
> > @@ -2204,12 +2203,21 @@ static int __ublk_ch_uring_cmd(struct io_uring_cmd *cmd,
> >
> > ublk_prep_cancel(cmd, issue_flags, ubq, tag);
> > return -EIOCBQUEUED;
> > }
> >
> > - if (READ_ONCE(io->task) != current)
> > + if (READ_ONCE(io->task) != current) {
> > + /*
> > + * ublk_register_io_buf() accesses only the request, not io,
> > + * so can be handled on any task
> > + */
> > + if (_IOC_NR(cmd_op) == UBLK_IO_REGISTER_IO_BUF)
> > + return ublk_register_io_buf(cmd, ubq, tag, ub_cmd->addr,
> > + issue_flags);
>
> Maybe you can move UBLK_IO_UNREGISTER_IO_BUF handling here, which seems
> more readable.
I would rather allow the ublk server to omit the q_id and tag
parameters for UBLK_IO_UNREGISTER_IO_BUF since they aren't really
used, and there's no enforcement that they correspond to the
registered buffer index (addr). In which case, there isn't any
io->task to check here. It would also be nice to skip the overhead of
looking up the ubq and io for UBLK_IO_UNREGISTER_IO_BUF since they
aren't used.
In the following patch I add an optimized case for
UBLK_IO_REGISTER_IO_BUF when called on the daemon task, which is why I
duplicate the UBLK_IO_REGISTER_IO_BUF handling here in this patch. But
UBLK_IO_UNREGISTER_IO_BUF doesn't need this special case, so I don't
see the need to handle UBLK_IO_UNREGISTER_IO_BUF in 2 different
places.
Best,
Caleb
