Add Support for Geneve (RFC8926)

[pouria]

Summary

if_geneve: Add geneve support (RFC8926)
geneve creates a generic network virtualization tunnel
interface for Tentant Systems over an L3 (IP/UDP) underlay network that
provides a Layer 2 (ethernet) or Layer 3 service using the geneve protocol.
This implementation is based on RFC8926.

• IPv4 and IPv6 is fully supported for both unicast and multicast underlay.
• Per-VNET geneve tunnel is implemented.
• RXCSUM/TXCSUM/TSO offloading capabilities are implemented.
• NETLINK/WITHOUT_NETLINK are fully supported.

parameters.

Test Plan

see D55183

[pouria]

Resolve PEP8 test warnings for netlink_route.py

[pouria]

Fix lint warning on if_geneve.sh test and remove changes to netlink_route.py to avoid error that are not mine

[glebius]

sys/net/if_clone.c

543 ↗

(On Diff #167856)

This looks like a bugfix to if_clone that is not related to Geneve. Is that correct?

sys/net/if_geneve.c

134

Can this be different to interface's vnet?

192

Can that be different to sc->gnv_ifp->if_vnet?

192

Can this be different to sc->gnv_ifp->if_vnet?

1676

I'd recommend to use some internal flag in softc for this purpose. The IFF_DRV_RUNNING is idea from the early SMP work on the network stack that did not pan out, IMHO. Just don't use it in new interface implementations.

2931

Are we compiling this file at all if neither INET and INET6 are defined? You have added glue to sys/conf/files correctly. So why do you need this (and other below) ifdef?

3176

Are we always sending with zero ip_id, even when IP_DF is not set? IMHO, ip_fillid() needs to be called, unless this is intentional.

3181

mcast can be bool and assigned as mcast = (m->m_flags & (M_MCAST | M_BCAST))

[pouria]

sys/net/if_clone.c

543 ↗

(On Diff #167856)

Yes, I asked about the reason in the original revision D39032 .
I will split this change into a separate revision.

sys/net/if_geneve.c

134

Yes, it will be used in geneve_socket_lookup to make sure we have the right vnet without any knowledge about interface.
However, after fixing the ucred of thread in D54109, this shouldn't be the case any more, so->so_vnet will be correctly set by socreate. I will remove it right away.

192

No, you're right, I can replace it with gnv_ifp, but I will remove because of fix on ucred. thank you

1676

sys/net/if_ethersubr.c use it for ether_output and ether_input_internal functions and my geneve l2 mode depends on those functions. AFAICU, It requires changes to ethersubr and its dependent modules. Correct me if I'm wrong, but I can look at it on a separate revision that is the case.

2931

The protocol itself does not require to be dependent on the INET or INET6 macros and other interfaces like if_vxlan do the same despite gluing. However, it makes no sense to me either if someone use GENEVE protocol without supporting one of the INET or INET6.
I prefer to remove it myself too, I simply followed others' footsteps on interface modules.
If that's the case, I will remove it.

3176

ip_output.c:370 will set it if we don't set the IP_FORWARDING and IP_RAWOUTPUT flags and I didn't. Please correct me if I'm wrong.

3181

Done.

[pouria]

• if_clone change is separated into D54190
• all the INET and INET6 macros are removed.
• remove vnet members from sc and gnvso due to fix on D54109
• replace int mcast with bool mcast

[glebius]

sys/net/if_geneve.c

1676

Right. But ether_output() doesn't know about geneve internal locking and checks the flag in a racy manner. This idea of a shared flag between drivers and stack is from the very early days of SMPng and IMHO it was incorrect in principle and it definitely doesn't work right now due to Ethernet layer not using internal driver locking. This is true for any driver.

A properly written driver today will not panic if Ethernet layer calls into it at any point after if_attach().

3176

You are correct, thanks!

[pouria]

sys/net/if_geneve.c

1676

So, to check if I'm understanding correctly:

1. In order to make it work I have to set the IFF_DRV_RUNNING flag anyway, but instead of geneve_init I must do this at geneve_clone_create point to make the ether layer believe that the interface is always ready and then forget about the IFF_DRV_RUNNING flag until ifdetach happens.
2. Then I should use a separate internal flag for my own uses?

Am I understand correctly?

[glebius]

sys/net/if_geneve.c

1676

Yes, exactly that. You may set it in geneve_init() as well, doesn't really matter. But there is no sense in ever clearing the flag, as it doesn't give a reliable protection against entering the driver with the flag cleared.

[pouria]

• Replace IFF_DRV_RUNNING with GENEVE_FLAG_RUNNING, address @glebius note

[zlei]

sys/net/if_geneve.c

1676

I'm recently fighting with ifp->if_drv_flags & IFF_DRV_RUNNING. I think the flag should ( if ever wanted ) be write only by the driver and protected by driver locks, but not by the net stack. For sync issue, the net stack should use atomic_load but not merely a test by ifp->if_drv_flags & IFF_DRV_RUNNING.

[glebius]

sys/net/if_geneve.c

1676

IMHO, the idea of the flag was not correct in the first place. A new properly written driver shall not rely on it as a measure to prevent the stack from talking to the driver.

[pouria]

Rebase and update according to 0bd0c3295ac09f759f2816b73cbd2d950e3bef7e .

• There is no ether_reassign, therefore, geneve_reassign removed for good.

All geneve tests are passed and reassignment is also tested.

[pouria]

Fix ability to modifying generic and link-specific attributes at the same time in geneve_clone_modify_nl

[pouria]

sbin/ifconfig/ifgeneve.c

233 ↗

(On Diff #168865)

D54443: @zlei
This is an example of netlink implementation of ifconfig for geneve.
For the same reason, we need the nl_parsed_geneve here.
That's why I want the nl_parsed_gre to be public KPI.

[pouria]

I'm going to breakdown this revision to be easier to review.
So, sorry for noise in advance.

[pouria]

Rebase geneve to main and limit this revision kernel only.
ifconfig netlink helper: D55174
ifconfig geneve implementation (netlink): D55184
ifconfig geneve implementation (without netlink): D55185
ifconfig(8) manual for geneve: D55181
geneve(4) manual: D55182
geneve tests: D55183
update geneve to follow RFC 6040: D55186

If you want to test all of the Geneve patches together,
you can find them with all patches applied on my github:
https://github.com/spmzt/freebsd-src/tree/geneve_total

[pouria]

@glebius Do you think removing nvlist/non-netlink support will help this revision get reviewed faster?
since it will reduce the code size.

[glebius]

Sorry if I wasn't clear - I have no objections against pushing this in. I'm not able to do a protocol level review, but wrt the interaction with the rest of the network stack all looks good to me.

P.S. My only comment was about use of IFF_DRV_RUNNING, which IMHO is not a real protection from stack calling into the driver. You can improve that later, though.

[pouria]

Rebase to main, apply and close D55186.

[pouria]

In D54172#1277853, @pouria wrote:

Do you think removing nvlist/non-netlink support will help this revision get reviewed faster?
since it will reduce the code size.

FYI, deleting nvlist/non-netlink support, removes ~550 LOC.