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

[Jens Axboe <axboe@kernel.dk>]

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