Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ip neighbors are assumed and made routable regardless of actually having a route #364

Closed
KanjiMonster opened this issue Jan 26, 2022 · 0 comments · Fixed by #374
Closed

Comments

@KanjiMonster
Copy link
Contributor

Expected Behavior

Only ip neighbors for which a route exists are being routed (with an appropriate flow).

Actual Behavior

Any reachable ip neighbor is assumed to be routable and flow tables are setup accordingly (and thus routed in hardware), even if there does not exist a route.

Steps to Reproduce the Problem

Consider the following setup

| Server | port1 <-------------> port1 | Switch |
  lo       10.0.1.2/24     10.0.1.1/24
  10.0.2.1/24
switch:~$ ip route
10.0.1.0/24 dev port1 proto kernel scope link src 10.0.1.1 
switch:~$ ip route add 10.0.2.0/24 dev port1
switch:~$ ip route
10.0.1.0/24 dev port1 proto kernel scope link src 10.0.1.1 
10.0.2.0/24 dev port1 scope link
switch:~$ ping 10.0.2.1 -I port1
PING 10.0.2.1 (10.0.2.1): 56 data bytes
64 bytes from 10.0.2.1: seq=0 ttl=64 time=1.126 ms
switch:~$ ip neigh show
10.0.2.1 dev port1 lladdr b0:26:28:62:d4:00 REACHABLE
10.0.1.2 dev port1 lladdr b0:26:28:62:d4:00 STALE
switch:~$ client_flowtable_dump 30 | grep 10.
--  etherType = 0x0800 vrf:mask = 0x0000:0x0000 dstIp4 = 10.0.1.0/255.255.255.0 dstIp6 = ::/:: | GoTo = 60 (ACL Policy) outPort = CONTROLLER (Reserved)  | priority = 2 hard_time = 0 idle_time = 0 cookie = 11
--  etherType = 0x0800 vrf:mask = 0x0000:0x0000 dstIp4 = 10.0.1.2/255.255.255.255 dstIp6 = ::/:: | GoTo = 60 (ACL Policy) groupId = 0x20000001 | priority = 3 hard_time = 0 idle_time = 0 cookie = 14
--  etherType = 0x0800 vrf:mask = 0x0000:0x0000 dstIp4 = 10.0.2.0/255.255.255.0 dstIp6 = ::/:: | GoTo = 60 (ACL Policy) outPort = CONTROLLER (Reserved)  | priority = 2 hard_time = 0 idle_time = 0 cookie = 17
--  etherType = 0x0800 vrf:mask = 0x0000:0x0000 dstIp4 = 10.0.2.1/255.255.255.255 dstIp6 = ::/:: | GoTo = 60 (ACL Policy) groupId = 0x20000001 | priority = 3 hard_time = 0 idle_time = 0 cookie = 16
switch:~$ ip route get 10.0.2.1
10.0.2.1 dev port1 src 10.0.1.1 uid 1000 
    cache 

So far everything's fine.

But if we remove the route, the neighbor stays routed.

switch:~$ ip route del 10.0.2.0/24 dev port1
switch:~$ ip route get 10.0.2.1
RTNETLINK answers: Network is unreachable
switch:~$ client_flowtable_dump 30 | grep 10.0.2.1
--  etherType = 0x0800 vrf:mask = 0x0000:0x0000 dstIp4 = 10.0.2.1/255.255.255.255 dstIp6 = ::/:: | GoTo = 60 (ACL Policy) groupId = 0x20000001 | priority = 3 hard_time = 0 idle_time = 0 cookie = 16

Removing the neighbor removes the forwarding entry:

switch:~$ ip neigh del 10.0.2.1 dev port1
switch:~$ client_flowtable_dump 30 | grep 10.0.2.1
switch:~$

But adding it manually will create a routed entry again, despite it still not being supposed to be routable

switch:~$ ip neigh add 10.0.2.1 lladdr b0:26:28:62:d4:00 dev port1
switch:~$ client_flowtable_dump 30 | grep 10.0.2.1
--  etherType = 0x0800 vrf:mask = 0x0000:0x0000 dstIp4 = 10.0.2.1/255.255.255.255 dstIp6 = ::/:: | GoTo = 60 (ACL Policy) groupId = 0x20000001 | priority = 3 hard_time = 0 idle_time = 0 cookie = 18
switch:~$ ip route get 10.0.2.1
RTNETLINK answers: Network is unreachable

The implication of the groupId being set for that enry is that any traffic with the destination address 10.0.2.1 coming in on a different interface will get routed according to entry, instead of being dropped, and 10.0.2.1 becoming/staying reachable for other connected devices.

To fix this, we would basically need to do the reverse we do for routes and nexthops; only add l3 neighbors as routable / set to forward to an interface if we have a route for them, and wait for a route to be added if not. And the inverse for routes being removed.

Specifications

  • Version: master
  • Platform: ag5648
  • Subsystem: nl_l3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant