linux-kernel - Re: [PATCH] powerpc/qspinlock: Fix deadlock in MCS queue

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: <r5teromuglnzq223dbou22m6if4ustlrvmp3tpvyjfjwatysta@s7ttblybzrxz>
Date: Wed, 28 Aug 2024 09:57:49 +0530
From: "Nysal Jan K.A." <nysal@...ux.ibm.com>
To: Nicholas Piggin <npiggin@...il.com>
Cc: Michael Ellerman <mpe@...erman.id.au>, stable@...r.kernel.org,
        Geetika Moolchandani <geetika@...ux.ibm.com>,
        Vaishnavi Bhat <vaish123@...ibm.com>,
        Jijo Varghese <vargjijo@...ibm.com>,
        Christophe Leroy <christophe.leroy@...roup.eu>,
        Naveen N Rao <naveen@...nel.org>, linuxppc-dev@...ts.ozlabs.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] powerpc/qspinlock: Fix deadlock in MCS queue

On Wed, Aug 28, 2024 at 01:19:46PM GMT, Nicholas Piggin wrote:
<snip>
> What probably makes it really difficult to hit is that I think both
> locks A and B need contention from other sources to push them into
> queueing slow path. I guess that's omitted for brevity in the flow
> above, which is fine.
> 

I'll mention that in the commit message, just so that it is clear.

> 
> AFAIKS this fix works.
> 
> There is one complication which is those two stores could be swapped by
> the compiler. So we could take an IRQ here that sees the node has been
> freed, but node->lock has not yet been cleared. Basically equivalent to
> the problem solved by the barrier() on the count++ side.
> 
> This reordering would not cause a problem in your scenario AFAIKS
> because when the lock call returns, node->lock *will* be cleared so it
> can not cause a problem later.
> 
> Still, should we put a barrier() between these just to make things a
> bit cleaner? I.e., when count is decremented, we definitely won't do
> any other stores to node. Otherwise,
> 

Agree, that will make it cleaner.

> Reviewed-by: Nicholas Piggin <npiggin@...il.com>
> 
> Thanks,
> Nick

Thanks for the review Nick, I'll send a v2 with these changes.

Regards
--Nysal