Kernel crash with unsupported DIF protection type

Kernel crash with unsupported DIF protection type

[Hannes Reinecke]

[-- Attachment #1: Type: text/plain, Size: 2571 bytes --]

Hi all,

I recently got my hands on some weird drives, insisting on having
been formatted with protection type 7:

# sg_readcap --16 /dev/sdb
Read Capacity results:
   Protection: prot_en=1, p_type=6, p_i_exponent=0
   Logical block provisioning: lbpme=0, lbprz=0
   Last logical block address=1758174767 (0x68cb9e2f), Number of
logical blocks=1758174768
   Logical block length=512 bytes
   Logical blocks per physical block exponent=0
   Lowest aligned logical block address=0
Hence:
   Device size: 900185481216 bytes, 858483.8 MiB, 900.19 GB

(I know. I've already complained.)
However, this drive causes a horrible crash:

sd 0:0:1:0: [sdb] formatted with unsupported protection type 7.
Disabling disk!
sd 0:0:1:0: [sdb] 1758174768 512-byte logical blocks: (900
GB/838 GiB)
sd 0:0:1:0: [sdb]  Result: hostbyte=DID_OK
driverbyte=DRIVER_SENSE
sd 0:0:1:0: [sdb]  Sense Key : Medium Error [current]
sd 0:0:1:0: [sdb]  Add. Sense: Medium format corrupted
sd 0:0:1:0: [sdb] CDB: Read(10): 28 00 00 00 00 00 00 00 08 00
end_request: I/O error, dev sdb, sector 0
Buffer I/O error on device sdb, logical block 0

[ tons and tons of I/O errors ]

[   15.551689] sd 0:0:1:0: [sdb] Enabling DIX T10-DIF-TYPE1-CRC
protection
[   15.561353] ------------[ cut here ]------------
[   15.569416] kernel BUG at
/usr/src/linux-3.0/drivers/scsi/scsi_lib.c:1069!

There are several odd things happening here:
- It says 'Disabling disk', which _should_ have set the
  capacity to '0':

drivers/scsi/sd.c:sd_read_protection_type()
	if (type > SD_DIF_TYPE3_PROTECTION) {
		sd_printk(KERN_ERR, sdkp, "formatted with unsupported "	\
			  "protection type %u. Disabling disk!\n", type);
		sdkp->capacity = 0;
		return;
	}

- it enables type 1 protection, which it evidently is not.

I've attached a tentative patch, which allows the system to boot.
However, I'm not completely happy with that, as the capacity is
_still_ updated after revalidation:

sd 0:0:1:0: [sdb] formatted with unsupported protection type 7.
Disabling disk!
sd 0:0:1:0: [sdb] 0 512-byte logical blocks: (0 B/0 B)
sd 0:0:1:0: [sdb] Write Protect is off
sd 0:0:1:0: [sdb] Mode Sense: cf 00 10 08
sd 0:0:1:0: [sdb] Write cache: disabled, read cache: enabled,
supports DPO and FUA
sd 0:0:1:0: [sdb] 1758174768 512-byte logical blocks: (900 GB/838 GiB)
sd 0:0:1:0: [sdb] Attached SCSI disk

Thoughts?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)


[-- Attachment #2: sd-disable-disk-with-incorrect-protection-information.patch --]
[-- Type: text/x-patch, Size: 3126 bytes --]

>From 88f73f9e3c9e9aaa6af73885db9c6230edab6fe0 Mon Sep 17 00:00:00 2001
From: Hannes Reinecke <hare@suse.de>
Date: Tue, 18 Sep 2012 10:37:52 +0200
Subject: [PATCH] sd: disable disk with incorrect protection information

SPC-3 only defines DIF protection up to type 3.
Other types are invalid and the drive cannot be used.
So make sure to disable the drive correctly by setting
the capacity to 0.

Signed-off-by: Hannes Reinecke <hare@suse.de>

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 4df73e5..6a0b216 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1671,26 +1671,25 @@ sd_spinup_disk(struct scsi_disk *sdkp)
 /*
  * Determine whether disk supports Data Integrity Field.
  */
-static void sd_read_protection_type(struct scsi_disk *sdkp, unsigned char *buffer)
+static int sd_read_protection_type(struct scsi_disk *sdkp, unsigned char *buffer)
 {
 	struct scsi_device *sdp = sdkp->device;
 	u8 type;
 
 	if (scsi_device_protection(sdp) == 0 || (buffer[12] & 1) == 0)
-		return;
+		return 0;
 
 	type = ((buffer[12] >> 1) & 7) + 1; /* P_TYPE 0 = Type 1 */
 
 	if (type == sdkp->protection_type || !sdkp->first_scan)
-		return;
+		return 0;
 
 	sdkp->protection_type = type;
 
 	if (type > SD_DIF_TYPE3_PROTECTION) {
 		sd_printk(KERN_ERR, sdkp, "formatted with unsupported "	\
 			  "protection type %u. Disabling disk!\n", type);
-		sdkp->capacity = 0;
-		return;
+		return -EINVAL;
 	}
 
 	if (scsi_host_dif_capable(sdp->host, type))
@@ -1699,6 +1698,7 @@ static void sd_read_protection_type(struct scsi_disk *sdkp, unsigned char *buffe
 	else
 		sd_printk(KERN_NOTICE, sdkp,
 			  "Disabling DIF Type %u protection\n", type);
+	return 0;
 }
 
 static void read_capacity_error(struct scsi_disk *sdkp, struct scsi_device *sdp,
@@ -1794,8 +1794,6 @@ static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
 	sector_size = get_unaligned_be32(&buffer[8]);
 	lba = get_unaligned_be64(&buffer[0]);
 
-	sd_read_protection_type(sdkp, buffer);
-
 	if ((sizeof(sdkp->capacity) == 4) && (lba >= 0xffffffffULL)) {
 		sd_printk(KERN_ERR, sdkp, "Too big for this kernel. Use a "
 			"kernel compiled with support for large block "
@@ -1804,6 +1802,12 @@ static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
 		return -EOVERFLOW;
 	}
 
+	if (sd_read_protection_type(sdkp, buffer))
+		/* Invalid or unhandled protection type */
+		sdkp->capacity = 0;
+	else
+		sdkp->capacity = lba + 1;
+
 	/* Logical blocks per physical block exponent */
 	sdkp->physical_block_size = (1 << (buffer[13] & 0xf)) * sector_size;
 
@@ -1823,7 +1827,6 @@ static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
 		sd_config_discard(sdkp, SD_LBP_WS16);
 	}
 
-	sdkp->capacity = lba + 1;
 	return sector_size;
 }
 
diff --git a/drivers/scsi/sd_dif.c b/drivers/scsi/sd_dif.c
index e52d5bc..1b15b51 100644
--- a/drivers/scsi/sd_dif.c
+++ b/drivers/scsi/sd_dif.c
@@ -320,7 +320,7 @@ void sd_dif_config_host(struct scsi_disk *sdkp)
 		dif = 0; dix = 1;
 	}
 
-	if (!dix)
+	if (!dix || type > SD_DIF_TYPE3_PROTECTION)
 		return;
 
 	/* Enable DMA of protection information */

Re: Kernel crash with unsupported DIF protection type

[Douglas Gilbert]

On 12-09-18 11:04 AM, Hannes Reinecke wrote:
> Hi all,
>
> I recently got my hands on some weird drives, insisting on having
> been formatted with protection type 7:
>
> # sg_readcap --16 /dev/sdb
> Read Capacity results:
>     Protection: prot_en=1, p_type=6, p_i_exponent=0
>     Logical block provisioning: lbpme=0, lbprz=0
>     Last logical block address=1758174767 (0x68cb9e2f), Number of
> logical blocks=1758174768
>     Logical block length=512 bytes
>     Logical blocks per physical block exponent=0
>     Lowest aligned logical block address=0
> Hence:
>     Device size: 900185481216 bytes, 858483.8 MiB, 900.19 GB
>
> (I know. I've already complained.)
> However, this drive causes a horrible crash:
>
> sd 0:0:1:0: [sdb] formatted with unsupported protection type 7.
> Disabling disk!
> sd 0:0:1:0: [sdb] 1758174768 512-byte logical blocks: (900
> GB/838 GiB)
> sd 0:0:1:0: [sdb]  Result: hostbyte=DID_OK
> driverbyte=DRIVER_SENSE
> sd 0:0:1:0: [sdb]  Sense Key : Medium Error [current]
> sd 0:0:1:0: [sdb]  Add. Sense: Medium format corrupted
> sd 0:0:1:0: [sdb] CDB: Read(10): 28 00 00 00 00 00 00 00 08 00
> end_request: I/O error, dev sdb, sector 0
> Buffer I/O error on device sdb, logical block 0
>
> [ tons and tons of I/O errors ]
>
> [   15.551689] sd 0:0:1:0: [sdb] Enabling DIX T10-DIF-TYPE1-CRC
> protection
> [   15.561353] ------------[ cut here ]------------
> [   15.569416] kernel BUG at
> /usr/src/linux-3.0/drivers/scsi/scsi_lib.c:1069!
>
> There are several odd things happening here:
> - It says 'Disabling disk', which _should_ have set the
>    capacity to '0':
>
> drivers/scsi/sd.c:sd_read_protection_type()
> 	if (type > SD_DIF_TYPE3_PROTECTION) {
> 		sd_printk(KERN_ERR, sdkp, "formatted with unsupported "	\
> 			  "protection type %u. Disabling disk!\n", type);
> 		sdkp->capacity = 0;
> 		return;
> 	}
>
> - it enables type 1 protection, which it evidently is not.
>
> I've attached a tentative patch, which allows the system to boot.
> However, I'm not completely happy with that, as the capacity is
> _still_ updated after revalidation:
>
> sd 0:0:1:0: [sdb] formatted with unsupported protection type 7.
> Disabling disk!
> sd 0:0:1:0: [sdb] 0 512-byte logical blocks: (0 B/0 B)
> sd 0:0:1:0: [sdb] Write Protect is off
> sd 0:0:1:0: [sdb] Mode Sense: cf 00 10 08
> sd 0:0:1:0: [sdb] Write cache: disabled, read cache: enabled,
> supports DPO and FUA
> sd 0:0:1:0: [sdb] 1758174768 512-byte logical blocks: (900 GB/838 GiB)
> sd 0:0:1:0: [sdb] Attached SCSI disk
>
> Thoughts?

The "Medium format corrupted" additional sense qualifier occurs
(with Seagate disks) when a FORMAT UNIT command is interrupted.
So maybe, for good measure, that disk also sets the DIF
protection type to an unsupported value (i.e. 7). So re-doing a
FORMAT  UNIT may clear that state.

Obviously the kernel should not crash when faced with such
a disk.

Doug Gilbert

Re: Kernel crash with unsupported DIF protection type

[Hannes Reinecke]

On 09/18/2012 11:30 AM, Douglas Gilbert wrote:
> On 12-09-18 11:04 AM, Hannes Reinecke wrote:
>> Hi all,
>>
>> I recently got my hands on some weird drives, insisting on having
>> been formatted with protection type 7:
>>
>> # sg_readcap --16 /dev/sdb
>> Read Capacity results:
>>     Protection: prot_en=1, p_type=6, p_i_exponent=0
>>     Logical block provisioning: lbpme=0, lbprz=0
>>     Last logical block address=1758174767 (0x68cb9e2f), Number of
>> logical blocks=1758174768
>>     Logical block length=512 bytes
>>     Logical blocks per physical block exponent=0
>>     Lowest aligned logical block address=0
>> Hence:
>>     Device size: 900185481216 bytes, 858483.8 MiB, 900.19 GB
>>
>> (I know. I've already complained.)
>> However, this drive causes a horrible crash:
>>
>> sd 0:0:1:0: [sdb] formatted with unsupported protection type 7.
>> Disabling disk!
>> sd 0:0:1:0: [sdb] 1758174768 512-byte logical blocks: (900
>> GB/838 GiB)
>> sd 0:0:1:0: [sdb]  Result: hostbyte=DID_OK
>> driverbyte=DRIVER_SENSE
>> sd 0:0:1:0: [sdb]  Sense Key : Medium Error [current]
>> sd 0:0:1:0: [sdb]  Add. Sense: Medium format corrupted
>> sd 0:0:1:0: [sdb] CDB: Read(10): 28 00 00 00 00 00 00 00 08 00
>> end_request: I/O error, dev sdb, sector 0
>> Buffer I/O error on device sdb, logical block 0
>>
>> [ tons and tons of I/O errors ]
>>
>> [   15.551689] sd 0:0:1:0: [sdb] Enabling DIX T10-DIF-TYPE1-CRC
>> protection
>> [   15.561353] ------------[ cut here ]------------
>> [   15.569416] kernel BUG at
>> /usr/src/linux-3.0/drivers/scsi/scsi_lib.c:1069!
>>
>> There are several odd things happening here:
>> - It says 'Disabling disk', which _should_ have set the
>>    capacity to '0':
>>
>> drivers/scsi/sd.c:sd_read_protection_type()
>>     if (type > SD_DIF_TYPE3_PROTECTION) {
>>         sd_printk(KERN_ERR, sdkp, "formatted with unsupported "    \
>>               "protection type %u. Disabling disk!\n", type);
>>         sdkp->capacity = 0;
>>         return;
>>     }
>>
>> - it enables type 1 protection, which it evidently is not.
>>
>> I've attached a tentative patch, which allows the system to boot.
>> However, I'm not completely happy with that, as the capacity is
>> _still_ updated after revalidation:
>>
>> sd 0:0:1:0: [sdb] formatted with unsupported protection type 7.
>> Disabling disk!
>> sd 0:0:1:0: [sdb] 0 512-byte logical blocks: (0 B/0 B)
>> sd 0:0:1:0: [sdb] Write Protect is off
>> sd 0:0:1:0: [sdb] Mode Sense: cf 00 10 08
>> sd 0:0:1:0: [sdb] Write cache: disabled, read cache: enabled,
>> supports DPO and FUA
>> sd 0:0:1:0: [sdb] 1758174768 512-byte logical blocks: (900 GB/838
>> GiB)
>> sd 0:0:1:0: [sdb] Attached SCSI disk
>>
>> Thoughts?
> 
> The "Medium format corrupted" additional sense qualifier occurs
> (with Seagate disks) when a FORMAT UNIT command is interrupted.
> So maybe, for good measure, that disk also sets the DIF
> protection type to an unsupported value (i.e. 7). So re-doing a
> FORMAT  UNIT may clear that state.
> 
Yeah, that's what I'm doing now.

Incidentally:

sg_format manpage says:

       Format with type 1 protection:

          sg_format --format --fmtpinfo=3 --pfu /dev/sdm

but the '--pfu' option requires an argument.
According to sbc3r23 the command should read:

sg_format --format --fmtpinfo=3 --pfu=1 /dev/sdm

Correct?

> Obviously the kernel should not crash when faced with such
> a disk.
> 
Precisely.
Especially the 'Disabling disk' no-op is worrying.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)


--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Kernel crash with unsupported DIF protection type

[Douglas Gilbert]

On 12-09-18 11:35 AM, Hannes Reinecke wrote:
> On 09/18/2012 11:30 AM, Douglas Gilbert wrote:
>> On 12-09-18 11:04 AM, Hannes Reinecke wrote:
>>> Hi all,
>>>
>>> I recently got my hands on some weird drives, insisting on having
>>> been formatted with protection type 7:
>>>
>>> # sg_readcap --16 /dev/sdb
>>> Read Capacity results:
>>>      Protection: prot_en=1, p_type=6, p_i_exponent=0
>>>      Logical block provisioning: lbpme=0, lbprz=0
>>>      Last logical block address=1758174767 (0x68cb9e2f), Number of
>>> logical blocks=1758174768
>>>      Logical block length=512 bytes
>>>      Logical blocks per physical block exponent=0
>>>      Lowest aligned logical block address=0
>>> Hence:
>>>      Device size: 900185481216 bytes, 858483.8 MiB, 900.19 GB
>>>
>>> (I know. I've already complained.)
>>> However, this drive causes a horrible crash:
>>>
>>> sd 0:0:1:0: [sdb] formatted with unsupported protection type 7.
>>> Disabling disk!
>>> sd 0:0:1:0: [sdb] 1758174768 512-byte logical blocks: (900
>>> GB/838 GiB)
>>> sd 0:0:1:0: [sdb]  Result: hostbyte=DID_OK
>>> driverbyte=DRIVER_SENSE
>>> sd 0:0:1:0: [sdb]  Sense Key : Medium Error [current]
>>> sd 0:0:1:0: [sdb]  Add. Sense: Medium format corrupted
>>> sd 0:0:1:0: [sdb] CDB: Read(10): 28 00 00 00 00 00 00 00 08 00
>>> end_request: I/O error, dev sdb, sector 0
>>> Buffer I/O error on device sdb, logical block 0
>>>
>>> [ tons and tons of I/O errors ]
>>>
>>> [   15.551689] sd 0:0:1:0: [sdb] Enabling DIX T10-DIF-TYPE1-CRC
>>> protection
>>> [   15.561353] ------------[ cut here ]------------
>>> [   15.569416] kernel BUG at
>>> /usr/src/linux-3.0/drivers/scsi/scsi_lib.c:1069!
>>>
>>> There are several odd things happening here:
>>> - It says 'Disabling disk', which _should_ have set the
>>>     capacity to '0':
>>>
>>> drivers/scsi/sd.c:sd_read_protection_type()
>>>      if (type > SD_DIF_TYPE3_PROTECTION) {
>>>          sd_printk(KERN_ERR, sdkp, "formatted with unsupported "    \
>>>                "protection type %u. Disabling disk!\n", type);
>>>          sdkp->capacity = 0;
>>>          return;
>>>      }
>>>
>>> - it enables type 1 protection, which it evidently is not.
>>>
>>> I've attached a tentative patch, which allows the system to boot.
>>> However, I'm not completely happy with that, as the capacity is
>>> _still_ updated after revalidation:
>>>
>>> sd 0:0:1:0: [sdb] formatted with unsupported protection type 7.
>>> Disabling disk!
>>> sd 0:0:1:0: [sdb] 0 512-byte logical blocks: (0 B/0 B)
>>> sd 0:0:1:0: [sdb] Write Protect is off
>>> sd 0:0:1:0: [sdb] Mode Sense: cf 00 10 08
>>> sd 0:0:1:0: [sdb] Write cache: disabled, read cache: enabled,
>>> supports DPO and FUA
>>> sd 0:0:1:0: [sdb] 1758174768 512-byte logical blocks: (900 GB/838
>>> GiB)
>>> sd 0:0:1:0: [sdb] Attached SCSI disk
>>>
>>> Thoughts?
>>
>> The "Medium format corrupted" additional sense qualifier occurs
>> (with Seagate disks) when a FORMAT UNIT command is interrupted.
>> So maybe, for good measure, that disk also sets the DIF
>> protection type to an unsupported value (i.e. 7). So re-doing a
>> FORMAT  UNIT may clear that state.
>>
> Yeah, that's what I'm doing now.
>
> Incidentally:
>
> sg_format manpage says:
>
>         Format with type 1 protection:
>
>            sg_format --format --fmtpinfo=3 --pfu /dev/sdm
>
> but the '--pfu' option requires an argument.
> According to sbc3r23 the command should read:
>
> sg_format --format --fmtpinfo=3 --pfu=1 /dev/sdm
>
> Correct?

Yes. And it is corrected in the 1.34 betas that I have
sent you :-)


I placed a copy of my latest sg3_utils-1.34 beta
in the News section at the top of this page:
   http://sg.danny.cz/sg

Doug Gilbert

Re: Kernel crash with unsupported DIF protection type

[Martin K. Petersen]

>>>>> "Hannes" == Hannes Reinecke <hare@suse.de> writes:

Hannes> I recently got my hands on some weird drives, insisting on
Hannes> having been formatted with protection type 7:

Lovely :|


Hannes> I've attached a tentative patch, which allows the system to
Hannes> boot.  However, I'm not completely happy with that, as the
Hannes> capacity is _still_ updated after revalidation:

How about this?


sd: Ensure we correctly disable devices with unknown protection type

We set the capacity to zero when we discovered a device formatted with
an unknown DIF protection type. However, the read_capacity code would
override the capacity and cause the device to be enabled regardless.

Make sd_read_protection_type() return an error if the protection type is
unknown. Also prevent duplicate printk lines when the device is being
revalidated.

Reported-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 1cf2d5d..1c54564 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1820,34 +1820,42 @@ sd_spinup_disk(struct scsi_disk *sdkp)
 /*
  * Determine whether disk supports Data Integrity Field.
  */
-static void sd_read_protection_type(struct scsi_disk *sdkp, unsigned char *buffer)
+static int sd_read_protection_type(struct scsi_disk *sdkp, unsigned char *buffer)
 {
 	struct scsi_device *sdp = sdkp->device;
 	u8 type;
+	int ret = 0;
 
 	if (scsi_device_protection(sdp) == 0 || (buffer[12] & 1) == 0)
-		return;
+		return ret;
 
 	type = ((buffer[12] >> 1) & 7) + 1; /* P_TYPE 0 = Type 1 */
 
-	if (type == sdkp->protection_type || !sdkp->first_scan)
-		return;
+	if (type > SD_DIF_TYPE3_PROTECTION)
+		ret = -ENODEV;
+	else if (scsi_host_dif_capable(sdp->host, type))
+		ret = 1;
+
+	if (sdkp->first_scan || type != sdkp->protection_type)
+		switch (ret) {
+		case -ENODEV:
+			sd_printk(KERN_ERR, sdkp, "formatted with unsupported" \
+				  " protection type %u. Disabling disk!\n",
+				  type);
+			break;
+		case 1:
+			sd_printk(KERN_NOTICE, sdkp,
+				  "Enabling DIF Type %u protection\n", type);
+			break;
+		case 0:
+			sd_printk(KERN_NOTICE, sdkp,
+				  "Disabling DIF Type %u protection\n", type);
+			break;
+		}
 
 	sdkp->protection_type = type;
 
-	if (type > SD_DIF_TYPE3_PROTECTION) {
-		sd_printk(KERN_ERR, sdkp, "formatted with unsupported "	\
-			  "protection type %u. Disabling disk!\n", type);
-		sdkp->capacity = 0;
-		return;
-	}
-
-	if (scsi_host_dif_capable(sdp->host, type))
-		sd_printk(KERN_NOTICE, sdkp,
-			  "Enabling DIF Type %u protection\n", type);
-	else
-		sd_printk(KERN_NOTICE, sdkp,
-			  "Disabling DIF Type %u protection\n", type);
+	return ret;
 }
 
 static void read_capacity_error(struct scsi_disk *sdkp, struct scsi_device *sdp,
@@ -1943,7 +1951,10 @@ static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
 	sector_size = get_unaligned_be32(&buffer[8]);
 	lba = get_unaligned_be64(&buffer[0]);
 
-	sd_read_protection_type(sdkp, buffer);
+	if (sd_read_protection_type(sdkp, buffer) < 0) {
+		sdkp->capacity = 0;
+		return -ENODEV;
+	}
 
 	if ((sizeof(sdkp->capacity) == 4) && (lba >= 0xffffffffULL)) {
 		sd_printk(KERN_ERR, sdkp, "Too big for this kernel. Use a "

Re: Kernel crash with unsupported DIF protection type

[Hannes Reinecke]

On 09/20/2012 09:53 PM, Martin K. Petersen wrote:
>>>>>> "Hannes" == Hannes Reinecke <hare@suse.de> writes:
> 
> Hannes> I recently got my hands on some weird drives, insisting on
> Hannes> having been formatted with protection type 7:
> 
> Lovely :|
> 
Hehe. Probably _really_ future drives :-)

> 
> Hannes> I've attached a tentative patch, which allows the system to
> Hannes> boot.  However, I'm not completely happy with that, as the
> Hannes> capacity is _still_ updated after revalidation:
> 
> How about this?
> 
> 
> sd: Ensure we correctly disable devices with unknown protection type
> 
> We set the capacity to zero when we discovered a device formatted with
> an unknown DIF protection type. However, the read_capacity code would
> override the capacity and cause the device to be enabled regardless.
> 
> Make sd_read_protection_type() return an error if the protection type is
> unknown. Also prevent duplicate printk lines when the device is being
> revalidated.
> 
> Reported-by: Hannes Reinecke <hare@suse.de>
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 1cf2d5d..1c54564 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -1820,34 +1820,42 @@ sd_spinup_disk(struct scsi_disk *sdkp)
>  /*
>   * Determine whether disk supports Data Integrity Field.
>   */
> -static void sd_read_protection_type(struct scsi_disk *sdkp, unsigned char *buffer)
> +static int sd_read_protection_type(struct scsi_disk *sdkp, unsigned char *buffer)
>  {
>  	struct scsi_device *sdp = sdkp->device;
>  	u8 type;
> +	int ret = 0;
>  
>  	if (scsi_device_protection(sdp) == 0 || (buffer[12] & 1) == 0)
> -		return;
> +		return ret;
>  
>  	type = ((buffer[12] >> 1) & 7) + 1; /* P_TYPE 0 = Type 1 */
>  
> -	if (type == sdkp->protection_type || !sdkp->first_scan)
> -		return;
> +	if (type > SD_DIF_TYPE3_PROTECTION)
> +		ret = -ENODEV;
> +	else if (scsi_host_dif_capable(sdp->host, type))
> +		ret = 1;
> +
> +	if (sdkp->first_scan || type != sdkp->protection_type)
> +		switch (ret) {
> +		case -ENODEV:
> +			sd_printk(KERN_ERR, sdkp, "formatted with unsupported" \
> +				  " protection type %u. Disabling disk!\n",
> +				  type);
> +			break;
> +		case 1:
> +			sd_printk(KERN_NOTICE, sdkp,
> +				  "Enabling DIF Type %u protection\n", type);
> +			break;
> +		case 0:
> +			sd_printk(KERN_NOTICE, sdkp,
> +				  "Disabling DIF Type %u protection\n", type);
> +			break;
> +		}
>  
>  	sdkp->protection_type = type;
>  
> -	if (type > SD_DIF_TYPE3_PROTECTION) {
> -		sd_printk(KERN_ERR, sdkp, "formatted with unsupported "	\
> -			  "protection type %u. Disabling disk!\n", type);
> -		sdkp->capacity = 0;
> -		return;
> -	}
> -
> -	if (scsi_host_dif_capable(sdp->host, type))
> -		sd_printk(KERN_NOTICE, sdkp,
> -			  "Enabling DIF Type %u protection\n", type);
> -	else
> -		sd_printk(KERN_NOTICE, sdkp,
> -			  "Disabling DIF Type %u protection\n", type);
> +	return ret;
>  }
>  
>  static void read_capacity_error(struct scsi_disk *sdkp, struct scsi_device *sdp,
> @@ -1943,7 +1951,10 @@ static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
>  	sector_size = get_unaligned_be32(&buffer[8]);
>  	lba = get_unaligned_be64(&buffer[0]);
>  
> -	sd_read_protection_type(sdkp, buffer);
> +	if (sd_read_protection_type(sdkp, buffer) < 0) {
> +		sdkp->capacity = 0;
> +		return -ENODEV;
> +	}
>  
>  	if ((sizeof(sdkp->capacity) == 4) && (lba >= 0xffffffffULL)) {
>  		sd_printk(KERN_ERR, sdkp, "Too big for this kernel. Use a "
> 
Yup. That's what I had in mind.

Acked-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)


--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Kernel crash with unsupported DIF protection type

[Hannes Reinecke]

On 09/20/2012 09:53 PM, Martin K. Petersen wrote:
>>>>>> "Hannes" == Hannes Reinecke <hare@suse.de> writes:
> 
> Hannes> I recently got my hands on some weird drives, insisting on
> Hannes> having been formatted with protection type 7:
> 
> Lovely :|
> 
> 
> Hannes> I've attached a tentative patch, which allows the system to
> Hannes> boot.  However, I'm not completely happy with that, as the
> Hannes> capacity is _still_ updated after revalidation:
> 
> How about this?
> 
> 
> sd: Ensure we correctly disable devices with unknown protection type
> 
> We set the capacity to zero when we discovered a device formatted with
> an unknown DIF protection type. However, the read_capacity code would
> override the capacity and cause the device to be enabled regardless.
> 
> Make sd_read_protection_type() return an error if the protection type is
> unknown. Also prevent duplicate printk lines when the device is being
> revalidated.
> 
> Reported-by: Hannes Reinecke <hare@suse.de>
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 1cf2d5d..1c54564 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -1820,34 +1820,42 @@ sd_spinup_disk(struct scsi_disk *sdkp)
>  /*
>   * Determine whether disk supports Data Integrity Field.
>   */
> -static void sd_read_protection_type(struct scsi_disk *sdkp, unsigned char *buffer)
> +static int sd_read_protection_type(struct scsi_disk *sdkp, unsigned char *buffer)
>  {
>  	struct scsi_device *sdp = sdkp->device;
>  	u8 type;
> +	int ret = 0;
>  
>  	if (scsi_device_protection(sdp) == 0 || (buffer[12] & 1) == 0)
> -		return;
> +		return ret;
>  
>  	type = ((buffer[12] >> 1) & 7) + 1; /* P_TYPE 0 = Type 1 */
>  
> -	if (type == sdkp->protection_type || !sdkp->first_scan)
> -		return;
> +	if (type > SD_DIF_TYPE3_PROTECTION)
> +		ret = -ENODEV;
> +	else if (scsi_host_dif_capable(sdp->host, type))
> +		ret = 1;
> +
> +	if (sdkp->first_scan || type != sdkp->protection_type)
> +		switch (ret) {
> +		case -ENODEV:
> +			sd_printk(KERN_ERR, sdkp, "formatted with unsupported" \
> +				  " protection type %u. Disabling disk!\n",
> +				  type);
> +			break;
> +		case 1:
> +			sd_printk(KERN_NOTICE, sdkp,
> +				  "Enabling DIF Type %u protection\n", type);
> +			break;
> +		case 0:
> +			sd_printk(KERN_NOTICE, sdkp,
> +				  "Disabling DIF Type %u protection\n", type);
> +			break;
> +		}
>  
>  	sdkp->protection_type = type;
>  
> -	if (type > SD_DIF_TYPE3_PROTECTION) {
> -		sd_printk(KERN_ERR, sdkp, "formatted with unsupported "	\
> -			  "protection type %u. Disabling disk!\n", type);
> -		sdkp->capacity = 0;
> -		return;
> -	}
> -
> -	if (scsi_host_dif_capable(sdp->host, type))
> -		sd_printk(KERN_NOTICE, sdkp,
> -			  "Enabling DIF Type %u protection\n", type);
> -	else
> -		sd_printk(KERN_NOTICE, sdkp,
> -			  "Disabling DIF Type %u protection\n", type);
> +	return ret;
>  }
>  
>  static void read_capacity_error(struct scsi_disk *sdkp, struct scsi_device *sdp,
> @@ -1943,7 +1951,10 @@ static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
>  	sector_size = get_unaligned_be32(&buffer[8]);
>  	lba = get_unaligned_be64(&buffer[0]);
>  
> -	sd_read_protection_type(sdkp, buffer);
> +	if (sd_read_protection_type(sdkp, buffer) < 0) {
> +		sdkp->capacity = 0;
> +		return -ENODEV;
> +	}
>  
>  	if ((sizeof(sdkp->capacity) == 4) && (lba >= 0xffffffffULL)) {
>  		sd_printk(KERN_ERR, sdkp, "Too big for this kernel. Use a "

Hehe. It helps to actually test patches.

Now I get:
sd 6:0:0:0: [sdb] formatted with unsupported protection type 7.
Disabling disk!

and silence after that.
However, I still get:

sd 6:0:0:0: [sdb] Write Protect is off
sd 6:0:0:0: [sdb] Mode Sense: cf 00 10 08
sd 6:0:0:0: [sdb] Write cache: disabled, read cache: enabled,
supports DPO and FUA
sd 6:0:0:0: [sdb] Enabling DIX T10-DIF-TYPE1-CRC protection
sd 6:0:0:0: [sdb] Attached SCSI disk

which is a bit odd, given that the disk is disabled.
Plus that we never actually claimed to support type1.
So maybe we should add this patch, too?

diff --git a/drivers/scsi/sd_dif.c b/drivers/scsi/sd_dif.c
index 0cb39ff..e0500bf 100644
--- a/drivers/scsi/sd_dif.c
+++ b/drivers/scsi/sd_dif.c
@@ -320,7 +320,7 @@ void sd_dif_config_host(struct scsi_disk *sdkp)
                dif = 0; dix = 1;
        }

-       if (!dix)
+       if (!dix || type > SD_DIF_TYPE3_PROTECTION)
                return;

        /* Enable DMA of protection information */

But then, main issue is resolved, as the system continues to boot.
So my Acked-by: still stands.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)


--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Kernel crash with unsupported DIF protection type

[Martin K. Petersen]

>>>>> "Hannes" == Hannes Reinecke <hare@suse.de> writes:

Hannes> Hehe. It helps to actually test patches.

I did. However, with scsi_debug I have all sorts of sanity checks in
place. Not sure what your disk is connected to?


Hannes> sd 6:0:0:0: [sdb] Write Protect is off sd 6:0:0:0: [sdb] Mode
Hannes> Sense: cf 00 10 08 sd 6:0:0:0: [sdb] Write cache: disabled, read
Hannes> cache: enabled, supports DPO and FUA sd 6:0:0:0: [sdb] Enabling
Hannes> DIX T10-DIF-TYPE1-CRC protection sd 6:0:0:0: [sdb] Attached SCSI
Hannes> disk

DIX and DIF are orthogonal. If the HBA driver does not support the
appropriate DIF type we'll still enable DIX. That's a feature.

But there are two issues here. First we should not configure DIX if
capacity is 0. Secondly, we need some bounds checking in the host mask
checking to protect against crackpot disks.

Updated patch below...


sd: Ensure we correctly disable devices with unknown protection type

We set the capacity to zero when we discovered a device formatted with
an unknown DIF protection type. However, the read_capacity code would
override the capacity and cause the device to be enabled regardless.

Make sd_read_protection_type() return an error if the protection type is
unknown. Also prevent duplicate printk lines when the device is being
revalidated.

Reported-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 1cf2d5d..7a22db3 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1820,34 +1820,42 @@ sd_spinup_disk(struct scsi_disk *sdkp)
 /*
  * Determine whether disk supports Data Integrity Field.
  */
-static void sd_read_protection_type(struct scsi_disk *sdkp, unsigned char *buffer)
+static int sd_read_protection_type(struct scsi_disk *sdkp, unsigned char *buffer)
 {
 	struct scsi_device *sdp = sdkp->device;
 	u8 type;
+	int ret = 0;
 
 	if (scsi_device_protection(sdp) == 0 || (buffer[12] & 1) == 0)
-		return;
+		return ret;
 
 	type = ((buffer[12] >> 1) & 7) + 1; /* P_TYPE 0 = Type 1 */
 
-	if (type == sdkp->protection_type || !sdkp->first_scan)
-		return;
+	if (type > SD_DIF_TYPE3_PROTECTION)
+		ret = -ENODEV;
+	else if (scsi_host_dif_capable(sdp->host, type))
+		ret = 1;
+
+	if (sdkp->first_scan || type != sdkp->protection_type)
+		switch (ret) {
+		case -ENODEV:
+			sd_printk(KERN_ERR, sdkp, "formatted with unsupported" \
+				  " protection type %u. Disabling disk!\n",
+				  type);
+			break;
+		case 1:
+			sd_printk(KERN_NOTICE, sdkp,
+				  "Enabling DIF Type %u protection\n", type);
+			break;
+		case 0:
+			sd_printk(KERN_NOTICE, sdkp,
+				  "Disabling DIF Type %u protection\n", type);
+			break;
+		}
 
 	sdkp->protection_type = type;
 
-	if (type > SD_DIF_TYPE3_PROTECTION) {
-		sd_printk(KERN_ERR, sdkp, "formatted with unsupported "	\
-			  "protection type %u. Disabling disk!\n", type);
-		sdkp->capacity = 0;
-		return;
-	}
-
-	if (scsi_host_dif_capable(sdp->host, type))
-		sd_printk(KERN_NOTICE, sdkp,
-			  "Enabling DIF Type %u protection\n", type);
-	else
-		sd_printk(KERN_NOTICE, sdkp,
-			  "Disabling DIF Type %u protection\n", type);
+	return ret;
 }
 
 static void read_capacity_error(struct scsi_disk *sdkp, struct scsi_device *sdp,
@@ -1943,7 +1951,10 @@ static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
 	sector_size = get_unaligned_be32(&buffer[8]);
 	lba = get_unaligned_be64(&buffer[0]);
 
-	sd_read_protection_type(sdkp, buffer);
+	if (sd_read_protection_type(sdkp, buffer) < 0) {
+		sdkp->capacity = 0;
+		return -ENODEV;
+	}
 
 	if ((sizeof(sdkp->capacity) == 4) && (lba >= 0xffffffffULL)) {
 		sd_printk(KERN_ERR, sdkp, "Too big for this kernel. Use a "
@@ -2788,7 +2799,8 @@ static void sd_probe_async(void *data, async_cookie_t cookie)
 	}
 
 	add_disk(gd);
-	sd_dif_config_host(sdkp);
+	if (sdkp->capacity)
+		sd_dif_config_host(sdkp);
 
 	sd_revalidate_disk(gd);
 
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 5f7d5b3..b19223d 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -871,7 +871,11 @@ static inline unsigned int scsi_host_dif_capable(struct Scsi_Host *shost, unsign
 	static unsigned char cap[] = { 0,
 				       SHOST_DIF_TYPE1_PROTECTION,
 				       SHOST_DIF_TYPE2_PROTECTION,
-				       SHOST_DIF_TYPE3_PROTECTION };
+				       SHOST_DIF_TYPE3_PROTECTION,
+	};
+
+	if (target_type > SHOST_DIF_TYPE3_PROTECTION)
+		return 0;
 
 	return shost->prot_capabilities & cap[target_type] ? target_type : 0;
 }
@@ -884,6 +888,9 @@ static inline unsigned int scsi_host_dix_capable(struct Scsi_Host *shost, unsign
 				       SHOST_DIX_TYPE2_PROTECTION,
 				       SHOST_DIX_TYPE3_PROTECTION };
 
+	if (target_type > SHOST_DIX_TYPE3_PROTECTION)
+		return 0;
+
 	return shost->prot_capabilities & cap[target_type];
 #endif
 	return 0;

Re: Kernel crash with unsupported DIF protection type

[Martin K. Petersen]

>>>>> "Martin" == Martin K Petersen <martin.petersen@oracle.com> writes:

Martin> Updated patch below...

This time without mangled closing bracket...


sd: Ensure we correctly disable devices with unknown protection type

We set the capacity to zero when we discovered a device formatted with
an unknown DIF protection type. However, the read_capacity code would
override the capacity and cause the device to be enabled regardless.

Make sd_read_protection_type() return an error if the protection type is
unknown. Also prevent duplicate printk lines when the device is being
revalidated.

Reported-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 1cf2d5d..7a22db3 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1820,34 +1820,42 @@ sd_spinup_disk(struct scsi_disk *sdkp)
 /*
  * Determine whether disk supports Data Integrity Field.
  */
-static void sd_read_protection_type(struct scsi_disk *sdkp, unsigned char *buffer)
+static int sd_read_protection_type(struct scsi_disk *sdkp, unsigned char *buffer)
 {
 	struct scsi_device *sdp = sdkp->device;
 	u8 type;
+	int ret = 0;
 
 	if (scsi_device_protection(sdp) == 0 || (buffer[12] & 1) == 0)
-		return;
+		return ret;
 
 	type = ((buffer[12] >> 1) & 7) + 1; /* P_TYPE 0 = Type 1 */
 
-	if (type == sdkp->protection_type || !sdkp->first_scan)
-		return;
+	if (type > SD_DIF_TYPE3_PROTECTION)
+		ret = -ENODEV;
+	else if (scsi_host_dif_capable(sdp->host, type))
+		ret = 1;
+
+	if (sdkp->first_scan || type != sdkp->protection_type)
+		switch (ret) {
+		case -ENODEV:
+			sd_printk(KERN_ERR, sdkp, "formatted with unsupported" \
+				  " protection type %u. Disabling disk!\n",
+				  type);
+			break;
+		case 1:
+			sd_printk(KERN_NOTICE, sdkp,
+				  "Enabling DIF Type %u protection\n", type);
+			break;
+		case 0:
+			sd_printk(KERN_NOTICE, sdkp,
+				  "Disabling DIF Type %u protection\n", type);
+			break;
+		}
 
 	sdkp->protection_type = type;
 
-	if (type > SD_DIF_TYPE3_PROTECTION) {
-		sd_printk(KERN_ERR, sdkp, "formatted with unsupported "	\
-			  "protection type %u. Disabling disk!\n", type);
-		sdkp->capacity = 0;
-		return;
-	}
-
-	if (scsi_host_dif_capable(sdp->host, type))
-		sd_printk(KERN_NOTICE, sdkp,
-			  "Enabling DIF Type %u protection\n", type);
-	else
-		sd_printk(KERN_NOTICE, sdkp,
-			  "Disabling DIF Type %u protection\n", type);
+	return ret;
 }
 
 static void read_capacity_error(struct scsi_disk *sdkp, struct scsi_device *sdp,
@@ -1943,7 +1951,10 @@ static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
 	sector_size = get_unaligned_be32(&buffer[8]);
 	lba = get_unaligned_be64(&buffer[0]);
 
-	sd_read_protection_type(sdkp, buffer);
+	if (sd_read_protection_type(sdkp, buffer) < 0) {
+		sdkp->capacity = 0;
+		return -ENODEV;
+	}
 
 	if ((sizeof(sdkp->capacity) == 4) && (lba >= 0xffffffffULL)) {
 		sd_printk(KERN_ERR, sdkp, "Too big for this kernel. Use a "
@@ -2788,7 +2799,8 @@ static void sd_probe_async(void *data, async_cookie_t cookie)
 	}
 
 	add_disk(gd);
-	sd_dif_config_host(sdkp);
+	if (sdkp->capacity)
+		sd_dif_config_host(sdkp);
 
 	sd_revalidate_disk(gd);
 
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 5f7d5b3..4908480 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -873,6 +873,9 @@ static inline unsigned int scsi_host_dif_capable(struct Scsi_Host *shost, unsign
 				       SHOST_DIF_TYPE2_PROTECTION,
 				       SHOST_DIF_TYPE3_PROTECTION };
 
+	if (target_type > SHOST_DIF_TYPE3_PROTECTION)
+		return 0;
+
 	return shost->prot_capabilities & cap[target_type] ? target_type : 0;
 }
 
@@ -884,6 +887,9 @@ static inline unsigned int scsi_host_dix_capable(struct Scsi_Host *shost, unsign
 				       SHOST_DIX_TYPE2_PROTECTION,
 				       SHOST_DIX_TYPE3_PROTECTION };
 
+	if (target_type > SHOST_DIX_TYPE3_PROTECTION)
+		return 0;
+
 	return shost->prot_capabilities & cap[target_type];
 #endif
 	return 0;