Re: [ovs-dev] [PATCH net-next] net: Add options as a flexible array to struct ip_tunnel_info

[Gal Pressman <gal@nvidia.com>]

Hi Ilya, thanks for the review.

On 09/02/2025 18:21, Ilya Maximets wrote:
> On 2/9/25 11:18, Gal Pressman via dev wrote:
>> Remove the hidden assumption that options are allocated at the end of
>> the struct, and teach the compiler about them using a flexible array.
>>
>> With this, we can revert the unsafe_memcpy() call we have in
>> tun_dst_unclone() [1], and resolve the false field-spanning write
>> warning caused by the memcpy() in ip_tunnel_info_opts_set().
>>
>> Note that this patch changes the layout of struct ip_tunnel_info since
>> there is padding at the end of the struct.
>> Before this, options would be written at 'info + 1' which is after the
>> padding.
>> After this patch, options are written right after 'mode' field (into the
>> padding).
> 
> This doesn't sound like a safe thing to do.  'info + 1' ensures that the
> options are aligned the same way as the struct ip_tunnel_info itself.

What is special about the alignment of struct ip_tunnel_info? What are
you assuming it to be, and how is it related to whatever alignment the
options need?

> In many places in the code, the options are cast into a specific tunnel
> options type that may require sufficient alignment.  And the alignment can
> no longer be guaranteed once the options are put directly after the 'mode'.

What guaranteed it was aligned before? A hidden assumption that a u64 is
hidden somewhere in ip_tunnel_info?

> May cause crashes on some architectures as well as performance impact on
> others.
> 
> Should the alignment attribute be also added to the field?

Align to what?
To the first field of every potential options type? To eight bytes?