Re: [PATCH 1/1] scsi: target: core: Add 8Fh VPD page

[David Disseldorp <ddiss@suse.de>]

Hi Sergey,

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

> 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.
Also, is there any test coverage for this? I don't see anything in
libiscsi...

> Reviewed-by: Konstantin Shelekhin <k.shelekhin@yadro.com>
> Reviewed-by: Dmitry Bogdanov <d.bogdanov@yadro.com>
> Reviewed-by: Anastasia Kovaleva <a.kovaleva@yadro.com>
> Signed-off-by: Sergey Samoylenko <s.samoylenko@yadro.com>
> ---
>  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
...
> +/* 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.

Cheers, David