Re: [PATCH v2 6/7] HID: sensor: custom: Register enable attribute after misc device

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] HID: sensor: custom: duplicate callback removal in hid_sensor_custom_probe()
--

commit 3c6fa7005fd38bc50c4da9b69a1182ec5457915f
Author: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>

HID: sensor: custom: Register enable attribute after misc device

This commit reorders hid_sensor_custom_probe() so enable_sensor_attr_group is
registered after hid_sensor_custom_dev_if_add() succeeds, preventing userspace
from enabling the sensor hub while probe can still fail.

> diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-sensor-custom.c
> index 4f824cadd370a..990e653995e15 100644
> --- a/drivers/hid/hid-sensor-custom.c
> +++ b/drivers/hid/hid-sensor-custom.c
> @@ -1059,20 +1059,21 @@ static int hid_sensor_custom_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto err_remove_callback;
>  
> -	ret = sysfs_create_group(&sensor_inst->pdev->dev.kobj,
> -				 &enable_sensor_attr_group);
> +	ret = hid_sensor_custom_dev_if_add(sensor_inst);
>  	if (ret)
>  		goto err_remove_attributes;
>  
> -	ret = hid_sensor_custom_dev_if_add(sensor_inst);
> +	ret = sysfs_create_group(&sensor_inst->pdev->dev.kobj,
> +				 &enable_sensor_attr_group);
>  	if (ret)
> -		goto err_remove_group;
> +		goto err_remove_dev_if;
>  
>  	return 0;
>  
> -err_remove_group:
> -	sysfs_remove_group(&sensor_inst->pdev->dev.kobj,
> -			   &enable_sensor_attr_group);
> +err_remove_dev_if:
> +	sensor_hub_remove_callback(hsdev, hsdev->usage);
> +	hid_sensor_custom_dev_if_remove(sensor_inst);
> +	goto err_remove_attributes;
>  err_remove_attributes:
>  	hid_sensor_custom_remove_attributes(sensor_inst);
>  err_remove_callback:

[Severity: Low]
In hid_sensor_custom_probe() in drivers/hid/hid-sensor-custom.c, when
sysfs_create_group() for enable_sensor_attr_group fails and jumps to
err_remove_dev_if, sensor_hub_remove_callback(hsdev, hsdev->usage) is called
before hid_sensor_custom_remove_attributes(sensor_inst), followed by a goto to
the immediately following err_remove_attributes label and a fallthrough to
err_remove_callback:

err_remove_dev_if:
	sensor_hub_remove_callback(hsdev, hsdev->usage);
	hid_sensor_custom_dev_if_remove(sensor_inst);
	goto err_remove_attributes;
err_remove_attributes:
	hid_sensor_custom_remove_attributes(sensor_inst);
err_remove_callback:
	sensor_hub_remove_callback(hsdev, hsdev->usage);

Because hid_sensor_custom_probe() still holds the initial reference on
sensor_inst until err_free (where kref_put() calls hid_sensor_custom_free() to
free sensor_inst->data_fifo and sensor_inst), removing the callback at
err_remove_dev_if unregisters the callback while the field sysfs attributes are
still exposed, and then calls sensor_hub_remove_callback() a second time at
err_remove_callback.

Should err_remove_dev_if just call hid_sensor_custom_dev_if_remove(sensor_inst)
and fall through to err_remove_attributes and err_remove_callback?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104201403.2039921-1-srinivas.pandruvada@linux.intel.com?part=6