[lkml]   [2012]   [Jan]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: Problems with get_driver() and driver_attach() (and new_id too)
    Hi Alan,

    On Thu, Jan 05, 2012 at 11:31:00AM -0500, Alan Stern wrote:
    > Greg and Kay:
    > There are some nasty problems connected with the driver core's
    > get_driver(), put_driver(), and driver_attach(). Not just
    > implementation bugs, but deeper conceptual difficulties.
    > Let's start with get_driver(). Its comment says that it increments the
    > driver's refcount, just like get_device() and a lot of other utility
    > routines.
    > But a struct driver is _not_ like a struct device! It resembles a
    > piece of code more than a piece of data -- it acts as an encapsulation
    > of a driver. Incrementing its refcount doesn't have much meaning
    > because a driver's lifetime isn't determined by the structure's
    > refcount; it's determined by when the driver's module gets unloaded.
    > What really matters for a driver is whether or not it is registered.
    > Drivers expect, for example, that none of their methods will be called
    > after driver_unregister() returns. It doesn't matter if some other
    > thread still holds a reference to the driver structure; that reference
    > mustn't be used for accessing the driver code after unregistration.
    > And of course, driver_attach() does access the driver code, by calling
    > the probe routine.

    Agree here.

    > An example where this is violated occurs in the usb-serial core. Each
    > serial driver module registers (at least) two driver structures, one on
    > the usb_serial_bus and one on the usb_bus. The usb_serial_driver
    > structure contains a pointer to the usb_driver structure, and this
    > pointer is passed to get_driver() when the serial driver's new_id sysfs
    > attribute is written to.
    > Now, udev scripts are capable of writing to sysfs attributes very soon
    > after the attribute is created. In the case of USB serial drivers, we
    > have a bug report of a situation where this write took place after the
    > usb_serial_driver was registered but before the usb_driver was
    > registered. Thus, get_driver() was handed a pointer to a driver
    > structure that had not even been initialized, let alone registered, and
    > so naturally it crashed.
    > Almost as bad is what can happen when a driver is unregistered while
    > some thread is holding a reference obtained from get_driver(). The
    > reference prevents the driver structure from being freed, but it
    > doesn't prevent the thread from calling driver_attach() after the
    > unregistration is complete, at which time the driver code does not
    > expect to be invoked.
    > To fix these problems, we need to change the semantics of get_driver()
    > and put_driver(). Instead of taking a reference to the driver
    > structure, get_driver() should check whether the driver is currently
    > registered. If not, return NULL; otherwise, pin the driver (i.e.,
    > block it from being unregistered) until put_driver() is called.

    Or maybe we should just drop get_driver() and put_driver() and just make
    sure that driver_attach() does not race with driver_unregister()?
    I think pinning driver so that it can't be unregistered (and
    consequently module unload hangs) its a mis-feature.

    > This will require some code auditing, because there are places where
    > get_driver() is called without checking the return value (see
    > drivers/pci/pci_driver.c:pci_add_dynid() for an example; there are
    > others). It should be marked __must_check.
    > Also, there are places that call driver_attach() without first calling
    > get_driver() (see drivers/input/gameport/gameport.c,
    > drivers/input/serio/serio.c, and drivers/char/agp/amd64-agp.c). They
    > may or may not be safe; I don't know.

    Serio and gameport are safe as everyting is protected by serio_mutex so
    it is not possible to yank the driver our while we are trying to attach
    it to a device.

    > One more thing. The new_id sysfs attribute can cause problems of its
    > own. Writes to it cause a dynamic ID structure to be allocated, and
    > these structures will leak unless they are properly deallocated.
    > Normally they are freed when the driver is unregistered. But what if
    > registration fails to begin with? It might fail at a point after the
    > new_id attribute was created, which means the attribute could have been
    > written to. The dynamic IDs need to be freed after registration fails,
    > but nobody does this currently.

    Don't we create corresponding sysfs attributes only after driver
    successfully registered? And attributes are the only way to add (and
    thus allocate) new ids so I do not see why we'd be leaking here.



     \ /
      Last update: 2012-01-05 19:03    [W:0.026 / U:187.896 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site