Re: [PATCH v12 2/2] block: implement NVMEM provider

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] block: self-deadlock in add_partition() when blk_nvmem_add() probes layout
- [High] block: ABBA deadlock between disk->open_mutex and nvmem->srcu in drop_partition()
- [Medium] block: blk_nvmem_reg_read() uses fput() instead of bdev_fput() on exclusive bdev
--

commit 348ca9207d847db1f952272a2d1ea367e90cb1ca
Author: Loic Poulain <loic.poulain@oss.qualcomm.com>

block: implement NVMEM provider

This commit implements a read-only NVMEM provider for block devices and
partitions whose device tree node describes an NVMEM layout. Each NVMEM
read opens the block device with BLK_OPEN_EXCL and reads from it using
kernel_read().

> diff --git a/block/blk-nvmem.c b/block/blk-nvmem.c
> new file mode 100644
> index 0000000000000..69b69184e1630
> --- /dev/null
> +++ b/block/blk-nvmem.c
> @@ -0,0 +1,112 @@

[ ... ]

> +static int blk_nvmem_reg_read(void *priv, unsigned int from,
> +			      void *val, size_t bytes)
> +{
> +	struct block_device *bdev = priv;
> +	struct file *bdev_file;
> +	loff_t pos = from;
> +	ssize_t ret;
> +
> +	/* open and prevent other exclusive openers */
> +	bdev_file = bdev_file_open_by_dev(bdev->bd_dev,
> +					  BLK_OPEN_READ | BLK_OPEN_EXCL,
> +					  blk_nvmem_reg_read, NULL);
> +	if (IS_ERR(bdev_file))
> +		return PTR_ERR(bdev_file);
> +
> +	if (file_bdev(bdev_file) != bdev) {
> +		fput(bdev_file);
> +		return -ENODEV;
> +	}
> +
> +	ret = kernel_read(bdev_file, val, bytes, &pos);
> +	fput(bdev_file);

[Severity: Medium]
As reported by Sashiko on v5 [1], does calling fput(bdev_file) here and on
the file_bdev(bdev_file) != bdev error path defer releasing the exclusive
claim on bdev past the return of blk_nvmem_reg_read()?

bdev_file_open_by_dev() is called with BLK_OPEN_EXCL and blk_nvmem_reg_read
as the holder, which claims bdev and the whole disk via bd_finish_claiming().
However, fput() defers __fput() -> bdev_release() to task_work (for user
threads) or delayed_fput_work (for kernel threads such as deferred probe
workers).

Until that deferred work runs, bdev_yield_claim() has not been called and
bdev->bd_holder remains set. If another consumer or userspace process tries
to mount or exclusively open the block device (or read another NVMEM cell on
the same disk) immediately after blk_nvmem_reg_read() returns, bd_may_claim()
can fail with -EBUSY.

Should blk_nvmem_reg_read() call bdev_fput(bdev_file) instead of
fput(bdev_file) so that bdev_yield_claim() releases the holder claim
synchronously before deferring the file release?

[ ... ]

> diff --git a/block/partitions/core.c b/block/partitions/core.c
> index eee954df1ac6e..c83227710584a 100644
> --- a/block/partitions/core.c
> +++ b/block/partitions/core.c
> @@ -289,6 +289,8 @@ void drop_partition(struct block_device *part)
>  	xa_erase(&part->bd_disk->part_tbl, bdev_partno(part));
>  	kobject_put(part->bd_holder_dir);
>  
> +	blk_nvmem_del(part);
> +
>  	device_del(&part->bd_device);
>  	put_device(&part->bd_device);
>  }

[Severity: High]
As reported by Sashiko on v11 [2], can calling blk_nvmem_del(part) inside
drop_partition() cause an AB-BA deadlock between disk->open_mutex and
nvmem->srcu?

On the NVMEM read path, __nvmem_reg_read() holds srcu_read_lock(&nvmem->srcu)
(and on sysfs reads, kernfs holds the active reference on the nvmem binary
attribute) and calls:

  __nvmem_reg_read()
    blk_nvmem_reg_read()
      bdev_file_open_by_dev()
        bdev_open()
          bd_prepare_to_claim()
          mutex_lock(&disk->open_mutex)

On the partition removal path (__del_gendisk(), bdev_del_partition(), or
bdev_disk_changed()), drop_partition() is called with disk->open_mutex held
(and in __del_gendisk(), blk_freeze_acquire_lock(q) is also held; in
bdev_disk_changed(), whole->bd_claiming is also held):

  drop_partition()
    blk_nvmem_del()
      nvmem_unregister()
        synchronize_srcu(&nvmem->srcu)
        device_unregister(&nvmem->dev)

If a reader enters __nvmem_reg_read() and looks up the bdev in
blkdev_get_no_open() just before drop_partition() runs, the reader will block
waiting for whole->bd_claiming or disk->open_mutex in bdev_open() while
holding nvmem->srcu, while drop_partition() holds disk->open_mutex and waits
in synchronize_srcu(&nvmem->srcu) for the reader to finish.

> @@ -406,6 +408,9 @@ static struct block_device *add_partition(struct gendisk *disk, int partno,
>  	/* suppress uevent if the disk suppresses it */
>  	if (!dev_get_uevent_suppress(ddev))
>  		kobject_uevent(&pdev->kobj, KOBJ_ADD);
> +
> +	blk_nvmem_add(bdev);

[Severity: High]
As reported by Sashiko on v11 [3], can calling blk_nvmem_add(bdev) from
add_partition() self-deadlock when the partition uses an nvmem-layout driver
that reads from the NVMEM device during registration?

add_partition() is called with disk->open_mutex held:

  lockdep_assert_held(&disk->open_mutex);

When called from add_disk_final() -> disk_scan_partitions(), the current
task has also claimed disk->part0 via bd_prepare_to_claim(disk->part0,
disk_scan_partitions, NULL), setting whole->bd_claiming.

Inside add_partition(), blk_nvmem_add() calls nvmem_register(), which
synchronously populates the layout child device and fires NVMEM_ADD
notifiers:

  add_partition()
    blk_nvmem_add()
      nvmem_register()
        nvmem_populate_layout()
          device_add()
            ...
              layout->add_cells()
                nvmem_device_read()
                  __nvmem_reg_read()
                    blk_nvmem_reg_read()
                      bdev_file_open_by_dev()
                        bdev_open()
                          bd_prepare_to_claim()
                          mutex_lock(&disk->open_mutex)

When a layout driver (such as u-boot-env, onie-tlv-layout, or
kontron-sl28vpd-layout) or an NVMEM_ADD notifier reads from the NVMEM device
during probe, bdev_open() will either sleep forever in bd_prepare_to_claim()
waiting on whole->bd_claiming owned by the same thread in
disk_scan_partitions(), or deadlock on mutex_lock(&disk->open_mutex) already
held by add_partition().

[1] https://lore.kernel.org/r/20260613132137.D65B51F000E9@smtp.kernel.org
[2] https://lore.kernel.org/r/20261015-block-as-nvmem-v11-3-f890ac74f87d@oss.qualcomm.com
[3] https://lore.kernel.org/r/20261015-block-as-nvmem-v11-3-f890ac74f87d@oss.qualcomm.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-block-as-nvmem-v12-0-f4a2a681087d@oss.qualcomm.com?part=2