linux-kernel - Re: kobject: provide kobject_put

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Tue, 7 Jan 2014 10:26:23 -0800
From:	Dmitry Torokhov <dmitry.torokhov@...il.com>
To:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:	Mikulas Patocka <mpatocka@...hat.com>,
	Mike Snitzer <snitzer@...hat.com>,
	Bart Van Assche <bvanassche@....org>,
	Jeff Mahoney <jeffm@...e.com>, linux-kernel@...r.kernel.org,
	device-mapper development <dm-devel@...hat.com>,
	tglx@...utronix.de, paulmck@...ux.vnet.ibm.com,
	torvalds@...ux-foundation.org, mingo@...nel.org
Subject: Re: kobject: provide kobject_put_wait to fix module unload race

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@...hat.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@...hat.com>
> > Cc: stable@...r.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.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/