Page MenuHomeFreeBSD

Allow SCTP to be build as a module
Needs ReviewPublic

Authored by tuexen on Oct 12 2019, 4:53 PM.

Details

Reviewers
rrs
manu
markj
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

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 27195

Event Timeline

markj created this revision.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
95 ↗(On Diff #63186)

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

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

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

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

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()

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
markj updated this revision to Diff 63403.Oct 17 2019, 5:08 PM

Fix compilation guard for sopeeloff().

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.

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
1

The file is missing a license header.

75

Extra newline.

93

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.Mon, Nov 18, 7:53 AM