Page MenuHomeFreeBSD
Differential D39768

sockets: avoid layering violation in handling SO_SETFIB
AbandonedPublic
Actions

Authored by stevek on Apr 23 2023, 5:52 AM.
Tags
None
Referenced Files
F139444081: D39768.id136508.diff
Fri, Dec 12, 4:03 AM
Unknown Object (File)
Thu, Dec 11, 3:21 AM
Unknown Object (File)
Fri, Dec 5, 10:32 PM
Unknown Object (File)
Fri, Nov 21, 2:17 PM
Unknown Object (File)
Fri, Nov 21, 2:38 AM
Unknown Object (File)
Mon, Nov 17, 9:03 PM
Unknown Object (File)
Mon, Nov 17, 7:35 PM
Unknown Object (File)
Sat, Nov 15, 5:25 PM
View All 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 56906
Build 53794: 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
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 added inline comments.Apr 23 2023, 5:28 PM
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 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
markj added a comment.Fri, Nov 28, 2:21 PM

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

stevek abandoned this revision.Mon, Dec 1, 9:39 PM
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.

Revision Contents
Changeset List

PathSize
sys/
dev/
gve/
1 line
kern/
7 lines
82 lines
18 lines
6 lines
net/
route/
41 lines
12 lines
netinet/
12 lines
8 lines
netinet6/
13 lines
7 lines
sys/
6 lines
6 lines

Diff 136508

View Options

sys/dev/gve/gve_adminq.c

Loading...
View Options

sys/kern/uipc_domain.c

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