linux-kernel - Re: [PATCH 8/19]: SCST SYSFS interface implementation

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, 12 Oct 2010 12:03:45 -0700
From:	Greg KH <greg@...ah.com>
To:	Vladislav Bolkhovitin <vst@...b.net>
Cc:	linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org,
	scst-devel <scst-devel@...ts.sourceforge.net>,
	James Bottomley <James.Bottomley@...senPartnership.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	FUJITA Tomonori <fujita.tomonori@....ntt.co.jp>,
	Mike Christie <michaelc@...wisc.edu>,
	Vu Pham <vuhuong@...lanox.com>,
	Bart Van Assche <bart.vanassche@...il.com>,
	James Smart <James.Smart@...lex.Com>,
	Joe Eykholt <jeykholt@...co.com>, Andy Yan <ayan@...vell.com>,
	Chetan Loke <generationgnu@...oo.com>,
	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Hannes Reinecke <hare@...e.de>,
	Richard Sharpe <realrichardsharpe@...il.com>,
	Daniel Henrique Debonzi <debonzi@...ux.vnet.ibm.com>
Subject: Re: [PATCH 8/19]: SCST SYSFS interface implementation

On Tue, Oct 12, 2010 at 10:53:45PM +0400, Vladislav Bolkhovitin wrote:
> > Seriously, you CAN NOT DO THIS!  If you embed a kobject in a different
> > structure, then you have to rely on the kobject to handle the reference
> > counting for that larger structure.  To do ANYTHING else is a bug and
> > wrong.
> > 
> > Please read the kobject documentation and fix this code up before
> > submitting it again.
> 
> Sure, I have read it and we rely on the kobject to handle the reference
> counting for the larger structure. It's only done not in a
> straightforward way, because the way it is implemented is simpler for us
> + for some other reasons.

Sorry, but I don't buy it.

> For instance, for structure scst_tgt it is done using
> tgt_kobj_release_cmpl completion. When a target driver calls
> scst_unregister_target(), scst_unregister_target() in the end calls
> scst_tgt_sysfs_del(), which calls kobject_put(&tgt->tgt_kobj) and wait
> for tgt_kobj_release_cmpl to complete.

Wait, why shouldn't the release then free the memory?

> At this point tgt_kobj can be taken only by the SYSFS.
> Scst_tgt_sysfs_del() can wait as much as needed until the SYSFS code
> released it. As far as I can see, it can't be forever, so it's OK.

I don't understand, why can't you just free the memory, what are you
having to wait for?

You are only having one kobject for your structure, right?  If so, then
free the memory in the release, to wait for something else to free the
memory is wrong.

> Then, after scst_tgt_sysfs_del() returned, scst_unregister_target()
> will free scst_tgt together with embedded tgt_kobj.

As no other kernel code is like this, I don't think it's valid to be
doing so, sorry.

Please fix this.

good luck,

greg k-h
--
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/