Re: [PATCH liburing] src/nolibc: Fix `malloc()` alignment

Jens Axboe
2021-10-11T13:00:20Z

On 10/11/21 6:56 AM, Ammar Faizi wrote:
> On Mon, Oct 11, 2021 at 7:13 PM Jens Axboe <axboe@kernel.dk> wrote:
>> On 10/11/21 12:49 AM, Ammar Faizi wrote:
>>> Add `__attribute__((__aligned__))` to the `user_p` to guarantee
>>> pointer returned by the `malloc()` is properly aligned for user.
>>>
>>> This attribute asks the compiler to align a type to the maximum
>>> useful alignment for the target machine we are compiling for,
>>> which is often, but by no means always, 8 or 16 bytes [1].
>>>
>>> Link: https://gcc.gnu.org/onlinedocs/gcc-11.2.0/gcc/Common-Variable-Attributes.html#Common-Variable-Attributes [1]
>>> Fixes: https://github.com/axboe/liburing/issues/454
>>> Reported-by: Louvian Lyndal <louvianlyndal@gmail.com>
>>> Signed-off-by: Ammar Faizi <ammar.faizi@students.amikom.ac.id>
>>> ---
>>> src/nolibc.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/src/nolibc.c b/src/nolibc.c
>>> index 5582ca0..251780b 100644
>>> --- a/src/nolibc.c
>>> +++ b/src/nolibc.c
>>> @@ -20,7 +20,7 @@ void *memset(void *s, int c, size_t n)
>>>
>>> struct uring_heap {
>>> size_t len;
>>> - char user_p[];
>>> + char user_p[] __attribute__((__aligned__));
>>> };
>>
>> This seems to over-align for me, at 16 bytes where 8 bytes would be fine.
>> What guarantees does malloc() give?
>>
>
> Section 7.20.3 of C99 states this about `malloc()`:
> __The pointer returned if the allocation succeeds is suitably aligned
> so that it may be assigned to a pointer to any type of object.__
>
> I have just browsed the glibc source code, malloc does give us 16 bytes
> alignment guarantee on x86-64.
>
> https://code.woboq.org/userspace/glibc/sysdeps/generic/malloc-alignment.h.html#_M/MALLOC_ALIGNMENT
>
> Lookie here on Linux x86-64...
>
> ```
> ammarfaizi2@integral:/tmp$ cat > test.c
> #include <stdio.h>
> int main(void)
> {
> printf("alignof = %zu\n", __alignof__(long double));
> return 0;
> }
> ammarfaizi2@integral:/tmp$ gcc -o test test.c
> ammarfaizi2@integral:/tmp$ ./test
> alignof = 16
> ammarfaizi2@integral:/tmp$
> ```
>
> We have `long double` which requires 16 byte alignment. So `malloc()`
> should cover this. Although we don't use floating point in liburing,
> it's probably better to have this guarantee as well?

Ah yes, good point. FWIW, I did apply your patch previously, for
alignment it's always better to error on the side of caution.

--
Jens Axboe