netdev - [PATCH net-next v2 08/11] openvswitch: Free skb(s) on recirculation error

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-Id: <1405444524-17087-1-git-send-email-pshelar@nicira.com>
Date:	Tue, 15 Jul 2014 10:15:24 -0700
From:	Pravin B Shelar <pshelar@...ira.com>
To:	davem@...emloft.net
Cc:	netdev@...r.kernel.org, Simon Horman <horms@...ge.net.au>,
	Jesse Gross <jesse@...ira.com>,
	Pravin B Shelar <pshelar@...ira.com>
Subject: [PATCH net-next v2 08/11] openvswitch: Free skb(s) on recirculation error

From: Simon Horman <horms@...ge.net.au>

This patch attempts to ensure that skb(s) are always freed (once)
if an error occurs in execute_recirc(). It does so in two ways:

1. Ensure that execute_recirc() always consimes skb passed to it.
   Specifically, free the skb if the call to ovs_flow_extract() fails.

2. Return from the recirc case in execute_recirc() whenever
   the skb has not been cloned immediately above.

This is only the case if the action is both the last action and the
keep_skb parameter of execute_recirc is not true.  As it is the last
action and the skb is consumed one way or another by execute_recirc() it
is correct to return here.  In particular this avoids the possibility of
the skb, which has been consumed by execute_recirc() from being freed.

Conversely if this is not the case then the skb has been cloned
and the clone has been consumed by execute_recirc().
This leads to three sub-cases:
* If execute_recirc() returned an error code then the original skb
  will be freed by the error handling code below the case statement in
  do_execute_actions().
* If this is not the last action then action processing will continue,
  using the original skb.
* If this is the last action then it must also be the case that keep_skb
  is true (otherwise the skb would not have been cloned). Thus
  do_execute_actions() will return without freeing the original skb.

Signed-off-by: Simon Horman <horms@...ge.net.au>
Signed-off-by: Jesse Gross <jesse@...ira.com>
Signed-off-by: Pravin B Shelar <pshelar@...ira.com>
---
 net/openvswitch/actions.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 8875697..f131d69 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -529,8 +529,10 @@ static int execute_recirc(struct datapath *dp, struct sk_buff *skb,
 	int err;

 	err = ovs_flow_extract(skb, p->port_no, &recirc_key);
-	if (err)
+	if (err) {
+		kfree_skb(skb);
 		return err;
+	}

 	recirc_key.ovs_flow_hash = hash;
 	recirc_key.recirc_id = nla_get_u32(a);
@@ -602,7 +604,7 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
 			err = execute_recirc(dp, recirc_skb, a);
 			__this_cpu_dec(net_xmit_recursion);

-			if (last_action || err)
+			if (recirc_skb == skb)
 				return err;

 			break;
-- 
1.9.3

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html