[v2] msi: free msi_desc entry only after we've released the kobject

Commit Message

Bjorn Helgaas Oct. 1, 2013, 5:53 a.m. UTC

[+cc Russell]

On Sat, Sep 28, 2013 at 11:37:27PM +0200, Veaceslav Falico wrote:
> On Thu, Sep 26, 2013 at 11:59:51AM +0200, Veaceslav Falico wrote:
> >Currently, we first do kobject_put(&entry->kobj) and the kfree(entry),
> >however kobject_put() doesn't guarantee us that it was the last reference
> >and that the kobj isn't used currently by someone else, so after we
> >kfree(entry) with the struct kobject - other users will begin using the
> >freed memory, instead of the actual kobject.
> 
> Hi Bjorn,
> 
> I've seen that you've dropped this bugfix (and the 3 cleanup patches) with
> "Changes Requested", however I don't recall any request to change this.

Oh, sorry, I didn't realize anybody else really looked at patchwork,
much less the reason codes.  I just intended to dispose of those for
now because I think we'll have several more revisions while we sort
things out.  I definitely think this is a serious issue that needs to
be fixed.  But you're right: I haven't given you any specific feedback
yet because I'm not confident about what the right fix is.

I think the current kobject delayed release is too aggressive, in the
sense that even after we've released all references, the object can
still be in sysfs, which causes future creates to fail.  E.g., this
fails:

    kset = kset_create_and_add("kobj_test", NULL, NULL);
    kset_unregister(kset);
    kset = kset_create_and_add("kobj_test", NULL, NULL);  // FAILS

when I think it should succeed.  We don't have a way for the caller to
determine when it's safe to do the second kset_create_and_add().

After we release all references, I think it's OK for the kobject
itself to continue to exist, i.e., we can delay calling t->release().
But it should be impossible to find a kobject with refcount == 0 via
sysfs, so we should be able to create a new one with the same name.

In terms of code, I'm suggesting something like the following:

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Russell King - ARM Linux Oct. 2, 2013, 8:41 p.m. UTC | #1

On Mon, Sep 30, 2013 at 10:53:46PM -0700, Bjorn Helgaas wrote:
> I think the current kobject delayed release is too aggressive,

I don't agree with that statement, but the rest of the sentence I do:

> in the
> sense that even after we've released all references, the object can
> still be in sysfs, which causes future creates to fail.  E.g., this
> fails:
> 
>     kset = kset_create_and_add("kobj_test", NULL, NULL);
>     kset_unregister(kset);
>     kset = kset_create_and_add("kobj_test", NULL, NULL);  // FAILS
> 
> when I think it should succeed.  We don't have a way for the caller to
> determine when it's safe to do the second kset_create_and_add().

The reason this happens is that for some reason I can't fathom, the
sysfs cleanup is done when the release function is called, not when
the object is unregistered.

I can see why that's done - it is so that when a kobject is unregistered,
its sysfs entry hangs around until all the children have gone (and hence
its reference count then hits zero.)

> After we release all references, I think it's OK for the kobject
> itself to continue to exist, i.e., we can delay calling t->release().
> But it should be impossible to find a kobject with refcount == 0 via
> sysfs, so we should be able to create a new one with the same name.
> 
> In terms of code, I'm suggesting something like the following:

I think I can give you an ack for this - it looks sensible enough, and
should still have the intended debugging behaviour.  I haven't tested
it though.

Acked-by: Russell King <[e-mail]>

Thanks Bjorn.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/lib/kobject.c b/lib/kobject.c
index 9621751..4e1c608 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -536,6 +536,32 @@  static struct kobject * __must_check kobject_get_unless_zero(struct kobject *kob
 	return kobj;
 }
 
+static void kobject_free(struct kobject *kobj)
+{
+	struct kobj_type *t = get_ktype(kobj);
+	const char *name = kobj->name;
+
+	if (t && t->release) {
+		pr_debug("kobject: '%s' (%p): calling ktype release\n",
+			 kobject_name(kobj), kobj);
+		t->release(kobj);
+	}
+
+	/* free name if we allocated it */
+	if (name) {
+		pr_debug("kobject: '%s': free name\n", name);
+		kfree(name);
+	}
+}
+
+#ifdef CONFIG_DEBUG_KOBJECT_RELEASE
+static void kobject_delayed_free(struct work_struct *work)
+{
+	kobject_free(container_of(to_delayed_work(work),
+		     struct kobject, release));
+}
+#endif
+
 /*
  * kobject_cleanup - free kobject resources.
  * @kobj: object to cleanup
@@ -543,7 +569,6 @@  static struct kobject * __must_check kobject_get_unless_zero(struct kobject *kob
 static void kobject_cleanup(struct kobject *kobj)
 {
 	struct kobj_type *t = get_ktype(kobj);
-	const char *name = kobj->name;
 
 	pr_debug("kobject: '%s' (%p): %s, parent %p\n",
 		 kobject_name(kobj), kobj, __func__, kobj->parent);
@@ -567,40 +592,21 @@  static void kobject_cleanup(struct kobject *kobj)
 		kobject_del(kobj);
 	}
 
-	if (t && t->release) {
-		pr_debug("kobject: '%s' (%p): calling ktype release\n",
-			 kobject_name(kobj), kobj);
-		t->release(kobj);
-	}
-
-	/* free name if we allocated it */
-	if (name) {
-		pr_debug("kobject: '%s': free name\n", name);
-		kfree(name);
-	}
-}
-
-#ifdef CONFIG_DEBUG_KOBJECT_RELEASE
-static void kobject_delayed_cleanup(struct work_struct *work)
-{
-	kobject_cleanup(container_of(to_delayed_work(work),
-				     struct kobject, release));
-}
-#endif
-
-static void kobject_release(struct kref *kref)
-{
-	struct kobject *kobj = container_of(kref, struct kobject, kref);
 #ifdef CONFIG_DEBUG_KOBJECT_RELEASE
 	pr_debug("kobject: '%s' (%p): %s, parent %p (delayed)\n",
 		 kobject_name(kobj), kobj, __func__, kobj->parent);
-	INIT_DELAYED_WORK(&kobj->release, kobject_delayed_cleanup);
+	INIT_DELAYED_WORK(&kobj->release, kobject_delayed_free);
 	schedule_delayed_work(&kobj->release, HZ);
 #else
-	kobject_cleanup(kobj);
+	kobject_free(kobj);
 #endif
 }
 
+static void kobject_release(struct kref *kref)
+{
+	kobject_cleanup(container_of(kref, struct kobject, kref));
+}
+
 /**
  * kobject_put - decrement refcount for object.
  * @kobj: object.