| Message ID | alpine.LRH.2.02.1401062249020.17811@file01.intranet.prod.int.rdu2.redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
This looks completely broken to me. You do a "kobject_put()" and then
after you've dropped that last use, you wait for the completion of
something that may already have been free'd.
Wtf? Am I missing something?
Linus
On Tue, Jan 7, 2014 at 12:01 PM, Mikulas Patocka <mpatocka@redhat.com> wrote:
>
>
> On Mon, 6 Jan 2014, Mike Snitzer wrote:
>
>> On Mon, Jan 06 2014 at 1:55pm -0500,
>> Mikulas Patocka <mpatocka@redhat.com> wrote:
>>
>> >
>> >
>> > On Sun, 5 Jan 2014, Greg Kroah-Hartman wrote:
>> >
>> > > On Sun, Jan 05, 2014 at 05:43:56PM +0100, Bart Van Assche wrote:
>> > > > On 01/04/14 19:06, Mikulas Patocka wrote:
>> > > > > - if (t && !t->release)
>> > > > > - pr_debug("kobject: '%s' (%p): does not have a release() "
>> > > > > - "function, it is broken and must be fixed.\n",
>> > > > > - kobject_name(kobj), kobj);
>> > > > > -
>> > > >
>> > > > Has it been considered to issue a warning if no release function has
>> > > > been defined and free_completion == NULL instead of removing the above
>> > > > debug message entirely ? I think even with this patch applied it is
>> > > > still wrong to invoke kobject_put() on an object without defining a
>> > > > release function.
>> > >
>> > > This patch isn't going to be applied, and I've reverted the original
>> > > commit, so there shouldn't be any issues anymore with this code.
>> >
>> > Why? This patch does the same thing as
>> > eee031649707db3c9920d9498f8d03819b74fc23, but it's smaller. So why did you
>> > accept eee031649707db3c9920d9498f8d03819b74fc23 and not this?
>> >
>> > The code to wait for kobject destruction using completion already exists
>> > in cpufreq_sysfs_release, cpuidle_sysfs_release,
>> > cpuidle_state_sysfs_release, cpuidle_driver_sysfs_release,
>> > ext4_sb_release, ext4_feat_release, f2fs_sb_release (these are the only
>> > kobject users that are correct w.r.t. module unloading), so if you accept
>> > this patch, you can simplify them to use kobject_put_wait.
>>
>> Hi Mikulas,
>>
>> Please just submit a DM-only patch that follows the same racey pattern
>> of firing a completion from the kobj_type .release method in dm_mod.
>> I'll get it queued up for 3.14.
>>
>> If/when we gets reports of a crash due to dm_mod unload racing with
>> kobject_put we can revisit this.
>>
>> Thanks,
>> Mike
>
> Here I'm sending dm-only patch.
>
>
>
> dm: wait until kobject is destroyed
>
> There may be other parts of the kernel taking reference to the dm kobject.
> We must wait until they drop the references before deallocating the md
> structure.
>
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> Cc: stable@vger.kernel.org
>
> ---
> drivers/md/dm-sysfs.c | 10 +++++++++-
> drivers/md/dm.c | 11 +++++++++++
> drivers/md/dm.h | 2 ++
> 3 files changed, 22 insertions(+), 1 deletion(-)
>
> Index: linux-3.13-rc7/drivers/md/dm-sysfs.c
> ===================================================================
> --- linux-3.13-rc7.orig/drivers/md/dm-sysfs.c 2014-01-07 02:06:08.000000000 +0100
> +++ linux-3.13-rc7/drivers/md/dm-sysfs.c 2014-01-07 02:07:09.000000000 +0100
> @@ -79,6 +79,11 @@ static const struct sysfs_ops dm_sysfs_o
> .show = dm_attr_show,
> };
>
> +static void dm_kobject_release(struct kobject *kobj)
> +{
> + complete(dm_get_completion_from_kobject(kobj));
> +}
> +
> /*
> * dm kobject is embedded in mapped_device structure
> * no need to define release function here
> @@ -86,6 +91,7 @@ static const struct sysfs_ops dm_sysfs_o
> static struct kobj_type dm_ktype = {
> .sysfs_ops = &dm_sysfs_ops,
> .default_attrs = dm_attrs,
> + .release = dm_kobject_release,
> };
>
> /*
> @@ -104,5 +110,7 @@ int dm_sysfs_init(struct mapped_device *
> */
> void dm_sysfs_exit(struct mapped_device *md)
> {
> - kobject_put(dm_kobject(md));
> + struct kobject *kobj = dm_kobject(md);
> + kobject_put(kobj);
> + wait_for_completion(dm_get_completion_from_kobject(kobj));
> }
> Index: linux-3.13-rc7/drivers/md/dm.c
> ===================================================================
> --- linux-3.13-rc7.orig/drivers/md/dm.c 2014-01-07 02:07:09.000000000 +0100
> +++ linux-3.13-rc7/drivers/md/dm.c 2014-01-07 04:58:37.000000000 +0100
> @@ -203,6 +203,9 @@ struct mapped_device {
> /* sysfs handle */
> struct kobject kobj;
>
> + /* wait until the kobject is released */
> + struct completion kobj_completion;
> +
> /* zero-length flush that will be cloned and submitted to targets */
> struct bio flush_bio;
>
> @@ -2049,6 +2052,7 @@ static struct mapped_device *alloc_dev(i
> init_waitqueue_head(&md->wait);
> INIT_WORK(&md->work, dm_wq_work);
> init_waitqueue_head(&md->eventq);
> + init_completion(&md->kobj_completion);
>
> md->disk->major = _major;
> md->disk->first_minor = minor;
> @@ -2931,6 +2935,13 @@ struct mapped_device *dm_get_from_kobjec
> return md;
> }
>
> +struct completion *dm_get_completion_from_kobject(struct kobject *kobj)
> +{
> + struct mapped_device *md = container_of(kobj, struct mapped_device, kobj);
> +
> + return &md->kobj_completion;
> +}
> +
> int dm_suspended_md(struct mapped_device *md)
> {
> return test_bit(DMF_SUSPENDED, &md->flags);
> Index: linux-3.13-rc7/drivers/md/dm.h
> ===================================================================
> --- linux-3.13-rc7.orig/drivers/md/dm.h 2014-01-07 02:06:08.000000000 +0100
> +++ linux-3.13-rc7/drivers/md/dm.h 2014-01-07 02:07:09.000000000 +0100
> @@ -15,6 +15,7 @@
> #include <linux/list.h>
> #include <linux/blkdev.h>
> #include <linux/hdreg.h>
> +#include <linux/completion.h>
>
> #include "dm-stats.h"
>
> @@ -152,6 +153,7 @@ int dm_sysfs_init(struct mapped_device *
> void dm_sysfs_exit(struct mapped_device *md);
> struct kobject *dm_kobject(struct mapped_device *md);
> struct mapped_device *dm_get_from_kobject(struct kobject *kobj);
> +struct completion *dm_get_completion_from_kobject(struct kobject *kobj);
>
> /*
> * Targets for linear and striped mappings
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
On Mon, Jan 06, 2014 at 11:01:22PM -0500, Mikulas Patocka wrote: > > > On Mon, 6 Jan 2014, Mike Snitzer wrote: > > > On Mon, Jan 06 2014 at 1:55pm -0500, > > Mikulas Patocka <mpatocka@redhat.com> wrote: > > > > > > > > > > > On Sun, 5 Jan 2014, Greg Kroah-Hartman wrote: > > > > > > > On Sun, Jan 05, 2014 at 05:43:56PM +0100, Bart Van Assche wrote: > > > > > On 01/04/14 19:06, Mikulas Patocka wrote: > > > > > > - if (t && !t->release) > > > > > > - pr_debug("kobject: '%s' (%p): does not have a release() " > > > > > > - "function, it is broken and must be fixed.\n", > > > > > > - kobject_name(kobj), kobj); > > > > > > - > > > > > > > > > > Has it been considered to issue a warning if no release function has > > > > > been defined and free_completion == NULL instead of removing the above > > > > > debug message entirely ? I think even with this patch applied it is > > > > > still wrong to invoke kobject_put() on an object without defining a > > > > > release function. > > > > > > > > This patch isn't going to be applied, and I've reverted the original > > > > commit, so there shouldn't be any issues anymore with this code. > > > > > > Why? This patch does the same thing as > > > eee031649707db3c9920d9498f8d03819b74fc23, but it's smaller. So why did you > > > accept eee031649707db3c9920d9498f8d03819b74fc23 and not this? > > > > > > The code to wait for kobject destruction using completion already exists > > > in cpufreq_sysfs_release, cpuidle_sysfs_release, > > > cpuidle_state_sysfs_release, cpuidle_driver_sysfs_release, > > > ext4_sb_release, ext4_feat_release, f2fs_sb_release (these are the only > > > kobject users that are correct w.r.t. module unloading), so if you accept > > > this patch, you can simplify them to use kobject_put_wait. > > > > Hi Mikulas, > > > > Please just submit a DM-only patch that follows the same racey pattern > > of firing a completion from the kobj_type .release method in dm_mod. > > I'll get it queued up for 3.14. > > > > If/when we gets reports of a crash due to dm_mod unload racing with > > kobject_put we can revisit this. > > > > Thanks, > > Mike > > Here I'm sending dm-only patch. > > > > dm: wait until kobject is destroyed > > There may be other parts of the kernel taking reference to the dm kobject. > We must wait until they drop the references before deallocating the md > structure. > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> > Cc: stable@vger.kernel.org > > --- > drivers/md/dm-sysfs.c | 10 +++++++++- > drivers/md/dm.c | 11 +++++++++++ > drivers/md/dm.h | 2 ++ > 3 files changed, 22 insertions(+), 1 deletion(-) > > Index: linux-3.13-rc7/drivers/md/dm-sysfs.c > =================================================================== > --- linux-3.13-rc7.orig/drivers/md/dm-sysfs.c 2014-01-07 02:06:08.000000000 +0100 > +++ linux-3.13-rc7/drivers/md/dm-sysfs.c 2014-01-07 02:07:09.000000000 +0100 > @@ -79,6 +79,11 @@ static const struct sysfs_ops dm_sysfs_o > .show = dm_attr_show, > }; > > +static void dm_kobject_release(struct kobject *kobj) > +{ > + complete(dm_get_completion_from_kobject(kobj)); > +} Please read the kobject documentation in the kernel tree for why this isn't ok. The fact that you didn't have a release function at all means this code has always been broken, why have you been ignoring the kernel complaining about this for so long before? You need to free the memory in the release function, not just sit around and wait for potentially forever. thanks, greg k-h -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Tue, 7 Jan 2014, Linus Torvalds wrote: > This looks completely broken to me. You do a "kobject_put()" and then > after you've dropped that last use, you wait for the completion of > something that may already have been free'd. > > Wtf? Am I missing something? > > Linus It is correct. The release method dm_kobject_release doesn't free the kobject. It just signals the completion and returns. This is the sequence of operations: * call kobject_put * wait until all users stop using the kobject, when it happens the release method is called * the release method signals the completion and returns * the unload code that waits on the completion continues * the unload code frees the mapped_device structure that contains the kobject Using kobject this way avoids the module unload race that was mentioned at the beginning of this thread. Mikulas > On Tue, Jan 7, 2014 at 12:01 PM, Mikulas Patocka <mpatocka@redhat.com> wrote: > > > > > > On Mon, 6 Jan 2014, Mike Snitzer wrote: > > > >> On Mon, Jan 06 2014 at 1:55pm -0500, > >> Mikulas Patocka <mpatocka@redhat.com> wrote: > >> > >> > > >> > > >> > On Sun, 5 Jan 2014, Greg Kroah-Hartman wrote: > >> > > >> > > On Sun, Jan 05, 2014 at 05:43:56PM +0100, Bart Van Assche wrote: > >> > > > On 01/04/14 19:06, Mikulas Patocka wrote: > >> > > > > - if (t && !t->release) > >> > > > > - pr_debug("kobject: '%s' (%p): does not have a release() " > >> > > > > - "function, it is broken and must be fixed.\n", > >> > > > > - kobject_name(kobj), kobj); > >> > > > > - > >> > > > > >> > > > Has it been considered to issue a warning if no release function has > >> > > > been defined and free_completion == NULL instead of removing the above > >> > > > debug message entirely ? I think even with this patch applied it is > >> > > > still wrong to invoke kobject_put() on an object without defining a > >> > > > release function. > >> > > > >> > > This patch isn't going to be applied, and I've reverted the original > >> > > commit, so there shouldn't be any issues anymore with this code. > >> > > >> > Why? This patch does the same thing as > >> > eee031649707db3c9920d9498f8d03819b74fc23, but it's smaller. So why did you > >> > accept eee031649707db3c9920d9498f8d03819b74fc23 and not this? > >> > > >> > The code to wait for kobject destruction using completion already exists > >> > in cpufreq_sysfs_release, cpuidle_sysfs_release, > >> > cpuidle_state_sysfs_release, cpuidle_driver_sysfs_release, > >> > ext4_sb_release, ext4_feat_release, f2fs_sb_release (these are the only > >> > kobject users that are correct w.r.t. module unloading), so if you accept > >> > this patch, you can simplify them to use kobject_put_wait. > >> > >> Hi Mikulas, > >> > >> Please just submit a DM-only patch that follows the same racey pattern > >> of firing a completion from the kobj_type .release method in dm_mod. > >> I'll get it queued up for 3.14. > >> > >> If/when we gets reports of a crash due to dm_mod unload racing with > >> kobject_put we can revisit this. > >> > >> Thanks, > >> Mike > > > > Here I'm sending dm-only patch. > > > > > > > > dm: wait until kobject is destroyed > > > > There may be other parts of the kernel taking reference to the dm kobject. > > We must wait until they drop the references before deallocating the md > > structure. > > > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> > > Cc: stable@vger.kernel.org > > > > --- > > drivers/md/dm-sysfs.c | 10 +++++++++- > > drivers/md/dm.c | 11 +++++++++++ > > drivers/md/dm.h | 2 ++ > > 3 files changed, 22 insertions(+), 1 deletion(-) > > > > Index: linux-3.13-rc7/drivers/md/dm-sysfs.c > > =================================================================== > > --- linux-3.13-rc7.orig/drivers/md/dm-sysfs.c 2014-01-07 02:06:08.000000000 +0100 > > +++ linux-3.13-rc7/drivers/md/dm-sysfs.c 2014-01-07 02:07:09.000000000 +0100 > > @@ -79,6 +79,11 @@ static const struct sysfs_ops dm_sysfs_o > > .show = dm_attr_show, > > }; > > > > +static void dm_kobject_release(struct kobject *kobj) > > +{ > > + complete(dm_get_completion_from_kobject(kobj)); > > +} > > + > > /* > > * dm kobject is embedded in mapped_device structure > > * no need to define release function here > > @@ -86,6 +91,7 @@ static const struct sysfs_ops dm_sysfs_o > > static struct kobj_type dm_ktype = { > > .sysfs_ops = &dm_sysfs_ops, > > .default_attrs = dm_attrs, > > + .release = dm_kobject_release, > > }; > > > > /* > > @@ -104,5 +110,7 @@ int dm_sysfs_init(struct mapped_device * > > */ > > void dm_sysfs_exit(struct mapped_device *md) > > { > > - kobject_put(dm_kobject(md)); > > + struct kobject *kobj = dm_kobject(md); > > + kobject_put(kobj); > > + wait_for_completion(dm_get_completion_from_kobject(kobj)); > > } > > Index: linux-3.13-rc7/drivers/md/dm.c > > =================================================================== > > --- linux-3.13-rc7.orig/drivers/md/dm.c 2014-01-07 02:07:09.000000000 +0100 > > +++ linux-3.13-rc7/drivers/md/dm.c 2014-01-07 04:58:37.000000000 +0100 > > @@ -203,6 +203,9 @@ struct mapped_device { > > /* sysfs handle */ > > struct kobject kobj; > > > > + /* wait until the kobject is released */ > > + struct completion kobj_completion; > > + > > /* zero-length flush that will be cloned and submitted to targets */ > > struct bio flush_bio; > > > > @@ -2049,6 +2052,7 @@ static struct mapped_device *alloc_dev(i > > init_waitqueue_head(&md->wait); > > INIT_WORK(&md->work, dm_wq_work); > > init_waitqueue_head(&md->eventq); > > + init_completion(&md->kobj_completion); > > > > md->disk->major = _major; > > md->disk->first_minor = minor; > > @@ -2931,6 +2935,13 @@ struct mapped_device *dm_get_from_kobjec > > return md; > > } > > > > +struct completion *dm_get_completion_from_kobject(struct kobject *kobj) > > +{ > > + struct mapped_device *md = container_of(kobj, struct mapped_device, kobj); > > + > > + return &md->kobj_completion; > > +} > > + > > int dm_suspended_md(struct mapped_device *md) > > { > > return test_bit(DMF_SUSPENDED, &md->flags); > > Index: linux-3.13-rc7/drivers/md/dm.h > > =================================================================== > > --- linux-3.13-rc7.orig/drivers/md/dm.h 2014-01-07 02:06:08.000000000 +0100 > > +++ linux-3.13-rc7/drivers/md/dm.h 2014-01-07 02:07:09.000000000 +0100 > > @@ -15,6 +15,7 @@ > > #include <linux/list.h> > > #include <linux/blkdev.h> > > #include <linux/hdreg.h> > > +#include <linux/completion.h> > > > > #include "dm-stats.h" > > > > @@ -152,6 +153,7 @@ int dm_sysfs_init(struct mapped_device * > > void dm_sysfs_exit(struct mapped_device *md); > > struct kobject *dm_kobject(struct mapped_device *md); > > struct mapped_device *dm_get_from_kobject(struct kobject *kobj); > > +struct completion *dm_get_completion_from_kobject(struct kobject *kobj); > > > > /* > > * Targets for linear and striped mappings > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Tue, 7 Jan 2014, Greg Kroah-Hartman wrote: > > Index: linux-3.13-rc7/drivers/md/dm-sysfs.c > > =================================================================== > > --- linux-3.13-rc7.orig/drivers/md/dm-sysfs.c 2014-01-07 02:06:08.000000000 +0100 > > +++ linux-3.13-rc7/drivers/md/dm-sysfs.c 2014-01-07 02:07:09.000000000 +0100 > > @@ -79,6 +79,11 @@ static const struct sysfs_ops dm_sysfs_o > > .show = dm_attr_show, > > }; > > > > +static void dm_kobject_release(struct kobject *kobj) > > +{ > > + complete(dm_get_completion_from_kobject(kobj)); > > +} > > Please read the kobject documentation in the kernel tree for why this > isn't ok. The fact that you didn't have a release function at all means > this code has always been broken, why have you been ignoring the kernel > complaining about this for so long before? I read that file and I say that the usage pattern presented in that file is broken w.r.t. module unload. I don't want to fix one race condition and introduce another one (albeit smaller). > You need to free the memory in the release function, not just sit around > and wait for potentially forever. How could that code wait forever? Even if userspace keeps the appropriate files in sysfs open, the dm device can be unloaded, it doesn't wait forever. > thanks, > > greg k-h Mikulas -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Tue, Jan 07, 2014 at 06:16:22AM -0800, Greg Kroah-Hartman wrote: > On Mon, Jan 06, 2014 at 11:01:22PM -0500, Mikulas Patocka wrote: > > > > > > On Mon, 6 Jan 2014, Mike Snitzer wrote: > > > > > On Mon, Jan 06 2014 at 1:55pm -0500, > > > Mikulas Patocka <mpatocka@redhat.com> wrote: > > > > > > > > > > > > > > > On Sun, 5 Jan 2014, Greg Kroah-Hartman wrote: > > > > > > > > > On Sun, Jan 05, 2014 at 05:43:56PM +0100, Bart Van Assche wrote: > > > > > > On 01/04/14 19:06, Mikulas Patocka wrote: > > > > > > > - if (t && !t->release) > > > > > > > - pr_debug("kobject: '%s' (%p): does not have a release() " > > > > > > > - "function, it is broken and must be fixed.\n", > > > > > > > - kobject_name(kobj), kobj); > > > > > > > - > > > > > > > > > > > > Has it been considered to issue a warning if no release function has > > > > > > been defined and free_completion == NULL instead of removing the above > > > > > > debug message entirely ? I think even with this patch applied it is > > > > > > still wrong to invoke kobject_put() on an object without defining a > > > > > > release function. > > > > > > > > > > This patch isn't going to be applied, and I've reverted the original > > > > > commit, so there shouldn't be any issues anymore with this code. > > > > > > > > Why? This patch does the same thing as > > > > eee031649707db3c9920d9498f8d03819b74fc23, but it's smaller. So why did you > > > > accept eee031649707db3c9920d9498f8d03819b74fc23 and not this? > > > > > > > > The code to wait for kobject destruction using completion already exists > > > > in cpufreq_sysfs_release, cpuidle_sysfs_release, > > > > cpuidle_state_sysfs_release, cpuidle_driver_sysfs_release, > > > > ext4_sb_release, ext4_feat_release, f2fs_sb_release (these are the only > > > > kobject users that are correct w.r.t. module unloading), so if you accept > > > > this patch, you can simplify them to use kobject_put_wait. > > > > > > Hi Mikulas, > > > > > > Please just submit a DM-only patch that follows the same racey pattern > > > of firing a completion from the kobj_type .release method in dm_mod. > > > I'll get it queued up for 3.14. > > > > > > If/when we gets reports of a crash due to dm_mod unload racing with > > > kobject_put we can revisit this. > > > > > > Thanks, > > > Mike > > > > Here I'm sending dm-only patch. > > > > > > > > dm: wait until kobject is destroyed > > > > There may be other parts of the kernel taking reference to the dm kobject. > > We must wait until they drop the references before deallocating the md > > structure. > > > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> > > Cc: stable@vger.kernel.org > > > > --- > > drivers/md/dm-sysfs.c | 10 +++++++++- > > drivers/md/dm.c | 11 +++++++++++ > > drivers/md/dm.h | 2 ++ > > 3 files changed, 22 insertions(+), 1 deletion(-) > > > > Index: linux-3.13-rc7/drivers/md/dm-sysfs.c > > =================================================================== > > --- linux-3.13-rc7.orig/drivers/md/dm-sysfs.c 2014-01-07 02:06:08.000000000 +0100 > > +++ linux-3.13-rc7/drivers/md/dm-sysfs.c 2014-01-07 02:07:09.000000000 +0100 > > @@ -79,6 +79,11 @@ static const struct sysfs_ops dm_sysfs_o > > .show = dm_attr_show, > > }; > > > > +static void dm_kobject_release(struct kobject *kobj) > > +{ > > + complete(dm_get_completion_from_kobject(kobj)); > > +} > > Please read the kobject documentation in the kernel tree for why this > isn't ok. If documentation says that this is not allowed then we need to fix documentation. > The fact that you didn't have a release function at all means > this code has always been broken, why have you been ignoring the kernel > complaining about this for so long before? > > You need to free the memory in the release function, not just sit around > and wait for potentially forever. Why? I understand that normally freeing is what's happening but not necessarily. Release is simply called when last reference to the [k]object is dropped, that's it. Saying that every release function has to free memory is just a cargo-cult programming to me. We already have (as far as I can see) correct examples of release functions not freeing memory: fs/char_dev.c::cdev_default_release(). Thanks.
On Tue, Jan 07 2014 at 1:00pm -0500, Mikulas Patocka <mpatocka@redhat.com> wrote: > > > On Tue, 7 Jan 2014, Linus Torvalds wrote: > > > This looks completely broken to me. You do a "kobject_put()" and then > > after you've dropped that last use, you wait for the completion of > > something that may already have been free'd. > > > > Wtf? Am I missing something? > > > > Linus > > It is correct. The release method dm_kobject_release doesn't free the > kobject. It just signals the completion and returns. > > This is the sequence of operations: > * call kobject_put > * wait until all users stop using the kobject, when it happens the release > method is called > * the release method signals the completion and returns > * the unload code that waits on the completion continues > * the unload code frees the mapped_device structure that contains the > kobject > > Using kobject this way avoids the module unload race that was mentioned at > the beginning of this thread. I've staged your patch in linux-next for 3.14, see: http://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=for-next&id=af7b1e5c767fc895788c971c8f4686402ac8344f -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Tue, 7 Jan 2014, Mike Snitzer wrote: > On Tue, Jan 07 2014 at 1:00pm -0500, > Mikulas Patocka <mpatocka@redhat.com> wrote: > > > > > > > On Tue, 7 Jan 2014, Linus Torvalds wrote: > > > > > This looks completely broken to me. You do a "kobject_put()" and then > > > after you've dropped that last use, you wait for the completion of > > > something that may already have been free'd. > > > > > > Wtf? Am I missing something? > > > > > > Linus > > > > It is correct. The release method dm_kobject_release doesn't free the > > kobject. It just signals the completion and returns. > > > > This is the sequence of operations: > > * call kobject_put > > * wait until all users stop using the kobject, when it happens the release > > method is called > > * the release method signals the completion and returns > > * the unload code that waits on the completion continues > > * the unload code frees the mapped_device structure that contains the > > kobject > > > > Using kobject this way avoids the module unload race that was mentioned at > > the beginning of this thread. > > I've staged your patch in linux-next for 3.14, see: > http://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=for-next&id=af7b1e5c767fc895788c971c8f4686402ac8344f Looking at this patch, I realize that it is buggy too. If module unload happens at this point (after the completion is signaled, but before the release function returns), it crashes. static void dm_kobject_release(struct kobject *kobj) { complete(dm_get_completion_from_kobject(kobj)); >========== module unload here ===============< } The patch that I sent initially in this thread doesn't have this bug because the completion is signaled in non-module code. That goes back to my initial claim - it is impossible to use the kobject interface correctly! But if Greg doesn't want a patch that fixes the kobject interface, I don't really know what to do about it. Maybe another possibility - maintain a list of all kobjects and walk them in the generic module unload code to check if their release method ponits to the module that is being unloaded? Greg, would you accept a patch like this? Mikulas -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Tue, Jan 07 2014 at 3:16pm -0500, Mikulas Patocka <mpatocka@redhat.com> wrote: > On Tue, 7 Jan 2014, Mike Snitzer wrote: > > > I've staged your patch in linux-next for 3.14, see: > > http://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=for-next&id=af7b1e5c767fc895788c971c8f4686402ac8344f > > Looking at this patch, I realize that it is buggy too. If module unload > happens at this point (after the completion is signaled, but before the > release function returns), it crashes. > > static void dm_kobject_release(struct kobject *kobj) > { > complete(dm_get_completion_from_kobject(kobj)); > >========== module unload here ===============< > } That race isn't a surprise, thought you pointed it out earlier in this thread? Anyway, I'm inclined to keep the racey patch staged in linux-dm.git until the stalemate with gregkh is resolved. DM is no worse than other code that follows this same wait for completion pattern. > The patch that I sent initially in this thread doesn't have this bug > because the completion is signaled in non-module code. > > That goes back to my initial claim - it is impossible to use the kobject > interface correctly! But if Greg doesn't want a patch that fixes the > kobject interface, I don't really know what to do about it. Right, I'm missing what is wrong with your proposed kobject_put_wait interface. Greg, can you please establish that you understand the problem, and existing kobject patterns, before you dismiss a fix? -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Index: linux-3.13-rc7/drivers/md/dm-sysfs.c =================================================================== --- linux-3.13-rc7.orig/drivers/md/dm-sysfs.c 2014-01-07 02:06:08.000000000 +0100 +++ linux-3.13-rc7/drivers/md/dm-sysfs.c 2014-01-07 02:07:09.000000000 +0100 @@ -79,6 +79,11 @@ static const struct sysfs_ops dm_sysfs_o .show = dm_attr_show, }; +static void dm_kobject_release(struct kobject *kobj) +{ + complete(dm_get_completion_from_kobject(kobj)); +} + /* * dm kobject is embedded in mapped_device structure * no need to define release function here @@ -86,6 +91,7 @@ static const struct sysfs_ops dm_sysfs_o static struct kobj_type dm_ktype = { .sysfs_ops = &dm_sysfs_ops, .default_attrs = dm_attrs, + .release = dm_kobject_release, }; /* @@ -104,5 +110,7 @@ int dm_sysfs_init(struct mapped_device * */ void dm_sysfs_exit(struct mapped_device *md) { - kobject_put(dm_kobject(md)); + struct kobject *kobj = dm_kobject(md); + kobject_put(kobj); + wait_for_completion(dm_get_completion_from_kobject(kobj)); } Index: linux-3.13-rc7/drivers/md/dm.c =================================================================== --- linux-3.13-rc7.orig/drivers/md/dm.c 2014-01-07 02:07:09.000000000 +0100 +++ linux-3.13-rc7/drivers/md/dm.c 2014-01-07 04:58:37.000000000 +0100 @@ -203,6 +203,9 @@ struct mapped_device { /* sysfs handle */ struct kobject kobj; + /* wait until the kobject is released */ + struct completion kobj_completion; + /* zero-length flush that will be cloned and submitted to targets */ struct bio flush_bio; @@ -2049,6 +2052,7 @@ static struct mapped_device *alloc_dev(i init_waitqueue_head(&md->wait); INIT_WORK(&md->work, dm_wq_work); init_waitqueue_head(&md->eventq); + init_completion(&md->kobj_completion); md->disk->major = _major; md->disk->first_minor = minor; @@ -2931,6 +2935,13 @@ struct mapped_device *dm_get_from_kobjec return md; } +struct completion *dm_get_completion_from_kobject(struct kobject *kobj) +{ + struct mapped_device *md = container_of(kobj, struct mapped_device, kobj); + + return &md->kobj_completion; +} + int dm_suspended_md(struct mapped_device *md) { return test_bit(DMF_SUSPENDED, &md->flags); Index: linux-3.13-rc7/drivers/md/dm.h =================================================================== --- linux-3.13-rc7.orig/drivers/md/dm.h 2014-01-07 02:06:08.000000000 +0100 +++ linux-3.13-rc7/drivers/md/dm.h 2014-01-07 02:07:09.000000000 +0100 @@ -15,6 +15,7 @@ #include <linux/list.h> #include <linux/blkdev.h> #include <linux/hdreg.h> +#include <linux/completion.h> #include "dm-stats.h" @@ -152,6 +153,7 @@ int dm_sysfs_init(struct mapped_device * void dm_sysfs_exit(struct mapped_device *md); struct kobject *dm_kobject(struct mapped_device *md); struct mapped_device *dm_get_from_kobject(struct kobject *kobj); +struct completion *dm_get_completion_from_kobject(struct kobject *kobj); /* * Targets for linear and striped mappings