linux-kernel - Re: [PATCHv3] lib/ratelimit: Lockless ratelimiting

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]

Date:   Fri, 20 Jul 2018 17:09:48 +0200
From:   Petr Mladek <pmladek@...e.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Dmitry Safonov <dima@...sta.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Arnd Bergmann <arnd@...db.de>, David Airlie <airlied@...ux.ie>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jani Nikula <jani.nikula@...ux.intel.com>,
        Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
        Rodrigo Vivi <rodrigo.vivi@...el.com>,
        Theodore Ts'o <tytso@....edu>,
        Thomas Gleixner <tglx@...utronix.de>,
        intel-gfx@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org
Subject: Re: [PATCHv3] lib/ratelimit: Lockless ratelimiting

On Wed 2018-07-18 19:49:10, Andy Shevchenko wrote:
> On Tue, Jul 17, 2018 at 3:59 AM, Dmitry Safonov <dima@...sta.com> wrote:
> > I would be glad if someone helps/bothers to review the change :C
> >
> 
> Perhaps Petr and / or Steven can help you.


> > Thanks,
> > Dmitry
> >
> > On Tue, 2018-07-03 at 23:56 +0100, Dmitry Safonov wrote:
> >> Currently ratelimit_state is protected with spin_lock. If the .lock
> >> is
> >> taken at the moment of ___ratelimit() call, the message is suppressed
> >> to
> >> make ratelimiting robust.
> >>
> >> That results in the following issue issue:
> >>       CPU0                          CPU1
> >> ------------------           ------------------
> >> printk_ratelimit()           printk_ratelimit()
> >>         |                             |
> >>   try_spin_lock()              try_spin_lock()
> >>         |                             |
> >> time_is_before_jiffies()          return 0; // suppress
> >>
> >> So, concurrent call of ___ratelimit() results in silently suppressing
> >> one of the messages, regardless if the limit is reached or not.
> >> And rc->missed is not increased in such case so the issue is covered
> >> from user.
> >>
> >> Convert ratelimiting to use atomic counters and drop spin_lock.
> >>
> >> Note: That might be unexpected, but with the first interval of
> >> messages
> >> storm one can print up to burst*2 messages. So, it doesn't guarantee
> >> that in *any* interval amount of messages is lesser than burst.
> >> But, that differs lightly from previous behavior where one can start
> >> burst=5 interval and print 4 messages on the last milliseconds of
> >> interval + new 5 messages from new interval (totally 9 messages in
> >> lesser than interval value):
> >>
> >>    msg0              msg1-msg4 msg0-msg4
> >>     |                     |       |
> >>     |                     |       |
> >>  |--o---------------------o-|-----o--------------------|--> (t)
> >>                           <------->
> >>                        Lesser than burst
> >>

I am a bit confused. Does this patch fix two problems?

   + Lost rc->missed update when try_spin_lock() fails
   + printing up to burst*2 messages in a give interval

It would make the review much easier if you split it into more
patches and fix the problems separately.

Otherwise, it looks promissing...

Best Regards,
Petr

PS: I have vacation the following two weeks. Anyway, please, CC me
if you send any new version.

Best Regards,
Petr