Page MenuHomeFreeBSD

Add redirects to the fast forwarding path
ClosedPublic

Authored by gnn on Nov 10 2020, 9:59 PM.

Details

Reviewers
ae
melifaro
Summary

An earlier commit effectively turned out the fast forwading path due to its lack of support for ICMP redirects. The following commit adds redirects to the fastforward path, again allowing for decent forwarding performance in the kernel.

Sponsored by: Rubicon Communications, LLC (d/b/a "Netgate")

Test Plan

Tested using 3 virtual machines in the following configuration

In order to do this we really need 3 hosts, two routers and one
client. The client sends to a network known by Router 2, but the
packet initially goes VIA Router 1. Router 1 sends the redirect to
the client. The client has a default router to Router 1. The client
sends a packet to the foreign network (e.g 128.32.0.1) which is then
sent to Router 2, but also a redirect is sent back to the client
giving it a new route 128.32.0.1 -> 192.168.1.2 which means only the
first packet of a stream goes via Router 1. The netstat -s -p ip
command will show the correct stats, one packet fast forwarded and one
redirect sent.

                    +-------------------------------+
                    |                               |
                    |           Router 1            |
                    |                               |
                    |  128.32.0.0/24 -> 192.168.1.2 |
                    |                               |
                    |                               |
                    +----------------+--------------+
                                     |
                      192.168.1.1    |
                                     |
--------------+----------------------+--------------------+------
              |                                           |
   +----------+-------------+                    +-------->--------+
   |    192.168.1.100       |                    |  192.168.1.2    |
   |                        |                    |                 |
   |     Client             |                    |   Router 2      |
   |                        |                    |                 |
   | default -> 192.168.1.1 |                    |                 |
   +------------------------+                    +-----------------+

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 34764
Build 31816: arc lint + arc unit

Event Timeline

gnn requested review of this revision.Nov 10 2020, 9:59 PM
sys/netinet/ip_fastfwd.c
431

Would it be possible to move all of the code under condition here in a separate function?

sys/netinet/ip_input.c
114–118

Any chance this variable finally gets a better comment? :-)

Address the comment about the comment.

sys/netinet/ip_fastfwd.c
431

I'd prefer to do that both here and in ip_input, in a followup, cleanup change.

sys/netinet/ip_fastfwd.c
431

Given the change is mostly about adding this piece of code, it would be beneficial to start with "cleaned up" version :-)

sys/netinet/ip_fastfwd.c
113

It is already defined in ip_var.h.

443

NULL pointer dereference is possible here in case, when m_gethdr() returns NULL.

447

It seem nh here can not be NULL.

gnn marked 3 inline comments as done.Nov 12 2020, 4:11 PM
sys/netinet/ip_fastfwd.c
113

It is declared in ip_var.h but we need to access it here so we have this #define.

Address melifaro@ request to move code into a function.

gnn marked an inline comment as done.Nov 12 2020, 4:50 PM

Thank you for addressing the comments, LGTM!

sys/netinet/ip_fastfwd.c
135

Nit: nh is always != NULL, as we return if ip_findroute() returns error.

This revision is now accepted and ready to land.Nov 12 2020, 8:13 PM

Committed revision 367628.

sys/netinet/ip_fastfwd.c
113

I meant it is already defined in ip_var.h and ip_var.h is included in this file.

121

We still going to the panic in case when m_gethdr() returns NULL.
This condition shoud be

if (mcopy == NULL || m_dup_pkthdr() == 0)