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 *'.
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.
>
>>>>
>>>> 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
