Re: [PATCH 2/3] mtd: cfi_cmdset_0002: implement point() for simple linear maps

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mtd: cfi_cmdset_0002: unbounded TASK_UNINTERRUPTIBLE sleep causes Hung Task Panic
--

commit 97c2e746b1f52c7744c56b40bf00fe410dca196f
Author: Orgad Shaneh <orgads@gmail.com>

mtd: cfi_cmdset_0002: implement point() for simple linear maps

This commit adds point() and unpoint() support for simple linear maps to
the cfi_cmdset_0002 driver. This allows filesystems like jffs2 or cramfs
to scan memory in place without copying every used eraseblock. When a chip
is pointed to, writes, erases, and sync operations wait for the unpoint.

> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
> index 517db2f2707f4..67578fef02bfa 100644
> --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c

[ ... ]

> @@ -960,8 +980,13 @@ static int get_chip(struct map_info *map, struct flchip *chip, unsigned long adr
>  		return -EIO;
>  
>  	case FL_POINT:
> -		/* Only if there's no operation suspended... */
> -		if (mode == FL_READY && chip->oldstate == FL_READY)
> +		/*
> +		 * Only if there's no operation suspended: the chip is in
> +		 * array mode, so reads, further points and the reboot
> +		 * reset (which only re-enters array mode) can go ahead.
> +		 */
> +		if ((mode == FL_READY || mode == FL_POINT ||
> +		     mode == FL_SHUTDOWN) && chip->oldstate == FL_READY)
>  			return 0;
>  		fallthrough;

[Severity: High]
Does this logic in get_chip() cause unintended blocking when the chip has a
suspended operation?

If a filesystem like CramFS mounts an MTD partition and calls mtd_point(),
chip->state becomes FL_POINT indefinitely until unmount. If this point
happens while an erase is suspended (chip->oldstate == FL_ERASING),
requests for FL_READY or FL_SHUTDOWN will fail the check for
chip->oldstate == FL_READY.

They will then fall through to the default sleep path. Because the point is
held indefinitely by the filesystem, could this cause the reboot notifier
(FL_SHUTDOWN) to deadlock permanently?

>  	default:
>  	sleep:
>  		set_current_state(TASK_UNINTERRUPTIBLE);
>  		add_wait_queue(&chip->wq, &wait);
>  		mutex_unlock(&chip->mutex);
>  		schedule();
>  		remove_wait_queue(&chip->wq, &wait);
>  		mutex_lock(&chip->mutex);
>  		goto resettime;

[Severity: High]
Is it safe to use TASK_UNINTERRUPTIBLE for unbounded synchronization here?

When chip->state is FL_POINT (held long-term by a mounted filesystem), and
another task initiates a flash write, erase, or sync, get_chip() is called
with FL_WRITING, FL_ERASING, or FL_SYNCING. These modes do not match the
FL_POINT exceptions and fall through directly to this sleep block.

Since FL_POINT won't be released until the filesystem is unmounted, tasks
sleeping here will remain in TASK_UNINTERRUPTIBLE indefinitely. Could this
cause a hung task panic after the standard 120-second timeout, allowing a
local user to crash the system simply by running sync(2) while CramFS is
mounted?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261010172142.2138956-1-orgads@gmail.com?part=2