linux-kernel - Re: [PATCH -v6 13/13] futex: futex_lock_pi() vs PREEMPT_RT

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, 7 Apr 2017 19:26:10 -0700
From:   Darren Hart <dvhart@...radead.org>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     tglx@...utronix.de, mingo@...nel.org, juri.lelli@....com,
        rostedt@...dmis.org, xlpang@...hat.com, bigeasy@...utronix.de,
        linux-kernel@...r.kernel.org, mathieu.desnoyers@...icios.com,
        jdesfossez@...icios.com, bristot@...hat.com
Subject: Re: [PATCH -v6 13/13] futex: futex_lock_pi() vs PREEMPT_RT_FULL

On Wed, Mar 22, 2017 at 11:36:00AM +0100, Peter Zijlstra wrote:
> When PREEMPT_RT_FULL does the spinlock -> rt_mutex substitution the PI
> chain code will (falsely) report a deadlock and BUG.
> 
> The problem is that we hold hb->lock (now an rt_mutex) while doing
> task_blocks_on_rt_mutex on the futex's pi_state::rtmutex. This, when
> interleaved just right with futex_unlock_pi() leads it to believe we
> have an AB-BA deadlock.
> 
>   Task1 (holds rt_mutex,	Task2 (does FUTEX_LOCK_PI)
>          does FUTEX_UNLOCK_PI)
> 
> 				lock hb->lock
> 				lock rt_mutex (as per start_proxy)
>   lock hb->lock
> 
> Which is a trivial AB-BA.
> 
> It is not an actual deadlock, because we won't be holding hb->lock by
> the time we actually block on rt_mutex, but the chainwalk code doesn't
> know that.
> 
> To avoid this problem, do the same thing we do in futex_unlock_pi()
> and drop hb->lock after acquiring wait_lock. This still fully
> serializes against futex_unlock_pi(), since adding to the wait_list
> does the very same lock dance, and removing it holds both locks.
> 
> Reported-by: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
> Suggested-by: Thomas Gleixner <tglx@...utronix.de>
> Tested-by: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>

I have gone through each of these carefully, and while I'm not naive enough to
say "there are no possible locking problems", each of Peter's claims were
supported by my review. I went down a number of paths which concerned me, but
eventually they each proved not to be a problem, and it was impressive to see
the knot of locks loosen and come free in the last few patches. That's a really
nice piece of work Peter.

I've made several comments on the comment blocks and commit messages to clarify
things where I think they would have saved me time or were inconsistent. I've
only made one code change recommendation iirc, which was the simple type
declaration of a new uval from int to u32.

I would like to see more testing because... well... futexes. But, we don't have
a futex torture suite yet, but that is something I'm hoping to be looking into
in the near future. What testing we do have available has passed between my
futex selftests, the LTP suite, the pi_stress, and the RT runs by Sebastian.

Peter, I presume there will be a v7 with the u32 change and hopefully a couple
text updates?

Thanks,

-- 
Darren Hart
VMware Open Source Technology Center