linux-kernel - Re: [PATCH v2 ratelimit 11/14] ratelimit: Force re-initialization when rate-limiting re-enabled

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fbcffae4-8d88-4bc8-8791-12713f0dc8c1@paulmck-laptop>
Date: Wed, 23 Apr 2025 12:54:39 -0700
From: "Paul E. McKenney" <paulmck@...nel.org>
To: Mark Brown <broonie@...nel.org>
Cc: linux-kernel@...r.kernel.org, kernel-team@...a.com,
	Andrew Morton <akpm@...ux-foundation.org>,
	Kuniyuki Iwashima <kuniyu@...zon.com>,
	Mateusz Guzik <mjguzik@...il.com>, Petr Mladek <pmladek@...e.com>,
	Steven Rostedt <rostedt@...dmis.org>,
	John Ogness <john.ogness@...utronix.de>,
	Sergey Senozhatsky <senozhatsky@...omium.org>,
	Jon Pan-Doh <pandoh@...gle.com>,
	Bjorn Helgaas <bhelgaas@...gle.com>,
	Karolina Stolarek <karolina.stolarek@...cle.com>,
	Aishwarya.TCV@....com, sraithal@....com
Subject: Re: [PATCH v2 ratelimit 11/14] ratelimit: Force re-initialization
 when rate-limiting re-enabled

On Wed, Apr 23, 2025 at 07:59:11PM +0100, Mark Brown wrote:
> On Wed, Apr 23, 2025 at 11:20:59AM -0700, Paul E. McKenney wrote:
> > On Wed, Apr 23, 2025 at 04:59:49PM +0100, Mark Brown wrote:
> 
> > They reported that the replacing the offending commit with the following
> > patch fixed things up:
> 
> >  
> > -	if (!interval || !burst)
> > +	if (interval <= 0 || burst <= 0)
> >  		return 1;
> >  
> 
> That fixes things, yes.
> 
> > If that fixes things for you, could you also please try the following,
> > also replacing that same commit?
> 
> >  
> > -	if (!interval || !burst)
> > +	if (interval <= 0 || burst <= 0) {
> > +		ret = burst > 0;
> >  		return 1;
> > +	}
> 
> Are you sure about that - the value set for ret will be ignored, we
> still return 1 regardless of the value of burst as for the first patch
> AFAICT?  I tried instead:
> 
> +	if (interval <= 0 || burst <= 0) {
> +		return burst > 0;
> +	}
> 
> which failed.

Thank you (and Bert) very much for testing this!

You are of course quite right.  But my previous patch above, the one
that just returned 1 if either interval or burst was non-positive,
was also wrong.

Before that patch, if both burst and interval were negative, rate limiting
would be imposed unconditionally.  With that patch, if if both burst and
interval were negative, rate limiting would never happen, which might
come as a nasty surprise to someone setting burst to zero in order to
unconditionally suppress output.

To preserve necessary portions of the old semantics, I believe that we
need something like this:

	interval:	<0	=0	>0

burst:
	<0		RL	!RL	RL

	=0		RL	!RL	RL

	>0		!RL	!RL	?RL

Here, "RL" says to always do rate-limiting, "!RL" says to never do
rate-limiting, and "?RL" says to let the atomic decrement decide.

The idea is that if interval is =0, rate-limiting never happens.  But if
interval <0, then burst has full control, so that burst <=0 always
rate-limits and burst >0 never rate-limits.  Finally, if interval >0,
burst again has full control, but while burst <=0 always rate-limits,
burst >0 lets the atomic decrement decide.

Except that in the "?RL" case, the "if" condition directs execution
elsewhere anyway, so I believe that the "interval == 0 || burst > 0"
condition in my updated patch (see below) fixes things.

Could you please try the patch shown below?  I might actually have
it right this time.  ;-)

							Thanx, Paul

------------------------------------------------------------------------

diff --git a/lib/ratelimit.c b/lib/ratelimit.c
index 04f16b8e24575..d6531e5c6ec4e 100644
--- a/lib/ratelimit.c
+++ b/lib/ratelimit.c
@@ -35,8 +35,8 @@ int ___ratelimit(struct ratelimit_state *rs, const char *func)
 	unsigned long flags;
 	int ret;

-	if (!interval || !burst)
-		return 1;
+	if (interval <= 0 || burst <= 0)
+		return interval == 0 || burst > 0;

 	/*
 	 * If we contend on this state's lock then just check if