linux-kernel - Re: [PATCH] futex: Remove unnecessary warning from get_futex

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: <20170809153152.eqbelk4mtot4cmzg@suse.de>
Date:   Wed, 9 Aug 2017 16:31:52 +0100
From:   Mel Gorman <mgorman@...e.de>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Mark Rutland <mark.rutland@....com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Ingo Molnar <mingo@...nel.org>,
        Davidlohr Bueso <dbueso@...e.de>,
        Hugh Dickins <hughd@...gle.com>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] futex: Remove unnecessary warning from get_futex_key

On Wed, Aug 09, 2017 at 05:08:34PM +0200, Peter Zijlstra wrote:
> On Wed, Aug 09, 2017 at 03:43:09PM +0100, Mel Gorman wrote:
> > On Wed, Aug 09, 2017 at 03:05:19PM +0100, Mark Rutland wrote:
> > > > diff --git a/kernel/futex.c b/kernel/futex.c
> > > > index 16dbe4c93895..f50b434756c1 100644
> > > > --- a/kernel/futex.c
> > > > +++ b/kernel/futex.c
> > > > @@ -670,13 +670,14 @@ get_futex_key(u32 __user *uaddr, int fshared, union futex_key *key, int rw)
> > > >  		 * this reference was taken by ihold under the page lock
> > > >  		 * pinning the inode in place so i_lock was unnecessary. The
> > > >  		 * only way for this check to fail is if the inode was
> > > > -		 * truncated in parallel so warn for now if this happens.
> > > > +		 * truncated in parallel which is almost certainly an
> > > > +		 * application bug. In such a case, just retry.
> > > >  		 *
> > > >  		 * We are not calling into get_futex_key_refs() in file-backed
> > > >  		 * cases, therefore a successful atomic_inc return below will
> > > >  		 * guarantee that get_futex_key() will still imply smp_mb(); (B).
> > > >  		 */
> > > > -		if (WARN_ON_ONCE(!atomic_inc_not_zero(&inode->i_count))) {
> > > > +		if (!atomic_inc_not_zero(&inode->i_count)) {
> > > 
> > > I applied the same diff yesterday, and haven't seen anything go wrong
> > > with my test case and/or with Syzkaller running, so FWIW:
> > > 
> > > Tested-by: Mark Rutland <mark.rutland@....com>
> > > 
> > > Thanks for putting this together!
> > > 
> > 
> > No problem. FWIW, I had the test case running for 12 hours in a loop as
> > well and other than having to adjust the number of threads doing futex()
> > to trigger the warning without the patch, I observed no other problems.
> > If Thomas is happy, I hope this can be merged for 4.13 (or picked up
> > directly by Linus if he feels like it). Even if it's delayed, I'll resubmit
> > to -stable manually if the "Cc: stable" gets stripped along the way.
> 
> Probably best if Linus picks this up directly as Thomas is on holidays.
> 

Whoops, I had no idea. Primarily I wanted it go through Thomas under the
general heading of "if you screw up futex.c, Thomas will look for you and
he will find you". The patch in this case is trivial and enough people
have looked at it. Linus, can you pick it up directly please?

-- 
Mel Gorman
SUSE Labs