Use cdev_del() instead of direct kobject_put() when cdev_add() fails. This aligns with standard kernel practice and maintains consistency within the driver's own error paths.
Found by code review.
Cc: stable@vger.kernel.org Fixes: 8cb5d216ab33 ("char: xillybus: Move class-related functions to new xillybus_class.c") Signed-off-by: Ma Ke make24@iscas.ac.cn --- Changes in v3: - modified the patch description, centralized cdev cleanup through standard API and maintained symmetry with driver's existing error handling; Changes in v2: - modified the patch as suggestions to avoid UAF. --- drivers/char/xillybus/xillybus_class.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/char/xillybus/xillybus_class.c b/drivers/char/xillybus/xillybus_class.c index c92a628e389e..e79cf9a0caa4 100644 --- a/drivers/char/xillybus/xillybus_class.c +++ b/drivers/char/xillybus/xillybus_class.c @@ -103,8 +103,7 @@ int xillybus_init_chrdev(struct device *dev, unit->num_nodes); if (rc) { dev_err(dev, "Failed to add cdev.\n"); - /* kobject_put() is normally done by cdev_del() */ - kobject_put(&unit->cdev->kobj); + cdev_del(unit->cdev); goto unregister_chrdev; }
@@ -157,8 +156,6 @@ int xillybus_init_chrdev(struct device *dev, device_destroy(&xillybus_class, MKDEV(unit->major, i + unit->lowest_minor));
- cdev_del(unit->cdev); - unregister_chrdev: unregister_chrdev_region(MKDEV(unit->major, unit->lowest_minor), unit->num_nodes);
On 18/07/2025 12:08, Ma Ke wrote:
Use cdev_del() instead of direct kobject_put() when cdev_add() fails. This aligns with standard kernel practice and maintains consistency within the driver's own error paths.
Could you please point at how and why this is "standard kernel practice"? In my reply to PATCH v2, I pointed out that indeed, in fs/fuse/cuse.c a failure of cdev_add() leads to a call to cdev_del(), like you suggested. However, in uio/uio.c the same scenario is handled by a call to kobject_put(), exactly as in my driver. So which way is "standard"?
There are indeed kernel-global efforts to align code with a certain coding style every now and then. Is there any such in relation to this issue?
Otherwise, please leave this alone. Playing around with error handling flows is a dangerous business, and can lead to vulnerabilities. One needs a good reason to do that on code that has been out there for a while (four years, in this case).
And now, to the patch itself:
@@ -157,8 +156,6 @@ int xillybus_init_chrdev(struct device *dev, device_destroy(&xillybus_class, MKDEV(unit->major, i + unit->lowest_minor));
- cdev_del(unit->cdev);
- unregister_chrdev: unregister_chrdev_region(MKDEV(unit->major, unit->lowest_minor), unit->num_nodes);
Why did you do this? It just adds a memory leak, and it's out of the way of even trying to fix anything: The only effect this has is that cdev_del() isn't called on error situations that occur after cdev_add() has been successful. It has nothing to do with the kobject_put() / cdev_add() thing, because that case jumps to unregister_chrdev, which is after this removal.
I have to say, both the language of the patch description as well as the weird removal of cdev_del() remind me of nonsense ChatGPT does when it's given tasks related to programming. If you're using AI to suggest patches, please stop.
Regards, Eli
linux-stable-mirror@lists.linaro.org
-
Eli Billauer -
Ma Ke