Re: [PATCH 04/58] mtd: rawnand: separate hardware disconnection from cleanup

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] mtd: rawnand: unlocked hardware access after nand_disconnect()
  in write_bbt()
- [Medium] mtd: rawnand: nand_suspend() returning -ENODEV aborts system
  suspend
--

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

mtd: rawnand: separate hardware disconnection from cleanup

This commit adds nand_disconnect() to mark a NAND chip as disconnected
under the chip and controller locks and makes nand_get_device(),
nand_suspend(), nand_resume(), and panic_nand_write() reject hardware
access once disconnected.

> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index a5b278ab9384f..8f7d2571e8d9a 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c

[ ... ]

> @@ -581,7 +607,9 @@ static int nand_block_markbad_lowlevel(struct nand_chip *chip, loff_t ofs)
>  		nand_erase_nand(chip, &einfo, 0);
>  
>  		/* Write bad block marker to OOB */
> -		nand_get_device(chip);
> +		ret = nand_get_device(chip);
> +		if (ret)
> +			return ret;
>  
>  		ret = nand_markbad_bbm(chip, ofs);
>  		nand_release_device(chip);

[Severity: High]
Can a concurrent nand_disconnect() cause write_bbt() to issue unlocked
hardware operations on the disconnected controller when updating an
on-flash BBT?

nand_block_markbad_lowlevel() releases the chip and controller locks
before calling nand_markbad_bbt() -> nand_update_bbt() -> write_bbt().
If nand_disconnect() runs before write_bbt() calls nand_erase_nand() or
scan_write_bbt(), nand_get_device() returns -ENODEV, which write_bbt()
treats as a bad BBT eraseblock:

drivers/mtd/nand/raw/nand_bbt.c:write_bbt() {
	...
		res = nand_erase_nand(this, &einfo, 1);
		if (res < 0) {
			pr_warn("nand_bbt: error while erasing BBT block %d\n",
				res);
			mark_bbt_block_bad(this, td, chip, block);
			continue;
		}
	...
}

mark_bbt_block_bad() then marks each reserved BBT block worn in memory
and calls nand_markbad_bbm() -> nand_default_block_markbad() ->
nand_do_write_oob() without calling nand_get_device(), accessing
controller resources that have already been freed after
nand_disconnect() returned.

[ ... ]

> @@ -4688,10 +4728,14 @@ static int nand_suspend(struct mtd_info *mtd)
>  	int ret = 0;
>  
>  	mutex_lock(&chip->lock);
> +	if (chip->disconnected) {
> +		mutex_unlock(&chip->lock);
> +		return -ENODEV;
> +	}
>  	if (chip->ops.suspend)
>  		ret = chip->ops.suspend(chip);
>  	if (!ret)
> -		chip->suspended = 1;
> +		WRITE_ONCE(chip->suspended, true);
>  	mutex_unlock(&chip->lock);
>  
>  	return ret;

[Severity: Medium]
Does returning -ENODEV from nand_suspend() abort system suspend when an
open MTD handle outlives controller unbind?

While an MTD handle remains open after del_mtd_device() and
nand_disconnect(), mtd->dev stays registered on dpm_list until the last
reference is put in mtd_device_release().  During system suspend,
mtd_cls_suspend() calls mtd_suspend() -> nand_suspend(), and returning
-ENODEV fails dpm_suspend() instead of treating the disconnected chip as
a no-op like nand_resume() and nand_sync() do.

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