linux-kernel - Re: [C/R v20][PATCH 15/96] cgroup freezer: Fix buggy resume test for tasks frozen with cgroup freezer

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: <201003230028.40915.rjw@sisk.pl>
Date:	Tue, 23 Mar 2010 00:28:40 +0100
From:	"Rafael J. Wysocki" <rjw@...k.pl>
To:	Oren Laadan <orenl@...columbia.edu>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org, linux-mm@...ck.org,
	linux-api@...r.kernel.org, Serge Hallyn <serue@...ibm.com>,
	Ingo Molnar <mingo@...e.hu>,
	containers@...ts.linux-foundation.org,
	Matt Helsley <matthltc@...ibm.com>,
	Cedric Le Goater <legoater@...e.fr>,
	Paul Menage <menage@...gle.com>,
	Li Zefan <lizf@...fujitsu.com>, Pavel Machek <pavel@....cz>,
	linux-pm@...ts.linux-foundation.org
Subject: Re: [C/R v20][PATCH 15/96] cgroup freezer: Fix buggy resume test for tasks frozen with cgroup freezer

On Wednesday 17 March 2010, Oren Laadan wrote:
> From: Matt Helsley <matthltc@...ibm.com>
> 
> When the cgroup freezer is used to freeze tasks we do not want to thaw
> those tasks during resume. Currently we test the cgroup freezer
> state of the resuming tasks to see if the cgroup is FROZEN.  If so
> then we don't thaw the task. However, the FREEZING state also indicates
> that the task should remain frozen.
> 
> This also avoids a problem pointed out by Oren Ladaan: the freezer state
> transition from FREEZING to FROZEN is updated lazily when userspace reads
> or writes the freezer.state file in the cgroup filesystem. This means that
> resume will thaw tasks in cgroups which should be in the FROZEN state if
> there is no read/write of the freezer.state file to trigger this
> transition before suspend.
> 
> NOTE: Another "simple" solution would be to always update the cgroup
> freezer state during resume. However it's a bad choice for several reasons:
> Updating the cgroup freezer state is somewhat expensive because it requires
> walking all the tasks in the cgroup and checking if they are each frozen.
> Worse, this could easily make resume run in N^2 time where N is the number
> of tasks in the cgroup. Finally, updating the freezer state from this code
> path requires trickier locking because of the way locks must be ordered.
> 
> Instead of updating the freezer state we rely on the fact that lazy
> updates only manage the transition from FREEZING to FROZEN. We know that
> a cgroup with the FREEZING state may actually be FROZEN so test for that
> state too. This makes sense in the resume path even for partially-frozen
> cgroups -- those that really are FREEZING but not FROZEN.
> 
> Reported-by: Oren Ladaan <orenl@...columbia.edu>
> Signed-off-by: Matt Helsley <matthltc@...ibm.com>
> Cc: Cedric Le Goater <legoater@...e.fr>
> Cc: Paul Menage <menage@...gle.com>
> Cc: Li Zefan <lizf@...fujitsu.com>
> Cc: Rafael J. Wysocki <rjw@...k.pl>
> Cc: Pavel Machek <pavel@....cz>
> Cc: linux-pm@...ts.linux-foundation.org

Looks reasonable.

Is anyone handling that already or do you want me to take it to my tree?

Rafael


> Seems like a candidate for -stable.
> ---
>  include/linux/freezer.h |    7 +++++--
>  kernel/cgroup_freezer.c |    9 ++++++---
>  kernel/power/process.c  |    2 +-
>  3 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/freezer.h b/include/linux/freezer.h
> index 5a361f8..da7e52b 100644
> --- a/include/linux/freezer.h
> +++ b/include/linux/freezer.h
> @@ -64,9 +64,12 @@ extern bool freeze_task(struct task_struct *p, bool sig_only);
>  extern void cancel_freezing(struct task_struct *p);
>  
>  #ifdef CONFIG_CGROUP_FREEZER
> -extern int cgroup_frozen(struct task_struct *task);
> +extern int cgroup_freezing_or_frozen(struct task_struct *task);
>  #else /* !CONFIG_CGROUP_FREEZER */
> -static inline int cgroup_frozen(struct task_struct *task) { return 0; }
> +static inline int cgroup_freezing_or_frozen(struct task_struct *task)
> +{
> +	return 0;
> +}
>  #endif /* !CONFIG_CGROUP_FREEZER */
>  
>  /*
> diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
> index 59e9ef6..eb3f34d 100644
> --- a/kernel/cgroup_freezer.c
> +++ b/kernel/cgroup_freezer.c
> @@ -47,17 +47,20 @@ static inline struct freezer *task_freezer(struct task_struct *task)
>  			    struct freezer, css);
>  }
>  
> -int cgroup_frozen(struct task_struct *task)
> +int cgroup_freezing_or_frozen(struct task_struct *task)
>  {
>  	struct freezer *freezer;
>  	enum freezer_state state;
>  
>  	task_lock(task);
>  	freezer = task_freezer(task);
> -	state = freezer->state;
> +	if (!freezer->css.cgroup->parent)
> +		state = CGROUP_THAWED; /* root cgroup can't be frozen */
> +	else
> +		state = freezer->state;
>  	task_unlock(task);
>  
> -	return state == CGROUP_FROZEN;
> +	return (state == CGROUP_FREEZING) || (state == CGROUP_FROZEN);
>  }
>  
>  /*
> diff --git a/kernel/power/process.c b/kernel/power/process.c
> index 5ade1bd..de53015 100644
> --- a/kernel/power/process.c
> +++ b/kernel/power/process.c
> @@ -145,7 +145,7 @@ static void thaw_tasks(bool nosig_only)
>  		if (nosig_only && should_send_signal(p))
>  			continue;
>  
> -		if (cgroup_frozen(p))
> +		if (cgroup_freezing_or_frozen(p))
>  			continue;
>  
>  		thaw_process(p);
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/