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

This is a discussion on fs/char_dev.c memory leak (broken reference counting) - Kernel ; 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. ...

+ Reply to Thread
Results 1 to 2 of 2

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

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

    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.

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

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

    Hans de Goede wrote:



    > 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@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread