linux-kernel - Re: [PATCH] hugetlb: select PREEMPT_COUNT if HUGETLB_PAGE for in

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:   Thu, 11 Mar 2021 10:32:24 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     Michal Hocko <mhocko@...e.com>
Cc:     Mike Kravetz <mike.kravetz@...cle.com>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org,
        "Paul E . McKenney" <paulmck@...nel.org>,
        Shakeel Butt <shakeelb@...gle.com>, tglx@...utronix.de,
        john.ogness@...utronix.de, urezki@...il.com, ast@...com,
        Eric Dumazet <edumazet@...gle.com>,
        Mina Almasry <almasrymina@...gle.com>,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH] hugetlb: select PREEMPT_COUNT if HUGETLB_PAGE for
 in_atomic use

On Thu, Mar 11, 2021 at 10:01:22AM +0100, Michal Hocko wrote:
> On Thu 11-03-21 09:46:30, Peter Zijlstra wrote:
> > On Wed, Mar 10, 2021 at 06:13:21PM -0800, Mike Kravetz wrote:
> > > from irq context.  Changing the check in the code from !in_task to
> > > in_atomic would handle the situations when called with irqs disabled.
> > 
> > It does not. local_irq_disable() does not change preempt_count().
> 
> You are right. Earlier I was suggesting to check of irq_disabled() as
> well http://lkml.kernel.org/r/YD4I+VPr3UNt063H@dhcp22.suse.cz
> 
> back then it was not really clear to me that in fact we do care about
> spin locks more than irq disabled code. I am not even sure whether we
> need to care about irq disabled regions without any locks held that
> wouldn't be covered by in_atomic. But it would be safer to add
> irq_disabled check as well.

Safer still is always doing it, replace it with if (true).

What's the purpose, doing the minimal 'correct', of the maximal safe
solution?

The whole changelog reads like a trainwreck, but akpm already commented
on that. I picked out a small factual incorrectness, simply because if
you can't get that right, the whole argument looses weight.

That said, I don't think you actually need it, if as you write the lock
should be IRQ-safe, then you're worried about the IRQ recursion
deadlock:


  spin_lock(&A);
  <IRQ>
	spin_lock(&A);


And that obviously cannot happen when IRQs are disabled, even if you
were holding a lock.


Anyway, I think I support Andrew here, this needs proper fixing, not
bandaids.