Message-ID: <573e20ec-5b85-4b11-b479-d149e5c434b0@ovn.org>
Date: Mon, 10 Mar 2025 13:25:19 +0100
From: Ilya Maximets <i.maximets@....org>
To: Aaron Conole <aconole@...hat.com>
Cc: i.maximets@....org, 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>,
 Simon Horman <horms@...nel.org>, dev@...nvswitch.org,
 linux-kernel@...r.kernel.org, Pravin B Shelar <pshelar@....org>,
 Eelco Chaudron <echaudro@...hat.com>
Subject: Re: [PATCH net] net: openvswitch: remove misbehaving actions length
 check

On 3/8/25 21:03, Aaron Conole wrote:
> Ilya Maximets <i.maximets@....org> writes:
> 
>> The actions length check is unreliable and produces different results
>> depending on the initial length of the provided netlink attribute and
>> the composition of the actual actions inside of it.  For example, a
>> user can add 4088 empty clone() actions without triggering -EMSGSIZE,
>> on attempt to add 4089 such actions the operation will fail with the
>> -EMSGSIZE verdict.  However, if another 16 KB of other actions will
>> be *appended* to the previous 4089 clone() actions, the check passes
>> and the flow is successfully installed into the openvswitch datapath.
>>
>> The reason for a such a weird behavior is the way memory is allocated.
>> When ovs_flow_cmd_new() is invoked, it calls ovs_nla_copy_actions(),
>> that in turn calls nla_alloc_flow_actions() with either the actual
>> length of the user-provided actions or the MAX_ACTIONS_BUFSIZE.  The
>> function adds the size of the sw_flow_actions structure and then the
>> actually allocated memory is rounded up to the closest power of two.
>>
>> So, if the user-provided actions are larger than MAX_ACTIONS_BUFSIZE,
>> then MAX_ACTIONS_BUFSIZE + sizeof(*sfa) rounded up is 32K + 24 -> 64K.
>> Later, while copying individual actions, we look at ksize(), which is
>> 64K, so this way the MAX_ACTIONS_BUFSIZE check is not actually
>> triggered and the user can easily allocate almost 64 KB of actions.
>>
>> However, when the initial size is less than MAX_ACTIONS_BUFSIZE, but
>> the actions contain ones that require size increase while copying
>> (such as clone() or sample()), then the limit check will be performed
>> during the reserve_sfa_size() and the user will not be allowed to
>> create actions that yield more than 32 KB internally.
>>
>> This is one part of the problem.  The other part is that it's not
>> actually possible for the userspace application to know beforehand
>> if the particular set of actions will be rejected or not.
>>
>> Certain actions require more space in the internal representation,
>> e.g. an empty clone() takes 4 bytes in the action list passed in by
>> the user, but it takes 12 bytes in the internal representation due
>> to an extra nested attribute, and some actions require less space in
>> the internal representations, e.g. set(tunnel(..)) normally takes
>> 64+ bytes in the action list provided by the user, but only needs to
>> store a single pointer in the internal implementation, since all the
>> data is stored in the tunnel_info structure instead.
>>
>> And the action size limit is applied to the internal representation,
>> not to the action list passed by the user.  So, it's not possible for
>> the userpsace application to predict if the certain combination of
>> actions will be rejected or not, because it is not possible for it to
>> calculate how much space these actions will take in the internal
>> representation without knowing kernel internals.
>>
>> All that is causing random failures in ovs-vswitchd in userspace and
>> inability to handle certain traffic patterns as a result.  For example,
>> it is reported that adding a bit more than a 1100 VMs in an OpenStack
>> setup breaks the network due to OVS not being able to handle ARP
>> traffic anymore in some cases (it tries to install a proper datapath
>> flow, but the kernel rejects it with -EMSGSIZE, even though the action
>> list isn't actually that large.)
>>
>> Kernel behavior must be consistent and predictable in order for the
>> userspace application to use it in a reasonable way.  ovs-vswitchd has
>> a mechanism to re-direct parts of the traffic and partially handle it
>> in userspace if the required action list is oversized, but that doesn't
>> work properly if we can't actually tell if the action list is oversized
>> or not.
>>
>> Solution for this is to check the size of the user-provided actions
>> instead of the internal representation.  This commit just removes the
>> check from the internal part because there is already an implicit size
>> check imposed by the netlink protocol.  The attribute can't be larger
>> than 64 KB.  Realistically, we could reduce the limit to 32 KB, but
>> we'll be risking to break some existing setups that rely on the fact
>> that it's possible to create nearly 64 KB action lists today.
>>
>> Vast majority of flows in real setups are below 100-ish bytes.  So
>> removal of the limit will not change real memory consumption on the
>> system.  The absolutely worst case scenario is if someone adds a flow
>> with 64 KB of empty clone() actions.  That will yield a 192 KB in the
>> internal representation consuming 256 KB block of memory.  However,
>> that list of actions is not meaningful and also a no-op.  Real world
>> very large action lists (that can occur for a rare cases of BUM
>> traffic handling) are unlikely to contain a large number of clones and
>> will likely have a lot of tunnel attributes making the internal
>> representation comparable in size to the original action list.
>> So, it should be fine to just remove the limit.
>>
>> Commit in the 'Fixes' tag is the first one that introduced the
>> difference between internal representation and the user-provided action
>> lists, but there were many more afterwards that lead to the situation
>> we have today.
>>
>> Fixes: 7d5437c709de ("openvswitch: Add tunneling interface.")
>> Signed-off-by: Ilya Maximets <i.maximets@....org>
>> ---
> 
> Thanks for the detailed explanation.  Do you think it's useful to
> check with selftest:
> 
>    # python3 ./ovs-dpctl.py add-dp flbr
>    # python3 ./ovs-dpctl.py add-flow flbr \
>      "in_port(0),eth(),eth_type(0x806),arp()" \
>      $(echo 'print("clone(),"*4089)' | python3)
> 
> I think a limit test is probably a good thing to have anyway (although
> after this commit we will rely on netlink limits).

I had a similar thought, but as you said, this commit will remove the limit
so we'll not really be testing OVS code at this point.  So, I thought it
may be better to not include such a test for easier backporting to older
kernels (given the Fixes tag goes far back).   But I agree that it's a good
thing in general to have tests that cover maximum size cases, so maybe we
can add something like this to net-next instead, once the fix is accepted?

Note: 4089 is too small for such a test, it should be somewhere around 16K.

> 
> 
> Reviewed-by: Aaron Conole <aconole@...hat.com>
> 

Thanks for review!

Best regards, Ilya Maximets.

Powered by blists - more mailing lists