modules: Apparently asking 0 alignment is dangerous, use 4 as default by jsarha · Pull Request #10324 · thesofproject/sof

jsarha · 2025-10-23T10:45:26Z

The current rballoc_align() implementation has this line:
assert(IS_ALIGNED(mem, align));

Which will cause division by zero due to IS_ALIGNED() Zephyr implementation:
define IS_ALIGNED(ptr, align) (((uintptr_t)(ptr)) % (align) == 0)

So better use 4 as the default value.

Copilot

Pull Request Overview

This PR fixes a potential division by zero error in memory allocation functions by changing the default alignment parameter from 0 to 4. The issue occurs because the underlying rballoc_align() implementation uses Zephyr's IS_ALIGNED() macro, which performs a modulo operation that would fail with a zero alignment value.

Key Changes:

Updated mod_balloc() to pass alignment of 4 instead of 0
Updated mod_alloc() to pass alignment of 4 instead of 0

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

src/include/sof/audio/module_adapter/module/generic.h

The current rballoc_align() implementation has this line: assert(IS_ALIGNED(mem, align)); Which will cause division by zero due to IS_ALIGNED() Zephyr implementation: define IS_ALIGNED(ptr, align) (((uintptr_t)(ptr)) % (align) == 0) So better use sizeof(void *) as the default value. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>

softwarecki

Using 0 as an alignment value might intuitively suggest "no alignment" or "not meaningful", but it could also be interpreted as an invalid value.
Any valid alignment can be expressed using 1 instead.
Defaulting to pointer-sized alignment seems like a reasonable and safe choice.

abonislawski

Thanks, much better than modified macro

kv2019i

CI failure seen, otherwise ready to go.

kv2019i · 2025-10-24T11:22:22Z

@lrudyX can you check quickbuild? Both me and Jyri checked the logs and doesn't seem related to this PR. There are no failures in FW log, but TestBasicHdaLinkRandomDma test is reported as timed out. The IPC log doesn't show any IPC timing out, so not sure what goes wrong. FYI @jsarha

lrudyX · 2025-10-24T11:43:17Z

@lrudyX can you check quickbuild? Both me and Jyri checked the logs and doesn't seem related to this PR. There are no failures in FW log, but TestBasicHdaLinkRandomDma test is reported as timed out. The IPC log doesn't show any IPC timing out, so not sure what goes wrong. FYI @jsarha

No, this is sporadic problem seen in many others PR

lgirdwood · 2025-10-24T12:14:13Z

@lrudyX can you check quickbuild? Both me and Jyri checked the logs and doesn't seem related to this PR. There are no failures in FW log, but TestBasicHdaLinkRandomDma test is reported as timed out. The IPC log doesn't show any IPC timing out, so not sure what goes wrong. FYI @jsarha

No, this is sporadic problem seen in many others PR

Can we rerun @lrudyX ? thanks.

lyakh

Fixing it at the actual failure location could be a better option

lyakh · 2025-10-27T11:41:42Z

src/include/sof/audio/module_adapter/module/generic.h

 {
-	return mod_balloc_align(mod, size, 0);
+	/* IS_ALIGNED() crashes if alignment is 0, so use pointer size as default alignment */
+	return mod_balloc_align(mod, size, sizeof(void *));


is this because of

sof/zephyr/lib/alloc.c

Line 296 in a86f371

assert(IS_ALIGNED(mem, align));

? Should we fix it there maybe?

Possibly both places. But defaulting to ptr size alignment is a sane thing to do even if there was no assert there.

Possibly both places. But defaulting to ptr size alignment is a sane thing to do even if there was no assert there.

@jsarha but why here? This is way too high a level imho. This is just a module allocation wrapper. In principle - yes, I'd agree, that allocating with less than 4 bytes alignment isn't very meaningful, and I actually suspect this is already the case with Zephyr heap allocator. So, doesn't seem to make much sense to me here.

@lyakh, I do not want to have parallel mod_alloc implementations for specified alignment and unspecified alignment, so I need to implement mod_alloc_align() and call it from mod_alloc(). In that call I have to specify the default alignment and sizeof(void *) is a sane default.

There is still a strong argument to fix the rballoc_align() implementation so that it would not crash with zero alignment argument, or even fixing the Zephyr IS_ALIGNED() macro so that it would not crash if align argument is zero. But those valid fixes do not make this change invalid.

@jsarha sorry, I still don't find this useful. Forcing a sizeof(void *) alignment in just one of high level allocation APIs seems too random to me, while we have a (perceived) bug in a specific place that should be fixed. And fixing it in two locations appears redundant to me too.

@lyakh , Ok, would you then let default alignment of 1 to pass? I feel that alignment of 1 would really be an odd choice, but not as odd as 0. Or should I just close this PR? Its not like any of my PR are moving any more.

Lets fix this in this PR, alignment "0" is simply wrong

@jsarha @abonislawski I think it's rather common to use 0 to mean "no explicit alignment," e.g. https://github.com/zephyrproject-rtos/zephyr/blob/a0096b719c22e1af818f56f6ae0657c1635c3cf0/kernel/kheap.c#L136 or

sof/zephyr/lib/alloc.c

Line 489 in a86f371

return rmalloc_align(flags, bytes, 0);

Upon checking it looks like alignment 0 is present in some zephyr docs, in that case our assert call for IS_ALIGNED is too general and needs an additional condition

lrudyX · 2025-10-27T12:04:12Z

@lrudyX can you check quickbuild? Both me and Jyri checked the logs and doesn't seem related to this PR. There are no failures in FW log, but TestBasicHdaLinkRandomDma test is reported as timed out. The IPC log doesn't show any IPC timing out, so not sure what goes wrong. FYI @jsarha

No, this is sporadic problem seen in many others PR

Can we rerun @lrudyX ? thanks.
We have DUT problem at the moment, I will rerun as soon as possible.

jsarha · 2025-10-31T10:23:25Z

Ok, here is an alternative solution. We need one or the other, preferrably soon. So please agree on either one (I would still vote for both).
#10345

Copilot AI review requested due to automatic review settings October 23, 2025 10:45

jsarha requested review from dbaluta, kv2019i, lbetlej, lgirdwood, mmaka1 and plbossart as code owners October 23, 2025 10:45

Copilot AI reviewed Oct 23, 2025

View reviewed changes

src/include/sof/audio/module_adapter/module/generic.h Outdated Show resolved Hide resolved

src/include/sof/audio/module_adapter/module/generic.h Outdated Show resolved Hide resolved

singalsu approved these changes Oct 23, 2025

View reviewed changes

jsarha force-pushed the module_api_no_0_alignment branch from d8f93c0 to b5ddf37 Compare October 23, 2025 14:44

jsarha force-pushed the module_api_no_0_alignment branch from b5ddf37 to 17e871c Compare October 23, 2025 14:47

jsarha mentioned this pull request Oct 23, 2025

Module api conversion of TDFB, template, tensorflow, and up down mixer modules. #10296

Merged

softwarecki approved these changes Oct 23, 2025

View reviewed changes

abonislawski approved these changes Oct 24, 2025

View reviewed changes

kv2019i approved these changes Oct 24, 2025

View reviewed changes

lgirdwood approved these changes Oct 24, 2025

View reviewed changes

lyakh requested changes Oct 27, 2025

View reviewed changes

tmleman approved these changes Oct 27, 2025

View reviewed changes

kv2019i mentioned this pull request Oct 31, 2025

zephyr: lib/alloc: Relax virtual_heap_alloc() assert condition #10345

Merged

Conversation

jsarha commented Oct 23, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

softwarecki left a comment

Choose a reason for hiding this comment

Uh oh!

abonislawski left a comment

Choose a reason for hiding this comment

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

kv2019i commented Oct 24, 2025

Uh oh!

lrudyX commented Oct 24, 2025

Uh oh!

lgirdwood commented Oct 24, 2025

Uh oh!

lyakh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsarha Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lrudyX commented Oct 27, 2025

Uh oh!

jsarha commented Oct 31, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

jsarha Oct 27, 2025 •

edited

Loading