Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1569749

Re: task_is_descendant() cleanup

Show all headers | View raw

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

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH] prctl: propagate has_child_subreaper flag to every descendant Pavel Tikhomirov <ptikhomirov@virtuozzo.com> - 2017-01-19 17:50 +0100
  Re: [PATCH] prctl: propagate has_child_subreaper flag to every         descendant Oleg Nesterov <oleg@redhat.com> - 2017-01-20 19:20 +0100
    Re: [PATCH] prctl: propagate has_child_subreaper flag to every  descendant Oleg Nesterov <oleg@redhat.com> - 2017-01-23 13:00 +0100
      task_is_descendant() cleanup Oleg Nesterov <oleg@redhat.com> - 2017-01-23 14:00 +0100
        Re: task_is_descendant() cleanup Kees Cook <keescook@chromium.org> - 2017-01-25 23:10 +0100
          Re: task_is_descendant() cleanup Oleg Nesterov <oleg@redhat.com> - 2017-01-30 15:00 +0100
      Re: [PATCH] prctl: propagate has_child_subreaper flag to every  descendant Oleg Nesterov <oleg@redhat.com> - 2017-01-23 17:10 +0100
  [PATCH] introduce the walk_process_tree() helper Oleg Nesterov <oleg@redhat.com> - 2017-01-23 13:00 +0100
    Re: [PATCH] introduce the walk_process_tree() helper Oleg Nesterov <oleg@redhat.com> - 2017-01-23 13:10 +0100
  setns() && PR_SET_CHILD_SUBREAPER Oleg Nesterov <oleg@redhat.com> - 2017-01-23 18:00 +0100
    Re: setns() && PR_SET_CHILD_SUBREAPER ebiederm@xmission.com (Eric W. Biederman) - 2017-01-23 19:30 +0100
      Re: setns() && PR_SET_CHILD_SUBREAPER Oleg Nesterov <oleg@redhat.com> - 2017-01-24 15:10 +0100
        Re: setns() && PR_SET_CHILD_SUBREAPER ebiederm@xmission.com (Eric W. Biederman) - 2017-01-24 16:30 +0100
          Re: setns() && PR_SET_CHILD_SUBREAPER Oleg Nesterov <oleg@redhat.com> - 2017-01-30 19:20 +0100
          [PATCH] exit: fix the setns() && PR_SET_CHILD_SUBREAPER interaction Oleg Nesterov <oleg@redhat.com> - 2017-01-30 19:20 +0100

csiph-web