freezer: exec should clear PF_NOFREEZE along with PF_KTHREAD

PATCH In review Oleg Nesterov 4 participants 6 messages Dec 6, 2012 – Dec 10, 2012 2 messages not in the archive

Subsystems (3)

Stable branch VFS exec and binfmt

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Oleg Nesterov

author

2012-12-06 12:42 UTC #2 lore raw

Hi,

(add lkml)

Quoted 18 lines · Ben Hutchings

Quoted 15 lines · Joseph Salisbury
Hi Oleg,
A bug was opened against the Ubuntu kernel[0]. It was found that reverting commit b40a79591ca918e7b91b0d9b6abd5d00f2e88c19 resolved this bug, and allowed suspend/resume to work properly.
I see that you are the author of this patch, so I wanted to run it by you. I was thinking of requesting a revert for 3.2 stable, but I wanted to get your feedback first.
Thanks,
Joe

Quoted 7 lines

Quoted 1 line
[0] https://bugs.launchpad.net/ubuntu/+source/v86d/+bug/1080530
If I understand this rightly:
1. Suspend is aborted because the v86d usermodehelper cannot be frozen.
2. Before this fix, usermodehelpers could inherit PF_NOFREEZE (what could possibly go wrong with that?!), which is why this didn't previously happen.

Yes, try_to_freeze_tasks() ignores PF_NOFREEZE tasks

Quoted 2 lines

3. However, usermodehelpers are supposed to be disabled while the freezer is running. Why is this one still running?

__usermodehelper_disable() doesn't try to kill/stop/whatever the already spawned tasks...

The question is, why v86d refuses to freeze? It sleeps in TASK_INTERRUPTIBLE.

Oh... I seem to understand. This is 3.2 kernel, it also has PF_FREEZER_NOSIG. which should be cleared along with PF_NOFREEZE.

This flag was removed upstream, but the older kernel need the fix. Could you test the patch below?

Oleg.

fs/exec.c +1 -1

@@ -1084,7 +1084,7 @@ int flush_old_exec(struct linux_binprm *
 
 	set_fs(USER_DS);
 	current->flags &=
-		~(PF_RANDOMIZE | PF_FORKNOEXEC | PF_KTHREAD | PF_NOFREEZE);
+		~(PF_RANDOMIZE | PF_FORKNOEXEC | PF_KTHREAD | PF_NOFREEZE | PF_FREEZER_NOSIG);
 	flush_thread();
 	current->personality &= ~bprm->per_clear;

Joseph Salisbury

2012-12-06 20:44 UTC #3 lore raw

Quoted 50 lines · Oleg Nesterov

Hi,
(add lkml)
Quoted 17 lines · Ben Hutchings
Quoted 15 lines · Joseph Salisbury
Hi Oleg,
A bug was opened against the Ubuntu kernel[0]. It was found that reverting commit b40a79591ca918e7b91b0d9b6abd5d00f2e88c19 resolved this bug, and allowed suspend/resume to work properly.
I see that you are the author of this patch, so I wanted to run it by you. I was thinking of requesting a revert for 3.2 stable, but I wanted to get your feedback first.
Thanks,
Joe
Quoted 6 lines
Quoted 1 line
[0] https://bugs.launchpad.net/ubuntu/+source/v86d/+bug/1080530
If I understand this rightly:
1. Suspend is aborted because the v86d usermodehelper cannot be frozen.
2. Before this fix, usermodehelpers could inherit PF_NOFREEZE (what could possibly go wrong with that?!), which is why this didn't previously happen.
Yes, try_to_freeze_tasks() ignores PF_NOFREEZE tasks
Quoted 2 lines
3. However, usermodehelpers are supposed to be disabled while the freezer is running. Why is this one still running?
__usermodehelper_disable() doesn't try to kill/stop/whatever the already spawned tasks...
The question is, why v86d refuses to freeze? It sleeps in TASK_INTERRUPTIBLE.
Oh... I seem to understand. This is 3.2 kernel, it also has PF_FREEZER_NOSIG. which should be cleared along with PF_NOFREEZE.
This flag was removed upstream, but the older kernel need the fix. Could you test the patch below?
Oleg.

fs/exec.c int flush_old_exec(struct linux_binprm * · line 1090

Quoted 8 lines

fs/exec.c +1 -1

@@ -1084,7 +1084,7 @@ int flush_old_exec(struct linux_binprm *
 
  	set_fs(USER_DS);
  	current->flags &=
-		~(PF_RANDOMIZE | PF_FORKNOEXEC | PF_KTHREAD | PF_NOFREEZE);
+		~(PF_RANDOMIZE | PF_FORKNOEXEC | PF_KTHREAD | PF_NOFREEZE | PF_FREEZER_NOSIG);
  	flush_thread();
  	current->personality &= ~bprm->per_clear;

Hi Oleg,

A couple of people have tested your patch and report if fixes the suspend/resume bug[0]. Thanks for the quick fix! Will you be requesting this patch in the stable kernels?

Thanks again,

Joe

[0] https://bugs.launchpad.net/bugs/1080530

Oleg Nesterov

author

2012-12-07 14:49 UTC #4 lore raw

cover freezer: PF_FREEZER_NOSIG should be cleared along with PF_NOFREEZE

Quoted 3 lines · Joseph Salisbury

A couple of people have tested your patch and report if fixes the suspend/resume bug[0].

Great, thanks a lot.

Quoted 2 lines

Will you be
requesting this patch in the stable kernels?

I have to ;)

I tried to explain this in the changelog, but once again just in case...

This patch is only for -stable. It is only needed if a) b40a7959 was backported and b) the kernel still has PF_FREEZER_NOSIG removed in 3.3.

Alernatively, b40a7959 can be reverted. But please note that this will just hide the problem revealed by "incomplete" b40a7959.

Oleg.

Oleg Nesterov

author

2012-12-07 14:49 UTC #5 lore raw

Re: [PATCH -stable 1/1] freezer: PF_FREEZER_NOSIG should be cleared along with PF_NOFREEZE

This patch is only for pre-v3.3 stable trees which backported b40a7959 "freezer: exec should clear PF_NOFREEZE along with PF_KTHREAD". v3.3+ doesn't need this fix.

b40a7959 is the trivial bugfix, but unfortunately I forgot that until 34b087e4 "freezer: kill unused set_freezable_with_signal()" there were another only-for-kernel-threads flag, PF_FREEZER_NOSIG, which should be cleared as well.

See https://bugs.launchpad.net/ubuntu/+source/v86d/+bug/1080530
The freezer fails because it expects that a PF_FREEZER_NOSIG task doesn't need a signal. Before b40a7959 it wrongly succeeds leaving the PF_NOFREEZE | PF_FREEZER_NOSIG task unfrozen.

Reported-and-tested-by Joseph Salisbury <[email protected]>

Cc [email protected] # pre v3.3 if commit b40a7959 was taken

Signed-off-by Oleg Nesterov <[email protected]>

fs/exec.c +1 -1

@@ -1084,7 +1084,7 @@ int flush_old_exec(struct linux_binprm *
 
 	set_fs(USER_DS);
 	current->flags &=
-		~(PF_RANDOMIZE | PF_FORKNOEXEC | PF_KTHREAD | PF_NOFREEZE);
+		~(PF_RANDOMIZE | PF_FORKNOEXEC | PF_KTHREAD | PF_NOFREEZE | PF_FREEZER_NOSIG);
 	flush_thread();
 	current->personality &= ~bprm->per_clear;

Ben Hutchings

2012-12-09 23:04 UTC #6 lore raw

Re: [PATCH -stable 1/1] freezer: PF_FREEZER_NOSIG should be cleared along with PF_NOFREEZE

Quoted 3 lines · Oleg Nesterov

This patch is only for pre-v3.3 stable trees which backported b40a7959 "freezer: exec should clear PF_NOFREEZE along with PF_KTHREAD". v3.3+ doesn't need this fix.

[...]

Thanks very much, I've added this to the queue for 3.2. However, in 3.2 PF_FORKNOEXEC is cleared by the binfmt code and not in fs/exec.c, and I left that as it is.

Ben.

Signature · 2 lines

Ben Hutchings
It is impossible to make anything foolproof because fools are so ingenious.

Greg Kroah-Hartman

maintainer

2012-12-10 20:34 UTC #7 lore raw

Re: [PATCH -stable 1/1] freezer: PF_FREEZER_NOSIG should be cleared along with PF_NOFREEZE

In reply to Oleg Nesterov

Quoted 14 lines · Oleg Nesterov

This patch is only for pre-v3.3 stable trees which backported b40a7959 "freezer: exec should clear PF_NOFREEZE along with PF_KTHREAD". v3.3+ doesn't need this fix.
b40a7959 is the trivial bugfix, but unfortunately I forgot that until 34b087e4 "freezer: kill unused set_freezable_with_signal()" there were another only-for-kernel-threads flag, PF_FREEZER_NOSIG, which should be cleared as well.
See https://bugs.launchpad.net/ubuntu/+source/v86d/+bug/1080530
The freezer fails because it expects that a PF_FREEZER_NOSIG task doesn't need a signal. Before b40a7959 it wrongly succeeds leaving the PF_NOFREEZE | PF_FREEZER_NOSIG task unfrozen.

Quoted 4 lines

Reported-and-tested-by Joseph Salisbury <[email protected]>
Cc [email protected] # pre v3.3 if commit b40a7959 was taken
From what I can tell, 3.0-stable didn't take this patch, so it should be

fine. Ben, I don't know about 3.2, you should check this.

Oleg, thanks for doing this work, much appreciated.

greg k-h