Re: [PATCH v10 06/10] block: implement NVMEM provider

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Registering an NVMEM device for a partition deadlocks on `disk->open_mutex` if the NVMEM layout driver reads data during probe.
- [High] ABBA deadlock between `disk->open_mutex` and `kernfs` active reference during NVMEM sysfs read and partition deletion.
- [High] Using `fput()` instead of `bdev_fput()` defers yielding the block device's exclusive claim, causing spurious `-EBUSY` errors.
--

--- Patch [6]: [PATCH v10 06/10] block: implement NVMEM provider ---
commit d9c54a3ae2929addc2c718f29f9c8b9fa41ca7c3
Author: Loic Poulain <[email protected]>

block: implement NVMEM provider
    
On embedded devices using an eMMC it is common that one or more partitions
on the eMMC are used to store MAC addresses and Wi-Fi calibration EEPROM
data. Allow referencing the partition in device tree for the kernel and
Wi-Fi drivers accessing it via the NVMEM layer.

> diff --git a/block/blk-nvmem.c b/block/blk-nvmem.c
> new file mode 100644
> --- /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)
> +{
[ ... ]
> +	if (file_bdev(bdev_file) != bdev) {
> +		fput(bdev_file);
> +		return -ENODEV;
> +	}
> +
> +	ret = kernel_read(bdev_file, val, bytes, &pos);
> +	fput(bdev_file);

[Severity: High]
Since this opens the block device with BLK_OPEN_EXCL, will using fput()
instead of bdev_fput() defer yielding the exclusive claim?

Calling fput() defers the file release callback to an asynchronous task_work
or delayed_fput workqueue. Could this cause other system services like mount
or blkid to spuriously fail with -EBUSY while waiting for the deferred
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);

[Severity: High]
Is there a potential ABBA deadlock here between disk->open_mutex and the
kernfs active reference?

If a user reads the NVMEM sysfs attribute, the read acquires a kernfs active
reference and calls blk_nvmem_reg_read(), which then blocks waiting on
disk->open_mutex.

Concurrently, if the partition is being removed, the teardown path in
__del_gendisk() holds disk->open_mutex and calls drop_partition(). This
calls blk_nvmem_del() and unregisters the NVMEM device. Device
unregistration removes sysfs attributes and invokes kernfs_drain(), which
will wait indefinitely for the kernfs active reference held by the read.

>  	device_del(&part->bd_device);
>  	put_device(&part->bd_device);
>  }
[ ... ]
> @@ -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]
Will this cause a recursive deadlock on disk->open_mutex if the NVMEM
layout driver reads data during probe?

The add_partition() function holds disk->open_mutex while executing. If the
partition has an nvmem-layout, nvmem_register() will synchronously probe
the layout driver.

If the layout driver reads the partition data via nvmem_device_read(), it
triggers blk_nvmem_reg_read(). This function calls bdev_file_open_by_dev(),
which attempts to acquire the exact same disk->open_mutex.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6