Re: [ovs-dev] [PATCH net] net: openvswitch: fix leak of nested actions

From: Aaron Conole
Date: Tue Apr 05 2022 - 19:35:40 EST

Ilya Maximets <i.maximets@xxxxxxx> writes:

> While parsing user-provided actions, openvswitch module may dynamically
> allocate memory and store pointers in the internal copy of the actions.
> So this memory has to be freed while destroying the actions.
> Currently there are only two such actions: ct() and set(). However,
> there are many actions that can hold nested lists of actions and
> ovs_nla_free_flow_actions() just jumps over them leaking the memory.
> For example, removal of the flow with the following actions will lead
> to a leak of the memory allocated by nf_ct_tmpl_alloc():
> actions:clone(ct(commit),0)
> Non-freed set() action may also leak the 'dst' structure for the
> tunnel info including device references.
> Under certain conditions with a high rate of flow rotation that may
> cause significant memory leak problem (2MB per second in reporter's
> case). The problem is also hard to mitigate, because the user doesn't
> have direct control over the datapath flows generated by OVS.
> Fix that by iterating over all the nested actions and freeing
> everything that needs to be freed recursively.
> New build time assertion should protect us from this problem if new
> actions will be added in the future.
> Unfortunately, openvswitch module doesn't use NLA_F_NESTED, so all
> attributes has to be explicitly checked. sample() and clone() actions
> are mixing extra attributes into the user-provided action list. That
> prevents some code generalization too.
> Fixes: 34ae932a4036 ("openvswitch: Make tunnel set action attach a metadata dst")
> Link:
> Reported-by: Stéphane Graber <stgraber@xxxxxxxxxx>
> Signed-off-by: Ilya Maximets <i.maximets@xxxxxxx>
> ---

Acked-by: Aaron Conole <aconole@xxxxxxxxxx>