Re: [PATCH net-next v2 4/4] docs: netconsole: document rate limit feature

[netdev-bot+sashiko@kernel.org]

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