Re: [PATCH 17/19] sched: Inherit task cookie on fork()

[Joel Fernandes]

Hi Peter,

On Thu, Apr 22, 2021 at 8:36 AM Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
>
> Note that sched_core_fork() is called from under tasklist_lock, and
> not from sched_fork() earlier. This avoids a few races later.
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> ---
>  include/linux/sched.h     |    2 ++
>  kernel/fork.c             |    3 +++
>  kernel/sched/core_sched.c |    6 ++++++
>  3 files changed, 11 insertions(+)
>
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -2172,8 +2172,10 @@ const struct cpumask *sched_trace_rd_spa
>
>  #ifdef CONFIG_SCHED_CORE
>  extern void sched_core_free(struct task_struct *tsk);
> +extern void sched_core_fork(struct task_struct *p);
>  #else
>  static inline void sched_core_free(struct task_struct *tsk) { }
> +static inline void sched_core_fork(struct task_struct *p) { }
>  #endif
>
>  #endif
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -2249,6 +2249,8 @@ static __latent_entropy struct task_stru
>
>         klp_copy_process(p);
>
> +       sched_core_fork(p);
> +
>         spin_lock(&current->sighand->siglock);
>
>         /*
> @@ -2336,6 +2338,7 @@ static __latent_entropy struct task_stru
>         return p;
>
>  bad_fork_cancel_cgroup:
> +       sched_core_free(p);
>         spin_unlock(&current->sighand->siglock);
>         write_unlock_irq(&tasklist_lock);
>         cgroup_cancel_fork(p, args);
> --- a/kernel/sched/core_sched.c
> +++ b/kernel/sched/core_sched.c
> @@ -100,6 +100,12 @@ static unsigned long sched_core_clone_co
>         return cookie;
>  }
>
> +void sched_core_fork(struct task_struct *p)
> +{
> +       RB_CLEAR_NODE(&p->core_node);
> +       p->core_cookie = sched_core_clone_cookie(current);

Does this make sense also for !CLONE_THREAD forks?

With earlier versions of core scheduling, we have done the following
on ChromeOS. Basically, if it is a "thread clone", share the cookie
since memory is shared within a process (same address space within a
process). Otherwise, set the cookie to a new unique cookie so that the
new process does not share core with parent initially (since their
address space will be different).

Example Psedu-ocode in sched_fork():

        if (current->core_cookie && (clone_flags & CLONE_THREAD)) {
                    p->core_cookie = clone_cookie(current);
        } else {
                     p->core_cookie = create_new_cookie();
       }

In your version though, I don't see that it always clones the cookie
whether it is a CLONE_THREAD clone or not. Is that correct? I feel
that's a security issue.

-Joel