Re: [PATCH v5 5/9] block: implement NVMEM provider

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] NVMEM provider opens the block device with exclusive write restriction, causing conflicts on mounted systems.
- [High] Hardcoded 4GB size limit incorrectly rejects modern eMMC devices.
- [High] Integer overflow in size assignment exposes massive out-of-bounds size.
- [High] Stale `dev_t` reference can cause data leaks across block device hotplugs (ABA problem).
- [High] Using `__free(fput)` defers block device claim release, creating race conditions.
--

commit 459b659c960efdc8a9e2177c5bc4f832e7ee5730
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..c005f059d9fe5
> --- /dev/null
> +++ b/block/blk-nvmem.c
> @@ -0,0 +1,109 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
[ ... ]
> +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;

[Severity: High]
Since the NVMEM provider claims the whole disk (disk->part0), will opening it
with BLK_OPEN_RESTRICT_WRITES and an exclusive holder (priv) return -EBUSY if
any partition on the disk is already mounted or opened for writing?

> +	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 using __free(fput) defer the release of the block device claim? Because
fput() schedules the file release asynchronously, could another subsystem
trying to mount or claim the device immediately after an NVMEM read
sporadically fail with -EBUSY?

Should this use bdev_fput() instead to synchronously yield the claim before
the rest of the file release?

> +	if (IS_ERR(bdev_file))
> +		return PTR_ERR(bdev_file);
[ ... ]
> +void blk_nvmem_add(struct block_device *bdev)
> +{
[ ... ]
> +	/*
> +	 * 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) {

[Severity: High]
Since the NVMEM provider is registered against the whole disk rather than the
individual NVMEM partition, does this 4GB limit incorrectly reject most modern
eMMC devices?

> +		dev_warn(dev, "block device too large to be an NVMEM provider\n");
> +		return;
> +	}
> +
> +	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 storing the ephemeral dev_t identifier without holding a pinned block
device reference cause an ABA problem?

If the block device is removed, blk_nvmem_del() calls nvmem_unregister()
which drops a reference but does not synchronously wait for consumers to
close. If a new device is plugged in and reuses the same dev_t, could a
consumer reading from the still-alive NVMEM device inadvertently access the
unrelated new block device?

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

[Severity: High]
Can this assignment cause an integer overflow?

The config.size field in struct nvmem_config is a signed 32-bit int, so disk
sizes between 2GB and 4GB will overflow to a negative value. When
nvmem_register() sign-extends this into its 64-bit size_t field, won't it
result in a massive out-of-bounds size being exposed via sysfs?

> +	config.word_size = 1;
> +	config.stride = 1;
> +	config.read_only = true;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612-block-as-nvmem-v5-0-95e0b30fff90@oss.qualcomm.com?part=5