Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rate-limit ICMP errors similar to kernel defaults #8

Closed
wants to merge 1 commit into from
Closed

rate-limit ICMP errors similar to kernel defaults #8

wants to merge 1 commit into from

Conversation

brada4
Copy link

@brada4 brada4 commented Sep 1, 2023

Rationale described here:
openwrt/openwrt#13340 (comment)

UDP closed port will return 80 octets for 64 octets input at 1000/sec for each ip6/ip6 by kernel while nftable will have no limit.
It is quick as in efficient approximation of more sophisticated linux kernel behaviour. Namely it does not implement 1 packet per address per timeslice on selected ICMP types.

@brada4
Copy link
Author

brada4 commented Sep 1, 2023

ping @jow-

@jow-
Copy link
Contributor

jow- commented Sep 3, 2023

The limit needs to be configurable, we shouldn't add hardcoded limits to the ruleset.

@brada4
Copy link
Author

brada4 commented Sep 3, 2023

yup, this exceeds default unconfigurable syn flood limit by magnitude.

@jow-
Copy link
Contributor

jow- commented Sep 4, 2023

yup, this exceeds default unconfigurable syn flood limit by magnitude.

??

@brada4
Copy link
Author

brada4 commented Sep 4, 2023

Syn limit is 25/s enable/disable.
Could we agree to set this to 1000/s and add option to reduce it if anyone ever asks? Sort of all commercial routers limit generated ICMP unavoidably with 1000/s being configurable maximum with defaults anywhere below.
At present it generates a bit more responses (84B in 100B out measured on ethernet) than ping, so it can even bring down upstream of symmetrical link. 1.2x amplification is not that much of concern globally.

@jow-
Copy link
Contributor

jow- commented Sep 4, 2023

Syn limit is 25/s enable/disable.

It defaults to 25/s and can be adjusted using the synflood_rate and synflood_burst options. Your change would need to introduce new reject_rate and reject_burst options which may default to 1000/sec and 50 respectively.

I just don't want to introduce any kind of hardcoded limits into the ruleset. Also please adjust the testcases accordingly.

@brada4
Copy link
Author

brada4 commented Sep 4, 2023

On weekend more time.
More like icmp_generation_rate, defaulting to 10 and clamping setting to 1000.
Burst can stay at default 5 to not grow settings.

@brada4
Copy link
Author

brada4 commented Sep 4, 2023

@jow- is there a resource in ucode that takes sysctl values or it is to exec them?

@jow-
Copy link
Contributor

jow- commented Sep 4, 2023

If there's a corresponding /sys/ or /proc/sys/ file then just readfile() it, otherwise popen() + read()

@brada4 brada4 closed this Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants