Repository navigation
buffer: prevent cache corruption - #7808
Conversation
Recent research discovered a set of potential issues related with cache prefetch. Specifically it seems like uncached access to memory can cause cache prefetch. This can cause problems in buffer_attach() and buffer_detach() where buffers are added to or removed from lists respectively via uncached addresses, after which they can be used via cached addresses. Add proper cache synchronisation and interrupt locking to protect against such memory corruption. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
|
"Recent research discovered a set of potential issues related with cache prefetch. Specifically it seems like uncached access to memory can cause cache prefetch. " is this reasonable? if access memory in uncached, then dsp should perform cache prefetch, right? |
|
|
I would suggest wait for some time for #7191 , if still reproducible, we can ask keqiao help for reproduce with this PR. |
| goto free; | ||
| } | ||
|
|
||
| irq_local_disable(flags); |
There was a problem hiding this comment.
when turn off irq, means some or one of irq have negative impact with buffer_attach.
this part of change can make buffer_attach more secure, just not sure it is must or not.
There was a problem hiding this comment.
this is needed because we're processing an IPC here and we might get interrupted by the scheduler, already running a pipeline to which we're attaching an additional source or sink.
There was a problem hiding this comment.
This does feel out-of-place in module_adapter.c level code. Given both IPC and pipeline are run in regular threads, we should be able to use plain mutex'es for exclusive access here. This would make it also userspace compatible for future-proofing this code.
But maybe beyond this PR, especially given pipeline_connect() already does the IRQ enable/disable dance, so the same should be done here to be safe.
There was a problem hiding this comment.
This does feel out-of-place in module_adapter.c level code. Given both IPC and pipeline are run in regular threads, we should be able to use plain mutex'es for exclusive access here. This would make it also userspace compatible for future-proofing this code.
@kv2019i maybe mutexes would work too, but would take context switching:
- binding of an additional pipeline is taking place
- a timer interrupt arrives
- the copying is scheduled
- since the LL scheduler has a higher priority than the IPC thread we switch to copying
- we try to take the mutex but it's held by the IPC thread
- the copying thread is suspended and the IPC thread is scheduled (priority inversion)
- the IPC thread finishes binding and releases the mutex
- we switch back to the copying thread
So, this should work, but would involve a bunch of switching. If the protected area is small enough, locking interrupts should be simple enough. But if we shouldn't use priority calls, then we can use a mutex
There was a problem hiding this comment.
@lyakh Ack. Let's revisit this when we introduce user-space, this is anyways not the only place where we have explicit irq control in the SOF audio code.
kv2019i
left a comment
kv2019i
left a comment
There was a problem hiding this comment.
Sorry, took a long time to review, but this is a complex piece of code to review. I'm leaning towards approving as this does problems in current codebase. See more comments inline.
| goto free; | ||
| } | ||
|
|
||
| irq_local_disable(flags); |
There was a problem hiding this comment.
This does feel out-of-place in module_adapter.c level code. Given both IPC and pipeline are run in regular threads, we should be able to use plain mutex'es for exclusive access here. This would make it also userspace compatible for future-proofing this code.
But maybe beyond this PR, especially given pipeline_connect() already does the IRQ enable/disable dance, so the same should be done here to be safe.
Recent research discovered a set of potential issues related with cache prefetch. Specifically it seems like uncached access to memory can cause cache prefetch. This can cause problems in buffer_attach() and buffer_detach() where buffers are added to or removed from lists respectively via uncached addresses, after which they can be used via cached addresses. Add proper cache synchronisation and interrupt locking to protect against such memory corruption.