Page MenuHomeFreeBSD

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

Authored by zlei.huang_gmail.com on Nov 3 2021, 12:17 PM.

Details

Summary

Submitted by: Luiz Amaral <email@luiz.eng.br>
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
  1. 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

Diff Detail

Repository
rG FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

zlei.huang_gmail.com retitled this revision from vxlan: Support socket ioctls SIOC[SG]TUNFIB to vxlan: Add support for socket ioctls SIOC[SG]TUNFIB.Nov 4 2021, 6:55 AM
zlei.huang_gmail.com added a reviewer: manpages.
gbe added a subscriber: gbe.

LGTM for the man page part.

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).

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

This revision is now accepted and ready to land.Nov 4 2021, 8:25 AM

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

Filled the test plan.

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.

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.

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

2762

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.

@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.

This revision now requires review to proceed.Nov 5 2021, 9:39 AM
zlei.huang_gmail.com marked 2 inline comments as done.

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

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

zlei.huang_gmail.com edited the summary of this revision. (Show Details)

Protect from concurrent ioctls, and rebase on latest main branch

Sorry, missed this one earlier.

sbin/ifconfig/ifconfig.8
221
280–283

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

289

While here, for consistency with "inet6" below.

293
This revision now requires changes to proceed.May 20 2022, 11:14 PM
sbin/ifconfig/ifconfig.8
280–283

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.

sbin/ifconfig/ifconfig.8
280–284

Thanks.

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

pauamma requested changes to this revision.Jul 1 2022, 4:39 AM

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–284

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
This revision now requires changes to proceed.Jul 1 2022, 4:39 AM

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.

2742–2746

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.

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.huang_gmail.com added inline comments.
sys/net/if_vxlan.c
2383

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

Perfect!

2742–2746

Smart!

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

This revision was not accepted when it landed; it landed in state Needs Review.Jul 8 2022, 6:19 PM
This revision was automatically updated to reflect the committed changes.