Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
From:   Aaron Conole <aconole@redhat.com>
To:     Ilya Maximets <i.maximets@ovn.org>
Cc:     netdev@vger.kernel.org, dev@openvswitch.org,
        linux-kernel@vger.kernel.org, Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        "David S. Miller" <davem@davemloft.net>
Subject: Re: [ovs-dev] [PATCH net] net: openvswitch: fix leak of nested actions
References: <20220404154345.2980792-1-i.maximets@ovn.org>
Date:   Tue, 05 Apr 2022 08:56:52 -0400
In-Reply-To: <20220404154345.2980792-1-i.maximets@ovn.org> (Ilya Maximets's
        message of "Mon, 4 Apr 2022 17:43:45 +0200")
Message-ID: <f7t4k371yej.fsf@redhat.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Ilya Maximets <i.maximets@ovn.org> 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 metada=
ta dst")
> Link: https://mail.openvswitch.org/pipermail/ovs-dev/2022-March/392922.ht=
ml
> Reported-by: St=C3=A9phane Graber <stgraber@ubuntu.com>
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> ---

Acked-by: Aaron Conole <aconole@redhat.com>