Skip to content

Commit

Permalink
ANDROID: sched: check on_rq in freezer_should_skip()
Browse files Browse the repository at this point in the history
In aosp/1979327 we attempted to prevent tasks with pending signals and
PF_FREEZER_SKIP from being immediately rescheduled, because such tasks
would crash the kernel if run while no capable CPUs were online. This was
implemented by declining to immediately reschedule them unless various
conditions were met. However, this ended up causing signals to fail to
be delivered if the signal was received while a task is processing a
syscall, such as futex(2), that will block with PF_FREEZER_SKIP set,
as the kernel relies on a check for TIF_SIGPENDING after setting the
task state to TASK_INTERRUPTIBLE in order to deliver such a signal.

This patch is an alternative solution to the original problem that
avoids introducing the signal delivery bug. It works by changing
how freezer_should_skip() is implemented. Instead of just checking
PF_FREEZER_SKIP, we also use the on_rq field to check whether the task
is not on a runqueue. In this way we ensure that a task that will be
immediately rescheduled will not return true from freezer_should_skip(),
and the task will block the freezer unless it is actually taken off
the runqueue.

Signed-off-by: Peter Collingbourne <[email protected]>
Bug: 202918514
Bug: 251700836
Change-Id: I3f9b705ce9ad2ca1d2df959f43cf05bef78560f8
  • Loading branch information
pcc committed Nov 2, 2022
1 parent 90a47b6 commit 56e639d
Show file tree
Hide file tree
Showing 4 changed files with 24 additions and 21 deletions.
22 changes: 21 additions & 1 deletion include/linux/freezer.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@
#include <linux/sched.h>
#include <linux/wait.h>
#include <linux/atomic.h>
#if defined(CONFIG_ARM64) && !defined(__GENKSYMS__)
#include <linux/mmu_context.h>
#endif

#ifdef CONFIG_FREEZER
extern atomic_t system_freezing_cnt; /* nr of freezing conds in effect */
Expand Down Expand Up @@ -108,10 +111,15 @@ static inline bool cgroup_freezing(struct task_struct *task)
* The caller shouldn't do anything which isn't allowed for a frozen task
* until freezer_cont() is called. Usually, freezer[_do_not]_count() pair
* wrap a scheduling operation and nothing much else.
*
* The write to current->flags uses release semantics to prevent a concurrent
* freezer_should_skip() from observing this write before a write to on_rq
* during a prior call to activate_task(), which may cause it to return true
* before deactivate_task() is called.
*/
static inline void freezer_do_not_count(void)
{
current->flags |= PF_FREEZER_SKIP;
smp_store_release(&current->flags, current->flags | PF_FREEZER_SKIP);
}

/**
Expand Down Expand Up @@ -161,7 +169,19 @@ static inline bool freezer_should_skip(struct task_struct *p)
* clearing %PF_FREEZER_SKIP.
*/
smp_mb();
#ifdef CONFIG_ARM64
return (p->flags & PF_FREEZER_SKIP) &&
(!p->on_rq || task_cpu_possible_mask(p) == cpu_possible_mask);
#else
/*
* On non-aarch64, avoid depending on task_cpu_possible_mask(), which is
* defined in <linux/mmu_context.h>, because including that header from
* here exposes a tricky bug in the tracepoint headers on x86, and that
* macro would end up being defined equal to cpu_possible_mask on other
* architectures anyway.
*/
return p->flags & PF_FREEZER_SKIP;
#endif
}

/*
Expand Down
2 changes: 1 addition & 1 deletion init/do_mounts_initrd.c
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ static void __init handle_initrd(void)
* In case that a resume from disk is carried out by linuxrc or one of
* its children, we need to tell the freezer not to wait for us.
*/
current->flags |= PF_FREEZER_SKIP;
freezer_do_not_count();

info = call_usermodehelper_setup("/linuxrc", argv, envp_init,
GFP_KERNEL, init_linuxrc, NULL, NULL);
Expand Down
2 changes: 1 addition & 1 deletion kernel/power/main.c
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@

void lock_system_sleep(void)
{
current->flags |= PF_FREEZER_SKIP;
freezer_do_not_count();
mutex_lock(&system_transition_mutex);
}
EXPORT_SYMBOL_GPL(lock_system_sleep);
Expand Down
19 changes: 1 addition & 18 deletions kernel/sched/core.c
Original file line number Diff line number Diff line change
Expand Up @@ -4617,23 +4617,6 @@ pick_next_task(struct rq *rq, struct task_struct *prev, struct rq_flags *rf)
BUG();
}

static bool __task_can_run(struct task_struct *prev)
{
if (__fatal_signal_pending(prev))
return true;

if (!frozen_or_skipped(prev))
return true;

/*
* We can't safely go back on the runqueue if we're an asymmetric
* task skipping the freezer. Doing so can lead to migration failures
* later on if there aren't any suitable CPUs left around for us to
* move to.
*/
return task_cpu_possible_mask(prev) == cpu_possible_mask;
}

/*
* __schedule() is the main scheduler function.
*
Expand Down Expand Up @@ -4727,7 +4710,7 @@ static void __sched notrace __schedule(bool preempt)
*/
prev_state = prev->state;
if (!preempt && prev_state) {
if (signal_pending_state(prev_state, prev) && __task_can_run(prev)) {
if (signal_pending_state(prev_state, prev)) {
prev->state = TASK_RUNNING;
} else {
prev->sched_contributes_to_load =
Expand Down

0 comments on commit 56e639d

Please sign in to comment.