Re: [PATCH v12 07/17] HID: asus: add joysticks anti-deadzone configuration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] HID: asus: ABBA deadlock between ally_prefs.lock and kernfs_drain()
--

commit 69043ee7052574919d9b6d50e4a364916a41871f
Author: Denis Benato <denis.benato@linux.dev>

HID: asus: add joysticks anti-deadzone configuration

This commit adds sysfs attributes to configure the joystick anti-deadzone
parameter on ROG Ally devices and restores the configured values during
device initialization.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 5cf7258bc4273..9bfdde560834d 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c

[ ... ]

> @@ -1321,11 +1324,152 @@ static struct device_attribute dev_attr_right_joystick_outer_threshold =
>  static struct device_attribute dev_attr_right_joystick_outer_threshold_range =
>  	__ATTR(outer_threshold_range, 0444, right_joystick_outer_threshold_range_show, NULL);
>  

[ ... ]

> +static ssize_t ally_anti_deadzone_show(struct device *dev, char *buf,
> +				       enum ally_joystick which)
> +{
> +	guard(mutex)(&ally_prefs.lock);
> +
> +	if (!ally_prefs.anti_deadzone_support) {
> +		hid_dbg(to_hid_device(dev), "Anti-deadzone not supported on this device\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return sysfs_emit(buf, "%u\n", *ally_anti_deadzones[which]);
> +}
> +
> +static ssize_t ally_anti_deadzone_store(struct device *dev, const char *buf,
> +					size_t count, enum ally_joystick which,
> +					const char *group)
> +{
> +	u8 value, dz[ARRAY_SIZE(ally_anti_deadzones)];
> +	struct hid_device *hdev = to_hid_device(dev);
> +	unsigned int i;
> +	int ret;
> +
> +	ret = kstrtou8(buf, 10, &value);
> +	if (ret || value > 100)
> +		return -EINVAL;
> +
> +	scoped_guard(mutex, &ally_prefs.lock) {
> +		if (!ally_prefs.anti_deadzone_support) {
> +			hid_dbg(hdev, "Anti-deadzone not supported on this device\n");
> +			return -EOPNOTSUPP;
> +		}
> +
> +		for (i = 0; i < ARRAY_SIZE(dz); i++)
> +			dz[i] = *ally_anti_deadzones[i];
> +		dz[which] = value;
> +
> +		ret = ally_set_anti_deadzone(hdev, dz[ALLY_LEFT_JOYSTICK],
> +					dz[ALLY_RIGHT_JOYSTICK]);
> +		if (ret)
> +			return ret;
> +
> +		*ally_anti_deadzones[which] = value;
> +	}
> +
> +	sysfs_notify(&dev->kobj, group, "anti_deadzone");
> +
> +	return count;
> +}

[ ... ]

> @@ -1602,7 +1748,7 @@ static int ally_prefs_setup(struct hid_device *hdev)
>  	}
>  
>  	/* Skip the calibration groups when the capability is missing. */
> -	if (ally_prefs.user_cal_support) {
> +	if (ally_prefs.user_cal_support || ally_prefs.anti_deadzone_support) {
>  		int cal_i;
>  
>  		for (cal_i = 0; cal_i < ARRAY_SIZE(ally_cal_attr_groups); cal_i++) {
> @@ -1643,8 +1789,7 @@ static void ally_sysfs_remove(struct hid_device *hdev)
>  	for (i = 0; i < ARRAY_SIZE(ally_attr_groups); i++)
>  		sysfs_remove_group(&hdev->dev.kobj, &ally_attr_groups[i]);
>  
> -	if (ally_prefs.user_cal_support || ally_prefs.anti_deadzone_support ||
> -	    ally_prefs.resp_curve_support) {
> +	if (ally_prefs.user_cal_support || ally_prefs.anti_deadzone_support) {
>  		for (i = 0; i < ARRAY_SIZE(ally_cal_attr_groups); i++)
>  			sysfs_remove_group(&hdev->dev.kobj, ally_cal_attr_groups[i]);
>  	}

[Severity: High]
Can this cause an ABBA deadlock between ally_prefs.lock and the kernfs active
reference when userspace accesses the anti_deadzone sysfs attribute
concurrently with device disconnect, unbind, or setup rollback?

In drivers/hid/hid-asus.c, when userspace reads or writes the anti_deadzone
sysfs attribute, kernfs_fop_read_iter() or kernfs_fop_write_iter() acquires
the kernfs active reference via kernfs_get_active() before invoking
ally_anti_deadzone_show() or ally_anti_deadzone_store(), both of which then
take ally_prefs.lock:

  kernfs_fop_read_iter() / kernfs_fop_write_iter()
    kernfs_get_active()               [acquires kn->active]
    ally_anti_deadzone_show() / ally_anti_deadzone_store()
      mutex_lock(&ally_prefs.lock)    [waits for ally_prefs.lock]

Concurrently, ally_sysfs_remove() (and the error rollback path in
ally_prefs_setup()) holds ally_prefs.lock via guard(mutex)(&ally_prefs.lock)
while calling sysfs_remove_group() on ally_cal_attr_groups:

  ally_sysfs_remove() / ally_prefs_setup()
    guard(mutex)(&ally_prefs.lock)    [holds ally_prefs.lock]
    sysfs_remove_group()
      __kernfs_remove()
        kernfs_drain()                [waits for kn->active to drain]

Wouldn't this lock inversion deadlock if a thread enters
ally_anti_deadzone_show() or ally_anti_deadzone_store() just as
ally_sysfs_remove() or ally_prefs_setup() removes the attribute groups under
ally_prefs.lock?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261031174130.208240-1-denis.benato@linux.dev?part=7