Re: [PATCH 2.6.10-rc1 2/5] driver-model: bus_recan_devices() locking fix

From: Dmitry Torokhov
Date: Mon Nov 08 2004 - 20:55:02 EST


On Monday 08 November 2004 08:27 pm, Tejun Heo wrote:
> On Fri, Nov 05, 2004 at 11:33:37AM -0500, Dmitry Torokhov wrote:
> > On Fri, 5 Nov 2004 15:14:39 +0900, Tejun Heo <tj@xxxxxxxxxxx> wrote:
> >
> > > Another problem I've encountered regarding kobject refcounting is
> > > that currently each sysfs attr carries its owner and gets the owning
> > > module when the attr is accessed. However, there are attributes which
> > > are provided not by the module which implements whatever the kobject
> > > represents but by upper-level module or the kernel builtins. So, it's
> > > possible to unload a module while it's kobject is still alive by
> > > holding on to such attributes, and, when the attribute is closed,
> > > panic occurs, of course (release callback is unloaded already).
> > >
> >
> > I actually spent quite few days thinking about this problem and here
> > is what I come up with:
> >
> > You have bus core module that provides generic release function for
> > devices on the bus. When object is registered you bump up module
> > count for the core module.
> > You also have modues that provide devices. At device unregister time
> > they do all device-specific cleanup, but leave attributes (and memory)
> > handled by core modules intact. This allows to unload device module
> > safely at any time and then when reference to the last generic attribute
> > is dropped the generic cleanup finally removes last traces of the device.
>
> Are you suggesting splitting every bus implementation into two
> separate modules? Otherwise, we will have to move kobj field of every
> bus-specific device structure to the head (to use the common release
> function). Either way, it can be done, but it just seems another
> twist added to the already twisted refcounting. Moreoever, not only
> the devices are problematic, refcounting in class devices and block
> device hierarchy seems broken too.
>

It is already defacto implementation standard - every subsystem has a core
modules (pci, usbcore, serio) plus modules (drivers) actually talking to
hardware and registering devices (ports, whatever) with the corresponding
core. It is just a matter of splitting cleanup functions into core and
device-specific parts.

> > <skip>
> > >
> > > We can do one of
> > >
> > > 1. add unload_sem to all driver-model structures.
> > > -> I believe this defeats the purpose of try_module_get()
> > > stuff. We wouldn't know when the module will unload.
> > >
> >
> > As Al Viro pointed out "rmmod module < /sys/bus/devices/xxxx/attr"
> > will deadlock the kernel with this scheme.
> >
> > > 2. get the owner field correct for all attributes.
> > > -> This will be a chore. We'll need to allocate separate
> > > attr structures for each kobject for all the generic attrs.
> > >
> > > 3. remove the owner field from the attribute structure and add an
> > > owner to kobject and make sysfs ops to hold the owner of the
> > > respective kobject.
> > > -> I think this is the best and most consistent solution. As
> > > we already assume that module which implements a child kobject
> > > should depend upon the module which implements its parent, all
> > > attr module reference counting will be correct by using the
> > > kobject's owner.
> > >
> > > So, I'm currently working on solution #3.
> > >
> >
> > I think this is an overkill and will inflate every kobject out there,
> > unless you will find a hole in my scheme...
>
> But all attrs will be deflated and it just seems the right thing to
> do(tm).
>

I think there are much more kobjects than attrs.

--
Dmitry
-
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/