linux-kernel - Re: [PATCH v2] cxl/memdev: automate cleanup with _

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250624154314.00004fde@huawei.com>
Date: Tue, 24 Jun 2025 15:43:14 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Pranav Tyagi <pranav.tyagi03@...il.com>
CC: <dave@...olabs.net>, <dave.jiang@...el.com>, <alison.schofield@...el.com>,
	<vishal.l.verma@...el.com>, <ira.weiny@...el.com>,
	<dan.j.williams@...el.com>, <linux-cxl@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <ming.li@...omail.com>, <rrichter@....com>,
	<peterz@...radead.org>, <skhan@...uxfoundation.org>,
	<linux-kernel-mentees@...ts.linux.dev>
Subject: Re: [PATCH v2] cxl/memdev: automate cleanup with __free()

On Mon, 23 Jun 2025 14:08:41 +0530
Pranav Tyagi <pranav.tyagi03@...il.com> wrote:

> Use the scope based resource management (defined in linux/cleanup.h) to
> automate the lifetime control of struct cxl_mbox_transfer_fw. This
> eliminates explicit kfree() calls and makes the code more robust and
> maintainable in presence of early returns.
> 
> Signed-off-by: Pranav Tyagi <pranav.tyagi03@...il.com>

Resend as I accidentally only sent 1st reply to Pranav.

> ---
>  drivers/cxl/core/memdev.c | 21 ++++++++-------------
>  1 file changed, 8 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index f88a13adf7fa..38f4449f9740 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -7,6 +7,7 @@
>  #include <linux/slab.h>
>  #include <linux/idr.h>
>  #include <linux/pci.h>
> +#include <linux/cleanup.h>
>  #include <cxlmem.h>
>  #include "trace.h"
>  #include "core.h"
> @@ -802,11 +803,10 @@ static int cxl_mem_activate_fw(struct cxl_memdev_state *mds, int slot)
>  static int cxl_mem_abort_fw_xfer(struct cxl_memdev_state *mds)
>  {
>  	struct cxl_mailbox *cxl_mbox = &mds->cxlds.cxl_mbox;
> -	struct cxl_mbox_transfer_fw *transfer;
>  	struct cxl_mbox_cmd mbox_cmd;
> -	int rc;
> -
> -	transfer = kzalloc(struct_size(transfer, data, 0), GFP_KERNEL);
> +	
> +	struct cxl_mbox_transfer_fw *transfer __free(kfree) =
> +		kzalloc(struct_size(transfer, data, 0), GFP_KERNEL);

This one is fine.

>  	if (!transfer)
>  		return -ENOMEM;
>  
> @@ -821,9 +821,7 @@ static int cxl_mem_abort_fw_xfer(struct cxl_memdev_state *mds)
>  
>  	transfer->action = CXL_FW_TRANSFER_ACTION_ABORT;
>  
> -	rc = cxl_internal_send_cmd(cxl_mbox, &mbox_cmd);
> -	kfree(transfer);
> -	return rc;
> +	return cxl_internal_send_cmd(cxl_mbox, &mbox_cmd);
>  }
>  
>  static void cxl_fw_cleanup(struct fw_upload *fwl)
> @@ -880,7 +878,7 @@ static enum fw_upload_err cxl_fw_write(struct fw_upload *fwl, const u8 *data,
>  	struct cxl_dev_state *cxlds = &mds->cxlds;
>  	struct cxl_mailbox *cxl_mbox = &cxlds->cxl_mbox;
>  	struct cxl_memdev *cxlmd = cxlds->cxlmd;
> -	struct cxl_mbox_transfer_fw *transfer;
> +	struct cxl_mbox_transfer_fw *transfer __free(kfree);

This one is not.

Look at the comments in cleanup.h and consider if this obeys the rules
on use of this feature that are laid out there.  As it stands you will have
kfree() of an uninitialized pointer with unpredictable results in some
of the error paths.

>  	struct cxl_mbox_cmd mbox_cmd;
>  	u32 cur_size, remaining;
>  	size_t size_in;
> @@ -949,7 +947,7 @@ static enum fw_upload_err cxl_fw_write(struct fw_upload *fwl, const u8 *data,
>  	rc = cxl_internal_send_cmd(cxl_mbox, &mbox_cmd);
>  	if (rc < 0) {
>  		rc = FW_UPLOAD_ERR_RW_ERROR;
> -		goto out_free;
> +		return rc;
>  	}
>  
>  	*written = cur_size;
> @@ -963,14 +961,11 @@ static enum fw_upload_err cxl_fw_write(struct fw_upload *fwl, const u8 *data,
>  			dev_err(&cxlmd->dev, "Error activating firmware: %d\n",
>  				rc);
>  			rc = FW_UPLOAD_ERR_HW_ERROR;
> -			goto out_free;
> +			return rc;
>  		}
>  	}
>  
>  	rc = FW_UPLOAD_ERR_NONE;

return FW_UPLOAD_ERR_NONE;

> -
> -out_free:
> -	kfree(transfer);
>  	return rc;
>  }
>