Re: task_is_descendant() cleanup

[Oleg Nesterov]

On 01/25, Kees Cook wrote:
>
> On Mon, Jan 23, 2017 at 4:52 AM, Oleg Nesterov <oleg@xxxxxxxxxx> 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.