On 8/14/24 16:53, Ilya Maximets wrote: > On 8/14/24 16:39, Dumitru Ceara wrote: >> On 8/14/24 15:31, Ilya Maximets wrote: >>> On 8/14/24 14:25, Dumitru Ceara wrote: >>>> On 8/14/24 14:24, Dumitru Ceara wrote: >>>>> Hi Ilya, >>>>> >>>>> On 8/14/24 14:09, Simon Horman wrote: >>>>>> On Wed, Aug 14, 2024 at 12:35:28PM +0200, Ilya Maximets wrote: >>>>>>> memcpy() is allowed to make alignment assumptions based on the argument >>>>>>> type. We have to cast before using it to avoid misaligned reads: >>>>> >>>>> I guess that's fine. But I'm not sure I understand how memcpy() is >>>>> allowed to make any assumption based on the argument type. Isn't >>>>> memcpy() supposed to be a function call? >>> >>> I guess, I need to re-phrase that into "compilers are allowed to make >>> alignment assumptions based on the pointer type passed to memcpy()". >>> >>> I can change that before applying the patch. >>> >> >> Sounds good. >> >>> Also, even having misaligned pointers is generally not allowed in C... >>> >>> Some more info: >>> https://github.com/llvm/llvm-project/issues/90848#issuecomment-2090636114 >>> >> >> Do you mean that the line above is already undefined behavior, just not >> flagged as one? >> >> const struct rtnl_link_stats64 *lstats = nl_attr_get(a); >> >>> There are also similar issues reported in other projects with GCC. >>> Mainly ARM builds are the main issue as lower ARM architectures are >>> picky when it comes to misaligned memory accesses. >>> >> >> Ack, accesses I understand, what I'm not sure I understand completely is >> just having misaligned pointers (that are never dereferenced). > > C11 6.3.2.3 Pointers > > 7) A pointer to an object type may be converted to a pointer to a different > object type. If the resulting pointer is not correctly aligned for the > referenced type, the behavior is undefined. > > So, in theory, cast from the 'void *' returned from nl_attr_get() to a > 'struct rtnl_link_stats64 *' constitutes an undefined behavior in case > that pointer is not correctly aligned for 'struct rtnl_link_stats64 *'. >
Right, that's exactly what I was wondering. > In practice, I'm not aware of this being an actual problem as long as you're > not trying to dereference it. And we may have such issues all over the place > and many other projects will if compilers will start to insist on this being > a problem. > I agree, this would probably pop up in a lot of places if enforced. In any case, for your change, with the amended commit message: Acked-by: Dumitru Ceara <[email protected]> Regards, Dumitru >> >>>>> >>>>> At least the man page on my machine says: >>>>> >>>>> NAME >>>>> memcpy - copy memory area >>>>> >>>>> LIBRARY >>>>> Standard C library (libc, -lc) >>>>> >>>>> SYNOPSIS >>>>> #include <string.h> >>>>> >>>>> void *memcpy(void dest[restrict .n], const void src[restrict .n], >>>>> size_t n); >>>>> >>>> >>>> Forgot this part but I guess it was kind of obvious: :) >>>> >>>> DESCRIPTION >>>> The memcpy() function copies n bytes from memory area src to >>>> memory area dest. The memory areas must not overlap. Use memmove(3) if >>>> the memory areas do overlap. >>>> >>>> >>>>> Is it actually the compiler doing some special magic behind the scenes? >>>>> >>>>>>> >>>>>>> lib/netdev-linux.c:6738:41: runtime error: load of misaligned address >>>>>>> 0x51b000008db4 for type 'const struct rpl_rtnl_link_stats64 *', >>>>>>> which requires 8 byte alignment >>>>>>> 0x51b000008db4: note: pointer points here >>>>>>> cc 00 17 00 01 00 00 00 00 00 00 00 01 00 00 00 >>>>>>> ^ >>>>>>> 0 0xad2d8 in get_stats_via_netlink lib/netdev-linux.c:6738:17 >>>>>>> 1 0x9dcce in netdev_linux_get_stats lib/netdev-linux.c:2293:13 >>>>>>> 2 0xee5b8 in netdev_get_stats lib/netdev.c:1722:16 >>>>>>> 3 0xc07ad in iface_refresh_stats vswitchd/bridge.c:2795:5 >>>>>>> 4 0xbf4fc in iface_create vswitchd/bridge.c:2274:5 >>>>>>> 5 0xbf4fc in bridge_add_ports__ vswitchd/bridge.c:1225:21 >>>>>>> 6 0x989fc in bridge_add_ports vswitchd/bridge.c:1236:5 >>>>>>> 7 0x989fc in bridge_reconfigure vswitchd/bridge.c:952:9 >>>>>>> 8 0x9102a in bridge_run vswitchd/bridge.c:3439:9 >>>>>>> 9 0xc8976 in main vswitchd/ovs-vswitchd.c:137:9 >>>>>>> 10 0x2a1c9 in __libc_start_call_main >>>>>>> 11 0x2a28a in __libc_start_main >>>>>>> 12 0xb3ff4 in _start (vswitchd/ovs-vswitchd+0x734ff4) >>>>>>> >>>>>>> Reported by Clang 18 with UBsan on Ubuntu 24.04. >>>>>>> >>>>>>> Fixes: c8c49a9db9f2 ("netdev-linux: Properly access 32-bit aligned >>>>>>> rtnl_link_stats64 structs.") >>>>>>> Signed-off-by: Ilya Maximets <[email protected]> >>>>>> >>>>>> Reviewed-by: Simon Horman <[email protected]> >>>>>> >>>>> >>>>> Regards, >>>>> Dumitru >>>>> >>>>> >>>> >>> >> > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
