Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 1 · Low: 1
- [Medium] The live-update path for the two rate-limit knobs does not
reset the token bucket, so the behaviour advertised by the new…
- [Low] Documentation/networking/netconsole.rst: the canonical configfs
inventory for a netconsole target ("The interface exposes these…
--
docs: netconsole: document rate limit feature
This adds a "Rate limiting" section to the netconsole documentation
describing the per-target token bucket and the two configfs files that
drive it, ratelimit_interval_ms and ratelimit_burst. It also notes that
messages dropped by the bucket never get a message ID assigned.
> diff --git a/Documentation/networking/netconsole.rst
> b/Documentation/networking/netconsole.rst
> index 4ab5d7b05cf10..a9ccf79834656 100644
> --- a/Documentation/networking/netconsole.rst
> +++ b/Documentation/networking/netconsole.rst
> @@ -177,6 +177,41 @@ You can modify these targets in runtime by creating the
> following targets::
> cat cmdline1/remote_ip
> 10.0.0.3
>
> +Rate limiting
> +-------------
> +
> +Netconsole hands every console message to every enabled target, so a host
> that
> +logs continuously can saturate the receiving agent. Each target carries a
> token
> +bucket that drops messages once the configured rate is exceeded, controlled
> by
> +two files in the target directory:
> +
> + =====================
> ================================================
> + ratelimit_interval_ms Length of the accounting interval, in
> + milliseconds. Zero, the default, sends
> + everything.
> + ratelimit_burst Messages allowed per interval. Defaults to
> + 10; zero drops every message once an
> + interval is set.
> + =====================
> ================================================
> +
> +Unlike most target parameters, both knobs can be written while the target is
> +enabled, which is when a flooding target most likely needs them.
[Severity: Low]
Should the target parameter table earlier in this file be extended too?
That table is introduced with "The interface exposes these parameters of a
netconsole target to userspace" and it does list read-only siblings such as
transmit_errors, but it gains no entry for ratelimit_interval_ms or
ratelimit_burst, even though netconsole_target_attrs[] in
drivers/net/netconsole.c registers them right next to attr_transmit_errors.
The sentence directly under that table also seems to conflict with the new
paragraph above:
The "enabled" attribute is also used to control whether the parameters of
a target can be updated or not -- you can modify the parameters of only
disabled targets (i.e. if "enabled" is 0).
Neither ratelimit_interval_ms_store() nor ratelimit_burst_store() has the
nt->state == STATE_ENABLED test that, for example, remote_port_store() uses
before returning -EINVAL with "target (%s) is enabled, disable to update
parameters". Would it be worth qualifying that blanket rule so the document
does not describe the same ABI in two contradictory ways?
> +
> +The limit is applied per message, not per packet, so a message big enough to
> be
> +split into several `ncfrag` packets is either sent whole or not at all.
> +
> +Crash output bypasses the bucket. While an oops, BUG() or panic() is in
> +progress every message is sent, whatever the limit says, so a small burst
> +cannot cost you part of a crash dump.
> +
> +A drop leaves nothing on the wire. On an extended target it shows up as a gap
> +in the sequence number the header carries; a basic target has no such marker.
> +
> +Capping a target at 500 messages a minute::
> +
> + echo 60000 > ratelimit_interval_ms
> + echo 500 > ratelimit_burst
[Severity: Medium]
Does this two-step example behave as described on the flooding target the
section is aimed at?
ratelimit_interval_ms_store() and ratelimit_burst_store() in
drivers/net/netconsole.c only publish the new values:
dynamic_netconsole_mutex_lock();
WRITE_ONCE(nt->ratelimit.interval, jifs);
dynamic_netconsole_mutex_unlock();
dynamic_netconsole_mutex_lock();
WRITE_ONCE(nt->ratelimit.burst, burst);
dynamic_netconsole_mutex_unlock();
In lib/ratelimit.c, ___ratelimit() re-seeds the live allowance from
rs->burst only on first use or once the interval has expired:
if (!(rs->flags & RATELIMIT_INITIALIZED)) {
rs->begin = jiffies;
rs->flags |= RATELIMIT_INITIALIZED;
atomic_set(&rs->rs_n_left, rs->burst);
}
if (time_is_before_jiffies(rs->begin + interval)) {
...
atomic_set(&rs->rs_n_left, rs->burst);
So after the first echo starts a 60 s interval with the default burst of 10,
a flooding target drains those 10 tokens before the second shell command
runs. Once burst=500 is stored, rs_n_left stays at 0 and every message is
dropped for the rest of the 60 s interval, while cat ratelimit_burst reports
500. Lowering the burst mid-interval leaves the larger allowance spendable,
and the next interval is still measured from the old rs->begin.
Should the stores go through ratelimit_state_reset_interval() in
include/linux/ratelimit.h, which takes rs->lock, clears
RATELIMIT_INITIALIZED, does atomic_set(&rs->rs_n_left, rs->burst) and resets
the missed counter? Otherwise, should this section say that a change only
takes effect at the next interval boundary?
There is also the plain load of rs->burst in the two atomic_set() calls in
___ratelimit(), which run under rs->lock. The configfs writers hold only
dynamic_netconsole_mutex, which the console write path
netconsole_write() -> netconsole_ratelimited() -> __ratelimit() never takes,
so the locksets do not intersect. Is that concurrent plain read against
WRITE_ONCE() something KCSAN would flag?
>
> Append User Data
> ----------------
[ ... ]
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260910-netcons_ratelimit-v2-0-ebf0dd91e26e%40debian.org
