[PATCH 0/2] ublk: two fixes on UBLK_F_AUTO_BUF_REG

[PATCH 0/2] ublk: two fixes on UBLK_F_AUTO_BUF_REG

[Ming Lei]

Hello Jens,

The 1st patches fix ublk_fetch() failure path for setting auto-buf-reg
data.

The 2nd patch makes sure that the buffer unregister is done in same
'io_ring_ctx' for issuing FETCH command.


Thanks,


Ming Lei (2):
  ublk: handle ublk_set_auto_buf_reg() failure correctly in ublk_fetch()
  ublk: run auto buf unregisgering in same io_ring_ctx with register

 drivers/block/ublk_drv.c      | 37 ++++++++++++++++++++++++++++++-----
 include/uapi/linux/ublk_cmd.h |  3 ++-
 2 files changed, 34 insertions(+), 6 deletions(-)

-- 
2.47.0

[PATCH 1/2] ublk: handle ublk_set_auto_buf_reg() failure correctly in ublk_fetch()

[Ming Lei]

If ublk_set_auto_buf_reg() fails, we need to unlock and return,
otherwise `ub->mutex` is leaked.

Fixes: 99c1e4eb6a3f ("ublk: register buffer to local io_uring with provided buf index via UBLK_F_AUTO_BUF_REG")
Reported-by: Caleb Sander Mateos <csander@purestorage.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/block/ublk_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 69230f4575d4..fcf568b89370 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -2075,7 +2075,7 @@ static int ublk_fetch(struct io_uring_cmd *cmd, struct ublk_queue *ubq,
 	if (ublk_support_auto_buf_reg(ubq)) {
 		ret = ublk_set_auto_buf_reg(cmd);
 		if (ret)
-			return ret;
+			goto out;
 	}
 
 	ublk_fill_io_cmd(io, cmd, buf_addr);
-- 
2.47.0

Re: [PATCH 1/2] ublk: handle ublk_set_auto_buf_reg() failure correctly in ublk_fetch()

[Caleb Sander Mateos]

On Tue, May 20, 2025 at 7:55 PM Ming Lei <ming.lei@redhat.com> wrote:
>
> If ublk_set_auto_buf_reg() fails, we need to unlock and return,
> otherwise `ub->mutex` is leaked.
>
> Fixes: 99c1e4eb6a3f ("ublk: register buffer to local io_uring with provided buf index via UBLK_F_AUTO_BUF_REG")
> Reported-by: Caleb Sander Mateos <csander@purestorage.com>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>

Reviewed-by: Caleb Sander Mateos <csander@purestorage.com>

[PATCH 2/2] ublk: run auto buf unregisgering in same io_ring_ctx with register

[Ming Lei]

UBLK_F_AUTO_BUF_REG requires that the buffer registered automatically
is unregistered in same `io_ring_ctx`, so check it explicitly.

Meantime return the failure code if io_buffer_unregister_bvec() fails,
then ublk server can handle the failure in consistent way.

Also force to clear UBLK_IO_FLAG_AUTO_BUF_REG in ublk_io_release()
because ublk_io_release() may be triggered not from handling
UBLK_IO_COMMIT_AND_FETCH_REQ, and from releasing the `io_ring_ctx`
for registering the buffer.

Fixes: 99c1e4eb6a3f ("ublk: register buffer to local io_uring with provided buf index via UBLK_F_AUTO_BUF_REG")
Reported-by: Caleb Sander Mateos <csander@purestorage.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/block/ublk_drv.c      | 35 +++++++++++++++++++++++++++++++----
 include/uapi/linux/ublk_cmd.h |  3 ++-
 2 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index fcf568b89370..2af6422d6a89 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -84,6 +84,7 @@ struct ublk_rq_data {
 
 	/* for auto-unregister buffer in case of UBLK_F_AUTO_BUF_REG */
 	u16 buf_index;
+	unsigned long buf_ctx_id;
 };
 
 struct ublk_uring_cmd_pdu {
@@ -1192,6 +1193,11 @@ static void ublk_auto_buf_reg_fallback(struct request *req, struct ublk_io *io)
 	refcount_set(&data->ref, 1);
 }
 
+static unsigned long ublk_uring_cmd_ctx_id(struct io_uring_cmd *cmd)
+{
+	return (unsigned long)(cmd_to_io_kiocb(cmd)->ctx);
+}
+
 static bool ublk_auto_buf_reg(struct request *req, struct ublk_io *io,
 			      unsigned int issue_flags)
 {
@@ -1211,6 +1217,8 @@ static bool ublk_auto_buf_reg(struct request *req, struct ublk_io *io,
 	}
 	/* one extra reference is dropped by ublk_io_release */
 	refcount_set(&data->ref, 2);
+
+	data->buf_ctx_id = ublk_uring_cmd_ctx_id(io->cmd);
 	/* store buffer index in request payload */
 	data->buf_index = pdu->buf.index;
 	io->flags |= UBLK_IO_FLAG_AUTO_BUF_REG;
@@ -1994,6 +2002,21 @@ static void ublk_io_release(void *priv)
 {
 	struct request *rq = priv;
 	struct ublk_queue *ubq = rq->mq_hctx->driver_data;
+	struct ublk_io *io = &ubq->ios[rq->tag];
+
+	/*
+	 * In case of UBLK_F_AUTO_BUF_REG, the `io_uring_ctx` for registering
+	 * this buffer may be released, so we reach here not from handling
+	 * `UBLK_IO_COMMIT_AND_FETCH_REQ`.
+	 *
+	 * Force to clear UBLK_IO_FLAG_AUTO_BUF_REG, so that ublk server
+	 * still may complete this IO request by issuing uring_cmd from
+	 * another `io_uring_ctx` in case that the `io_ring_ctx` for
+	 * registering the buffer is gone
+	 */
+	if (ublk_support_auto_buf_reg(ubq) &&
+			(io->flags & UBLK_IO_FLAG_AUTO_BUF_REG))
+		io->flags &= ~UBLK_IO_FLAG_AUTO_BUF_REG;
 
 	ublk_put_req_ref(ubq, rq);
 }
@@ -2109,14 +2132,18 @@ static int ublk_commit_and_fetch(const struct ublk_queue *ubq,
 	}
 
 	if (ublk_support_auto_buf_reg(ubq)) {
+		struct ublk_rq_data *data = blk_mq_rq_to_pdu(req);
 		int ret;
 
 		if (io->flags & UBLK_IO_FLAG_AUTO_BUF_REG) {
-			struct ublk_rq_data *data = blk_mq_rq_to_pdu(req);
 
-			WARN_ON_ONCE(io_buffer_unregister_bvec(cmd,
-						data->buf_index,
-						issue_flags));
+			if (data->buf_ctx_id != ublk_uring_cmd_ctx_id(cmd))
+				return -EBADF;
+
+			ret = io_buffer_unregister_bvec(cmd, data->buf_index,
+							issue_flags);
+			if (ret)
+				return ret;
 			io->flags &= ~UBLK_IO_FLAG_AUTO_BUF_REG;
 		}
 
diff --git a/include/uapi/linux/ublk_cmd.h b/include/uapi/linux/ublk_cmd.h
index c4b9942697fc..3db604a3045e 100644
--- a/include/uapi/linux/ublk_cmd.h
+++ b/include/uapi/linux/ublk_cmd.h
@@ -226,7 +226,8 @@
  *
  * For using this feature:
  *
- * - ublk server has to create sparse buffer table
+ * - ublk server has to create sparse buffer table on the same `io_ring_ctx`
+ *   for issuing `UBLK_IO_FETCH_REQ` and `UBLK_IO_COMMIT_AND_FETCH_REQ`
  *
  * - ublk server passes auto buf register data via uring_cmd's sqe->addr,
  *   `struct ublk_auto_buf_reg` is populated from sqe->addr, please see
-- 
2.47.0

Re: [PATCH 2/2] ublk: run auto buf unregisgering in same io_ring_ctx with register

[Caleb Sander Mateos]

On Tue, May 20, 2025 at 7:55 PM Ming Lei <ming.lei@redhat.com> wrote:
>
> UBLK_F_AUTO_BUF_REG requires that the buffer registered automatically
> is unregistered in same `io_ring_ctx`, so check it explicitly.
>
> Meantime return the failure code if io_buffer_unregister_bvec() fails,
> then ublk server can handle the failure in consistent way.
>
> Also force to clear UBLK_IO_FLAG_AUTO_BUF_REG in ublk_io_release()
> because ublk_io_release() may be triggered not from handling
> UBLK_IO_COMMIT_AND_FETCH_REQ, and from releasing the `io_ring_ctx`
> for registering the buffer.
>
> Fixes: 99c1e4eb6a3f ("ublk: register buffer to local io_uring with provided buf index via UBLK_F_AUTO_BUF_REG")
> Reported-by: Caleb Sander Mateos <csander@purestorage.com>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>  drivers/block/ublk_drv.c      | 35 +++++++++++++++++++++++++++++++----
>  include/uapi/linux/ublk_cmd.h |  3 ++-
>  2 files changed, 33 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index fcf568b89370..2af6422d6a89 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -84,6 +84,7 @@ struct ublk_rq_data {
>
>         /* for auto-unregister buffer in case of UBLK_F_AUTO_BUF_REG */
>         u16 buf_index;
> +       unsigned long buf_ctx_id;
>  };
>
>  struct ublk_uring_cmd_pdu {
> @@ -1192,6 +1193,11 @@ static void ublk_auto_buf_reg_fallback(struct request *req, struct ublk_io *io)
>         refcount_set(&data->ref, 1);
>  }
>
> +static unsigned long ublk_uring_cmd_ctx_id(struct io_uring_cmd *cmd)
> +{
> +       return (unsigned long)(cmd_to_io_kiocb(cmd)->ctx);

Is the fact that a struct io_uring_cmd * can be passed to
cmd_to_io_kiocb() an io_uring internal implementation detail? Maybe it
would be good to add a helper in include/linux/io_uring/cmd.h so ublk
isn't depending on io_uring internals.

Also, storing buf_ctx_id as a void * instead would allow this cast to
be avoided, but not a big deal.

> +}
> +
>  static bool ublk_auto_buf_reg(struct request *req, struct ublk_io *io,
>                               unsigned int issue_flags)
>  {
> @@ -1211,6 +1217,8 @@ static bool ublk_auto_buf_reg(struct request *req, struct ublk_io *io,
>         }
>         /* one extra reference is dropped by ublk_io_release */
>         refcount_set(&data->ref, 2);
> +
> +       data->buf_ctx_id = ublk_uring_cmd_ctx_id(io->cmd);
>         /* store buffer index in request payload */
>         data->buf_index = pdu->buf.index;
>         io->flags |= UBLK_IO_FLAG_AUTO_BUF_REG;
> @@ -1994,6 +2002,21 @@ static void ublk_io_release(void *priv)
>  {
>         struct request *rq = priv;
>         struct ublk_queue *ubq = rq->mq_hctx->driver_data;
> +       struct ublk_io *io = &ubq->ios[rq->tag];
> +
> +       /*
> +        * In case of UBLK_F_AUTO_BUF_REG, the `io_uring_ctx` for registering
> +        * this buffer may be released, so we reach here not from handling
> +        * `UBLK_IO_COMMIT_AND_FETCH_REQ`.

What do you mean by this? That the io_uring was closed while a ublk
I/O owned by the server still had a registered buffer?

> +        *
> +        * Force to clear UBLK_IO_FLAG_AUTO_BUF_REG, so that ublk server
> +        * still may complete this IO request by issuing uring_cmd from
> +        * another `io_uring_ctx` in case that the `io_ring_ctx` for
> +        * registering the buffer is gone
> +        */
> +       if (ublk_support_auto_buf_reg(ubq) &&
> +                       (io->flags & UBLK_IO_FLAG_AUTO_BUF_REG))
> +               io->flags &= ~UBLK_IO_FLAG_AUTO_BUF_REG;

This is racy, since ublk_io_release() can be called on a thread other
than the ubq_daemon. Could we avoid touching io->flags here and
instead have ublk_commit_and_fetch() check whether the reference count
is already 1?

Also, the ublk_support_auto_buf_reg(ubq) check seems redundant, since
UBLK_IO_FLAG_AUTO_BUF_REG is set in ublk_auto_buf_reg(), which is only
called if ublk_support_auto_buf_reg(ubq).

>
>         ublk_put_req_ref(ubq, rq);
>  }
> @@ -2109,14 +2132,18 @@ static int ublk_commit_and_fetch(const struct ublk_queue *ubq,
>         }
>
>         if (ublk_support_auto_buf_reg(ubq)) {
> +               struct ublk_rq_data *data = blk_mq_rq_to_pdu(req);
>                 int ret;
>
>                 if (io->flags & UBLK_IO_FLAG_AUTO_BUF_REG) {
> -                       struct ublk_rq_data *data = blk_mq_rq_to_pdu(req);

Why does this variable need to move to the outer scope?

Best,
Caleb

>
> -                       WARN_ON_ONCE(io_buffer_unregister_bvec(cmd,
> -                                               data->buf_index,
> -                                               issue_flags));
> +                       if (data->buf_ctx_id != ublk_uring_cmd_ctx_id(cmd))
> +                               return -EBADF;
> +
> +                       ret = io_buffer_unregister_bvec(cmd, data->buf_index,
> +                                                       issue_flags);
> +                       if (ret)
> +                               return ret;
>                         io->flags &= ~UBLK_IO_FLAG_AUTO_BUF_REG;
>                 }
>
> diff --git a/include/uapi/linux/ublk_cmd.h b/include/uapi/linux/ublk_cmd.h
> index c4b9942697fc..3db604a3045e 100644
> --- a/include/uapi/linux/ublk_cmd.h
> +++ b/include/uapi/linux/ublk_cmd.h
> @@ -226,7 +226,8 @@
>   *
>   * For using this feature:
>   *
> - * - ublk server has to create sparse buffer table
> + * - ublk server has to create sparse buffer table on the same `io_ring_ctx`
> + *   for issuing `UBLK_IO_FETCH_REQ` and `UBLK_IO_COMMIT_AND_FETCH_REQ`
>   *
>   * - ublk server passes auto buf register data via uring_cmd's sqe->addr,
>   *   `struct ublk_auto_buf_reg` is populated from sqe->addr, please see
> --
> 2.47.0
>

Re: [PATCH 2/2] ublk: run auto buf unregisgering in same io_ring_ctx with register

[Ming Lei]

On Wed, May 21, 2025 at 08:58:20AM -0700, Caleb Sander Mateos wrote:
> On Tue, May 20, 2025 at 7:55 PM Ming Lei <ming.lei@redhat.com> wrote:
> >
> > UBLK_F_AUTO_BUF_REG requires that the buffer registered automatically
> > is unregistered in same `io_ring_ctx`, so check it explicitly.
> >
> > Meantime return the failure code if io_buffer_unregister_bvec() fails,
> > then ublk server can handle the failure in consistent way.
> >
> > Also force to clear UBLK_IO_FLAG_AUTO_BUF_REG in ublk_io_release()
> > because ublk_io_release() may be triggered not from handling
> > UBLK_IO_COMMIT_AND_FETCH_REQ, and from releasing the `io_ring_ctx`
> > for registering the buffer.
> >
> > Fixes: 99c1e4eb6a3f ("ublk: register buffer to local io_uring with provided buf index via UBLK_F_AUTO_BUF_REG")
> > Reported-by: Caleb Sander Mateos <csander@purestorage.com>
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >  drivers/block/ublk_drv.c      | 35 +++++++++++++++++++++++++++++++----
> >  include/uapi/linux/ublk_cmd.h |  3 ++-
> >  2 files changed, 33 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> > index fcf568b89370..2af6422d6a89 100644
> > --- a/drivers/block/ublk_drv.c
> > +++ b/drivers/block/ublk_drv.c
> > @@ -84,6 +84,7 @@ struct ublk_rq_data {
> >
> >         /* for auto-unregister buffer in case of UBLK_F_AUTO_BUF_REG */
> >         u16 buf_index;
> > +       unsigned long buf_ctx_id;
> >  };
> >
> >  struct ublk_uring_cmd_pdu {
> > @@ -1192,6 +1193,11 @@ static void ublk_auto_buf_reg_fallback(struct request *req, struct ublk_io *io)
> >         refcount_set(&data->ref, 1);
> >  }
> >
> > +static unsigned long ublk_uring_cmd_ctx_id(struct io_uring_cmd *cmd)
> > +{
> > +       return (unsigned long)(cmd_to_io_kiocb(cmd)->ctx);
> 
> Is the fact that a struct io_uring_cmd * can be passed to
> cmd_to_io_kiocb() an io_uring internal implementation detail? Maybe it
> would be good to add a helper in include/linux/io_uring/cmd.h so ublk
> isn't depending on io_uring internals.

All this definition is defined in kernel public header, not sure if it is
big deal to add the helper, especially there is just single user.

But I will do it.

> 
> Also, storing buf_ctx_id as a void * instead would allow this cast to
> be avoided, but not a big deal.
> 
> > +}
> > +
> >  static bool ublk_auto_buf_reg(struct request *req, struct ublk_io *io,
> >                               unsigned int issue_flags)
> >  {
> > @@ -1211,6 +1217,8 @@ static bool ublk_auto_buf_reg(struct request *req, struct ublk_io *io,
> >         }
> >         /* one extra reference is dropped by ublk_io_release */
> >         refcount_set(&data->ref, 2);
> > +
> > +       data->buf_ctx_id = ublk_uring_cmd_ctx_id(io->cmd);
> >         /* store buffer index in request payload */
> >         data->buf_index = pdu->buf.index;
> >         io->flags |= UBLK_IO_FLAG_AUTO_BUF_REG;
> > @@ -1994,6 +2002,21 @@ static void ublk_io_release(void *priv)
> >  {
> >         struct request *rq = priv;
> >         struct ublk_queue *ubq = rq->mq_hctx->driver_data;
> > +       struct ublk_io *io = &ubq->ios[rq->tag];
> > +
> > +       /*
> > +        * In case of UBLK_F_AUTO_BUF_REG, the `io_uring_ctx` for registering
> > +        * this buffer may be released, so we reach here not from handling
> > +        * `UBLK_IO_COMMIT_AND_FETCH_REQ`.
> 
> What do you mean by this? That the io_uring was closed while a ublk
> I/O owned by the server still had a registered buffer?

The buffer is registered to `io_ring_ctx A`, which is closed and the buffer
is used up and un-registered on `io_ring_ctx A`, so this callback is
triggered, but the io command isn't completed yet, which can be run from
`io_ring_ctx B`

> 
> > +        *
> > +        * Force to clear UBLK_IO_FLAG_AUTO_BUF_REG, so that ublk server
> > +        * still may complete this IO request by issuing uring_cmd from
> > +        * another `io_uring_ctx` in case that the `io_ring_ctx` for
> > +        * registering the buffer is gone
> > +        */
> > +       if (ublk_support_auto_buf_reg(ubq) &&
> > +                       (io->flags & UBLK_IO_FLAG_AUTO_BUF_REG))
> > +               io->flags &= ~UBLK_IO_FLAG_AUTO_BUF_REG;
> 
> This is racy, since ublk_io_release() can be called on a thread other
> than the ubq_daemon.

Yeah, it can be true.

> Could we avoid touching io->flags here and
> instead have ublk_commit_and_fetch() check whether the reference count
> is already 1?

It is still a little racy because the buffer unregister from another thread
can happen just after the check immediately.

Adding one spinlock should cover it.

And it shouldn't be one big thing, because anyway the buffer can only be
released once.

> 
> Also, the ublk_support_auto_buf_reg(ubq) check seems redundant, since
> UBLK_IO_FLAG_AUTO_BUF_REG is set in ublk_auto_buf_reg(), which is only
> called if ublk_support_auto_buf_reg(ubq).

It has document benefit at least, so I'd suggest to keep it.


Thanks
Ming

Re: [PATCH 2/2] ublk: run auto buf unregisgering in same io_ring_ctx with register

[Caleb Sander Mateos]

On Wed, May 21, 2025 at 5:42 PM Ming Lei <ming.lei@redhat.com> wrote:
>
> On Wed, May 21, 2025 at 08:58:20AM -0700, Caleb Sander Mateos wrote:
> > On Tue, May 20, 2025 at 7:55 PM Ming Lei <ming.lei@redhat.com> wrote:
> > >
> > > UBLK_F_AUTO_BUF_REG requires that the buffer registered automatically
> > > is unregistered in same `io_ring_ctx`, so check it explicitly.
> > >
> > > Meantime return the failure code if io_buffer_unregister_bvec() fails,
> > > then ublk server can handle the failure in consistent way.
> > >
> > > Also force to clear UBLK_IO_FLAG_AUTO_BUF_REG in ublk_io_release()
> > > because ublk_io_release() may be triggered not from handling
> > > UBLK_IO_COMMIT_AND_FETCH_REQ, and from releasing the `io_ring_ctx`
> > > for registering the buffer.
> > >
> > > Fixes: 99c1e4eb6a3f ("ublk: register buffer to local io_uring with provided buf index via UBLK_F_AUTO_BUF_REG")
> > > Reported-by: Caleb Sander Mateos <csander@purestorage.com>
> > > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > > ---
> > >  drivers/block/ublk_drv.c      | 35 +++++++++++++++++++++++++++++++----
> > >  include/uapi/linux/ublk_cmd.h |  3 ++-
> > >  2 files changed, 33 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> > > index fcf568b89370..2af6422d6a89 100644
> > > --- a/drivers/block/ublk_drv.c
> > > +++ b/drivers/block/ublk_drv.c
> > > @@ -84,6 +84,7 @@ struct ublk_rq_data {
> > >
> > >         /* for auto-unregister buffer in case of UBLK_F_AUTO_BUF_REG */
> > >         u16 buf_index;
> > > +       unsigned long buf_ctx_id;
> > >  };
> > >
> > >  struct ublk_uring_cmd_pdu {
> > > @@ -1192,6 +1193,11 @@ static void ublk_auto_buf_reg_fallback(struct request *req, struct ublk_io *io)
> > >         refcount_set(&data->ref, 1);
> > >  }
> > >
> > > +static unsigned long ublk_uring_cmd_ctx_id(struct io_uring_cmd *cmd)
> > > +{
> > > +       return (unsigned long)(cmd_to_io_kiocb(cmd)->ctx);
> >
> > Is the fact that a struct io_uring_cmd * can be passed to
> > cmd_to_io_kiocb() an io_uring internal implementation detail? Maybe it
> > would be good to add a helper in include/linux/io_uring/cmd.h so ublk
> > isn't depending on io_uring internals.
>
> All this definition is defined in kernel public header, not sure if it is
> big deal to add the helper, especially there is just single user.
>
> But I will do it.
>
> >
> > Also, storing buf_ctx_id as a void * instead would allow this cast to
> > be avoided, but not a big deal.
> >
> > > +}
> > > +
> > >  static bool ublk_auto_buf_reg(struct request *req, struct ublk_io *io,
> > >                               unsigned int issue_flags)
> > >  {
> > > @@ -1211,6 +1217,8 @@ static bool ublk_auto_buf_reg(struct request *req, struct ublk_io *io,
> > >         }
> > >         /* one extra reference is dropped by ublk_io_release */
> > >         refcount_set(&data->ref, 2);
> > > +
> > > +       data->buf_ctx_id = ublk_uring_cmd_ctx_id(io->cmd);
> > >         /* store buffer index in request payload */
> > >         data->buf_index = pdu->buf.index;
> > >         io->flags |= UBLK_IO_FLAG_AUTO_BUF_REG;
> > > @@ -1994,6 +2002,21 @@ static void ublk_io_release(void *priv)
> > >  {
> > >         struct request *rq = priv;
> > >         struct ublk_queue *ubq = rq->mq_hctx->driver_data;
> > > +       struct ublk_io *io = &ubq->ios[rq->tag];
> > > +
> > > +       /*
> > > +        * In case of UBLK_F_AUTO_BUF_REG, the `io_uring_ctx` for registering
> > > +        * this buffer may be released, so we reach here not from handling
> > > +        * `UBLK_IO_COMMIT_AND_FETCH_REQ`.
> >
> > What do you mean by this? That the io_uring was closed while a ublk
> > I/O owned by the server still had a registered buffer?
>
> The buffer is registered to `io_ring_ctx A`, which is closed and the buffer
> is used up and un-registered on `io_ring_ctx A`, so this callback is
> triggered, but the io command isn't completed yet, which can be run from
> `io_ring_ctx B`
>
> >
> > > +        *
> > > +        * Force to clear UBLK_IO_FLAG_AUTO_BUF_REG, so that ublk server
> > > +        * still may complete this IO request by issuing uring_cmd from
> > > +        * another `io_uring_ctx` in case that the `io_ring_ctx` for
> > > +        * registering the buffer is gone
> > > +        */
> > > +       if (ublk_support_auto_buf_reg(ubq) &&
> > > +                       (io->flags & UBLK_IO_FLAG_AUTO_BUF_REG))
> > > +               io->flags &= ~UBLK_IO_FLAG_AUTO_BUF_REG;
> >
> > This is racy, since ublk_io_release() can be called on a thread other
> > than the ubq_daemon.
>
> Yeah, it can be true.
>
> > Could we avoid touching io->flags here and
> > instead have ublk_commit_and_fetch() check whether the reference count
> > is already 1?
>
> It is still a little racy because the buffer unregister from another thread
> can happen just after the check immediately.

True. I think it might be better to just skip the unregister if the
contexts don't match rather than returning -EINVAL. Then there is no
race. If userspace has already closed the old io_uring context,
skipping the unregister is the desired behavior. If userspace hasn't
closed the old io_uring, then that's a userspace bug and they get what
they deserve (a buffer stuck registered). If userspace wants to submit
the UBLK_IO_COMMIT_AND_FETCH_REQ on a different io_uring for some
reason, they can always issue an explicit UBLK_IO_UNREGISTER_IO_BUF on
the old io_uring to unregister the buffer.

>
> Adding one spinlock should cover it.

I would really prefer not to add a spinlock in the I/O path.

>
> And it shouldn't be one big thing, because anyway the buffer can only be
> released once.
>
> >
> > Also, the ublk_support_auto_buf_reg(ubq) check seems redundant, since
> > UBLK_IO_FLAG_AUTO_BUF_REG is set in ublk_auto_buf_reg(), which is only
> > called if ublk_support_auto_buf_reg(ubq).
>
> It has document benefit at least, so I'd suggest to keep it.

Doesn't checking for UBLK_IO_FLAG_AUTO_BUF_REG already document that
this only applies to auto buffer registration?

Best,
Caleb

Re: [PATCH 2/2] ublk: run auto buf unregisgering in same io_ring_ctx with register

[Ming Lei]

On Wed, May 21, 2025 at 06:05:14PM -0700, Caleb Sander Mateos wrote:
> On Wed, May 21, 2025 at 5:42 PM Ming Lei <ming.lei@redhat.com> wrote:
> >
> > On Wed, May 21, 2025 at 08:58:20AM -0700, Caleb Sander Mateos wrote:
> > > On Tue, May 20, 2025 at 7:55 PM Ming Lei <ming.lei@redhat.com> wrote:
> > > >
> > > > UBLK_F_AUTO_BUF_REG requires that the buffer registered automatically
> > > > is unregistered in same `io_ring_ctx`, so check it explicitly.
> > > >
> > > > Meantime return the failure code if io_buffer_unregister_bvec() fails,
> > > > then ublk server can handle the failure in consistent way.
> > > >
> > > > Also force to clear UBLK_IO_FLAG_AUTO_BUF_REG in ublk_io_release()
> > > > because ublk_io_release() may be triggered not from handling
> > > > UBLK_IO_COMMIT_AND_FETCH_REQ, and from releasing the `io_ring_ctx`
> > > > for registering the buffer.
> > > >
> > > > Fixes: 99c1e4eb6a3f ("ublk: register buffer to local io_uring with provided buf index via UBLK_F_AUTO_BUF_REG")
> > > > Reported-by: Caleb Sander Mateos <csander@purestorage.com>
> > > > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > > > ---
> > > >  drivers/block/ublk_drv.c      | 35 +++++++++++++++++++++++++++++++----
> > > >  include/uapi/linux/ublk_cmd.h |  3 ++-
> > > >  2 files changed, 33 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> > > > index fcf568b89370..2af6422d6a89 100644
> > > > --- a/drivers/block/ublk_drv.c
> > > > +++ b/drivers/block/ublk_drv.c
> > > > @@ -84,6 +84,7 @@ struct ublk_rq_data {
> > > >
> > > >         /* for auto-unregister buffer in case of UBLK_F_AUTO_BUF_REG */
> > > >         u16 buf_index;
> > > > +       unsigned long buf_ctx_id;
> > > >  };
> > > >
> > > >  struct ublk_uring_cmd_pdu {
> > > > @@ -1192,6 +1193,11 @@ static void ublk_auto_buf_reg_fallback(struct request *req, struct ublk_io *io)
> > > >         refcount_set(&data->ref, 1);
> > > >  }
> > > >
> > > > +static unsigned long ublk_uring_cmd_ctx_id(struct io_uring_cmd *cmd)
> > > > +{
> > > > +       return (unsigned long)(cmd_to_io_kiocb(cmd)->ctx);
> > >
> > > Is the fact that a struct io_uring_cmd * can be passed to
> > > cmd_to_io_kiocb() an io_uring internal implementation detail? Maybe it
> > > would be good to add a helper in include/linux/io_uring/cmd.h so ublk
> > > isn't depending on io_uring internals.
> >
> > All this definition is defined in kernel public header, not sure if it is
> > big deal to add the helper, especially there is just single user.
> >
> > But I will do it.
> >
> > >
> > > Also, storing buf_ctx_id as a void * instead would allow this cast to
> > > be avoided, but not a big deal.
> > >
> > > > +}
> > > > +
> > > >  static bool ublk_auto_buf_reg(struct request *req, struct ublk_io *io,
> > > >                               unsigned int issue_flags)
> > > >  {
> > > > @@ -1211,6 +1217,8 @@ static bool ublk_auto_buf_reg(struct request *req, struct ublk_io *io,
> > > >         }
> > > >         /* one extra reference is dropped by ublk_io_release */
> > > >         refcount_set(&data->ref, 2);
> > > > +
> > > > +       data->buf_ctx_id = ublk_uring_cmd_ctx_id(io->cmd);
> > > >         /* store buffer index in request payload */
> > > >         data->buf_index = pdu->buf.index;
> > > >         io->flags |= UBLK_IO_FLAG_AUTO_BUF_REG;
> > > > @@ -1994,6 +2002,21 @@ static void ublk_io_release(void *priv)
> > > >  {
> > > >         struct request *rq = priv;
> > > >         struct ublk_queue *ubq = rq->mq_hctx->driver_data;
> > > > +       struct ublk_io *io = &ubq->ios[rq->tag];
> > > > +
> > > > +       /*
> > > > +        * In case of UBLK_F_AUTO_BUF_REG, the `io_uring_ctx` for registering
> > > > +        * this buffer may be released, so we reach here not from handling
> > > > +        * `UBLK_IO_COMMIT_AND_FETCH_REQ`.
> > >
> > > What do you mean by this? That the io_uring was closed while a ublk
> > > I/O owned by the server still had a registered buffer?
> >
> > The buffer is registered to `io_ring_ctx A`, which is closed and the buffer
> > is used up and un-registered on `io_ring_ctx A`, so this callback is
> > triggered, but the io command isn't completed yet, which can be run from
> > `io_ring_ctx B`
> >
> > >
> > > > +        *
> > > > +        * Force to clear UBLK_IO_FLAG_AUTO_BUF_REG, so that ublk server
> > > > +        * still may complete this IO request by issuing uring_cmd from
> > > > +        * another `io_uring_ctx` in case that the `io_ring_ctx` for
> > > > +        * registering the buffer is gone
> > > > +        */
> > > > +       if (ublk_support_auto_buf_reg(ubq) &&
> > > > +                       (io->flags & UBLK_IO_FLAG_AUTO_BUF_REG))
> > > > +               io->flags &= ~UBLK_IO_FLAG_AUTO_BUF_REG;
> > >
> > > This is racy, since ublk_io_release() can be called on a thread other
> > > than the ubq_daemon.
> >
> > Yeah, it can be true.
> >
> > > Could we avoid touching io->flags here and
> > > instead have ublk_commit_and_fetch() check whether the reference count
> > > is already 1?
> >
> > It is still a little racy because the buffer unregister from another thread
> > can happen just after the check immediately.
> 
> True. I think it might be better to just skip the unregister if the
> contexts don't match rather than returning -EINVAL. Then there is no
> race. If userspace has already closed the old io_uring context,
> skipping the unregister is the desired behavior. If userspace hasn't
> closed the old io_uring, then that's a userspace bug and they get what
> they deserve (a buffer stuck registered). If userspace wants to submit
> the UBLK_IO_COMMIT_AND_FETCH_REQ on a different io_uring for some
> reason, they can always issue an explicit UBLK_IO_UNREGISTER_IO_BUF on
> the old io_uring to unregister the buffer.

Sounds good, will add the above words on ublk_commit_and_fetch().

Follows two invariants:

- ublk_io_release() is always called once no matter if it is called
from any thread context, request can't be completed until ublk_io_release()
is called

- new UBLK_IO_COMMIT_AND_FETCH_REQ can't be issued until old request
is completed & new request comes

The merged code should work just fine.

But we need to document the feature only works on same io_ring_context.


Thanks, 
Ming

Re: [PATCH 2/2] ublk: run auto buf unregisgering in same io_ring_ctx with register

[Caleb Sander Mateos]

There's also a typo "unregisgering" in the commit message.

On Tue, May 20, 2025 at 7:55 PM Ming Lei <ming.lei@redhat.com> wrote:
>
> UBLK_F_AUTO_BUF_REG requires that the buffer registered automatically
> is unregistered in same `io_ring_ctx`, so check it explicitly.
>
> Meantime return the failure code if io_buffer_unregister_bvec() fails,
> then ublk server can handle the failure in consistent way.
>
> Also force to clear UBLK_IO_FLAG_AUTO_BUF_REG in ublk_io_release()
> because ublk_io_release() may be triggered not from handling
> UBLK_IO_COMMIT_AND_FETCH_REQ, and from releasing the `io_ring_ctx`
> for registering the buffer.
>
> Fixes: 99c1e4eb6a3f ("ublk: register buffer to local io_uring with provided buf index via UBLK_F_AUTO_BUF_REG")
> Reported-by: Caleb Sander Mateos <csander@purestorage.com>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>  drivers/block/ublk_drv.c      | 35 +++++++++++++++++++++++++++++++----
>  include/uapi/linux/ublk_cmd.h |  3 ++-
>  2 files changed, 33 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index fcf568b89370..2af6422d6a89 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -84,6 +84,7 @@ struct ublk_rq_data {
>
>         /* for auto-unregister buffer in case of UBLK_F_AUTO_BUF_REG */
>         u16 buf_index;
> +       unsigned long buf_ctx_id;
>  };
>
>  struct ublk_uring_cmd_pdu {
> @@ -1192,6 +1193,11 @@ static void ublk_auto_buf_reg_fallback(struct request *req, struct ublk_io *io)
>         refcount_set(&data->ref, 1);
>  }
>
> +static unsigned long ublk_uring_cmd_ctx_id(struct io_uring_cmd *cmd)
> +{
> +       return (unsigned long)(cmd_to_io_kiocb(cmd)->ctx);
> +}
> +
>  static bool ublk_auto_buf_reg(struct request *req, struct ublk_io *io,
>                               unsigned int issue_flags)
>  {
> @@ -1211,6 +1217,8 @@ static bool ublk_auto_buf_reg(struct request *req, struct ublk_io *io,
>         }
>         /* one extra reference is dropped by ublk_io_release */
>         refcount_set(&data->ref, 2);
> +
> +       data->buf_ctx_id = ublk_uring_cmd_ctx_id(io->cmd);
>         /* store buffer index in request payload */
>         data->buf_index = pdu->buf.index;
>         io->flags |= UBLK_IO_FLAG_AUTO_BUF_REG;
> @@ -1994,6 +2002,21 @@ static void ublk_io_release(void *priv)
>  {
>         struct request *rq = priv;
>         struct ublk_queue *ubq = rq->mq_hctx->driver_data;
> +       struct ublk_io *io = &ubq->ios[rq->tag];
> +
> +       /*
> +        * In case of UBLK_F_AUTO_BUF_REG, the `io_uring_ctx` for registering
> +        * this buffer may be released, so we reach here not from handling
> +        * `UBLK_IO_COMMIT_AND_FETCH_REQ`.
> +        *
> +        * Force to clear UBLK_IO_FLAG_AUTO_BUF_REG, so that ublk server
> +        * still may complete this IO request by issuing uring_cmd from
> +        * another `io_uring_ctx` in case that the `io_ring_ctx` for
> +        * registering the buffer is gone
> +        */
> +       if (ublk_support_auto_buf_reg(ubq) &&
> +                       (io->flags & UBLK_IO_FLAG_AUTO_BUF_REG))
> +               io->flags &= ~UBLK_IO_FLAG_AUTO_BUF_REG;
>
>         ublk_put_req_ref(ubq, rq);
>  }
> @@ -2109,14 +2132,18 @@ static int ublk_commit_and_fetch(const struct ublk_queue *ubq,
>         }
>
>         if (ublk_support_auto_buf_reg(ubq)) {
> +               struct ublk_rq_data *data = blk_mq_rq_to_pdu(req);
>                 int ret;
>
>                 if (io->flags & UBLK_IO_FLAG_AUTO_BUF_REG) {
> -                       struct ublk_rq_data *data = blk_mq_rq_to_pdu(req);
>
> -                       WARN_ON_ONCE(io_buffer_unregister_bvec(cmd,
> -                                               data->buf_index,
> -                                               issue_flags));
> +                       if (data->buf_ctx_id != ublk_uring_cmd_ctx_id(cmd))
> +                               return -EBADF;
> +
> +                       ret = io_buffer_unregister_bvec(cmd, data->buf_index,
> +                                                       issue_flags);
> +                       if (ret)
> +                               return ret;
>                         io->flags &= ~UBLK_IO_FLAG_AUTO_BUF_REG;
>                 }
>
> diff --git a/include/uapi/linux/ublk_cmd.h b/include/uapi/linux/ublk_cmd.h
> index c4b9942697fc..3db604a3045e 100644
> --- a/include/uapi/linux/ublk_cmd.h
> +++ b/include/uapi/linux/ublk_cmd.h
> @@ -226,7 +226,8 @@
>   *
>   * For using this feature:
>   *
> - * - ublk server has to create sparse buffer table
> + * - ublk server has to create sparse buffer table on the same `io_ring_ctx`
> + *   for issuing `UBLK_IO_FETCH_REQ` and `UBLK_IO_COMMIT_AND_FETCH_REQ`
>   *
>   * - ublk server passes auto buf register data via uring_cmd's sqe->addr,
>   *   `struct ublk_auto_buf_reg` is populated from sqe->addr, please see
> --
> 2.47.0
>

Re: [PATCH 0/2] ublk: two fixes on UBLK_F_AUTO_BUF_REG

[Jens Axboe]

On Wed, 21 May 2025 10:54:58 +0800, Ming Lei wrote:
> The 1st patches fix ublk_fetch() failure path for setting auto-buf-reg
> data.
> 
> The 2nd patch makes sure that the buffer unregister is done in same
> 'io_ring_ctx' for issuing FETCH command.
> 
> 
> [...]

Applied, thanks!

[1/2] ublk: handle ublk_set_auto_buf_reg() failure correctly in ublk_fetch()
      commit: 5b2a1c40684ba59570b302a56de31defc8d514ee
[2/2] ublk: run auto buf unregisgering in same io_ring_ctx with register
      commit: 349c41125d3945c53f2c3d48d7225dbdd2c99801

Best regards,
-- 
Jens Axboe

Re: [PATCH 0/2] ublk: two fixes on UBLK_F_AUTO_BUF_REG

[Jens Axboe]

On 5/21/25 7:11 AM, Jens Axboe wrote:
> 
> On Wed, 21 May 2025 10:54:58 +0800, Ming Lei wrote:
>> The 1st patches fix ublk_fetch() failure path for setting auto-buf-reg
>> data.
>>
>> The 2nd patch makes sure that the buffer unregister is done in same
>> 'io_ring_ctx' for issuing FETCH command.
>>
>>
>> [...]
> 
> Applied, thanks!
> 
> [1/2] ublk: handle ublk_set_auto_buf_reg() failure correctly in ublk_fetch()
>       commit: 5b2a1c40684ba59570b302a56de31defc8d514ee
> [2/2] ublk: run auto buf unregisgering in same io_ring_ctx with register
>       commit: 349c41125d3945c53f2c3d48d7225dbdd2c99801

Dropped 2/2 for now.

-- 
Jens Axboe