Re: [PATCH v12 06/17] HID: asus: add triggers inner and outer range configuration

[sashiko-bot@kernel.org]

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

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

HID: asus: add triggers inner and outer range configuration

This commit adds sysfs attributes to configure the inner and outer trigger
ranges for ROG Ally devices, along with restoring those trigger ranges on
device initialization.

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

[ ... ]

> @@ -1342,9 +1351,201 @@ static const struct attribute_group ally_right_joystick_axis_group = {
>  	.attrs = right_joystick_axis_attrs,
>  };
>  

[ ... ]

> +static ssize_t ally_trigger_limit_show(char *buf, enum ally_trigger_limit which)
> +{
> +	guard(mutex)(&ally_prefs.lock);
> + 
> +	if (!ally_prefs.user_cal_support)
> +		return -EOPNOTSUPP;
> + 
> +	return sysfs_emit(buf, "%u\n", *ally_trigger_limits[which]);
> +}
> + 
> +static ssize_t ally_trigger_limit_store(struct device *dev, const char *buf,
> +					size_t count,
> +					enum ally_trigger_limit which,
> +					int lo, int hi,
> +					const char *group, const char *name)
> +{
> +	u8 value, v[ARRAY_SIZE(ally_trigger_limits)];
> +	struct hid_device *hdev = to_hid_device(dev);
> +	size_t i;
> +	int ret;
> + 
> +	ret = kstrtou8(buf, 10, &value);
> +	if (ret || value < lo || value > hi)
> +		return -EINVAL;
> + 
> +	scoped_guard(mutex, &ally_prefs.lock) {
> +		if (!ally_prefs.user_cal_support)
> +			return -EOPNOTSUPP;
> + 
> +		for (i = 0; i < ARRAY_SIZE(v); i++)
> +			v[i] = *ally_trigger_limits[i];
> +		v[which] = value;
> + 
> +		ret = ally_set_trigger_ranges(hdev, v[ALLY_LEFT_MIN], v[ALLY_LEFT_MAX],
> +					      v[ALLY_RIGHT_MIN], v[ALLY_RIGHT_MAX]);
> +		if (ret)
> +			return ret;
> + 
> +		*ally_trigger_limits[which] = value;
> +	}
> + 
> +	sysfs_notify(&dev->kobj, group, name);
> + 
> +	return count;
> +}

[ ... ]

> +static const struct attribute_group ally_left_trigger_group = {
> +	.name = "left_trigger",
> +	.attrs = left_trigger_attrs,
> +};
> +
> +static const struct attribute_group ally_right_trigger_group = {
> +	.name = "right_trigger",
> +	.attrs = right_trigger_attrs,
> +};
>  static const struct attribute_group *const ally_cal_attr_groups[] = {
>  	&ally_left_joystick_axis_group,
>  	&ally_right_joystick_axis_group,
> +	&ally_left_trigger_group,
> +	&ally_right_trigger_group,
>  };

[Severity: High]
Can registering ally_left_trigger_group and ally_right_trigger_group in
ally_cal_attr_groups[] cause an AB-BA deadlock between ally_prefs.lock and
the kernfs active references on the trigger limit sysfs attributes?

When userspace reads or writes range_lower_limit or range_upper_limit,
kernfs_fop_read_iter() or kernfs_fop_write_iter() acquires the attribute's
active reference (kn->active) before calling ally_trigger_limit_show() or
ally_trigger_limit_store(), which then acquires ally_prefs.lock:

  CPU 0:
  kernfs_fop_read_iter() / kernfs_fop_write_iter()
    kernfs_get_active()               [holds kn->active]
    ally_trigger_limit_show() / ally_trigger_limit_store()
      mutex_lock(&ally_prefs.lock)

Meanwhile, during device removal in ally_sysfs_remove() (or error rollback
in ally_prefs_setup()), ally_prefs.lock is acquired before calling
sysfs_remove_group() on ally_cal_attr_groups[]:

  drivers/hid/hid-asus.c:ally_sysfs_remove() {
      ...
      guard(mutex)(&ally_prefs.lock);
      ...
      if (ally_prefs.user_cal_support || ally_prefs.anti_deadzone_support ||
          ally_prefs.resp_curve_support) {
          for (i = 0; i < ARRAY_SIZE(ally_cal_attr_groups); i++)
              sysfs_remove_group(&hdev->dev.kobj, ally_cal_attr_groups[i]);
      }
  }

sysfs_remove_group() calls __kernfs_remove() -> kernfs_drain(), which waits
for kn->active to drain while holding ally_prefs.lock:

  CPU 1:
  ally_sysfs_remove() / ally_prefs_setup()
    mutex_lock(&ally_prefs.lock)
    sysfs_remove_group()
      __kernfs_remove()
        kernfs_drain()                [waits for kn->active]

If a sysfs read or write on the new trigger attributes runs concurrently
with device disconnect, unbind, or setup rollback, wouldn't CPU 0 and CPU 1
deadlock waiting on each other?

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