Re: [PATCH v4 4/8] block: implement NVMEM provider

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Assigning the 64-bit block device size to the `int` typed `config.size` field causes an integer overflow for partitions between 2GB and 4GB, disabling NVMEM bounds checking.
- [High] Opening the block device per-read with `BLK_OPEN_RESTRICT_WRITES` and closing it via `__free(fput)` causes unpredictable `-EBUSY` failures for concurrent writers and floods system workqueues.
- [High] Using `devm_nvmem_register()` on a driverless block device creates an unresolvable reference cycle, permanently leaking both the block device and the NVMEM device on teardown.
- [High] The NVMEM provider stores `devt` as private data, leading to a silent Wrong Device Access if the original block device is removed and its `devt` is recycled.
--

commit 14c60083ba6e54d749e04b67cf95e61484f8571f
Author: Daniel Golle <daniel@makrotopia.org>

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
> index 0000000000000..a6e62fa98675e
> --- /dev/null
> +++ b/block/blk-nvmem.c
[ ... ]
> +static int blk_nvmem_reg_read(void *priv, unsigned int from,
> +			      void *val, size_t bytes)
> +{
> +	blk_mode_t mode = BLK_OPEN_READ | BLK_OPEN_RESTRICT_WRITES;
> +	dev_t devt = (dev_t)(uintptr_t)priv;
> +	size_t bytes_left = bytes;
> +	loff_t pos = from;
> +	int ret = 0;
> +
> +	struct file *bdev_file __free(fput) = bdev_file_open_by_dev(devt, mode, priv, NULL);

[Severity: High]
Does releasing the block device file via standard fput() instead of bdev_fput()
cause unpredictable -EBUSY failures for concurrent writers?

Since blk_nvmem_reg_read() opens the block device with
BLK_OPEN_RESTRICT_WRITES, it exclusively blocks writers. The standard
__free(fput) defers the release asynchronously to a workqueue.

Because bdev_fput() is bypassed, the write restriction isn't synchronously
yielded, which can spuriously block other system utilities or writers with
-EBUSY. Furthermore, if a driver reads NVMEM data in small chunks, will this
instantiate and defer thousands of file closures, heavily loading system
workqueues?

> +	if (IS_ERR(bdev_file))
> +		return PTR_ERR(bdev_file);
[ ... ]
> +static int blk_nvmem_register(struct device *dev)
> +{
> +	struct block_device *bdev = dev_to_bdev(dev);
> +	struct nvmem_config config = {};
> +
> +	/* skip devices which do not have a device tree node */
> +	if (!dev_of_node(dev))
> +		return 0;
> +
> +	/* skip devices without an nvmem layout defined */
> +	struct device_node *child __free(device_node) =
> +		of_get_child_by_name(dev_of_node(dev), "nvmem-layout");
> +	if (!child)
> +		return 0;
> +
> +	/*
> +	 * skip block device too large to be represented as NVMEM devices,
> +	 * the NVMEM reg_read callback uses an unsigned int offset
> +	 */
> +	if (bdev_nr_bytes(bdev) > UINT_MAX) {
> +		dev_warn(dev, "block device too large to be an NVMEM provider\n");
> +		return -ENODEV;
> +	}
> +
> +	config.id = NVMEM_DEVID_NONE;
> +	config.dev = dev;
> +	config.name = dev_name(dev);
> +	config.owner = THIS_MODULE;
> +	config.priv = (void *)(uintptr_t)dev->devt;

[Severity: High]
Can using a dynamically recycled device number lead to reading from the wrong
device?

blk_nvmem_register() stores the block device's devt in config.priv, which
is then passed to blk_nvmem_reg_read() to open the device. If the original
block device is removed, its devt is freed and can be reallocated to a newly
plugged block device.

If a kernel consumer still holds a reference to the nvmem_device and performs
a read, will bdev_file_open_by_dev() dynamically resolve the recycled devt
and successfully open the new device, potentially exposing arbitrary data
to the driver?

> +	config.reg_read = blk_nvmem_reg_read;
> +	config.size = bdev_nr_bytes(bdev);

[Severity: High]
Is it possible for the assignment of bdev_nr_bytes() to config.size to cause
an integer overflow that bypasses NVMEM bounds checking?

bdev_nr_bytes() returns a 64-bit loff_t, and the limit check in
blk_nvmem_register() allows block devices up to UINT_MAX. However, the size
field in struct nvmem_config is defined as a signed 32-bit int.

If the block device size is between 2GB and 4GB-1, will assigning it to
config.size result in a negative value (e.g., -1073741824)? When the NVMEM
core later copies this into nvmem->size (which is a 64-bit size_t), this
negative value sign-extends to a massive positive value, potentially allowing
consumers to read far past the end of the block device.

> +	config.word_size = 1;
> +	config.stride = 1;
> +	config.read_only = true;
> +	config.root_only = true;
> +	config.ignore_wp = true;
> +	config.of_node = to_of_node(dev->fwnode);
> +
> +	return PTR_ERR_OR_ZERO(devm_nvmem_register(dev, &config));
> +}
> +
> +static struct class_interface blk_nvmem_bus_interface __refdata = {
> +	.class = &block_class,
> +	.add_dev = &blk_nvmem_register,
> +};

[Severity: High]
Will using devm_nvmem_register() on a driverless block device create a
reference cycle that leaks both devices?

Block devices are driverless class devices, so devres_release_all() is only
invoked in device_release() when the device's reference count reaches zero.
However, nvmem_register() creates a child nvmem_device that takes a reference
on its parent block device.

Since blk_nvmem_bus_interface lacks a .remove_dev callback, the NVMEM device
is never manually unregistered when the block device is removed. Does this mean
the NVMEM device perpetually holds its reference to the parent block device,
preventing the parent's reference count from reaching zero and causing an
indefinite memory leak of both devices?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609-block-as-nvmem-v4-0-45712e6b22c6@oss.qualcomm.com?part=4