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

[Eyal Birger <eyal.birger@gmail.com>]

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.