On 05/12/2022 10:15, Kevin Brodsky wrote:
On 01/12/2022 17:20, Kristina Martsenko wrote:
On 30/11/2022 17:19, Teo Couprie Diaz wrote:
The __packed attribute for struct compat_group_req led to a failure of the LTP test accept02 when trying to join a multicast group. Keep it for COMPAT32 but remove it in COMPAT64, do the same for the other structs used here.
Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com
Not really sure this is the right way to go, nor if the others structs should be left alone. Chaning them all didn't lead to failures in setsockopt or accept LTP tests, which was where the issue was detected.
FWIW I agree with changing it for the other structs as well (might save some time debugging later).
Definitely. That's a good catch, compat_* structs like these that do not contain user pointers should be exactly the same as native in compat64, including alignment, padding, etc.
include/net/compat.h | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/include/net/compat.h b/include/net/compat.h index f2d7423217d2..5cb091e2a6a6 100644 --- a/include/net/compat.h +++ b/include/net/compat.h @@ -68,7 +68,11 @@ struct compat_group_req { __u32 gr_interface; struct __kernel_sockaddr_storage gr_group
Looking at this struct __kernel_sockaddr_storage, I suspect the way it is manually aligned right now is not working in PCuABI. Indeed since 5e5412c365a3 ("net/socket: fix GCC8+ Wpacked-not-aligned warnings") that struct is aligned by having a void * in a union. It is not annotated with __user, which for us means that the alignment of the struct is:
- 8 from a kernel perspective
- 16 from a userspace perspective in purecap
That does indeed look broken.
This will result in the kernel and userspace disagreeing on the layout of the struct in PCuABI. I don't think we can simply stick a __user in there, for one thing because it is used to define these compat_* structs too, and more importantly because it doesn't make sense, as this void * is a trick to align to 4/8 bytes depending on the ABI and has nothing to do with storing pointers in the struct. I'm not quite sure what the best solution is here, maybe replacing void * with long just works?
Agreed about __user. Not sure either about the best solution, but it sounds like replacing with 'long' should work. Actually musl already uses 'unsigned long' for the alignment, which may explain why the purecap accept02 LTP test passes (even though it failed for compat)...
In any case this is a tangent, not directly related to this patch - I can create a ticket to investigate this separately if that makes sense.
Makes sense (or we can just change it to unsigned long).
Thanks, Kristina