Messages in this thread | | | Date | Sun, 14 Apr 2013 04:35:39 +0100 | From | Al Viro <> | Subject | Re: [PATCH] module: Fix race condition between load and unload module |
| |
On Fri, Apr 12, 2013 at 04:47:50PM -0700, Linus Torvalds wrote: > This is a much more generic bug in kobjects, and I would hate to add > some random workaround for just one case of this bug like you do. The > more fundamental bug needs to be fixed too. > > I think the more fundamental bugfix is to just fix kobject_get() to > return NULL if the refcount was zero, because in that case the kobject > no longer really exists. > > So instead of having > > kref_get(&kobj->kref); > > it should do > > if (!atomic_inc_not_zero(&kobj->kref.refcount)) > kobj = NULL; > > and I think that should fix your race automatically, no? Proper patch > attached (but TOTALLY UNTESTED - it seems to compile, though). > > The problem is that we lose the warning for when the refcount is zero > and somebody does a kobject_get(), but that is ok *assuming* that > people actually check the return value of kobject_get() rather than > just "know" that if they passed in a non-NULL kobj, they'll get it > right back. > > Greg - please take a look... I'm adding Al to the discussion too, > because Al just *loooves* these kinds of races ;)
Unless I'm misreading what's going on, we have the following to thank for that: /* remove from sysfs if the caller did not do it */ if (kobj->state_in_sysfs) { pr_debug("kobject: '%s' (%p): auto cleanup kobject_del\n", kobject_name(kobj), kobj); kobject_del(kobj); } in kobject_cleanup(). Why don't we require kobject_del() before the final kobject_put(), if the sucker had been added? FWIW, I thought it *was* required all along...
| |