[PATCH v4] char: xillybus: Fix error handling in xillybus_init_chrdev()

[PATCH v4] char: xillybus: Fix error handling in xillybus_init_chrdev()

[Ma Ke]

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;
 	}
 
-- 
2.25.1

Re: [PATCH v4] char: xillybus: Fix error handling in xillybus_init_chrdev()

[Eli Billauer]

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

Re: [PATCH v4] char: xillybus: Fix error handling in xillybus_init_chrdev()

[Greg KH]

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