linux-kernel - Re: [PATCH v2] sched/fair: Fix fault in reweight

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: <20220125011804.mhlhdenbjluzqkgf@oracle.com>
Date:   Mon, 24 Jan 2022 20:18:04 -0500
From:   Daniel Jordan <daniel.m.jordan@...cle.com>
To:     Tadeusz Struk <tadeusz.struk@...aro.org>
Cc:     peterz@...radead.org, Ingo Molnar <mingo@...hat.com>,
        Juri Lelli <juri.lelli@...hat.com>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Ben Segall <bsegall@...gle.com>, Mel Gorman <mgorman@...e.de>,
        Daniel Bristot de Oliveira <bristot@...hat.com>,
        Zhang Qiao <zhangqiao22@...wei.com>, stable@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        syzbot+af7a719bc92395ee41b3@...kaller.appspotmail.com
Subject: Re: [PATCH v2] sched/fair: Fix fault in reweight_entity

Hi,

On Thu, Jan 20, 2022 at 12:01:39PM -0800, Tadeusz Struk wrote:
> Syzbot found a GPF in reweight_entity(). This has been bisected to commit
> 4ef0c5c6b5ba ("kernel/sched: Fix sched_fork() access an invalid sched_task_group")
> 
> There is a race between sched_post_fork() and setpriority(PRIO_PGRP)
> within a thread group that causes a null-ptr-deref in reweight_entity()
> in CFS. The scenario is that the main process spawns number of new
> threads, which then call setpriority(PRIO_PGRP, 0, prio), wait, and exit.
> For each of the new threads the copy_process() gets invoked, which adds
> the new task_struct to the group, and eventually calls sched_post_fork() for it.
> 
> In the above scenario there is a possibility that setpriority(PRIO_PGRP)
> and set_one_prio() will be called for a thread in the group that is just
> being created by copy_process(), and for which the sched_post_fork() has
> not been executed yet. This will trigger a null pointer dereference in
> reweight_entity(), as it will try to access the run queue pointer, which
> hasn't been set.

It's kinda strange that p->se.cfs_rq is NULLed in __sched_fork().
AFAICT, that lets set_task_rq_fair() distinguish between fork and other
paths per ad936d8658fd, but it's causing this problem now and it's not
the only way that set_task_rq_fair() could tell the difference.

We might be able to get rid of the NULL assignment instead of adding
code to detect it.  Maybe something like this, against today's mainline?
set_task_rq_fair() would rely on TASK_NEW instead of NULL.

Haven't thought it all the way through, so could be missing something.
Will think more

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 848eaa0efe0ea..9a5b264c5dc10 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -4241,10 +4241,6 @@ static void __sched_fork(unsigned long clone_flags, struct task_struct *p)
 	p->se.vruntime			= 0;
 	INIT_LIST_HEAD(&p->se.group_node);
 
-#ifdef CONFIG_FAIR_GROUP_SCHED
-	p->se.cfs_rq			= NULL;
-#endif
-
 #ifdef CONFIG_SCHEDSTATS
 	/* Even if schedstat is disabled, there should not be garbage */
 	memset(&p->stats, 0, sizeof(p->stats));
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 5146163bfabb9..7aff3b603220d 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -3339,15 +3339,19 @@ static inline void update_tg_load_avg(struct cfs_rq *cfs_rq)
  * caller only guarantees p->pi_lock is held; no other assumptions,
  * including the state of rq->lock, should be made.
  */
-void set_task_rq_fair(struct sched_entity *se,
-		      struct cfs_rq *prev, struct cfs_rq *next)
+void set_task_rq_fair(struct task_struct *p, struct cfs_rq *next)
 {
+	struct sched_entity *se = &p->se;
+	struct cfs_rq *prev = se->cfs_rq;
 	u64 p_last_update_time;
 	u64 n_last_update_time;
 
 	if (!sched_feat(ATTACH_AGE_LOAD))
 		return;
 
+	if (p->__state == TASK_NEW)
+		return;
+
 	/*
 	 * We are supposed to update the task to "current" time, then its up to
 	 * date and ready to go to new CPU/cfs_rq. But we have difficulty in
@@ -3355,7 +3359,7 @@ void set_task_rq_fair(struct sched_entity *se,
 	 * time. This will result in the wakee task is less decayed, but giving
 	 * the wakee more load sounds not bad.
 	 */
-	if (!(se->avg.last_update_time && prev))
+	if (!se->avg.last_update_time)
 		return;
 
 #ifndef CONFIG_64BIT
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index de53be9057390..a6f749f136ee1 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -514,11 +514,10 @@ extern int sched_group_set_shares(struct task_group *tg, unsigned long shares);
 extern int sched_group_set_idle(struct task_group *tg, long idle);
 
 #ifdef CONFIG_SMP
-extern void set_task_rq_fair(struct sched_entity *se,
-			     struct cfs_rq *prev, struct cfs_rq *next);
+extern void set_task_rq_fair(struct task_struct *p, struct cfs_rq *next);
 #else /* !CONFIG_SMP */
-static inline void set_task_rq_fair(struct sched_entity *se,
-			     struct cfs_rq *prev, struct cfs_rq *next) { }
+static inline void set_task_rq_fair(struct task_struct *p,
+				    struct cfs_rq *next) {}
 #endif /* CONFIG_SMP */
 #endif /* CONFIG_FAIR_GROUP_SCHED */
 
@@ -1910,7 +1909,7 @@ static inline void set_task_rq(struct task_struct *p, unsigned int cpu)
 #endif
 
 #ifdef CONFIG_FAIR_GROUP_SCHED
-	set_task_rq_fair(&p->se, p->se.cfs_rq, tg->cfs_rq[cpu]);
+	set_task_rq_fair(p, tg->cfs_rq[cpu]);
 	p->se.cfs_rq = tg->cfs_rq[cpu];
 	p->se.parent = tg->se[cpu];
 #endif