linux-kernel - Re: [PATCH v2 4/4] task: RCUify the assignment of rq->curr

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:   Sun, 15 Sep 2019 11:48:25 -0700
From:   "Paul E. McKenney" <paulmck@...nel.org>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Oleg Nesterov <oleg@...hat.com>,
        Russell King - ARM Linux admin <linux@...linux.org.uk>,
        Chris Metcalf <cmetcalf@...hip.com>,
        Christoph Lameter <cl@...ux.com>,
        Kirill Tkhai <tkhai@...dex.ru>, Mike Galbraith <efault@....de>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...nel.org>,
        Linux List Kernel Mailing <linux-kernel@...r.kernel.org>,
        Davidlohr Bueso <dave@...olabs.net>,
        Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH v2 4/4] task: RCUify the assignment of rq->curr

On Sun, Sep 15, 2019 at 01:25:02PM -0500, Eric W. Biederman wrote:
> ebiederm@...ssion.com (Eric W. Biederman) writes:
> 
> > "Paul E. McKenney" <paulmck@...nel.org> writes:
> >
> >> So this looks good in and of itself, but I still do not see what prevents
> >> the unfortunate sequence of events called out in my previous email.
> >> On the other hand, if ->rcu and ->rcu_users were not allocated on top
> >> of each other by a union, I would be happy to provide a Reviewed-by.
> >>
> >> And I am fundamentally distrusting of a refcount_dec_and_test() that
> >> is immediately followed by code that clobbers the now-zero value.
> >> Yes, this does have valid use cases, but it has a lot more invalid
> >> use cases.  The valid use cases have excluded all increments somehow
> >> else, so that the refcount_dec_and_test() call's only job is to
> >> synchronize between concurrent calls to put_task_struct_rcu_user().
> >> But I am not seeing the "excluded all increments somehow".
> >>
> >> So, what am I missing here?
> >
> > Probably only that the users of the task_struct in this sense are now
> > quite mature.
> >
> > The two data structures that allow rcu access to the task_struct are
> > the pid hash and the runqueue.    The practical problem is that they
> > have two very different lifetimes.  So we need some kind of logic that
> > let's us know when they are both done.  A recount does that job very
> > well.
> >
> > Placing the recount on the same storage as the unused (at that point)
> > rcu_head removes the need to be clever in other ways to avoid bloating
> > the task_struct.
> >
> > If you really want a reference to the task_struct from rcu context you
> > can just use get_task_struct.  Because until the grace period completes
> > it is guaranteed that the task_struct has a positive count.
> >
> > Right now I can't imagine a use case for wanting to increase rcu_users
> > anywhere or to decrease rcu_users except where we do.  If there is such
> > a case most likely it will increase the reference count at
> > initialization time.
> >
> > If anyone validly wants to increment rcu_users from an rcu critical
> > section we can move it out of the union at that time.
> 
> Paul were you worrying about incrementing rcu_users because Frederic
> Weisbecker brought the concept up earlier in the review?
> 
> It was his confusion that the point of rcu_users was so that it could
> be incremented from an rcu critical section.  That definitely is not
> the point of rcu_users.
> 
> If you were wondering about someone messing with rcu_users from an rcu
> critical region independently that does suggest the code could use
> a "comment saying don't do that!"  Multiple people getting confused
> about the purpose of a reference count independently does suggest there
> is a human factor problem in there somewhere.

I would welcome a "this is never incremented" comment or some such.

							Thanx, Paul