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]

Message-ID: <4CC1DAA2.7030602@vlnb.net>
Date:	Fri, 22 Oct 2010 22:40:34 +0400
From:	Vladislav Bolkhovitin <vst@...b.net>
To:	Greg KH <greg@...ah.com>
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

Greg KH, on 10/22/2010 09:56 PM wrote:
> On Fri, Oct 22, 2010 at 09:30:53PM +0400, Vladislav Bolkhovitin wrote:
>> +	unsigned int tgt_kobj_initialized:1;
> 
> It's the middle of the merge window, and I'm about to go on vacation, so
> I didn't read this patch after this line.
> 
> It's obvious that this patch is wrong, you shouldn't need to worry about
> this.  And even if you did, you don't need this flag.
> 
> Why are you trying to do something that no one else needs?  Why make
> things harder than they have to be.

I tried to explain that to you in http://lkml.org/lkml/2010/10/14/291
and mentioned there the need to create this flag to track
half-initialized kobjects. You agreed
(http://lkml.org/lkml/2010/10/14/299) that don't return half-initialized
objects is a regular kernel practice, but then requested to strictly
bound the larger object freeing to its kobject release(), which means
that all SYSFS creating functions now have to return half-initialized
SYSFS hierarchy in case of any error. Hence the flag to track it.

Simply, any SCST object has a lot of other things to initialize besides
its kobject, hence the need either to free it independently from its
kobject, or track by a flag if its kobjects initialized.

For illustration, here is the simplified code from my previous example,
i.e. without this patch. I added to it scst_unregister_target() to make
it more complete.

void scst_tgt_sysfs_del(struct scst_tgt *tgt)
{
...
	kobject_put(&tgt->tgt_kobj);
	wait_for_completion(&tgt->tgt_kobj_release_cmpl);
}

int scst_tgt_sysfs_create(struct scst_tgt *tgt)
{
	init_completion(&tgt->tgt_kobj_release_cmpl);

	res = kobject_init_and_add(&tgt->tgt_kobj, &tgt_ktype,
			&tgt->tgtt->tgtt_kobj, tgt->tgt_name);
	if (res != 0)
		goto out;

	res = sysfs_create_file(&tgt->tgt_kobj,
			&tgt_enable_attr.attr);
	if (res != 0)
		goto out_err;
...
out:
	return res;

out_err:
	scst_tgt_sysfs_del(tgt);
	goto out;
}

struct scst_tgt *scst_register_target()
{
	struct scst_tgt *tgt;
	int rc = 0;

	rc = scst_alloc_tgt();
	if (rc != 0)
		goto out;
...
	tgt->tgt_name = kmalloc(strlen(target_name) + 1, GFP_KERNEL);
	if (tgt->tgt_name == NULL)
		goto out_free_tgt;
...

	mutex_lock();

	rc = scst_tgt_sysfs_create(tgt);
	if (rc < 0)
		goto out_unlock;

	tgt->default_acg = scst_alloc_add_acg();
	if (tgt->default_acg == NULL)
		goto out_sysfs_del;
...

out:
	return tgt;

out_sysfs_del:
	mutex_unlock();
	scst_tgt_sysfs_del(tgt)
	goto out_free_tgt;

out_unlock:
	mutex_unlock();

out_free_tgt:
	scst_free_tgt(tgt);
}

void scst_unregister_target(struct scst_tgt *tgt)
{
...

	scst_tgt_sysfs_del(tgt);
...
	scst_free_tgt(tgt);
}

You can see complete source code of those functions in the original
patch set I sent in this thread (patches 4 and 8).

What am I missing and how errors processing should be done with neither
freeing tgt not in tgt_kobj release() as above, nor without
tgt_kobj_initialized flag as in the patch?

Thanks,
Vlad
--
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/