Re: [PATCH net-next v2 2/4] netconsole: allow configuring the rate limit interval through configfs

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 0 · Low: 3

- [Low] DISCARDED (resolved later in the series).
- [Low] DISCARDED (documented as an intentional exception later in the
  series).
- [Low] Wrong range bound in ratelimit_interval_ms_store()
  (drivers/net/netconsole.c:1363-1365): the user value is converted…

--

netconsole: allow configuring the rate limit interval through configfs

This exposes the per-target token bucket interval as a new configfs
attribute, ratelimit_interval_ms.  The value is converted from
milliseconds to jiffies on store and back to milliseconds on show, and
defaults to zero so a target remains unlimited unless configured.

> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> index 13292c3f818e9..faf8f9bbc6c0c 100644
> --- a/drivers/net/netconsole.c
> +++ b/drivers/net/netconsole.c
> @@ -945,6 +945,14 @@ static ssize_t transmit_errors_show(struct config_item *item, char *buf)
>  	return sysfs_emit(buf, "%llu\n", xmit_drop_count + enomem_count);
>  }
>  
> +static ssize_t ratelimit_interval_ms_show(struct config_item *item, char *buf)
> +{
> +	struct netconsole_target *nt = to_target(item);
> +
> +	return sysfs_emit(buf, "%u\n",
> +			  jiffies_to_msecs(READ_ONCE(nt->ratelimit.interval)));
> +}
> +

[ ... ]

> @@ -1340,6 +1348,29 @@ static ssize_t remote_mac_store(struct config_item *item, const char *buf,
>  	return ret;
>  }
>  
> +static ssize_t ratelimit_interval_ms_store(struct config_item *item,
> +					   const char *buf, size_t count)
> +{
> +	struct netconsole_target *nt = to_target(item);
> +	unsigned int interval;
> +	unsigned long jifs;
> +	ssize_t ret;
> +
> +	ret = kstrtouint(buf, 10, &interval);
> +	if (ret)
> +		return ret;
> +
> +	jifs = msecs_to_jiffies(interval);
> +	if (jifs > INT_MAX)
> +		return -ERANGE;

[Severity: Low]

Can the "jifs > INT_MAX" test in ratelimit_interval_ms_store() ever fire on
a 32-bit kernel?

msecs_to_jiffies() saturates before the comparison happens:

kernel/time/time.c:__msecs_to_jiffies() {
	if ((int)m < 0)
		return MAX_JIFFY_OFFSET;
	return _msecs_to_jiffies(m);
}

and MAX_JIFFY_OFFSET is defined as:

include/linux/jiffies.h:
	#define MAX_JIFFY_OFFSET ((LONG_MAX >> 1)-1)

On 32-bit that is 1073741822, which is smaller than INT_MAX (2147483647).
For every supported HZ <= 1000 the non-saturating path returns at most
"interval" itself, so on 32-bit the check looks like dead code.

The visible effect is that the same write behaves differently per
architecture.  With CONFIG_NETCONSOLE_DYNAMIC on 32-bit:

	echo 3000000000 > /sys/kernel/config/netconsole/<t>/ratelimit_interval_ms

kstrtouint() accepts the value, __msecs_to_jiffies() takes the "(int)m < 0"
branch and returns MAX_JIFFY_OFFSET, the range check passes, and
WRITE_ONCE() stores 1073741822 into nt->ratelimit.interval.  The write
returns success, while the identical write on a 64-bit kernel returns
-ERANGE.

Would checking MAX_JIFFY_OFFSET, or validating "interval" before the
conversion, express the intent better here?

> +
> +	dynamic_netconsole_mutex_lock();
> +	WRITE_ONCE(nt->ratelimit.interval, jifs);
> +	dynamic_netconsole_mutex_unlock();
> +
> +	return count;
> +}
> +

[Severity: Low]

Following on from the clamping above, can the read-back in
ratelimit_interval_ms_show() wrap on 32-bit?

include/linux/jiffies.h:
	static inline unsigned int jiffies_to_msecs(const unsigned long j)
	{
		return (MSEC_PER_SEC / HZ) * j;
	}

That multiplication is done in 32-bit arithmetic.  With HZ=100 and the
stored value 1073741822 from the example above, the attribute prints
2147483628 rather than 10737418220, so the value read back is unrelated to
what was written.

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