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 v4: - Apologize, due to the long time that has passed since the last v2 version, I was negligent when submitting v3. I have now corrected it; 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 | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/char/xillybus/xillybus_class.c b/drivers/char/xillybus/xillybus_class.c index c92a628e389e..493bbed918c2 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; }
Hello,
On 19/07/2025 15:17, 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.
Sorry, to the extent it matters, I'm not acknowledging this.
This is merely a code styling issue, and as far as I know, there is no "standard kernel practice" on this matter. If such standard practice exists, the correct way is to prepare a patchset of *all* occurrences of kobject_put() used this way, and replace them all (e.g. fs/char_dev.c, uio/uio.c and tty/tty_io.c). Should xillybus_class be included in this patchset, I will of course ack it, not that it would matter much.
In my opinion, using cdev_del() is incorrect, as you can't delete something that failed to be added. Practically this causes no problem, but as a question of style, the kobject_put() call acts as the reversal of cdev_alloc(). This is formally more accurate, and this is the reason I chose to do it this way.
But more than anything, I find this patch pointless, unless someone explains why it has any use. I'm open to new insights.
Regards, Eli
On Sun, Jul 20, 2025 at 10:05:43AM +0200, Eli Billauer wrote:
Hello,
On 19/07/2025 15:17, 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.
Sorry, to the extent it matters, I'm not acknowledging this.
This is merely a code styling issue, and as far as I know, there is no "standard kernel practice" on this matter. If such standard practice exists, the correct way is to prepare a patchset of *all* occurrences of kobject_put() used this way, and replace them all (e.g. fs/char_dev.c, uio/uio.c and tty/tty_io.c). Should xillybus_class be included in this patchset, I will of course ack it, not that it would matter much.
In my opinion, using cdev_del() is incorrect, as you can't delete something that failed to be added. Practically this causes no problem, but as a question of style, the kobject_put() call acts as the reversal of cdev_alloc(). This is formally more accurate, and this is the reason I chose to do it this way.
But the "problem" is that just the kobject_put() might not be enough, the kmap needs to be freed. But the map might be the part that failed, so then the kobject_put() is incorrect to be calling!
So yes, I agree, this patch shouldn't be applied, it's not going to solve the real problem here in that the cdev api is a total mess in many places like this (full disclosure, it's probably all my fault as well...)
As failing this codepath is NOT triggerable by a user, this whole discussion is just theory at this point in time. So let's just leave it alone.
Long-term, the cdev api should be fixed up, it's been on my TODO list for decades now, and in a way I'm punting it to just wait for the rust versions of drivers to take over as that's the correct fix for many of these types of corner cases as the bindings will get this right.
Ma Ke, if you want a real project, please work on fixing up the cdev api, don't worry about code paths that never can be hit like this.
thanks,
greg k-h
linux-stable-mirror@lists.linaro.org
-
Eli Billauer -
Greg KH -
Ma Ke