Message-ID: <acf28b0e-5d34-46e0-823f-8028e2fb8356@ovn.org>
Date: Sun, 9 Feb 2025 21:16:34 +0100
From: Ilya Maximets <i.maximets@....org>
To: Gal Pressman <gal@...dia.com>, netdev@...r.kernel.org,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Andrew Lunn <andrew+netdev@...n.ch>
Cc: i.maximets@....org, dev@...nvswitch.org,
 "Gustavo A. R. Silva" <gustavoars@...nel.org>, Jiri Pirko
 <jiri@...nulli.us>, Jamal Hadi Salim <jhs@...atatu.com>,
 Kees Cook <kees@...nel.org>, David Ahern <dsahern@...nel.org>,
 Yotam Gigi <yotam.gi@...il.com>, Tariq Toukan <tariqt@...dia.com>,
 linux-hardening@...r.kernel.org, Simon Horman <horms@...nel.org>,
 Cong Wang <xiyou.wangcong@...il.com>, Cosmin Ratiu <cratiu@...dia.com>
Subject: Re: [ovs-dev] [PATCH net-next] net: Add options as a flexible array
 to struct ip_tunnel_info

On 2/9/25 20:37, Gal Pressman wrote:
> 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?

There is nothing special in it.  But the fact that there was no explicit
alignment of the options before doesn't make them not actually aligned.

> 
>> 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?

I agree that it may break if the structure changed.  However, there are,
in fact, __be64 fields in the structure that make the whole thing aligned
and it's part of dst_metadata that by itself is also aligned, so any options
in the tunnel info will be inherently aligned for 8-byte accesses.

I didn't check the actual structure layout with this change, but it seems
like the start of the options will be only 2-byte aligned, making them
non-aligned for most types of accesses and likely causing problems after
the cast.

So, not saying that the current code is good, but it's implicitly doing the
right thing.  We should make that more explicit and add some guarantees that
the alignment will not break, but we should not break the alignment itself.

> 
>> 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?

Ideally we would have a proper union with all the potential option types
instead of this hacky construct.  But if that's not the the way to go, then
8 bytes may indeed be the way, as it is the maximum guaranteed alignment
for allocations and the current alignment of the structure.

But I'll leave this for others to weigh in.

Best regards, Ilya Maximets.

Powered by blists - more mailing lists