Re: [PATCH net 5/5] net: restrict message_cost and message_burst to non-negative values

[Fernando Fernandez Mancera <fmancera@suse.de>]

On 10/23/26 2:19 PM, Joel Granados wrote:
> On Fri, Oct 23, 2026 at 05:13:31PM +0800, Jiayuan Chen wrote:
>> net.core.message_cost and net.core.message_burst feed the interval and
>> burst of net_ratelimit_state, but neither has a lower bound:
>> message_burst uses plain proc_dointvec, and message_cost uses
>> proc_dointvec_jiffies, which happily stores -1 second as -HZ jiffies.
>>
>> Since commit a940d145cc38 ("ratelimit: Warn if ->interval or ->burst are
>> negative"), ___ratelimit() splats on such values, so the next
>> net_ratelimit() caller warns, e.g. a martian source log:
>>
>>    Negative interval (-250) or burst (10): Uninitialized ratelimit_state structure?
>>    WARNING: lib/ratelimit.c:43 at ___ratelimit+0x56e/0x960, CPU#1: ping/3218
>>    RIP: 0010:___ratelimit+0x578/0x960
>>    Call Trace:
>>     <IRQ>
>>     net_ratelimit+0x1c/0x30
>>     ip_handle_martian_source+0x1bb/0x340
>>     ip_route_input_slow+0x1057/0x3650
>>     ip_route_input_noref+0x104/0x260
>>     arp_process+0x109b/0x2510
>>     arp_rcv+0x381/0x460
>>     __netif_receive_skb_one_core+0x173/0x1c0
>>     __netif_receive_skb+0x21/0x150
>>     process_backlog+0x37e/0x1200
>>     __napi_poll.constprop.0+0xa5/0x5e0
>>     net_rx_action+0x4c0/0xf70
>>     handle_softirqs+0x204/0x9b0
>>     __do_softirq+0x10/0x18
>>     do_softirq.part.0+0x49/0xd0
>>     </IRQ>
>>
>> Negative values never meant anything here: ___ratelimit() treats them
>> like 0, and 0 already expresses both "no limit" (interval) and
>> "suppress everything" (burst).
>>
>> Reject them at write time. message_burst can simply use
>> proc_dointvec_minmax with extra1 = 0. proc_dointvec_jiffies ignores
>> extra1/extra2, so give message_cost a small handler that parses into a
>> temporary and only stores the value if it is not negative.
>>
>> Fixes: a940d145cc38 ("ratelimit: Warn if ->interval or ->burst are negative")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
>> ---
>>   net/core/sysctl_net_core.c | 21 +++++++++++++++++++--
>>   1 file changed, 19 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/core/sysctl_net_core.c b/net/core/sysctl_net_core.c
>> index 473c162736d6..8146ed819695 100644
>> --- a/net/core/sysctl_net_core.c
>> +++ b/net/core/sysctl_net_core.c
>> @@ -378,6 +378,22 @@ static int proc_do_skb_defer_max(const struct ctl_table *table, int write,
>>   	return ret;
>>   }
>>   
>> +static int proc_do_message_cost(const struct ctl_table *table, int write,
>> +				void *buffer, size_t *lenp, loff_t *ppos)
>> +{
>> +	int ret, interval = READ_ONCE(*(int *)table->data);
>> +	struct ctl_table tmp = *table;
>> +
>> +	tmp.data = &interval;
>> +	ret = proc_dointvec_jiffies(&tmp, write, buffer, lenp, ppos);
>> +	if (write && !ret) {
>> +		if (interval < 0)
>> +			return -EINVAL;
>> +		WRITE_ONCE(*(int *)table->data, interval);
>> +	}
>> +	return ret;
>> +}
>> +
>>   #ifdef CONFIG_BPF_JIT
>>   static int proc_dointvec_minmax_bpf_enable(const struct ctl_table *table, int write,
>>   					   void *buffer, size_t *lenp,
>> @@ -544,14 +560,15 @@ static struct ctl_table net_core_table[] = {
>>   		.data		= &net_ratelimit_state.interval,
>>   		.maxlen		= sizeof(int),
>>   		.mode		= 0644,
>> -		.proc_handler	= proc_dointvec_jiffies,
>> +		.proc_handler	= proc_do_message_cost,
>>   	},
>>   	{
>>   		.procname	= "message_burst",
>>   		.data		= &net_ratelimit_state.burst,
>>   		.maxlen		= sizeof(int),
>>   		.mode		= 0644,
>> -		.proc_handler	= proc_dointvec,
>> +		.proc_handler	= proc_dointvec_minmax,
>> +		.extra1		= SYSCTL_ZERO,
>>   	},
>>   #ifdef CONFIG_RPS
>>   	{
>> -- 
>> 2.43.0
>>
> 
> I would prefer something like this [1] (not tested and not compiled).
> This is more in line with the patterns inside the jiffies.c file. I put
> SYSCTL_ZERO there, but I have a doubt if you actually need SYSCTL_ONE.
> 
> If the interval variable can be zero, then leave it as SYSCTL_ZERO, but
> if it cant be zero and can be one the use SYSCTL_ONE
> 

FWIW, I also like this solution and indeed we can convert other users in 
the future.

About SYSCTL_ZERO, I believe it is the right choice. AFAIU, it just 
disables limiting.

Thanks,
Fernando.

> best
> 
> 
> [1]
> 
> diff --git i/include/linux/jiffies.h w/include/linux/jiffies.h
> index bbd57061802c..288ba81ed53e 100644
> --- i/include/linux/jiffies.h
> +++ w/include/linux/jiffies.h
> @@ -646,6 +646,8 @@ extern unsigned long nsecs_to_jiffies(u64 n);
>   struct ctl_table;
>   int proc_dointvec_jiffies(const struct ctl_table *table, int dir, void *buffer,
>                            size_t *lenp, loff_t *ppos);
> +int proc_dointvec_jiffies_minmax(const struct ctl_table *table, int dir, void *buffer,
> +                                size_t *lenp, loff_t *ppos);
>   int proc_dointvec_ms_jiffies_minmax(const struct ctl_table *table, int dir,
>                                      void *buffer, size_t *lenp, loff_t *ppos);
>   int proc_dointvec_userhz_jiffies(const struct ctl_table *table, int dir,
> diff --git i/kernel/time/jiffies.c w/kernel/time/jiffies.c
> index 9b3487d40cd6..cf91996fe4e3 100644
> --- i/kernel/time/jiffies.c
> +++ w/kernel/time/jiffies.c
> @@ -165,6 +165,14 @@ static int do_proc_int_conv_jiffies(bool *negp, ulong *u_ptr, int *k_ptr,
>                               sysctl_u2k_int_conv_hz, sysctl_k2u_int_conv_hz);
>   }
> 
> +static int do_proc_int_conv_jiffies_minmax(bool *negp, ulong *u_ptr, int *k_ptr,
> +                                   int dir, const struct ctl_table *tbl)
> +{
> +       return proc_int_conv(negp, u_ptr, k_ptr, dir, tbl, true,
> +                            sysctl_u2k_int_conv_hz, sysctl_k2u_int_conv_hz);
> +}
> +
> +
>   static int do_proc_int_conv_userhz_jiffies(bool *negp, ulong *u_ptr,
>                                             int *k_ptr, int dir,
>                                             const struct ctl_table *tbl)
> @@ -263,6 +271,15 @@ int proc_dointvec_jiffies(const struct ctl_table *table, int dir,
>   }
>   EXPORT_SYMBOL(proc_dointvec_jiffies);
> 
> +int proc_dointvec_jiffies_minmax(const struct ctl_table *table, int dir,
> +                         void *buffer, size_t *lenp, loff_t *ppos)
> +{
> +       return proc_dointvec_conv(table, dir, buffer, lenp, ppos,
> +                                 do_proc_int_conv_jiffies_minmax);
> +}
> +EXPORT_SYMBOL(proc_dointvec_jiffies);
> +
> +
>   /**
>    * proc_dointvec_userhz_jiffies - read a vector of integers as 1/USER_HZ seconds
>    * @table: the sysctl table
> diff --git i/net/core/sysctl_net_core.c w/net/core/sysctl_net_core.c
> index eb35da3556f4..7226697bf7c1 100644
> --- i/net/core/sysctl_net_core.c
> +++ w/net/core/sysctl_net_core.c
> @@ -538,7 +538,8 @@ static struct ctl_table net_core_table[] = {
>                  .data           = &net_ratelimit_state.interval,
>                  .maxlen         = sizeof(int),
>                  .mode           = 0644,
> -               .proc_handler   = proc_dointvec_jiffies,
> +               .proc_handler   = proc_dointvec_jiffies_minmax,
> +               .extra1         = SYSCTL_ZERO,
>          },
>          {
>                  .procname       = "message_burst",