linux-scsi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] scsi: target: core: check SR field in REPORT LUNS
@ 2021-01-20 10:27 Dmitry Bogdanov
  2021-01-22 22:42 ` David Disseldorp
  2021-01-27  4:36 ` Martin K. Petersen
  0 siblings, 2 replies; 8+ messages in thread
From: Dmitry Bogdanov @ 2021-01-20 10:27 UTC (permalink / raw)
  To: Martin Petersen, target-devel
  Cc: linux-scsi, linux, Dmitry Bogdanov, Roman Bolshakov

Now REPORT LUNS for software device servers always reports all luns
regardless of SELECT REPORT field.
Add handling of that field according to SPC-4:
* accept known values,
* reject unknown values.

Changes since v1:
- unmap transport data in error handling case

Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com>
Signed-off-by: Dmitry Bogdanov <d.bogdanov@yadro.com>
---
patch to 5.11/scsi-queue

 drivers/target/target_core_spc.c | 27 +++++++++++++++++++++++++++
 include/scsi/scsi_proto.h        | 10 ++++++++++
 2 files changed, 37 insertions(+)

diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
index ca5579ebc81d..044ac45cdf47 100644
--- a/drivers/target/target_core_spc.c
+++ b/drivers/target/target_core_spc.c
@@ -1210,10 +1210,12 @@ sense_reason_t spc_emulate_report_luns(struct se_cmd *cmd)
 {
 	struct se_dev_entry *deve;
 	struct se_session *sess = cmd->se_sess;
+	unsigned char *cdb = cmd->t_task_cdb;
 	struct se_node_acl *nacl;
 	struct scsi_lun slun;
 	unsigned char *buf;
 	u32 lun_count = 0, offset = 8;
+	u8 sr = cdb[2];
 	__be32 len;
 
 	buf = transport_kmap_data_sg(cmd);
@@ -1230,6 +1232,28 @@ sense_reason_t spc_emulate_report_luns(struct se_cmd *cmd)
 
 	nacl = sess->se_node_acl;
 
+	switch (sr) {
+	case SCSI_SELECT_WELLKNOWN:
+	case SCSI_SELECT_ADMINISTRATIVE:
+	case SCSI_SELECT_SUBSIDIARY:
+		/* report empty lun list */
+		goto out;
+	case SCSI_SELECT_TOP_LEVEL:
+		if (cmd->se_lun->unpacked_lun != 0)
+			goto out;
+		fallthrough;
+	case SCSI_SELECT_REGULAR:
+	case SCSI_SELECT_ALL_ACCESSIBLE:
+		break;
+	default:
+		pr_debug("TARGET_CORE[%s]: Invalid REPORT LUNS with unsupported "
+				 "SELECT REPORT %#x for 0x%08llx from %s\n",
+				 cmd->se_tfo->fabric_name, sr, cmd->se_lun->unpacked_lun,
+				 sess->se_node_acl->initiatorname);
+		transport_kunmap_data_sg(cmd);
+		return TCM_INVALID_CDB_FIELD;
+	}
+
 	rcu_read_lock();
 	hlist_for_each_entry_rcu(deve, &nacl->lun_entry_hlist, link) {
 		/*
@@ -1252,6 +1276,8 @@ sense_reason_t spc_emulate_report_luns(struct se_cmd *cmd)
 	 * See SPC3 r07, page 159.
 	 */
 done:
+	if ((sr != SCSI_SELECT_REGULAR) && (sr != SCSI_SELECT_ALL_ACCESSIBLE))
+		goto out;
 	/*
 	 * If no LUNs are accessible, report virtual LUN 0.
 	 */
@@ -1263,6 +1289,7 @@ sense_reason_t spc_emulate_report_luns(struct se_cmd *cmd)
 		lun_count = 1;
 	}
 
+out:
 	if (buf) {
 		len = cpu_to_be32(lun_count * 8);
 		memcpy(buf, &len, min_t(int, sizeof len, cmd->data_length));
diff --git a/include/scsi/scsi_proto.h b/include/scsi/scsi_proto.h
index c36860111932..280169c75d85 100644
--- a/include/scsi/scsi_proto.h
+++ b/include/scsi/scsi_proto.h
@@ -341,4 +341,14 @@ enum zbc_zone_cond {
 	ZBC_ZONE_COND_OFFLINE		= 0xf,
 };
 
+/* Select Report fot REPORT LUNS */
+enum scsi_select_report {
+	SCSI_SELECT_REGULAR		= 0x0,
+	SCSI_SELECT_WELLKNOWN		= 0x1,
+	SCSI_SELECT_ALL_ACCESSIBLE	= 0x2,
+	SCSI_SELECT_ADMINISTRATIVE	= 0x10,
+	SCSI_SELECT_TOP_LEVEL		= 0x11,
+	SCSI_SELECT_SUBSIDIARY		= 0x12,
+};
+
 #endif /* _SCSI_PROTO_H_ */
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] scsi: target: core: check SR field in REPORT LUNS
  2021-01-20 10:27 [PATCH v2] scsi: target: core: check SR field in REPORT LUNS Dmitry Bogdanov
@ 2021-01-22 22:42 ` David Disseldorp
  2021-01-26  9:13   ` Roman Bolshakov
  2021-01-27  4:36 ` Martin K. Petersen
  1 sibling, 1 reply; 8+ messages in thread
From: David Disseldorp @ 2021-01-22 22:42 UTC (permalink / raw)
  To: Dmitry Bogdanov
  Cc: Martin Petersen, target-devel, linux-scsi, linux, Roman Bolshakov

Hi,

On Wed, 20 Jan 2021 13:27:00 +0300, Dmitry Bogdanov wrote:

> Now REPORT LUNS for software device servers always reports all luns
> regardless of SELECT REPORT field.
> Add handling of that field according to SPC-4:
> * accept known values,
> * reject unknown values.

We currently advertise SPC-3 VERSION compliance via standard INQUIRY
data, so I think we should either support SPC-3 SELECT REPORT values or
bump the VERSION field (SPC-4 behaviour is already scattered throughout
LIO).
Out of curiosity, do you know of any initiators which use this field?

Cheers, David

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] scsi: target: core: check SR field in REPORT LUNS
  2021-01-22 22:42 ` David Disseldorp
@ 2021-01-26  9:13   ` Roman Bolshakov
  2021-01-28 20:28     ` David Disseldorp
  0 siblings, 1 reply; 8+ messages in thread
From: Roman Bolshakov @ 2021-01-26  9:13 UTC (permalink / raw)
  To: David Disseldorp
  Cc: Dmitry Bogdanov, Martin Petersen, target-devel, linux-scsi, linux

On Fri, Jan 22, 2021 at 11:42:51PM +0100, David Disseldorp wrote:
> On Wed, 20 Jan 2021 13:27:00 +0300, Dmitry Bogdanov wrote:
> 
> > Now REPORT LUNS for software device servers always reports all luns
> > regardless of SELECT REPORT field.
> > Add handling of that field according to SPC-4:
> > * accept known values,
> > * reject unknown values.
> 
> We currently advertise SPC-3 VERSION compliance via standard INQUIRY
> data, so I think we should either support SPC-3 SELECT REPORT values or
> bump the VERSION field (SPC-4 behaviour is already scattered throughout
> LIO).
> Out of curiosity, do you know of any initiators which use this field?
> 

Hi David,

SELECT REPORT field can be used for vVOL (LU conglomerate) discovery and
for well-known lun listing.

The field is used by VMware ESXi:
https://support.purestorage.com/Solutions/VMware_Platform_Guide/User_Guides_for_VMware_Solutions/Virtual_Volumes_User_Guide/vVols_User_Guide%3A_Protocol_Endpoints

"PEs greatly extend the number of vVols that can be connected to an ESXi
cluster; each PE can have up to 16,383 vVols per host bound to it
simultaneously. Moreover, a new binding does not require a complete I/O
rescan. Instead, ESXi issues a REPORT_LUNS SCSI command with SELECT
REPORT to the PE to which the sub-lun is bound. The PE returns a list of
sub-lun IDs for the vVols bound to that host. In large clusters,
REPORT_LUNS is significantly faster than a full I/O rescan because it is
more precisely targeted."

The post also confirms that:
https://sourceforge.net/p/scst/mailman/message/33030432/

A few more targets that support SELECT REPORT field below.

Sun/Oracle tape library:
https://docs.oracle.com/en/storage/tape-storage/storagetek-sl150-modular-tape-library/slorm/report-luns-a0h.html#GUID-4140F40D-BD9A-495C-9A86-8BD7E91C985C

IBM Flash Storage:
https://www.ibm.com/support/pages/sites/default/files/support/ssg/ssgdocs.nsf/0/95d7115d7eb428e485257f80005cc3a7/%24FILE/FlashSystem_840_SCSI_Interface_Manual_1.2.pdf

With regards to bumping TCM to SPC-4, are there any objections if we
submit a separate patch for that? Or resubmit a series with the patch?

Thanks,
Roman

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] scsi: target: core: check SR field in REPORT LUNS
  2021-01-20 10:27 [PATCH v2] scsi: target: core: check SR field in REPORT LUNS Dmitry Bogdanov
  2021-01-22 22:42 ` David Disseldorp
@ 2021-01-27  4:36 ` Martin K. Petersen
  2021-01-27 14:45   ` Dmitriy Bogdanov
  1 sibling, 1 reply; 8+ messages in thread
From: Martin K. Petersen @ 2021-01-27  4:36 UTC (permalink / raw)
  To: Dmitry Bogdanov
  Cc: Martin Petersen, target-devel, linux-scsi, linux, Roman Bolshakov


Dmitry,

> +	switch (sr) {
> +	case SCSI_SELECT_WELLKNOWN:
> +	case SCSI_SELECT_ADMINISTRATIVE:
> +	case SCSI_SELECT_SUBSIDIARY:
> +		/* report empty lun list */
> +		goto out;

I'm a bit concerned about things inadvertently breaking if we return an
empty list for the well known LUNs.

-- 
Martin K. Petersen	Oracle Linux Engineering

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH v2] scsi: target: core: check SR field in REPORT LUNS
  2021-01-27  4:36 ` Martin K. Petersen
@ 2021-01-27 14:45   ` Dmitriy Bogdanov
  2021-01-28 20:47     ` Bart Van Assche
  0 siblings, 1 reply; 8+ messages in thread
From: Dmitriy Bogdanov @ 2021-01-27 14:45 UTC (permalink / raw)
  To: Martin K. Petersen; +Cc: target-devel, linux-scsi, linux, Roman Bolshakov

Hi Martin,

>> +	switch (sr) {
>> +	case SCSI_SELECT_WELLKNOWN:
>> +	case SCSI_SELECT_ADMINISTRATIVE:
>> +	case SCSI_SELECT_SUBSIDIARY:
>> +		/* report empty lun list */
>> +		goto out;

> I'm a bit concerned about things inadvertently breaking if we return an empty list for the well known LUNs.
According to SPC we shall report an empty list if there is no well-known LUNS. 
FreeBSD has the same logic in REPORT LUNS handling. SCST does not support SELECT_WELLKNOWN case at all.

I don't know the history of the existing behaviour to send always LUN0 instead of empty list. Probably it was 
for the SCSI_SELECT_ALL_ACCESSIBLE(0x02) case, where SPC allows LUN0. My patch keeps it for the 0x00, 0x02, 0x11 cases.
Thus, I believe it does not break the backward compatibility.

BR,
 Dmitry

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] scsi: target: core: check SR field in REPORT LUNS
  2021-01-26  9:13   ` Roman Bolshakov
@ 2021-01-28 20:28     ` David Disseldorp
  0 siblings, 0 replies; 8+ messages in thread
From: David Disseldorp @ 2021-01-28 20:28 UTC (permalink / raw)
  To: Roman Bolshakov
  Cc: Dmitry Bogdanov, Martin Petersen, target-devel, linux-scsi, linux

Hi Roman,

On Tue, 26 Jan 2021 12:13:44 +0300, Roman Bolshakov wrote:

> Hi David,
> 
> SELECT REPORT field can be used for vVOL (LU conglomerate) discovery and
> for well-known lun listing.
> 
> The field is used by VMware ESXi:
> https://support.purestorage.com/Solutions/VMware_Platform_Guide/User_Guides_for_VMware_Solutions/Virtual_Volumes_User_Guide/vVols_User_Guide%3A_Protocol_Endpoints
> 
> "PEs greatly extend the number of vVols that can be connected to an ESXi
> cluster; each PE can have up to 16,383 vVols per host bound to it
> simultaneously. Moreover, a new binding does not require a complete I/O
> rescan. Instead, ESXi issues a REPORT_LUNS SCSI command with SELECT
> REPORT to the PE to which the sub-lun is bound. The PE returns a list of
> sub-lun IDs for the vVols bound to that host. In large clusters,
> REPORT_LUNS is significantly faster than a full I/O rescan because it is
> more precisely targeted."

Interesting, thanks.

...
> With regards to bumping TCM to SPC-4, are there any objections if we
> submit a separate patch for that? Or resubmit a series with the patch?

I don't object to that. FWIW, the following crude metrics could be seen
as an argument in favour of SPC-4 versioning:
  linux> git grep -ic -e "SPC4" -e "SPC-4" -- drivers/target/
  drivers/target/target_core_alua.c:7
  drivers/target/target_core_alua.h:5
  drivers/target/target_core_device.c:1
  drivers/target/target_core_fabric_lib.c:5
  drivers/target/target_core_pr.c:37
  drivers/target/target_core_spc.c:19
  drivers/target/target_core_tmr.c:3
  drivers/target/target_core_transport.c:2
  drivers/target/target_core_ua.c:1
  drivers/target/target_core_ua.h:1
  drivers/target/target_core_xcopy.c:3
  drivers/target/target_core_xcopy.h:1
  linux> git grep -ic -e "SPC3" -e "SPC-3" -- drivers/target/
  drivers/target/target_core_alua.c:1
  drivers/target/target_core_configfs.c:14
  drivers/target/target_core_pr.c:75
  drivers/target/target_core_spc.c:4
  drivers/target/target_core_tmr.c:1
  drivers/target/target_core_transport.c:4
  drivers/target/target_core_ua.c:1

Most of the SPC3 target_core_pr and target_core_configfs matches above
are debug/error messages, rather than spec references.

Cheers, David

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] scsi: target: core: check SR field in REPORT LUNS
  2021-01-27 14:45   ` Dmitriy Bogdanov
@ 2021-01-28 20:47     ` Bart Van Assche
  2021-02-19 13:48       ` Dmitriy Bogdanov
  0 siblings, 1 reply; 8+ messages in thread
From: Bart Van Assche @ 2021-01-28 20:47 UTC (permalink / raw)
  To: Dmitriy Bogdanov, Martin K. Petersen
  Cc: target-devel, linux-scsi, linux, Roman Bolshakov

On 1/27/21 6:45 AM, Dmitriy Bogdanov wrote:
> Hi Martin,
> 
>>> +	switch (sr) {
>>> +	case SCSI_SELECT_WELLKNOWN:
>>> +	case SCSI_SELECT_ADMINISTRATIVE:
>>> +	case SCSI_SELECT_SUBSIDIARY:
>>> +		/* report empty lun list */
>>> +		goto out;
> 
>> I'm a bit concerned about things inadvertently breaking if we return an empty list for the well known LUNs.
 >
> According to SPC we shall report an empty list if there is no well-known LUNS.
> FreeBSD has the same logic in REPORT LUNS handling. SCST does not support SELECT_WELLKNOWN case at all.
> 
> I don't know the history of the existing behaviour to send always LUN0 instead of empty list. Probably it was
> for the SCSI_SELECT_ALL_ACCESSIBLE(0x02) case, where SPC allows LUN0. My patch keeps it for the 0x00, 0x02, 0x11 cases.
> Thus, I believe it does not break the backward compatibility.

Will this change require users to update their LUN configuration? Some 
initiator operating systems require presence of a dummy LUN 0. Although 
I agree that it is cleaner not to provide a hardcoded LUN 0, I think 
Martin is concerned about this patch potentially breaking existing 
configurations and causing frustration among LIO users.

Bart.


^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH v2] scsi: target: core: check SR field in REPORT LUNS
  2021-01-28 20:47     ` Bart Van Assche
@ 2021-02-19 13:48       ` Dmitriy Bogdanov
  0 siblings, 0 replies; 8+ messages in thread
From: Dmitriy Bogdanov @ 2021-02-19 13:48 UTC (permalink / raw)
  To: Bart Van Assche, Martin K. Petersen
  Cc: target-devel, linux-scsi, linux, Roman Bolshakov

Hi,

>>> I'm a bit concerned about things inadvertently breaking if we return an empty list for the well known LUNs.
> >
>> According to SPC we shall report an empty list if there is no well-known LUNS.
>> FreeBSD has the same logic in REPORT LUNS handling. SCST does not support SELECT_WELLKNOWN case at all.
>> 
>> I don't know the history of the existing behaviour to send always LUN0 instead of empty list. Probably it was
>> for the SCSI_SELECT_ALL_ACCESSIBLE(0x02) case, where SPC allows LUN0. My patch keeps it for the 0x00, 0x02, 0x11 cases.
>> Thus, I believe it does not break the backward compatibility.

>Will this change require users to update their LUN configuration? Some 
>initiator operating systems require presence of a dummy LUN 0. Although 
>I agree that it is cleaner not to provide a hardcoded LUN 0, I think 
>Martin is concerned about this patch potentially breaking existing 
>configurations and causing frustration among LIO users.

No reconfiguration on initiator side is required.
W-LUN is a specific LUN for the specific SCSI target device that is well known
for the Initiator. Generic Linux TCM does not have W-LUNs. Some storage
systems based on Linux TCM may have W-LUNs. But then they shall / already have
its own handling of REPORT LUNS command.
Basically, it is an error to report LUN0 as W-LUN for the Initiator that
expects some other numbers.

BR,
 Dmitry

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-02-19 13:49 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-20 10:27 [PATCH v2] scsi: target: core: check SR field in REPORT LUNS Dmitry Bogdanov
2021-01-22 22:42 ` David Disseldorp
2021-01-26  9:13   ` Roman Bolshakov
2021-01-28 20:28     ` David Disseldorp
2021-01-27  4:36 ` Martin K. Petersen
2021-01-27 14:45   ` Dmitriy Bogdanov
2021-01-28 20:47     ` Bart Van Assche
2021-02-19 13:48       ` Dmitriy Bogdanov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).