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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > Cc: stable@xxxxxxxxxxxxxxx
> >
> > ---
> > 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.
--
Dmitry
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
