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

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

[Hans de Goede]

Hi all,

First some background which is not necessary to understand the rest of this 
post: I encountered the below problem while looking into some videodev.c 
reference counting patches in combination with converting videodev.c from 
register_chrdev, to register_chrdev_region + cdev. I wanted to put links to 
relevant posts here, but for some reason the video4linux-list archive is 
private, I'll kick someone about that.

Now the problem, struct cdev contains a kobj for reference counting, which gets 
initialized either in cdev_alloc (for dynamically allocated cdev structs) or in 
cdev_init (for structs part of a larger struct). Thus setting the reference 
count to 1. When the device underlying the cdev is removed (usb disconnect for 
example) cdev_del gets called which does a kobject_put on the kobj, lowering 
the ref count to 0 causing it to be released.

However releasing the cdev struct, while some apps still has an open fd 
refering to the character device is not a good idea, so chrdev_open does a 
kobj_get on the cdev's kobj.

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.

I see 2 solutions here:

1) Fix all cdev users to call cdev_put on their cdev in their fops release
    method, adding a release method to do this where necessary, but this seems
    rather error prone

2) Add a private fops struct to cdev, which gets filled with the device drivers
    fops, except for release, which will point to a chrdev_release function
    which call cdev_put after calling the device driver fops release method if
    present, to me this seems the preferable solution.

###

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.

If the reference counting in cdev is fixed, this would almost be 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 structs 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.

Regards,

Hans

p.s.

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

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

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.

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list