Page MenuHomeFreeBSD
Differential D21997

Allow SCTP to be build as a module
ClosedPublic
Actions

Authored by markj on Oct 12 2019, 4:53 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Sep 20, 2:32 AM
Unknown Object (File)
Sun, Sep 20, 2:29 AM
Unknown Object (File)
Sun, Sep 20, 2:29 AM
Unknown Object (File)
Sun, Sep 20, 2:28 AM
Unknown Object (File)
Sun, Sep 20, 2:28 AM
Unknown Object (File)
Sun, Sep 20, 2:28 AM
Unknown Object (File)
Sun, Sep 20, 2:27 AM
Unknown Object (File)
Sun, Sep 20, 2:27 AM
View All Files
Subscribers
ae
andrew
asomers
bdragon
bz
emaste
farrokhi
View All 11 Subscribers

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 32209
Build 29705: 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
71

I forgot to add the code to register SCTP for IPv6, will fix.

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

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 ↗(On Diff #63186)

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 ↗(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/netinet6/sctp6_usrreq.c
60 ↗(On Diff #63186)

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 ↗(On Diff #63186)

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
137

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 ↗(On Diff #63186)

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
137

Ok

sys/netinet6/sctp6_usrreq.c
60 ↗(On Diff #63186)

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
60 ↗(On Diff #63186)

Committed in r353466

tuexen added a comment.Oct 13 2019, 9:41 AM

I'll focus on the eventhandler changes...

markj updated this revision to Diff 63292.Oct 15 2019, 3:19 PM
markj marked an inline comment as done.
  • 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
tuexen added a comment.Oct 15 2019, 3:30 PM

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

markj updated this revision to Diff 63299.Oct 15 2019, 4:10 PM
  • 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
markj updated this revision to Diff 63403.Oct 17 2019, 5:08 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.
tuexen updated this revision to Diff 63629.Oct 24 2019, 10:43 AM
tuexen retitled this revision from Support a loadable sctp module. to .

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
markj added a comment.Oct 24 2019, 2:54 PM

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
2

The file is missing a license header.

76

Extra newline.

94

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
markj added a comment.Jun 18 2020, 3:56 PM
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
tuexen added a comment.Jun 18 2020, 4:01 PM
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.
markj updated this revision to Diff 74220.Jul 8 2020, 11:54 PM
markj retitled this revision from Allow SCTP to be build as a module to Allow SCTP to be build as a module.
  • 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
markj added a comment.Jul 8 2020, 11:57 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
5

now 2020

markj updated this revision to Diff 74221.Jul 9 2020, 12:02 AM

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

Revision Contents
Changeset List

PathSize
sys/
modules/
4 lines
sctp/
30 lines
netinet/
197 lines
7 lines
75 lines
2 lines

Diff 74221

View Options

sys/modules/Makefile

Loading...
View Options

sys/modules/sctp/Makefile

Loading...
View Options

sys/netinet/sctp_module.c

Loading...
View Options

sys/netinet/sctp_os_bsd.h

Loading...
View Options

sys/netinet/sctp_syscalls.c

Loading...
View Options

sys/netinet/sctp_usrreq.c

Loading...