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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAEXW_YTHZF69YHD-r=ST97sagjnxEDy6492nDKaaJtkKMoQN9Q@mail.gmail.com>
Date:   Mon, 10 May 2021 12:06:38 -0400
From:   Joel Fernandes <joel@...lfernandes.org>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     "Hyser,Chris" <chris.hyser@...cle.com>,
        Josh Don <joshdon@...gle.com>, Ingo Molnar <mingo@...nel.org>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Valentin Schneider <valentin.schneider@....com>,
        Mel Gorman <mgorman@...e.de>,
        LKML <linux-kernel@...r.kernel.org>,
        Thomas Glexiner <tglx@...utronix.de>
Subject: Re: [PATCH 17/19] sched: Inherit task cookie on fork()

Hi Peter,

On Thu, Apr 22, 2021 at 8:36 AM Peter Zijlstra <peterz@...radead.org> 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@...radead.org>
> ---
>  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