netdev - Re: printk_ratelimit and net_ratelimit conflict and tunable behavior

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>] [day] [month] [year] [list]

Message-Id: <200802281610.m1SGAdMe023192@ftw.mot.com>
Date:	Thu, 28 Feb 2008 10:10:39 -0600
From:	Steven Hawkes <fsh016@....mot.com>
To:	davem@...emloft.net, joe@...ches.com, linux-kernel@...r.kernel.org,
	netdev@...r.kernel.org
Subject: Re: printk_ratelimit and net_ratelimit conflict and tunable behavior

Joe Perches wrote:

> On Mon, 2008-02-25 at 17:49 -0600, Hawkes Steve-FSH016 wrote:
> > Are you saying the few lines of code to handle changes to the tunables
> > aren't worth keeping?
> 
> Yes.
> 
> I think the tunables, if needed at all, should be set by modifying
> the struct and the call might as well be:
> 
> bool __printk_ratelimit(struct printk_ratelimit_state *state)

The tunables are used in the current rate-limiting algorithm. Wouldn't
incorporating them into the structure require protecting modification of the
tunables by the same spinlock used in the rate limiting? That could be done
by pulling the spinlock variable out into printk_ratelimit() and
net_ratelimit() and into the struct (the spinlock is needed internal to
__printk_ratelimit to allow the spin_unlock() done right before actually
printing the message). That seems a bit more complex.

Or are you suggesting copying the tunables into the struct each time
__printk_ratelimit() is called? I was looking at them as not part of the
state of rate limiting, but rather external attributes controlling rate
limiting.

Joe Perches wrote:

> Another quibble is not directed to your change because it's
> preexisting but "tok" isn't a good name and may not even need
> to be in the structure.  It does save a multiply though.

I agree the original name can be improved upon.

The toks state variable is needed because it actually maintains the current
rate-limiting water level, so to speak. The "bucket" is initially filled to
its capacity, ratelimit_jiffies * ratelimit_burst. Each time
__printk_ratelimit is called, water gets added to the bucket in proportion
to the time since the last call (capped by the capacity of the
bucket). Prints are allowed as long as the bucket has at least
ratelimit_jiffies of water left. Each allowed print sucks a
ratelimit_jiffies amount out of the bucket. (At least I think that's the way
the current kernel works; it wasn't immediately obvious to me.)
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html