Re: [RFC PATCH] scsi: target: detect XCOPY NAA descriptor conflicts

[David Disseldorp <ddiss@suse.de>]

On Thu, 3 Sep 2020 10:36:58 -0500, Michael Christie wrote:

> > On Sep 2, 2020, at 5:23 PM, David Disseldorp <ddiss@suse.de> wrote:
> > 
> > Hi Mike,
> > 
> > On Tue, 1 Sep 2020 22:17:51 -0500, Michael Christie wrote:
> >   
> >>> --- a/drivers/target/target_core_xcopy.c
> >>> +++ b/drivers/target/target_core_xcopy.c
> >>> @@ -68,8 +68,14 @@ static int target_xcopy_locate_se_dev_e4_iter(struct se_device *se_dev,
> >>> 	if (rc != 0)
> >>> 		return 0;
> >>> 
> >>> -	info->found_dev = se_dev;
> >>> 	pr_debug("XCOPY 0xe4: located se_dev: %p\n", se_dev);
> >>> +	if (info->found_dev) {
> >>> +		pr_warn("XCOPY 0xe4 descriptor conflict for se_dev %p and %p\n",
> >>> +			info->found_dev, se_dev);
> >>> +		target_undepend_item(&info->found_dev->dev_group.cg_item);
> >>> +		return -ENOTUNIQ;
> >>> +	}
> >>> +	info->found_dev = se_dev;    
> >> 
> >> Was it valid to copy to/from the same LUN? You would copy from/to different src/destinations on that LUN. Would your patch break that?  
> > 
> > XCOPY allows for copies to occur on the same LUN or between separate
> > src/destinations. The intention of this patch is that regardless of the
> > source or destination, if the NAA WWN could refer to multiple LUNs on
> > the same target (via target_for_each_device()) then the XCOPY should
> > fail and force the initiator to fallback to initiator driver copy.  
> 
> So is the answer to my question a maybe but it probably will never happen?

A srcŢst XCOPY? I think it's just as likely as a cross device XCOPY.
The UUID collision error is probably unlikely to be triggered because:
- XCOPY is a pretty exotic SCSI command mostly used by ESXi
- Users may already provide a vpd_unit_serial with enough unique
  hexadecimal characters(?)
- The initiator could detect the NAA WWN collision itself by comparing
  the INQUIRY(dev-id)->NAA between other LUNs on the target, and thereby
  detect+avoid sending XCOPY requests with ambiguous src/dest WWNs

> If the user has multiple backend devices with the same serial, then your patch would now return error right?

Yes, XCOPY will now fail if the src or dest NAA WWN matches more than
one se_dev. Keep in mind that the NAA WWN is derived from only the
hexadecimal characters of the vpd_unit_serial (see
spc_parse_naa_6h_vendor_specific), so collision might be more likely.

> Is the reason that this patch is a RFC to try and figure out if that case is valid or ever happens? If so, the only way I could see that happening on purpose is if someone was trying to bypass a device issue.

Sorry, I should have mode this more clear in the patch itself. The
reasons for RFC are:
- there might be a better approach. I considered detecting NAA WWN
  collisions when the vpd_unit_serial is set via configfs. Throwing a
  configfs error might break existing setups, rather than just
  triggering the XCOPY error (allowing for subsequent READ/WRITE
  fallback).
- I've tested this with libiscsi's iscsi-dd (-x) and test suite, but not
  against the ESXi initiator yet.

> For example, I create 2 tcmu devices. They both point to the same real device. Then export dev1 through target port1 and dev2 through target port2. Each tcmu device would then have it’s own data/cmd ring and locking, so you do not hit those perf issues. I do this for perf testing. I don’t think that type of thing is common or ever done, so I think the patch would be ok if that is a concern and it’s better than possible data corruption.
> 
> Code wise it looks ok to me.

Thanks a lot for the feedback, Mike.

Cheers, David