Message-ID: <CAKgT0Uc5Ba3Vno39KqdBRSXGYpyuGHeyef9=CkthoVkWipLS7g@mail.gmail.com>
Date:   Mon, 28 Oct 2019 08:54:08 -0700
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     Ttttabcd <ttttabcd@...tonmail.com>
Cc:     "alexander.h.duyck@...hat.com" <alexander.h.duyck@...hat.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "idosch@...lanox.com" <idosch@...lanox.com>,
        Netdev <netdev@...r.kernel.org>,
        "lartc@...r.kernel.org" <lartc@...r.kernel.org>
Subject: Re: Commit 0ddcf43d5d4a causes the local table to conflict with the
 main table

On Sat, Oct 26, 2019 at 12:10 AM Ttttabcd <ttttabcd@...tonmail.com> wrote:

I am dropping all of the above. The fact is commit 0ddcf43d5d4a
("ipv4: FIB Local/MAIN table collapse") has been in the kernel for
over 4 and a half years so hopefully by now most of the network
developers are aware that the tables are merged, and become unmerged
when custom rules are added.

<snip>

> Suppose now that a software engineer wants to add 192.168.0.0/16 to the local address, so he naturally executed the following command.
>
> ip route add local 192.168.0.0/16 dev lo
>
> But he does not know that there is a trap in the main table, another overlapping route!

So the first question I would have is why is the developer
intentionally overlapping the routes and creating duplicate routes?
For most users any address other than the localhost route is always a
/32 when it comes to local routing. Why is there a need to create
another route that is looped back into the system?

> ip route
> 192.168.3.0/24 dev wlan0 proto kernel scope link src 192.168.3.62 metric 600
>
> ip route get 192.168.1.100
> local 192.168.1.100 dev lo src 192.168.1.100 uid 0
>     cache <local>
>
> ip route get 192.168.3.100
> 192.168.3.100 dev wlan0 src 192.168.3.62 uid 0
>     cache
>
> This will cause the entire network of 192.168.3.0/24 not to be routed to the local lo interface as he thought!

I agree this is the behavior. However it muddles the routing tables as
you have overlapping entries in the first place. As it stands doesn't
this effectively render 192.168.0.0/16 a blackhole since what you end
up with is it trying to loop back packets that will be recognized as
something from the local system anyway?

> This will lead to bugs that are very difficult to find! If I am not a programmer who knows a little about the kernel implementation, I can't find out what caused the problem (I checked a lot of source code and read a lot of patches and kernel mail to solve this problem).
>
> Of course, you can say that no one will modify the local routing table under normal circumstances. But is the linux system also designed for geeks? Is it also designed for programmers who want to exploit the full potential of the system?

I would argue that adding routes to the local table is a very "geek"
thing to do. Normally people aren't going to be adding routes there in
the first place. Normally routes are added to main as the assumption
is you are attempting to route traffic out to some external entity.
The local table normally only consists of the localhost route and a
set of /32 addresses as I mentioned earlier.

> If you provide a mechanism to modify the local table, you must ensure that the mechanism is working correctly.
>
> And it's absolutely impossible to make this mechanism a significant difference in the execution process after triggering some incredible conditions (custom FIB rules are enabled, even if they are later disabled).
>
> In summary, I don't think this Commit 0ddcf43d5d4a is a good idea.
>
> Welcome to discuss.

I would disagree. There is a significant performance gain to be had
from this commit. What it is basically doing is taking advantage of
the fact that normally your local trie is going to consist of /32
routes and localhost. In the vast majority of cases there will never
be a clash as you have pointed out.

If anything what I would suggest, assuming the priority inversion
issue you have pointed out needs to be addressed, would be to look at
adding a special case for the addition of non /32 non-localhost routes
to local so that we could unmerge the table on such an addition. The
instance of non-/32 routes being added to local has apparently been
low enough that this hasn't been an issue up until now, and if we are
wanting to focus on the correctness of the fib lookup then this would
be the easiest way to sort it out while maintaining both the
performance and correctness.

Thanks.

- Alex

Powered by blists - more mailing lists