linux-kernel - Re: [PATCH] kernel/futex.c: notice the return value after rt_mutex_finish_proxy

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, 12 Sep 2013 15:37:01 -0700
From:	Darren Hart <dvhart@...ux.intel.com>
To:	Thomas Gleixner <tglx@...utronix.de>
Cc:	Chen Gang <gang.chen@...anux.com>, ccross@...roid.com,
	Mel Gorman <mgorman@...e.de>,
	Andrew Morton <akpm@...ux-foundation.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] kernel/futex.c: notice the return value after
 rt_mutex_finish_proxy_lock() fails

On Thu, 2013-09-12 at 16:32 +0200, Thomas Gleixner wrote:
> On Tue, 20 Aug 2013, Chen Gang wrote:
> 
> > rt_mutex_finish_proxy_lock() can return failure code (e.g. -EINTR,
> > -ETIMEDOUT).
> > 
> > Original implementation has already noticed about it, but not check it
> > before next work.
> > 
> > Also let coments within 80 columns to pass "./scripts/checkpatch.pl".
> > 
> > 
> > Signed-off-by: Chen Gang <gang.chen@...anux.com>
> > ---
> >  kernel/futex.c |   30 ++++++++++++++++--------------
> >  1 files changed, 16 insertions(+), 14 deletions(-)
> > 
> > diff --git a/kernel/futex.c b/kernel/futex.c
> > index c3a1a55..1a94e7d 100644
> > --- a/kernel/futex.c
> > +++ b/kernel/futex.c
> > @@ -2373,21 +2373,23 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
> >               ret = rt_mutex_finish_proxy_lock(pi_mutex, to, &rt_waiter, 1);
> >               debug_rt_mutex_free_waiter(&rt_waiter);
> >  
> > -             spin_lock(q.lock_ptr);
> > -             /*
> > -              * Fixup the pi_state owner and possibly acquire the lock if we
> > -              * haven't already.
> > -              */
> > -             res = fixup_owner(uaddr2, &q, !ret);
> > -             /*
> > -              * If fixup_owner() returned an error, proprogate that.  If it
> > -              * acquired the lock, clear -ETIMEDOUT or -EINTR.
> > -              */
> > -             if (res)
> > -                     ret = (res < 0) ? res : 0;
> > +             if (!ret) {
> 
> Again. This is completely wrong!  
> 
> We MUST call fixup_owner even if finish_proxy_lock() returned with an
> error code. Simply because finish_proxy_lock() is called outside of
> the spin_lock(q.lock_ptr) region and another thread might have
> modified the futex state. So we need to handle the corner cases
> otherwise we might leave the futex in some undefined state.
> 
> You're reintroducing a hard to decode bug, which got analyzed and
> fixed in futex_lock_pi() years ago. See the history for the
> explanation.
> 
> Sigh.
> 
>         tglx

Chen, perhaps you can let us know what the failure scenario is that you
are trying to address with this patch. I only replied the once as I
pointed out the corner-case and expected you to follow up with that.
This region of code is very fragile to modifications as it has become
more corner-cases than core logic in some places :-)

For starters, I'm not following your second sentence in the commit log.
Can you elaborate on the following?

"Original implementation has already noticed about it, but not check it
before next work."

Do you have a test-case that demonstrates a failure mode?

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/