| Message ID | 20181122133800.1251-1-ddiss@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] target: sanitize ALUA and PR state file paths before use | expand |
Ping - any feedback on this change? On Thu, 22 Nov 2018 14:38:00 +0100, David Disseldorp wrote: > Block ALUA and PR state storage if any of the dynamic subdirectory > components include a path separator. > > Fixes: c66ac9db8d4a ("[SCSI] target: Add LIO target core v4.0.0-rc6") > Signed-off-by: David Disseldorp <ddiss@suse.de> > Signed-off-by: Lee Duncan <lduncan@suse.com> > --- > Note: > Submitted as an RFC, as I've not properly tested the alua code path. > I'm also not sure whether it's reasonable to break existing setups > with a '/' in the configured unit_serial. Where "break" means fail > APTPL PR requests; ALUA state-save failures are ignored internally. I'd also be happy with any pointers on properly testing ALUA. Cheers, David
On Mon, 2018-12-10 at 14:36 +0100, David Disseldorp wrote: > Ping - any feedback on this change? > > On Thu, 22 Nov 2018 14:38:00 +0100, David Disseldorp wrote: > > > Block ALUA and PR state storage if any of the dynamic subdirectory > > components include a path separator. > > > > Fixes: c66ac9db8d4a ("[SCSI] target: Add LIO target core v4.0.0-rc6") > > Signed-off-by: David Disseldorp <ddiss@suse.de> > > Signed-off-by: Lee Duncan <lduncan@suse.com> > > --- > > Note: > > Submitted as an RFC, as I've not properly tested the alua code path. > > I'm also not sure whether it's reasonable to break existing setups > > with a '/' in the configured unit_serial. Where "break" means fail > > APTPL PR requests; ALUA state-save failures are ignored internally. > > I'd also be happy with any pointers on properly testing ALUA. Embedding SCSI properties like the unit serial number in a file system path seems wrong to me. I think the path in which these properties are stored should be derived from configfs item names configured by the user. These names are guaranteed to be ASCII strings. If you want to see an example of alternative approach you are welcome to have a look at scst_pr_set_file_name() and scst_pr_sync_device_file() in the SCST source code. Regarding testing ALUA code: I use sg_rtpg and sg_stpg to test SCSI target ALUA code. Bart.
On Mon, 10 Dec 2018 11:19:59 -0800, Bart Van Assche wrote: > Embedding SCSI properties like the unit serial number in a file system path > seems wrong to me. I completely agree, but unfortunately that's where we currently stand. > I think the path in which these properties are stored > should be derived from configfs item names configured by the user. These > names are guaranteed to be ASCII strings. If you want to see an example of > alternative approach you are welcome to have a look at scst_pr_set_file_name() > and scst_pr_sync_device_file() in the SCST source code. Sounds reasonable, although that would leave us with the task of migrating consumers of the existing unit-serial-number derived paths to use the new state file locations. > Regarding testing ALUA code: I use sg_rtpg and sg_stpg to test SCSI target > ALUA code. Thanks, I'll have a play with them. Cheers, David
diff --git a/drivers/target/target_core_alua.c b/drivers/target/target_core_alua.c index 4f134b0c3e29..517945f881e0 100644 --- a/drivers/target/target_core_alua.c +++ b/drivers/target/target_core_alua.c @@ -918,9 +918,16 @@ static int core_alua_update_tpg_primary_metadata( { unsigned char *md_buf; struct t10_wwn *wwn = &tg_pt_gp->tg_pt_gp_dev->t10_wwn; + const char *tpgs_name; char *path; int len, rc; + tpgs_name = config_item_name(&tg_pt_gp->tg_pt_gp_group.cg_item); + if (strchr(wwn->unit_serial, '/') || strchr(tpgs_name, '/')) { + pr_err("Unable to construct valid ALUA metadata path\n"); + return -EINVAL; + } + md_buf = kzalloc(ALUA_MD_BUF_LEN, GFP_KERNEL); if (!md_buf) { pr_err("Unable to allocate buf for ALUA metadata\n"); @@ -937,8 +944,7 @@ static int core_alua_update_tpg_primary_metadata( rc = -ENOMEM; path = kasprintf(GFP_KERNEL, "%s/alua/tpgs_%s/%s", db_root, - &wwn->unit_serial[0], - config_item_name(&tg_pt_gp->tg_pt_gp_group.cg_item)); + &wwn->unit_serial[0], tpgs_name); if (path) { rc = core_alua_write_tpg_metadata(path, md_buf, len); kfree(path); @@ -1210,6 +1216,8 @@ static int core_alua_update_tpg_secondary_metadata(struct se_lun *lun) { struct se_portal_group *se_tpg = lun->lun_tpg; unsigned char *md_buf; + const char *fabric_name; + const char *wwn; char *path; int len, rc; @@ -1227,17 +1235,22 @@ static int core_alua_update_tpg_secondary_metadata(struct se_lun *lun) atomic_read(&lun->lun_tg_pt_secondary_offline), lun->lun_tg_pt_secondary_stat); + fabric_name = se_tpg->se_tpg_tfo->get_fabric_name(); + wwn = se_tpg->se_tpg_tfo->tpg_get_wwn(se_tpg); + if (strchr(fabric_name, '/') || strchr(wwn, '/')) { + pr_err("Unable to construct valid ALUA metadata path\n"); + rc = -EINVAL; + goto out_free; + } + if (se_tpg->se_tpg_tfo->tpg_get_tag != NULL) { path = kasprintf(GFP_KERNEL, "%s/alua/%s/%s+%hu/lun_%llu", - db_root, se_tpg->se_tpg_tfo->get_fabric_name(), - se_tpg->se_tpg_tfo->tpg_get_wwn(se_tpg), + db_root, fabric_name, wwn, se_tpg->se_tpg_tfo->tpg_get_tag(se_tpg), lun->unpacked_lun); } else { path = kasprintf(GFP_KERNEL, "%s/alua/%s/%s/lun_%llu", - db_root, se_tpg->se_tpg_tfo->get_fabric_name(), - se_tpg->se_tpg_tfo->tpg_get_wwn(se_tpg), - lun->unpacked_lun); + db_root, fabric_name, wwn, lun->unpacked_lun); } if (!path) { rc = -ENOMEM; diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c index 10db5656fd5d..48397cf919d4 100644 --- a/drivers/target/target_core_pr.c +++ b/drivers/target/target_core_pr.c @@ -1980,6 +1980,11 @@ static int __core_scsi3_write_aptpl_to_file( int ret; loff_t pos = 0; + if (strchr(wwn->unit_serial, '/')) { + pr_err("Unable to construct valid APTPL metadata path\n"); + return -EINVAL; + } + path = kasprintf(GFP_KERNEL, "%s/pr/aptpl_%s", db_root, &wwn->unit_serial[0]); if (!path)
Block ALUA and PR state storage if any of the dynamic subdirectory components include a path separator. Fixes: c66ac9db8d4a ("[SCSI] target: Add LIO target core v4.0.0-rc6") Signed-off-by: David Disseldorp <ddiss@suse.de> Signed-off-by: Lee Duncan <lduncan@suse.com> --- Note: Submitted as an RFC, as I've not properly tested the alua code path. I'm also not sure whether it's reasonable to break existing setups with a '/' in the configured unit_serial. Where "break" means fail APTPL PR requests; ALUA state-save failures are ignored internally. drivers/target/target_core_alua.c | 27 ++++++++++++++++++++------- drivers/target/target_core_pr.c | 5 +++++ 2 files changed, 25 insertions(+), 7 deletions(-)