[PATCH net-next 0/2] Track recursive calls in TC act_mirred

[PATCH net-next 0/2] Track recursive calls in TC act_mirred

[John Hurley]

These patches aim to prevent act_mirred causing stack overflow events from
recursively calling packet xmit or receive functions. Such events can
occur with poor TC configuration that causes packets to travel in loops
within the system.

Florian Westphal advises that a recursion crash and packets looping are
separate issues and should be treated as such. David Miller futher points
out that pcpu counters cannot track the precise skb context required to
detect loops. Hence these patches are not aimed at detecting packet loops,
rather, preventing stack flows arising from such loops.

John Hurley (2):
  net: sched: refactor reinsert action
  net: sched: protect against stack overflow in TC act_mirred

 include/net/pkt_cls.h     |  2 +-
 include/net/sch_generic.h |  2 +-
 net/core/dev.c            |  4 +---
 net/sched/act_mirred.c    | 17 ++++++++++++++++-
 4 files changed, 19 insertions(+), 6 deletions(-)

-- 
2.7.4

[PATCH net-next 1/2] net: sched: refactor reinsert action

[John Hurley]

The TC_ACT_REINSERT return type was added as an in-kernel only option to
allow a packet ingress or egress redirect. This is used to avoid
unnecessary skb clones in situations where they are not required. If a TC
hook returns this code then the packet is 'reinserted' and no skb consume
is carried out as no clone took place.

This return type is only used in act_mirred. Rather than have the reinsert
called from the main datapath, call it directly in act_mirred. Instead of
returning TC_ACT_REINSERT, change the type to the new TC_ACT_CONSUMED
which tells the caller that the packet has been stolen by another process
and that no consume call is required.

Moving all redirect calls to the act_mirred code is in preparation for
tracking recursion created by act_mirred.

Signed-off-by: John Hurley <john.hurley@netronome.com>
Reviewed-by: Simon Horman <simon.horman@netronome.com>
---
 include/net/pkt_cls.h     | 2 +-
 include/net/sch_generic.h | 2 +-
 net/core/dev.c            | 4 +---
 net/sched/act_mirred.c    | 3 ++-
 4 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index 720f2b3..1a7596b 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -10,7 +10,7 @@
 #include <net/net_namespace.h>
 
 /* TC action not accessible from user space */
-#define TC_ACT_REINSERT		(TC_ACT_VALUE_MAX + 1)
+#define TC_ACT_CONSUMED		(TC_ACT_VALUE_MAX + 1)
 
 /* Basic packet classifier frontend definitions. */
 
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 21f434f..855167b 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -279,7 +279,7 @@ struct tcf_result {
 		};
 		const struct tcf_proto *goto_tp;
 
-		/* used by the TC_ACT_REINSERT action */
+		/* used in the skb_tc_reinsert function */
 		struct {
 			bool		ingress;
 			struct gnet_stats_queue *qstats;
diff --git a/net/core/dev.c b/net/core/dev.c
index d6edd21..5852931 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4689,9 +4689,7 @@ sch_handle_ingress(struct sk_buff *skb, struct packet_type **pt_prev, int *ret,
 		__skb_push(skb, skb->mac_len);
 		skb_do_redirect(skb);
 		return NULL;
-	case TC_ACT_REINSERT:
-		/* this does not scrub the packet, and updates stats on error */
-		skb_tc_reinsert(skb, &cl_res);
+	case TC_ACT_CONSUMED:
 		return NULL;
 	default:
 		break;
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 58e7573d..8c1d736 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -277,7 +277,8 @@ static int tcf_mirred_act(struct sk_buff *skb, const struct tc_action *a,
 		if (use_reinsert) {
 			res->ingress = want_ingress;
 			res->qstats = this_cpu_ptr(m->common.cpu_qstats);
-			return TC_ACT_REINSERT;
+			skb_tc_reinsert(skb, res);
+			return TC_ACT_CONSUMED;
 		}
 	}
 
-- 
2.7.4

[PATCH net-next 2/2] net: sched: protect against stack overflow in TC act_mirred

[John Hurley]

TC hooks allow the application of filters and actions to packets at both
ingress and egress of the network stack. It is possible, with poor
configuration, that this can produce loops whereby an ingress hook calls
a mirred egress action that has an egress hook that redirects back to
the first ingress etc. The TC core classifier protects against loops when
doing reclassifies but there is no protection against a packet looping
between multiple hooks and recursively calling act_mirred. This can lead
to stack overflow panics.

Add a per CPU counter to act_mirred that is incremented for each recursive
call of the action function when processing a packet. If a limit is passed
then the packet is dropped and CPU counter reset.

Note that this patch does not protect against loops in TC datapaths. Its
aim is to prevent stack overflow kernel panics that can be a consequence
of such loops.

Signed-off-by: John Hurley <john.hurley@netronome.com>
Reviewed-by: Simon Horman <simon.horman@netronome.com>
---
 net/sched/act_mirred.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 8c1d736..c3fce36 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -27,6 +27,9 @@
 static LIST_HEAD(mirred_list);
 static DEFINE_SPINLOCK(mirred_list_lock);
 
+#define MIRRED_RECURSION_LIMIT    4
+static DEFINE_PER_CPU(unsigned int, mirred_rec_level);
+
 static bool tcf_mirred_is_act_redirect(int action)
 {
 	return action == TCA_EGRESS_REDIR || action == TCA_INGRESS_REDIR;
@@ -210,6 +213,7 @@ static int tcf_mirred_act(struct sk_buff *skb, const struct tc_action *a,
 	struct sk_buff *skb2 = skb;
 	bool m_mac_header_xmit;
 	struct net_device *dev;
+	unsigned int rec_level;
 	int retval, err = 0;
 	bool use_reinsert;
 	bool want_ingress;
@@ -217,6 +221,14 @@ static int tcf_mirred_act(struct sk_buff *skb, const struct tc_action *a,
 	int m_eaction;
 	int mac_len;
 
+	rec_level = __this_cpu_inc_return(mirred_rec_level);
+	if (unlikely(rec_level > MIRRED_RECURSION_LIMIT)) {
+		net_warn_ratelimited("Packet exceeded mirred recursion limit on dev %s\n",
+				     netdev_name(skb->dev));
+		__this_cpu_dec(mirred_rec_level);
+		return TC_ACT_SHOT;
+	}
+
 	tcf_lastuse_update(&m->tcf_tm);
 	bstats_cpu_update(this_cpu_ptr(m->common.cpu_bstats), skb);
 
@@ -278,6 +290,7 @@ static int tcf_mirred_act(struct sk_buff *skb, const struct tc_action *a,
 			res->ingress = want_ingress;
 			res->qstats = this_cpu_ptr(m->common.cpu_qstats);
 			skb_tc_reinsert(skb, res);
+			__this_cpu_dec(mirred_rec_level);
 			return TC_ACT_CONSUMED;
 		}
 	}
@@ -293,6 +306,7 @@ static int tcf_mirred_act(struct sk_buff *skb, const struct tc_action *a,
 		if (tcf_mirred_is_act_redirect(m_eaction))
 			retval = TC_ACT_SHOT;
 	}
+	__this_cpu_dec(mirred_rec_level);
 
 	return retval;
 }
-- 
2.7.4

Re: [PATCH net-next 2/2] net: sched: protect against stack overflow in TC act_mirred

[Eyal Birger]

Hi John,

On Mon, 24 Jun 2019 23:13:36 +0100
John Hurley <john.hurley@netronome.com> wrote:

> TC hooks allow the application of filters and actions to packets at
> both ingress and egress of the network stack. It is possible, with
> poor configuration, that this can produce loops whereby an ingress
> hook calls a mirred egress action that has an egress hook that
> redirects back to the first ingress etc. The TC core classifier
> protects against loops when doing reclassifies but there is no
> protection against a packet looping between multiple hooks and
> recursively calling act_mirred. This can lead to stack overflow
> panics.
> 
> Add a per CPU counter to act_mirred that is incremented for each
> recursive call of the action function when processing a packet. If a
> limit is passed then the packet is dropped and CPU counter reset.
> 
> Note that this patch does not protect against loops in TC datapaths.
> Its aim is to prevent stack overflow kernel panics that can be a
> consequence of such loops.
> 
> Signed-off-by: John Hurley <john.hurley@netronome.com>
> Reviewed-by: Simon Horman <simon.horman@netronome.com>
> ---
>  net/sched/act_mirred.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
> index 8c1d736..c3fce36 100644
> --- a/net/sched/act_mirred.c
> +++ b/net/sched/act_mirred.c
> @@ -27,6 +27,9 @@
>  static LIST_HEAD(mirred_list);
>  static DEFINE_SPINLOCK(mirred_list_lock);
>  
> +#define MIRRED_RECURSION_LIMIT    4

Could you increase the limit to maybe 6 or 8? I am aware of cases where
mirred ingress is used for cascading several layers of logical network
interfaces and 4 seems a little limiting.

Thanks,
Eyal.

Re: [PATCH net-next 2/2] net: sched: protect against stack overflow in TC act_mirred

[John Hurley]

On Tue, Jun 25, 2019 at 9:30 AM Eyal Birger <eyal.birger@gmail.com> wrote:
>
> Hi John,
>
> On Mon, 24 Jun 2019 23:13:36 +0100
> John Hurley <john.hurley@netronome.com> wrote:
>
> > TC hooks allow the application of filters and actions to packets at
> > both ingress and egress of the network stack. It is possible, with
> > poor configuration, that this can produce loops whereby an ingress
> > hook calls a mirred egress action that has an egress hook that
> > redirects back to the first ingress etc. The TC core classifier
> > protects against loops when doing reclassifies but there is no
> > protection against a packet looping between multiple hooks and
> > recursively calling act_mirred. This can lead to stack overflow
> > panics.
> >
> > Add a per CPU counter to act_mirred that is incremented for each
> > recursive call of the action function when processing a packet. If a
> > limit is passed then the packet is dropped and CPU counter reset.
> >
> > Note that this patch does not protect against loops in TC datapaths.
> > Its aim is to prevent stack overflow kernel panics that can be a
> > consequence of such loops.
> >
> > Signed-off-by: John Hurley <john.hurley@netronome.com>
> > Reviewed-by: Simon Horman <simon.horman@netronome.com>
> > ---
> >  net/sched/act_mirred.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> >
> > diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
> > index 8c1d736..c3fce36 100644
> > --- a/net/sched/act_mirred.c
> > +++ b/net/sched/act_mirred.c
> > @@ -27,6 +27,9 @@
> >  static LIST_HEAD(mirred_list);
> >  static DEFINE_SPINLOCK(mirred_list_lock);
> >
> > +#define MIRRED_RECURSION_LIMIT    4
>
> Could you increase the limit to maybe 6 or 8? I am aware of cases where
> mirred ingress is used for cascading several layers of logical network
> interfaces and 4 seems a little limiting.
>
> Thanks,
> Eyal.

Hi Eyal,
The value of 4 is basically a revert to what it was on older kernels
when TC had a TTL value in the skb:
https://elixir.bootlin.com/linux/v3.19.8/source/include/uapi/linux/pkt_cls.h#L97

I also found with my testing that a value greater than 4 was sailing
close to the edge.
With a larger value (on my system anyway), I could still trigger a
stack overflow here.
I'm not sure on the history of why a value of 4 was selected here but
it seems to fall into line with my findings.
Is there a hard requirement for >4 recursive calls here?

John

Re: [PATCH net-next 2/2] net: sched: protect against stack overflow in TC act_mirred

[Florian Westphal]

John Hurley <john.hurley@netronome.com> wrote:
> Hi Eyal,
> The value of 4 is basically a revert to what it was on older kernels
> when TC had a TTL value in the skb:
> https://elixir.bootlin.com/linux/v3.19.8/source/include/uapi/linux/pkt_cls.h#L97

IIRC this TTL value was not used ever.

> I also found with my testing that a value greater than 4 was sailing
> close to the edge.
> With a larger value (on my system anyway), I could still trigger a
> stack overflow here.
> I'm not sure on the history of why a value of 4 was selected here but
> it seems to fall into line with my findings.
> Is there a hard requirement for >4 recursive calls here?

One alternative would be to (instead of dropping the skb), to
decrement the ttl and use netif_rx() instead.

Re: [PATCH net-next 2/2] net: sched: protect against stack overflow in TC act_mirred

[John Hurley]

On Tue, Jun 25, 2019 at 10:15 AM Florian Westphal <fw@strlen.de> wrote:
>
> John Hurley <john.hurley@netronome.com> wrote:
> > Hi Eyal,
> > The value of 4 is basically a revert to what it was on older kernels
> > when TC had a TTL value in the skb:
> > https://elixir.bootlin.com/linux/v3.19.8/source/include/uapi/linux/pkt_cls.h#L97
>
> IIRC this TTL value was not used ever.

It was used to carry out this looping check on ingress redirects:
https://elixir.bootlin.com/linux/v3.19.8/source/net/core/dev.c#L3468
It appears this was removed/unused after changes in 4.2


>
> > I also found with my testing that a value greater than 4 was sailing
> > close to the edge.
> > With a larger value (on my system anyway), I could still trigger a
> > stack overflow here.
> > I'm not sure on the history of why a value of 4 was selected here but
> > it seems to fall into line with my findings.
> > Is there a hard requirement for >4 recursive calls here?
>
> One alternative would be to (instead of dropping the skb), to
> decrement the ttl and use netif_rx() instead.

Yes, this seems like something worth investigating.
Thanks

Re: [PATCH net-next 2/2] net: sched: protect against stack overflow in TC act_mirred

[Jamal Hadi Salim]

On 2019-06-25 5:06 a.m., John Hurley wrote:
> On Tue, Jun 25, 2019 at 9:30 AM Eyal Birger <eyal.birger@gmail.com> wrote:

> I'm not sure on the history of why a value of 4 was selected here but
> it seems to fall into line with my findings.

Back then we could only loop in one direction (as opposed to two right
now) - so seeing something twice would have been suspect enough,
so 4 seems to be a good number. I still think 4 is a good number.

> Is there a hard requirement for >4 recursive calls here?

I think this is where testcases help (which then get permanently
added in tdc repository). Eyal - if you have a test scenario where
this could be demonstrated it would help.

cheers,
jamal

Re: [PATCH net-next 2/2] net: sched: protect against stack overflow in TC act_mirred

[Eyal Birger]

Hi Jamal, John,

On Tue, 25 Jun 2019 07:24:37 -0400
Jamal Hadi Salim <jhs@mojatatu.com> wrote:

> On 2019-06-25 5:06 a.m., John Hurley wrote:
> > On Tue, Jun 25, 2019 at 9:30 AM Eyal Birger <eyal.birger@gmail.com>
> > wrote:  
> 
> > I'm not sure on the history of why a value of 4 was selected here
> > but it seems to fall into line with my findings.  
> 
> Back then we could only loop in one direction (as opposed to two right
> now) - so seeing something twice would have been suspect enough,
> so 4 seems to be a good number. I still think 4 is a good number.

I think the introduction of mirred ingress affects the 'seeing something
twice is suspicious' paradigm - see below.

> > Is there a hard requirement for >4 recursive calls here?  
> 
> I think this is where testcases help (which then get permanently
> added in tdc repository). Eyal - if you have a test scenario where
> this could be demonstrated it would help.

I don't have a _hard_ requirement for >4 recursive calls.

I did encounter use cases for 2 layers of stacked net devices using TC
mirred ingress. For example, first layer redirects traffic based on
incoming protocol - e.g. some tunneling criterion - and the second
layer redirects traffic based on the IP packet src/dst, something like:

  +-----------+  +-----------+  +-----------+  +-----------+
  |    ip0    |  |    ip1    |  |    ip2    |  |    ip3    |
  +-----------+  +-----------+  +-----------+  +-----------+
          \          /                 \           /
           \        /                   \         /
         +-----------+                 +-----------+
         |   proto0  |                 |   proto1  |
         +-----------+                 +-----------+
                    \                   /
                     \                 /
                        +-----------+
                        |    eth0   |
                        +-----------+

Where packets stem from eth0 and are redirected to the appropriate devices
using mirred ingress redirect with different criteria.
This is useful for example when each 'ip' device is part of a different
routing domain.

There are probably many other ways to do this kind of thing, but using mirred
ingress to demux the traffic provides freedom in the demux criteria while
having the benefit of a netdevice at each node allowing to use tcpdump and
other such facilities.

As such, I was concerned that a hard limit of 4 may be restrictive.

I too think Florian's suggestion of using netif_rx() in order to break
the recursion when limit is met (or always use it?) is a good approach
to try in order not to force restrictions while keeping the stack sane.

Eyal.

Re: [PATCH net-next 0/2] Track recursive calls in TC act_mirred

[Jamal Hadi Salim]

On 2019-06-24 6:13 p.m., John Hurley wrote:
> These patches aim to prevent act_mirred causing stack overflow events from
> recursively calling packet xmit or receive functions. Such events can
> occur with poor TC configuration that causes packets to travel in loops
> within the system.
> 
> Florian Westphal advises that a recursion crash and packets looping are
> separate issues and should be treated as such. David Miller futher points
> out that pcpu counters cannot track the precise skb context required to
> detect loops. Hence these patches are not aimed at detecting packet loops,
> rather, preventing stack flows arising from such loops.

Sigh. So we are still trying to save 2 bits?
John, you said ovs has introduced a similar loop handling code;
Is their approach similar to this? Bigger question: Is this approach
"good enough"?

Not to put more work for you, but one suggestion is to use skb metadata
approach which is performance unfriendly (could be argued to more
correct).

Also: Please consider submitting a test case or two for tdc.

cheers,
jamal

Re: [PATCH net-next 0/2] Track recursive calls in TC act_mirred

[John Hurley]

On Tue, Jun 25, 2019 at 12:18 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> On 2019-06-24 6:13 p.m., John Hurley wrote:
> > These patches aim to prevent act_mirred causing stack overflow events from
> > recursively calling packet xmit or receive functions. Such events can
> > occur with poor TC configuration that causes packets to travel in loops
> > within the system.
> >
> > Florian Westphal advises that a recursion crash and packets looping are
> > separate issues and should be treated as such. David Miller futher points
> > out that pcpu counters cannot track the precise skb context required to
> > detect loops. Hence these patches are not aimed at detecting packet loops,
> > rather, preventing stack flows arising from such loops.
>
> Sigh. So we are still trying to save 2 bits?
> John, you said ovs has introduced a similar loop handling code;
> Is their approach similar to this? Bigger question: Is this approach
> "good enough"?
>

Hi Jamal.
Yes, OvS implements a similar approach to prevent recursion:
https://elixir.bootlin.com/linux/v5.2-rc6/source/net/openvswitch/actions.c#L1530

It was discussed on a previous thread that there are really 2 issues
here (even if it is the same thing that triggers them).
Firstly, the infinite looping of packets caused by poor config and,
secondly, the kernel panic caused by a stack overflow due to the
recursion in use.
These patches target the latter.
I think this approach is good enough to deal with the crashes as it
tracks a packets recursive calls (through act_mirred) on the network
stack.
If the packet is scheduled and releases the CPU then the counter is reset.
The packet may still loop but it will not cause stack overflows.


> Not to put more work for you, but one suggestion is to use skb metadata
> approach which is performance unfriendly (could be argued to more
> correct).
>
> Also: Please consider submitting a test case or two for tdc.
>
> cheers,
> jamal
>

Re: [PATCH net-next 0/2] Track recursive calls in TC act_mirred

[David Miller]

From: John Hurley <john.hurley@netronome.com>
Date: Mon, 24 Jun 2019 23:13:34 +0100

> These patches aim to prevent act_mirred causing stack overflow events from
> recursively calling packet xmit or receive functions. Such events can
> occur with poor TC configuration that causes packets to travel in loops
> within the system.
> 
> Florian Westphal advises that a recursion crash and packets looping are
> separate issues and should be treated as such. David Miller futher points
> out that pcpu counters cannot track the precise skb context required to
> detect loops. Hence these patches are not aimed at detecting packet loops,
> rather, preventing stack flows arising from such loops.

Series applied, thanks.