ICMP6 rate-limit enhancements
Needs RevisionPublic
Actions

Authored by jtl on Apr 13 2017, 4:59 PM.

Details

Reviewers

glebius
tuexen
bz
ae
hiren

Group Reviewers

manpages
transport
network

Summary

Add a rate limit for ICMP6 informational messages and make the current
rate limit on ICMP6 error messages be MP safe.

To do this without increasing code duplication, move the bulk of the
badport_bandlim() function from the protocol-specific file ip_icmp.c
to the protocol-independent file subr_counter.c.

At the same time, break out the SCTP and TCP limits separately. This
removes the dependency on ip_icmp.c for an IPv6-only kernel.

(Note that the diff builds on changes from D10387.)

Release Notes: yes

Test Plan

The system compiles.
A no-INET kernel compiles.
The rate limits appear to work correctly.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 8719
Build 9058: arc lint + arc unit

Event Timeline

jtl created this revision.Apr 13 2017, 4:59 PM

Herald added a reviewer: manpages. · View Herald TranscriptApr 13 2017, 4:59 PM

Herald added a reviewer: transport. · View Herald Transcript

Herald added subscribers: ae, imp. · View Herald Transcript

jtl added a reviewer: network.Apr 13 2017, 5:32 PM

Looks good to me.

In general I'm fine with the patch. Mostly minor SCTP related nits. It would be great if you can address them.

share/man/man4/tcp.4
601	Does setting this number to -1 means no rate limit? Does setting this number to 0 means not sending RSTs at all? This could be clearly stated.
sys/netinet/sctp_sysctl.c
861	This needs to be int32_t.
sys/netinet/sctp_sysctl.h
117	Can we use a int32_t? We use fixed sizes whenever possible.
118	This can be an uint32.
543	This needs then to be INT32_MAX
547	This can be 0.
548	This can be 1
549	This can be SCTPCTL_BADPORT_LIM_OUTPUT_MAX
sys/netinet/sctp_usrreq.c
86	Please move this call into sctp_pcb_init().
100	Can you move this into sctp_pcb_finish()? sctp_finish() is just a wrapper.

Very nice work! Thank you.

I raised concerns to Jonathan in private about this change. I dislike that counter(9) code is polluted with network stuff. I have submitted him a patch on top of his patch, and we are in process of negotiating that.

This revision now requires changes to proceed.Apr 26 2017, 9:27 PM

rgrimes added a subscriber: rgrimes.Apr 26 2017, 9:56 PM

wblock added a subscriber: wblock.Jul 14 2017, 3:10 PM

wblock added inline comments.

share/man/man4/tcp.4
596	Passive -> active: s/will send/sends/
605	Passive -> active: s/will create/creates/
share/man/man9/counter.9
156	Reads better without "The function". Just "Initializes the rate limit..."
163	s/which will be used//
166	s/a integer/an integer/ s/which has/with/ "non-positive" is really unclear. The comments make it clear that this means zero or negative. I know there is a math term for this, but can't think of it at this time of morning. Probably best to be explicit, saying "a zero or negative value", or possibly just "less than one".
175	Passive -> active: s/then log messages will be/log messages are/
187	Passive -> active: s/will take/take/
189	s/The function removes/Removes/