Page MenuHomeFreeBSD

ICMP6 rate-limit enhancements
Needs RevisionPublic

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

Details

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
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 8719
Build 9058: arc lint + arc unit

Event Timeline

jtl created this revision.Apr 13 2017, 4:59 PM
jtl added a reviewer: network.Apr 13 2017, 5:32 PM
ae accepted this revision as: ae.Apr 13 2017, 11:05 PM

Looks good to me.

tuexen edited edge metadata.Apr 14 2017, 9:45 AM

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.

hiren accepted this revision as: hiren.Apr 14 2017, 3:39 PM
hiren added a subscriber: hiren.

Very nice work! Thank you.

glebius requested changes to this revision.Apr 26 2017, 9:27 PM

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