Re: [PATCH v3 1/2] drm/drm_crtc: ensure dma_fence_ops remain valid during device unbind

[Christian König]

On 7/21/26 13:20, Philipp Stanner wrote:
> On Tue, 2026-07-21 at 09:21 +0100, André Draszik wrote:
>> In [1], sashiko reported the following issue:
>>
>> === snip ===
>> Looking at how these fences are managed, drm_crtc_create_fence()
>> creates a dma_fence without taking a reference to the drm_device or
>> drm_crtc. Because the sync_file framework exposes this fence to
>> userspace, the fence can outlive the CRTC.
>>
>> The dma_fence contract requires that data accessed by dma_fence_ops
>> (like get_driver_name) must remain valid for an RCU grace period after
>> the fence is signaled. However, drm_crtc_cleanup() and the subsequent
>> freeing of the device do not wait for an RCU grace period via
>> synchronize_rcu().
>>
>> If userspace calls ioctl(SYNC_IOC_FILE_INFO) concurrently with a device
>> hot-unplug:
>>
>> CPU1 (Userspace)
>> sync_file_get_name()
>>   ops = rcu_dereference(fence->ops);
>>   if (!dma_fence_test_signaled_flag())
>>     // Preempted or delayed here
> 
> nit: no one will be preempted here since the RCU read lock must be
> held. The Sashiko tool misses the point, which is simply that someone
> illegally frees up stuff that might be still in use, with or without
> delay or preemption, that's all irrelevant for the issue.
> 
> Anyways, thanks for fixing this:

Seconded.

> 
>>
>>
> 
> […]
> 
>> Link: https://sashiko.dev/#/patchset/20260618-linux-drm_crtc_fix2-v1-1-c03e77b36f34@xxxxxxxxxx?part=1
>> Fixes: 6d6003c4b613 ("drm/fence: add fence timeline to drm_crtc")
>> Cc: stable@xxxxxxxxxxxxxxx
>> Signed-off-by: André Draszik <andre.draszik@xxxxxxxxxx>
> 
> Reviewed-by: Philipp Stanner <phasta@xxxxxxxxxx>

Reviewed-by: Christian König <christian.koenig@xxxxxxx> for both patches.

> 
>>
>> ---
>> v3:
>> - Philipp: update kerneldoc, add Fixes:
>>
>> v2: new patch
>> ---
>>  drivers/gpu/drm/drm_crtc.c | 15 ++++++++++++---
>>  1 file changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> index 63ead8ba6756..e8e80c936852 100644
>> --- a/drivers/gpu/drm/drm_crtc.c
>> +++ b/drivers/gpu/drm/drm_crtc.c
>> @@ -493,14 +493,23 @@ EXPORT_SYMBOL(__drmm_crtc_alloc_with_planes);
>>   * drm_crtc_cleanup - Clean up the core crtc usage
>>   * @crtc: CRTC to cleanup
>>   *
>> - * This function cleans up @crtc and removes it from the DRM mode setting
>> - * core. Note that the function does *not* free the crtc structure itself,
>> - * this is the responsibility of the caller.
>> + * This function cleans up @crtc and removes it from the DRM mode setting core,
>> + * after first waiting an RCU grace period to ensure @crtc->dev can safely be
>> + * dereferenced by our dma_fence_ops.
>> + *
>> + * Note that the function does *not* free the crtc structure itself, this is the
>> + * responsibility of the caller.
>>   */
>>  void drm_crtc_cleanup(struct drm_crtc *crtc)
>>  {
>>  	struct drm_device *dev = crtc->dev;
>>  
>> +	/* Ensure our dma_fence_ops remain valid for an RCU grace period after
>> +	 * the fence is signaled. This is necessary because our dma_fence_ops
>> +	 * dereference crtc->dev.
>> +	 */
>> +	synchronize_rcu();
>> +
>>  	/* Note that the crtc_list is considered to be static; should we
>>  	 * remove the drm_crtc at runtime we would have to decrement all
>>  	 * the indices on the drm_crtc after us in the crtc_list.