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@xxxxxxxxxxxxxxx> > > > > > > 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@xxxxxxxxxxxxxxx> > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
