Re: [PATCH 39/58] mtd: rawnand: qcom: retain open-handle state on removal

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] mtd: rawnand: qcom: broken error handling in
  qcom_probe_nand_devices()
--

commit 0a9084cf5fec792ff4833fe6c2d5c8f9780c60a1
Author: James Hilliard <james.hilliard1@gmail.com>

mtd: rawnand: qcom: retain open-handle state on removal

This commit ties the lifetime of qcom_nand_host and qcom_nand_controller to
the MTD and NAND controller release callbacks, validates boot partitions
before registering MTD devices, and adds cleanup of registered chips on
probe failure and removal.

> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
> index 2d98f79b492d6..f812de1d776fc 100644
> --- a/drivers/mtd/nand/raw/qcom_nandc.c
> +++ b/drivers/mtd/nand/raw/qcom_nandc.c

[ ... ]

> @@ -2077,15 +2091,13 @@ static int qcom_probe_nand_devices(struct qcom_nand_controller *nandc)
>  	int ret = -ENODEV;
>  
>  	for_each_available_child_of_node_scoped(dn, child) {
> -		host = devm_kzalloc(dev, sizeof(*host), GFP_KERNEL);
> +		host = kzalloc_obj(*host);
>  		if (!host)
>  			return -ENOMEM;
>  
>  		ret = qcom_nand_host_init_and_register(nandc, host, child);
> -		if (ret) {
> -			devm_kfree(dev, host);
> +		if (ret)
>  			continue;
> -		}
>  
>  		list_add_tail(&host->node, &nandc->host_list);
>  	}

[Severity: Medium]
Should qcom_probe_nand_devices() return ret instead of continuing when
qcom_nand_host_init_and_register() fails?

Continuing the loop means that if an earlier child node fails (for example
with -EPROBE_DEFER or invalid boot partitions) and a later child succeeds,
ret is overwritten with 0 and the partial probe is not unwound via
err_setup.

Worse, if the earlier child fails in qcom_nand_attach_chip() when
qcom_alloc_bam_transaction() returns NULL after qcom_free_bam_transaction()
freed nandc->bam_txn:

drivers/mtd/nand/raw/qcom_nandc.c:qcom_nand_attach_chip() {
    ...
	if (nandc->props->supports_bam)
		qcom_free_bam_transaction(nandc);

	nandc->max_cwperpage = max_t(unsigned int, nandc->max_cwperpage,
				     cwperpage);

	/* Now allocate the BAM transaction based on updated max_cwperpage */
	if (nandc->props->supports_bam) {
		nandc->bam_txn = qcom_alloc_bam_transaction(nandc);
		if (!nandc->bam_txn) {
			dev_err(nandc->dev,
				"failed to allocate bam transaction\n");
			return -ENOMEM;
		}
	}
    ...
}

continuing to the next child node calls nand_scan(), which runs
qcom_clear_bam_transaction() and dereferences nandc->bam_txn while it is
NULL.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107-submit-rawnand-retained-handle-lifetime-v1-v1-0-0181ef0cc4c8@gmail.com?part=39