Details

Reviewers

rrs
manu
tuexen

Group Reviewers

transport
manpages

Commits

rS363079: Provide support for building SCTP as a loadable module.

Summary

This is not intended to be committed as-is and should be split into
several pieces. For instance:

dtrace probes can be moved to sctp_kdtrace.c separately
rt_addrmsg eventhandler can be added separately
addition of the SCTP_SUPPORT option can be done separately

There are some XXX comments that I plan to address.

My goal is to enable removal of SCTP from our GENERIC kernels: I believe
there are relatively few users, and SCTP presents a relatively large
attack surface. Consider that IPSEC has more users and is similarly not
in GENERIC; one can set ipsec_enable="YES" in /etc/rc.conf and have it
automatically be loaded.

Test Plan

I tested tsctp and was able to pass traffic.

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 27047
Build 25335: arc lint + arc unit

Event Timeline

markj created this revision.Oct 12 2019, 4:53 PM

Herald added subscribers: jonathan, farrokhi, ae, asomers. · View Herald TranscriptOct 12 2019, 4:53 PM

Harbormaster completed remote builds in B27003: Diff 63186.Oct 12 2019, 4:53 PM

markj edited the test plan for this revision. (Show Details)Oct 12 2019, 4:55 PM

markj added a reviewer: tuexen.

markj edited the summary of this revision. (Show Details)

markj added inline comments.Oct 12 2019, 8:40 PM

sys/netinet/sctp_usrreq.c
98	I forgot to add the code to register SCTP for IPv6, will fix.

I would suggest to get

the generic fixes from above in head first
then get the eventhandler stuff or the dtrace changes in as separate commits.

That reduces the patch set and really focusses on the changes related to allowing SCTP to be a module.

If you agree, you can choose if we should do the eventhandler or dtrace stuff first. I would like to test things for the kernel stack be getting it in.

I would prefer to add a file sctp_module.c or so which contains the module loading and unloading stuff instead of adding it to sctp_usrreq.c.
I guess the unloading of the module needs some changes. You can only unload the stack if there are no endpoints. Then you need to perform the teardown which also gets rid of zones as some timers.

sys/dev/xen/netback/netback.c
49 ↗	(On Diff #63186)	Can you get this committed, since it doesn't seem to be related to this change, but it more a general fix.
sys/modules/pf/Makefile
9 ↗	(On Diff #63186)	Can you get this committed, since it doesn't seem to be related to this change, but it more a general fix.
sys/netinet/sctp_crc32.h
36	Not sure why this is needed. We use this pattern in all SCTP related files. So why change it only here. If this is wrong, we need to change it separately in all files.
sys/netinet6/ip6_forward.c
41	Can you get this committed, since it doesn't seem to be related to this change, but it more a general fix.
sys/netinet6/sctp6_usrreq.c
59–60	I guess removing `extern struct protosw inetsw[];` is a general fix. Should I run this upstream and commit it to head or do you prefer to commit it to head first and I will upstream it then.
sys/netpfil/pf/pf.c
45	The changes to this this files except the `\|\| defined(SCTP_SUPPORT)` are a general fix. Can you commit them?

tuexen added a reviewer: rrs.Oct 12 2019, 9:04 PM

tuexen added inline comments.Oct 12 2019, 9:09 PM

sys/netinet/sctp_usrreq.c
241	I would prefer to have almost all this stuff in a separate file, `sctp_module.c`, for example.

markj marked 4 inline comments as done.Oct 12 2019, 11:02 PM

markj added inline comments.

sys/netinet/sctp_crc32.h
36	I will just revert this hunk. __FBSDID is typically only used in .c files, but it doesn't really matter.
sys/netinet/sctp_usrreq.c
241	Ok
sys/netinet6/sctp6_usrreq.c
59–60	I'm not sure why I deleted it. It's not necessary for the rest of the changes, so please go ahead and commit it however you prefer.

tuexen added inline comments.Oct 13 2019, 9:40 AM

sys/netinet6/sctp6_usrreq.c
59–60	Committed in r353466

I'll focus on the eventhandler changes...

Rebase
Fix some XXX comments
Fix handling of COMPAT32 syscalls
Call ipproto_register() as well as pf_proto_register()

Harbormaster completed remote builds in B27043: Diff 63292.Oct 15 2019, 3:19 PM

I think the changes to in_kdtrace.c and sctp_kdtrace.c are not needed anymore, since they are in head.

Rebase on a more recent head revision
Update kernel configurations

Herald added a reviewer: manu. · View Herald TranscriptOct 15 2019, 4:10 PM

Herald added subscribers: bdragon, jhibbits, emaste, andrew. · View Herald Transcript

Harbormaster completed remote builds in B27047: Diff 63299.Oct 15 2019, 4:10 PM

Fix compilation guard for sopeeloff().

Harbormaster completed remote builds in B27085: Diff 63403.Oct 17 2019, 5:08 PM

tuexen commandeered this revision.Oct 24 2019, 10:41 AM

tuexen edited reviewers, added: markj; removed: tuexen.

Create a file sctp_module.c which handles all modules specific things which are FreeBSD specific. Also add sctp to the global list of modules.

Herald added subscribers: bz, imp. · View Herald TranscriptOct 24 2019, 10:43 AM

Harbormaster completed remote builds in B27195: Diff 63629.Oct 24 2019, 10:43 AM

A next incremental step might be to add the SCTP_SUPPORT option to sys/conf, and commit the changes to convert ifdef SCTP to if defined(SCTP) || defined(SCTP_SUPPORT).

sys/netinet/sctp_module.c
1 ↗	(On Diff #63629)	The file is missing a license header.
75 ↗	(On Diff #63629)	Extra newline.
93 ↗	(On Diff #63629)	I think we could address this by keeping a set of 4 global flags which indicate whether the protocol tuple <socket type, domain> has been registered. Then we conditionalize deregistration on those flags.

tuexen retitled this revision from to Allow SCTP to be build as a module.Nov 18 2019, 7:53 AM

In D21997#483847, @markj wrote:

A next incremental step might be to add the SCTP_SUPPORT option to sys/conf, and commit the changes to convert ifdef SCTP to if defined(SCTP) || defined(SCTP_SUPPORT).

Michael, do you have any objection to my committing some pieces of this? I would really like to move this work along.

Herald added a reviewer: transport. · View Herald TranscriptJun 18 2020, 3:56 PM

Herald added a subscriber: melifaro. · View Herald Transcript

In D21997#558809, @markj wrote:

In D21997#483847, @markj wrote:

A next incremental step might be to add the SCTP_SUPPORT option to sys/conf, and commit the changes to convert ifdef SCTP to if defined(SCTP) || defined(SCTP_SUPPORT).

Michael, do you have any objection to my committing some pieces of this? I would really like to move this work along.

Right now I'm busy bug fixing, not adding features... Get it in as long as it works...

markj commandeered this revision.Jul 8 2020, 11:43 PM

markj edited reviewers, added: tuexen; removed: markj.

Updates after testing with VIMAGE jails.
Don't modify kernel configs in this diff.

Harbormaster completed remote builds in B32208: Diff 74220.Jul 8 2020, 11:54 PM

I'll commit this in the next day or two if there are no objections, and start a mailing list thread before changing GENERIC.

emaste added inline comments.Jul 8 2020, 11:59 PM

sys/netinet/sctp_module.c
4 ↗	(On Diff #74220)	now 2020

Update copyright year.

Harbormaster completed remote builds in B32209: Diff 74221.Jul 9 2020, 12:02 AM

This revision was not accepted when it landed; it landed in state Needs Review.Jul 10 2020, 2:56 PM

Closed by commit rS363079: Provide support for building SCTP as a loadable module. (authored by markj). · Explain Why

This revision was automatically updated to reflect the committed changes.

markj added a commit: rS363079: Provide support for building SCTP as a loadable module..

Herald added a reviewer: manpages. · View Herald TranscriptJul 10 2020, 2:56 PM

Allow SCTP to be build as a module
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 63299

sys/amd64/conf/GENERIC

sys/amd64/conf/MINIMAL

sys/arm/conf/std.armv6

sys/arm/conf/std.armv7

sys/arm64/conf/GENERIC

sys/conf/NOTES

sys/conf/options

sys/i386/conf/GENERIC

sys/i386/conf/MINIMAL

sys/kern/uipc_socket.c

sys/mips/conf/ERL

sys/mips/conf/OCTEON1

sys/modules/sctp/Makefile

sys/netinet/in_proto.c

sys/netinet/ip_divert.c

sys/netinet/ip_output.c

sys/netinet/sctp_crc32.h

sys/netinet/sctp_crc32.c

sys/netinet/sctp_os_bsd.h

sys/netinet/sctp_syscalls.c

sys/netinet/sctp_sysctl.c

sys/netinet/sctp_usrreq.c

sys/netinet/sctp_var.h

sys/netinet6/in6_proto.c

sys/netinet6/ip6_forward.c

sys/netinet6/ip6_output.c

sys/netinet6/sctp6_usrreq.c

sys/netinet6/sctp6_var.h

sys/netipsec/ipsec_output.c

sys/netpfil/pf/pf.c

sys/powerpc/conf/GENERIC

sys/powerpc/conf/GENERIC64

sys/riscv/conf/GENERIC

sys/sparc64/conf/GENERIC

Allow SCTP to be build as a moduleClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 63299

sys/amd64/conf/GENERIC

sys/amd64/conf/MINIMAL

sys/arm/conf/std.armv6

sys/arm/conf/std.armv7

sys/arm64/conf/GENERIC

sys/conf/NOTES

sys/conf/options

sys/i386/conf/GENERIC

sys/i386/conf/MINIMAL

sys/kern/uipc_socket.c

sys/mips/conf/ERL

sys/mips/conf/OCTEON1

sys/modules/sctp/Makefile

sys/netinet/in_proto.c

sys/netinet/ip_divert.c

sys/netinet/ip_output.c

sys/netinet/sctp_crc32.h

sys/netinet/sctp_crc32.c

sys/netinet/sctp_os_bsd.h

sys/netinet/sctp_syscalls.c

sys/netinet/sctp_sysctl.c

sys/netinet/sctp_usrreq.c

sys/netinet/sctp_var.h

sys/netinet6/in6_proto.c

sys/netinet6/ip6_forward.c

sys/netinet6/ip6_output.c

sys/netinet6/sctp6_usrreq.c

sys/netinet6/sctp6_var.h

sys/netipsec/ipsec_output.c

sys/netpfil/pf/pf.c

sys/powerpc/conf/GENERIC

sys/powerpc/conf/GENERIC64

sys/riscv/conf/GENERIC

sys/sparc64/conf/GENERIC

Allow SCTP to be build as a module
ClosedPublic
Actions

Revision Contents
Changeset List