linux-kernel - Re: [PATCH] android: binder: Disable preemption while holding the global binder lock

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:   Sat, 10 Sep 2016 19:28:43 +0200
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     Peter Zijlstra <peterz@...radead.org>, devel@...verdev.osuosl.org,
        Riley Andrews <riandrews@...roid.com>,
        linux-kernel@...r.kernel.org,
        Christoph Hellwig <hch@...radead.org>,
        Arve Hj??nnev??g <arve@...roid.com>,
        Todd Kjos <tkjos@...gle.com>
Subject: Re: [PATCH] android: binder: Disable preemption while holding the
 global binder lock

On Sat, Sep 10, 2016 at 06:37:29PM +0200, Thomas Gleixner wrote:
> On Sat, 10 Sep 2016, Peter Zijlstra wrote:
> 
> > On Sat, Sep 10, 2016 at 09:16:59AM -0700, Christoph Hellwig wrote:
> > > On Thu, Sep 08, 2016 at 09:12:50AM -0700, Todd Kjos wrote:
> > > > In Android systems, the display pipeline relies on low
> > > > latency binder transactions and is therefore sensitive to
> > > > delays caused by contention for the global binder lock.
> > > > Jank is siginificantly reduced by disabling preemption
> > > > while the global binder lock is held.
> > > 
> > > That's now how preempt_disable is supposed to use.  It is for critical
> > 
> > not, that's supposed to be _not_. Just to be absolutely clear, this is
> > NOT how you're supposed to use preempt_disable().
> > 
> > > sections that use per-cpu or similar resources.
> > > 
> > > > 
> > > > Originally-from: Riley Andrews <riandrews@...gle.com>
> > > > Signed-off-by: Todd Kjos <tkjos@...gle.com>
> > 
> > > > @@ -389,7 +390,11 @@ static int task_get_unused_fd_flags(struct
> > > > binder_proc *proc, int flags)
> > > >   rlim_cur = task_rlimit(proc->tsk, RLIMIT_NOFILE);
> > > >   unlock_task_sighand(proc->tsk, &irqs);
> > > > 
> > > > - return __alloc_fd(files, 0, rlim_cur, flags);
> > > > + preempt_enable_no_resched();
> > > > + ret = __alloc_fd(files, 0, rlim_cur, flags);
> > > > + preempt_disable();
> > 
> > And the fact that people want to use preempt_enable_no_resched() shows
> > that they're absolutely clueless.
> > 
> > This is so broken its not funny.
> > 
> > NAK NAK NAK
> 
> Indeed. Sprinkling random preempt_enabe/disable() pairs all over the place
> documents clearly that this is tinkering and not proper software
> engineering.

I have pointed out in the other thread for this patch (the one that had
a patch that could be applied) that the single lock in the binder code
is the main problem here, it should be solved instead of this messing
around with priorities.

So don't worry, I'm not taking this change :)

thanks,

greg k-h