sockets: avoid layering violation in handling SO_SETFIB

[stevek]

Summary

Setting FIB on socket can be handled by the protocol pr_ctloutput
routine instead. This avoids having protocol domain family-specific
knowledge in generic sockopt handling.

Make the setfib syscall a registered syscall to allow for network
stack as a module functionality (where different network stack
implementations can be loaded instead of just the default FreeBSD
network stack.)

Obtained from: Juniper Networks, Inc.

[stevek]

These changes were tested with the test application in tools/regression/sockets/so_setfib

[stevek]

Removed the whitespace change in sys/net/route.h

[stevek]

This comment was removed by stevek.

[stevek]

The syscall registration can be removed if sys_setfib is moved to sys/kern, which it should be able to be moved. Since the setting is on the thread itself and there's a callback function for the netstack to be able to veto the FIB number, there's no need for it to live in sys/net/route/route_tables.c now.

[stevek]

Moved setfib syscall to uipc_syscalls.c and just call the callback to check validity of the FIB number

[stevek]

Removed includes that are no longer necessary.

[melifaro]

Conceptually it looks good to me. Please see some implementation-level comments - happy to discuss it further.

sys/kern/uipc_socket.c

198

Are these the only two functions that are required to support custom netstack?
If not, I'd rather prefer to group them in a single place/structure so one doesn't have to non-atomically swap 100 different pointers in different places.

For example, it can be done similar to the Netlink way: https://github.com/freebsd/freebsd-src/blob/main/sys/netlink/netlink_glue.c

What do you think?

3252

It's worth creating DEFAULT() method in pr_init to avoid doin NULL-checking in runtime.

3609

I'd rather suggest having the alternative netstack implement these functions (maybe as stubs) so we don't have any conditions here.

4319

I'd rather do an explicit check in the ctl_output() functions. Hiding the check here and then checking some specific error code may be a bit hard to untangle for the code readers.

sys/net/route/route_tables.c

403

I'd probably just fetch family once - it will improve readability.

sys/net/rtsock.c

383

To be safe, it's worth checking the level/option here.

[stevek]

sys/kern/uipc_socket.c

198

Originally (and what we currently have in our tree at Juniper), I used a kobj to represent the "netstack". The intention was for long term to allow for more than one implementation to exist at the same time and be able to switch which is used for processes/jails.

The major point where there needs to be calls separated is the exports handling for NFS, but there are other places.

See https://gist.github.com/hackagadget/8520546c54b7afa4cb7e7efd1c55711d for the netstack_if.m file that we use.

Something similar or just a plain structure could be used.

sys/net/route/route_tables.c

403

Sure, this was mostly a cut/paste originally.

sys/net/rtsock.c

383

sosetfib() itself checks the sopt_level and sopt_name. I originally had a macro that everyone used that would then call the sosetfib() function so the check could happen and only call the function if necessary to avoid the function call overhead. Perhaps it might make sense to go back to that.

[stevek]

In D39768#905218, @melifaro wrote:

Conceptually it looks good to me. Please see some implementation-level comments - happy to discuss it further.

Maybe I should just pull in all my networks stack as a module changes in to a branch on my GitHub and we can discuss with a bigger picture in mind.

[melifaro]

In D39768#905234, @stevek wrote:

In D39768#905218, @melifaro wrote:

Conceptually it looks good to me. Please see some implementation-level comments - happy to discuss it further.

Maybe I should just pull in all my networks stack as a module changes in to a branch on my GitHub and we can discuss with a bigger picture in mind.

That would be awesome! I’d love to discuss & have the overall mechanics committed first and then go with per-feature diffs

[glebius]

What's the point of so_checkfiballowed? Can't find it being called. If we bring the processing SO_SETFIB down to protocols, then all protocols that aren't inetsw, inet6sw or routesw will return error just automagically. Where can we need so_checkfiballowed?

[stevek]

In D39768#906366, @glebius wrote:

What's the point of so_checkfiballowed? Can't find it being called. If we bring the processing SO_SETFIB down to protocols, then all protocols that aren't inetsw, inet6sw or routesw will return error just automagically. Where can we need so_checkfiballowed?

Yes, you're right we don't need that at all and it can be eliminated.

[stevek]

Made changes according to review comments.
Also fixed an issue when compiling gve_adminq.c due to the addition of
prototypes that use "bool", but the .c file was not including sys/types.h

[markj]

I think this diff is mostly OBE after commit caccbaef8e263. Do we need to keep it open?

[stevek]

In D39768#1233167, @markj wrote:

I think this diff is mostly OBE after commit caccbaef8e263. Do we need to keep it open?

I think we can close this now, yes.