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

From: Joel Fernandes
Date: Mon May 10 2021 - 12:07:02 EST


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