void* can cause issues for alignment now that its size depends on whether it is compat or purecap. In struct __kernel_sockaddr_storage, it is not annotated with __user, so it is affected. Change it to an unsigned long. Musl uses the same type for its struct definitions.
Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com --- include/uapi/linux/socket.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/uapi/linux/socket.h b/include/uapi/linux/socket.h index 51d6bb2f6765..28ff6a4aebca 100644 --- a/include/uapi/linux/socket.h +++ b/include/uapi/linux/socket.h @@ -22,7 +22,7 @@ struct __kernel_sockaddr_storage { /* space to achieve desired size, */ /* _SS_MAXSIZE value minus size of ss_family */ }; - void *__align; /* implementation specific desired alignment */ + unsigned long __align; /* implementation specific desired alignment */ }; };
base-commit: bbd29c87073f43eb8c09e8a2c2f4508b46a831a0
On 12/12/2022 18:07, Teo Couprie Diaz wrote:
void* can cause issues for alignment now that its size depends on whether it is compat or purecap. In struct __kernel_sockaddr_storage, it is not annotated with __user, so it is affected.
To be more specific, the issue is not that its size depends on the ABI (that's the intention in fact), but rather that it is a trick to get 32-bit alignment on a 32-bit arch and 64-bit on a 64-bit arch. So far void * fitted the bill, but purecap broke that.
Change it to an unsigned long. Musl uses the same type for its struct definitions.
I've just looked at the definition of struct sockaddr_storage in Musl and it is not in fact using this trick (probably because compat is not a concern there), the __align unsigned long at the end is not actually aligning anything (it's a struct, not a union). Or am I missing something?
Kevin
Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com
include/uapi/linux/socket.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/uapi/linux/socket.h b/include/uapi/linux/socket.h index 51d6bb2f6765..28ff6a4aebca 100644 --- a/include/uapi/linux/socket.h +++ b/include/uapi/linux/socket.h @@ -22,7 +22,7 @@ struct __kernel_sockaddr_storage { /* space to achieve desired size, */ /* _SS_MAXSIZE value minus size of ss_family */ };
void *__align; /* implementation specific desired alignment */
unsigned long __align; /* implementation specific desired alignment */};
base-commit: bbd29c87073f43eb8c09e8a2c2f4508b46a831a0
On 13/12/2022 09:45, Kevin Brodsky wrote:
Change it to an unsigned long. Musl uses the same type for its struct definitions.
I've just looked at the definition of struct sockaddr_storage in Musl and it is not in fact using this trick (probably because compat is not a concern there), the __align unsigned long at the end is not actually aligning anything (it's a struct, not a union). Or am I missing something?
I'm not sure what you mean. Without the __ss_align unsigned long, the alignment of the whole struct would be 2. With it, the alignment is 8 (or 4 on 32-bit). I thought that whether it is a struct or union doesn't matter, the same rules apply (I think the kernel just uses a union to make it clear that the size of __data is 126, to not factor in the size of __align). Am I missing something?
Thanks, Kristina
On 13/12/2022 13:30, Kristina Martsenko wrote:
On 13/12/2022 09:45, Kevin Brodsky wrote:
Change it to an unsigned long. Musl uses the same type for its struct definitions.
I've just looked at the definition of struct sockaddr_storage in Musl and it is not in fact using this trick (probably because compat is not a concern there), the __align unsigned long at the end is not actually aligning anything (it's a struct, not a union). Or am I missing something?
I'm not sure what you mean. Without the __ss_align unsigned long, the alignment of the whole struct would be 2. With it, the alignment is 8 (or 4 on 32-bit). I thought that whether it is a struct or union doesn't matter, the same rules apply (I think the kernel just uses a union to make it clear that the size of __data is 126, to not factor in the size of __align). Am I missing something?
I think I see it now, __ss_align has the desired alignment and because of the layout of the struct, you always end up with the start of the struct having the same alignment as __ss_align (the other members having a smaller alignment requirement than __ss_align). Really confusing though, using a union like in the uapi header is a lot clearer. Either way, I agree this is equivalent in terms of type usage, no need to change that paragraph and sorry for the noise!
Kevin
On 13/12/2022 12:55, Kevin Brodsky wrote:
On 13/12/2022 13:30, Kristina Martsenko wrote:
On 13/12/2022 09:45, Kevin Brodsky wrote:
Change it to an unsigned long. Musl uses the same type for its struct definitions.
I've just looked at the definition of struct sockaddr_storage in Musl and it is not in fact using this trick (probably because compat is not a concern there), the __align unsigned long at the end is not actually aligning anything (it's a struct, not a union). Or am I missing something?
I'm not sure what you mean. Without the __ss_align unsigned long, the alignment of the whole struct would be 2. With it, the alignment is 8 (or 4 on 32-bit). I thought that whether it is a struct or union doesn't matter, the same rules apply (I think the kernel just uses a union to make it clear that the size of __data is 126, to not factor in the size of __align). Am I missing something?
I think I see it now, __ss_align has the desired alignment and because of the layout of the struct, you always end up with the start of the struct having the same alignment as __ss_align (the other members having a smaller alignment requirement than __ss_align). Really confusing though, using a union like in the uapi header is a lot clearer. Either way, I agree this is equivalent in terms of type usage, no need to change that paragraph and sorry for the noise!
Kevin
That is indeed a bit confusing ! No worries, I think it helps everyone (myself very much included !) to have that kind of thinking out loud, and thanks Kristina for the details as well.
Will keep the paragraph as-is then if you're happy with it.
Thanks both, Téo
On 13/12/2022 09:45, Kevin Brodsky wrote:
On 12/12/2022 18:07, Teo Couprie Diaz wrote:
void* can cause issues for alignment now that its size depends on whether it is compat or purecap. In struct __kernel_sockaddr_storage, it is not annotated with __user, so it is affected.
To be more specific, the issue is not that its size depends on the ABI (that's the intention in fact), but rather that it is a trick to get 32-bit alignment on a 32-bit arch and 64-bit on a 64-bit arch. So far void * fitted the bill, but purecap broke that.
Right, I will reword to make it clearer :)
Change it to an unsigned long. Musl uses the same type for its struct definitions.
I've just looked at the definition of struct sockaddr_storage in Musl and it is not in fact using this trick (probably because compat is not a concern there), the __align unsigned long at the end is not actually aligning anything (it's a struct, not a union). Or am I missing something?
Discussed down thread, so will keep it as is.
Kevin
Thanks for the review !
Téo
Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com
include/uapi/linux/socket.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/uapi/linux/socket.h b/include/uapi/linux/socket.h index 51d6bb2f6765..28ff6a4aebca 100644 --- a/include/uapi/linux/socket.h +++ b/include/uapi/linux/socket.h @@ -22,7 +22,7 @@ struct __kernel_sockaddr_storage { /* space to achieve desired size, */ /* _SS_MAXSIZE value minus size of ss_family */ };
void *__align; /* implementation specific desired alignment */
unsigned long __align; /* implementation specific desired alignment */base-commit: bbd29c87073f43eb8c09e8a2c2f4508b46a831a0
linux-morello@op-lists.linaro.org
-
Kevin Brodsky -
Kristina Martsenko -
Teo Couprie Diaz