[Patch -mm 3/3] RFC: Introduce kobject->owner for refcounting.

[Patch -mm 3/3] RFC: Introduce kobject->owner for refcounting.

[Cornelia Huck]

Grab and release the module kobject refcount if kobj->owner is set.
This prevents calls to the release function after the module has
been unloaded.

Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>

---
 include/linux/kobject.h |    1 +
 lib/kobject.c           |    6 +++++-
 2 files changed, 6 insertions(+), 1 deletion(-)

--- linux-2.6.orig/include/linux/kobject.h
+++ linux-2.6/include/linux/kobject.h
@@ -51,6 +51,7 @@ enum kobject_action {
 };
 
 struct kobject {
+	struct module		* owner;
 	const char		* k_name;
 	char			name[KOBJ_NAME_LEN];
 	struct kref		kref;
--- linux-2.6.orig/lib/kobject.c
+++ linux-2.6/lib/kobject.c
@@ -177,6 +177,8 @@ void kobject_init(struct kobject * kobj)
 	INIT_LIST_HEAD(&kobj->entry);
 	init_waitqueue_head(&kobj->poll);
 	kobj->kset = kset_get(kobj->kset);
+	/* Attempt to grab reference of owning module's kobject. */
+	mod_kobject_get(kobj->owner);
 }
 
 
@@ -254,6 +256,7 @@ int kobject_shadow_add(struct kobject * 
 			printk("kobject_add failed for %s (%d)\n",
 			       kobject_name(kobj), error);
 		 dump_stack();
+		 mod_kobject_put(kobj->owner);
 	}
 
 	return error;
@@ -491,6 +494,7 @@ void kobject_cleanup(struct kobject * ko
 	struct kobj_type * t = get_ktype(kobj);
 	struct kset * s = kobj->kset;
 	struct kobject * parent = kobj->parent;
+	struct module * owner = kobj->owner;
 
 	pr_debug("kobject %s: cleaning up\n",kobject_name(kobj));
 	if (kobj->k_name != kobj->name)
@@ -503,7 +507,7 @@ void kobject_cleanup(struct kobject * ko
 			"if this is not a directory kobject, it is broken "
 			"and must be fixed.\n",
 			kobj->name);
-
+	mod_kobject_put(owner);
 	if (s)
 		kset_put(s);
 	kobject_put(parent);

Re: [Patch -mm 3/3] RFC: Introduce kobject->owner for refcounting.

[Greg KH]

On Mon, Apr 16, 2007 at 07:36:27PM +0200, Cornelia Huck wrote:
> Grab and release the module kobject refcount if kobj->owner is set.
> This prevents calls to the release function after the module has
> been unloaded.

But doesn't this cause reference counts to be grabbed on modules that
don't want them?  (like network drivers)?

I don't understand what exactly you are trying to protect from here.
Especially as Tejun just ripped the lifetime rules between sysfs and
kobjects apart.

thanks,

greg k-h

Re: [Patch -mm 3/3] RFC: Introduce kobject->owner for refcounting.

[Alan Stern]

On Mon, 16 Apr 2007, Greg KH wrote:

> On Mon, Apr 16, 2007 at 07:36:27PM +0200, Cornelia Huck wrote:
> > Grab and release the module kobject refcount if kobj->owner is set.
> > This prevents calls to the release function after the module has
> > been unloaded.
> 
> But doesn't this cause reference counts to be grabbed on modules that
> don't want them?  (like network drivers)?

The refcounts we are talking about will not prevent modules from being 
unloaded.  That is, they won't cause rmmod to fail -- they will simply 
cause it to wait until the refcount drops to 0.  This should happen 
naturally as a result of the unregistration calls made by the module's 
exit routine.

(That was the intention, anyway.  I haven't yet seen all the parts of 
Cornelia's patch set, but that's how we discussed it.)

> I don't understand what exactly you are trying to protect from here.
> Especially as Tejun just ripped the lifetime rules between sysfs and
> kobjects apart.

There are other ways for userspace to acquire references to kernel 
objects.  Think of procfs or debugfs, for example.  Or opening a device 
file.

Consider this example:  A user has a pcmcia card containing a USB host 
controller.  He plugs a USB flash drive into the card and mounts a 
filesystem stored on the flash drive.  The mount call increments the 
module refcount for usb-storage (the owner of the block device containing 
the filesystem), but it doesn't do anything to the module refcount for the 
pcmcia driver (I forget its name -- yenta or something like that).

So what happens if the pcmcia driver is unloaded while the filesystem is
mounted?  What happens when the filesystem is finally unmounted and a
release method belonging to the pcmcia driver is called?

The fundamental rule is that whenever you hand out a pointer to a routine
living in a module, the receiver has to increment the module's refcount.  
But the driver core violates this rule all over the place.  The most
pervasive offender is struct device -- it contains a release() method
pointer which often points into a module, but the driver core doesn't
do anything like try_module_get() in device_initialize() or module_put()
after calling dev->release().

Alan Stern

Re: [Patch -mm 3/3] RFC: Introduce kobject->owner for refcounting.

[Rusty Russell]

On Mon, 2007-04-16 at 15:53 -0400, Alan Stern wrote:
> The fundamental rule is that whenever you hand out a pointer to a routine
> living in a module, the receiver has to increment the module's refcount.  
> But the driver core violates this rule all over the place.

Hi Alan,

	Your rule is overly simplistic, unfortunately.  You have two choices:
take a reference count, *or* ensure that the reference will go away when
the module's cleanup routine is called.  Network drivers are a classic
example of the latter.

	Note that you cannot do both: if the cleanup routine calls something
which drops a reference count, it implies that the cleanup routine needs
to be called with non-zero reference count, and it won't be (ignoring
--force).

I hope that clarifies?
Rusty.

Re: [Patch -mm 3/3] RFC: Introduce kobject->owner for refcounting.

[Cornelia Huck]

On Tue, 17 Apr 2007 12:53:10 +1000,
Rusty Russell <rusty@rustcorp.com.au> wrote:

> On Mon, 2007-04-16 at 15:53 -0400, Alan Stern wrote:
> > The fundamental rule is that whenever you hand out a pointer to a routine
> > living in a module, the receiver has to increment the module's refcount.  
> > But the driver core violates this rule all over the place.
> 
> Hi Alan,
> 
> 	Your rule is overly simplistic, unfortunately.  You have two choices:
> take a reference count, *or* ensure that the reference will go away when
> the module's cleanup routine is called.  Network drivers are a classic
> example of the latter.

Hm, but that's exactly the problem we face here. There is no race-free
way (at least, nobody could think about one yet) to make sure all
references are freed at exit time and simultaneously to make sure that
the release functions have finished before the module has been deleted.

> 
> 	Note that you cannot do both: if the cleanup routine calls something
> which drops a reference count, it implies that the cleanup routine needs
> to be called with non-zero reference count, and it won't be (ignoring
> --force).

That's where that second reference count (the kref in the embedded
kobject in the module) comes into play, that doesn't prevent rmmod from
being run.

Re: [Patch -mm 3/3] RFC: Introduce kobject->owner for refcounting.

[Alan Stern]

On Tue, 17 Apr 2007, Cornelia Huck wrote:

> On Tue, 17 Apr 2007 12:53:10 +1000,
> Rusty Russell <rusty@rustcorp.com.au> wrote:
> 
> > On Mon, 2007-04-16 at 15:53 -0400, Alan Stern wrote:
> > > The fundamental rule is that whenever you hand out a pointer to a routine
> > > living in a module, the receiver has to increment the module's refcount.  
> > > But the driver core violates this rule all over the place.
> > 
> > Hi Alan,
> > 
> > 	Your rule is overly simplistic, unfortunately.  You have two choices:
> > take a reference count, *or* ensure that the reference will go away when
> > the module's cleanup routine is called.  Network drivers are a classic
> > example of the latter.
> 
> Hm, but that's exactly the problem we face here. There is no race-free
> way (at least, nobody could think about one yet) to make sure all
> references are freed at exit time and simultaneously to make sure that
> the release functions have finished before the module has been deleted.

More specifically, there _is_ no way in general to ensure that a reference
will go away when the module's cleanup routine is called, unless you are
very careful not to pass that reference on to _anybody_.  The driver core
certainly can't do this; it passes a device reference to anyone who
creates a child of that device (the parent pointer) -- and it can't 
guarantee that every one of its clients will drop their references when 
their exit routines run.

I'm not so sure I fully believe that network drivers can do what Rusty
says, at least not in every circumstance.  There simply are too many
possible ways for references to linger on after you thought they were all
gone.  For example, what happens when the user has mounted a filesystem on
a USB drive running via USB-over-TCP though a network interface?

Alan Stern

Re: [Patch -mm 3/3] RFC: Introduce kobject->owner for refcounting.

[Rusty Russell]

On Tue, 2007-04-17 at 12:08 -0400, Alan Stern wrote:
> More specifically, there _is_ no way in general to ensure that a reference
> will go away when the module's cleanup routine is called, unless you are
> very careful not to pass that reference on to _anybody_.  The driver core
> certainly can't do this; it passes a device reference to anyone who
> creates a child of that device (the parent pointer) -- and it can't 
> guarantee that every one of its clients will drop their references when 
> their exit routines run.

Hi Alan,

	Your assertion is correct.  I haven't studied the driver core, so I
might be off-base here, but you'll note that if the module references
the core kmalloc'ed object rather than the other way around it can be
done safely.  The core can also reference the module, but it must be
able to live without it once it's gone (eg. by returning -ENOENT).

A really poor example is below:

int register_foo(struct foo *foo)
{
	struct registered_foo *r = kmalloc(...);
	if (!r)
		return -ENOMEM;
	r->foo = foo;
	atomic_set(&r->refcnt, 1);
	spin_lock(&foo_lock);
	list_add(&foos, &r->list);
	spin_unlock(&foo_lock);
	return 0;
}

void unregister_foo(struct foo *foo)
{
	struct registered_foo *i, *found = NULL;
	spin_lock(&foo_lock);
	list_for_each_entry(i, &foos, list) {
		if (i->foo == foo) {
			i->foo = NULL;
			if (atomic_dec_and_test(&i->refcnt))
				kfree(i);
			break;
		}
	}
	spin_unlock(&foo_lock);
}

int get_foo_value(struct registered_foo *r)
{
	u32 val = -ENOENT;
	spin_lock(&foo_lock);
	if (r->foo)
		val = r->foo->val;
	spin_unlock(&foo_lock);
	return val;
}

> I'm not so sure I fully believe that network drivers can do what Rusty
> says, at least not in every circumstance.  There simply are too many
> possible ways for references to linger on after you thought they were all
> gone.  For example, what happens when the user has mounted a filesystem on
> a USB drive running via USB-over-TCP though a network interface?

That's actually fine, it's when packets linger in the system holding
references to the interface they came from that things get tricky.
Hence the networking code tries not to do that.

Cheers,
Rusty.

Re: [Patch -mm 3/3] RFC: Introduce kobject->owner for refcounting.

[Alan Stern]

On Wed, 18 Apr 2007, Rusty Russell wrote:

> Hi Alan,
> 
> 	Your assertion is correct.  I haven't studied the driver core, so I
> might be off-base here, but you'll note that if the module references
> the core kmalloc'ed object rather than the other way around it can be
> done safely.  The core can also reference the module, but it must be
> able to live without it once it's gone (eg. by returning -ENOENT).

"Live without it once it's gone..."  Do you mean once the object is gone 
or once the module is gone?  The core in general has no way to know when 
the module is gone; all it knows about is the object.  The trouble arises 
when the module is gone (whether the core knows it or not) but the object 
is still present.

> A really poor example is below:
> 
> int register_foo(struct foo *foo)
> {
> 	struct registered_foo *r = kmalloc(...);
> 	if (!r)
> 		return -ENOMEM;
> 	r->foo = foo;
> 	atomic_set(&r->refcnt, 1);
> 	spin_lock(&foo_lock);
> 	list_add(&foos, &r->list);
> 	spin_unlock(&foo_lock);
> 	return 0;
> }
> 
> void unregister_foo(struct foo *foo)
> {
> 	struct registered_foo *i, *found = NULL;
> 	spin_lock(&foo_lock);
> 	list_for_each_entry(i, &foos, list) {
> 		if (i->foo == foo) {
> 			i->foo = NULL;
> 			if (atomic_dec_and_test(&i->refcnt))
> 				kfree(i);
> 			break;
> 		}
> 	}
> 	spin_unlock(&foo_lock);
> }
> 
> int get_foo_value(struct registered_foo *r)
> {
> 	u32 val = -ENOENT;
> 	spin_lock(&foo_lock);
> 	if (r->foo)
> 		val = r->foo->val;
> 	spin_unlock(&foo_lock);
> 	return val;
> }

The example is fine as far as it goes, but it assumes that all
interactions with the underlying r->foo object can be done under a
spinlock.  Of course this isn't true in general.

Alan Stern

Re: [Patch -mm 3/3] RFC: Introduce kobject->owner for refcounting.

[Rusty Russell]

On Wed, 2007-04-18 at 11:20 -0400, Alan Stern wrote:
> On Wed, 18 Apr 2007, Rusty Russell wrote:
> 
> > Hi Alan,
> > 
> > 	Your assertion is correct.  I haven't studied the driver core, so I
> > might be off-base here, but you'll note that if the module references
> > the core kmalloc'ed object rather than the other way around it can be
> > done safely.  The core can also reference the module, but it must be
> > able to live without it once it's gone (eg. by returning -ENOENT).
> 
> "Live without it once it's gone..."  Do you mean once the object is gone 
> or once the module is gone?  The core in general has no way to know when 
> the module is gone; all it knows about is the object.  The trouble arises 
> when the module is gone (whether the core knows it or not) but the object 
> is still present.

Hi Alan,

	I meant that the module is gone: it has told the object (via
unregister_xxx) that it's gone.

> > A really poor example is below:
...
> The example is fine as far as it goes, but it assumes that all
> interactions with the underlying r->foo object can be done under a
> spinlock.  Of course this isn't true in general.

There are certainly other ways of doing it, such as a mutex, a refcnt &
completion (for function pointers), or disabling preemption across the
access and using stop_machine().  Of course, these add complexity.

This is the reason that I've always disliked module removal.  We have a
lot of code to deal with it and it has awkward semantics (unless --wait
is used).  OTOH, I'm not a fan of the network approach, either: I feel
that bringing up an interface should bump the refcnt of the module which
implements that interface.  Currently taking out e1000 will just kill my
eth0.

Cheers,
Rusty.







> 
> Alan Stern