Page MenuHomeFreeBSD

Allow SCTP to be build as a module
ClosedPublic

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

Details

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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

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 ↗(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
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 ↗(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
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 ↗(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
markj added a comment.Jun 18 2020, 3:56 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).

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

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.
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
4 ↗(On Diff #74220)

now 2020

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

Update copyright year.

This revision was not accepted when it landed; it landed in state Needs Review.Jul 10 2020, 2:56 PM
This revision was automatically updated to reflect the committed changes.