linux-kernel - Re: [PATCH v6 05/15] lockdep: Implement crossrelease feature

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:   Tue, 25 Apr 2017 14:40:44 +0900
From:   Byungchul Park <byungchul.park@....com>
To:     Peter Zijlstra <peterz@...radead.org>
CC:     <mingo@...nel.org>, <tglx@...utronix.de>, <walken@...gle.com>,
        <boqun.feng@...il.com>, <kirill@...temov.name>,
        <linux-kernel@...r.kernel.org>, <linux-mm@...ck.org>,
        <iamjoonsoo.kim@....com>, <akpm@...ux-foundation.org>,
        <willy@...radead.org>, <npiggin@...il.com>, <kernel-team@....com>
Subject: Re: [PATCH v6 05/15] lockdep: Implement crossrelease feature

On Mon, Apr 24, 2017 at 12:17:47PM +0200, Peter Zijlstra wrote:
> On Mon, Apr 24, 2017 at 02:11:02PM +0900, Byungchul Park wrote:
> > On Wed, Apr 19, 2017 at 04:25:03PM +0200, Peter Zijlstra wrote:
> 
> > > I still don't like work_id; it doesn't have anything to do with
> > > workqueues per se, other than the fact that they end up using it.
> > > 
> > > It's a history generation id; touching it completely invalidates our
> > > history. Workqueues need this because they run independent work from the
> > > same context.
> > > 
> > > But the same is true for other sites. Last time I suggested
> > > lockdep_assert_empty() to denote all suck places (and note we already
> > > have lockdep_sys_exit() that hooks into the return to user path).
> > 
> > I'm sorry but I don't understand what you intend. It would be appriciated
> > if you explain more.
> > 
> > You might know why I introduced the 'work_id'.. Is there any alternative?
> 
> My complaint is mostly about naming.. and "hist_gen_id" might be a
> better name.

Ah, I also think the name, 'work_id', is not good... and frankly I am
not sure if 'hist_gen_id' is good, either. What about to apply 'rollback',
which I did for locks in irq, into works of workqueues? If you say yes,
I will try to do it.

> But let me explain.
> 
> 
> The reason workqueues need this is because the lock history for each
> 'work' are independent. The locks of Work-B do not depend on the locks
> of the preceding Work-A, because the completion of Work-B is not
> dependent on those locks.
> 
> But this is true for many things; pretty much all kthreads fall in this
> pattern, where they have an 'idle' state and future completions do not
> depend on past completions. Its just that since they all have the 'same'
> form -- the kthread does the same over and over -- it doesn't matter
> much.
> 
> The same is true for system-calls, once a system call is complete (we've
> returned to userspace) the next system call does not depend on the lock
> history of the previous one.

Yes. I agree. As you said, actually two independent job e.g. syscalls,
works.. should not depend on each other.

Frankly speaking, nevertheless, if they depend on each other, then I
think it would be better to detect the cases, too. But for now, since
it's more important to avoid false positive detections, I will do it as
conservatively as possible, as my current implementation.

And thank you for additional explanation!