[PATCH v2 0/5] Bring the BusLogic host bus adapter driver up to Y2021

[PATCH v2 0/5] Bring the BusLogic host bus adapter driver up to Y2021

[Maciej W. Rozycki]

Hi,

 This is v2 of the series with 2/5 updated to use `vscnprintf' rather than 
`vsnprintf'.  No other changes.

 So we are here owing to Christoph's recent ISA bounce buffering sweep: 
<https://lore.kernel.org/linux-scsi/20210331073001.46776-1-hch@lst.de/T/#m981284e74e93216626a0728ce1601ca18fca92e8> 
which has prompted me to verify the current version of Linux with my old 
server, which has been long equipped with venerable Linux 2.6.18 and which 
I now have available for general experimenting, and the BusLogic BT-958 
PCI SCSI host bus adapter the server has used for 20-something years now.  
This revealed numerous issues with the BusLogic driver.

 Firstly (1/5) it has suffered from some bitrot and messages produced have 
become messy from the lack of update for proper `pr_cont' support.

 Secondly (2/5) there has been a potential buffer overrun/stack corruption 
security issue from using an unbounded `vsprintf' call.

 Thirdly (3/5) it has become obvious the BusLogic driver would have been 
non-functional, should I have upgraded the kernel, at least with this 
configuration for some 8 years now, and the underlying cause has been a 
long-known issue with the MultiMaster firmware I have dealt with already, 
back in 2003.  To put it short the firmware cannot cope with commands that 
request an allocation length exceeding the length of actual data returned.

 I have originally observed it with a LOG SENSE command in the course of 
investigating why smartmontools bring the system to a death, and worked it 
around: <https://sourceforge.net/p/smartmontools/mailman/message/4993087/> 
by issuing the command twice, first just to obtain the allocation length 
required.  As it turns out we need a similar workaround in the kernel now.

 But in the course of investigating this issue I have discovered there is 
a second bottom to it and hence I have prepared follow-up changes (4-5/5) 
to address problems with our handling of Vital Product Data INQUIRY pages.

 See individual change descriptions for further details.

 Questions, comments, concerns?  Otherwise please apply.

  Maciej

[PATCH v2 1/5] scsi: BusLogic: Fix missing `pr_cont' use

[Maciej W. Rozycki]

Update BusLogic driver's messaging system to use `pr_cont' for 
continuation lines, bringing messy output:

pci 0000:00:13.0: PCI->APIC IRQ transform: INT A -> IRQ 17
scsi: ***** BusLogic SCSI Driver Version 2.1.17 of 12 September 2013 *****
scsi: Copyright 1995-1998 by Leonard N. Zubkoff <lnz@dandelion.com>
scsi0: Configuring BusLogic Model BT-958 PCI Wide Ultra SCSI Host Adapter
scsi0:   Firmware Version: 5.07B, I/O Address: 0x7000, IRQ Channel: 17/Level
scsi0:   PCI Bus: 0, Device: 19, Address:
0xE0012000,
Host Adapter SCSI ID: 7
scsi0:   Parity Checking: Enabled, Extended Translation: Enabled
scsi0:   Synchronous Negotiation: Ultra, Wide Negotiation: Enabled
scsi0:   Disconnect/Reconnect: Enabled, Tagged Queuing: Enabled
scsi0:   Scatter/Gather Limit: 128 of 8192 segments, Mailboxes: 211
scsi0:   Driver Queue Depth: 211, Host Adapter Queue Depth: 192
scsi0:   Tagged Queue Depth:
Automatic
, Untagged Queue Depth: 3
scsi0:   SCSI Bus Termination: Both Enabled
, SCAM: Disabled

scsi0: *** BusLogic BT-958 Initialized Successfully ***
scsi host0: BusLogic BT-958

back to order:

pci 0000:00:13.0: PCI->APIC IRQ transform: INT A -> IRQ 17
scsi: ***** BusLogic SCSI Driver Version 2.1.17 of 12 September 2013 *****
scsi: Copyright 1995-1998 by Leonard N. Zubkoff <lnz@dandelion.com>
scsi0: Configuring BusLogic Model BT-958 PCI Wide Ultra SCSI Host Adapter
scsi0:   Firmware Version: 5.07B, I/O Address: 0x7000, IRQ Channel: 17/Level
scsi0:   PCI Bus: 0, Device: 19, Address: 0xE0012000, Host Adapter SCSI ID: 7
scsi0:   Parity Checking: Enabled, Extended Translation: Enabled
scsi0:   Synchronous Negotiation: Ultra, Wide Negotiation: Enabled
scsi0:   Disconnect/Reconnect: Enabled, Tagged Queuing: Enabled
scsi0:   Scatter/Gather Limit: 128 of 8192 segments, Mailboxes: 211
scsi0:   Driver Queue Depth: 211, Host Adapter Queue Depth: 192
scsi0:   Tagged Queue Depth: Automatic, Untagged Queue Depth: 3
scsi0:   SCSI Bus Termination: Both Enabled, SCAM: Disabled
scsi0: *** BusLogic BT-958 Initialized Successfully ***
scsi host0: BusLogic BT-958

Also diagnostic output such as with the `BusLogic=TraceConfiguration' 
parameter is affected and becomes vertical and therefore hard to read.  
This has now been corrected, e.g.:

pci 0000:00:13.0: PCI->APIC IRQ transform: INT A -> IRQ 17
blogic_cmd(86) Status = 30:  4 ==>  4: FF 05 93 00
blogic_cmd(95) Status = 28: (Modify I/O Address)
blogic_cmd(91) Status = 30:  1 ==>  1: 01
blogic_cmd(04) Status = 30:  4 ==>  4: 41 41 35 30
blogic_cmd(8D) Status = 30: 14 ==> 14: 45 DC 00 20 00 00 00 00 00 40 30 37 42 1D
scsi: ***** BusLogic SCSI Driver Version 2.1.17 of 12 September 2013 *****
scsi: Copyright 1995-1998 by Leonard N. Zubkoff <lnz@dandelion.com>
blogic_cmd(04) Status = 30:  4 ==>  4: 41 41 35 30
blogic_cmd(0B) Status = 30:  3 ==>  3: 00 08 07
blogic_cmd(0D) Status = 30: 34 ==> 34: 03 01 07 04 00 00 00 00 00 00 00 00 00 00 00 00 FF 42 44 46 FF 00 00 00 00 00 00 00 00 00 FF 00 FF 00
blogic_cmd(8D) Status = 30: 14 ==> 14: 45 DC 00 20 00 00 00 00 00 40 30 37 42 1D
blogic_cmd(84) Status = 30:  1 ==>  1: 37
blogic_cmd(8B) Status = 30:  5 ==>  5: 39 35 38 20 20
blogic_cmd(85) Status = 30:  1 ==>  1: 42
blogic_cmd(86) Status = 30:  4 ==>  4: FF 05 93 00
blogic_cmd(91) Status = 30: 64 ==> 64: 41 46 3E 20 39 35 38 20 20 00 C4 00 04 01 07 2F 07 04 35 FF FF FF FF FF FF FF FF FF FF 01 00 FE FF 08 FF FF 00 00 00 00 00 00 00 01 00 01 00 00 FF FF 00 00 00 00 00 00 00 00 00 00 00 00 00 FC
scsi0: Configuring BusLogic Model BT-958 PCI Wide Ultra SCSI Host Adapter

etc.

Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
Fixes: 4bcc595ccd80 ("printk: reinstate KERN_CONT for printing continuation lines")
Cc: stable@vger.kernel.org # v4.9+
---
No changes from v1.
---
 drivers/scsi/BusLogic.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

linux-buslogic-pr-cont.diff
Index: linux-macro-ide/drivers/scsi/BusLogic.c
===================================================================
--- linux-macro-ide.orig/drivers/scsi/BusLogic.c
+++ linux-macro-ide/drivers/scsi/BusLogic.c
@@ -3603,7 +3603,7 @@ static void blogic_msg(enum blogic_msgle
 			if (buf[0] != '\n' || len > 1)
 				printk("%sscsi%d: %s", blogic_msglevelmap[msglevel], adapter->host_no, buf);
 		} else
-			printk("%s", buf);
+			pr_cont("%s", buf);
 	} else {
 		if (begin) {
 			if (adapter != NULL && adapter->adapter_initd)
@@ -3611,7 +3611,7 @@ static void blogic_msg(enum blogic_msgle
 			else
 				printk("%s%s", blogic_msglevelmap[msglevel], buf);
 		} else
-			printk("%s", buf);
+			pr_cont("%s", buf);
 	}
 	begin = (buf[len - 1] == '\n');
 }

RE: [PATCH v2 1/5] scsi: BusLogic: Fix missing `pr_cont' use

[David Laight]

From: Maciej W. Rozycki
> Sent: 20 April 2021 19:02
> 
> Update BusLogic driver's messaging system to use `pr_cont' for
> continuation lines, bringing messy output:

If reasonably possible it is best to avoid use of pr_cont().

If there are concurrent writes from multiple cpu I believe
the writes still get separated.
(Something has to give...)

For instance I've seen concurrent 'oops' generate the list of
loaded modules one module per line with the two lists interleaved!

Quite often one of the %p[A-Z] modifiers can help.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [PATCH v2 1/5] scsi: BusLogic: Fix missing `pr_cont' use

[Maciej W. Rozycki]

On Wed, 21 Apr 2021, David Laight wrote:

> > Update BusLogic driver's messaging system to use `pr_cont' for
> > continuation lines, bringing messy output:
> 
> If reasonably possible it is best to avoid use of pr_cont().

 I know, however for that the whole driver's messaging system would have 
to be redesigned.  Joe (cc-ed) has offered to do it with the original 
iteration, however I believe consensus has been it will best be done as a 
separate follow-up change, while this small fix can be easily backported.

> If there are concurrent writes from multiple cpu I believe
> the writes still get separated.
> (Something has to give...)

 NB this driver may not see a lot of SMP use as I reckon it's had some 
portability issues, and in any case the hardware requires port I/O which 
precludes its use with some of the most recent PCIe systems which do not 
support PCI I/O cycles anymore.  And older systems were often UP.  Not 
that the driver should not be kept in a clean style of course.

  Maciej

Re: [PATCH v2 1/5] scsi: BusLogic: Fix missing `pr_cont' use

[Khalid Aziz]

On 4/20/21 12:01 PM, Maciej W. Rozycki wrote:
> Update BusLogic driver's messaging system to use `pr_cont' for 
> continuation lines, bringing messy output:
> 
> pci 0000:00:13.0: PCI->APIC IRQ transform: INT A -> IRQ 17
> scsi: ***** BusLogic SCSI Driver Version 2.1.17 of 12 September 2013 *****
> scsi: Copyright 1995-1998 by Leonard N. Zubkoff <lnz@dandelion.com>
> scsi0: Configuring BusLogic Model BT-958 PCI Wide Ultra SCSI Host Adapter
> scsi0:   Firmware Version: 5.07B, I/O Address: 0x7000, IRQ Channel: 17/Level
> scsi0:   PCI Bus: 0, Device: 19, Address:
> 0xE0012000,
> Host Adapter SCSI ID: 7
> scsi0:   Parity Checking: Enabled, Extended Translation: Enabled
> scsi0:   Synchronous Negotiation: Ultra, Wide Negotiation: Enabled
> scsi0:   Disconnect/Reconnect: Enabled, Tagged Queuing: Enabled
> scsi0:   Scatter/Gather Limit: 128 of 8192 segments, Mailboxes: 211
> scsi0:   Driver Queue Depth: 211, Host Adapter Queue Depth: 192
> scsi0:   Tagged Queue Depth:
> Automatic
> , Untagged Queue Depth: 3
> scsi0:   SCSI Bus Termination: Both Enabled
> , SCAM: Disabled
> 
> scsi0: *** BusLogic BT-958 Initialized Successfully ***
> scsi host0: BusLogic BT-958
> 
> back to order:
> 
> pci 0000:00:13.0: PCI->APIC IRQ transform: INT A -> IRQ 17
> scsi: ***** BusLogic SCSI Driver Version 2.1.17 of 12 September 2013 *****
> scsi: Copyright 1995-1998 by Leonard N. Zubkoff <lnz@dandelion.com>
> scsi0: Configuring BusLogic Model BT-958 PCI Wide Ultra SCSI Host Adapter
> scsi0:   Firmware Version: 5.07B, I/O Address: 0x7000, IRQ Channel: 17/Level
> scsi0:   PCI Bus: 0, Device: 19, Address: 0xE0012000, Host Adapter SCSI ID: 7
> scsi0:   Parity Checking: Enabled, Extended Translation: Enabled
> scsi0:   Synchronous Negotiation: Ultra, Wide Negotiation: Enabled
> scsi0:   Disconnect/Reconnect: Enabled, Tagged Queuing: Enabled
> scsi0:   Scatter/Gather Limit: 128 of 8192 segments, Mailboxes: 211
> scsi0:   Driver Queue Depth: 211, Host Adapter Queue Depth: 192
> scsi0:   Tagged Queue Depth: Automatic, Untagged Queue Depth: 3
> scsi0:   SCSI Bus Termination: Both Enabled, SCAM: Disabled
> scsi0: *** BusLogic BT-958 Initialized Successfully ***
> scsi host0: BusLogic BT-958
> 
> Also diagnostic output such as with the `BusLogic=TraceConfiguration' 
> parameter is affected and becomes vertical and therefore hard to read.  
> This has now been corrected, e.g.:
> 
> pci 0000:00:13.0: PCI->APIC IRQ transform: INT A -> IRQ 17
> blogic_cmd(86) Status = 30:  4 ==>  4: FF 05 93 00
> blogic_cmd(95) Status = 28: (Modify I/O Address)
> blogic_cmd(91) Status = 30:  1 ==>  1: 01
> blogic_cmd(04) Status = 30:  4 ==>  4: 41 41 35 30
> blogic_cmd(8D) Status = 30: 14 ==> 14: 45 DC 00 20 00 00 00 00 00 40 30 37 42 1D
> scsi: ***** BusLogic SCSI Driver Version 2.1.17 of 12 September 2013 *****
> scsi: Copyright 1995-1998 by Leonard N. Zubkoff <lnz@dandelion.com>
> blogic_cmd(04) Status = 30:  4 ==>  4: 41 41 35 30
> blogic_cmd(0B) Status = 30:  3 ==>  3: 00 08 07
> blogic_cmd(0D) Status = 30: 34 ==> 34: 03 01 07 04 00 00 00 00 00 00 00 00 00 00 00 00 FF 42 44 46 FF 00 00 00 00 00 00 00 00 00 FF 00 FF 00
> blogic_cmd(8D) Status = 30: 14 ==> 14: 45 DC 00 20 00 00 00 00 00 40 30 37 42 1D
> blogic_cmd(84) Status = 30:  1 ==>  1: 37
> blogic_cmd(8B) Status = 30:  5 ==>  5: 39 35 38 20 20
> blogic_cmd(85) Status = 30:  1 ==>  1: 42
> blogic_cmd(86) Status = 30:  4 ==>  4: FF 05 93 00
> blogic_cmd(91) Status = 30: 64 ==> 64: 41 46 3E 20 39 35 38 20 20 00 C4 00 04 01 07 2F 07 04 35 FF FF FF FF FF FF FF FF FF FF 01 00 FE FF 08 FF FF 00 00 00 00 00 00 00 01 00 01 00 00 FF FF 00 00 00 00 00 00 00 00 00 00 00 00 00 FC
> scsi0: Configuring BusLogic Model BT-958 PCI Wide Ultra SCSI Host Adapter
> 
> etc.
> 
> Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
> Fixes: 4bcc595ccd80 ("printk: reinstate KERN_CONT for printing continuation lines")
> Cc: stable@vger.kernel.org # v4.9+
> ---
> No changes from v1.
> ---
>  drivers/scsi/BusLogic.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> linux-buslogic-pr-cont.diff
> Index: linux-macro-ide/drivers/scsi/BusLogic.c
> ===================================================================
> --- linux-macro-ide.orig/drivers/scsi/BusLogic.c
> +++ linux-macro-ide/drivers/scsi/BusLogic.c
> @@ -3603,7 +3603,7 @@ static void blogic_msg(enum blogic_msgle
>  			if (buf[0] != '\n' || len > 1)
>  				printk("%sscsi%d: %s", blogic_msglevelmap[msglevel], adapter->host_no, buf);
>  		} else
> -			printk("%s", buf);
> +			pr_cont("%s", buf);
>  	} else {
>  		if (begin) {
>  			if (adapter != NULL && adapter->adapter_initd)
> @@ -3611,7 +3611,7 @@ static void blogic_msg(enum blogic_msgle
>  			else
>  				printk("%s%s", blogic_msglevelmap[msglevel], buf);
>  		} else
> -			printk("%s", buf);
> +			pr_cont("%s", buf);
>  	}
>  	begin = (buf[len - 1] == '\n');
>  }
> 

Acked-by: Khalid Aziz <khalid@gonehiking.org>

[PATCH v2 2/5] scsi: BusLogic: Avoid unbounded `vsprintf' use

[Maciej W. Rozycki]

Existing `blogic_msg' invocations do not appear to overrun its internal 
buffer of a fixed length of 100, which would cause stack corruption, but 
it's easy to miss with possible further updates and a fix is cheap in 
performance terms, so limit the output produced into the buffer by using 
`vscnprintf' rather than `vsprintf'.

Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
---
Changes from v1:

- use `vscnprintf' instead of `vsnprintf' for the correct character count.
---
 drivers/scsi/BusLogic.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

linux-buslogic-vscnprintf.diff
Index: linux-macro-ide/drivers/scsi/BusLogic.c
===================================================================
--- linux-macro-ide.orig/drivers/scsi/BusLogic.c
+++ linux-macro-ide/drivers/scsi/BusLogic.c
@@ -3588,7 +3588,7 @@ static void blogic_msg(enum blogic_msgle
 	int len = 0;
 
 	va_start(args, adapter);
-	len = vsprintf(buf, fmt, args);
+	len = vscnprintf(buf, sizeof(buf), fmt, args);
 	va_end(args);
 	if (msglevel == BLOGIC_ANNOUNCE_LEVEL) {
 		static int msglines = 0;

Re: [PATCH v2 2/5] scsi: BusLogic: Avoid unbounded `vsprintf' use

[Khalid Aziz]

On 4/20/21 12:01 PM, Maciej W. Rozycki wrote:
> Existing `blogic_msg' invocations do not appear to overrun its internal 
> buffer of a fixed length of 100, which would cause stack corruption, but 
> it's easy to miss with possible further updates and a fix is cheap in 
> performance terms, so limit the output produced into the buffer by using 
> `vscnprintf' rather than `vsprintf'.
> 
> Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
> ---
> Changes from v1:
> 
> - use `vscnprintf' instead of `vsnprintf' for the correct character count.
> ---
>  drivers/scsi/BusLogic.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> linux-buslogic-vscnprintf.diff
> Index: linux-macro-ide/drivers/scsi/BusLogic.c
> ===================================================================
> --- linux-macro-ide.orig/drivers/scsi/BusLogic.c
> +++ linux-macro-ide/drivers/scsi/BusLogic.c
> @@ -3588,7 +3588,7 @@ static void blogic_msg(enum blogic_msgle
>  	int len = 0;
>  
>  	va_start(args, adapter);
> -	len = vsprintf(buf, fmt, args);
> +	len = vscnprintf(buf, sizeof(buf), fmt, args);
>  	va_end(args);
>  	if (msglevel == BLOGIC_ANNOUNCE_LEVEL) {
>  		static int msglines = 0;
> 

Acked-by: Khalid Aziz <khalid@gonehiking.org>

[PATCH v2 3/5] scsi: Provide for avoiding trailing allocation length with VPD inquiries

[Maciej W. Rozycki]

Allow SCSI hosts to request avoiding trailing allocation length with VPD 
inquiries, and use the mechanism to work around an issue with at least 
some BusLogic MultiMaster host bus adapters and observed with the BT-958 
model specifically where issuing commands that return less data than 
provided for causes fatal failures:

scsi host0: BusLogic BT-958
scsi 0:0:0:0: Direct-Access     IBM      DDYS-T18350M     SA5A PQ: 0 ANSI: 3
scsi 0:0:1:0: Direct-Access     SEAGATE  ST336607LW       0006 PQ: 0 ANSI: 3
scsi 0:0:5:0: Direct-Access     IOMEGA   ZIP 100          E.08 PQ: 0 ANSI: 2
sd 0:0:1:0: [sdb] 71687372 512-byte logical blocks: (36.7 GB/34.2 GiB)
sd 0:0:0:0: [sda] 35843670 512-byte logical blocks: (18.4 GB/17.1 GiB)
sd 0:0:1:0: [sdb] Write Protect is off
sd 0:0:5:0: [sdc] Attached SCSI removable disk
sd 0:0:0:0: [sda] Write Protect is off
sd 0:0:1:0: [sdb] Write cache: enabled, read cache: enabled, supports DPO and FUA
sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA
scsi0: *** BusLogic BT-958 Initialized Successfully ***
scsi0: *** BusLogic BT-958 Initialized Successfully ***
scsi0: *** BusLogic BT-958 Initialized Successfully ***
scsi0: *** BusLogic BT-958 Initialized Successfully ***
sd 0:0:0:0: Device offlined - not ready after error recovery
sd 0:0:1:0: Device offlined - not ready after error recovery
sd 0:0:0:0: scsi_vpd_inquiry(0): buf[64] => -5
sd 0:0:1:0: scsi_vpd_inquiry(0): buf[64] => -5
sd 0:0:0:0: [sda] Attached SCSI disk
sd 0:0:1:0: [sdb] Attached SCSI disk
VFS: Cannot open root device "802" or unknown-block(8,2): error -6

(here and elsewhere reported with some instrumentation added so as to 
show the causing requests and with irrelevant messages filtered out).

As already observed back in 2003 and worked around in smartmontools at 
least some versions of BusLogic firmware such as 5.07B are unable to 
handle such commands, but it is possible to request enough data first 
for the length of the data response to be determined and then reissue 
the same command with the allocation length matching the response 
expected.

It is what this change does on a host-by-host basis, by providing a flag 
for individual HBA drivers to enable this workaround, currently set by 
the BusLogic driver, and then issuing these double calls as requested, 
which then produce results as expected:

sd 0:0:1:0: [sdb] Write cache: enabled, read cache: enabled, supports DPO and FUA
sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA
sd 0:0:1:0: scsi_vpd_inquiry(0): buf[64] => 13
sd 0:0:0:0: scsi_vpd_inquiry(0): buf[64] => 7
 sdb: sdb1 sdb2
sd 0:0:1:0: scsi_vpd_inquiry(0): buf[64] => 13
sd 0:0:1:0: [sdb] Attached SCSI disk
 sda: sda1 sda2 sda3 sda4 < sda5 sda6 sda7 sda8 sda9 sda10 >
sd 0:0:0:0: scsi_vpd_inquiry(0): buf[64] => 7
sd 0:0:0:0: [sda] Attached SCSI disk
EXT4-fs (sda2): mounting ext2 file system using the ext4 subsystem
EXT4-fs (sda2): mounted filesystem without journal. Opts: (null). Quota mode: disabled.
VFS: Mounted root (ext2 filesystem) readonly on device 8:2.

The minimum request size of 4 for the repeated call has been chosen to 
match one required for a successful return from `scsi_vpd_inquiry'.

Interestingly enough it has only started triggering with not so recent 
commit af73623f5f10 ("[SCSI] sd: Reduce buffer size for vpd request") 
that decreased the allocation length for the originating request from 
512 down to 64.  Previously the request was rejected outright by the 
respective targets as invalid and therefore did not trigger the issue 
with MultiMaster firmware as that would only happen for a command that 
succeeded but produced less data than provided for:

scsi0: CCB #36 Target 0: Result 2 Host Adapter Status 00 Target Status 02
scsi0: CDB    12 01 00 02 00 00
scsi0: Sense  70 00 05 00 00 00 00 18 00 00 00 00 24 00 00 C0 00 03 00 [...]
sd 0:0:0:0: scsi_vpd_inquiry(0): buf[512] => -5
scsi0: CCB #37 Target 1: Result 2 Host Adapter Status 00 Target Status 02
scsi0: CDB    12 01 00 02 00 00
scsi0: Sense  70 00 05 00 00 00 00 0A 00 00 00 00 24 00 01 C9 00 03 00 [...]
sd 0:0:1:0: scsi_vpd_inquiry(0): buf[512] => -5

(here with the buffer size set back to 512, the `BusLogic=TraceErrors' 
parameter and trailing sense data zeros trimmed for brevity).  Note the 
sense key of 0x5 returned denoting an illegal request even for page 0.

Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
Fixes: 881a256d84e6 ("[SCSI] Add VPD helper")
Cc: stable@vger.kernel.org # v2.6.30+
---
No changes from v1.
---
 drivers/scsi/BusLogic.c  |    1 +
 drivers/scsi/scsi.c      |   24 +++++++++++++++++++++---
 include/scsi/scsi_host.h |    3 +++
 3 files changed, 25 insertions(+), 3 deletions(-)

linux-buslogic-get-vpd-page-buffer.diff
Index: linux-macro-ide/drivers/scsi/BusLogic.c
===================================================================
--- linux-macro-ide.orig/drivers/scsi/BusLogic.c
+++ linux-macro-ide/drivers/scsi/BusLogic.c
@@ -2301,6 +2301,7 @@ static void __init blogic_inithoststruct
 	host->sg_tablesize = adapter->drvr_sglimit;
 	host->unchecked_isa_dma = adapter->need_bouncebuf;
 	host->cmd_per_lun = adapter->untag_qdepth;
+	host->no_trailing_allocation_length = true;
 }
 
 /*
Index: linux-macro-ide/drivers/scsi/scsi.c
===================================================================
--- linux-macro-ide.orig/drivers/scsi/scsi.c
+++ linux-macro-ide/drivers/scsi/scsi.c
@@ -346,8 +346,19 @@ int scsi_get_vpd_page(struct scsi_device
 	if (sdev->skip_vpd_pages)
 		goto fail;
 
-	/* Ask for all the pages supported by this device */
-	result = scsi_vpd_inquiry(sdev, buf, 0, buf_len);
+	/*
+	 * Ask for all the pages supported by this device.  Determine the
+	 * actual data length first if so required by the host, e.g.
+	 * BusLogic BT-958.
+	 */
+	if (sdev->host->no_trailing_allocation_length) {
+		result = scsi_vpd_inquiry(sdev, buf, 0, min(4, buf_len));
+		if (result < 4)
+			goto fail;
+	} else {
+		result = buf_len;
+	}
+	result = scsi_vpd_inquiry(sdev, buf, 0, min(result, buf_len));
 	if (result < 4)
 		goto fail;
 
@@ -366,7 +377,14 @@ int scsi_get_vpd_page(struct scsi_device
 	goto fail;
 
  found:
-	result = scsi_vpd_inquiry(sdev, buf, page, buf_len);
+	if (sdev->host->no_trailing_allocation_length) {
+		result = scsi_vpd_inquiry(sdev, buf, page, min(4, buf_len));
+		if (result < 4)
+			goto fail;
+	} else {
+		result = buf_len;
+	}
+	result = scsi_vpd_inquiry(sdev, buf, page, min(result, buf_len));
 	if (result < 0)
 		goto fail;
 
Index: linux-macro-ide/include/scsi/scsi_host.h
===================================================================
--- linux-macro-ide.orig/include/scsi/scsi_host.h
+++ linux-macro-ide/include/scsi/scsi_host.h
@@ -653,6 +653,9 @@ struct Scsi_Host {
 	/* The transport requires the LUN bits NOT to be stored in CDB[1] */
 	unsigned no_scsi2_lun_in_cdb:1;
 
+	/* Allocation length must not exceed actual data length. */
+	unsigned no_trailing_allocation_length:1;
+
 	/*
 	 * Optional work queue to be utilized by the transport
 	 */

[PATCH v2 4/5] scsi: Avoid using reserved length byte with VPD inquiries

[Maciej W. Rozycki]

As discussed in a previous workaround for a BusLogic BT-958 problem with 
VPD inquiries with an allocation length of 512 bytes as requested before 
commit af73623f5f10 ("[SCSI] sd: Reduce buffer size for vpd request") 
are rejected outright as invalid at least by some SCSI target devices as 
are any requests with a non-zero value in byte #3:

scsi host0: BusLogic BT-958
scsi 0:0:0:0: Direct-Access     IBM      DDYS-T18350M     SA5A PQ: 0 ANSI: 3
scsi 0:0:1:0: Direct-Access     SEAGATE  ST336607LW       0006 PQ: 0 ANSI: 3
scsi 0:0:5:0: Direct-Access     IOMEGA   ZIP 100          E.08 PQ: 0 ANSI: 2
[...]
scsi0: CCB #36 Target 0: Result 2 Host Adapter Status 00 Target Status 02
scsi0: CDB    12 01 00 01 06 00
scsi0: Sense  70 00 05 00 00 00 00 18 00 00 00 00 24 00 00 C0 00 03 00 [...]
sd 0:0:0:0: scsi_vpd_inquiry(0): buf[262] => -5
scsi0: CCB #37 Target 1: Result 2 Host Adapter Status 00 Target Status 02
scsi0: CDB    12 01 00 01 06 00
scsi0: Sense  70 00 05 00 00 00 00 0A 00 00 00 00 24 00 01 C8 00 03 00 [...]
sd 0:0:1:0: scsi_vpd_inquiry(0): buf[262] => -5

(here with the buffer size tweaked to 262 so as to verify if a bit in 
byte #3 of the INQUIRY command is ignored and the length of 6 assumed or 
tripped over, the `BusLogic=TraceErrors' parameter and trailing sense 
data zeros trimmed for brevity).  Note the sense key of 0x5 denoting an 
illegal request.

For the record with the buffer size of 6 requests for page 0 complete 
successfully and due to page truncation `scsi_get_vpd_page' proceeds 
with an attempt to get inexistent page 0x89:

sd 0:0:0:0: scsi_vpd_inquiry(0): buf[6] => 7
sd 0:0:1:0: scsi_vpd_inquiry(0): buf[6] => 13
sd 0:0:0:0: scsi_vpd_inquiry(137): buf[6] => -5
sd 0:0:1:0: scsi_vpd_inquiry(137): buf[6] => -5

Upon a further investigation it has turned out at least SCSI-2 considers 
byte #3 of the INQUIRY command[1] as well as byte #2 of vital product 
data pages[2] reserved and expects a value of zero there.  The response 
from SCSI-3 devices shown above indicates the same expectation.

Therefore it is unsafe to issue INQUIRY requests unconditionally with 
the allocation length beyond 255, as they may fail with an otherwise 
supported request or cause undefined behaviour with some hardware.

Now we actually never do that as all our callers of `scsi_get_vpd_page' 
either hardcode the buffer size to a value between 8 and 255 or 
calculate it from a structure size, of which the largest is:

struct c2_inquiry {
	u8                         peripheral_info;      /*     0     1 */
	u8                         page_code;            /*     1     1 */
	u8                         reserved1;            /*     2     1 */
	u8                         page_len;             /*     3     1 */
	u8                         page_id[4];           /*     4     4 */
	u8                         sw_version[3];        /*     8     3 */
	u8                         sw_date[3];           /*    11     3 */
	u8                         features_enabled;     /*    14     1 */
	u8                         max_lun_supported;    /*    15     1 */
	u8                         partitions[239];      /*    16   239 */

	/* size: 255, cachelines: 2, members: 10 */
	/* last cacheline: 127 bytes */
};

As from commit b3ae8780b429 ("[SCSI] Add EVPD page 0x83 and 0x80 to sysfs")
we now also have the SCSI_VPD_PG_LEN macro that reflects the limitation.

However for the sake of a possible future requirement to support VPD 
pages that do have a length exceeding 255 bytes and now that the danger 
of using the formerly reserved byte #3 of the INQUIRY command has been 
identified execute calls to `scsi_get_vpd_page' with a request size 
exceeding 255 bytes in two stages, by determining the actual length of 
data to be returned first and only then issuing the intended request for 
full data.

References:

[1] "Information technology - Small Computer System Interface - 2", 
    WORKING DRAFT, X3T9.2, Project 375D, Revision 10L, 7-SEP-93, Section 
    8.2.5 "INQUIRY command", pp.104-108

[2] same, Section 8.3.4 "Vital product data parameters", pp.154-159

Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
Fixes: 881a256d84e6 ("[SCSI] Add VPD helper")
---
Hi,

 NB the SCSI-2 working draft is the only normative reference I have access 
to, downloaded many years ago and not online anymore.  I have more recent 
vendor documents that do indicate that bytes #3 & #2 respectively are a 
part of the length field, but based on empirical evidence presented here 
it is unsafe to unconditionally assume that the bytes can be set to a 
non-zero value.  So I think it will be safest long-term if we handle it 
correctly right away now that the knowledge is fresh, as past experience 
with commit af73623f5f10 ("[SCSI] sd: Reduce buffer size for vpd request") 
indicates the circumstances are not always correctly understood.

 No backport requested as we have no current use that would request VPD 
data beyond 255 bytes.

  Maciej

No changes from v1.
---
 drivers/scsi/scsi.c |   14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

linux-scsi-vpd-inquiry-buffer.diff
Index: linux-macro-ide/drivers/scsi/scsi.c
===================================================================
--- linux-macro-ide.orig/drivers/scsi/scsi.c
+++ linux-macro-ide/drivers/scsi/scsi.c
@@ -348,10 +348,15 @@ int scsi_get_vpd_page(struct scsi_device
 
 	/*
 	 * Ask for all the pages supported by this device.  Determine the
-	 * actual data length first if so required by the host, e.g.
-	 * BusLogic BT-958.
+	 * actual data length first if the length requested is beyond 255
+	 * bytes as the high order length byte used to be reserved with
+	 * older SCSI standard revisions and a non-zero value there may
+	 * cause either such an INQUIRY command to be rejected by a target
+	 * or undefined behaviour to occur.  Also do so if so required by
+	 * the host, e.g. BusLogic BT-958.
 	 */
-	if (sdev->host->no_trailing_allocation_length) {
+	if (buf_len > SCSI_VPD_PG_LEN ||
+	    sdev->host->no_trailing_allocation_length) {
 		result = scsi_vpd_inquiry(sdev, buf, 0, min(4, buf_len));
 		if (result < 4)
 			goto fail;
@@ -377,7 +382,8 @@ int scsi_get_vpd_page(struct scsi_device
 	goto fail;
 
  found:
-	if (sdev->host->no_trailing_allocation_length) {
+	if (buf_len > SCSI_VPD_PG_LEN ||
+	    sdev->host->no_trailing_allocation_length) {
 		result = scsi_vpd_inquiry(sdev, buf, page, min(4, buf_len));
 		if (result < 4)
 			goto fail;

[PATCH v2 5/5] scsi: Set allocation length to 255 for ATA Information VPD page

[Maciej W. Rozycki]

Set the allocation length to 255 for the ATA Information VPD page 
requested in the WRITE SAME handler, so as not to limit information 
examined by `scsi_get_vpd_page' in the supported vital product data 
pages unnecessarily.

Originally it was thought that Areca hardware may have issues with a 
valid allocation length supplied for a VPD inquiry, however older SCSI 
standard revisions[1] consider 255 the maximum length allowed and what 
has later become the high order byte is considered reserved and must be 
zero with the INQUIRY command.  Therefore it was unnecessary to reduce 
the amount of data requested from 512 as far down as to 64, arbitrarily 
chosen, and 255 would as well do.

With commit b3ae8780b429 ("[SCSI] Add EVPD page 0x83 and 0x80 to sysfs") 
we have since got the SCSI_VPD_PG_LEN macro, so use that instead.

References:

[1] "Information technology - Small Computer System Interface - 2",
    WORKING DRAFT, X3T9.2, Project 375D, Revision 10L, 7-SEP-93, Section
    8.2.5 "INQUIRY command", pp.104-108

Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
Fixes: af73623f5f10 ("[SCSI] sd: Reduce buffer size for vpd request")
---
No changes from v1.
---
 drivers/scsi/sd.c |    5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

Index: linux-macro-ide/drivers/scsi/sd.c
===================================================================
--- linux-macro-ide.orig/drivers/scsi/sd.c
+++ linux-macro-ide/drivers/scsi/sd.c
@@ -3076,16 +3076,13 @@ static void sd_read_write_same(struct sc
 	}
 
 	if (scsi_report_opcode(sdev, buffer, SD_BUF_SIZE, INQUIRY) < 0) {
-		/* too large values might cause issues with arcmsr */
-		int vpd_buf_len = 64;
-
 		sdev->no_report_opcodes = 1;
 
 		/* Disable WRITE SAME if REPORT SUPPORTED OPERATION
 		 * CODES is unsupported and the device has an ATA
 		 * Information VPD page (SAT).
 		 */
-		if (!scsi_get_vpd_page(sdev, 0x89, buffer, vpd_buf_len))
+		if (!scsi_get_vpd_page(sdev, 0x89, buffer, SCSI_VPD_PG_LEN))
 			sdev->no_write_same = 1;
 	}
 

[PING][PATCH v2 0/5] Bring the BusLogic host bus adapter driver up to Y2021

[Maciej W. Rozycki]

On Tue, 20 Apr 2021, Maciej W. Rozycki wrote:

>  Questions, comments, concerns?  Otherwise please apply.

 Ping for: 

<https://patchwork.kernel.org/project/linux-scsi/list/?series=470455&archive=both>.

 Where are we with this patch series?  I can see it's been archived in 
patchwork in the new state.  With the unexpected serial device fixes which 
preempted me and which I've just posted, moving them off the table I now 
have some spare cycles to get back here, but I'm not sure what to do.

 Nix was kind enough to verify and tell me off-list that 5/5 still works 
correctly with his system that required the earlier change referred there 
and the need for which was not completely understood back then -- Nix, are 
you OK with adding a `Tested-by' tag for your verification?

 Otherwise is there anything I need to do to move forward with these 
changes?  Shall I just repost the series as it was given that it still 
applies to Linus master verbatim?

  Maciej

Re: [PING][PATCH v2 0/5] Bring the BusLogic host bus adapter driver up to Y2021

[Martin K. Petersen]

Maciej,

> Where are we with this patch series?  I can see it's been archived in
> patchwork in the new state.  With the unexpected serial device fixes
> which preempted me and which I've just posted, moving them off the
> table I now have some spare cycles to get back here, but I'm not sure
> what to do.

Some of the patches were clashing with my device discovery changes.
Your series is still in my inbox, have just been swamped with testing
and integrating several fundamental core modifications the last few
weeks.

I'll get to it before the merge window...

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PING][PATCH v2 0/5] Bring the BusLogic host bus adapter driver up to Y2021

[Maciej W. Rozycki]

Martin,

> > Where are we with this patch series?  I can see it's been archived in
> > patchwork in the new state.  With the unexpected serial device fixes
> > which preempted me and which I've just posted, moving them off the
> > table I now have some spare cycles to get back here, but I'm not sure
> > what to do.
> 
> Some of the patches were clashing with my device discovery changes.
> Your series is still in my inbox, have just been swamped with testing
> and integrating several fundamental core modifications the last few
> weeks.
> 
> I'll get to it before the merge window...

 Sounds good, thanks!  I got distracted again, in particular by this nice 
HiFive Unmatched board, but please do let me know if I can assist you with 
these changes somehow.  E.g. shall I resolve the clashes (which branch?)?
I'm away from my lab for the time being, but I can do all the usual stuff 
remotely.

  Maciej

Re: [PING][PATCH v2 0/5] Bring the BusLogic host bus adapter driver up to Y2021

[Martin K. Petersen]

Maciej,

> Sounds good, thanks!  I got distracted again, in particular by this
> nice HiFive Unmatched board, but please do let me know if I can assist
> you with these changes somehow.  E.g. shall I resolve the clashes
> (which branch?)?  I'm away from my lab for the time being, but I can
> do all the usual stuff remotely.

I'll post my revamp of the VPD/discard/zeroout stuff soon. I will
include your Buslogic-specific patches when I do.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PING][PATCH v2 0/5] Bring the BusLogic host bus adapter driver up to Y2021

[Nix]

On 11 Jun 2021, Maciej W. Rozycki said:

> On Tue, 20 Apr 2021, Maciej W. Rozycki wrote:
>
>>  Questions, comments, concerns?  Otherwise please apply.
>
>  Ping for: 
>
> <https://patchwork.kernel.org/project/linux-scsi/list/?series=470455&archive=both>.
>
>  Where are we with this patch series?  I can see it's been archived in 
> patchwork in the new state.  With the unexpected serial device fixes which 
> preempted me and which I've just posted, moving them off the table I now 
> have some spare cycles to get back here, but I'm not sure what to do.
>
>  Nix was kind enough to verify and tell me off-list that 5/5 still works 
> correctly with his system that required the earlier change referred there 
> and the need for which was not completely understood back then -- Nix, are 
> you OK with adding a `Tested-by' tag for your verification?

Sure, as long as I don't have to verify it again, because the loft is a
superheated pollen hell right now and I am not going up there unless it
actually catches fire or something :)

If that needs an actual name, like S-o-b, you can use

Tested-by: Nick Alcock <nick.alcock@oracle.com>