linux-kernel - Re: Re: [PATCH] futex: Remove the owner check when waking task in handle_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: <OFFC4F71E6.FCD3279D-ON48257BF3.000E2A58-48257BF3.00251AFA@zte.com.cn>
Date:	Fri, 27 Sep 2013 14:45:05 +0800
From:	zhang.yi20@....com.cn
To:	Darren Hart <dvhart@...ux.intel.com>
Cc:	linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...nel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Thomas Gleixner <tglx@...utronix.de>
Subject: Re: Re: [PATCH] futex: Remove the owner check when waking task in
 handle_futex_death



Darren Hart <dvhart@...ux.intel.com> wrote on 2013/09/27 02:15:17:


> Re: [PATCH] futex: Remove the owner check when waking task in handle_futex_death
>
> On Thu, 2013-09-26 at 09:09 +0800, zhang.yi20@....com.cn wrote:
> > Hi all,
> >
> > Task processes all its owned robust futex when it is exiting,
> > to ensure the futexes can be taken by other tasks.
> >
> > Though this can not work good in sometimes.
> > Think about this scene：
> > 1. A robust mutex is shared for two processes, each process has
> >    multi threads to lock the mutex.
> > 2. One of the threads locks the mutex, and the others are waiting
> >    and sorted in order of priority.
> > 3. The process to which the mutex owner thread belongs is dying
> >    without unlocking the mutex，and handle_futex_death is invoked
> >    to wake the first waiter.
> > 4. If the first waiter belongs to the same process，it has no chance
> >    to return to the userspace to lock the mutex, and it won't wake
> >    the next waiter because it is not the owner of the mutex.
> > 5. The rest waiters of the other process may block forever.
> >
> > This patch remove the owner check when waking task in handle_futex_death.
> > If above occured, The dying task can wake the next waiter by processing its list_op_pending.
> > The waked task could return to userspace and try to lock the mutex again.
> >
>
> The problem is if you allow the non-owner to do the wake, you risk
> multiple threads calling futex_wake(). Or is that your intention? Wake
> one waiter for every thread that calls handle_futex_death()?

Yes.

> >
> > Signed-off-by: Zhang Yi <zhang.yi20@....com.cn>
> > Reviewed-by: Xie Baoyou <xie.baoyou@....com.cn>
> > Reviewed-by: Lu Zhongjun <lu.zhongjun@....com.cn>
> >
> >
> >
> > --- linux/kernel/futex.c   2013-09-25 09:24:34.639634244 +0000
> > +++ linux/kernel/futex.c   2013-09-25 10:12:17.619673546 +0000
> > @@ -2541,14 +2541,15 @@ retry:
> >        }
> >        if (nval != uval)
> >           goto retry;
> > -
> > -      /*
> > -       * Wake robust non-PI futexes here. The wakeup of
> > -       * PI futexes happens in exit_pi_state():
> > -       */
> > -      if (!pi && (uval & FUTEX_WAITERS))
> > -         futex_wake(uaddr, 1, 1, FUTEX_BITSET_MATCH_ANY);
> >     }
> > +
> > +   /*
> > +    * Wake robust non-PI futexes here. The wakeup of
> > +    * PI futexes happens in exit_pi_state():
> > +    */
> > +   if (!pi)
>
>
> Why did you drop the FUTEX_WAITERS condition?
>
> You sent a different patch earlier this year that didn't appear to get
> any review:
>
> https://lkml.org/lkml/2013/4/8/65
>
> This one woke all the waiters and let them sort it out.
>
> It seems we've hashed through this already, but I'm not finding the
> email logs and I don't recall off the top of my head.
>
> > +      futex_wake(uaddr, 1, 1, FUTEX_BITSET_MATCH_ANY);
> > +
> >     return 0;
> >  }
>

The earlier patch cannot solve another problem:
The owner wakes the next waiter through normal unlocking which make the
futex value as zero, the waked task exits before actually locking the mutex.
In this case, the owner doesn't call handle_futex_death() and the waked task
doesn't call futex_wake() when they are dying. The rest waiters will still
block as the same.

This is also the reason that I drop the owner and FUTEX_WAITERS check,
because the futex value can be zero at that time.

In normal case, the userspace code ensure that the owner of lock in robust-list
is current task, we may do redundant waking just for the  list_op_pending.
So I think the patch results in little negative effect.