Page MenuHomeFreeBSD
Differential D39768

sockets: avoid layering violation in handling SO_SETFIB
Needs ReviewPublic
Actions

Authored by stevek on Apr 23 2023, 5:52 AM.
Tags
None
Referenced Files
F103193104: D39768.id120889.diff
Fri, Nov 22, 2:38 AM
Unknown Object (File)
Tue, Nov 19, 2:45 PM
Unknown Object (File)
Fri, Nov 15, 10:25 PM
Unknown Object (File)
Tue, Nov 12, 5:25 PM
Unknown Object (File)
Sun, Oct 27, 7:43 PM
Unknown Object (File)
Fri, Oct 25, 10:47 PM
Unknown Object (File)
Oct 2 2024, 12:16 PM
Unknown Object (File)
Sep 27 2024, 3:45 PM
View All 73 Files
Subscribers
ae
gbe
glebius
imp
melifaro
olce
zlei

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 51140
Build 48031: 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
stevek added a comment.Apr 23 2023, 5:53 AM

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

stevek updated this revision to Diff 120887.Apr 23 2023, 5:54 AM

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.
stevek added a comment.Apr 23 2023, 6:02 AM

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 updated this revision to Diff 120888.Apr 23 2023, 6:33 AM

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
stevek updated this revision to Diff 120889.Apr 23 2023, 6:42 AM

Removed includes that are no longer necessary.

Harbormaster completed remote builds in B51140: Diff 120889.Apr 23 2023, 6:42 AM
gbe added a subscriber: gbe.Apr 23 2023, 7:19 AM
melifaro added a comment.Apr 23 2023, 3:58 PM

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
393

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
393

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.

stevek added a comment.Apr 23 2023, 5:40 PM
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 added a comment.Apr 23 2023, 9:30 PM
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 added a comment.Apr 25 2023, 9:51 PM

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 added a comment.Apr 27 2023, 3:37 AM
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 updated this revision to Diff 136508.Apr 3 2024, 10:57 PM

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 added a subscriber: zlei.Apr 16 2024, 3:02 PM
allanjude added a reviewer: markj.Aug 1 2024, 3:31 PM

Revision Contents
Changeset List

PathSize
sys/
kern/
78 lines
18 lines
6 lines
net/
route/
41 lines
8 lines
netinet/
12 lines
8 lines
netinet6/
13 lines
7 lines
sys/
6 lines
6 lines

Diff 120889

View Options

sys/kern/uipc_socket.c

Loading...
View Options

sys/kern/uipc_syscalls.c

Loading...
View Options

sys/kern/uipc_usrreq.c

Loading...
View Options

sys/net/route/route_tables.c

Loading...
View Options

sys/net/rtsock.c

Loading...
View Options

sys/netinet/ip_output.c

Loading...
View Options

sys/netinet/raw_ip.c

Loading...
View Options

sys/netinet6/ip6_output.c

Loading...
View Options

sys/netinet6/raw_ip6.c

Loading...
View Options

sys/sys/socket.h

Loading...
View Options

sys/sys/sockopt.h

Loading...