Re: [PATCH v2 1/4] ublk: cleanup partition flags handling and introduce UBLK_F_NO_PARTITONS

[Ming Lei <ming.lei@redhat.com>]

On Sat, Feb 28, 2026 at 07:30:06AM +0200, Alexander Atanasov wrote:
> 
> 
> > On 28 Feb 2026, at 4:06, Ming Lei <ming.lei@redhat.com> wrote:
> > 
> > On Fri, Feb 27, 2026 at 10:29:56AM -0800, Caleb Sander Mateos wrote:
> >> On Fri, Feb 27, 2026 at 7:07 AM Alexander Atanasov <alex@zazolabs.com> wrote:
> >>> 
> >>> Currently the code misuse GD_SUPPRESS_PART_SCAN flag
> >>> as it tries to use it as a switch for the auto partition scan.
> >>> To fully disable creation of partitions GENHD_FL_NO_PART must
> >>> be used, switch code to use it instead GD_SUPPRESS_PART_SCAN.
> >>> 
> >>> Rules for partitions become:
> >>> - Unprivileged daemons - never scan partitions, they have
> >>> GENHD_FL_NO_PART always set - they are devices without partitions.
> >>> 
> >>> - Trusted daemons - by default have partitions enabled,
> >>> automatic initial scan can be disabled via UBLK_F_NO_AUTO_PART_SCAN flag.
> >>> Partitions can be disabled via UBLK_F_NO_PARTITIONS flag.
> >>> 
> >>> Rework the code to work as described above:
> >>> - remove checks in ublk_partition_scan_work and rely on
> >>> the caller to schedule the work only if it has to be done.
> >>> - set GENHD_FL_NO_PART on unprivileged devices
> >>> - set GENHD_FL_NO_PART depending on UBLK_F_NO_PARTITIONS on trusted
> >>> devices.
> >>> 
> >>> Fixes: 8443e2087e70 ("ublk: add UBLK_F_NO_AUTO_PART_SCAN feature flag")
> >>> Signed-off-by: Alexander Atanasov <alex@zazolabs.com>
> >>> ---
> >>> drivers/block/ublk_drv.c      | 23 +++++++++--------------
> >>> include/uapi/linux/ublk_cmd.h |  3 +++
> >>> 2 files changed, 12 insertions(+), 14 deletions(-)
> >>> 
> >>> -v1->v2:
> >>>  - remove code duplication and extra flag settings
> >>>  - switch from out-in to out-out to preserve backwards compatibility
> >>> 
> >>> 
> >>> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> >>> index 3c918db4905c..36da63ba2a57 100644
> >>> --- a/drivers/block/ublk_drv.c
> >>> +++ b/drivers/block/ublk_drv.c
> >>> @@ -81,6 +81,7 @@
> >>>                | (IS_ENABLED(CONFIG_BLK_DEV_INTEGRITY) ? UBLK_F_INTEGRITY : 0) \
> >>>                | UBLK_F_SAFE_STOP_DEV \
> >>>                | UBLK_F_BATCH_IO \
> >>> +               | UBLK_F_NO_PARTITIONS \
> >>>                | UBLK_F_NO_AUTO_PART_SCAN)
> >>> 
> >>> #define UBLK_F_ALL_RECOVERY_FLAGS (UBLK_F_USER_RECOVERY \
> >>> @@ -2361,14 +2362,9 @@ static void ublk_partition_scan_work(struct work_struct *work)
> >>>        if (!disk)
> >>>                return;
> >>> 
> >>> -       if (WARN_ON_ONCE(!test_and_clear_bit(GD_SUPPRESS_PART_SCAN,
> >>> -                                            &disk->state)))
> >>> -               goto out;
> >>> -
> >>>        mutex_lock(&disk->open_mutex);
> >>>        bdev_disk_changed(disk, false);
> >>>        mutex_unlock(&disk->open_mutex);
> >>> -out:
> >>>        ublk_put_disk(disk);
> >>> }
> >>> 
> >>> @@ -4409,10 +4405,10 @@ static int ublk_ctrl_start_dev(struct ublk_device *ub,
> >>>         * wait while holding ub->mutex, which can deadlock with other
> >>>         * operations that need the mutex. Defer partition scan to async
> >>>         * work.
> >>> -        * For unprivileged daemons, keep GD_SUPPRESS_PART_SCAN set
> >>> -        * permanently.
> >>> +        * For unprivileged daemons, set GENHD_FL_NO_PART to
> >>> +        * disable partitions.
> >>>         */
> >>> -       set_bit(GD_SUPPRESS_PART_SCAN, &disk->state);
> >>> +       disk->flags |= GENHD_FL_NO_PART;
> >>> 
> >>>        ublk_get_device(ub);
> >>>        ub->dev_info.state = UBLK_S_DEV_LIVE;
> >>> @@ -4429,12 +4425,11 @@ static int ublk_ctrl_start_dev(struct ublk_device *ub,
> >>> 
> >>>        set_bit(UB_STATE_USED, &ub->state);
> >>> 
> >>> -       /* Skip partition scan if disabled by user */
> >>> -       if (ub->dev_info.flags & UBLK_F_NO_AUTO_PART_SCAN) {
> >>> -               clear_bit(GD_SUPPRESS_PART_SCAN, &disk->state);
> >>> -       } else {
> >>> -               /* Schedule async partition scan for trusted daemons */
> >>> -               if (!ub->unprivileged_daemons)
> >>> +       if (!ub->unprivileged_daemons && !(ub->dev_info.flags & UBLK_F_NO_PARTITIONS)) {
> >>> +               /* Enable partitions on device */
> >>> +               disk->flags &= ~GENHD_FL_NO_PART;
> >> 
> >> Is it safe to modify disk->flags after the disk has been added? This looks racy.
> > 
> > Yeah, GENHD_FL_NO_PART should cover unprivileged_daemons & UBLK_F_NO_PARTITIONS only
> > in static way.
> 
> 
> Once set disk->flags are not modified anywhere. Where modifications are required then there is a disk->state.
> No one inside the driver can touch the disk since the mutex is held.
> Outside the driver flags are only checked.
> add_disk(..) does not start any asynchronous operations, that’s the reason there is a work queue.
> So where do you see a race ?

The block layer has a clear two-tier design for partition control:

1. disk->flags (e.g. GENHD_FL_NO_PART) — set before add_disk(), meant to be
static/permanent. These are regular integers, not atomic.

2. disk->state (e.g. GD_SUPPRESS_PART_SCAN) — atomic bit operations
(set_bit/clear_bit/test_and_clear_bit), designed for runtime toggling.

This distinction is intentional — disk->flags uses non-atomic read-modify-write (&=, |=),
so concurrent modification is genuinely unsafe. Similar change may not even
compile in memory safety language, such as Rust.

Also the ublk mutex can't protect concurrent partition scanning from ioctl(BLKRRPART).

BTW, the patch title has typo in `UBLK_F_NO_PARTITONS`.

Thanks, 
Ming