Re: [PATCH v2 07/10] qla2xxx: Terminate exchange if corrputed.

[Bart Van Assche <Bart.VanAssche@sandisk.com>]

On Wed, 2016-12-21 at 13:57 -0800, Himanshu Madhani wrote:
> diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
> index f7df01b..b14455e 100644
> --- a/drivers/scsi/qla2xxx/qla_def.h
> +++ b/drivers/scsi/qla2xxx/qla_def.h
> @@ -1556,7 +1556,8 @@ struct link_statistics {
>  struct atio {
>  	uint8_t		entry_type;		/* Entry type. */
>  	uint8_t		entry_count;		/* Entry count. */
> -	uint8_t		data[58];
> +	uint16_t	attr_n_length;
> +	uint8_t		data[56];
>  	uint32_t	signature;
>  #define ATIO_PROCESSED 0xDEADDEAD		/* Signature */
> };

Are struct atio and/or struct atio_from_isp the description of a firmware
data structure? If so, do all firmware versions initialize the attr_n_length
field or is there a minimum firmware version required for this field to be
valid? Please mention any changed dependencies on the firmware version in the
patch description. Please also fix the typo in the patch title when reposting
this patch ("corrputed").

> +			/* This packet is corrupted. The header + payload
> +			 * can not be trusted. There is no point in passing
> +			 * it further up.
> +			 */

This is not the proper Linux kernel comment style (see also
Documentation/process/coding-style.rst).

> diff --git a/drivers/scsi/qla2xxx/qla_target.h b/drivers/scsi/qla2xxx/qla_target.h
> index f26c5f6..f31d343 100644
> --- a/drivers/scsi/qla2xxx/qla_target.h
> +++ b/drivers/scsi/qla2xxx/qla_target.h
> @@ -427,13 +427,33 @@ struct atio_from_isp {
>  		struct {
>  			uint8_t  entry_type;	/* Entry type. */
>  			uint8_t  entry_count;	/* Entry count. */
> -			uint8_t  data[58];
> +			uint16_t attr_n_length;
> +#define FCP_CMD_LENGTH_MASK 0x0fff
> +#define FCP_CMD_LENGTH_MIN  0x38
> +			uint8_t  data[56];
>  			uint32_t signature;
>  #define ATIO_PROCESSED 0xDEADDEAD		/* Signature */
>  		} raw;
>  	} u;
>  } __packed;
>  
> +static inline int fcpcmd_is_corrupted(struct atio *atio)
> +{
> +	if (atio->entry_type == ATIO_TYPE7 &&
> +	    (le16_to_cpu(atio->attr_n_length & FCP_CMD_LENGTH_MASK) <
> +	    FCP_CMD_LENGTH_MIN))
> +		return 1;
> +	else
> +		return 0;
> +}

The above code shows that attr_n_length is a little endian field so please
declare it as little endian where appropriate (__le16 instead of uint16_t).

Bart.