Date:	Tue, 07 Sep 2010 19:08:59 -0700
From:	"Nicholas A. Bellinger" <nab@...ux-iscsi.org>
To:	Joel Becker <Joel.Becker@...cle.com>
Cc:	Konrad Rzeszutek Wilk <konrad@...nok.org>,
	linux-scsi <linux-scsi@...r.kernel.org>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	FUJITA Tomonori <fujita.tomonori@....ntt.co.jp>,
	Mike Christie <michaelc@...wisc.edu>,
	Christoph Hellwig <hch@....de>, Hannes Reinecke <hare@...e.de>,
	James Bottomley <James.Bottomley@...e.de>,
	Jens Axboe <axboe@...nel.dk>,
	Boaz Harrosh <bharrosh@...asas.com>,
	Linux-fsdevel <linux-fsdevel@...r.kernel.org>
Subject: Re: [RFC 02/22] configfs: Add struct
	configfs_item_operations->check_link() in configfs_unlink()

On Tue, 2010-09-07 at 15:44 -0700, Joel Becker wrote:
> On Tue, Sep 07, 2010 at 05:01:01PM -0400, Konrad Rzeszutek Wilk wrote:
> > > > 	I NAK'd this a while back.  I'm willing to be convinced, but so
> > > > far it remains that way.
> > >
> > > Hi Joel,
> > >
> > > Thanks for bringing this point up again.  So a brief refresh on why this
> > > is currently required in the fabric independent configfs handlers in
> > > drivers/target/target_core_fabric_configfs.c (patch #19).
> > 
> > Well, that is great that you mentioned your requirements. But I don't see a 
> > quote of Joel's concerns? Is there an LKML link for it perhaps?
> 
> 	It was a while back.  Essentially, the concern is that configfs
> is defined to be userspace-driven.  If the user asks you to remove
> something, the module should be handling safe teardown rather than
> rejecting the user's request.
> 	Now, the world isn't always clean-cut.  Code outside of the
> filesystem representation can lay a claim on a configfs item to say "I'm
> busy with this."  An example is ocfs2 pinning the configfs item
> describing its cluster heartbeat device.   But this is ocfs2 - a
> separate thing - claiming it.  There is a defined API to take and
> release these claims.
> 	This API doesn't cover symlinks, as symlinks are simply linkage
> between config items.  It may be, as Nick suggested at the bottom of his
> reply, that we want the API extended to cover that case.  But he hasn't
> yet convinced me that safe teardown is impossible or disasterous.
> That's what I'd like to see.  It's not obvious on the face of all the
> file trees in the email.
> 	Nick, can you provide some form of description, not long
> pathnames, that explains a) what breaks when the symlink is removed b)
> why that can't be allowed if the user is dumb enough to request it?
> 

Hi Joel,

So, the case where configfs will actually OOPs without the
->check_link() patch (or without some other internal solution) is on the
unlink(2) path is when the symlink is created to a destination outside
of the source struct config_group.  This may have not been exactly
apparent in my LIO-Target example, but here is another shot at an
example without the other complexities of target mode invovled.

Say we have two different struct config_subsystem in two different LKM
sub_parent and sub_child.  I will spare the actual mkdir(2) and ln(2)
calls here, but (I hope) these are obvious:

First, we start out with the parent source struct config_group from
sub_parent module:

	/sys/kernel/config/sub_parent/group1/parent/

Next, we have a symlink from sub_parent/group1/parent to a different LKM
in sub_child:

	/sys/kernel/config/sub_child/group1/src_0/src_link -> ../../../../sub_parent/group1/parent

And then a second symlink from sub_child/group1/src_0/src_link to a
sstuct config_group outside of group1, but still within sub_child:

	/sys/kernel/config/sub_child/group2/dst_0/dst_link -> ../../../group1/src_0/

So once the sub_child/group2/dest_0/dst_link has been created to back to
sub_child/group1/src_0/src_link, the oops will appear any time that
'unlink sub_child/group1/src_0/src_link' is called while the second
group2/dst_0/dst_link is still present.  I don't recall the actual
backtrace of the OOPs that occurs when the unlink(2) is called, but it
is easily reproducable .

I am really starting to think that fixing this properly below the struct
config_item_operations API is going to make the most sense, but I have
not realized this in a patch for fs/configfs/ just yet..  I am happy to
do this in the next days if you think this would be the cleanest
resolution for the above case.

Thanks Joel!

--nab

--
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/

Powered by blists - more mailing lists