Re: fs/char_dev.c memory leak (broken reference counting)

[Hans de Goede]

Hans de Goede wrote:

<snip>

> And here is the problem when the fd refering to the character device 
> gets closed, no-one does a kobj_put. chrdev_open replace the file's f_op 
> pointer with the device driver fops, so the only fops release which will 
> get called is that of the device driver, cdev_put (which will call 
> kobj_put on the kobj) is exported, so device driver release methods 
> could and I guess should call cdev_put, but under drivers/char there is 
> not a single driver calling cdev_put !!
>
> So unless I'm missing something the kojb release callback never gets 
> called.

Never mind, I just found out that cdev_put gets called from __fput() in 
fs/file_table.c, thats somewhat convoluted if I may say so, I think atleast a 
comment in char_dev.c explaining this would be in order.

So that only leaves this part of my mail:

> ###
>
> While on this topic in case of an usb device whose driver exports an 
> chardev to userspace, the device can be disconnected while the chardev 
> is still open. Currently usb-chardev drivers need to do their own 
> reference counting in their open / release fops to make sure their 
> device structure stays around until the last user has closed the device.
>
> The reference counting in cdev is almost an 
> exact duplicate of the ref counting done in the device driver, thus I 
> would like to propose to add a release function ptr to the cdev struct 
> which if not NULL gets called from the cdev kobj release handler, then 
> then device driver no longer has to duplicate the ref counting.
>
> This esp seems to make sense in cases where the device driver uses 
> cdev_init, as then the cdev structure could currently be freed by the 
> device driver (in case of hot unplug) without it knowing for sure that 
> there are no more users of the cdev structure. For example even when the 
> device driver does its own ref counting in the open / release fops, 
> there could still be some users in the form of open cdev sysfs files.

I would still very much like to see this release callback get added, if there 
are no objections I'll do a patch for this.

Regards,

Hans

p.s.

Please keep me in the CC, I'm not subscribed to the kernel mailinglist.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/