Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
Message-ID: <67fbbd3c-d4ed-4fa1-9802-fbb5c045b426@intel.com>
Date:   Fri, 17 Nov 2023 08:50:14 -0700
User-Agent: Betterbird (Linux)
Subject: Re: [PATCH] cxl/cdat: Free correct buffer on checksum error
To:     Ira Weiny <ira.weiny@intel.com>,
        Davidlohr Bueso <dave@stgolabs.net>,
        Jonathan Cameron <jonathan.cameron@huawei.com>,
        Alison Schofield <alison.schofield@intel.com>,
        Vishal Verma <vishal.l.verma@intel.com>,
        "Dan Williams" <dan.j.williams@intel.com>
CC:     <linux-cxl@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        Fan Ni <nifan.cxl@gmail.com>
References: <20231116-fix-cdat-devm-free-v1-1-b148b40707d7@intel.com>
Content-Language: en-US
From:   Dave Jiang <dave.jiang@intel.com>
In-Reply-To: <20231116-fix-cdat-devm-free-v1-1-b148b40707d7@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?UUN1bG00VXlkb0loRW8xLzJTMnFpakJFejNVZVZJc3IzcFBWZGRUZUN1dXhQ?=
 =?utf-8?B?YlJNVUJWOUp1TTZRakJmR095RTZMZ2I0Sk5Yc2RVOWdvV2IwS3lTd0c3cUd0?=
 =?utf-8?B?U2RITzZIVlJ4cUNhZWdlWDMrOHd0a3d6bFhTQkJjTjBteUV5VDV3bjdCdVVC?=
 =?utf-8?B?K0tRTks4WEZkOGRoYkM2bS9yVWRGbmtrREE2TFA2NVFtaDB6RGVuenRxa2hv?=
 =?utf-8?B?NFc1ODNtbXA1OVBucmNnQWcrZFhFcDZEQ2pMODVSWmxTampWM0pLTXVGaWxU?=
 =?utf-8?B?eHBBS2F2bE1VQWtjeVFPUm1SZ0VWaVVLaXpXYi96MHRKRGsweXRLYzZGV0tP?=
 =?utf-8?B?U3hyczlqZU5oZzVXaHBZalRJQVZxZm1rTDRsVkNITE1GUlZlTHB5QUpiaEo1?=
 =?utf-8?B?L3RGQUpqMUJvcE9NVDhyYXNUSVNrcERrbWhNRjhKQmRteElZN3BObUlPVDE2?=
 =?utf-8?B?WTV4ajlCWWFtWHNmK0FEajQ0QlBGVUxQSTVYenQ1L1BhMzg3a2dIcSs3d1FN?=
 =?utf-8?B?dk1RbHZXU2pQSnNHQ1F6aDYyL1VMZmlQdlpLcVorQzMweWZkQlJxd1NZQUFy?=
 =?utf-8?B?bFNnVGlOTVhCd3NucE5Sek9yd29HbmI5VHpuNjZicWhVV3htTVhqTzlmMjhn?=
 =?utf-8?B?c0pPWnhWdWljZGxxSmhxNm9HTm5pSmtKbVMvRXdtdHVqeTREM1hvUlpPQjRG?=
 =?utf-8?B?U3pkWXJ3M3krc2NMczY2YmdIU3ZTRjBhcmsrZ3pzVlYyay9ENDFqMjZyWC93?=
 =?utf-8?B?M3R4UHdWbEpEd1NQWXdncVZuTDR3TXV4MDZsUkplQVFCZC8vVzhHUm5kRm90?=
 =?utf-8?B?cGZuejZObTZMWk9VWmErN2RYRkVoZUgvZzZtODJ1cDNnRkd5bE5xczNtZEtR?=
 =?utf-8?B?UFhCWk1RNSttaEt6V201cjNLems2UnZET1dhTnFNNmFHRno0QkJKdWFBTGVG?=
 =?utf-8?B?akxGUkt2Vml5eFVtTzhMLzFKRmIxWkUyeGNtL0dueXl6MTY5Tjk2ZGhOWStB?=
 =?utf-8?B?Q2FtSmdodmIyV2NlUXZKdHdDQjdNUWo4a3kvWWk4c2tqZkhzNG1KVDI0bENw?=
 =?utf-8?B?Z2QyNFBmNUM0RUw2R2lIWE9NeE4yR1BWVC9DUlovZ3pNNkFvNWdZa1g2V2JN?=
 =?utf-8?B?VVlzZitWNTBxeVRHeDc2UUxBak9BRFB5TXUxQzRzV3I3dXlvVmFkSlRLSi9T?=
 =?utf-8?B?NVFKSUZ6WldnVXFNVER1bVhLdXlCUjhpWStNNCtncVFkRU9rcVRHeHhKQWxu?=
 =?utf-8?B?dFVjaWxGVmUxTFdmSEthejhaSmt4a29mZ1Y0dEFzelROOGZUWlNVd3U0UktE?=
 =?utf-8?B?RXAzaUJJRnNTMFBzbFVDMlVWb2NKcnFyVkR5ZFdEYmNkd1NCSlQydVM3ckIx?=
 =?utf-8?B?V1hFcGU0WS9TbVphaklaTStjMVNMa0dhSFVxU1JPS2RuNlg1MzdNRUU1SDlK?=
 =?utf-8?B?V2dRZW9FQ2J0SVVRLzR1UDFWb1Z0YTBhOXV2blpuUTlxNHhLbk9va3lrZEpT?=
 =?utf-8?B?c0p3LzB4bXR4UHpHN1NIaFZpQ2pvVzRXZ213VWNLMG1PQWp4SXY0R0Yzd2lk?=
 =?utf-8?B?azJIbTRiQWNDZTdMemFpaHlSN0pjMDYzMk8rL3ZVUjEwUU9MTmFPclRyNGli?=
 =?utf-8?B?aDA1U0JmbG9mM1hQTitKbmlHV0J3V0FNZHdLQ0Q0aDZVREc3UitNQ05ESGRE?=
 =?utf-8?B?MGZFbUpKUklzNzQ0NzgwR1hkbCtjUm1xZ0JKMHc5ZzVsaHFJZWpjSWZaSFFY?=
 =?utf-8?B?ck1VOGNlU3o3ZUliSzk5c2F0OCsrK2VvOWVRaTRmQlZsbHcxV0I4VjkwNms5?=
 =?utf-8?B?akljeTc3RXlZZERGUDBZanJYYVdsMnpuSncyamcweW01UUVBdHRLekc4c3Ro?=
 =?utf-8?B?MmZucVhYYVNPdWlaQnpjd05mNEkxT05pMVd0ZUJLK0RhVFdMRXc4a2RoT2Rq?=
 =?utf-8?B?VW52R0xvUS9LV3ljQjN4dGtjeTJKdk5CUHB6ZnlBemhLZzBka2RRWERIV0xp?=
 =?utf-8?B?b2pJQmp5cUw3QUdDUWFUZTBpWkdkVm9KdFRVMGNXNyt3a0ZzdjFRbjFkd2py?=
 =?utf-8?B?VnREek52bnBpZzVPcDUxYU5xdVkzT3FqOVlDSTRMazdySlFCNlRhLzBFQmNS?=
 =?utf-8?Q?M/YOJIbub7gAq9m0PythblNv+?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 4e0c5095-c1ef-4cdc-8e66-08dbe784e59b
X-MS-Exchange-CrossTenant-AuthSource: PH7PR11MB5984.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Nov 2023 15:50:17.9583
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: h6/leIjjJpPWrhBrpMi5uPYxQcNqlVXm4JTvsb363Iafs6VdEMHUO4lisqaj2DkL9yi0SoGdB2kAjiC/enZv8g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY8PR11MB7292
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE,
	URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on fry.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (fry.vger.email [0.0.0.0]); Fri, 17 Nov 2023 07:50:36 -0800 (PST)


On 11/16/23 17:03, Ira Weiny wrote:
> The new 6.7-rc1 kernel now checks the checksum on CDAT data.  While
> using a branch of Fan's DCD qemu work (and specifying DCD devices), the
> following splat was observed.
> 
> 	WARNING: CPU: 1 PID: 1384 at drivers/base/devres.c:1064 devm_kfree+0x4f/0x60
> 	...
> 	RIP: 0010:devm_kfree+0x4f/0x60
> 	...
>  	? devm_kfree+0x4f/0x60
>  	read_cdat_data+0x1a0/0x2a0 [cxl_core]
>  	cxl_port_probe+0xdf/0x200 [cxl_port]
> 	...
> 
> The issue in qemu is still unknown but the spat is a straight forward
> bug in the CDAT checksum processing code.  Use a CDAT buffer variable to
> ensure the devm_free() works correctly on error.
> 
> Cc: jonathan.cameron@huawei.com
> Cc: Fan Ni <nifan.cxl@gmail.com>
> Fixes: 670e4e88f3b1 ("cxl: Add checksum verification to CDAT from CXL")
> Cc: Dave Jiang <dave.jiang@intel.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>

Thanks for the fix

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>  drivers/cxl/core/pci.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index eff20e83d0a6..5aaa0b36c42a 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -620,7 +620,7 @@ void read_cdat_data(struct cxl_port *port)
>  	struct pci_dev *pdev = NULL;
>  	struct cxl_memdev *cxlmd;
>  	size_t cdat_length;
> -	void *cdat_table;
> +	void *cdat_table, *cdat_buf;
>  	int rc;
>  
>  	if (is_cxl_memdev(uport)) {
> @@ -651,16 +651,16 @@ void read_cdat_data(struct cxl_port *port)
>  		return;
>  	}
>  
> -	cdat_table = devm_kzalloc(dev, cdat_length + sizeof(__le32),
> +	cdat_buf = devm_kzalloc(dev, cdat_length + sizeof(__le32),
>  				  GFP_KERNEL);
> -	if (!cdat_table)
> +	if (!cdat_buf)
>  		return;
>  
> -	rc = cxl_cdat_read_table(dev, cdat_doe, cdat_table, &cdat_length);
> +	rc = cxl_cdat_read_table(dev, cdat_doe, cdat_buf, &cdat_length);
>  	if (rc)
>  		goto err;
>  
> -	cdat_table = cdat_table + sizeof(__le32);
> +	cdat_table = cdat_buf + sizeof(__le32);
>  	if (cdat_checksum(cdat_table, cdat_length))
>  		goto err;
>  
> @@ -670,7 +670,7 @@ void read_cdat_data(struct cxl_port *port)
>  
>  err:
>  	/* Don't leave table data allocated on error */
> -	devm_kfree(dev, cdat_table);
> +	devm_kfree(dev, cdat_buf);
>  	dev_err(dev, "Failed to read/validate CDAT.\n");
>  }
>  EXPORT_SYMBOL_NS_GPL(read_cdat_data, CXL);
> 
> ---
> base-commit: 7475e51b87969e01a6812eac713a1c8310372e8a
> change-id: 20231116-fix-cdat-devm-free-b47d32b4b833
> 
> Best regards,