linux-kernel - Re: task_is_descendant() cleanup

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: <20170130134947.GA24718@redhat.com>
Date:   Mon, 30 Jan 2017 14:49:48 +0100
From:   Oleg Nesterov <oleg@...hat.com>
To:     Kees Cook <keescook@...omium.org>
Cc:     Pavel Tikhomirov <ptikhomirov@...tuozzo.com>,
        Ingo Molnar <mingo@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Cyrill Gorcunov <gorcunov@...nvz.org>,
        John Stultz <john.stultz@...aro.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Nicolas Pitre <nicolas.pitre@...aro.org>,
        Michal Hocko <mhocko@...e.com>,
        Stanislav Kinsburskiy <skinsbursky@...tuozzo.com>,
        Mateusz Guzik <mguzik@...hat.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Pavel Emelyanov <xemul@...tuozzo.com>,
        Konstantin Khorenko <khorenko@...tuozzo.com>
Subject: Re: task_is_descendant() cleanup

On 01/25, Kees Cook wrote:
>
> On Mon, Jan 23, 2017 at 4:52 AM, Oleg Nesterov <oleg@...hat.com> wrote:
> > On 01/23, Oleg Nesterov wrote:
> >>
> >> Btw task_is_descendant() looks wrong at first glance.
> >
> > No, I missed the 2nd ->group_leader dereference. Still this function looks
> > overcomplicated and the usage of thread_group_leader/group_leader just add
> > the unnecessary confusion. It can be simplified a little bit:
> >
> >         static int task_is_descendant(struct task_struct *parent,
> >                                       struct task_struct *child)
> >         {
> >                 int rc = 0;
> >                 struct task_struct *walker;
> >
> >                 if (!parent || !child)
> >                         return 0;
> >
> >                 rcu_read_lock();
> >                 for (walker = child; walker->pid; walker = rcu_dereference(walker->real_parent))
> >                         if (same_thread_group(parent, walker)) {
> >                                 rc = 1;
> >                                 break;
> >                         }
> >                 rcu_read_unlock();
> >
> >                 return rc;
> >         }
> >
> > Kees, I can send a patch if you think this very minor cleanup makes any sense.
>
> Err, isn't checking same_thread_group() at every level more expensive
> than what I currently have?

Well, same_thread_group(p1,p2) is just

	 p1->signal == p2->signal

yes this is a bit more expensive than

	walker == parent

we currently have, yes. But this eliminates

	if (!thread_group_leader(walker))
		walker = rcu_dereference(walker->group_leader);

we currently do at every level. And note that "parent" can exec and change its
->group_leader at any time, we probably do not care but this looks confusing.


But please forget, this is really minor.

Oleg.