Page MenuHomeFreeBSD

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

Authored by zlei on Nov 3 2021, 12:17 PM.
Referenced Files
Unknown Object (File)
Feb 24 2024, 1:57 PM
Unknown Object (File)
Feb 14 2024, 4:30 AM
Unknown Object (File)
Jan 28 2024, 4:30 PM
Unknown Object (File)
Jan 28 2024, 4:30 PM
Unknown Object (File)
Jan 28 2024, 4:30 PM
Unknown Object (File)
Jan 28 2024, 4:30 PM
Unknown Object (File)
Jan 28 2024, 4:30 PM
Unknown Object (File)
Jan 20 2024, 1:28 AM

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
Lint Skipped
Unit
Tests Skipped

Event Timeline

zlei requested review of this revision.Nov 3 2021, 12:17 PM
zlei 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 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.

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.

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.

@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 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 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–282

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

sbin/ifconfig/ifconfig.8
280–283

Thanks.

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

Rebased on latest main branch.

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

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

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 marked 2 inline comments as done.Jul 1 2022, 1:48 PM
zlei added inline comments.
sys/net/if_vxlan.c
2383

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

Perfect!

2740–2744

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.

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

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

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

Ping @melifaro .