On 8/14/24 21:09, Dumitru Ceara wrote: > 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]>
Thanks! I updated the commit message and applied the change to all branches down to 2.17. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
