freezer for cgroup v2

Cgroup v2 freezer tries to put tasks into a state similar to jobctl stop. This means that tasks can be killed, ptraced (using PTRACE_SEIZE*), and interrupted. It is possible to attach to a frozen task, get some information (e.g. read registers) and detach.

Quoted 5 lines · Roman Gushchin

Cgroup v2 freezer tries to put tasks into a state similar to jobctl stop. This means that tasks can be killed, ptraced (using PTRACE_SEIZE*), and interrupted. It is possible to attach to a frozen task, get some information (e.g. read registers) and detach.

I fail to understand how this all supposed to work.

Review static inline int signal_pending_state(long state, struct task_struct *p) · line 373

Quoted 6 lines

(quoted diff) +2 -0

@@ -368,6 +369,8 @@ static inline int signal_pending_state(long state, struct task_struct *p)
 		return 0;
 	if (!signal_pending(p))
 		return 0;
+	if (unlikely(cgroup_task_frozen(p) && p->jobctl == JOBCTL_TRAP_FREEZE))
+		return __fatal_signal_pending(p);

I think I will never agree with this change ;) and I don't think it actually helps.

Review static inline int signal_pending_state(long state, struct task_struct *p)

Quoted 12 lines

(quoted diff) +11 -0

+void cgroup_enter_frozen(void)
+{
+	if (!current->frozen) {
+		spin_lock_irq(&css_set_lock);
+		current->frozen = true;
+		cgroup_inc_frozen_cnt(task_dfl_cgroup(current), false, true);
+		spin_unlock_irq(&css_set_lock);
+	}
+
+	__set_current_state(TASK_INTERRUPTIBLE);
+	schedule();

So once again, suppose it races with PTRACE_INTERRUPT, or SIGSTOP, or something else which should be handled by get_signal() before do_freezer_trap().

If (say) PTRACE_INTERRUPT comes before schedule it will be lost. Otherwise the frozen task will react. This can't be right. Or I am totally confused.

Perhaps you can split this patch? start with cgroup_enter_frozen() using TASK_KILLABLE, then teach it to handle ptrace/stop/etc? I think this way it would be simpler to discuss the necessary changes and document what exactly are you trying to do.

and btw.... what about suspend? try_to_freeze_tasks() will obviously fail if there is a ->frozen thread?

Quoted 9 lines · Oleg Nesterov

Quoted 5 lines · Roman Gushchin

Cgroup v2 freezer tries to put tasks into a state similar to jobctl stop. This means that tasks can be killed, ptraced (using PTRACE_SEIZE*), and interrupted. It is possible to attach to a frozen task, get some information (e.g. read registers) and detach.

I fail to understand how this all supposed to work.

Quoted 8 lines

Review static inline int signal_pending_state(long state, struct task_struct *p) · line 373

Quoted 6 lines

(quoted diff) +2 -0

@@ -368,6 +369,8 @@ static inline int signal_pending_state(long state, struct task_struct *p)
 		return 0;
 	if (!signal_pending(p))
 		return 0;
+	if (unlikely(cgroup_task_frozen(p) && p->jobctl == JOBCTL_TRAP_FREEZE))
+		return __fatal_signal_pending(p);

I think I will never agree with this change ;) and I don't think it actually helps.

See below.

Quoted 16 lines

Review static inline int signal_pending_state(long state, struct task_struct *p)

Quoted 12 lines

(quoted diff) +11 -0

+void cgroup_enter_frozen(void)
+{
+	if (!current->frozen) {
+		spin_lock_irq(&css_set_lock);
+		current->frozen = true;
+		cgroup_inc_frozen_cnt(task_dfl_cgroup(current), false, true);
+		spin_unlock_irq(&css_set_lock);
+	}
+
+	__set_current_state(TASK_INTERRUPTIBLE);
+	schedule();

So once again, suppose it races with PTRACE_INTERRUPT, or SIGSTOP, or something else which should be handled by get_signal() before do_freezer_trap().

Quoted 2 lines

If (say) PTRACE_INTERRUPT comes before schedule it will be lost. Otherwise the frozen task will react. This can't be right. Or I am totally confused.

Why?
PTRACE_INTERRUPT will set JOBCTL_TRAP_STOP, so signal_pending_state() will return true, schedule() will return immediately, and we'll handle the trap.

Quoted 2 lines

and btw.... what about suspend? try_to_freeze_tasks() will obviously fail if there is a ->frozen thread?

I have to think a bit more here, but something like this will probably work:

kernel/freezer.c +1 -2

index b162b74611e4..590ac4d10b02 100644
@@ -134,7 +134,7 @@ bool freeze_task(struct task_struct *p)
                return false;
 
        spin_lock_irqsave(&freezer_lock, flags);
-       if (!freezing(p) || frozen(p)) {
+       if (!freezing(p) || frozen(p) || cgroup_task_frozen()) {
                spin_unlock_irqrestore(&freezer_lock, flags);
                return false;
        }
 
--

If the task is already frozen by the cgroup freezer, we don't have to do anything additionally.

Quoted 38 lines · Roman Gushchin

Quoted 9 lines · Oleg Nesterov

Quoted 5 lines · Roman Gushchin

Cgroup v2 freezer tries to put tasks into a state similar to jobctl stop. This means that tasks can be killed, ptraced (using PTRACE_SEIZE*), and interrupted. It is possible to attach to a frozen task, get some information (e.g. read registers) and detach.

I fail to understand how this all supposed to work.

Quoted 8 lines

Review static inline int signal_pending_state(long state, struct task_struct *p) · line 373

Quoted 6 lines

(quoted diff) +2 -0

@@ -368,6 +369,8 @@ static inline int signal_pending_state(long state, struct task_struct *p)
 		return 0;
 	if (!signal_pending(p))
 		return 0;
+	if (unlikely(cgroup_task_frozen(p) && p->jobctl == JOBCTL_TRAP_FREEZE))
+		return __fatal_signal_pending(p);

I think I will never agree with this change ;) and I don't think it actually helps.

See below.

Quoted 16 lines

Review static inline int signal_pending_state(long state, struct task_struct *p)

Quoted 12 lines

(quoted diff) +11 -0

+void cgroup_enter_frozen(void)
+{
+	if (!current->frozen) {
+		spin_lock_irq(&css_set_lock);
+		current->frozen = true;
+		cgroup_inc_frozen_cnt(task_dfl_cgroup(current), false, true);
+		spin_unlock_irq(&css_set_lock);
+	}
+
+	__set_current_state(TASK_INTERRUPTIBLE);
+	schedule();

So once again, suppose it races with PTRACE_INTERRUPT, or SIGSTOP, or something else which should be handled by get_signal() before do_freezer_trap().

Quoted 6 lines

Quoted 2 lines

If (say) PTRACE_INTERRUPT comes before schedule it will be lost. Otherwise the frozen task will react. This can't be right. Or I am totally confused.

Why?
PTRACE_INTERRUPT will set JOBCTL_TRAP_STOP, so signal_pending_state() will return true, schedule() will return immediately, and we'll handle the trap.

OK, I misread the JOBCTL_TRAP_FREEZE check as "jobctl & JOBCTL_TRAP_FREEZE".

But p->jobctl == JOBCTL_TRAP_FREEZE doesn't look right too. For example, JOBCTL_STOP_DEQUEUED can be set. You probably need something like

	jobctl & (JOBCTL_PENDING_MASK | JOBCTL_TRAP_FREEZE) == JOBCTL_TRAP_FREEZE

And you need a barrier in between, iow you need set_current_state(TASK_INTERRUPTIBLE).

But this doesn't really matter. I don't think you need to modify signal_pending_state() and penalize schedule(). You can do something like

	spin_lock_irq(sigllock);
	if (jobctl & (JOBCTL_PENDING_MASK | JOBCTL_TRAP_FREEZE) == JOBCTL_TRAP_FREEZE &&
	    !__fatal_signal_pending())
	{
		__set_current_state(TASK_INTERRUPTIBLE);
		clear_thread_flag(TIF_SIGPENDING);
	}
	spin_unlock_irq(siglock);

	schedule();
	// recalc_sigpending() is not needed

in cgroup_enter_frozen() with the same effect. Which looks equally ugly and suboptimal, but at least this doesn't touch the sched code.

Quoted 22 lines

Quoted 3 lines

and btw.... what about suspend? try_to_freeze_tasks() will obviously fail if there is a ->frozen thread?

I have to think a bit more here, but something like this will probably work:

kernel/freezer.c +1 -2

index b162b74611e4..590ac4d10b02 100644
@@ -134,7 +134,7 @@ bool freeze_task(struct task_struct *p)
                return false;
 
        spin_lock_irqsave(&freezer_lock, flags);
-       if (!freezing(p) || frozen(p)) {
+       if (!freezing(p) || frozen(p) || cgroup_task_frozen()) {
                spin_unlock_irqrestore(&freezer_lock, flags);
                return false;
        }
 
--

Quoted 2 lines

If the task is already frozen by the cgroup freezer, we don't have to do anything additionally.

I don't think so. A cgroup_task_frozen() task can be killed after try_to_freeze_tasks() succeeds, and the exiting task can close files, do IO, etc. Or it can be thawed by cgroup_freeze_task(false).

In short, if try_to_freeze_tasks() succeeds, the caller has all rights to assume that nobody can escape from __refrigerator().

And what about TASK_STOPPED/TASK_TRACED tasks? They can not be frozen or thawed, right? This doesn't look good, and this differs from the current freezer controller...

Quoted 28 lines · Oleg Nesterov

Quoted 21 lines

Quoted 2 lines

and btw.... what about suspend? try_to_freeze_tasks() will obviously fail if there is a ->frozen thread?

I have to think a bit more here, but something like this will probably work:

kernel/freezer.c +1 -2

index b162b74611e4..590ac4d10b02 100644
@@ -134,7 +134,7 @@ bool freeze_task(struct task_struct *p)
                return false;
 
        spin_lock_irqsave(&freezer_lock, flags);
-       if (!freezing(p) || frozen(p)) {
+       if (!freezing(p) || frozen(p) || cgroup_task_frozen()) {
                spin_unlock_irqrestore(&freezer_lock, flags);
                return false;
        }
 
--

Quoted 2 lines

If the task is already frozen by the cgroup freezer, we don't have to do anything additionally.

I don't think so. A cgroup_task_frozen() task can be killed after try_to_freeze_tasks() succeeds, and the exiting task can close files, do IO, etc. Or it can be thawed by cgroup_freeze_task(false).

Quoted 2 lines

In short, if try_to_freeze_tasks() succeeds, the caller has all rights to assume that nobody can escape from __refrigerator().

But this is what we do with stopped and ptraced tasks, isn't it?

We do use freezable_schedule() and the system freezer just ignores such tasks.

Quoted 3 lines

And what about TASK_STOPPED/TASK_TRACED tasks? They can not be frozen or thawed, right? This doesn't look good, and this differs from the current freezer controller...

Good question!

It looks like cgroup v1 freezer just ignores them treating as already frozen, which doesn't look nice.

Quoted 30 lines · Roman Gushchin

Quoted 28 lines · Oleg Nesterov

Quoted 21 lines

Quoted 2 lines

and btw.... what about suspend? try_to_freeze_tasks() will obviously fail if there is a ->frozen thread?

I have to think a bit more here, but something like this will probably work:

kernel/freezer.c +1 -2

index b162b74611e4..590ac4d10b02 100644
@@ -134,7 +134,7 @@ bool freeze_task(struct task_struct *p)
                return false;
 
        spin_lock_irqsave(&freezer_lock, flags);
-       if (!freezing(p) || frozen(p)) {
+       if (!freezing(p) || frozen(p) || cgroup_task_frozen()) {
                spin_unlock_irqrestore(&freezer_lock, flags);
                return false;
        }
 
--

Quoted 2 lines

If the task is already frozen by the cgroup freezer, we don't have to do anything additionally.

I don't think so. A cgroup_task_frozen() task can be killed after try_to_freeze_tasks() succeeds, and the exiting task can close files, do IO, etc. Or it can be thawed by cgroup_freeze_task(false).

Quoted 4 lines

Quoted 2 lines

In short, if try_to_freeze_tasks() succeeds, the caller has all rights to assume that nobody can escape from __refrigerator().

But this is what we do with stopped and ptraced tasks, isn't it?

No,

Quoted 1 line

We do use freezable_schedule() and the system freezer just ignores such tasks.

	static inline void freezable_schedule(void)
	{
		freezer_do_not_count();
		schedule();
		freezer_count();
	}

and note that freezer_count() calls try_to_freeze().

IOW, the task sleeping in freezable_schedule() doesn't really differ from the task sleeping in __refrigerator(). It is not that "the system freezer just ignores such tasks", it ignores them because it can safely count them as frozen.

Quoted 5 lines

Quoted 4 lines

And what about TASK_STOPPED/TASK_TRACED tasks? They can not be frozen or thawed, right? This doesn't look good, and this differs from the current freezer controller...

Quoted 4 lines

Good question!

It looks like cgroup v1 freezer just ignores them treating as already frozen, which doesn't look nice.

Not sure I understand you, but see above... cgroup v1 freezer looks fine wrt stopped/traced tasks.

Quoted 38 lines

Quoted 22 lines

Quoted 22 lines

Quoted 15 lines

kernel/freezer.c +1 -2

@@ -134,7 +134,7 @@ bool freeze_task(struct task_struct *p)
                return false;
 
        spin_lock_irqsave(&freezer_lock, flags);
-       if (!freezing(p) || frozen(p)) {
+       if (!freezing(p) || frozen(p) || cgroup_task_frozen()) {
                spin_unlock_irqrestore(&freezer_lock, flags);
                return false;
        }
 
--

Quoted 2 lines

If the task is already frozen by the cgroup freezer, we don't have to do anything additionally.

I don't think so. A cgroup_task_frozen() task can be killed after try_to_freeze_tasks() succeeds, and the exiting task can close files, do IO, etc. Or it can be thawed by cgroup_freeze_task(false).

Quoted 4 lines

Quoted 2 lines

In short, if try_to_freeze_tasks() succeeds, the caller has all rights to assume that nobody can escape from __refrigerator().

But this is what we do with stopped and ptraced tasks, isn't it?

No,

Quoted 1 line

We do use freezable_schedule() and the system freezer just ignores such tasks.

	static inline void freezable_schedule(void)
	{
		freezer_do_not_count();
		schedule();
		freezer_count();
	}

Quoted 5 lines

and note that freezer_count() calls try_to_freeze().

IOW, the task sleeping in freezable_schedule() doesn't really differ from the task sleeping in __refrigerator(). It is not that "the system freezer just ignores such tasks", it ignores them because it can safely count them as frozen.

Right, so the task is sleeping peacefully, and we know, that it won't get anywhere, because we'll catch it in freezer_count(). We allow it to sleep there, we don't force it to __refrigerator(), and we treat it as frozen.

How's that different to cgroup v2 freezer? If the task is frozen by cgroup v2 freezer, let it sleep there, and catch if it tries to escape. Exactly as it works for SIGSTOP.

Am I missing something?

So, you think that v2 freezer should follow the same approach, and allow tasks sleeping on SIGSTOP, for instance, to be treated as frozen?
Hm, maybe. I have to think more here.

Quoted 48 lines · Roman Gushchin

Quoted 38 lines

Quoted 22 lines

Quoted 22 lines

Quoted 15 lines

kernel/freezer.c +1 -2

@@ -134,7 +134,7 @@ bool freeze_task(struct task_struct *p)
                return false;
 
        spin_lock_irqsave(&freezer_lock, flags);
-       if (!freezing(p) || frozen(p)) {
+       if (!freezing(p) || frozen(p) || cgroup_task_frozen()) {
                spin_unlock_irqrestore(&freezer_lock, flags);
                return false;
        }
 
--

Quoted 2 lines

If the task is already frozen by the cgroup freezer, we don't have to do anything additionally.

I don't think so. A cgroup_task_frozen() task can be killed after try_to_freeze_tasks() succeeds, and the exiting task can close files, do IO, etc. Or it can be thawed by cgroup_freeze_task(false).

Quoted 4 lines

Quoted 2 lines

In short, if try_to_freeze_tasks() succeeds, the caller has all rights to assume that nobody can escape from __refrigerator().

But this is what we do with stopped and ptraced tasks, isn't it?

No,

Quoted 1 line

We do use freezable_schedule() and the system freezer just ignores such tasks.

	static inline void freezable_schedule(void)
	{
		freezer_do_not_count();
		schedule();
		freezer_count();
	}

Quoted 5 lines

and note that freezer_count() calls try_to_freeze().

IOW, the task sleeping in freezable_schedule() doesn't really differ from the task sleeping in __refrigerator(). It is not that "the system freezer just ignores such tasks", it ignores them because it can safely count them as frozen.

Right, so the task is sleeping peacefully, and we know, that it won't get anywhere, because we'll catch it in freezer_count(). We allow it to sleep there, we don't force it to __refrigerator(), and we treat it as frozen.

Quoted 5 lines

How's that different to cgroup v2 freezer? If the task is frozen by cgroup v2 freezer, let it sleep there, and catch if it tries to escape. Exactly as it works for SIGSTOP.

Am I missing something?

Roman, perhaps we misunderstood each other...

I still think that the cgroup_task_frozen() check in freeze_task() you proposed
a) is not right, and b) it is not what we do with the STOPPED/TRACED tasks which call freezable_schedule(). This is what I tried to say.

If you meant that freezer v2 can too use freezable_schedule() - I agree.

Quoted 4 lines

So, you think that v2 freezer should follow the same approach, and allow tasks sleeping on SIGSTOP, for instance, to be treated as frozen?
Hm, maybe. I have to think more here.

I think this would be nice. Otherwise, say, CGRP_FREEZE can be never reported if I read this code correctly. And this looks "symmetrical" with the fact that a ->frozen task reacts to SIGSTOP and it is still treated as frozen after that.