vxlan: Add support for socket ioctls SIOC[SG]TUNFIB

[zlei]

Summary

Submitted by: Luiz Amaral <[e-mail]>
PR: 244004

Test Plan

Prepare fibs:

# sysctl net.fibs=3
# sysctl net | grep fibs
net.fibs: 3
net.add_addr_allfibs: 0

1. Verify that SIOCSTUNFIB and SIOCGTUNFIB work

# setfib 1 ifconfig vxlan0 create
# ifconfig vxlan0 | grep tunnelfib
	tunnelfib: 1
# ifconfig vxlan0 tunnelfib 2
# ifconfig vxlan0 | grep tunnelfib
	tunnelfib: 2
# ifconfig vxlan0 destroy

2. Verify packets are encapsulated and forwarded as intended

# ifconfig epair0 create
epair0a
# ifconfig epair0a inet 192.168.100.1/24 fib 1
# ifconfig epair0b inet 192.168.100.2/24 fib 2
# ifconfig vxlan0 create vxlanid 108 vxlanlocal 192.168.100.1 vxlanremote 192.168.100.2 tunnelfib 1
# ifconfig vxlan0 inet 192.0.2.2/24
# ifconfig vxlan0
vxlan0: flags=8843<UP,BROADCAST,RUNNING,SIMPLEX,MULTICAST> metric 0 mtu 1450
	options=80020<JUMBO_MTU,LINKSTATE>
	ether 58:9c:fc:10:ff:bb
	inet 192.0.2.2 netmask 0xffffff00 broadcast 192.0.2.255
	groups: vxlan
	vxlan vni 108 local 192.168.100.1:4789 remote 192.168.100.2:4789
	tunnelfib: 1
	media: Ethernet autoselect (autoselect <full-duplex>)
	status: active
	nd6 options=29<PERFORMNUD,IFDISABLED,AUTO_LINKLOCAL>

# netstat -4rn
Routing tables

Internet:
Destination        Gateway            Flags     Netif Expire
default            192.168.117.2      UGS         em0
127.0.0.1          link#2             UH          lo0
192.0.2.0/24       link#5             U        vxlan0
192.0.2.2          link#5             UHS         lo0
192.168.117.0/24   link#1             U           em0
192.168.117.143    link#1             UHS         lo0

# netstat -4rn -F 1
Routing tables (fib: 1)

Internet:
Destination        Gateway            Flags     Netif Expire
192.168.100.0/24   link#3             U       epair0a
192.168.100.1      link#3             UHS         lo0

# netstat -4rn -F 2
Routing tables (fib: 2)

Internet:
Destination        Gateway            Flags     Netif Expire
192.168.100.0/24   link#4             U       epair0b
192.168.100.2      link#4             UHS         lo0

# ping -c1 192.0.2.3 ;; tcpdump listen on epair0b before pinging

# tcpdump -ni epair0b 'udp port 4789'
tcpdump: verbose output suppressed, use -v or -vv for full protocol decode
listening on epair0b, link-type EN10MB (Ethernet), capture size 262144 bytes
23:53:12.452700 IP 192.168.100.1.29534 > 192.168.100.2.4789: VXLAN, flags [I] (0x08), vni 108
ARP, Request who-has 192.0.2.3 tell 192.0.2.2, length 28

[gbe]

LGTM for the man page part.

[kp]

I'm not familiar with the vxlan code but this does look sane.

Let's give Bryan a few more days, but if he's not had the time for this by let's say Monday ping me to commit this.

Also, if someone would feel called to write a regression test for vxlan in general and this new feature specifically I'd love to see it. Something like the basic test in tests/sys/net/if_vlan.sh would already be valuable. (In fact, when I wrote that test it found a panic in the if_vlan code).

[melifaro]

LGTM, ty!
Any chance you can fill in the "testing" section?

[zlei]

In D32820#741028, @melifaro wrote:

LGTM, ty!
Any chance you can fill in the "testing" section?

Filled the test plan.

[zlei]

In D32820#741027, @kp wrote:

I'm not familiar with the vxlan code but this does look sane.

Let's give Bryan a few more days, but if he's not had the time for this by let's say Monday ping me to commit this.

Also, if someone would feel called to write a regression test for vxlan in general and this new feature specifically I'd love to see it. Something like the basic test in tests/sys/net/if_vlan.sh would already be valuable. (In fact, when I wrote that test it found a panic in the if_vlan code).

I'm glad to do it, but currently if_vxlan is not VNETified and IIUC it is hard to write a regression test for vxlan right now.

During the testing I found a bug. I can confirm it is not side effect of this differential. I'll report later.

[kp]

In D32820#741041, @zlei.huang_gmail.com wrote:

I'm glad to do it, but currently if_vxlan is not VNETified and IIUC it is hard to write a regression test for vxlan right now.

Yeah, that's true. If it's not VNET-ready you can't (easily) write tests for it.

[bryanv]

Left a few comments. It has been a long while since I've dealt with vxlan - and FreeBSD network stack in general - so somebody more familiar should give a correctness review.

sys/net/if_vxlan.c

2383

error is already initialized before the switch

2760

The softc lock isn't held for this in the ioctl handler - which is probably fine, or at least other tunnelers have the same race - so I don't think this needs to be under the lock.

[zlei]

@bryanv

1. Removed redundant error initialization.
2. Moved M_SETFIB(m, sc->vxl_fibnum) out of read lock

I checked other tunnel implementation ( GIF, GRE, ME ), and I have no clues why the tunnel fib of sc is not read protected.
They does not have per sc locks. They have sx locks to prevent concurrent ioctls.

It seems we are vulnerable to concurrent ioctls.

[zlei]

Sorry my bad, resubmit diff as last one include local WIP stashes.

[pauamma_gundo.com]

Manual page LGTM as well, English-wise. Can't speak for the rest or for consistency.

[zlei]

Protect from concurrent ioctls, and rebase on latest main branch

[zlei]

Ping .

[pauamma_gundo.com]

Sorry, missed this one earlier.

sbin/ifconfig/ifconfig.8

221

280–282

Is "not" missing in that sentence? It directly contradicts "Disable" above.

289

While here, for consistency with "inet6" below.

293

[email_luiz.eng.br]

sbin/ifconfig/ifconfig.8

280–282

The next sentence says "This flag disables this behavior.".

It describes the default behavior of attempting to load the driver and then explains that the flag disables the given behavior.

[pauamma_gundo.com]

sbin/ifconfig/ifconfig.8

280–283

Thanks.

Rewrite for better flow and clarity. (May need a linebreak.)

[zlei]

Rebased on latest main branch.

[zlei]

D35409 and D35384 have been merged. Let's move on :)

[pauamma_gundo.com]

A few more nits, and https://reviews.freebsd.org/D32820?id=105449#inline-217280 still (and a few more minor nits)

sbin/ifconfig/ifconfig.8

280–283

That should have been

This flag disables
.Nm Ns 's behavior of loading network interface drivers not already present in the kernel.
.It Fl u

(missing line break.)

293

435

448

[melifaro]

Also: maybe it's worth considering splitting this review into two? Most of the ifconfig.8 changes does not look directly related to the review topic.

sys/net/if_vxlan.c

2383

For consistency I'd rather do a read lock here.

2740–2744

I'd rather move it down - so (1) it is protected by the lock and (2) it reflect the reality - we pass the packet to BPF for own transmit, not tunnelled transmit, so the packet should retain the fib.

[zlei]

In D32820#809133, @pauamma_gundo.com wrote:

A few more nits, and https://reviews.freebsd.org/D32820?id=105449#inline-217280 still (and a few more minor nits)

Address them in D35686 .

[zlei]

@melifaro Done!

[zlei]

sys/net/if_vxlan.c

2383

For consistency I'd rather do a read lock here.

Perfect!

2740–2744

Smart!

[zlei]

Ping .

[pauamma_gundo.com]

Nothing left for me to review here since the manual page was addressed elsewhere, I think.

[zlei]

Thanks for the review!

[zlei]

Hi @melifaro ,
Any chance will this be MFCed into stable/13 ?

[zlei]

In D32820#824395, @zlei.huang_gmail.com wrote:

Hi @melifaro ,
Any chance will this be MFCed into stable/13 ?

Ping @melifaro .