Re: [PATCH] drm/bridge: aux-hpd-bridge: replay last HPD status on hpd_enable

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing cancellation of `hpd_work` on device unbind leads to access of a destroyed mutex and use-after-free.
- [High] Lockless read of `last_status` in `hpd_work` creates a TOCTOU race condition that can deliver stale HPD events out of order.
--

--- Patch [1]: [PATCH] drm/bridge: aux-hpd-bridge: replay last HPD status on hpd_enable ---
commit ac67055ec791a818497d8d0b82453f5ad00a28d3
Author: Yongxing Mou <[email protected]>

drm/bridge: aux-hpd-bridge: replay last HPD status on hpd_enable

This commit caches the last HPD status reported by an upstream driver
and replays it when a downstream consumer enables HPD handling on the
bridge. The replay is deferred to a work item to run outside the
.hpd_enable() call context.

> diff --git a/drivers/gpu/drm/bridge/aux-hpd-bridge.c b/drivers/gpu/drm/bridge/aux-hpd-bridge.c
> index a2e525aa57887..bb81aabf58d49 100644
> --- a/drivers/gpu/drm/bridge/aux-hpd-bridge.c
> +++ b/drivers/gpu/drm/bridge/aux-hpd-bridge.c
[ ... ]
> @@ -154,6 +166,8 @@ void drm_aux_hpd_bridge_notify(struct device *dev, enum drm_connector_status sta
>  	if (!data)
>  		return;
>  
> +	WRITE_ONCE(data->last_status, status);
> +
>  	drm_bridge_hpd_notify(&data->bridge, status);

[Severity: High]
Could this create a race condition with drm_aux_hpd_bridge_hpd_work() where
stale HPD events are delivered out of order?

If drm_aux_hpd_bridge_hpd_work() reads the status locklessly but gets
preempted before calling drm_bridge_hpd_notify(), a concurrent hardware
event here could update the status and dispatch it synchronously. 

When drm_aux_hpd_bridge_hpd_work() resumes, it might then dispatch the older, 
stale status it cached earlier, leaving the consumer with an outdated 
connection state.

>  }
>  EXPORT_SYMBOL_GPL(drm_aux_hpd_bridge_notify);
> @@ -165,11 +179,45 @@ static int drm_aux_hpd_bridge_attach(struct drm_bridge *bridge,
>  	return flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR ? 0 : -EINVAL;
>  }
>  
> +static void drm_aux_hpd_bridge_hpd_work(struct work_struct *work)
> +{
> +	struct drm_aux_hpd_bridge_data *data =
> +		container_of(work, struct drm_aux_hpd_bridge_data, hpd_work);
> +	enum drm_connector_status status = READ_ONCE(data->last_status);
> +
> +	if (status == connector_status_unknown)
> +		return;
> +
> +	drm_bridge_hpd_notify(&data->bridge, status);

[Severity: High]
Can this work item execute after the device is unbound?

If the auxiliary device unbinds while this work item is pending, devres
teardown will call drm_bridge_remove_void(), which in turn destroys
bridge->hpd_mutex and can free the data structure. 

If this work item executes after that teardown, wouldn't it result in a 
use-after-free on data and an attempt to lock a destroyed mutex inside
drm_bridge_hpd_notify()?

[ ... ]
> @@ -192,6 +240,9 @@ static int drm_aux_hpd_bridge_probe(struct auxiliary_device *auxdev,
>  	data->bridge.interlace_allowed = true;
>  	data->bridge.ycbcr_420_allowed = true;
>  
> +	data->last_status = connector_status_unknown;
> +	INIT_WORK(&data->hpd_work, drm_aux_hpd_bridge_hpd_work);
> +

[Severity: High]
Since INIT_WORK() is used here, should there be a corresponding devm cleanup
action to ensure the work is cancelled on unbind?

Adding something like devm_add_action_or_reset() to call cancel_work_sync() 
would ensure the work item doesn't outlive the data structure it operates on.

>  	auxiliary_set_drvdata(auxdev, data);
>  
>  	return devm_drm_bridge_add(data->dev, &data->bridge);

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