[PATCH v2] hwmon: (corsair-cpro) Create debugfs entries after hwmon registration

[PATCH v2] hwmon: (corsair-cpro) Create debugfs entries after hwmon registration

[Linmao Li]

ccp_debugfs_init() registers debugfs files whose private data is the devm
allocated ccp.  It runs before hwmon_device_register_with_info(), so when
that registration fails, ccp_probe() returns with the files still in
place.  The HID core then frees ccp, and ccp_remove() is not called for a
failed probe, so nothing removes them later either.  Reading one of the
files dereferences the freed pointer.

Create the debugfs entries only after the hwmon device has been
registered, so no failing path can leave them behind.

The two version queries stay where they are.  They send USB commands
without holding ccp->mutex, which is only safe as long as nothing else
can call send_usb_cmd(); once the hwmon device is registered its
callbacks can do so concurrently.  Only the debugfs creation moves, and
it is told which queries succeeded.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/linux-hwmon/20260708031612.BD7E61F000E9@smtp.kernel.org/
Suggested-by: Guenter Roeck <linux@roeck-us.net>
Fixes: 5997eb60f896 ("hwmon: (corsair-cpro) Add firmware and bootloader information")
Signed-off-by: Linmao Li <lilinmao@kylinos.cn>
---

Notes (v2changelog):
    v2: Create the debugfs entries after hwmon registration instead of
        removing them on the error path, as suggested by Guenter Roeck.  The
        version queries stay before the registration.
    Link: https://lore.kernel.org/linux-hwmon/20260828061949.3151191-1-lilinmao@kylinos.cn/

 drivers/hwmon/corsair-cpro.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/hwmon/corsair-cpro.c b/drivers/hwmon/corsair-cpro.c
index 8354a002f4c5..56de0fe0f544 100644
--- a/drivers/hwmon/corsair-cpro.c
+++ b/drivers/hwmon/corsair-cpro.c
@@ -566,21 +566,18 @@ static int bootloader_show(struct seq_file *seqf, void *unused)
 }
 DEFINE_SHOW_ATTRIBUTE(bootloader);
 
-static void ccp_debugfs_init(struct ccp_device *ccp)
+static void ccp_debugfs_init(struct ccp_device *ccp, bool fw_valid, bool bl_valid)
 {
 	char name[32];
-	int ret;
 
 	scnprintf(name, sizeof(name), "corsaircpro-%s", dev_name(&ccp->hdev->dev));
 	ccp->debugfs = debugfs_create_dir(name, NULL);
 
-	ret = get_fw_version(ccp);
-	if (!ret)
+	if (fw_valid)
 		debugfs_create_file("firmware_version", 0444,
 				    ccp->debugfs, ccp, &firmware_fops);
 
-	ret = get_bl_version(ccp);
-	if (!ret)
+	if (bl_valid)
 		debugfs_create_file("bootloader_version", 0444,
 				    ccp->debugfs, ccp, &bootloader_fops);
 }
@@ -588,6 +585,7 @@ static void ccp_debugfs_init(struct ccp_device *ccp)
 static int ccp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 {
 	struct ccp_device *ccp;
+	bool fw_valid, bl_valid;
 	int ret;
 
 	ccp = devm_kzalloc(&hdev->dev, sizeof(*ccp), GFP_KERNEL);
@@ -632,7 +630,13 @@ static int ccp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	if (ret)
 		goto out_hw_close;
 
-	ccp_debugfs_init(ccp);
+	/*
+	 * Query the versions before registering the hwmon device: they send
+	 * USB commands without holding ccp->mutex, which is only safe while
+	 * nothing else can call send_usb_cmd().
+	 */
+	fw_valid = !get_fw_version(ccp);
+	bl_valid = !get_bl_version(ccp);
 
 	ccp->hwmon_dev = hwmon_device_register_with_info(&hdev->dev, "corsaircpro",
 							 ccp, &ccp_chip_info, NULL);
@@ -641,6 +645,8 @@ static int ccp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		goto out_hw_close;
 	}
 
+	ccp_debugfs_init(ccp, fw_valid, bl_valid);
+
 	return 0;
 
 out_hw_close:

base-commit: 8d3ae59288f1e7d58d76558a6ee96d533bc5019f
-- 
2.25.1

Re: [PATCH v2] hwmon: (corsair-cpro) Create debugfs entries after hwmon registration

[sashiko-bot]

> ccp_debugfs_init() registers debugfs files whose private data is the devm
> allocated ccp.  It runs before hwmon_device_register_with_info(), so when
> that registration fails, ccp_probe() returns with the files still in
> place.  The HID core then frees ccp, and ccp_remove() is not called for a
> failed probe, so nothing removes them later either.  Reading one of the
> files dereferences the freed pointer.
> 
> Create the debugfs entries only after the hwmon device has been
> registered, so no failing path can leave them behind.
> 
> The two version queries stay where they are.  They send USB commands
> without holding ccp->mutex, which is only safe as long as nothing else
> can call send_usb_cmd(); once the hwmon device is registered its
> callbacks can do so concurrently.  Only the debugfs creation moves, and
> it is told which queries succeeded.
> 
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://lore.kernel.org/linux-hwmon/20260708031612.BD7E61F000E9@smtp.kernel.org/
> Suggested-by: Guenter Roeck <linux@roeck-us.net>
> Fixes: 5997eb60f896 ("hwmon: (corsair-cpro) Add firmware and bootloader information")
> Signed-off-by: Linmao Li <lilinmao@kylinos.cn>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831014509.3352442-1-lilinmao@kylinos.cn?part=1

Re: [PATCH v2] hwmon: (corsair-cpro) Create debugfs entries after hwmon registration

[Guenter Roeck]

On Mon, Aug 31, 2026 at 09:45:09AM +0800, Linmao Li wrote:
> ccp_debugfs_init() registers debugfs files whose private data is the devm
> allocated ccp.  It runs before hwmon_device_register_with_info(), so when
> that registration fails, ccp_probe() returns with the files still in
> place.  The HID core then frees ccp, and ccp_remove() is not called for a
> failed probe, so nothing removes them later either.  Reading one of the
> files dereferences the freed pointer.
> 
> Create the debugfs entries only after the hwmon device has been
> registered, so no failing path can leave them behind.
> 
> The two version queries stay where they are.  They send USB commands
> without holding ccp->mutex, which is only safe as long as nothing else
> can call send_usb_cmd(); once the hwmon device is registered its
> callbacks can do so concurrently.  Only the debugfs creation moves, and
> it is told which queries succeeded.
> 
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://lore.kernel.org/linux-hwmon/20260708031612.BD7E61F000E9@smtp.kernel.org/
> Suggested-by: Guenter Roeck <linux@roeck-us.net>
> Fixes: 5997eb60f896 ("hwmon: (corsair-cpro) Add firmware and bootloader information")
> Signed-off-by: Linmao Li <lilinmao@kylinos.cn>

Applied.

Thanks,
Guenter

> ---
> 
> Notes (v2changelog):
>     v2: Create the debugfs entries after hwmon registration instead of
>         removing them on the error path, as suggested by Guenter Roeck.  The
>         version queries stay before the registration.
>     Link: https://lore.kernel.org/linux-hwmon/20260828061949.3151191-1-lilinmao@kylinos.cn/
> 
>  drivers/hwmon/corsair-cpro.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
> 
> 
> base-commit: 8d3ae59288f1e7d58d76558a6ee96d533bc5019f
> 
> diff --git a/drivers/hwmon/corsair-cpro.c b/drivers/hwmon/corsair-cpro.c
> index 8354a002f4c5..56de0fe0f544 100644
> --- a/drivers/hwmon/corsair-cpro.c
> +++ b/drivers/hwmon/corsair-cpro.c
> @@ -566,21 +566,18 @@ static int bootloader_show(struct seq_file *seqf, void *unused)
>  }
>  DEFINE_SHOW_ATTRIBUTE(bootloader);
>  
> -static void ccp_debugfs_init(struct ccp_device *ccp)
> +static void ccp_debugfs_init(struct ccp_device *ccp, bool fw_valid, bool bl_valid)
>  {
>  	char name[32];
> -	int ret;
>  
>  	scnprintf(name, sizeof(name), "corsaircpro-%s", dev_name(&ccp->hdev->dev));
>  	ccp->debugfs = debugfs_create_dir(name, NULL);
>  
> -	ret = get_fw_version(ccp);
> -	if (!ret)
> +	if (fw_valid)
>  		debugfs_create_file("firmware_version", 0444,
>  				    ccp->debugfs, ccp, &firmware_fops);
>  
> -	ret = get_bl_version(ccp);
> -	if (!ret)
> +	if (bl_valid)
>  		debugfs_create_file("bootloader_version", 0444,
>  				    ccp->debugfs, ccp, &bootloader_fops);
>  }
> @@ -588,6 +585,7 @@ static void ccp_debugfs_init(struct ccp_device *ccp)
>  static int ccp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  {
>  	struct ccp_device *ccp;
> +	bool fw_valid, bl_valid;
>  	int ret;
>  
>  	ccp = devm_kzalloc(&hdev->dev, sizeof(*ccp), GFP_KERNEL);
> @@ -632,7 +630,13 @@ static int ccp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  	if (ret)
>  		goto out_hw_close;
>  
> -	ccp_debugfs_init(ccp);
> +	/*
> +	 * Query the versions before registering the hwmon device: they send
> +	 * USB commands without holding ccp->mutex, which is only safe while
> +	 * nothing else can call send_usb_cmd().
> +	 */
> +	fw_valid = !get_fw_version(ccp);
> +	bl_valid = !get_bl_version(ccp);
>  
>  	ccp->hwmon_dev = hwmon_device_register_with_info(&hdev->dev, "corsaircpro",
>  							 ccp, &ccp_chip_info, NULL);
> @@ -641,6 +645,8 @@ static int ccp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  		goto out_hw_close;
>  	}
>  
> +	ccp_debugfs_init(ccp, fw_valid, bl_valid);
> +
>  	return 0;
>  
>  out_hw_close: