| Message ID | 1452876675-19834-1-git-send-email-hannes@stressinduktion.org |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Fri, Jan 15, 2016 at 8:51 AM, Hannes Frederic Sowa <hannes@stressinduktion.org> 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. > > 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> > Cc: Eric Dumazet <eric.dumazet@gmail.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. > v4) added unlikely as suggested by Eric > > Thanks to all reviewers! > > net/openvswitch/actions.c | 21 ++++++++++++++++----- > 1 file changed, 16 insertions(+), 5 deletions(-) > > diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c > index c88d0f2d3e019b..da66f9e1660dbb 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(); This function always run in soft-irq context. Do we really need this preempt-disable call here?
From: pravin shelar <pshelar@ovn.org> Date: Sat, 16 Jan 2016 09:04:50 -0800 > This function always run in soft-irq context. Do we really need this > preempt-disable call here? Hannes, please respond to this feedback. Thanks.
On 18.01.2016 01:26, David Miller wrote: > From: pravin shelar <pshelar@ovn.org> > Date: Sat, 16 Jan 2016 09:04:50 -0800 > >> This function always run in soft-irq context. Do we really need this >> preempt-disable call here? > > Hannes, please respond to this feedback. Sorry, was traveling on the weekend. Pravin, I added preempt protection because of the discussion about this patch: <http://www.spinics.net/lists/linux-rt-users/msg14343.html> I didn't want to add a recursion limiter into struct task_struct for ovs, so I disable preemption here. On non-rt kernels this will have no change in behavior (besides the accesses to the preempt counter), but it does the right thing on -rt kernels. I don't think that -rt kernels are important for openvswitch right now, so this is just a safe guard. Bye, Hannes
From: Hannes Frederic Sowa <hannes@stressinduktion.org> Date: Mon, 18 Jan 2016 12:21:16 +0100 > I don't think that -rt kernels are important for openvswitch right > now, so this is just a safe guard. Hannes, whilst I don't want to explicitly make things harder for the -rt folks, I don't think it's appropriate to put unnecessary preemtption disables into the main kernel like this. Someone reading the code as-is will say this is superfluous and (correctly) submit a patch to remove the preemption disables. Please remove this from your patch, someone can deal with this in the -rt kernel as needed.
On 18.01.2016 17:50, David Miller wrote: > From: Hannes Frederic Sowa <hannes@stressinduktion.org> > Date: Mon, 18 Jan 2016 12:21:16 +0100 > >> I don't think that -rt kernels are important for openvswitch right >> now, so this is just a safe guard. > > Hannes, whilst I don't want to explicitly make things harder for the -rt > folks, I don't think it's appropriate to put unnecessary preemtption > disables into the main kernel like this. > > Someone reading the code as-is will say this is superfluous and (correctly) > submit a patch to remove the preemption disables. > > Please remove this from your patch, someone can deal with this in the > -rt kernel as needed. I am fine with that. Actually I had the same feeling, but seeing such a patch being discussed and explicitly ignoring this issue in another part made me feel a little bit bad. Will resend in some seconds. Bye, Hannes
diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c index c88d0f2d3e019b..da66f9e1660dbb 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 (unlikely(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; }