Re: [PATCH net] net: reject flow_limit_table_len values below 2

[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: 3 · Low: 0 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The Fixes: tag blames the wrong commit.
- [Medium] The new lower-bound check does not reject every value below 2,
  even though the patch title says it does.
- [Medium] net: signedness mismatch in flow_limit_table_len_sysctl()
  accepts INT_MIN

Pre-existing issues:
- [Low] This problem wasn't introduced by this patch, but
  Documentation/networking/scaling.rst does not match the code.

--

net: reject flow_limit_table_len values below 2

This patch makes flow_limit_table_len_sysctl() reject a length of 1 as
well as values that are not powers of two. A length of 1 gave
log_buckets == 0, so skb_flow_limit() computed hash_32(hash, 0). That is a
shift by 32, and it produced an out-of-bounds index into the buckets[]
array.

> Fixes: 99bbc7074190 ("rps: selective flow shedding during softnet overflow")

[Severity: Medium]
Is this the right commit for the Fixes: tag?

Before c3025e94daa9 ("net: rps: change skb_flow_limit() hash function"),
skb_flow_limit() computed the bucket index with a mask:

    new_flow = skb_get_hash(skb) & (fl->num_buckets - 1);

and flow_limit_cpu_sysctl() stored:

    cur->num_buckets = netdev_flow_limit_table_len;

With a length of 1 the mask is 0. The index was always 0, which is inside
the one-byte buckets[] array.

The shift by 32 only became possible when c3025e94daa9 changed this to:

    new_flow = hash_32(skb_get_hash(skb), fl->log_buckets);
    cur->log_buckets = ilog2(netdev_flow_limit_table_len);

With the current tag, stable branches older than c3025e94daa9 would pick
this patch up. In those branches it fixes nothing, but writing 1 to
net.core.flow_limit_table_len would start returning -EINVAL.

Should this be the following instead?

Fixes: c3025e94daa9 ("net: rps: change skb_flow_limit() hash function")

> diff --git a/net/core/sysctl_net_core.c b/net/core/sysctl_net_core.c
> index eb35da3556f4a..bb0fa69f94f5a 100644
> --- a/net/core/sysctl_net_core.c
> +++ b/net/core/sysctl_net_core.c
> @@ -283,7 +283,12 @@ static int flow_limit_table_len_sysctl(const struct ctl_table *table, int write,
>  	ptr = table->data;
>  	old = *ptr;
>  	ret = proc_dointvec(table, write, buffer, lenp, ppos);
> -	if (!ret && write && !is_power_of_2(*ptr)) {
> +	/* A length of 1 is a power of two but gives log_buckets == 0,
> +	 * which makes skb_flow_limit() compute hash_32(hash, 0) -- a
> +	 * shift by 32 -- and index the one-byte buckets[] array with the
> +	 * full 32-bit hash.  Require at least two buckets.
> +	 */
> +	if (!ret && write && (*ptr < 2 || !is_power_of_2(*ptr))) {

[Severity: Medium]
Does the new lower bound hold for every value proc_dointvec() can
store here?

netdev_flow_limit_table_len is a signed int, and proc_dointvec()
parses writes through proc_int_u2k_conv_uop(), which accepts
"-2147483648" and stores INT_MIN. flow_limit_table_len_sysctl() then
reads the result through unsigned int *ptr and sees 0x80000000. That
is not below 2, and is_power_of_2() is true for it, so the write is
accepted even though the subject says values below 2 are rejected.

If a CPU is then enabled through net.core.flow_limit_cpu_bitmap,
flow_limit_cpu_sysctl() computes

    len = sizeof(*cur) + netdev_flow_limit_table_len;

which comes out negative, and kzalloc_node(len, GFP_KERNEL, ...) sees
it as a huge size_t. Without __GFP_NOWARN, that reaches
WARN_ON_ONCE_GFP(order > MAX_PAGE_ORDER, gfp) in alloc_order_allowed().

The check also has no upper bound, and that matters beyond the
allocation size. struct sd_flow_limit records recent bucket indices in
u16 history[FLOW_LIMIT_HISTORY], but skb_flow_limit() computes

    new_flow = hash_32(skb_get_hash(skb), fl->log_buckets);

For a table length above 65536, log_buckets is above 16 and new_flow
is truncated when it is stored into fl->history[]. When that entry
ages out, fl->buckets[old_flow] is decremented for a different bucket
than the one that was incremented. Buckets at index 65536 and above
are incremented but never decremented, so flows that hash there end
up being limited no matter what their real rate is. Lengths from 2^17
through 2^21 still allocate successfully, so this happens without any
warning. Larger values hit the same allocator WARN as the INT_MIN
case.

None of this reads or writes out of bounds, and both the INT_MIN case
and the missing upper bound predate this patch. But since the patch is
tightening exactly this range check, could it be closed completely?
For example, proc_dointvec_minmax() with extra1 = 2 and
extra2 = 1 << 16, together with the existing is_power_of_2() check,
would compare as a signed int and match the u16 history[] limit. Or
is there a reason to keep accepting lengths above 65536?

[Severity: Medium]
Does this check reject every value below 2?

netdev_flow_limit_table_len is a signed int that proc_dointvec() parses.
flow_limit_table_len_sysctl() reads it through an unsigned pointer:

    unsigned int old, *ptr;

so *ptr < 2 is an unsigned comparison.

proc_int_u2k_conv_uop() accepts a write of "-2147483648":

    if (u > (ulong) INT_MAX + 1)
        return -EINVAL;
    WRITE_ONCE(*k_ptr, -u);

and this stores INT_MIN. As an unsigned value, 0x80000000 is not below 2,
and is_power_of_2(0x80000000) is true, so the write succeeds.

The check also has no upper bound, so powers of two up to 2^30 are
accepted.

Later, when a CPU is enabled through net.core.flow_limit_cpu_bitmap,
flow_limit_cpu_sysctl() does:

    len = sizeof(*cur) + netdev_flow_limit_table_len;
    ...
    cur = kzalloc_node(len, GFP_KERNEL, cpu_to_node(i));

For INT_MIN, len is negative and turns into a huge size_t. For values of
2^22 or more, the request needs an order above MAX_PAGE_ORDER. In both
cases the call goes through
kzalloc_node()->___kmalloc_large_node()->__alloc_frozen_pages_noprof()
and reaches alloc_order_allowed():

    return !(WARN_ON_ONCE_GFP(order > MAX_PAGE_ORDER, gfp));

This WARNs, because __GFP_NOWARN is not set. flow_limit_cpu_sysctl() then
returns -ENOMEM partway through its per-CPU loop. The CPUs it already
changed are not rolled back.

The allocation fails, so nothing is read or written out of bounds. The
INT_MIN case and the missing upper bound were also already there before
this patch. But the subject line says values below 2 are rejected, and
INT_MIN still gets through.

Would proc_dointvec_minmax() with extra1 = 2 and a reasonable extra2 cover
both cases? One example is 1 << 16, given the u16 history[] limit.
Comparing as a signed int would also fix the lower bound.

>  		*ptr = old;
>  		ret = -EINVAL;
>  	}

[Severity: Low]
This is a pre-existing issue and was not introduced by this patch, but
Documentation/networking/scaling.rst doesn't match the code. Its Flow Limit
section says the count is kept:

    count over the last 256 packets. If a flow exceeds a set ratio (by

but net/core/dev.h defines:

    #define FLOW_LIMIT_HISTORY	(1 << 7)  /* must be ^2 and !overflow buckets */

That is 128, and it is the window skb_flow_limit() actually uses.

The description of net.core.flow_limit_table_len in the same file also
says nothing about valid values. It mentions neither the power-of-two
requirement nor the new minimum of 2, and writes of any other value fail
with -EINVAL.

Could the documentation be updated along with this change?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261021155505.67208-1-phx0fer%40gmail.com