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

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

[Ammar Faizi]

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__));
 };
 
 void *malloc(size_t len)
-- 
2.30.2

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

[Jens Axboe]

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?

-- 
Jens Axboe

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

[Ammar Faizi]

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?

What do you think?
Will we ever need this?


Currently we only use it for probe ring func, so 8 bytes is fine:
```
struct io_uring_probe *io_uring_get_probe_ring(struct io_uring *ring)
{
	struct io_uring_probe *probe;
	size_t len;
	int r;

	len = sizeof(*probe) + 256 * sizeof(struct io_uring_probe_op);
	probe = malloc(len);
	if (!probe)
		return NULL;
	memset(probe, 0, len);

	r = io_uring_register_probe(ring, probe, 256);
	if (r >= 0)
		return probe;

	free(probe);
	return NULL;
}
```

I will send v2 to make it 8 byte aligned if you think it better to do
so.

-- 
Ammar Faizi

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

[Jens Axboe]

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