Thread (45 messages) 45 messages, 4 authors, 6d ago

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

From: sashiko-bot@kernel.org
Date: 2026-07-04 06:11:36

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 [off-list ref]

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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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?

[ ... ]
quoted hunk ↗ jump to hunk
@@ -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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help