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.
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 __aligned(4); +#ifdef CONFIG_COMPAT32 } __packed; +#else +}; +#endif
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
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).
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 __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.
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
On 01/12/2022 16: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).
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 __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.
That does sound much cleaner, I'll give it a look !
Otherwise this patch looks good to me!
Thanks, Kristina
Thanks for the review, Téo
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
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
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.
__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
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
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
On 05/12/2022 13:24, Teo Couprie Diaz wrote:
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 ?
You could, it's not necessary though as this is specifying a *minimum* alignment and native will always have at least this alignment.
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...
The message of the commit I mentioned above has some details on that, though I have to admit the details are not entirely clear to me either.
Kevin
__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
On 05/12/2022 14:27, Kevin Brodsky wrote:
On 05/12/2022 13:24, Teo Couprie Diaz wrote:
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 ?
You could, it's not necessary though as this is specifying a *minimum* alignment and native will always have at least this alignment.
Noted, seems like it's not really interesting to add then.
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...
The message of the commit I mentioned above has some details on that, though I have to admit the details are not entirely clear to me either.
Kevin
I'll give it a look then, thanks !
Sent the v2 with the macro.
Téo
__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
linux-morello mailing list -- linux-morello@op-lists.linaro.org To unsubscribe send an email to linux-morello-leave@op-lists.linaro.org
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
On 05/12/2022 15:37, Kristina Martsenko wrote:
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)...
Ah thanks for pointing this out, that does explain things indeed! In that case it sounds like we should just replace that void * with unsigned long in the uapi header too.
Kevin
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
linux-morello mailing list -- linux-morello@op-lists.linaro.org To unsubscribe send an email to linux-morello-leave@op-lists.linaro.org
linux-morello@op-lists.linaro.org
-
Kevin Brodsky -
Kristina Martsenko -
Teo Couprie Diaz