zephyr: Protect Zephyr heap metadata from corruption

[serhiy-katsyuba-intel]

Zephyr stores heap metadata just before each allocated chunk. So not only must the chunk start be aligned to cache line size, but the chunk size must also be aligned up to cache line size. This is to prevent metadata corruption in case an invalidate or writeback is called for a chunk -- it may corrupt metadata for the next chunk if it is located in the same cache line.

[serhiy-katsyuba-intel]

The cached allocation already had such size ALIGN_UP() done in heap_alloc_aligned_cached(). This PR adds same for non-cached allocation.

This fixes sporadic CI failures on Google AEC mock test_001_06_rtc_arc_dmic_driver_topo.

[tmleman]

As usual, a week of debugging and hours of analysis result in a one-line fix. Great job!

[kv2019i]

Thanks for catching this @serhiy-katsyuba-intel ! Some changes see inline, but the fix is solid.

FYI @jsarha @lyakh this explains Jyri why you saw issues with combining rmalloc()/rballoc(). rmalloc() had a bug so if you routed more allocs to it, you started to hit this bug (which is difficult to debug as you need a suitable alloc pattern so that the metadata gets corrupted). UPDATE: this doesn't explain, rballoc() was changed in commit cc24c9c and since that (in 2022) it has done the same split to cached/noncached behaviour (as rmalloc), so this doesn't relate to recent refactoring after all.

[kv2019i]

Wow @serhiy-katsyuba-intel , this is bad. I mean applying same rules was added in the initial implementation in commit 956a587 (2021). It seems rballoc() has been good, but the bug has been lurking in rmalloc(). Now when in recent memory refactoring work, there has been commits like commit b7a1616 that have changed old rballoc() usage with calls to framework that might now end up using rmalloc() instead, the bug (that was always in rmalloc()), is now hit. UPDATE: hmm, in main the logic is same in rballoc(), so this hypothesis w.r.t. rmalloc/rballoc does not hold.

[kv2019i]

@serhiy-katsyuba-intel Can you move this whole segment:

»       /*                                                                                                                                                                                   
»        * Zephyr sys_heap stores metadata at start of each                                                                                                                                  
»        * heap allocation. To ensure no allocated cached buffer                                                                                                                             
»        * overlaps the same cacheline with the metadata chunk,                                                                                                                              
»        * align both allocation start and size of allocation                                                                                                                                
»        * to cacheline. As cached and non-cached allocations are                                                                                                                            
»        * mixed, same rules need to be followed for both type of                                                                                                                            
»        * allocations.                                                                                                                                                                      
»        */
#ifdef CONFIG_SOF_ZEPHYR_HEAP_CACHED
»       min_align = MAX(PLATFORM_DCACHE_ALIGN, min_align);
»       by

.. and put it to heap_alloc_aligned(). No need to have this logic (and comment) twice in the file as these are both static functions. This also handles the logic what to do if CONFIG_SOF_ZEPHYR_HEAP_CACHED is not set correctly.

[lyakh]

This shouldn't be needed. heap_alloc_aligned_cached() already aligns both the size and the beginning of cached allocations, so cached and uncached allocations should already never cross cache-line boundaries. Since it fixes (or seems to fix) some failures, I'm reluctant to "ask for change" but it really doesn't seem correct to me.

[kv2019i]

Right @lyakh , so the comment (in code) is a bit misleading. The rule that metadata chunk must not live on the same cacheline does hold for all allocations. It is misleading in that we enforce this rule by aligning up the cached allocation sizes in heap_alloc_aligned_cached(). So if you make a 4 byte cached alloc, we turn it to a 64 byte alloc on a 64 byte boundary -> Zephyr heap cannot put any metadata on this cacheline as it is not free.

This does have the big caveat that if any user allocates noncached heap object, then converts it to cached usage on its own, bad things will happen (end of object allocation may have heap metadata for unrelated object and cache flush can corrupt this). This was one of the reasons for introducing the __sparse annotations in our interfaces to catch these cases.

@serhiy-katsyuba-intel does the above logic hold in the case you are observing with the RTC test case?

[serhiy-katsyuba-intel]

This does have the big caveat that if any user allocates noncached heap object, then converts it to cached usage on its own, bad things will happen (end of object allocation may have heap metadata for unrelated object and cache flush can corrupt this). This was one of the reasons for introducing the __sparse annotations in our interfaces to catch these cases.

What happen if someone allocate noncached mem and without converting the address to cached call invalidate/writeback? I'm not sure if we have checks in invalidate and writeback to check if address belongs to cached space and I'm not sure if actually invalidate/writeback to memory will happen. Such bug could easily be introduced by a mistake in refactoring.

If fact I found one such bug introduced by this commit d519e7e at line 362. ring_buffer->_data_buffer previously was always cached, after that commit it became noncached when is_shared==true (as a mistake during refactoring). All invalidates/writeback are still in code for ring_buffer->_data_buffer.

Just in case, fixing ring_buffer->_data_buffer allocation does not help AEC CI problem. However, similar bugs could (and will :) ) be easily introduced as a mistake at refactoring. Protecting heat metadata from corruption might be an OK workaround to have. Especially as such bugs could trigger randomly then something unrelated just added to code.

[kv2019i]

@serhiy-katsyuba-intel wrote:

What happen if someone allocate noncached mem and without converting the address to cached call invalidate/writeback? I

I was checking this today and this should be safe. The cache line lookup is based on the physical addresses, and the uncached addresses will just never have dirty lines in the cache. It's still wasted DSP cycles to do the invalidate/wb, but should not corrupt the metadata. The fact that your test to fix ring_buffer->_data_buffer allocation, did not fix the AEC CI problem, is at least compatible with this analysis.

So it's still a bit of a mystery why this PR helps with the AEC case. I surveyed the codebase again and we have limited the uncache-to-cache conversions very aggressively, so these are only done in a very few places.

Oh well, this PR is for sure safer as it protects against rogue cache ops.

[serhiy-katsyuba-intel]

Pushed new version with size alignment moved inside heap_alloc_aligned() and updated commit description.

[lyakh]

I don't think we need to also align the size to the same value. If the user requests a page-aligned buffer, only the beginning should be page-aligned. The size can stay cache-line size aligned. Yes, usually if you request a page-aligned buffer the size is also a multiple of the page size, but for correctness / cache management we only need cache-line size alignment

[serhiy-katsyuba-intel]

Agree. This was a copy-paste from the cached allocation implementation. Anyway, pushed a new version with size aligned to cache line.

[kv2019i]

Thanks. So in the end, this was a smaller issue. But seems the consensus is to take the more conservative approach, to protect against such issues in the future. I'm ok with this approach as well. We may now revisit the discussions to optimize the heap w.r.t. SRAM usage.