linux-kernel - Re: [PATCH] ratelimit: check the condition in WARN

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <502E8A2C.3060606@suse.cz>
Date:	Fri, 17 Aug 2012 20:15:08 +0200
From:	Jiri Slaby <jslaby@...e.cz>
To:	Joe Perches <joe@...ches.com>
CC:	davem@...emloft.net, jirislaby@...il.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ratelimit: check the condition in WARN_RATELIMIT first

On 08/17/2012 07:39 PM, Joe Perches wrote:
> On Fri, 2012-08-17 at 15:42 +0200, Jiri Slaby wrote:
>> Before calling __ratelimit in __WARN_RATELIMIT, check the condition
>> first. When this check was not there, we got constant income of:
>> tty_init_dev: 60 callbacks suppressed
>> tty_init_dev: 59 callbacks suppressed
> []
>> diff --git a/include/linux/ratelimit.h b/include/linux/ratelimit.h
> []
>> @@ -49,8 +49,9 @@ extern int ___ratelimit(struct ratelimit_state *rs, const char *func);
>>  #define __WARN_RATELIMIT(condition, state, format...)		\
>>  ({								\
>>  	int rtn = 0;						\
>> -	if (unlikely(__ratelimit(state)))			\
>> -		rtn = WARN(condition, format);			\
>> +	int __rtcond = !!condition;				\
>> +	if (unlikely(__rtcond && __ratelimit(state)))		\
>> +		rtn = WARN(__rtcond, format);			\
>>  	rtn;							\
>>  })
>>  
> 
> Hi Jiri.
> 
> This seems fine to me but are there any conditions that
> are computationally expensive?

It's not about expensiveness of the computation. The complexity remained
the same except I moved the computation one layer up.

> ratelimit(state) isn't
> and this will now always do condition.
> 
> (looks instead of speculates)
> 
> There's 1 current use of WARN_RATELIMIT and there's
> a condition of 1 so there's no problem here.

There is going to be one more in monday's -next. I've just added one to
the TTY code. The thing is that when you call ratelimit(state) it will
emit how many times you have called that function like I described in
the changelog:
tty_init_dev: 60 callbacks suppressed

Even when the condition is always false. Hence I added the condition to
the if and lazy evaluation will take care and ratelimit() won't be
called at all...

> __WARN_RATELIMIT is pretty stupid.
> It's only called from WARN_RATELIMIT.
> I think it shouldn't exist at all.
> 
> Maybe something like this?

Yup, something like that looks OK to me.

thanks,
-- 
js
suse labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/