Re: Kernel crash with unsupported DIF protection type

[Hannes Reinecke <hare@suse.de>]

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