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.
Then shouldn't I add a macro for removing the alignment in COMPAT64 as well ?
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
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?
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.
Thanks for the explanation, that makes sense of a bit of confusion I still had !
Changing to long might work ? I'm still a bit confused on how the combination of `aligned(X)` and a void* is used to handle the alignment however...
__aligned(4);+#ifdef CONFIG_COMPAT32 } __packed; +#else +}; +#endif
It might be nice to define a new macro similar to EPOLL_PACKED, and use that on each of the structs, just to have less #ifdefing.
Agreed.
Kevin
Ready for v2 with the macro once I know if I should do it for the alignment as well !
Thanks for the review, Téo
Otherwise this patch looks good to me!
Thanks, Kristina
struct compat_group_source_req { __u32 gsr_interface; @@ -76,7 +80,11 @@ struct compat_group_source_req { __aligned(4); struct __kernel_sockaddr_storage gsr_source __aligned(4); +#ifdef CONFIG_COMPAT32 } __packed; +#else +}; +#endif struct compat_group_filter { union { @@ -88,7 +96,11 @@ struct compat_group_filter { __u32 gf_numsrc_aux; struct __kernel_sockaddr_storage gf_slist[1] __aligned(4); +#ifdef CONFIG_COMPAT32 } __packed; +#else
};+#endif struct { __u32 gf_interface; struct __kernel_sockaddr_storage gf_group @@ -97,8 +109,16 @@ struct compat_group_filter { __u32 gf_numsrc; struct __kernel_sockaddr_storage gf_slist_flex[] __aligned(4); +#ifdef CONFIG_COMPAT32 } __packed; +#else
};+#endif }; -} __packed; +#ifdef CONFIG_COMPAT32
- } __packed;
+#else
- };
+#endif #endif /* NET_COMPAT_H */
base-commit: 2fcc0af155a19755f68602f400fc845d2c9e6077
linux-morello mailing list -- linux-morello@op-lists.linaro.org To unsubscribe send an email to linux-morello-leave@op-lists.linaro.org