chain-dma: fix NULL dereference#11016
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens the Zephyr low-latency scheduler against a NULL (or otherwise invalid) task->priv_data during task completion, preventing a crash when a chain-dma stream is terminated after schedule_task_free() has been triggered from chain_task_pause().
Changes:
- Guard
zephyr_ll_task_done()againsttask->priv_data == NULLbefore checkingpdata->freeing. - Refresh
pdata = task->priv_dataafter re-acquiring the scheduler lock inzephyr_ll_run()and treatNULLpdata as a condition to complete the task.
| if (pdata->freeing || state == SOF_TASK_STATE_COMPLETED) { | ||
| if (!pdata || pdata->freeing || state == SOF_TASK_STATE_COMPLETED) { | ||
| zephyr_ll_task_done(sch, task); |
There was a problem hiding this comment.
Pointer pdata is dereferenced at line 267, so the compiler has the right to assume that it cannot be NULL here and will optimize out this check.
And why are you assigning it a second time?
There was a problem hiding this comment.
@tmleman you're right. I first thought that since it only crashes after the call to do_task_run() so it must be happening there, that .pdata gets assigned NULL by the chain-dma component. But then I realised, that that assignment only happens in IPC context and the IPC thread priority is lower than the LL thread, so it cannot preempt it even with the lock dropped. So, I checked and indeed .pdata == NULL already before do_task_run() and it doesn't crash before because of our everybody's favourite #6093 . I've updated the commit.
Fix a NULL dereference when terminating a chain-dma stream after schedule_task_free() has been called from chain_task_pause(). Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
|
@lyakh can you check CI, could be a false positive. Thanks |
ujfalusi
left a comment
There was a problem hiding this comment.
It would be better to understand what is the issue before hiding it?
| task->state = SOF_TASK_STATE_FREE; | ||
|
|
||
| if (pdata->freeing) | ||
| if (pdata && pdata->freeing) |
There was a problem hiding this comment.
The real issue is with the 48cdc4d + CONFIG_USERSPACE=y
this combination causes the ChainDMA crash on PTL and only on PTL as CONFIG_USERSPACE=y is only set for PTL, all other platform has USERSPACE disabled.
it boils down to some difference due to the use of kernel/sys/sem.c vs kernel/userspace/sem.c for semaphore.
Reverting the commit or disabling USERSPACE fixed the crash on PTL.
Fix a NULL dereference when terminating a chain-dma stream after schedule_task_free() has been called from chain_task_pause().
If somebody can fix chain DMA instead to not leave the task running after it's freed, that might be a preferred solution