Re: [RFC 02/22] configfs: Add struct configfs_item_operations->check_link() in configfs_unlink()

Re: [RFC 02/22] configfs: Add struct configfs_item_operations->check_link() in configfs_unlink()

[Nicholas A. Bellinger]

On Wed, 2010-09-01 at 23:48 -0700, Joel Becker wrote:
> On Thu, Sep 02, 2010 at 12:31:07AM -0400, Konrad Rzeszutek Wilk wrote:
> > On Monday 30 August 2010 05:20:25 Nicholas A. Bellinger wrote:
> > > From: Nicholas Bellinger <nab@linux-iscsi.org>
> > >
> > > This patch adds a optional struct configfs_item_operations->check_link()
> > > check called in fs/configfs/symlink.c:configfs_unlink() that can be used by
> > > configfs consumers to check for an explict struct config_group dependence
> > > with active symlink and fail with -EPERM before the unlink(2) syscall is
> > > allowed to occur.
> > >
> > > Currently without this patch, there is not a method that a consumer can
> > > tell configfs_unlink() that it needs to fail for this particular case. 
> > > Allowing ->check_link() to propigate up the errno to VFS is also another
> > > option for the call, but currently for TCM using the existing -EPERM in
> > > configfs_unlink() is fine here.
> > >
> > > Note this patch is used by TCM v4 generic configfs fabric module
> > > infrastructure to allow  explict Initiator Port MappedLUNs symlinks to
> > > create a dependency for the fabric TPG Port LUNs living in a configfs group
> > > that is not a direct struct config_group parent.
> > >
> > > Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
> 
> 	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).

So, say we have an TCM fabric module endpoint (a LIO-Target iSCSI
TargetName+TargetPortalGroupTag in the example below) 

target# tree /sys/kernel/config/target/iscsi/iqn.2003-01.org.linux-iscsi.target.i686\:sn.e475ed6fcdd0/tpgt_1/       
<SNIP>
|-- lun
|   |-- lun_0
|   |   |-- alua_tg_pt_gp
|   |   |-- alua_tg_pt_offline
|   |   |-- alua_tg_pt_status
|   |   |-- alua_tg_pt_write_md
|   |   `-- lio_west_port -> ../../../../../../target/core/iblock_0/lvm_test0
|   |-- lun_1
|   |   |-- alua_tg_pt_gp
|   |   |-- alua_tg_pt_offline
|   |   |-- alua_tg_pt_status
|   |   |-- alua_tg_pt_write_md
|   |   `-- lio_east_port -> ../../../../../../target/core/pscsi_0/sdd
|   |-- lun_2
|   |   |-- alua_tg_pt_gp
|   |   |-- alua_tg_pt_offline
|   |   |-- alua_tg_pt_status
|   |   |-- alua_tg_pt_write_md
|   |   `-- ramdisk_port -> ../../../../../../target/core/rd_dr_0/ramdisk
|   |-- lun_3
|   |   |-- alua_tg_pt_gp
|   |   |-- alua_tg_pt_offline
|   |   |-- alua_tg_pt_status
|   |   |-- alua_tg_pt_write_md
|   |   `-- fileio_port -> ../../../../../../target/core/fileio_0/fileio
|   `-- lun_4
|       |-- alua_tg_pt_gp
|       |-- alua_tg_pt_offline
|       |-- alua_tg_pt_status
|       |-- alua_tg_pt_write_md
|       `-- lio_sync_fileio -> ../../../../../../target/core/fileio_1/sync_file
<SNIP>

with some Port/LUNs that are using configfs symlinks back to TCM Core (a seperate module) in
/sys/kernel/config/target/core/$HBA/$DEV/.

So this symlink happens in target_fabric_port_link() here:

http://git.kernel.org/?p=linux/kernel/git/nab/lio-core-2.6.git;a=blob;f=drivers/target/target_core_fabric_configfs.c;hb=refs/heads/lio-4.0#l619

Then, we add an iSCSI Initiator NodeACL with MappedLUNs using the same
fabric independent handler code in target_core_fabric_configfs.c:

target# tree /sys/kernel/config/target/iscsi/iqn.2003-01.org.linux-iscsi.target.i686\:sn.e475ed6fcdd0/tpgt_1/acls/   
<SNIP>
|   `-- iqn.2008-04.com.sun.virtualbox.initiator
|       |-- attrib
|       |   |-- dataout_timeout
|       |   |-- dataout_timeout_retries
|       |   |-- default_erl
|       |   |-- nopin_response_timeout
|       |   |-- nopin_timeout
|       |   |-- random_datain_pdu_offsets
|       |   |-- random_datain_seq_offsets
|       |   `-- random_r2t_offsets
|       |-- auth
|       |   |-- authenticate_target
|       |   |-- password
|       |   |-- password_mutual
|       |   |-- userid
|       |   `-- userid_mutual
|       |-- cmdsn_depth
|       |-- info
|       |-- lun_0
|       |   |-- lun_0 -> ../../../../../../../target/iscsi/iqn.2003-01.org.linux-iscsi.target.i686:sn.e475ed6fcdd0/tp
gt_1/lun/lun_0
|       |   `-- write_protect
|       |-- lun_1
|       |   |-- lun_1 -> ../../../../../../../target/iscsi/iqn.2003-01.org.linux-iscsi.target.i686:sn.e475ed6fcdd0/tp
gt_1/lun/lun_1
|       |   `-- write_protect
|       |-- lun_2
|       |   |-- lun_2 -> ../../../../../../../target/iscsi/iqn.2003-01.org.linux-iscsi.target.i686:sn.e475ed6fcdd0/tp
gt_1/lun/lun_2
|       |   `-- write_protect
|       |-- lun_3
|       |   |-- lun_3 -> ../../../../../../../target/iscsi/iqn.2003-01.org.linux-iscsi.target.i686:sn.e475ed6fcdd0/tpgt_1/lun/lun_3
|       |   `-- write_protect
<SNIP>

So minus the fabric dependent groups and attributes listed above, notice
the explict LUN ACLs for the iqn.2008-04.com.sun.virtualbox.initiator
NodeACL (known in TCM lingo at MappedLUNs), which contain a symlink back
to the fabric module's $IQN/$TPGT/lun/lun_$ID configfs group that
contains parent struct se_lun->lun_group.  You can see how this works in
target_fabric_mappedlun_link() here:

http://git.kernel.org/?p=linux/kernel/git/nab/lio-core-2.6.git;a=blob;f=drivers/target/target_core_fabric_configfs.c;hb=refs/heads/lio-4.0#l67

So, the reason why something like this patch is required is if unlink(2)
is called on the struct lun_group -> struct
se_subsystem_dev->se_dev_group, the ->check_link() is called in
target_fabric_port_check_link(), and we know to fail the unlink(2) by
checking the struct se_lun->lun_acl_count (eg: if LUN ACls for NodeACLs
exist)

This method has been working fine for the ~12 months, if of course the
configfs consumer is aware of limitiations with symlinks, and provides
the special caller to do the check.  However, if you don't like the idea
of configfs consumers handling this, I would also be open to looking
into a patch that would do this referencing counting for parent / child
symlinks within configfs.ko itself.

What do you think..?

Thanks!

--nab

Re: [RFC 02/22] configfs: Add struct configfs_item_operations->check_link() in configfs_unlink()

[Konrad Rzeszutek Wilk]

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

Re: [RFC 02/22] configfs: Add struct configfs_item_operations->check_link() in configfs_unlink()

[Joel Becker]

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?

Joel

-- 

"The lawgiver, of all beings, most owes the law allegiance.  He of all
 men should behave as though the law compelled him.  But it is the
 universal weakness of mankind that what we are given to administer we
 presently imagine we own."
        - H.G. Wells

Joel Becker
Consulting Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

Re: [RFC 02/22] configfs: Add struct configfs_item_operations->check_link() in configfs_unlink()

[Nicholas A. Bellinger]

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

Re: [RFC 02/22] configfs: Add struct configfs_item_operations->check_link() in configfs_unlink()

[Joel Becker]

On Tue, Sep 07, 2010 at 07:08:59PM -0700, Nicholas A. Bellinger wrote:
> On Tue, 2010-09-07 at 15:44 -0700, Joel Becker wrote:
> > 	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?
> > 
> 
> 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.

	I wish you'd mentioned the oops earlier.  We need to figure out
who is oopsing.

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

	I think you have too many ../ on your examples.  dst_link
pointing to ../../../../group1/src_0 resolves to
/sys/kernel/config/group1/src_0, which I don't think you want.  I think
you have:

    sub_parent/group1/parent
    sub_child/group1/src_0/src_link -> ../../../sub_parent/group1/parent
    sub_child/group2/dst_0/dst_link -> ../../group1/src_0

	If I'm reading this right, 'ls sub_child/group1/src_0/src_link'
gives you the contents of sub_parent/group1/parent, 'ls
sub_child/group2/dst_0/dst_link' gives you 'src_link', and 'ls
sub_child/group2/dst_0/dst_link/src_link' brings you right back to the
contents of sub_parent/group1/parent.  Am I right?

> 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'm confused.  Above it appears that
sub_child/group2/dst_0/dst_link -> ../../group1/src_0, but this
paragraph suggests that sub_child/group2/dst_0/dst_link ->
../../group1/src_0/src_link.  Which is it?

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

	My big question is whether the oops comes from something
configfs is doing wrong when symlinking symlinks, or if your module is
just surprised when things change.  If the former, it needs to be fixed
in configfs.  It shouldn't require a hook for your module to avoid
configfs breakage.  If it is the latter, and your module needs to know
not to break itself, then we discuss how to help it.

Joel

-- 

"Friends may come and go, but enemies accumulate." 
        - Thomas Jones

Joel Becker
Consulting Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

Re: [RFC 02/22] configfs: Add struct configfs_item_operations->check_link() in configfs_unlink()

[Nicholas A. Bellinger]

On Wed, 2010-09-08 at 12:26 -0700, Joel Becker wrote:
> On Tue, Sep 07, 2010 at 07:08:59PM -0700, Nicholas A. Bellinger wrote:
> > On Tue, 2010-09-07 at 15:44 -0700, Joel Becker wrote:
> > > 	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?
> > > 
> > 
> > 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.
> 
> 	I wish you'd mentioned the oops earlier.  We need to figure out
> who is oopsing.

Ok, I ran the code w/o ->check_link() again and have a few corrections
below.

> 
> > 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/
> 
> 	I think you have too many ../ on your examples.  dst_link
> pointing to ../../../../group1/src_0 resolves to
> /sys/kernel/config/group1/src_0, which I don't think you want.  I think
> you have:
> 
>     sub_parent/group1/parent
>     sub_child/group1/src_0/src_link -> ../../../sub_parent/group1/parent
>     sub_child/group2/dst_0/dst_link -> ../../group1/src_0
> 
> 	If I'm reading this right, 'ls sub_child/group1/src_0/src_link'
> gives you the contents of sub_parent/group1/parent, 'ls
> sub_child/group2/dst_0/dst_link' gives you 'src_link', and 'ls
> sub_child/group2/dst_0/dst_link/src_link' brings you right back to the
> contents of sub_parent/group1/parent.  Am I right?

This is correct.

> 
> > 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'm confused.  Above it appears that
> sub_child/group2/dst_0/dst_link -> ../../group1/src_0, but this
> paragraph suggests that sub_child/group2/dst_0/dst_link ->
> ../../group1/src_0/src_link.  Which is it?

Sorry, the symlink should be from sub_child/group2/dst_0/dst_link
-> ../../group1/src_0/

> 
> > 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.
> 
> 	My big question is whether the oops comes from something
> configfs is doing wrong when symlinking symlinks, or if your module is
> just surprised when things change.  If the former, it needs to be fixed
> in configfs.  It shouldn't require a hook for your module to avoid
> configfs breakage.  If it is the latter, and your module needs to know
> not to break itself, then we discuss how to help it.
> 

So after re-running this again, I was a bit off about where the OOOPs is
actually occuring.  So, the OOPs does not occur during in the simple
example here with the first unlink(2):

	unlink sub_child/group1/src_0/src_link

but rather after the second unlink(2) is called after the first for
src_link occurs:

	unlink sub_child/group2/dst_0/dst_link

So back to the OOPs with the current TCM code example, on v2.6.36-rc3
this actually triggers a SLUB warning "Object already free" from inside
of TCM code. This is attributed to the releasing a specific LUN ACLs
from the second unlink(2)'s struct config_item_operations->drop_link(),
that the first unlink had already released.  This is because the first
unlink(2) will currently assume that the remaining LUN ACLs are safe to
release because, it still assumes the disabled check_link call.

I am still pretty certain that I had seen an OOPs in fs/configfs/ code
while doing this at some point (perhaps was w/o slub_debug=FZ..?), but
so far I have not been able to trigger an issue in fs/configfs/
directly.

So, I believe that the main issue here is still involving the unlinking
the first symlink (sub_child/group1/src_0/src_link), and then unlinking
the second symlink (sub_child/group2/dst_0/dst_link) which is still
depending upon the first configfs consumer data structures still being
present.

Does this make sense..?

--nab

Re: [RFC 02/22] configfs: Add struct configfs_item_operations->check_link() in configfs_unlink()

[Joel Becker]

On Wed, Sep 08, 2010 at 01:53:27PM -0700, Nicholas A. Bellinger wrote:
> On Wed, 2010-09-08 at 12:26 -0700, Joel Becker wrote:
> So after re-running this again, I was a bit off about where the OOOPs is
> actually occuring.  So, the OOPs does not occur during in the simple
> example here with the first unlink(2):
> 
> 	unlink sub_child/group1/src_0/src_link
> 
> but rather after the second unlink(2) is called after the first for
> src_link occurs:
> 
> 	unlink sub_child/group2/dst_0/dst_link
> 
> So back to the OOPs with the current TCM code example, on v2.6.36-rc3
> this actually triggers a SLUB warning "Object already free" from inside
> of TCM code. This is attributed to the releasing a specific LUN ACLs
> from the second unlink(2)'s struct config_item_operations->drop_link(),
> that the first unlink had already released.  This is because the first
> unlink(2) will currently assume that the remaining LUN ACLs are safe to
> release because, it still assumes the disabled check_link call.

	The trivial solution is to refcount your ACLs.  You get both
allow_link() calls, so you should be able to increment a counter there,
and then drop them when the last drop_link() call is made.  That will
keep your consumer structures around until all links are exhausted.

Joel

-- 

"I'm so tired of being tired,
 Sure as night will follow day.
 Most things I worry about
 Never happen anyway."

Joel Becker
Consulting Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

Re: [RFC 02/22] configfs: Add struct configfs_item_operations->check_link() in configfs_unlink()

[Nicholas A. Bellinger]

On Fri, 2010-09-10 at 08:28 -0700, Joel Becker wrote:
> On Wed, Sep 08, 2010 at 01:53:27PM -0700, Nicholas A. Bellinger wrote:
> > On Wed, 2010-09-08 at 12:26 -0700, Joel Becker wrote:
> > So after re-running this again, I was a bit off about where the OOOPs is
> > actually occuring.  So, the OOPs does not occur during in the simple
> > example here with the first unlink(2):
> > 
> > 	unlink sub_child/group1/src_0/src_link
> > 
> > but rather after the second unlink(2) is called after the first for
> > src_link occurs:
> > 
> > 	unlink sub_child/group2/dst_0/dst_link
> > 
> > So back to the OOPs with the current TCM code example, on v2.6.36-rc3
> > this actually triggers a SLUB warning "Object already free" from inside
> > of TCM code. This is attributed to the releasing a specific LUN ACLs
> > from the second unlink(2)'s struct config_item_operations->drop_link(),
> > that the first unlink had already released.  This is because the first
> > unlink(2) will currently assume that the remaining LUN ACLs are safe to
> > release because, it still assumes the disabled check_link call.
> 
> 	The trivial solution is to refcount your ACLs.  You get both
> allow_link() calls, so you should be able to increment a counter there,
> and then drop them when the last drop_link() call is made.  That will
> keep your consumer structures around until all links are exhausted.
> 

Hi Joel,

So I am a bit confused wrt to this last response..  The ->check_link()
patch and it's use in the fabric independent code within
target_core_fabric_configfs.c does exactly this for the 'MappedLUN'
symlink case, eg: requires the consumer to do the allow_link() +
drop_link() refcounting, and add the
API check into fs/configfs/symlink.c:configfs_unlink()

Is there another form of configfs consumer refcounting that you had in
mind beyond using an atomic_t for this with ->check_link() here..?

http://git.kernel.org/?p=linux/kernel/git/nab/lio-core-2.6.git;a=blob;f=drivers/target/target_core_fabric_configfs.c;hb=refs/heads/lio-4.0#l675

So beyond a configfs consumer solution, what do you think about checking
for the sub_child/group2/dst_0/dst_link style of symlink
in fs/configfs/symlink.c:configfs_symlink() in order to add some form of
internal refcount when the symlink source is within the same consumer
LKM, but outside of the parent struct config_group..?

This would involve the conversion of fs/configfs/symlink.c:
configfs_unlink() path to check for the existence of this internal
refcount and returning -EPERM when any sub_child/group2/dst_0/dst_link
exist when 'unlink sub_child/group1/src_0/src_link' is attempted.

What do you think..?

Thanks!

--nab

Re: [RFC 02/22] configfs: Add struct configfs_item_operations->check_link() in configfs_unlink()

[Joel Becker]

On Fri, Sep 10, 2010 at 12:06:46PM -0700, Nicholas A. Bellinger wrote:
> On Fri, 2010-09-10 at 08:28 -0700, Joel Becker wrote:
> > 	The trivial solution is to refcount your ACLs.  You get both
> > allow_link() calls, so you should be able to increment a counter there,
> > and then drop them when the last drop_link() call is made.  That will
> > keep your consumer structures around until all links are exhausted.
> > 
> 
> So I am a bit confused wrt to this last response..  The ->check_link()
> patch and it's use in the fabric independent code within
> target_core_fabric_configfs.c does exactly this for the 'MappedLUN'
> symlink case, eg: requires the consumer to do the allow_link() +
> drop_link() refcounting, and add the
> API check into fs/configfs/symlink.c:configfs_unlink()

	You can refcount without check_link().
 
> Is there another form of configfs consumer refcounting that you had in
> mind beyond using an atomic_t for this with ->check_link() here..?

	I'm saying that you won't crash if you don't free the ACLs on
the first drop_link().  That is, the drop_link() goes through as
configfs wants it to, but you don't crash.

> So beyond a configfs consumer solution, what do you think about checking
> for the sub_child/group2/dst_0/dst_link style of symlink
> in fs/configfs/symlink.c:configfs_symlink() in order to add some form of
> internal refcount when the symlink source is within the same consumer
> LKM, but outside of the parent struct config_group..?
> 
> This would involve the conversion of fs/configfs/symlink.c:
> configfs_unlink() path to check for the existence of this internal
> refcount and returning -EPERM when any sub_child/group2/dst_0/dst_link
> exist when 'unlink sub_child/group1/src_0/src_link' is attempted.

	You're still fighting allowing the links to go away.  You
haven't explained why that is necessary.  You had a problem with a crash
because you expected one reference to your ACLs and actually have two,
but you can fix that without modifying configfs.

Joel

-- 

"I have never let my schooling interfere with my education."
        - Mark Twain

Joel Becker
Consulting Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

Re: [RFC 02/22] configfs: Add struct configfs_item_operations->check_link() in configfs_unlink()

[Nicholas A. Bellinger]

On Fri, 2010-09-10 at 12:44 -0700, Joel Becker wrote:
> On Fri, Sep 10, 2010 at 12:06:46PM -0700, Nicholas A. Bellinger wrote:
> > On Fri, 2010-09-10 at 08:28 -0700, Joel Becker wrote:
> > > 	The trivial solution is to refcount your ACLs.  You get both
> > > allow_link() calls, so you should be able to increment a counter there,
> > > and then drop them when the last drop_link() call is made.  That will
> > > keep your consumer structures around until all links are exhausted.
> > > 
> > 
> > So I am a bit confused wrt to this last response..  The ->check_link()
> > patch and it's use in the fabric independent code within
> > target_core_fabric_configfs.c does exactly this for the 'MappedLUN'
> > symlink case, eg: requires the consumer to do the allow_link() +
> > drop_link() refcounting, and add the
> > API check into fs/configfs/symlink.c:configfs_unlink()
> 
> 	You can refcount without check_link().

So what do you recommend here..?

>  
> > Is there another form of configfs consumer refcounting that you had in
> > mind beyond using an atomic_t for this with ->check_link() here..?
> 
> 	I'm saying that you won't crash if you don't free the ACLs on
> the first drop_link().  That is, the drop_link() goes through as
> configfs wants it to, but you don't crash.

The problem is that the 'unlink sub_child/group1/src_0/src_link' can't
signal to the other struct config_group to also call an internal 'unlink
sub_child/group2/dst_0/dst_link' to drop the child link outside of it's
struct config_group.

> 
> > So beyond a configfs consumer solution, what do you think about checking
> > for the sub_child/group2/dst_0/dst_link style of symlink
> > in fs/configfs/symlink.c:configfs_symlink() in order to add some form of
> > internal refcount when the symlink source is within the same consumer
> > LKM, but outside of the parent struct config_group..?
> > 
> > This would involve the conversion of fs/configfs/symlink.c:
> > configfs_unlink() path to check for the existence of this internal
> > refcount and returning -EPERM when any sub_child/group2/dst_0/dst_link
> > exist when 'unlink sub_child/group1/src_0/src_link' is attempted.
> 
> 	You're still fighting allowing the links to go away.  You
> haven't explained why that is necessary.  You had a problem with a crash
> because you expected one reference to your ACLs and actually have two,
> but you can fix that without modifying configfs.

If this is the case then I must be mis-understanding what you mean by
configfs consumer refcounting from allow_link() and drop_link().  Can
you give me a bit more detail where I should be looking..?

Thanks!

--nab

Re: [RFC 02/22] configfs: Add struct configfs_item_operations->check_link() in configfs_unlink()

[Joel Becker]

[Sorry on the delay, I was out of town]

On Fri, Sep 10, 2010 at 12:52:03PM -0700, Nicholas A. Bellinger wrote:
> On Fri, 2010-09-10 at 12:44 -0700, Joel Becker wrote:
> > 	You can refcount without check_link().
> 
> So what do you recommend here..?

	That your ACL object, or whatever it is that considers itself to
be refcounted by the number of links, keep track of that and only free
itself when all are gone rather than freeing itself when the first goes
away.

> The problem is that the 'unlink sub_child/group1/src_0/src_link' can't
> signal to the other struct config_group to also call an internal 'unlink
> sub_child/group2/dst_0/dst_link' to drop the child link outside of it's
> struct config_group.

	Nor should it.  I'm asking what is so wrong about a world where
sub_child/group1/src_0/src_link is gone but
sub_child/group2/dst_0/dst_link remains?  Maybe that target object can't
work anymore, but the user broke it by breaking the link.

> > 	You're still fighting allowing the links to go away.  You
> > haven't explained why that is necessary.  You had a problem with a crash
> > because you expected one reference to your ACLs and actually have two,
> > but you can fix that without modifying configfs.
> 
> If this is the case then I must be mis-understanding what you mean by
> configfs consumer refcounting from allow_link() and drop_link().  Can
> you give me a bit more detail where I should be looking..?

	Here's how I sort of understood things.  First, you create the
src_link pointing to $object.  This somehow allocates some sort of ACL
structure hanging off of $object.  Then you create dst_link pointing to
src_link, which really ends up pointing to the $object.  So now you have
src_link and dst_link pointing to $object.
	Finally, someone unlinks src_link.  This triggers $object to
free the ACL structure.  When the caller later removes dst_link, it
crashes because it was expecting ACL to still be there.  Do I have it
right?
	I'm saying that $object should count how many people are
pointing to it, so that when you remove src_link, ACL is *not* freed.
It will only be freed when both src_link and dst_link are removed.  This
way you do not crash.  Perhaps I'm not understanding the ACL object.
Perhaps I'm missing the mechanism entirely.  But I don't see why the ACL
object must necessarily be freed when one symlink is removed but not the
other.

Joel

-- 

"And yet I fight,
 And yet I fight this battle all alone.
 No one to cry to;
 No place to call home."

Joel Becker
Consulting Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

Re: [RFC 02/22] configfs: Add struct configfs_item_operations->check_link() in configfs_unlink()

[Nicholas A. Bellinger]

On Mon, 2010-09-20 at 15:06 -0700, Joel Becker wrote:
> [Sorry on the delay, I was out of town]
> 

Hi Joel,

Many, thanks for your followup on this item, my comments are below.

> On Fri, Sep 10, 2010 at 12:52:03PM -0700, Nicholas A. Bellinger wrote:
> > On Fri, 2010-09-10 at 12:44 -0700, Joel Becker wrote:
> > > 	You can refcount without check_link().
> > 
> > So what do you recommend here..?
> 
> 	That your ACL object, or whatever it is that considers itself to
> be refcounted by the number of links, keep track of that and only free
> itself when all are gone rather than freeing itself when the first goes
> away.
> 

Ok, I see what you mean by internal refcounting within the configfs
consumer to handle this case..

> > The problem is that the 'unlink sub_child/group1/src_0/src_link' can't
> > signal to the other struct config_group to also call an internal 'unlink
> > sub_child/group2/dst_0/dst_link' to drop the child link outside of it's
> > struct config_group.
> 
> 	Nor should it.  I'm asking what is so wrong about a world where
> sub_child/group1/src_0/src_link is gone but
> sub_child/group2/dst_0/dst_link remains?  Maybe that target object can't
> work anymore, but the user broke it by breaking the link.
> 

Yes, so trying to avoid the unlink alltogether here was my main
intention thus far.  

Actually leaving the sub_child/group2/dst_0/dst_link in the example here
would be acceptable for the TCM MappedLUN case, because really we never
expect this case to this unless someone is poking at configfs directly,
and our userspace code will never do this intentionally.

> > > 	You're still fighting allowing the links to go away.  You
> > > haven't explained why that is necessary.  You had a problem with a crash
> > > because you expected one reference to your ACLs and actually have two,
> > > but you can fix that without modifying configfs.
> > 
> > If this is the case then I must be mis-understanding what you mean by
> > configfs consumer refcounting from allow_link() and drop_link().  Can
> > you give me a bit more detail where I should be looking..?
> 
> 	Here's how I sort of understood things.  First, you create the
> src_link pointing to $object.  This somehow allocates some sort of ACL
> structure hanging off of $object.  Then you create dst_link pointing to
> src_link, which really ends up pointing to the $object.  So now you have
> src_link and dst_link pointing to $object.
> 	Finally, someone unlinks src_link.  This triggers $object to
> free the ACL structure.  When the caller later removes dst_link, it
> crashes because it was expecting ACL to still be there.  Do I have it
> right?

Correct.

> 	I'm saying that $object should count how many people are
> pointing to it, so that when you remove src_link, ACL is *not* freed.
> It will only be freed when both src_link and dst_link are removed.  This
> way you do not crash.  Perhaps I'm not understanding the ACL object.
> Perhaps I'm missing the mechanism entirely.  But I don't see why the ACL
> object must necessarily be freed when one symlink is removed but not the
> other.
> 

No, I think your points here make perfect sense.  I will look into a
patch that leaves the TCM fabric MappedLUNs symlinks in place when the
underlying TPG fabric LUN symlink is removed without breaking anything,
but still does the necessary accounting to ensure that shutdown with
active I/Os still works as expected.  I will plan to drop the
->check_link() patch from the forthcoming RFC v2 series.

In the end I think his is the best approach for .37, eg: no configfs
change required.  I am still open to the discussion for resolving this
within fs/configfs proper, but at this point I don't have a strong
preference and will follow your direction here.

Many thanks for your invaluable input Joel!

--nab

Re: [RFC 02/22] configfs: Add struct configfs_item_operations->check_link() in configfs_unlink()

[Boaz Harrosh]

On 09/22/2010 09:16 AM, Nicholas A. Bellinger wrote:
> On Mon, 2010-09-20 at 15:06 -0700, Joel Becker wrote:
>> [Sorry on the delay, I was out of town]
>>
> 
> Hi Joel,
> 
> Many, thanks for your followup on this item, my comments are below.
> 
>> On Fri, Sep 10, 2010 at 12:52:03PM -0700, Nicholas A. Bellinger wrote:
>>> On Fri, 2010-09-10 at 12:44 -0700, Joel Becker wrote:
>>>> 	You can refcount without check_link().
>>>
>>> So what do you recommend here..?
>>
>> 	That your ACL object, or whatever it is that considers itself to
>> be refcounted by the number of links, keep track of that and only free
>> itself when all are gone rather than freeing itself when the first goes
>> away.
>>
> 
> Ok, I see what you mean by internal refcounting within the configfs
> consumer to handle this case..
> 
>>> The problem is that the 'unlink sub_child/group1/src_0/src_link' can't
>>> signal to the other struct config_group to also call an internal 'unlink
>>> sub_child/group2/dst_0/dst_link' to drop the child link outside of it's
>>> struct config_group.
>>
>> 	Nor should it.  I'm asking what is so wrong about a world where
>> sub_child/group1/src_0/src_link is gone but
>> sub_child/group2/dst_0/dst_link remains?  Maybe that target object can't
>> work anymore, but the user broke it by breaking the link.
>>
> 
> Yes, so trying to avoid the unlink alltogether here was my main
> intention thus far.  
> 
> Actually leaving the sub_child/group2/dst_0/dst_link in the example here
> would be acceptable for the TCM MappedLUN case, because really we never
> expect this case to this unless someone is poking at configfs directly,
> and our userspace code will never do this intentionally.
> 
>>>> 	You're still fighting allowing the links to go away.  You
>>>> haven't explained why that is necessary.  You had a problem with a crash
>>>> because you expected one reference to your ACLs and actually have two,
>>>> but you can fix that without modifying configfs.
>>>
>>> If this is the case then I must be mis-understanding what you mean by
>>> configfs consumer refcounting from allow_link() and drop_link().  Can
>>> you give me a bit more detail where I should be looking..?
>>
>> 	Here's how I sort of understood things.  First, you create the
>> src_link pointing to $object.  This somehow allocates some sort of ACL
>> structure hanging off of $object.  Then you create dst_link pointing to
>> src_link, which really ends up pointing to the $object.  So now you have
>> src_link and dst_link pointing to $object.
>> 	Finally, someone unlinks src_link.  This triggers $object to
>> free the ACL structure.  When the caller later removes dst_link, it
>> crashes because it was expecting ACL to still be there.  Do I have it
>> right?
> 
> Correct.
> 
>> 	I'm saying that $object should count how many people are
>> pointing to it, so that when you remove src_link, ACL is *not* freed.
>> It will only be freed when both src_link and dst_link are removed.  This
>> way you do not crash.  Perhaps I'm not understanding the ACL object.
>> Perhaps I'm missing the mechanism entirely.  But I don't see why the ACL
>> object must necessarily be freed when one symlink is removed but not the
>> other.
>>
> 
> No, I think your points here make perfect sense.  I will look into a
> patch that leaves the TCM fabric MappedLUNs symlinks in place when the
> underlying TPG fabric LUN symlink is removed without breaking anything,
> but still does the necessary accounting to ensure that shutdown with
> active I/Os still works as expected.  

Perhaps a strengthen chmod here. And if then, done by root, a big fat
"shoot self in the foot" message in dmsg for the poking where you don't
need to. type.

(BTW: could you re establish the link after it's deleted the way you
 do at setup?)

Boaz
> I will plan to drop the
> ->check_link() patch from the forthcoming RFC v2 series.
> 
> In the end I think his is the best approach for .37, eg: no configfs
> change required.  I am still open to the discussion for resolving this
> within fs/configfs proper, but at this point I don't have a strong
> preference and will follow your direction here.
> 
> Many thanks for your invaluable input Joel!
> 
> --nab
> 

Re: [RFC 02/22] configfs: Add struct configfs_item_operations->check_link() in configfs_unlink()

[Nicholas A. Bellinger]

On Wed, 2010-09-22 at 13:18 +0200, Boaz Harrosh wrote:
> On 09/22/2010 09:16 AM, Nicholas A. Bellinger wrote:
> > On Mon, 2010-09-20 at 15:06 -0700, Joel Becker wrote:
> >> [Sorry on the delay, I was out of town]
> >>
> > 
> > Hi Joel,
> > 
> > Many, thanks for your followup on this item, my comments are below.
> > 
> >> On Fri, Sep 10, 2010 at 12:52:03PM -0700, Nicholas A. Bellinger wrote:
> >>> On Fri, 2010-09-10 at 12:44 -0700, Joel Becker wrote:
> >>>> 	You can refcount without check_link().
> >>>
> >>> So what do you recommend here..?
> >>
> >> 	That your ACL object, or whatever it is that considers itself to
> >> be refcounted by the number of links, keep track of that and only free
> >> itself when all are gone rather than freeing itself when the first goes
> >> away.
> >>
> > 
> > Ok, I see what you mean by internal refcounting within the configfs
> > consumer to handle this case..
> > 
> >>> The problem is that the 'unlink sub_child/group1/src_0/src_link' can't
> >>> signal to the other struct config_group to also call an internal 'unlink
> >>> sub_child/group2/dst_0/dst_link' to drop the child link outside of it's
> >>> struct config_group.
> >>
> >> 	Nor should it.  I'm asking what is so wrong about a world where
> >> sub_child/group1/src_0/src_link is gone but
> >> sub_child/group2/dst_0/dst_link remains?  Maybe that target object can't
> >> work anymore, but the user broke it by breaking the link.
> >>
> > 
> > Yes, so trying to avoid the unlink alltogether here was my main
> > intention thus far.  
> > 
> > Actually leaving the sub_child/group2/dst_0/dst_link in the example here
> > would be acceptable for the TCM MappedLUN case, because really we never
> > expect this case to this unless someone is poking at configfs directly,
> > and our userspace code will never do this intentionally.
> > 
> >>>> 	You're still fighting allowing the links to go away.  You
> >>>> haven't explained why that is necessary.  You had a problem with a crash
> >>>> because you expected one reference to your ACLs and actually have two,
> >>>> but you can fix that without modifying configfs.
> >>>
> >>> If this is the case then I must be mis-understanding what you mean by
> >>> configfs consumer refcounting from allow_link() and drop_link().  Can
> >>> you give me a bit more detail where I should be looking..?
> >>
> >> 	Here's how I sort of understood things.  First, you create the
> >> src_link pointing to $object.  This somehow allocates some sort of ACL
> >> structure hanging off of $object.  Then you create dst_link pointing to
> >> src_link, which really ends up pointing to the $object.  So now you have
> >> src_link and dst_link pointing to $object.
> >> 	Finally, someone unlinks src_link.  This triggers $object to
> >> free the ACL structure.  When the caller later removes dst_link, it
> >> crashes because it was expecting ACL to still be there.  Do I have it
> >> right?
> > 
> > Correct.
> > 
> >> 	I'm saying that $object should count how many people are
> >> pointing to it, so that when you remove src_link, ACL is *not* freed.
> >> It will only be freed when both src_link and dst_link are removed.  This
> >> way you do not crash.  Perhaps I'm not understanding the ACL object.
> >> Perhaps I'm missing the mechanism entirely.  But I don't see why the ACL
> >> object must necessarily be freed when one symlink is removed but not the
> >> other.
> >>
> > 
> > No, I think your points here make perfect sense.  I will look into a
> > patch that leaves the TCM fabric MappedLUNs symlinks in place when the
> > underlying TPG fabric LUN symlink is removed without breaking anything,
> > but still does the necessary accounting to ensure that shutdown with
> > active I/Os still works as expected.  
> 
> Perhaps a strengthen chmod here. And if then, done by root, a big fat
> "shoot self in the foot" message in dmsg for the poking where you don't
> need to. type.
> 

Hmm, I don't believe configfs currently supports a distinction between
permissions for something along these lines.

> (BTW: could you re establish the link after it's deleted the way you
>  do at setup?)
> 

Yes, the "dangling" MappedLUNs symlinks for this special case would need
to be explictly removed via unlink(2) and then re-created using a new
TPG LUN struct se_lun->lun_group containing a valid symlink back to
a TCM Core backstore a valid configfs symlink source.

Thanks!

--nab

Re: [RFC 02/22] configfs: Add struct configfs_item_operations->check_link() in configfs_unlink()

[Joel Becker]

On Wed, Sep 22, 2010 at 12:16:42AM -0700, Nicholas A. Bellinger wrote:
> On Mon, 2010-09-20 at 15:06 -0700, Joel Becker wrote:
> > [Sorry on the delay, I was out of town]
> > 
> 
> Many, thanks for your followup on this item, my comments are below.

	No problem.  I'm sorry I was so busy that we couldn't iron this
out a few months ago.  I'm glad we're getting somewhere.

> > On Fri, Sep 10, 2010 at 12:52:03PM -0700, Nicholas A. Bellinger wrote:
> > > On Fri, 2010-09-10 at 12:44 -0700, Joel Becker wrote:
> 
> Actually leaving the sub_child/group2/dst_0/dst_link in the example here
> would be acceptable for the TCM MappedLUN case, because really we never
> expect this case to this unless someone is poking at configfs directly,
> and our userspace code will never do this intentionally.

	Right.  If someone pokes around and makes it stop working,
that's their fault.  What we (configfs and TCM) must guarantee is that
user poking doesn't cause a crash or put things in an unrecoverable
state.  "Remove all TCM mappins" is a valid recovery state, though
"Recreate the symlink and it works again" is a better one.

> > 	I'm saying that $object should count how many people are
> > pointing to it, so that when you remove src_link, ACL is *not* freed.
> > It will only be freed when both src_link and dst_link are removed.  This
> > way you do not crash.  Perhaps I'm not understanding the ACL object.
> > Perhaps I'm missing the mechanism entirely.  But I don't see why the ACL
> > object must necessarily be freed when one symlink is removed but not the
> > other.
> > 
> 
> No, I think your points here make perfect sense.  I will look into a
> patch that leaves the TCM fabric MappedLUNs symlinks in place when the
> underlying TPG fabric LUN symlink is removed without breaking anything,
> but still does the necessary accounting to ensure that shutdown with
> active I/Os still works as expected.  I will plan to drop the
> ->check_link() patch from the forthcoming RFC v2 series.

	This sounds like a good plan for initial inclusion.

> In the end I think his is the best approach for .37, eg: no configfs
> change required.  I am still open to the discussion for resolving this
> within fs/configfs proper, but at this point I don't have a strong
> preference and will follow your direction here.

	I think we can revisit this as necessary.

Joel

-- 

Life's Little Instruction Book #313

	"Never underestimate the power of love."

Joel Becker
Consulting Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127