diff mbox

[net,v3] ovs: limit ovs recursions in ovs_execute_actions to not corrupt stack

Message ID 1452868427-5655-1-git-send-email-hannes@stressinduktion.org
State Superseded, archived
Delegated to: David Miller
Headers show

Commit Message

Hannes Frederic Sowa Jan. 15, 2016, 2:33 p.m. UTC 
It was seen that defective configurations of openvswitch could overwrite
the STACK_END_MAGIC and cause a hard crash of the kernel because of too
many recursions within ovs.

This problem arises due to the high stack usage of openvswitch. The rest
of the kernel is fine with the current limit of 10 (RECURSION_LIMIT).

We use the already existing recursion counter in ovs_execute_actions to
implement an upper bound of 5 recursions.

Cc: Pravin Shelar <pshelar@ovn.org>
Cc: Simon Horman <simon.horman@netronome.com>
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
Reviewed-by: Simon Horman <simon.horman@netronome.com>
---
v2) added preemption guards
v3) Pravin suggested to reuse the ovs_execute_actions counter which this
    patch does. Also only allow 5 recursions as suggested by Pravin.

 net/openvswitch/actions.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

Comments

Eric Dumazet Jan. 15, 2016, 4:45 p.m. UTC | #1 
On Fri, 2016-01-15 at 15:33 +0100, Hannes Frederic Sowa wrote:
> It was seen that defective configurations of openvswitch could overwrite
> the STACK_END_MAGIC and cause a hard crash of the kernel because of too
> many recursions within ovs.

...

> +
> +	preempt_disable();
> +	level = __this_cpu_inc_return(exec_actions_level);
> +	if (level > ovs_recursion_limit) {

if (unlikely(level > ovs_recursion_limit)) {
Hannes Frederic Sowa Jan. 15, 2016, 4:51 p.m. UTC | #2 
On 15.01.2016 17:45, Eric Dumazet wrote:
> On Fri, 2016-01-15 at 15:33 +0100, Hannes Frederic Sowa wrote:
>> It was seen that defective configurations of openvswitch could overwrite
>> the STACK_END_MAGIC and cause a hard crash of the kernel because of too
>> many recursions within ovs.
>
> ...
>
>> +
>> +	preempt_disable();
>> +	level = __this_cpu_inc_return(exec_actions_level);
>> +	if (level > ovs_recursion_limit) {
>
> if (unlikely(level > ovs_recursion_limit)) {
>

Thanks, I added it!

Bye,
Hannes
diff mbox

Patch

diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index c88d0f2d3e019b..9d8a09fd716abe 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -1160,17 +1160,28 @@  int ovs_execute_actions(struct datapath *dp, struct sk_buff *skb,
 			const struct sw_flow_actions *acts,
 			struct sw_flow_key *key)
 {
-	int level = this_cpu_read(exec_actions_level);
-	int err;
+	static const int ovs_recursion_limit = 5;
+	int err, level;
+
+	preempt_disable();
+	level = __this_cpu_inc_return(exec_actions_level);
+	if (level > ovs_recursion_limit) {
+		net_crit_ratelimited("ovs: recursion limit reached on datapath %s, probable configuration error\n",
+				     ovs_dp_name(dp));
+		kfree_skb(skb);
+		err = -ENETDOWN;
+		goto out;
+	}
 
-	this_cpu_inc(exec_actions_level);
 	err = do_execute_actions(dp, skb, key,
 				 acts->actions, acts->actions_len);
 
-	if (!level)
+	if (level == 1)
 		process_deferred_actions(dp);
 
-	this_cpu_dec(exec_actions_level);
+out:
+	__this_cpu_dec(exec_actions_level);
+	preempt_enable();
 	return err;
 }