* [PATCH 0/1] scsi: target: core: Add 8Fh VPD page @ 2021-07-29 20:19 Sergey Samoylenko 2021-07-29 20:19 ` [PATCH 1/1] " Sergey Samoylenko 0 siblings, 1 reply; 7+ messages in thread From: Sergey Samoylenko @ 2021-07-29 20:19 UTC (permalink / raw) To: martin.petersen, michael.christie, ddiss, bvanassche, target-devel Cc: linux-scsi, linux, Sergey Samoylenko Continuing to cover the TCM copy manager with tests, there was a need for the Third-party Copy VPD page. Sergey Samoylenko (1): scsi: target: core: Add 8Fh VPD page drivers/target/target_core_spc.c | 230 ++++++++++++++++++++++++++++++- 1 file changed, 226 insertions(+), 4 deletions(-) -- 2.25.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/1] scsi: target: core: Add 8Fh VPD page 2021-07-29 20:19 [PATCH 0/1] scsi: target: core: Add 8Fh VPD page Sergey Samoylenko @ 2021-07-29 20:19 ` Sergey Samoylenko 2021-08-13 14:52 ` David Disseldorp 0 siblings, 1 reply; 7+ messages in thread From: Sergey Samoylenko @ 2021-07-29 20:19 UTC (permalink / raw) To: martin.petersen, michael.christie, ddiss, bvanassche, target-devel Cc: linux-scsi, linux, Sergey Samoylenko, Konstantin Shelekhin, Dmitry Bogdanov, Anastasia Kovaleva 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. 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 @@ -506,6 +506,217 @@ spc_emulate_evpd_86(struct se_cmd *cmd, unsigned char *buf) return 0; } +/* Third-party copy descriptor length is a multiple of four */ +static u16 __spc_evpd_8f_desc_calc_padding(u16 len) +{ + return (4 - (len & 3)) & 3; +} + +/* Supported Commands third-party copy descriptor, spc4 7.8.17.4 */ +static int spc_evpd_8f_encode_supp_cmds(unsigned char *buf) +{ + u16 off = 0; + + /* + * TCM supports only two Third-party copy commands: + * 83h/00h - EXTENDED COPY(LID1) + * 84h/03h - RECEIVE COPY OPERATING PARAMETERS + */ + + /* Set THIRD-PARTY COPY DESCRIPTOR TYPE (0001h) */ + put_unaligned_be16(0x0001, &buf[off]); + off += 2; + /* Skip THIRD-PARTY COPY DESCRIPTOR LENGTH */ + off += 2; + /* Set COMMANDS SUPPORTED LIST LENGTH for two codes */ + buf[off++] = 6; + /* First command support descriptor (EXTENDED COPY(LID1)) */ + buf[off++] = EXTENDED_COPY; /* operation code */ + buf[off++] = 1; /* action list length */ + buf[off++] = 0; /* service action */ + /* Second descriptor (RECEIVE COPY OPERATING PARAMETERS) */ + buf[off++] = RECEIVE_COPY_RESULTS; + buf[off++] = 1; + buf[off++] = RCR_SA_OPERATING_PARAMETERS; + /* Descriptor pad */ + off += __spc_evpd_8f_desc_calc_padding(off); + + /* Set THIRD-PARTY COPY DESCRIPTOR LENGTH */ + put_unaligned_be16(off - 4, &buf[2]); + + return off; +} + +/* Parameter Data third-party copy descriptor, spc4 7.8.17.5 */ +static int spc_evpd_8f_encode_param_data(unsigned char *buf) +{ + u16 off = 0; + + /* Set THIRD-PARTY COPY DESCRIPTOR TYPE (0004h) */ + put_unaligned_be16(0x0004, &buf[off]); + off += 2; + /* Set THIRD-PARTY COPY DESCRIPTOR LENGTH (001Ch) */ + put_unaligned_be16(0x001c, &buf[off]); + off += 2; + /* Skip over Reserved */ + off += 4; + /* Set MAXIMUM CSCD DESCRIPTOR COUNT */ + put_unaligned_be16(RCR_OP_MAX_TARGET_DESC_COUNT, &buf[off]); + off += 2; + /* Set MAXIMUM SEGMENT DESCRIPTOR COUNT */ + put_unaligned_be16(RCR_OP_MAX_SG_DESC_COUNT, &buf[off]); + off += 2; + /* Set MAXIMUM DESCRIPTOR LIST LENGTH */ + put_unaligned_be32(RCR_OP_MAX_DESC_LIST_LEN, &buf[off]); + off += 4; + /* + * The MAXIMUM INLINE DATA LENGTH field should be set to ZERO + * if the copy manager does not support segment descriptor + * type code 04h. + */ + off += 4; + /* Skip over Reserved */ + off += 12; + + return off; +} + +/* Supported Descriptors third-party copy descriptor, spc4 7.8.17.6 */ +static int spc_evpd_8f_encode_supp_descrs(unsigned char *buf) +{ + u16 off = 0; + + /* + * List of supported descriptor type codes: + * 02h - Copy from block device to block device segment descriptor + * E4h - Identification Descriptor CSCD descriptor + */ + + /* Set THIRD-PARTY COPY DESCRIPTOR TYPE (0008h) */ + put_unaligned_be16(0x0008, &buf[off]); + off += 2; + /* Skip THIRD-PARTY COPY DESCRIPTOR LENGTH */ + off += 2; + /* Set SUPPORTED DESCRIPTOR LIST LENGTH */ + buf[off++] = 2; + /* List of supported descriptor type codes */ + buf[off++] = 0x02; + buf[off++] = 0xe4; + /* Descriptor pad */ + off += __spc_evpd_8f_desc_calc_padding(off); + + /* Set THIRD-PARTY COPY DESCRIPTOR LENGTH */ + put_unaligned_be16(off - 4, &buf[2]); + + return off; +} + +/* Supported CSCD Descriptor IDs third-party copy descriptor, spc4 7.8.17.7 */ +static int spc_evpd_8f_encode_supp_cscd_descr_id(unsigned char *buf) +{ + u16 off = 0; + + /* + * The TCM copy manager doesn't support CSCD Descriptod IDs other + * than 0000h and therefore IDs List should be zero length. + */ + + /* Set THIRD-PARTY COPY DESCRIPTOR TYPE (000Ch) */ + put_unaligned_be16(0x000c, &buf[off]); + off += 2; + /* Skip THIRD-PARTY COPY DESCRIPTOR LENGTH */ + off += 2; + /* Set SUPPORTED CSCD DESCRIPTOR IDS LIST LENGTH to ZERO */ + off += 2; + /* Descriptor pad */ + off += __spc_evpd_8f_desc_calc_padding(off); + + /* Set THIRD-PARTY COPY DESCRIPTOR LENGTH */ + put_unaligned_be16(off - 4, &buf[2]); + + return off; +} + +/* General Copy Operations third-party copy descriptor, spc4 7.8.17.10 */ +static int spc_evpd_8f_encode_general_copy_ops(unsigned char *buf) +{ + u16 off = 0; + + /* Set THIRD-PARTY COPY DESCRIPTOR TYPE (8001h) */ + put_unaligned_be16(0x8001, &buf[off]); + off += 2; + /* Set THIRD-PARTY COPY DESCRIPTOR LENGTH (0020h) */ + put_unaligned_be16(0x0020, &buf[off]); + off += 2; + /* Set TOTAL CONCURRENT COPIES */ + put_unaligned_be32(RCR_OP_TOTAL_CONCURR_COPIES, &buf[off]); + off += 4; + /* Set MAXIMUM IDENTIFIED CONCURRENT COPIES */ + put_unaligned_be32(RCR_OP_MAX_CONCURR_COPIES, &buf[off]); + off += 4; + /* Set MAXIMUM SEGMENT LENGTH */ + put_unaligned_be32(RCR_OP_MAX_SEGMENT_LEN, &buf[off]); + off += 4; + /* Set DATA SEGMENT GRANULARITY (log 2) */ + buf[off++] = RCR_OP_DATA_SEG_GRAN_LOG2; + /* + * Set INLINE DATA GRANULARITY (log 2) to ZERO. + * TCM XCOPY manager does not support 04h (copy + * inline data to stream device) descriptor type. + */ + off += 1; + /* Reserved */ + off += 18; + + return off; +} + +/* 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]); + + /* + * Page Length for VPD 0x8f + */ + page_len = &buf[off] - &buf[0] - 4; + put_unaligned_be16(page_len, &buf[2]); + + return 0; +} + /* Block Limits VPD page */ static sense_reason_t spc_emulate_evpd_b0(struct se_cmd *cmd, unsigned char *buf) @@ -702,6 +913,7 @@ static struct { { .page = 0x80, .emulate = spc_emulate_evpd_80 }, { .page = 0x83, .emulate = spc_emulate_evpd_83 }, { .page = 0x86, .emulate = spc_emulate_evpd_86 }, + { .page = 0x8f, .emulate = spc_emulate_evpd_8f }, { .page = 0xb0, .emulate = spc_emulate_evpd_b0 }, { .page = 0xb1, .emulate = spc_emulate_evpd_b1 }, { .page = 0xb2, .emulate = spc_emulate_evpd_b2 }, @@ -712,7 +924,9 @@ static struct { static sense_reason_t spc_emulate_evpd_00(struct se_cmd *cmd, unsigned char *buf) { - int p; + bool emulate_3pc = cmd->se_dev->dev_attrib.emulate_3pc; + int i, p; + uint8_t page; /* * Only report the INQUIRY EVPD=1 pages after a valid NAA @@ -720,9 +934,17 @@ spc_emulate_evpd_00(struct se_cmd *cmd, unsigned char *buf) * during device creation/restart. */ if (cmd->se_dev->dev_flags & DF_EMULATED_VPD_UNIT_SERIAL) { - buf[3] = ARRAY_SIZE(evpd_handlers); - for (p = 0; p < ARRAY_SIZE(evpd_handlers); ++p) - buf[p + 4] = evpd_handlers[p].page; + for (i = 0, p = 4; i < ARRAY_SIZE(evpd_handlers); ++i) { + page = evpd_handlers[i].page; + /* + * The 8Fh VPD page is announced only if the copy + * manager is activated. + */ + if ((page == 0x8f) && !emulate_3pc) + continue; + buf[p++] = page; + } + buf[3] = p - 4; } return 0; -- 2.25.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] scsi: target: core: Add 8Fh VPD page 2021-07-29 20:19 ` [PATCH 1/1] " Sergey Samoylenko @ 2021-08-13 14:52 ` David Disseldorp 2021-08-16 18:13 ` Roman Bolshakov 2021-08-16 18:16 ` Sergey Samoylenko 0 siblings, 2 replies; 7+ messages in thread From: David Disseldorp @ 2021-08-13 14:52 UTC (permalink / raw) To: Sergey Samoylenko Cc: martin.petersen, michael.christie, bvanassche, target-devel, linux-scsi, linux, Konstantin Shelekhin, Dmitry Bogdanov, Anastasia Kovaleva 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] scsi: target: core: Add 8Fh VPD page 2021-08-13 14:52 ` David Disseldorp @ 2021-08-16 18:13 ` Roman Bolshakov 2021-08-16 22:00 ` David Disseldorp 2021-08-16 18:16 ` Sergey Samoylenko 1 sibling, 1 reply; 7+ messages in thread From: Roman Bolshakov @ 2021-08-16 18:13 UTC (permalink / raw) To: David Disseldorp Cc: Sergey Samoylenko, martin.petersen, michael.christie, bvanassche, target-devel, linux-scsi, linux, Konstantin Shelekhin, Dmitry Bogdanov, Anastasia Kovaleva On Fri, Aug 13, 2021 at 04:52:55PM +0200, David Disseldorp wrote: > 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... > Hi David, ESXi is one of the hosts that inspects Third Party Copy VPD Page. Windows detects ODX support using the page [1][2]. The page is also used by libiscsi to detect presence and features of copy manager as was agreed with Bart in the PR [3]: "Implementing REPORT SUPPORTED OPERATION CODES in LIO would require more work than implementing the third-party copy VPD page. I'm fine with relying on the third-party copy VPD page, or in other words, to skip the copy offloading tests if that page is not supported. There are plans to implement XCOPY support in the Linux kernel sd driver. If nobody else volunteers I plan to work on this myself. I'm considering to only support SCSI targets that support the third-party copy VPD page. Or in other words, we will need support for that VPD page anyway." 1. https://www.slideshare.net/CalvinChen5/a-joint-effort-of-the-storage-industry 2. http://sg.danny.cz/sg/ddpt_xcopy_odx.html 3. https://github.com/sahlberg/libiscsi/pull/353 > > 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. > It's doable but would require to change the signature of all existing VPD handlers. SE_INQUIRY_BUF is hardcoded to 1kb but it's also capped by EDTL to avoid buffer overruns: memcpy(rbuf, buf, min_t(u32, SE_INQUIRY_BUF, cmd->data_length)); Regards, Roman ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] scsi: target: core: Add 8Fh VPD page 2021-08-16 18:13 ` Roman Bolshakov @ 2021-08-16 22:00 ` David Disseldorp 0 siblings, 0 replies; 7+ messages in thread From: David Disseldorp @ 2021-08-16 22:00 UTC (permalink / raw) To: Roman Bolshakov Cc: Sergey Samoylenko, martin.petersen, michael.christie, bvanassche, target-devel, linux-scsi, linux, Konstantin Shelekhin, Dmitry Bogdanov, Anastasia Kovaleva On Mon, 16 Aug 2021 21:13:59 +0300, Roman Bolshakov wrote: > On Fri, Aug 13, 2021 at 04:52:55PM +0200, David Disseldorp wrote: > > 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... > > > > Hi David, > > ESXi is one of the hosts that inspects Third Party Copy VPD Page. > Windows detects ODX support using the page [1][2]. Thanks for the links. I haven't seen ESXi attempt to use it, but also haven't checked for some time. It'd be good to get some of this information in the commit message. > The page is also used by libiscsi to detect presence and features of > copy manager as was agreed with Bart in the PR [3]: I'm probably missing something, but why wasn't the 3PC flag in the standard inquiry page an option for this check? > "Implementing REPORT SUPPORTED OPERATION CODES in LIO would require more > work than implementing the third-party copy VPD page. > > I'm fine with relying on the third-party copy VPD page, or in other > words, to skip the copy offloading tests if that page is not supported. > > There are plans to implement XCOPY support in the Linux kernel sd > driver. If nobody else volunteers I plan to work on this myself. I'm > considering to only support SCSI targets that support the third-party > copy VPD page. Or in other words, we will need support for that VPD page > anyway." Okay, fair enough. > > 1. https://www.slideshare.net/CalvinChen5/a-joint-effort-of-the-storage-industry > 2. http://sg.danny.cz/sg/ddpt_xcopy_odx.html > 3. https://github.com/sahlberg/libiscsi/pull/353 > > > > 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. > > > > It's doable but would require to change the signature of all existing > VPD handlers. SE_INQUIRY_BUF is hardcoded to 1kb but it's also capped by > EDTL to avoid buffer overruns: > > memcpy(rbuf, buf, min_t(u32, SE_INQUIRY_BUF, cmd->data_length)); That's checking the amount copied into the response buffer. My concern is the prior writes to the staging buf. Cheers, David ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH 1/1] scsi: target: core: Add 8Fh VPD page 2021-08-13 14:52 ` David Disseldorp 2021-08-16 18:13 ` Roman Bolshakov @ 2021-08-16 18:16 ` Sergey Samoylenko 2021-08-16 22:48 ` David Disseldorp 1 sibling, 1 reply; 7+ messages in thread From: Sergey Samoylenko @ 2021-08-16 18:16 UTC (permalink / raw) To: David Disseldorp Cc: martin.petersen@oracle.com, michael.christie@oracle.com, bvanassche@acm.org, target-devel@vger.kernel.org, linux-scsi@vger.kernel.org, linux@yadro.com, Konstantin Shelekhin, Dmitriy Bogdanov, Anastasia Kovaleva Hi David, > 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. I know that the ESXi 7.0 requests the 8Fh VPD page. ESXi is one of a few initiators who is using the XCOPY commands (vmkfstools tool). > 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. > >> 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. 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? > > Cheers, David Best regards, Sergey ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] scsi: target: core: Add 8Fh VPD page 2021-08-16 18:16 ` Sergey Samoylenko @ 2021-08-16 22:48 ` David Disseldorp 0 siblings, 0 replies; 7+ messages in thread From: David Disseldorp @ 2021-08-16 22:48 UTC (permalink / raw) To: Sergey Samoylenko Cc: martin.petersen@oracle.com, michael.christie@oracle.com, bvanassche@acm.org, target-devel@vger.kernel.org, linux-scsi@vger.kernel.org, linux@yadro.com, Konstantin Shelekhin, Dmitriy Bogdanov, Anastasia Kovaleva On Mon, 16 Aug 2021 18:16:45 +0000, Sergey Samoylenko wrote: > Hi David, > > > 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. > 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). > > > 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. > > > > >> 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. > > 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-08-16 22:48 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-07-29 20:19 [PATCH 0/1] scsi: target: core: Add 8Fh VPD page Sergey Samoylenko 2021-07-29 20:19 ` [PATCH 1/1] " Sergey Samoylenko 2021-08-13 14:52 ` David Disseldorp 2021-08-16 18:13 ` Roman Bolshakov 2021-08-16 22:00 ` David Disseldorp 2021-08-16 18:16 ` Sergey Samoylenko 2021-08-16 22:48 ` David Disseldorp
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.