| From edc6948dcb8c44bee64d0ad0bdbb569071d7ff79 Mon Sep 17 00:00:00 2001 |
| From: Sasha Levin <sashal@kernel.org> |
| Date: Mon, 4 Apr 2022 17:43:45 +0200 |
| Subject: net: openvswitch: fix leak of nested actions |
| MIME-Version: 1.0 |
| Content-Type: text/plain; charset=UTF-8 |
| Content-Transfer-Encoding: 8bit |
| |
| From: Ilya Maximets <i.maximets@ovn.org> |
| |
| [ Upstream commit 1f30fb9166d4f15a1aa19449b9da871fe0ed4796 ] |
| |
| 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: https://mail.openvswitch.org/pipermail/ovs-dev/2022-March/392922.html |
| Reported-by: Stéphane Graber <stgraber@ubuntu.com> |
| Signed-off-by: Ilya Maximets <i.maximets@ovn.org> |
| Acked-by: Aaron Conole <aconole@redhat.com> |
| Signed-off-by: David S. Miller <davem@davemloft.net> |
| Signed-off-by: Sasha Levin <sashal@kernel.org> |
| --- |
| net/openvswitch/flow_netlink.c | 95 ++++++++++++++++++++++++++++++++-- |
| 1 file changed, 90 insertions(+), 5 deletions(-) |
| |
| diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c |
| index 2679007f8aeb..c591b923016a 100644 |
| --- a/net/openvswitch/flow_netlink.c |
| +++ b/net/openvswitch/flow_netlink.c |
| @@ -2288,6 +2288,62 @@ static struct sw_flow_actions *nla_alloc_flow_actions(int size) |
| return sfa; |
| } |
| |
| +static void ovs_nla_free_nested_actions(const struct nlattr *actions, int len); |
| + |
| +static void ovs_nla_free_check_pkt_len_action(const struct nlattr *action) |
| +{ |
| + const struct nlattr *a; |
| + int rem; |
| + |
| + nla_for_each_nested(a, action, rem) { |
| + switch (nla_type(a)) { |
| + case OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL: |
| + case OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER: |
| + ovs_nla_free_nested_actions(nla_data(a), nla_len(a)); |
| + break; |
| + } |
| + } |
| +} |
| + |
| +static void ovs_nla_free_clone_action(const struct nlattr *action) |
| +{ |
| + const struct nlattr *a = nla_data(action); |
| + int rem = nla_len(action); |
| + |
| + switch (nla_type(a)) { |
| + case OVS_CLONE_ATTR_EXEC: |
| + /* The real list of actions follows this attribute. */ |
| + a = nla_next(a, &rem); |
| + ovs_nla_free_nested_actions(a, rem); |
| + break; |
| + } |
| +} |
| + |
| +static void ovs_nla_free_dec_ttl_action(const struct nlattr *action) |
| +{ |
| + const struct nlattr *a = nla_data(action); |
| + |
| + switch (nla_type(a)) { |
| + case OVS_DEC_TTL_ATTR_ACTION: |
| + ovs_nla_free_nested_actions(nla_data(a), nla_len(a)); |
| + break; |
| + } |
| +} |
| + |
| +static void ovs_nla_free_sample_action(const struct nlattr *action) |
| +{ |
| + const struct nlattr *a = nla_data(action); |
| + int rem = nla_len(action); |
| + |
| + switch (nla_type(a)) { |
| + case OVS_SAMPLE_ATTR_ARG: |
| + /* The real list of actions follows this attribute. */ |
| + a = nla_next(a, &rem); |
| + ovs_nla_free_nested_actions(a, rem); |
| + break; |
| + } |
| +} |
| + |
| static void ovs_nla_free_set_action(const struct nlattr *a) |
| { |
| const struct nlattr *ovs_key = nla_data(a); |
| @@ -2301,25 +2357,54 @@ static void ovs_nla_free_set_action(const struct nlattr *a) |
| } |
| } |
| |
| -void ovs_nla_free_flow_actions(struct sw_flow_actions *sf_acts) |
| +static void ovs_nla_free_nested_actions(const struct nlattr *actions, int len) |
| { |
| const struct nlattr *a; |
| int rem; |
| |
| - if (!sf_acts) |
| + /* Whenever new actions are added, the need to update this |
| + * function should be considered. |
| + */ |
| + BUILD_BUG_ON(OVS_ACTION_ATTR_MAX != 23); |
| + |
| + if (!actions) |
| return; |
| |
| - nla_for_each_attr(a, sf_acts->actions, sf_acts->actions_len, rem) { |
| + nla_for_each_attr(a, actions, len, rem) { |
| switch (nla_type(a)) { |
| - case OVS_ACTION_ATTR_SET: |
| - ovs_nla_free_set_action(a); |
| + case OVS_ACTION_ATTR_CHECK_PKT_LEN: |
| + ovs_nla_free_check_pkt_len_action(a); |
| + break; |
| + |
| + case OVS_ACTION_ATTR_CLONE: |
| + ovs_nla_free_clone_action(a); |
| break; |
| + |
| case OVS_ACTION_ATTR_CT: |
| ovs_ct_free_action(a); |
| break; |
| + |
| + case OVS_ACTION_ATTR_DEC_TTL: |
| + ovs_nla_free_dec_ttl_action(a); |
| + break; |
| + |
| + case OVS_ACTION_ATTR_SAMPLE: |
| + ovs_nla_free_sample_action(a); |
| + break; |
| + |
| + case OVS_ACTION_ATTR_SET: |
| + ovs_nla_free_set_action(a); |
| + break; |
| } |
| } |
| +} |
| + |
| +void ovs_nla_free_flow_actions(struct sw_flow_actions *sf_acts) |
| +{ |
| + if (!sf_acts) |
| + return; |
| |
| + ovs_nla_free_nested_actions(sf_acts->actions, sf_acts->actions_len); |
| kfree(sf_acts); |
| } |
| |
| -- |
| 2.35.1 |
| |