Re: [PATCH 2/2] drm/xe/i2c: Expose AMC Alert reason sysfs

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] NULL or ERR_PTR dereference in `xe_amc_exit` causing kernel panic.
- [Medium] Race condition (TOCTOU) between uevent emission and sysfs attribute creation.
- [Low] Spurious error message logged on successful sysfs file creation due to a missing return.

Pre-existing issues:
- [High] Use-After-Free (UAF) due to un-canceled asynchronous work during teardown.
--

--- Patch [2]: [PATCH 2/2] drm/xe/i2c: Expose AMC Alert reason sysfs ---
commit c923eec141de210a872cc999c40aa460aad20d6f
Author: Badal Nilawar <[email protected]>

drm/xe/i2c: Expose AMC Alert reason sysfs

AMC raises an SMBUS alert before performing a power removal or power-cycle
operation, and the driver places the device into vendor-specific wedge mode.
This commit exposes a read-only xe_amc_alert_reason sysfs attribute to help
users identify the required recovery action.

> diff --git a/drivers/gpu/drm/xe/xe_amc.c b/drivers/gpu/drm/xe/xe_amc.c
> index 8ecadee6eea39..ceb2c4d618fef 100644
> --- a/drivers/gpu/drm/xe/xe_amc.c
> +++ b/drivers/gpu/drm/xe/xe_amc.c
[ ... ]
> @@ -104,6 +107,54 @@ static const struct amc_request amc_get_alert_reason = {
[ ... ]
> +static void xe_create_amc_alert_sysfs(struct xe_device *xe)
> +{
> +	struct device *dev = xe->drm.dev;
> +	int ret;
> +
> +	if (xe->i2c->amc->sysfs_created)
> +		return;
> +
> +	ret = device_create_file(dev, &dev_attr_xe_amc_alert_reason);
> +	if (ret)
> +		goto failed;
> +
> +	xe->i2c->amc->sysfs_created = true;
> +failed:
> +	dev_err(dev, "Failed to create sysfs file for amc alert reason\n");
> +}

[Severity: Low]
Is this error message printed even when the sysfs file is successfully created?
It looks like there is a missing return statement before the failed label,
causing the code to fall through to dev_err() on success.

[ ... ]
> @@ -158,10 +209,16 @@ static void xe_amc_work(struct work_struct *work)
>  	case AMC_ALERT_THERMAL_TRIP:
>  	case AMC_ALERT_OOB_REQUEST:
>  	case AMC_ALERT_OOB_RESET:
> -	case AMC_ALERT_CATERR:
> +	case AMC_ALERT_CATERR: {
> +		struct xe_device *xe = i2c_client_to_xe_device(client);
> +
>  		dev_warn(amc->i2c->drm_dev, "AMC Alert: %s\n", amc_alert[alert_reason]);
> -		xe_device_declare_wedged(i2c_client_to_xe_device(client));
> +		amc->alert_reason = alert_reason;
> +		xe_device_set_wedged_method(xe, DRM_WEDGE_RECOVERY_VENDOR);
> +		xe_device_declare_wedged(xe);
> +		xe_create_amc_alert_sysfs(xe);

[Severity: Medium]
Could this create a race condition with userspace?

xe_device_declare_wedged() emits a KOBJ_CHANGE uevent. If userspace reacts
to the uevent instantly, it might check the sysfs directory before
xe_create_amc_alert_sysfs() completes, resulting in a -ENOENT error when
trying to read the alert reason.

>  		break;
> +	}
>  	default:
>  		dev_warn(amc->i2c->drm_dev, "unknown AMC alert: %d\n", alert_reason);
>  		break;
> @@ -190,8 +247,12 @@ int xe_amc_init(struct xe_i2c *i2c)
>  
>  void xe_amc_exit(struct xe_i2c *i2c)
>  {
> +	struct xe_device *xe;
> +
>  	if (i2c->amc) {
>  		cancel_work_sync(&i2c->amc->work);
> +		xe = i2c_client_to_xe_device(i2c->client[XE_I2C_CLIENT_AMC]);

[Severity: High]
Can i2c->client[XE_I2C_CLIENT_AMC] be NULL or an ERR_PTR here?

The I2C client is populated asynchronously in xe_i2c_client_work(). If
xe_amc_exit() is called before this work executes, or if the client creation
failed, i2c_client_to_xe_device() will unconditionally dereference an invalid
pointer when it accesses client->adapter.

> +		xe_remove_amc_alert_sysfs(xe);
>  		kfree(i2c->amc);
>  	}
>  }

[Severity: High]
This is a pre-existing issue, but looking at the driver teardown path in
drivers/gpu/drm/xe/xe_i2c.c:

static void xe_i2c_remove(void *data)
{
	struct xe_i2c *i2c = data;
	unsigned int i;

	xe_amc_exit(i2c);

	for (i = 0; i < XE_I2C_MAX_CLIENTS; i++) {
		i2c_unregister_device(i2c->client[i]);
		i2c->client[i] = NULL;
	}

	bus_unregister_notifier(&i2c_bus_type, &i2c->bus_notifier);
	xe_i2c_unregister_adapter(i2c);
}

Is there a missing cancel_work_sync(&i2c->work) here? 

The teardown path unregisters the notifier and xe_amc_exit() cancels amc->work,
but it appears to leave i2c->work unmanaged. If this asynchronous work executes
concurrently or after the devm-managed i2c structure is freed, could it lead
to a use-after-free?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2