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
