Re: [PATCH net-next v2 1/4] netconsole: add a per-target message rate limit

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 0 · Low: 1

- [High] The oops_in_progress bypass in netconsole_ratelimited()
  (drivers/net/netconsole.c:299-306) does not deliver the guarantee its…
- [Low] In netconsole_write() (drivers/net/netconsole.c:2518-2534) the
  rate-limit token is consumed by netconsole_ratelimited() ->…

--

netconsole: add a per-target message rate limit

This adds a struct ratelimit_state token bucket to each netconsole target
and consults it once per message in netconsole_write(). The bucket is
initialised with a zero interval (unlimited) and the check is skipped while
oops_in_progress is set.

> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> index b358e5c367351..13292c3f818e9 100644
> --- a/drivers/net/netconsole.c
> +++ b/drivers/net/netconsole.c

[ ... ]

> @@ -282,6 +285,26 @@ static void dynamic_netconsole_mutex_unlock(void)
>  	mutex_unlock(&dynamic_netconsole_mutex);
>  }
>  
> +static void netconsole_ratelimit_init(struct netconsole_target *nt)
> +{
> +	ratelimit_state_init(&nt->ratelimit, 0, DEFAULT_RATELIMIT_BURST);
> +	/* The flag keeps ___ratelimit() from reporting the suppressed count
> +	 * itself, which would printk() from inside the console being
> +	 * serviced. Nothing calls ratelimit_state_exit(), so the count is
> +	 * never reported on release either.
> +	 */
> +	ratelimit_set_flags(&nt->ratelimit, RATELIMIT_MSG_ON_RELEASE);
> +}
> +
> +static bool netconsole_ratelimited(struct netconsole_target *nt)
> +{
> +	/* A limit meant for steady-state logging must not eat a crash dump. */
> +	if (oops_in_progress)
> +		return false;
> +
> +	return !__ratelimit(&nt->ratelimit);
> +}

[Severity: High]
Does the oops_in_progress test in netconsole_ratelimited() actually cover
oops and BUG() output on netconsole?  The changelog says:

    Skip the bucket while oops_in_progress is set, so a limit configured
    for steady-state logging never truncates an oops, BUG() or panic().

and the documentation patch later in the series repeats it ("Crash output
bypasses the bucket. While an oops, BUG() or panic() is in progress every
message is sent").

Both consoles are registered with CON_NBCON_ATOMIC_UNSAFE:

drivers/net/netconsole.c
    static struct console netconsole = {
        .name = "netcon",
        .flags = CON_ENABLED | CON_NBCON | CON_NBCON_ATOMIC_UNSAFE,

and console_is_usable() refuses such a write_atomic() callback unless
unsafe takeovers are allowed:

include/linux/console.h:console_is_usable() {
    ...
            if ((flags & CON_NBCON_ATOMIC_UNSAFE) && !nbcon_allow_unsafe_takeover())
                    return false;
    ...
}

kernel/printk/nbcon.c
    bool nbcon_allow_unsafe_takeover(void)
    {
            return panic_on_this_cpu() && panic_nbcon_allow_unsafe_takeover;
    }

So during a non-panic oops the atomic flush skips netconsole entirely.  In
the emergency context that oops_enter() establishes, only nbcon_atomic is
selected, not nbcon_offload:

kernel/printk/internal.h:printk_get_console_flush_type() {
    ...
    case NBCON_PRIO_EMERGENCY:
            if (have_nbcon_console && !have_boot_console)
                    ft->nbcon_atomic = true;
    ...
}

which means the netconsole kthread is not woken while the backtrace is
being emitted.  The wake happens later, from
nbcon_cpu_emergency_exit() -> nbcon_kthreads_wake(), reached via
oops_exit(), and on x86 oops_end() runs bust_spinlocks(0) (which drops
oops_in_progress back to 0) before oops_exit().

Doesn't that mean every record of an oops or BUG() backtrace reaches
netconsole_write() with oops_in_progress == 0 and is charged against the
bucket?  With the configfs interval knob from the following patch set to a
nonzero value, a backtrace is many more records than
DEFAULT_RATELIMIT_BURST (10), so would it be silently truncated?  panic()
looks protected because it does bust_spinlocks(1) before the panic-CPU
unsafe atomic flush, but oops/BUG do not appear to be.

Would keying the bypass off the nbcon record/context priority, or the nbcon
emergency nesting count, work better here than the global oops_in_progress
flag?

[ ... ]

> @@ -2482,6 +2515,9 @@ static void netconsole_write(struct nbcon_write_context *wctxt, bool extended)
>  		    !netif_running(nt->np.dev))
>  			continue;
>  
> +		if (netconsole_ratelimited(nt))
> +			continue;
> +
>  		/* If nbcon_enter_unsafe() fails, just return given netconsole
>  		 * lost the ownership, and iterating over the targets will not
>  		 * be able to re-acquire.

[Severity: Low]
This isn't much of a bug, but is the token spent a bit early here?
netconsole_ratelimited() -> __ratelimit() decrements rs->rs_n_left:

lib/ratelimit.c:___ratelimit() {
    ...
	if (atomic_read(&rs->rs_n_left) > 0 && atomic_dec_return(&rs->rs_n_left) >= 0)
		ret = 1;
    ...
}

and only afterwards does netconsole_write() check ownership:

drivers/net/netconsole.c:netconsole_write() {
    ...
	if (!nbcon_enter_unsafe(wctxt))
		return;
    ...
}

If nbcon_enter_unsafe() returns false the message is never transmitted, yet
the token is gone and there is no way to give it back.  The return value of
nbcon_exit_unsafe() is also ignored, so a further loop iteration can spend
another target's token before lost ownership is noticed.

In practice this only shows up on the panic-CPU unsafe takeover path, since
console_is_usable() rejects netconsole's write_atomic() for every other
context, and there the re-emitted record hits the oops_in_progress bypass
anyway.  Would consuming the token only after the write context is acquired
(or only on a successful send) be cleaner?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260910-netcons_ratelimit-v2-0-ebf0dd91e26e%40debian.org