Re: [PATCH] drm/xe: Fix NULL pointer dereference in xe_exec_ioctl

["Tapani Pälli" <tapani.palli@intel.com>]

On 12/17/25 13:51, Matthew Auld wrote:
> On 17/12/2025 10:51, Tapani Pälli wrote:
>>
>> On 12/17/25 12:31, Matthew Auld wrote:
>>> On 17/12/2025 10:27, Matthew Auld wrote:
>>>> On 17/12/2025 06:17, Tapani Pälli wrote:
>>>>> Helper function xe_sync_needs_wait expects sync->fence when accessing
>>>>> flags, patch makes sure we call only when sync->fence exists.
>>>>>
>>>>> Fixes NULL pointer dereference seen with Vulkan workloads:
>>>>>
>>>>> [  118.410401] RIP: 0010:xe_sync_needs_wait+0x27/0x50 [xe]
>>>>>
>>>>> Fixes: 4ac9048d0501 ("drm/xe: Wait on in-syncs when swicthing to 
>>>>> dma- fence mode")
>>>>> Signed-off-by: Tapani Pälli <tapani.palli@intel.com>
>>>>> ---
>>>>>   drivers/gpu/drm/xe/xe_exec.c | 3 ++-
>>>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/xe/xe_exec.c b/drivers/gpu/drm/xe/ 
>>>>> xe_exec.c
>>>>> index 730a5c9c2637..ea368f02cb9f 100644
>>>>> --- a/drivers/gpu/drm/xe/xe_exec.c
>>>>> +++ b/drivers/gpu/drm/xe/xe_exec.c
>>>>> @@ -184,7 +184,8 @@ int xe_exec_ioctl(struct drm_device *dev, void 
>>>>> *data, struct drm_file *file)
>>>>>           if (xe_sync_is_ufence(&syncs[num_syncs]))
>>>>>               num_ufence++;
>>>>> -        if (!num_in_sync && xe_sync_needs_wait(&syncs[num_syncs]))
>>>>> +        if (!num_in_sync && syncs[num_syncs].fence &&
>>>>> +            xe_sync_needs_wait(&syncs[num_syncs]))
>>>>
>>>> In xe_sync_entry_parse() it looks like it will always populate the 
>>>> fence for the !signal case, otherwise throwing an error if that is 
>>>> not possible. And xe_sync_needs_wait() will only touch the fence if 
>>>> it's the signal case? So it seems like this should not be possible?
>>>
>>> Sorry meant to type:
>>>
>>> s/touch the fence if it's the signal/touch the fence if it's the 
>>> !signal/
>>>
>> I'm not sure of the complete flow but it is quite easy to reproduce.
>
> Oh, I think I see it now.
>
> I assume the below also fixes it? There is also the case in 
> xe_sync_entry_wait() which has the same bug, I think, so would also 
> need to handle that.
>
> @@ -178,6 +179,9 @@ int xe_sync_entry_parse(struct xe_device *xe, 
> struct xe_file *xef,
>
> sync_in.timeline_value);
>                         if (err)
>                                 return err;
> +
> +                       if (!sync->fence)
> +                               sync->fence = dma_fence_get_stub();
>
I will try this one.


> So it looks like dma_fence_chain_find_seqno() can return NULL if the 
> seqno has already signalled. Also means we are missing some IGT 
> coverage for this.
>
> Can you resend your patch but perhaps we move the check into 
> needs_wait() and then add a similar check in xe_sync_entry_wait() 
> also? Or perhaps might be cleaner to have xe_sync_entry_wait() first 
> check needs_wait(), which will now also check for a NULL fence?
>
Yes, that sounds fine to me, I'll add null check to needs_wait().


>>
>> Here is the stacktrace snippet:
>>
>> [  118.410401] RIP: 0010:xe_sync_needs_wait+0x27/0x50 [xe]
>> [  118.410940] Code: 90 90 90 0f 1f 44 00 00 55 48 89 f1 48 89 e5 48 
>> 83 ec 08 48 83 7e 08 00 0f 84 79 be 1c 00 31 c0 f6 41 4c 01 75 11 48 
>> 8b 41 08 <48> 8b 40 30 48 d1 e8 83 e0 01 83 f0 01 c9 31 d2 31 c9 31 
>> f6 31 ff
>> [  118.410949] RSP: 0018:ffffccec8ea7bb18 EFLAGS: 00010246
>> [  118.410957] RAX: 0000000000000000 RBX: 00007ff23812f460 RCX: 
>> ffff8c1790db4800
>> [  118.410964] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 
>> 0000000000000000
>> [  118.410968] RBP: ffffccec8ea7bb20 R08: 0000000000000000 R09: 
>> 0000000000000000
>> [  118.410973] R10: 0000000000000000 R11: 0000000000000000 R12: 
>> ffff8c1790db4800
>> [  118.410978] R13: 0000000000000000 R14: ffff8c178b67e000 R15: 
>> 0000000000000000
>> [  118.410984] FS:  0000000101aff6c0(0000) GS:ffff8c27350d1000(0000) 
>> knlGS:000000007fe20000
>> [  118.410992] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> [  118.410998] CR2: 0000000000000030 CR3: 00000001037ef005 CR4: 
>> 0000000000772ef0
>> [  118.411004] PKRU: 55555554
>> [  118.411008] Call Trace:
>> [  118.411013]  <TASK>
>> [  118.411022]  xe_exec_ioctl+0x375/0xea0 [xe]
>> [  118.411377]  ? __x64_sys_ioctl+0xbd/0x100
>> [  118.411392]  ? do_syscall_64+0xa7/0x580
>> [  118.411403]  ? dma_fence_free+0x1a/0x30
>> [  118.411419]  ? __pfx_xe_exec_fn+0x10/0x10 [xe]
>> [  118.411752]  ? drm_syncobj_array_free+0x56/0x80 [drm]
>> [  118.411915]  ? drm_syncobj_query_ioctl+0x20f/0x460 [drm]
>> [  118.412035]  ? __pfx_xe_exec_ioctl+0x10/0x10 [xe]
>> [  118.412372]  drm_ioctl_kernel+0xae/0x110 [drm]
>> [  118.412524]  drm_ioctl+0x2ee/0x5d0 [drm]
>> [  118.412649]  ? __pfx_xe_exec_ioctl+0x10/0x10 [xe]
>> [  118.412989]  ? __pm_runtime_resume+0x5f/0x90
>> [  118.413002]  xe_drm_ioctl+0x61/0xb0 [xe]
>> [  118.413329]  __x64_sys_ioctl+0xa3/0x100
>> [  118.413338]  x64_sys_call+0x1060/0x2360
>> [  118.413347]  do_syscall_64+0x74/0x580
>> [  118.413355]  ? x64_sys_call+0x1060/0x2360
>> [  118.413360]  ? do_syscall_64+0xa7/0x580
>> [  118.413368]  ? __do_sys_getpid+0x1d/0x30
>> [  118.413377]  ? x64_sys_call+0xf9d/0x2360
>> [  118.413383]  ? do_syscall_64+0xa7/0x580
>> [  118.413389]  ? x64_sys_call+0x1060/0x2360
>> [  118.413395]  ? do_syscall_64+0xa7/0x580
>> [  118.413401]  ? sysvec_apic_timer_interrupt+0x54/0xd0
>> [  118.413410]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
>> [  118.413418] RIP: 0033:0x7ff29c93287d
>>
>>
>>
>>>>
>>>>>               num_in_sync++;
>>>>>       }
>>>>
>>>
>