RE: [PATCH v2 3/4] cxl: Add checksum verification to CDAT from CXL

From: Dan Williams <dan.j.williams@intel.com>
To: Dave Jiang <dave.jiang@intel.com>, <linux-cxl@vger.kernel.org>
Cc: Ira Weiny <ira.weiny@intel.com>,
	Jonathan Cameron <Jonathan.Cameron@huawei.com>,
	<dan.j.williams@intel.com>, <vishal.l.verma@intel.com>,
	<alison.schofield@intel.com>
Subject: RE: [PATCH v2 3/4] cxl: Add checksum verification to CDAT from CXL
Date: Fri, 12 May 2023 14:38:05 -0700	[thread overview]
Message-ID: <645eb1bd61964_1e6f29419@dwillia2-xfh.jf.intel.com.notmuch> (raw)
In-Reply-To: <168382794106.3510737.6205740820012769763.stgit@djiang5-mobl3>

Dave Jiang wrote:
> A CDAT table is available from a CXL device. The table is read by the
> driver and cached in software. With the CXL subsystem needing to parse the
> CDAT table, the checksum should be verified. Add checksum verification
> after the CDAT table is read from device.
> 
> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> 
> ---
> Post patch series split:
> v2:
> - Split out fix for cdat_read() error path return. (Davidlohr)
> - Make port->cdat consistent. (Davidlohr)
> 
> ___
> v5:
> - Return on CDAT errors. (Dan)
> v3:
> - Just return the final sum. (Alison)
> v2:
> - Drop ACPI checksum export and just use local verification. (Dan)
> ---
>  drivers/cxl/core/pci.c |   20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index f332fe7af92b..64ae45ae7ad6 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -528,6 +528,16 @@ static int cxl_cdat_read_table(struct device *dev,
>  	return 0;
>  }
>  
> +static unsigned char cdat_checksum(void *buf, size_t size)
> +{
> +	unsigned char sum, *data = buf;
> +	size_t i;
> +
> +	for (sum = 0, i = 0; i < size; i++)
> +		sum += data[i];
> +	return sum;
> +}
> +
>  /**
>   * read_cdat_data - Read the CDAT data on this port
>   * @port: Port to read data from
> @@ -574,7 +584,15 @@ void read_cdat_data(struct cxl_port *port)
>  		return;
>  	}
>  
> -	port->cdat.table = cdat_table + sizeof(__le32);
> +	cdat_table = cdat_table + sizeof(__le32);
> +	if (cdat_checksum(cdat_table, cdat_length)) {
> +		/* Don't leave table data allocated on error */
> +		devm_kfree(dev, cdat_table);
> +		dev_err(dev, "CDAT data checksum error\n");
> +		return;

Rather than duplicate this pattern I would switch to a "goto err"
arrangement:

err:
	devm_kfree(dev, cdat_table);
	dev_err(dev, "failed to read/validate CDAT\n");

...then you also don't need the "return" statement. The error message
nuance of "failed to read" vs "failed to checksum" does not really need
separate error statements, the result is the same.