Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210422120459.447350175@infradead.org> <20210422123308.980003687@infradead.org>
In-Reply-To: <20210422123308.980003687@infradead.org>
From:   Joel Fernandes <joel@joelfernandes.org>
Date:   Mon, 10 May 2021 12:06:38 -0400
Message-ID: <CAEXW_YTHZF69YHD-r=ST97sagjnxEDy6492nDKaaJtkKMoQN9Q@mail.gmail.com>
Subject: Re: [PATCH 17/19] sched: Inherit task cookie on fork()
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     "Hyser,Chris" <chris.hyser@oracle.com>,
        Josh Don <joshdon@google.com>, Ingo Molnar <mingo@kernel.org>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Valentin Schneider <valentin.schneider@arm.com>,
        Mel Gorman <mgorman@suse.de>,
        LKML <linux-kernel@vger.kernel.org>,
        Thomas Glexiner <tglx@linutronix.de>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Peter,

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