Re: [PATCH 15/26] sh: maple: remove not needed maple_unsupported_device driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [High] Asynchronous work items are not canceled on the `maple_bus_init` error path.
- [High] Error path in `maple_bus_init` frees devices that are still linked on the global `maple_waitq`.
- [High] Directly calling `kfree` via `maple_free_dev` on `device_register` failure in `maple_attach_driver` leaks kobject state.
- [High] Re-queuing a maple packet without setting the busy flag causes list corruption.
--

commit 8ed68bea570556010f84062b91e5bd48da1f0c8d
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>

sh: maple: remove not needed maple_unsupported_device driver

This commit removes the maple_unsupported_device fallback driver since
the driver core handles devices without drivers. It also updates the
shared interrupt registrations to use &maple_bus instead of the removed
driver.

> diff --git a/drivers/sh/maple/maple.c b/drivers/sh/maple/maple.c
> --- a/drivers/sh/maple/maple.c
> +++ b/drivers/sh/maple/maple.c

[ ... ]

> @@ -378,7 +376,6 @@ static void maple_attach_driver(struct maple_device *mdev)
>  		if (matched == 0) {
>  			/* Driver does not exist yet */
>  			dev_info(&mdev->dev, "no driver found\n");
> -			mdev->driver = &maple_unsupported_device;
>  		}
>  		dev_set_name(&mdev->dev, "%d:0%d.%lX", mdev->port,
>  			     mdev->unit, function);

[Severity: High]
This isn't a regression introduced by this patch, but does the error path in
maple_attach_driver() leak kobject state?

If device_register(&mdev->dev) fails:

	error = device_register(&mdev->dev);
	if (error) {
		dev_warn(&mdev->dev, "could not register device at"
			" (%d, %d), with error 0x%X\n", mdev->unit,
			mdev->port, error);
		maple_free_dev(mdev);
		mdev = NULL;
		return;
	}

maple_free_dev() directly calls kfree(). Since device_register() was
called, shouldn't failures be handled by calling put_device() to safely
invoke the release callback instead of bypassing it?

[ ... ]

> @@ -863,17 +851,14 @@ static int __init maple_bus_init(void)
>  	kmem_cache_destroy(maple_queue_cache);
>  
>  cleanup_bothirqs:
> -	free_irq(HW_EVENT_VSYNC, 0);
> +	free_irq(HW_EVENT_VSYNC, &maple_bus);
>  
>  cleanup_irq:
> -	free_irq(HW_EVENT_MAPLE_DMA, 0);
> +	free_irq(HW_EVENT_MAPLE_DMA, &maple_bus);
>  
>  cleanup_dma:
>  	free_pages((unsigned long) maple_sendbuf, MAPLE_DMA_PAGES);

[Severity: High]
This is a pre-existing issue, but can this error path lead to a
use-after-free crash?

In maple_bus_init(), if initialization fails after setting the DMA
interrupt handler (for example, if KMEM_CACHE allocation fails), the error
path calls free_irq() and frees the maple_sendbuf memory.

However, it does not appear to call cancel_work_sync() for
maple_dma_process and maple_vblank_process. If an interrupt fired before
free_irq(), could the scheduled work items execute asynchronously after the
buffers are freed, dereferencing the freed memory?

[Severity: High]
This is another pre-existing issue in maple_bus_init(), but does the port
initialization loop leave dangling pointers on the global maple_waitq?

During the loop, each allocated device is immediately queued onto the
global maple_waitq via maple_add_packet():

	for (i = 0; i < MAPLE_PORTS; i++) {
		checked[i] = false;
		empty[i] = false;
		mdev[i] = maple_alloc_dev(i, 0);
		if (!mdev[i]) {
			while (i-- > 0)
				maple_free_dev(mdev[i]);

If a subsequent allocation fails, the error handler loops backwards and
calls maple_free_dev(). However, maple_free_dev() frees mdev and mdev->mq
directly via kfree() without executing list_del() to remove them from
maple_waitq. Could this corrupt the linked list?

[Severity: High]
This is a pre-existing issue not touched by this patch, but can re-queuing
a maple packet without setting the busy flag cause list corruption?

Looking at setup_maple_commands() in drivers/sh/maple/maple.c, a
MAPLE_COMMAND_GETCOND packet is queued if mdev->busy is 0:

	if (mdev->interval > 0 && atomic_read(&mdev->busy) == 0 &&
		time_after(jiffies, mdev->when)) {
		/* bounce if we cannot add */
		add = maple_add_packet(mdev,
			be32_to_cpu(mdev->devinfo.function),
			MAPLE_COMMAND_GETCOND, 1, NULL);
		if (!add)
			mdev->when = jiffies + mdev->interval;
	}

It successfully calls maple_add_packet(), which links mdev->mq->list into
maple_waitq. However, unlike other packet types, it fails to set
mdev->busy = 1.

If a concurrent userspace action (such as a read via /dev/mtdX in vmu-flash
leading to maple_vmu_read_block()) executes, would it read mdev->busy as 0,
assume the queue is free, and call maple_add_packet() again? This would
reuse the exact same mdev->mq object, executing list_add_tail() on an
already-linked node, which corrupts the list.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703-b4-maple-cleanup-v1-0-41e424964da5@gmail.com?part=15