On Mon, 16 Aug 2021 18:16:45 +0000, Sergey Samoylenko wrote:

Hi David,

quoted

Hi Sergey,

On Thu, 29 Jul 2021 23:19:43 +0300, Sergey Samoylenko wrote:
 

quoted

The 8Fh VPD page announces the capabilities supported by
the TCM XCOPY manager. It helps to expand the coverage of
the third-party copy manager with SCSI testing utilities.  

Please list which initiators use this VPD page, if you know of any.  

I know that the ESXi 7.0 requests the 8Fh VPD page.

Thanks. Please put this in the commit message.

ESXi is one of
a few initiators who is using the XCOPY commands (vmkfstools tool).

quoted

Also, is there any test coverage for this? I don't see anything in
libiscsi...  

After activating XCOPY in a target we got an error from
the SCSI.ReceiveCopyResults.CopyStatus test in the libiscsi.
Discussing with Bart, we decided to implement the 8Fh VPD page
for announcing TCM XCOPY features.
It is here: https://github.com/sahlberg/libiscsi/pull/353

The libiscsi has an initial version for parsing 8Fh VPD. This is used
in the SCSI.ReceiveCopyResults.CopyStatus test. It is a good idea
to add test coverage for 8Fh VPD in libiscsi. I should do this.

quoted

quoted

Reviewed-by: Konstantin Shelekhin <redacted>
Reviewed-by: Dmitry Bogdanov <redacted>
Reviewed-by: Anastasia Kovaleva <redacted>
Signed-off-by: Sergey Samoylenko <redacted>
---
 drivers/target/target_core_spc.c | 230 ++++++++++++++++++++++++++++++-
 1 file changed, 226 insertions(+), 4 deletions(-)

diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
index 22703a0dbd07..169341712b10 100644
--- a/drivers/target/target_core_spc.c
+++ b/drivers/target/target_core_spc.c  

...  

quoted

+/* Third-party Copy VPD page */
+static sense_reason_t
+spc_emulate_evpd_8f(struct se_cmd *cmd, unsigned char *buf)
+{
+	struct se_device *dev = cmd->se_dev;
+	int off;
+	u16 page_len;
+
+	if (!dev->dev_attrib.emulate_3pc)
+		return TCM_INVALID_CDB_FIELD;
+
+	/*
+	 * Since the Third-party copy manager in TCM is quite simple
+	 * and supports only two commands, the function sets
+	 * many descriptor parameters as constants.
+	 *
+	 * As the Copy manager supports the EXTENDED COPY(LID1) command,
+	 * the Third-party Copy VPD page should include five mandatory
+	 * Third-party copy descriptors. Its are:
+	 *   0001h - Supported Commands
+	 *   0004h - Parameter Data
+	 *   0008h - Supported Descriptors
+	 *   000Ch - Supported CSCD Descriptor IDs
+	 *   8001h - General Copy Operations
+	 *
+	 * See spc4 section 7.8.17
+	 */
+
+	off = 4;
+
+	/* fill descriptors */
+	off += spc_evpd_8f_encode_supp_cmds(&buf[off]);
+	off += spc_evpd_8f_encode_param_data(&buf[off]);
+	off += spc_evpd_8f_encode_supp_descrs(&buf[off]);
+	off += spc_evpd_8f_encode_supp_cscd_descr_id(&buf[off]);
+	off += spc_evpd_8f_encode_general_copy_ops(&buf[off]);  

This looks risky in terms of buf overrun. I think it'd be good to pass
a @remaining or @buf_end param to these helper functions.  

I thought about it, but spc_emulate_evpd_XX functions have a prototype:
	sense_reason_t	(*emulate)(struct se_cmd *, unsigned char *);
and they don't know anything about buffer length. I can use the
"SE_INQUIRY_BUF" definition, but I don't like this solution.
	
We can change the prototype of spc_emulate_evpd_XX functions, like:
	static struct {
		uint8_t page;
		sense_reason_t	(*emulate)(struct se_cmd *, unsigned char *buf, size_t len);
	} evpd_handlers[] = {
		...
	};
and return the TCM_OUT_OF_RESOURCES if we try to overrun buffer
in spc_emulate_evpd_XX. But this will require changing all "emulate_evpd" functions.

David, what do you think of this?

Ideally inquiry and mode sense handlers, not to mention the configfs
callbacks, would all carry explicit bounds checks. As a start I'd be
fine with buflen=SE_INQUIRY_BUF at the top of spc_emulate_evpd_8f(), but
any further steps towards doing it properly would be helpful IMO.

Cheers, David