Project Tags

None

Referenced Files

None

Subscribers

Details

Reviewers

brooks
melifaro
markj

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.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 51137
Build 48028: arc lint + arc unit

Event Timeline

stevek created this revision.Apr 23 2023, 5:52 AM

Herald added a reviewer: brooks. · View Herald TranscriptApr 23 2023, 5:52 AM

Herald added a reviewer: melifaro. · View Herald Transcript

Herald added subscribers: glebius, melifaro, ae, imp. · View Herald Transcript

stevek requested review of this revision.Apr 23 2023, 5:52 AM

Harbormaster completed remote builds in B51137: Diff 120886.Apr 23 2023, 5:52 AM

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

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

Harbormaster completed remote builds in B51138: Diff 120887.Apr 23 2023, 5:55 AM

stevek added a comment.Apr 23 2023, 5:58 AM

This comment was removed by 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.

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

Harbormaster completed remote builds in B51139: Diff 120888.Apr 23 2023, 6:33 AM

Removed includes that are no longer necessary.

Harbormaster completed remote builds in B51140: Diff 120889.Apr 23 2023, 6:42 AM

gbe subscribed.Apr 23 2023, 7:19 AM

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

sys/kern/uipc_socket.c
200	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?
3236	It's worth creating DEFAULT() method in `pr_init` to avoid doin NULL-checking in runtime.
3595	I'd rather suggest having the alternative netstack implement these functions (maybe as stubs) so we don't have any conditions here.
4409	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
485	I'd probably just fetch family once - it will improve readability.
sys/net/rtsock.c
386	To be safe, it's worth checking the level/option here.

stevek added inline comments.Apr 23 2023, 5:28 PM

sys/kern/uipc_socket.c
200	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
485	Sure, this was mostly a cut/paste originally.
sys/net/rtsock.c
386	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.

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.

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

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?

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.

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

Herald added a subscriber: olce. · View Herald TranscriptApr 3 2024, 10:57 PM

Harbormaster completed remote builds in B56906: Diff 136508.Apr 3 2024, 10:57 PM

zlei subscribed.Apr 16 2024, 3:02 PM

allanjude added a reviewer: markj.Aug 1 2024, 3:31 PM

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

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.

sockets: avoid layering violation in handling SO_SETFIB
AbandonedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 120886

sys/kern/init_sysent.c

sys/kern/syscalls.master

sys/kern/uipc_socket.c

sys/kern/uipc_usrreq.c

sys/net/route.h

sys/net/route/route_tables.c

sys/net/rtsock.c

sys/netinet/ip_output.c

sys/netinet/raw_ip.c

sys/netinet6/ip6_output.c

sys/netinet6/raw_ip6.c

sys/sys/socket.h

sys/sys/sockopt.h

sys/sys/sysproto.h

sockets: avoid layering violation in handling SO_SETFIBAbandonedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 120886

sys/kern/init_sysent.c

sys/kern/syscalls.master

sys/kern/uipc_socket.c

sys/kern/uipc_usrreq.c

sys/net/route.h

sys/net/route/route_tables.c

sys/net/rtsock.c

sys/netinet/ip_output.c

sys/netinet/raw_ip.c

sys/netinet6/ip6_output.c

sys/netinet6/raw_ip6.c

sys/sys/socket.h

sys/sys/sockopt.h

sys/sys/sysproto.h

sockets: avoid layering violation in handling SO_SETFIB
AbandonedPublic
Actions

Revision Contents
Changeset List