Re: [PATCH v3 03/58] mtd: separate class devices from retained backing objects

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] mtd: vmlinux link failure when CONFIG_MTD=m due to obj-y
  subdirs under CONFIG_MTD_DEVICE=y
- [Medium] mtd: mtd_otp_nvmem_register() falls back to parent driver
  owner when mtd->owner is NULL
- [Medium] mtd: NULL mtd->dev dereference in __mtd_del_partition() on
  race
--

commit 9d436aa88bcc3bc57c2e1b8cfecc83b868e1eda7
Author: James Hilliard <james.hilliard1@gmail.com>

mtd: separate class devices from retained backing objects

This commit allocates MTD class devices separately from struct mtd_info,
places the class and device release handling into a built-in component, and
adds reference-counted backing lifetime management via mtd_set_release() and
mtd_put_master().

> diff --git a/drivers/Makefile b/drivers/Makefile
> index 0841ea851847e..86adb803c79a4 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -84,7 +84,7 @@ obj-y				+= scsi/
>  obj-y				+= nvme/
>  obj-$(CONFIG_ATA)		+= ata/
>  obj-$(CONFIG_TARGET_CORE)	+= target/
> -obj-$(CONFIG_MTD)		+= mtd/
> +obj-$(CONFIG_MTD_DEVICE)		+= mtd/
>  obj-$(CONFIG_SPI)		+= spi/
>  obj-$(CONFIG_SPMI)		+= spmi/
>  obj-$(CONFIG_HSI)		+= hsi/

[ ... ]

> diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
> index 0421c6208de77..d13a065874bad 100644
> --- a/drivers/mtd/Kconfig
> +++ b/drivers/mtd/Kconfig
> @@ -1,5 +1,9 @@
> +config MTD_DEVICE
> +	bool
> +
>  menuconfig MTD
>  	tristate "Memory Technology Device (MTD) support"
> +	select MTD_DEVICE
>  	imply NVMEM
>  	help
>  	  Memory Technology Devices are flash, RAM and similar chips, often
> diff --git a/drivers/mtd/Makefile b/drivers/mtd/Makefile
> index 7b6dd53e81504..8bb19a6da358a 100644
> --- a/drivers/mtd/Makefile
> +++ b/drivers/mtd/Makefile
> @@ -4,6 +4,7 @@
>  #
>  
>  # Core functionality.
> +obj-$(CONFIG_MTD_DEVICE)		+= mtddevice.o
>  obj-$(CONFIG_MTD)		+= mtd.o
>  mtd-y				:= mtdcore.o mtdsuper.o mtdconcat.o mtdpart.o mtdchar.o
>  mtd-$(CONFIG_MTD_VIRT_CONCAT)	+= mtd_virt_concat.o

[Severity: High]
Does entering drivers/mtd/ via bool CONFIG_MTD_DEVICE break linking vmlinux
when CONFIG_MTD=m?

With CONFIG_MTD_DEVICE=y, Kbuild descends into drivers/mtd/ for built-in
objects and follows the unconditional obj-y subdirectories in
drivers/mtd/Makefile:

drivers/mtd/Makefile:
	obj-y				+= parsers/
	...
	obj-y		+= chips/ lpddr/ maps/ devices/ nand/ tests/

Any bool Kconfig option inside those subdirectories (such as
CONFIG_MTD_BCM63XX_PARTS=y under allmodconfig or CONFIG_MTD_NAND_XWAY=y) is
then compiled into vmlinux while referencing symbols exported by modular
mtd.ko or nand.ko, failing the vmlinux link.  Gating those subdirectory
assignments on obj-$(CONFIG_MTD) instead of obj-y would avoid built-in
traversal when CONFIG_MTD=m.

[ ... ]

> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 620326279957f..5793c865f3a20 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c

[ ... ]

> @@ -901,13 +949,13 @@ int del_mtd_device(struct mtd_info *mtd)
>   * Set a few defaults based on the parent devices, if not provided by the
>   * driver
>   */
> -static void mtd_set_dev_defaults(struct mtd_info *mtd)
> +void mtd_set_dev_defaults(struct mtd_info *mtd)
>  {
> -	if (mtd->dev.parent) {
> -		if (!mtd->owner && mtd->dev.parent->driver)
> -			mtd->owner = mtd->dev.parent->driver->owner;
> +	if (mtd->parent_dev) {
> +		if (!mtd->master.release && !mtd->owner && mtd->parent_dev->driver)
> +			mtd->owner = mtd->parent_dev->driver->owner;
>  		if (!mtd->name)
> -			mtd->name = dev_name(mtd->dev.parent);
> +			mtd->name = dev_name(mtd->parent_dev);
>  	} else {
>  		pr_debug("mtd device won't show a device symlink in sysfs\n");
>  	}
> @@ -960,13 +1008,13 @@ static struct nvmem_device *mtd_otp_nvmem_register(struct mtd_info *mtd,
>  	struct device_node *np;
>  
>  	/* DT binding is optional */
> -	np = of_get_compatible_child(mtd->dev.of_node, compatible);
> +	np = of_get_compatible_child(mtd_get_of_node(mtd), compatible);
>  
>  	/* OTP nvmem will be registered on the physical device */
> -	config.dev = mtd->dev.parent;
> +	config.dev = mtd->parent_dev;
>  	config.name = compatible;
>  	config.id = NVMEM_DEVID_AUTO;
> -	config.owner = THIS_MODULE;
> +	config.owner = mtd->owner;
>  	config.add_legacy_fixed_of_cells = !mtd_type_is_nand(mtd);
>  	config.type = NVMEM_TYPE_OTP;
>  	config.root_only = true;

[Severity: Medium]
When a master uses mtd_set_release() and its operations core is built-in,
mtd_set_dev_defaults() leaves mtd->owner as NULL, so does passing
config.owner = mtd->owner with config.dev = mtd->parent_dev cause
nvmem_register() to fall back to the controller driver's module?

drivers/nvmem/core.c:nvmem_register() {
    ...
	nvmem->owner = config->owner;
	if (!nvmem->owner && config->dev->driver)
		nvmem->owner = config->dev->driver->owner;
    ...
}

Because config.dev is mtd->parent_dev, nvmem_register() sets nvmem->owner to
mtd->parent_dev->driver->owner when mtd->owner is NULL, pinning the
controller driver module whenever an OTP NVMEM device or cell is held.

[ ... ]

> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> index c316cfe92c7e4..eb7704a5d7141 100644
> --- a/drivers/mtd/mtdpart.c
> +++ b/drivers/mtd/mtdpart.c

[ ... ]

> @@ -329,7 +329,7 @@ static int __mtd_del_partition(struct mtd_info *mtd)
>  			return err;
>  	}
>  
> -	sysfs_remove_files(&mtd->dev.kobj, mtd_partition_attrs);
> +	sysfs_remove_files(&mtd->dev->kobj, mtd_partition_attrs);
>  
>  	list_del_init(&mtd->part.node);
>  	err = del_mtd_device(mtd);

[Severity: Medium]
Can __mtd_del_partition() dereference a NULL mtd->dev if BLKPG_DEL_PARTITION
races with add_mtd_partitions()?

Previously mtd->dev was an embedded struct device, so &mtd->dev.kobj was
valid even before registration.  Now child->dev is NULL until
add_mtd_device() allocates it, but add_mtd_partitions() links child onto
parent->partitions and drops master->master.partitions_lock before calling
add_mtd_device():

CPU0                                    CPU1
add_mtd_partitions()
  mutex_lock(&partitions_lock)
  list_add_tail(&child->part.node,
                &parent->partitions)
  mutex_unlock(&partitions_lock)
                                        mtd_del_partition()
                                          mutex_lock(&partitions_lock)
                                          __mtd_del_partition(child)
                                            sysfs_remove_files(
                                              &child->dev->kobj, ...)
                                              <-- NULL dereference
  add_mtd_device(child)

When CONFIG_MTD_PARTITIONED_MASTER=y (or when parsing subpartitions of an
already-registered partition), the parent is already accessible to
BLKPG_DEL_PARTITION ioctls while child->dev is still NULL.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111-submit-rawnand-retained-handle-lifetime-v1-v3-0-ce537999741d@gmail.com?part=3

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/