ublk: check for unprivileged daemon on each I/O fetch

[PATCH] ublk: check for unprivileged daemon on each I/O fetch

Posted by Caleb Sander Mateos 1 month, 3 weeks ago

Commit ab03a61c6614 ("ublk: have a per-io daemon instead of a per-queue
daemon") allowed each ublk I/O to have an independent daemon task.
However, nr_privileged_daemon is only computed based on whether the last
I/O fetched in each ublk queue has an unprivileged daemon task.
Fix this by checking whether every fetched I/O's daemon is privileged.
Change nr_privileged_daemon from a count of queues to a boolean
indicating whether any I/Os have an unprivileged daemon.

Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
Fixes: ab03a61c6614 ("ublk: have a per-io daemon instead of a per-queue daemon")
---
 drivers/block/ublk_drv.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 6561d2a561fa..a035070dd690 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -233,11 +233,11 @@ struct ublk_device {
 
 	struct ublk_params	params;
 
 	struct completion	completion;
 	unsigned int		nr_queues_ready;
-	unsigned int		nr_privileged_daemon;
+	bool 			unprivileged_daemons;
 	struct mutex cancel_mutex;
 	bool canceling;
 	pid_t 	ublksrv_tgid;
 };
 
@@ -1548,11 +1548,11 @@ static void ublk_reset_ch_dev(struct ublk_device *ub)
 		ublk_queue_reinit(ub, ublk_get_queue(ub, i));
 
 	/* set to NULL, otherwise new tasks cannot mmap io_cmd_buf */
 	ub->mm = NULL;
 	ub->nr_queues_ready = 0;
-	ub->nr_privileged_daemon = 0;
+	ub->unprivileged_daemons = false;
 	ub->ublksrv_tgid = -1;
 }
 
 static struct gendisk *ublk_get_disk(struct ublk_device *ub)
 {
@@ -1978,16 +1978,14 @@ static void ublk_reset_io_flags(struct ublk_device *ub)
 /* device can only be started after all IOs are ready */
 static void ublk_mark_io_ready(struct ublk_device *ub, struct ublk_queue *ubq)
 	__must_hold(&ub->mutex)
 {
 	ubq->nr_io_ready++;
-	if (ublk_queue_ready(ubq)) {
+	if (ublk_queue_ready(ubq))
 		ub->nr_queues_ready++;
-
-		if (capable(CAP_SYS_ADMIN))
-			ub->nr_privileged_daemon++;
-	}
+	if (!ub->unprivileged_daemons && !capable(CAP_SYS_ADMIN))
+		ub->unprivileged_daemons = true;
 
 	if (ub->nr_queues_ready == ub->dev_info.nr_hw_queues) {
 		/* now we are ready for handling ublk io request */
 		ublk_reset_io_flags(ub);
 		complete_all(&ub->completion);
@@ -2878,12 +2876,12 @@ static int ublk_ctrl_start_dev(struct ublk_device *ub,
 	ub->dev_info.ublksrv_pid = ublksrv_pid;
 	ub->ub_disk = disk;
 
 	ublk_apply_params(ub);
 
-	/* don't probe partitions if any one ubq daemon is un-trusted */
-	if (ub->nr_privileged_daemon != ub->nr_queues_ready)
+	/* don't probe partitions if any daemon task is un-trusted */
+	if (ub->unprivileged_daemons)
 		set_bit(GD_SUPPRESS_PART_SCAN, &disk->state);
 
 	ublk_get_device(ub);
 	ub->dev_info.state = UBLK_S_DEV_LIVE;
 
-- 
2.45.2

Re: [PATCH] ublk: check for unprivileged daemon on each I/O fetch

Posted by Jens Axboe 1 month, 3 weeks ago

On Fri, 08 Aug 2025 09:52:15 -0600, Caleb Sander Mateos wrote:
> Commit ab03a61c6614 ("ublk: have a per-io daemon instead of a per-queue
> daemon") allowed each ublk I/O to have an independent daemon task.
> However, nr_privileged_daemon is only computed based on whether the last
> I/O fetched in each ublk queue has an unprivileged daemon task.
> Fix this by checking whether every fetched I/O's daemon is privileged.
> Change nr_privileged_daemon from a count of queues to a boolean
> indicating whether any I/Os have an unprivileged daemon.
> 
> [...]

Applied, thanks!

[1/1] ublk: check for unprivileged daemon on each I/O fetch
      commit: 5058a62875e1916e5133a1639f0207ea2148c0bc

Best regards,
-- 
Jens Axboe

Re: [PATCH] ublk: check for unprivileged daemon on each I/O fetch

Posted by Ming Lei 1 month, 3 weeks ago

On Fri, Aug 08, 2025 at 09:52:15AM -0600, Caleb Sander Mateos wrote:
> Commit ab03a61c6614 ("ublk: have a per-io daemon instead of a per-queue
> daemon") allowed each ublk I/O to have an independent daemon task.
> However, nr_privileged_daemon is only computed based on whether the last
> I/O fetched in each ublk queue has an unprivileged daemon task.
> Fix this by checking whether every fetched I/O's daemon is privileged.
> Change nr_privileged_daemon from a count of queues to a boolean
> indicating whether any I/Os have an unprivileged daemon.
> 
> Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
> Fixes: ab03a61c6614 ("ublk: have a per-io daemon instead of a per-queue daemon")

Looks fine,

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

Thanks,
Ming

Re: [PATCH] ublk: check for unprivileged daemon on each I/O fetch

Posted by Uday Shankar 1 month, 3 weeks ago

On Fri, Aug 08, 2025 at 09:52:15AM -0600, Caleb Sander Mateos wrote:
> Commit ab03a61c6614 ("ublk: have a per-io daemon instead of a per-queue
> daemon") allowed each ublk I/O to have an independent daemon task.
> However, nr_privileged_daemon is only computed based on whether the last
> I/O fetched in each ublk queue has an unprivileged daemon task.
> Fix this by checking whether every fetched I/O's daemon is privileged.
> Change nr_privileged_daemon from a count of queues to a boolean
> indicating whether any I/Os have an unprivileged daemon.
> 
> Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
> Fixes: ab03a61c6614 ("ublk: have a per-io daemon instead of a per-queue daemon")

Nice catch!

> ---
>  drivers/block/ublk_drv.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index 6561d2a561fa..a035070dd690 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -233,11 +233,11 @@ struct ublk_device {
>  
>  	struct ublk_params	params;
>  
>  	struct completion	completion;
>  	unsigned int		nr_queues_ready;
> -	unsigned int		nr_privileged_daemon;
> +	bool 			unprivileged_daemons;
>  	struct mutex cancel_mutex;
>  	bool canceling;
>  	pid_t 	ublksrv_tgid;
>  };
>  
> @@ -1548,11 +1548,11 @@ static void ublk_reset_ch_dev(struct ublk_device *ub)
>  		ublk_queue_reinit(ub, ublk_get_queue(ub, i));
>  
>  	/* set to NULL, otherwise new tasks cannot mmap io_cmd_buf */
>  	ub->mm = NULL;
>  	ub->nr_queues_ready = 0;
> -	ub->nr_privileged_daemon = 0;
> +	ub->unprivileged_daemons = false;
>  	ub->ublksrv_tgid = -1;
>  }
>  
>  static struct gendisk *ublk_get_disk(struct ublk_device *ub)
>  {
> @@ -1978,16 +1978,14 @@ static void ublk_reset_io_flags(struct ublk_device *ub)
>  /* device can only be started after all IOs are ready */
>  static void ublk_mark_io_ready(struct ublk_device *ub, struct ublk_queue *ubq)
>  	__must_hold(&ub->mutex)
>  {
>  	ubq->nr_io_ready++;
> -	if (ublk_queue_ready(ubq)) {
> +	if (ublk_queue_ready(ubq))
>  		ub->nr_queues_ready++;
> -
> -		if (capable(CAP_SYS_ADMIN))
> -			ub->nr_privileged_daemon++;
> -	}
> +	if (!ub->unprivileged_daemons && !capable(CAP_SYS_ADMIN))
> +		ub->unprivileged_daemons = true;

Shorter:

ub->unprivileged_daemons |= !capable(CAP_SYS_ADMIN);

>  
>  	if (ub->nr_queues_ready == ub->dev_info.nr_hw_queues) {
>  		/* now we are ready for handling ublk io request */
>  		ublk_reset_io_flags(ub);
>  		complete_all(&ub->completion);
> @@ -2878,12 +2876,12 @@ static int ublk_ctrl_start_dev(struct ublk_device *ub,
>  	ub->dev_info.ublksrv_pid = ublksrv_pid;
>  	ub->ub_disk = disk;
>  
>  	ublk_apply_params(ub);
>  
> -	/* don't probe partitions if any one ubq daemon is un-trusted */
> -	if (ub->nr_privileged_daemon != ub->nr_queues_ready)
> +	/* don't probe partitions if any daemon task is un-trusted */
> +	if (ub->unprivileged_daemons)
>  		set_bit(GD_SUPPRESS_PART_SCAN, &disk->state);
>  
>  	ublk_get_device(ub);
>  	ub->dev_info.state = UBLK_S_DEV_LIVE;
>  
> -- 
> 2.45.2
>

Re: [PATCH] ublk: check for unprivileged daemon on each I/O fetch

Posted by Caleb Sander Mateos 1 month, 3 weeks ago

On Fri, Aug 8, 2025 at 2:01 PM Uday Shankar <ushankar@purestorage.com> wrote:
>
> On Fri, Aug 08, 2025 at 09:52:15AM -0600, Caleb Sander Mateos wrote:
> > Commit ab03a61c6614 ("ublk: have a per-io daemon instead of a per-queue
> > daemon") allowed each ublk I/O to have an independent daemon task.
> > However, nr_privileged_daemon is only computed based on whether the last
> > I/O fetched in each ublk queue has an unprivileged daemon task.
> > Fix this by checking whether every fetched I/O's daemon is privileged.
> > Change nr_privileged_daemon from a count of queues to a boolean
> > indicating whether any I/Os have an unprivileged daemon.
> >
> > Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
> > Fixes: ab03a61c6614 ("ublk: have a per-io daemon instead of a per-queue daemon")
>
> Nice catch!
>
> > ---
> >  drivers/block/ublk_drv.c | 16 +++++++---------
> >  1 file changed, 7 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> > index 6561d2a561fa..a035070dd690 100644
> > --- a/drivers/block/ublk_drv.c
> > +++ b/drivers/block/ublk_drv.c
> > @@ -233,11 +233,11 @@ struct ublk_device {
> >
> >       struct ublk_params      params;
> >
> >       struct completion       completion;
> >       unsigned int            nr_queues_ready;
> > -     unsigned int            nr_privileged_daemon;
> > +     bool                    unprivileged_daemons;
> >       struct mutex cancel_mutex;
> >       bool canceling;
> >       pid_t   ublksrv_tgid;
> >  };
> >
> > @@ -1548,11 +1548,11 @@ static void ublk_reset_ch_dev(struct ublk_device *ub)
> >               ublk_queue_reinit(ub, ublk_get_queue(ub, i));
> >
> >       /* set to NULL, otherwise new tasks cannot mmap io_cmd_buf */
> >       ub->mm = NULL;
> >       ub->nr_queues_ready = 0;
> > -     ub->nr_privileged_daemon = 0;
> > +     ub->unprivileged_daemons = false;
> >       ub->ublksrv_tgid = -1;
> >  }
> >
> >  static struct gendisk *ublk_get_disk(struct ublk_device *ub)
> >  {
> > @@ -1978,16 +1978,14 @@ static void ublk_reset_io_flags(struct ublk_device *ub)
> >  /* device can only be started after all IOs are ready */
> >  static void ublk_mark_io_ready(struct ublk_device *ub, struct ublk_queue *ubq)
> >       __must_hold(&ub->mutex)
> >  {
> >       ubq->nr_io_ready++;
> > -     if (ublk_queue_ready(ubq)) {
> > +     if (ublk_queue_ready(ubq))
> >               ub->nr_queues_ready++;
> > -
> > -             if (capable(CAP_SYS_ADMIN))
> > -                     ub->nr_privileged_daemon++;
> > -     }
> > +     if (!ub->unprivileged_daemons && !capable(CAP_SYS_ADMIN))
> > +             ub->unprivileged_daemons = true;
>
> Shorter:
>
> ub->unprivileged_daemons |= !capable(CAP_SYS_ADMIN);

I was trying to avoid the capable() call if unprivileged_daemons was
already set. But maybe that's not a common case and it's not worth
optimizing?

Best,
Caleb

>
> >
> >       if (ub->nr_queues_ready == ub->dev_info.nr_hw_queues) {
> >               /* now we are ready for handling ublk io request */
> >               ublk_reset_io_flags(ub);
> >               complete_all(&ub->completion);
> > @@ -2878,12 +2876,12 @@ static int ublk_ctrl_start_dev(struct ublk_device *ub,
> >       ub->dev_info.ublksrv_pid = ublksrv_pid;
> >       ub->ub_disk = disk;
> >
> >       ublk_apply_params(ub);
> >
> > -     /* don't probe partitions if any one ubq daemon is un-trusted */
> > -     if (ub->nr_privileged_daemon != ub->nr_queues_ready)
> > +     /* don't probe partitions if any daemon task is un-trusted */
> > +     if (ub->unprivileged_daemons)
> >               set_bit(GD_SUPPRESS_PART_SCAN, &disk->state);
> >
> >       ublk_get_device(ub);
> >       ub->dev_info.state = UBLK_S_DEV_LIVE;
> >
> > --
> > 2.45.2
> >

Re: [PATCH] ublk: check for unprivileged daemon on each I/O fetch

Posted by Jens Axboe 1 month, 3 weeks ago

On 8/8/25 12:03 PM, Caleb Sander Mateos wrote:
> On Fri, Aug 8, 2025 at 2:01?PM Uday Shankar <ushankar@purestorage.com> wrote:
>>
>> On Fri, Aug 08, 2025 at 09:52:15AM -0600, Caleb Sander Mateos wrote:
>>> Commit ab03a61c6614 ("ublk: have a per-io daemon instead of a per-queue
>>> daemon") allowed each ublk I/O to have an independent daemon task.
>>> However, nr_privileged_daemon is only computed based on whether the last
>>> I/O fetched in each ublk queue has an unprivileged daemon task.
>>> Fix this by checking whether every fetched I/O's daemon is privileged.
>>> Change nr_privileged_daemon from a count of queues to a boolean
>>> indicating whether any I/Os have an unprivileged daemon.
>>>
>>> Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
>>> Fixes: ab03a61c6614 ("ublk: have a per-io daemon instead of a per-queue daemon")
>>
>> Nice catch!
>>
>>> ---
>>>  drivers/block/ublk_drv.c | 16 +++++++---------
>>>  1 file changed, 7 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
>>> index 6561d2a561fa..a035070dd690 100644
>>> --- a/drivers/block/ublk_drv.c
>>> +++ b/drivers/block/ublk_drv.c
>>> @@ -233,11 +233,11 @@ struct ublk_device {
>>>
>>>       struct ublk_params      params;
>>>
>>>       struct completion       completion;
>>>       unsigned int            nr_queues_ready;
>>> -     unsigned int            nr_privileged_daemon;
>>> +     bool                    unprivileged_daemons;
>>>       struct mutex cancel_mutex;
>>>       bool canceling;
>>>       pid_t   ublksrv_tgid;
>>>  };
>>>
>>> @@ -1548,11 +1548,11 @@ static void ublk_reset_ch_dev(struct ublk_device *ub)
>>>               ublk_queue_reinit(ub, ublk_get_queue(ub, i));
>>>
>>>       /* set to NULL, otherwise new tasks cannot mmap io_cmd_buf */
>>>       ub->mm = NULL;
>>>       ub->nr_queues_ready = 0;
>>> -     ub->nr_privileged_daemon = 0;
>>> +     ub->unprivileged_daemons = false;
>>>       ub->ublksrv_tgid = -1;
>>>  }
>>>
>>>  static struct gendisk *ublk_get_disk(struct ublk_device *ub)
>>>  {
>>> @@ -1978,16 +1978,14 @@ static void ublk_reset_io_flags(struct ublk_device *ub)
>>>  /* device can only be started after all IOs are ready */
>>>  static void ublk_mark_io_ready(struct ublk_device *ub, struct ublk_queue *ubq)
>>>       __must_hold(&ub->mutex)
>>>  {
>>>       ubq->nr_io_ready++;
>>> -     if (ublk_queue_ready(ubq)) {
>>> +     if (ublk_queue_ready(ubq))
>>>               ub->nr_queues_ready++;
>>> -
>>> -             if (capable(CAP_SYS_ADMIN))
>>> -                     ub->nr_privileged_daemon++;
>>> -     }
>>> +     if (!ub->unprivileged_daemons && !capable(CAP_SYS_ADMIN))
>>> +             ub->unprivileged_daemons = true;
>>
>> Shorter:
>>
>> ub->unprivileged_daemons |= !capable(CAP_SYS_ADMIN);
> 
> I was trying to avoid the capable() call if unprivileged_daemons was
> already set. But maybe that's not a common case and it's not worth
> optimizing?

Definitely worth it, you did the right thing.

-- 
Jens Axboe