ublk: clean up user copy references on ublk server exit

[PATCH] ublk: clean up user copy references on ublk server exit

Posted by Caleb Sander Mateos 1 month, 3 weeks ago

If a ublk server process releases a ublk char device file, any requests
dispatched to the ublk server but not yet completed will retain a ref
value of UBLK_REFCOUNT_INIT. Before commit e63d2228ef83 ("ublk: simplify
aborting ublk request"), __ublk_fail_req() would decrement the reference
count before completing the failed request. However, that commit
optimized __ublk_fail_req() to call __ublk_complete_rq() directly
without decrementing the request reference count.
The leaked reference count incorrectly allows user copy and zero copy
operations on the completed ublk request. It also triggers the
WARN_ON_ONCE(refcount_read(&io->ref)) warnings in ublk_queue_reinit()
and ublk_deinit_queue().
Commit c5c5eb24ed61 ("ublk: avoid ublk_io_release() called after ublk
char dev is closed") already fixed the issue for ublk devices using
UBLK_F_SUPPORT_ZERO_COPY or UBLK_F_AUTO_BUF_REG. However, the reference
count leak also affects UBLK_F_USER_COPY, the other reference-counted
data copy mode. Fix the condition in ublk_check_and_reset_active_ref()
to include all reference-counted data copy modes. This ensures that any
ublk requests still owned by the ublk server when it exits have their
reference counts reset to 0.

Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
Fixes: e63d2228ef83 ("ublk: simplify aborting ublk request")
---
 drivers/block/ublk_drv.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index df9831783a13..78f3e22151b9 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -1581,12 +1581,11 @@ static void ublk_set_canceling(struct ublk_device *ub, bool canceling)
 
 static bool ublk_check_and_reset_active_ref(struct ublk_device *ub)
 {
 	int i, j;
 
-	if (!(ub->dev_info.flags & (UBLK_F_SUPPORT_ZERO_COPY |
-					UBLK_F_AUTO_BUF_REG)))
+	if (!ublk_dev_need_req_ref(ub))
 		return false;
 
 	for (i = 0; i < ub->dev_info.nr_hw_queues; i++) {
 		struct ublk_queue *ubq = ublk_get_queue(ub, i);
 
-- 
2.45.2

Re: [PATCH] ublk: clean up user copy references on ublk server exit

Posted by Jens Axboe 1 month, 2 weeks ago

On Fri, 12 Dec 2025 17:19:49 -0700, Caleb Sander Mateos wrote:
> If a ublk server process releases a ublk char device file, any requests
> dispatched to the ublk server but not yet completed will retain a ref
> value of UBLK_REFCOUNT_INIT. Before commit e63d2228ef83 ("ublk: simplify
> aborting ublk request"), __ublk_fail_req() would decrement the reference
> count before completing the failed request. However, that commit
> optimized __ublk_fail_req() to call __ublk_complete_rq() directly
> without decrementing the request reference count.
> The leaked reference count incorrectly allows user copy and zero copy
> operations on the completed ublk request. It also triggers the
> WARN_ON_ONCE(refcount_read(&io->ref)) warnings in ublk_queue_reinit()
> and ublk_deinit_queue().
> Commit c5c5eb24ed61 ("ublk: avoid ublk_io_release() called after ublk
> char dev is closed") already fixed the issue for ublk devices using
> UBLK_F_SUPPORT_ZERO_COPY or UBLK_F_AUTO_BUF_REG. However, the reference
> count leak also affects UBLK_F_USER_COPY, the other reference-counted
> data copy mode. Fix the condition in ublk_check_and_reset_active_ref()
> to include all reference-counted data copy modes. This ensures that any
> ublk requests still owned by the ublk server when it exits have their
> reference counts reset to 0.
> 
> [...]

Applied, thanks!

[1/1] ublk: clean up user copy references on ublk server exit
      commit: daa24603d9f0808929514ee62ced30052ca7221c

Best regards,
-- 
Jens Axboe

Re: [PATCH] ublk: clean up user copy references on ublk server exit

Posted by Caleb Sander Mateos 1 month, 2 weeks ago

Ming, would you mind taking a (hopefully quick) look at this fix? The
warning reproduces easily with the ublk user copy selftests I recently
added.

Thanks,
Caleb

On Fri, Dec 12, 2025 at 7:19 PM Caleb Sander Mateos
<csander@purestorage.com> wrote:
>
> If a ublk server process releases a ublk char device file, any requests
> dispatched to the ublk server but not yet completed will retain a ref
> value of UBLK_REFCOUNT_INIT. Before commit e63d2228ef83 ("ublk: simplify
> aborting ublk request"), __ublk_fail_req() would decrement the reference
> count before completing the failed request. However, that commit
> optimized __ublk_fail_req() to call __ublk_complete_rq() directly
> without decrementing the request reference count.
> The leaked reference count incorrectly allows user copy and zero copy
> operations on the completed ublk request. It also triggers the
> WARN_ON_ONCE(refcount_read(&io->ref)) warnings in ublk_queue_reinit()
> and ublk_deinit_queue().
> Commit c5c5eb24ed61 ("ublk: avoid ublk_io_release() called after ublk
> char dev is closed") already fixed the issue for ublk devices using
> UBLK_F_SUPPORT_ZERO_COPY or UBLK_F_AUTO_BUF_REG. However, the reference
> count leak also affects UBLK_F_USER_COPY, the other reference-counted
> data copy mode. Fix the condition in ublk_check_and_reset_active_ref()
> to include all reference-counted data copy modes. This ensures that any
> ublk requests still owned by the ublk server when it exits have their
> reference counts reset to 0.
>
> Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
> Fixes: e63d2228ef83 ("ublk: simplify aborting ublk request")
> ---
>  drivers/block/ublk_drv.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index df9831783a13..78f3e22151b9 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -1581,12 +1581,11 @@ static void ublk_set_canceling(struct ublk_device *ub, bool canceling)
>
>  static bool ublk_check_and_reset_active_ref(struct ublk_device *ub)
>  {
>         int i, j;
>
> -       if (!(ub->dev_info.flags & (UBLK_F_SUPPORT_ZERO_COPY |
> -                                       UBLK_F_AUTO_BUF_REG)))
> +       if (!ublk_dev_need_req_ref(ub))
>                 return false;
>
>         for (i = 0; i < ub->dev_info.nr_hw_queues; i++) {
>                 struct ublk_queue *ubq = ublk_get_queue(ub, i);
>
> --
> 2.45.2
>

Re: [PATCH] ublk: clean up user copy references on ublk server exit

Posted by Ming Lei 1 month, 2 weeks ago

On Fri, Dec 19, 2025 at 11:52:23AM -0500, Caleb Sander Mateos wrote:
> Ming, would you mind taking a (hopefully quick) look at this fix? The
> warning reproduces easily with the ublk user copy selftests I recently
> added.

Sure, sorry for missing this fix.

> 
> On Fri, Dec 12, 2025 at 7:19 PM Caleb Sander Mateos
> <csander@purestorage.com> wrote:
> >
> > If a ublk server process releases a ublk char device file, any requests
> > dispatched to the ublk server but not yet completed will retain a ref
> > value of UBLK_REFCOUNT_INIT. Before commit e63d2228ef83 ("ublk: simplify
> > aborting ublk request"), __ublk_fail_req() would decrement the reference
> > count before completing the failed request. However, that commit
> > optimized __ublk_fail_req() to call __ublk_complete_rq() directly
> > without decrementing the request reference count.
> > The leaked reference count incorrectly allows user copy and zero copy
> > operations on the completed ublk request. It also triggers the
> > WARN_ON_ONCE(refcount_read(&io->ref)) warnings in ublk_queue_reinit()
> > and ublk_deinit_queue().
> > Commit c5c5eb24ed61 ("ublk: avoid ublk_io_release() called after ublk
> > char dev is closed") already fixed the issue for ublk devices using
> > UBLK_F_SUPPORT_ZERO_COPY or UBLK_F_AUTO_BUF_REG. However, the reference
> > count leak also affects UBLK_F_USER_COPY, the other reference-counted
> > data copy mode. Fix the condition in ublk_check_and_reset_active_ref()
> > to include all reference-counted data copy modes. This ensures that any
> > ublk requests still owned by the ublk server when it exits have their
> > reference counts reset to 0.
> >
> > Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
> > Fixes: e63d2228ef83 ("ublk: simplify aborting ublk request")
> > ---
> >  drivers/block/ublk_drv.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> > index df9831783a13..78f3e22151b9 100644
> > --- a/drivers/block/ublk_drv.c
> > +++ b/drivers/block/ublk_drv.c
> > @@ -1581,12 +1581,11 @@ static void ublk_set_canceling(struct ublk_device *ub, bool canceling)
> >
> >  static bool ublk_check_and_reset_active_ref(struct ublk_device *ub)
> >  {
> >         int i, j;
> >
> > -       if (!(ub->dev_info.flags & (UBLK_F_SUPPORT_ZERO_COPY |
> > -                                       UBLK_F_AUTO_BUF_REG)))
> > +       if (!ublk_dev_need_req_ref(ub))
> >                 return false;
> >
> >         for (i = 0; i < ub->dev_info.nr_hw_queues; i++) {
> >                 struct ublk_queue *ubq = ublk_get_queue(ub, i);

This patch looks correct for covering user copy:

Reviewed-by: Ming Lei <ming.lei@redhat.com>


Thanks,
Ming