Re: [PATCH v2] msi: free msi_desc entry only after we've released the kobject
On Fri, Oct 04, 2013 at 10:46:31AM -0600, Bjorn Helgaas wrote: On Sat, Sep 28, 2013 at 3:37 PM, Veaceslav Falico wrote: On Thu, Sep 26, 2013 at 11:59:51AM +0200, Veaceslav Falico wrote: Currently, we first do kobject_put(>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. I talked to Greg KH about this recently, and he said he might take a look at doing a more extensive cleanup of populate_msi_sysfs() using attribute groups, so I don't know if you want to wait and see whether he does anything, or go ahead on the path you were on. Sorry for the delay, was sick. I'll continue going ahead, however if Greg/you don't really need it or are working on it - please say now, so I'll stop waisting your time. If you continue, my advice is: - Put all these patches in a single series with a version number (I think the next posting would be v3) to help me keep track of them. Will do, if/when there'll be next version. Now they're divided into 1 bugfix and 1 cleanup patchset. - In populate_msi_sysfs(), drop the pci_dev_get() (or explain why it's needed). My reasoning is that the "msi_irqs" kset should already hold a reference on the pdev (acquired in kset_create_and_add() -> kset_register() -> kobject_add_internal()), and each irq entry should hold a reference on the kset (see kobject_add_internal() again), so it is redundant to acquire a reference on the pdev directly. This means dropping the pci_dev_put() in msi_kobj_release(), of course. It's done in my patch pci: remove redundant pci_dev_get/put() on kobject (un)register http://patchwork.ozlabs.org/patch/278201/ - Move the kfree(entry) from free_msi_irqs() to msi_kobj_release() (I think one of your patches already did this). It's done in my patch msi: free msi_desc entry only after we've released the kobject http://patchwork.ozlabs.org/patch/278150/ - In populate_msi_sysfs(), drop the kobject_del() in the out_unroll loop. I think we would only need that if there were a way to create a new irq entry in "msi_irqs" before the old irq entry was released. But I don't think that's possible. We only create irq entries in populate_msi_sysfs(), which always starts with a fresh, empty "msi_irqs" kset. It's done in my patch msi: free msi_desc entry only after we've released the kobject http://patchwork.ozlabs.org/patch/278150/ - In free_msi_irqs(), similarly remove the kobject_del(). It's done in my patch msi: remove useless kobject_del() in free_msi_irqs() http://patchwork.ozlabs.org/patch/278202/ - Add a kobject_del() before each kset_unregister(dev->msi_kset) call. This will remove "msi_irqs" from sysfs, so future creates will succeed even if somebody still has the old "msi_irqs" open. I think it's done in your patch kobject: remove kset from sysfs immediately in kset_unregister() http://patchwork.ozlabs.org/patch/281618/ So it'll collide and use kobject_del() twice. Or did you actually drop your patch? - Keep the msi_kset cleanup in populate_msi_sysfs() instead of relying on free_msi_irqs(). I think it's less error prone to keep the creation and error path cleanup in the same function. It is less error prone, however the current design is that "once we fail something while creating irqs, always call free_msi_irqs()", so, if we add the msi_kset cleanup to populate_msi_sysfs() (it wasn't there before, so we can't 'keep' it) - we'll have to verify if it was don in free_msi_irqs(), cause free_msi_irqs() is being called not only on rollback in msi_capability_init(), but also in pci_disable_msi() and friends. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] msi: free msi_desc entry only after we've released the kobject
On Fri, Oct 04, 2013 at 10:46:31AM -0600, Bjorn Helgaas wrote: On Sat, Sep 28, 2013 at 3:37 PM, Veaceslav Falico vfal...@redhat.com 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. I talked to Greg KH about this recently, and he said he might take a look at doing a more extensive cleanup of populate_msi_sysfs() using attribute groups, so I don't know if you want to wait and see whether he does anything, or go ahead on the path you were on. Sorry for the delay, was sick. I'll continue going ahead, however if Greg/you don't really need it or are working on it - please say now, so I'll stop waisting your time. If you continue, my advice is: - Put all these patches in a single series with a version number (I think the next posting would be v3) to help me keep track of them. Will do, if/when there'll be next version. Now they're divided into 1 bugfix and 1 cleanup patchset. - In populate_msi_sysfs(), drop the pci_dev_get() (or explain why it's needed). My reasoning is that the msi_irqs kset should already hold a reference on the pdev (acquired in kset_create_and_add() - kset_register() - kobject_add_internal()), and each irq entry should hold a reference on the kset (see kobject_add_internal() again), so it is redundant to acquire a reference on the pdev directly. This means dropping the pci_dev_put() in msi_kobj_release(), of course. It's done in my patch pci: remove redundant pci_dev_get/put() on kobject (un)register http://patchwork.ozlabs.org/patch/278201/ - Move the kfree(entry) from free_msi_irqs() to msi_kobj_release() (I think one of your patches already did this). It's done in my patch msi: free msi_desc entry only after we've released the kobject http://patchwork.ozlabs.org/patch/278150/ - In populate_msi_sysfs(), drop the kobject_del() in the out_unroll loop. I think we would only need that if there were a way to create a new irq entry in msi_irqs before the old irq entry was released. But I don't think that's possible. We only create irq entries in populate_msi_sysfs(), which always starts with a fresh, empty msi_irqs kset. It's done in my patch msi: free msi_desc entry only after we've released the kobject http://patchwork.ozlabs.org/patch/278150/ - In free_msi_irqs(), similarly remove the kobject_del(). It's done in my patch msi: remove useless kobject_del() in free_msi_irqs() http://patchwork.ozlabs.org/patch/278202/ - Add a kobject_del() before each kset_unregister(dev-msi_kset) call. This will remove msi_irqs from sysfs, so future creates will succeed even if somebody still has the old msi_irqs open. I think it's done in your patch kobject: remove kset from sysfs immediately in kset_unregister() http://patchwork.ozlabs.org/patch/281618/ So it'll collide and use kobject_del() twice. Or did you actually drop your patch? - Keep the msi_kset cleanup in populate_msi_sysfs() instead of relying on free_msi_irqs(). I think it's less error prone to keep the creation and error path cleanup in the same function. It is less error prone, however the current design is that once we fail something while creating irqs, always call free_msi_irqs(), so, if we add the msi_kset cleanup to populate_msi_sysfs() (it wasn't there before, so we can't 'keep' it) - we'll have to verify if it was don in free_msi_irqs(), cause free_msi_irqs() is being called not only on rollback in msi_capability_init(), but also in pci_disable_msi() and friends. Bjorn -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] msi: free msi_desc entry only after we've released the kobject
On Sat, Sep 28, 2013 at 3:37 PM, Veaceslav Falico wrote: > On Thu, Sep 26, 2013 at 11:59:51AM +0200, Veaceslav Falico wrote: >> >> Currently, we first do kobject_put(>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. I talked to Greg KH about this recently, and he said he might take a look at doing a more extensive cleanup of populate_msi_sysfs() using attribute groups, so I don't know if you want to wait and see whether he does anything, or go ahead on the path you were on. If you continue, my advice is: - Put all these patches in a single series with a version number (I think the next posting would be v3) to help me keep track of them. - In populate_msi_sysfs(), drop the pci_dev_get() (or explain why it's needed). My reasoning is that the "msi_irqs" kset should already hold a reference on the pdev (acquired in kset_create_and_add() -> kset_register() -> kobject_add_internal()), and each irq entry should hold a reference on the kset (see kobject_add_internal() again), so it is redundant to acquire a reference on the pdev directly. This means dropping the pci_dev_put() in msi_kobj_release(), of course. - Move the kfree(entry) from free_msi_irqs() to msi_kobj_release() (I think one of your patches already did this). - In populate_msi_sysfs(), drop the kobject_del() in the out_unroll loop. I think we would only need that if there were a way to create a new irq entry in "msi_irqs" before the old irq entry was released. But I don't think that's possible. We only create irq entries in populate_msi_sysfs(), which always starts with a fresh, empty "msi_irqs" kset. - In free_msi_irqs(), similarly remove the kobject_del(). - Add a kobject_del() before each kset_unregister(dev->msi_kset) call. This will remove "msi_irqs" from sysfs, so future creates will succeed even if somebody still has the old "msi_irqs" open. - Keep the msi_kset cleanup in populate_msi_sysfs() instead of relying on free_msi_irqs(). I think it's less error prone to keep the creation and error path cleanup in the same function. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] msi: free msi_desc entry only after we've released the kobject
On Sat, Sep 28, 2013 at 3:37 PM, Veaceslav Falico vfal...@redhat.com 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. I talked to Greg KH about this recently, and he said he might take a look at doing a more extensive cleanup of populate_msi_sysfs() using attribute groups, so I don't know if you want to wait and see whether he does anything, or go ahead on the path you were on. If you continue, my advice is: - Put all these patches in a single series with a version number (I think the next posting would be v3) to help me keep track of them. - In populate_msi_sysfs(), drop the pci_dev_get() (or explain why it's needed). My reasoning is that the msi_irqs kset should already hold a reference on the pdev (acquired in kset_create_and_add() - kset_register() - kobject_add_internal()), and each irq entry should hold a reference on the kset (see kobject_add_internal() again), so it is redundant to acquire a reference on the pdev directly. This means dropping the pci_dev_put() in msi_kobj_release(), of course. - Move the kfree(entry) from free_msi_irqs() to msi_kobj_release() (I think one of your patches already did this). - In populate_msi_sysfs(), drop the kobject_del() in the out_unroll loop. I think we would only need that if there were a way to create a new irq entry in msi_irqs before the old irq entry was released. But I don't think that's possible. We only create irq entries in populate_msi_sysfs(), which always starts with a fresh, empty msi_irqs kset. - In free_msi_irqs(), similarly remove the kobject_del(). - Add a kobject_del() before each kset_unregister(dev-msi_kset) call. This will remove msi_irqs from sysfs, so future creates will succeed even if somebody still has the old msi_irqs open. - Keep the msi_kset cleanup in populate_msi_sysfs() instead of relying on free_msi_irqs(). I think it's less error prone to keep the creation and error path cleanup in the same function. Bjorn -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] msi: free msi_desc entry only after we've released the kobject
On Wed, Oct 2, 2013 at 2:41 PM, Russell King - ARM Linux
wrote:
> 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:
I think you're right. I think we need to keep the delayed release the
way it is, and pay more attention to the way we remove things from
sysfs.
>> 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.
Right. I think we might want to have kset_unregister() call
kobject_del() explicitly. That way the kset is removed from sysfs
immediately even if the release is delayed, and it is safe to create
another kset with the same name as soon as kset_register() returns.
Part of my confusion has been that I didn't connect "kobject_del()"
with the removal from sysfs (even though this is well-documented in
Documentation/kobject.txt). I think using kobject_del()an important
part of fixing not only this MSI issue, but also races in the PCI
device removal path. A name like "kobject_sysfs_unlink()" might be
more descriptive and help convey the idea that this is the point where
we prevent any future references via sysfs.
> 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
Thanks. I think I'm going to propose something like the patch below
instead. It solves the kset_create_and_add(), kset_unregister(),
kset_create_and_add() problem as well. I think we'll have to address
the MSI issue by adding kobject_del() in the appropriate places.
diff --git a/lib/kobject.c b/lib/kobject.c
index 9621751..9098992 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -753,6 +753,7 @@ void kset_unregister(struct kset *k)
{
if (!k)
return;
+ kobject_del(>kobj);
kobject_put(>kobj);
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] msi: free msi_desc entry only after we've released the kobject
On Wed, Oct 2, 2013 at 2:41 PM, Russell King - ARM Linux
li...@arm.linux.org.uk wrote:
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:
I think you're right. I think we need to keep the delayed release the
way it is, and pay more attention to the way we remove things from
sysfs.
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.
Right. I think we might want to have kset_unregister() call
kobject_del() explicitly. That way the kset is removed from sysfs
immediately even if the release is delayed, and it is safe to create
another kset with the same name as soon as kset_register() returns.
Part of my confusion has been that I didn't connect kobject_del()
with the removal from sysfs (even though this is well-documented in
Documentation/kobject.txt). I think using kobject_del()an important
part of fixing not only this MSI issue, but also races in the PCI
device removal path. A name like kobject_sysfs_unlink() might be
more descriptive and help convey the idea that this is the point where
we prevent any future references via sysfs.
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 rmk+ker...@arm.linux.org.uk
Thanks. I think I'm going to propose something like the patch below
instead. It solves the kset_create_and_add(), kset_unregister(),
kset_create_and_add() problem as well. I think we'll have to address
the MSI issue by adding kobject_del() in the appropriate places.
diff --git a/lib/kobject.c b/lib/kobject.c
index 9621751..9098992 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -753,6 +753,7 @@ void kset_unregister(struct kset *k)
{
if (!k)
return;
+ kobject_del(k-kobj);
kobject_put(k-kobj);
}
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] msi: free msi_desc entry only after we've released the kobject
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
Thanks Bjorn.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] msi: free msi_desc entry only after we've released the kobject
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 rmk+ker...@arm.linux.org.uk Thanks Bjorn. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] msi: free msi_desc entry only after we've released the kobject
[+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(>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:
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(>release, kobject_delayed_cleanup);
+ INIT_DELAYED_WORK(>release, kobject_delayed_free);
schedule_delayed_work(>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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the
Re: [PATCH v2] msi: free msi_desc entry only after we've released the kobject
[+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:
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.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] msi: free msi_desc entry only after we've released the kobject
On Thu, Sep 26, 2013 at 11:59:51AM +0200, Veaceslav Falico wrote:
Currently, we first do kobject_put(>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.
I'm really sorry for bugging - but I need this fix to get included for my
testing to work :(.
This fix fixes the critical bug when we free the entry with the kobject
.../msi_irqs/IRQ_NR still being used, however the issue with msi_irqs kset
itself still exists, and really is a different issue - and I'm trying to
find a fix for it.
Thank you, and sorry for the mess and the noise.
Fix this by using the kobject->release callback, which is called last when
the kobject is indeed not used and is cleaned up - it's msi_kobj_release(),
which can do the kfree(entry) safely (kobject_put/cleanup doesn't use the
kobj itself after ->release() was called, so we're safe).
In case we've failed to create the sysfs directories - just kfree()
it - cause we don't have the kobjects attached.
Also, remove the same functionality from populate_msi_sysfs(), cause on
failure we anyway call free_msi_irqs(), which will take care of all the
kobjects properly.
And add the forgotten pci_dev_put(pdev) in case of failure to register the
kobject in populate_msi_sysfs().
CC: Bjorn Helgaas
CC: Neil Horman
CC: Greg Kroah-Hartman
CC: linux-...@vger.kernel.org
CC: linux-kernel@vger.kernel.org
Signed-off-by: Veaceslav Falico
---
Notes:
v1 -> v2:
Make it as a standalone patch, which is a bugfix, and add the forgotten
pci_dev_put() so that it won't break bisecting. The pci_dev_put() will
go away anyway in the following patchset, which cleans removes
kobject_del and useless pci_dev_get/put(). Rebased on linux-pci/next.
drivers/pci/msi.c | 31 ---
1 file changed, 12 insertions(+), 19 deletions(-)
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index d5f90d6..5d70f49 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -374,19 +374,22 @@ static void free_msi_irqs(struct pci_dev *dev)
iounmap(entry->mask_base);
}
+ list_del(>list);
+
/*
* Its possible that we get into this path
* When populate_msi_sysfs fails, which means the entries
* were not registered with sysfs. In that case don't
-* unregister them.
+* unregister them, and just free. Otherwise the
+* kobject->release will take care of freeing the entry via
+* msi_kobj_release().
*/
if (entry->kobj.parent) {
kobject_del(>kobj);
kobject_put(>kobj);
+ } else {
+ kfree(entry);
}
-
- list_del(>list);
- kfree(entry);
}
}
@@ -509,6 +512,7 @@ static void msi_kobj_release(struct kobject *kobj)
struct msi_desc *entry = to_msi_desc(kobj);
pci_dev_put(entry->dev);
+ kfree(entry);
}
static struct kobj_type msi_irq_ktype = {
@@ -522,7 +526,6 @@ static int populate_msi_sysfs(struct pci_dev *pdev)
struct msi_desc *entry;
struct kobject *kobj;
int ret;
- int count = 0;
pdev->msi_kset = kset_create_and_add("msi_irqs", NULL, >dev.kobj);
if (!pdev->msi_kset)
@@ -534,23 +537,13 @@ static int populate_msi_sysfs(struct pci_dev *pdev)
pci_dev_get(pdev);
ret = kobject_init_and_add(kobj, _irq_ktype, NULL,
"%u", entry->irq);
- if (ret)
- goto out_unroll;
-
- count++;
+ if (ret) {
+ pci_dev_put(pdev);
+ return ret;
+ }
}
return 0;
-
-out_unroll:
- list_for_each_entry(entry, >msi_list, list) {
- if (!count)
- break;
- kobject_del(>kobj);
- kobject_put(>kobj);
- count--;
- }
- return ret;
}
/**
--
1.8.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] msi: free msi_desc entry only after we've released the kobject
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.
I'm really sorry for bugging - but I need this fix to get included for my
testing to work :(.
This fix fixes the critical bug when we free the entry with the kobject
.../msi_irqs/IRQ_NR still being used, however the issue with msi_irqs kset
itself still exists, and really is a different issue - and I'm trying to
find a fix for it.
Thank you, and sorry for the mess and the noise.
Fix this by using the kobject-release callback, which is called last when
the kobject is indeed not used and is cleaned up - it's msi_kobj_release(),
which can do the kfree(entry) safely (kobject_put/cleanup doesn't use the
kobj itself after -release() was called, so we're safe).
In case we've failed to create the sysfs directories - just kfree()
it - cause we don't have the kobjects attached.
Also, remove the same functionality from populate_msi_sysfs(), cause on
failure we anyway call free_msi_irqs(), which will take care of all the
kobjects properly.
And add the forgotten pci_dev_put(pdev) in case of failure to register the
kobject in populate_msi_sysfs().
CC: Bjorn Helgaas bhelg...@google.com
CC: Neil Horman nhor...@tuxdriver.com
CC: Greg Kroah-Hartman gre...@linuxfoundation.org
CC: linux-...@vger.kernel.org
CC: linux-kernel@vger.kernel.org
Signed-off-by: Veaceslav Falico vfal...@redhat.com
---
Notes:
v1 - v2:
Make it as a standalone patch, which is a bugfix, and add the forgotten
pci_dev_put() so that it won't break bisecting. The pci_dev_put() will
go away anyway in the following patchset, which cleans removes
kobject_del and useless pci_dev_get/put(). Rebased on linux-pci/next.
drivers/pci/msi.c | 31 ---
1 file changed, 12 insertions(+), 19 deletions(-)
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index d5f90d6..5d70f49 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -374,19 +374,22 @@ static void free_msi_irqs(struct pci_dev *dev)
iounmap(entry-mask_base);
}
+ list_del(entry-list);
+
/*
* Its possible that we get into this path
* When populate_msi_sysfs fails, which means the entries
* were not registered with sysfs. In that case don't
-* unregister them.
+* unregister them, and just free. Otherwise the
+* kobject-release will take care of freeing the entry via
+* msi_kobj_release().
*/
if (entry-kobj.parent) {
kobject_del(entry-kobj);
kobject_put(entry-kobj);
+ } else {
+ kfree(entry);
}
-
- list_del(entry-list);
- kfree(entry);
}
}
@@ -509,6 +512,7 @@ static void msi_kobj_release(struct kobject *kobj)
struct msi_desc *entry = to_msi_desc(kobj);
pci_dev_put(entry-dev);
+ kfree(entry);
}
static struct kobj_type msi_irq_ktype = {
@@ -522,7 +526,6 @@ static int populate_msi_sysfs(struct pci_dev *pdev)
struct msi_desc *entry;
struct kobject *kobj;
int ret;
- int count = 0;
pdev-msi_kset = kset_create_and_add(msi_irqs, NULL, pdev-dev.kobj);
if (!pdev-msi_kset)
@@ -534,23 +537,13 @@ static int populate_msi_sysfs(struct pci_dev *pdev)
pci_dev_get(pdev);
ret = kobject_init_and_add(kobj, msi_irq_ktype, NULL,
%u, entry-irq);
- if (ret)
- goto out_unroll;
-
- count++;
+ if (ret) {
+ pci_dev_put(pdev);
+ return ret;
+ }
}
return 0;
-
-out_unroll:
- list_for_each_entry(entry, pdev-msi_list, list) {
- if (!count)
- break;
- kobject_del(entry-kobj);
- kobject_put(entry-kobj);
- count--;
- }
- return ret;
}
/**
--
1.8.4
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] msi: free msi_desc entry only after we've released the kobject
On Thu, Sep 26, 2013 at 11:59:51AM +0200, Veaceslav Falico wrote: > Currently, we first do kobject_put(>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. > > Fix this by using the kobject->release callback, which is called last when > the kobject is indeed not used and is cleaned up - it's msi_kobj_release(), > which can do the kfree(entry) safely (kobject_put/cleanup doesn't use the > kobj itself after ->release() was called, so we're safe). > > In case we've failed to create the sysfs directories - just kfree() > it - cause we don't have the kobjects attached. > > Also, remove the same functionality from populate_msi_sysfs(), cause on > failure we anyway call free_msi_irqs(), which will take care of all the > kobjects properly. > > And add the forgotten pci_dev_put(pdev) in case of failure to register the > kobject in populate_msi_sysfs(). > > CC: Bjorn Helgaas > CC: Neil Horman > CC: Greg Kroah-Hartman > CC: linux-...@vger.kernel.org > CC: linux-kernel@vger.kernel.org > Signed-off-by: Veaceslav Falico > --- > Acked-by: Neil Horman -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] msi: free msi_desc entry only after we've released the kobject
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. Fix this by using the kobject-release callback, which is called last when the kobject is indeed not used and is cleaned up - it's msi_kobj_release(), which can do the kfree(entry) safely (kobject_put/cleanup doesn't use the kobj itself after -release() was called, so we're safe). In case we've failed to create the sysfs directories - just kfree() it - cause we don't have the kobjects attached. Also, remove the same functionality from populate_msi_sysfs(), cause on failure we anyway call free_msi_irqs(), which will take care of all the kobjects properly. And add the forgotten pci_dev_put(pdev) in case of failure to register the kobject in populate_msi_sysfs(). CC: Bjorn Helgaas bhelg...@google.com CC: Neil Horman nhor...@tuxdriver.com CC: Greg Kroah-Hartman gre...@linuxfoundation.org CC: linux-...@vger.kernel.org CC: linux-kernel@vger.kernel.org Signed-off-by: Veaceslav Falico vfal...@redhat.com --- Acked-by: Neil Horman nhor...@tuxdriver.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
