[PATCH net 1/2] openvswitch: Fix helper reference leak

[PATCH net 1/2] openvswitch: Fix helper reference leak

[Joe Stringer]

If the actions (re)allocation fails, or the actions list is larger than the
maximum size, and the conntrack action is the last action when these
problems are hit, then references to helper modules may be leaked. Fix
the issue.

Fixes: cae3a2627520 ("openvswitch: Allow attaching helpers to ct action")
Signed-off-by: Joe Stringer <joe@ovn.org>
---
 net/openvswitch/conntrack.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index c2cc11168fd5..585a5aa81f89 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -53,6 +53,8 @@ struct ovs_conntrack_info {
 	struct md_labels labels;
 };
 
+static void __ovs_ct_free_action(struct ovs_conntrack_info *ct_info);
+
 static u16 key_to_nfproto(const struct sw_flow_key *key)
 {
 	switch (ntohs(key->eth.type)) {
@@ -708,7 +710,7 @@ int ovs_ct_copy_action(struct net *net, const struct nlattr *attr,
 	nf_conntrack_get(&ct_info.ct->ct_general);
 	return 0;
 err_free_ct:
-	nf_conntrack_free(ct_info.ct);
+	__ovs_ct_free_action(&ct_info);
 	return err;
 }
 
@@ -750,6 +752,11 @@ void ovs_ct_free_action(const struct nlattr *a)
 {
 	struct ovs_conntrack_info *ct_info = nla_data(a);
 
+	__ovs_ct_free_action(ct_info);
+}
+
+static void __ovs_ct_free_action(struct ovs_conntrack_info *ct_info)
+{
 	if (ct_info->helper)
 		module_put(ct_info->helper->me);
 	if (ct_info->ct)
-- 
2.1.4

[PATCH net 2/2] openvswitch: Respect conntrack zone even if invalid

[Joe Stringer]

If userspace executes ct(zone=1), and the connection tracker determines
that the packet is invalid, then the ct_zone flow key field is populated
with the default zone rather than the zone that was specified. Even
though connection tracking failed, this field should be updated with the
value that the action specified. Fix the issue.

Fixes: 7f8a436eaa2c ("openvswitch: Add conntrack action")
Signed-off-by: Joe Stringer <joe@ovn.org>
---
 net/openvswitch/conntrack.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index 585a5aa81f89..3e8892216f94 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -143,6 +143,7 @@ static void __ovs_ct_update_key(struct sw_flow_key *key, u8 state,
  * previously sent the packet to conntrack via the ct action.
  */
 static void ovs_ct_update_key(const struct sk_buff *skb,
+			      const struct ovs_conntrack_info *info,
 			      struct sw_flow_key *key, bool post_ct)
 {
 	const struct nf_conntrack_zone *zone = &nf_ct_zone_dflt;
@@ -160,13 +161,15 @@ static void ovs_ct_update_key(const struct sk_buff *skb,
 		zone = nf_ct_zone(ct);
 	} else if (post_ct) {
 		state = OVS_CS_F_TRACKED | OVS_CS_F_INVALID;
+		if (info)
+			zone = &info->zone;
 	}
 	__ovs_ct_update_key(key, state, zone, ct);
 }
 
 void ovs_ct_fill_key(const struct sk_buff *skb, struct sw_flow_key *key)
 {
-	ovs_ct_update_key(skb, key, false);
+	ovs_ct_update_key(skb, NULL, key, false);
 }
 
 int ovs_ct_put_key(const struct sw_flow_key *key, struct sk_buff *skb)
@@ -420,7 +423,7 @@ static int __ovs_ct_lookup(struct net *net, struct sw_flow_key *key,
 		}
 	}
 
-	ovs_ct_update_key(skb, key, true);
+	ovs_ct_update_key(skb, info, key, true);
 
 	return 0;
 }
-- 
2.1.4

Re: [PATCH net 1/2] openvswitch: Fix helper reference leak

[Pravin Shelar]

On Wed, Dec 9, 2015 at 2:07 PM, Joe Stringer <joe@ovn.org> wrote:
> If the actions (re)allocation fails, or the actions list is larger than the
> maximum size, and the conntrack action is the last action when these
> problems are hit, then references to helper modules may be leaked. Fix
> the issue.
>
> Fixes: cae3a2627520 ("openvswitch: Allow attaching helpers to ct action")
> Signed-off-by: Joe Stringer <joe@ovn.org>
> ---
>  net/openvswitch/conntrack.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
> index c2cc11168fd5..585a5aa81f89 100644
> --- a/net/openvswitch/conntrack.c
> +++ b/net/openvswitch/conntrack.c
> @@ -53,6 +53,8 @@ struct ovs_conntrack_info {
>         struct md_labels labels;
>  };
>
> +static void __ovs_ct_free_action(struct ovs_conntrack_info *ct_info);
> +
>  static u16 key_to_nfproto(const struct sw_flow_key *key)
>  {
>         switch (ntohs(key->eth.type)) {
> @@ -708,7 +710,7 @@ int ovs_ct_copy_action(struct net *net, const struct nlattr *attr,
>         nf_conntrack_get(&ct_info.ct->ct_general);

I see another issue here. Updates to ct_info are done after it is
copied to action list. ct action on the action list and ct_info are
inconsistent, So it is still leaking nf-conntrack reference.

Re: [PATCH net 1/2] openvswitch: Fix helper reference leak

[Joe Stringer]

On 9 December 2015 at 14:50, Pravin Shelar <pshelar@nicira.com> wrote:
> On Wed, Dec 9, 2015 at 2:07 PM, Joe Stringer <joe@ovn.org> wrote:
>> If the actions (re)allocation fails, or the actions list is larger than the
>> maximum size, and the conntrack action is the last action when these
>> problems are hit, then references to helper modules may be leaked. Fix
>> the issue.
>>
>> Fixes: cae3a2627520 ("openvswitch: Allow attaching helpers to ct action")
>> Signed-off-by: Joe Stringer <joe@ovn.org>
>> ---
>>  net/openvswitch/conntrack.c | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
>> index c2cc11168fd5..585a5aa81f89 100644
>> --- a/net/openvswitch/conntrack.c
>> +++ b/net/openvswitch/conntrack.c
>> @@ -53,6 +53,8 @@ struct ovs_conntrack_info {
>>         struct md_labels labels;
>>  };
>>
>> +static void __ovs_ct_free_action(struct ovs_conntrack_info *ct_info);
>> +
>>  static u16 key_to_nfproto(const struct sw_flow_key *key)
>>  {
>>         switch (ntohs(key->eth.type)) {
>> @@ -708,7 +710,7 @@ int ovs_ct_copy_action(struct net *net, const struct nlattr *attr,
>>         nf_conntrack_get(&ct_info.ct->ct_general);
>
> I see another issue here. Updates to ct_info are done after it is
> copied to action list. ct action on the action list and ct_info are
> inconsistent, So it is still leaking nf-conntrack reference.

You're referring to the below?

         __set_bit(IPS_CONFIRMED_BIT, &ct_info.ct->status);
         nf_conntrack_get(&ct_info.ct->ct_general);

ct_info.ct points to the same location as the version that has been
copied into the action list. I agree it's a bit misleading though. If
you're not seeing something else, it seems cosmetic so I can (should?)
follow up on net-next.

Re: [PATCH net 1/2] openvswitch: Fix helper reference leak

[Pravin Shelar]

On Wed, Dec 9, 2015 at 3:10 PM, Joe Stringer <joe@ovn.org> wrote:
> On 9 December 2015 at 14:50, Pravin Shelar <pshelar@nicira.com> wrote:
>> On Wed, Dec 9, 2015 at 2:07 PM, Joe Stringer <joe@ovn.org> wrote:
>>> If the actions (re)allocation fails, or the actions list is larger than the
>>> maximum size, and the conntrack action is the last action when these
>>> problems are hit, then references to helper modules may be leaked. Fix
>>> the issue.
>>>
>>> Fixes: cae3a2627520 ("openvswitch: Allow attaching helpers to ct action")
>>> Signed-off-by: Joe Stringer <joe@ovn.org>
>>> ---
>>>  net/openvswitch/conntrack.c | 9 ++++++++-
>>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
>>> index c2cc11168fd5..585a5aa81f89 100644
>>> --- a/net/openvswitch/conntrack.c
>>> +++ b/net/openvswitch/conntrack.c
>>> @@ -53,6 +53,8 @@ struct ovs_conntrack_info {
>>>         struct md_labels labels;
>>>  };
>>>
>>> +static void __ovs_ct_free_action(struct ovs_conntrack_info *ct_info);
>>> +
>>>  static u16 key_to_nfproto(const struct sw_flow_key *key)
>>>  {
>>>         switch (ntohs(key->eth.type)) {
>>> @@ -708,7 +710,7 @@ int ovs_ct_copy_action(struct net *net, const struct nlattr *attr,
>>>         nf_conntrack_get(&ct_info.ct->ct_general);
>>
>> I see another issue here. Updates to ct_info are done after it is
>> copied to action list. ct action on the action list and ct_info are
>> inconsistent, So it is still leaking nf-conntrack reference.
>
> You're referring to the below?
>
>          __set_bit(IPS_CONFIRMED_BIT, &ct_info.ct->status);
>          nf_conntrack_get(&ct_info.ct->ct_general);
>
> ct_info.ct points to the same location as the version that has been
> copied into the action list. I agree it's a bit misleading though. If
> you're not seeing something else, it seems cosmetic so I can (should?)
> follow up on net-next.

Right, there is no leak, cosmetic changes can be done later. I do not
have problem with this patch as it is.

Acked-by: Pravin B Shelar <pshelar@nicira.com>

Re: [PATCH net 2/2] openvswitch: Respect conntrack zone even if invalid

[Pravin Shelar]

On Wed, Dec 9, 2015 at 2:07 PM, Joe Stringer <joe@ovn.org> wrote:
> If userspace executes ct(zone=1), and the connection tracker determines
> that the packet is invalid, then the ct_zone flow key field is populated
> with the default zone rather than the zone that was specified. Even
> though connection tracking failed, this field should be updated with the
> value that the action specified. Fix the issue.
>
> Fixes: 7f8a436eaa2c ("openvswitch: Add conntrack action")
> Signed-off-by: Joe Stringer <joe@ovn.org>

Acked-by: Pravin B Shelar <pshelar@nicira.com>

Thanks.

Re: [PATCH net 1/2] openvswitch: Fix helper reference leak

[David Miller]

From: Joe Stringer <joe@ovn.org>
Date: Wed,  9 Dec 2015 14:07:39 -0800

> If the actions (re)allocation fails, or the actions list is larger than the
> maximum size, and the conntrack action is the last action when these
> problems are hit, then references to helper modules may be leaked. Fix
> the issue.
> 
> Fixes: cae3a2627520 ("openvswitch: Allow attaching helpers to ct action")
> Signed-off-by: Joe Stringer <joe@ovn.org>

Applied and queued up for -stable.

Re: [PATCH net 2/2] openvswitch: Respect conntrack zone even if invalid

[David Miller]

From: Joe Stringer <joe@ovn.org>
Date: Wed,  9 Dec 2015 14:07:40 -0800

> If userspace executes ct(zone=1), and the connection tracker determines
> that the packet is invalid, then the ct_zone flow key field is populated
> with the default zone rather than the zone that was specified. Even
> though connection tracking failed, this field should be updated with the
> value that the action specified. Fix the issue.
> 
> Fixes: 7f8a436eaa2c ("openvswitch: Add conntrack action")
> Signed-off-by: Joe Stringer <joe@ovn.org>

Applied and queued up for -stable, thanks.

Re: [PATCH net 1/2] openvswitch: Fix helper reference leak

[Joe Stringer]

On 9 December 2015 at 15:33, Pravin Shelar <pshelar@nicira.com> wrote:
> On Wed, Dec 9, 2015 at 3:10 PM, Joe Stringer <joe@ovn.org> wrote:
>> On 9 December 2015 at 14:50, Pravin Shelar <pshelar@nicira.com> wrote:
>>> On Wed, Dec 9, 2015 at 2:07 PM, Joe Stringer <joe@ovn.org> wrote:
>>>> If the actions (re)allocation fails, or the actions list is larger than the
>>>> maximum size, and the conntrack action is the last action when these
>>>> problems are hit, then references to helper modules may be leaked. Fix
>>>> the issue.
>>>>
>>>> Fixes: cae3a2627520 ("openvswitch: Allow attaching helpers to ct action")
>>>> Signed-off-by: Joe Stringer <joe@ovn.org>
>>>> ---
>>>>  net/openvswitch/conntrack.c | 9 ++++++++-
>>>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
>>>> index c2cc11168fd5..585a5aa81f89 100644
>>>> --- a/net/openvswitch/conntrack.c
>>>> +++ b/net/openvswitch/conntrack.c
>>>> @@ -53,6 +53,8 @@ struct ovs_conntrack_info {
>>>>         struct md_labels labels;
>>>>  };
>>>>
>>>> +static void __ovs_ct_free_action(struct ovs_conntrack_info *ct_info);
>>>> +
>>>>  static u16 key_to_nfproto(const struct sw_flow_key *key)
>>>>  {
>>>>         switch (ntohs(key->eth.type)) {
>>>> @@ -708,7 +710,7 @@ int ovs_ct_copy_action(struct net *net, const struct nlattr *attr,
>>>>         nf_conntrack_get(&ct_info.ct->ct_general);
>>>
>>> I see another issue here. Updates to ct_info are done after it is
>>> copied to action list. ct action on the action list and ct_info are
>>> inconsistent, So it is still leaking nf-conntrack reference.
>>
>> You're referring to the below?
>>
>>          __set_bit(IPS_CONFIRMED_BIT, &ct_info.ct->status);
>>          nf_conntrack_get(&ct_info.ct->ct_general);
>>
>> ct_info.ct points to the same location as the version that has been
>> copied into the action list. I agree it's a bit misleading though. If
>> you're not seeing something else, it seems cosmetic so I can (should?)
>> follow up on net-next.
>
> Right, there is no leak, cosmetic changes can be done later. I do not
> have problem with this patch as it is.
>
> Acked-by: Pravin B Shelar <pshelar@nicira.com>

Turns out that there wasn't a leak on nf-conntrack, but on the
template. I sent a patch.