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