sched/vtime: Fix guest/system mis-accounting on task switch

PATCH Applied Frederic Weisbecker 3 participants 4 messages Sep 25, 2019 – Oct 9, 2019 Scheduler

Share & tools

Frederic Weisbecker

author

2019-09-25 21:42 UTC #0 lore raw

PATCH 1/1 sched/vtime: Fix guest/system mis-accounting on task switch

vtime_account_system() assumes that the target task to account cputime to is always the current task. This is most often true indeed except on task switch where we call:

vtime_common_task_switch(prev)
vtime_account_system(prev)

Here prev is the scheduling-out task where we account the cputime to. It doesn't match current that is already the scheduling-in task at this stage of the context switch.

So we end up checking the wrong task flags to determine if we are accounting guest or system time to the previous task.

As a result the wrong task is used to check if the target is running in guest mode. We may then spuriously account or leak either system or guest time on task switch.

Fix this assumption and also turn vtime_guest_enter/exit() to use the task passed in parameter as well to avoid future similar issues.

Fixes 2a42eb9594a1 ("sched/cputime: Accumulate vtime on top of nsec clocksource")

Signed-off-by Frederic Weisbecker <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Rik van Riel <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Wanpeng Li <[email protected]>

Cc Ingo Molnar <[email protected]>

---
 kernel/sched/cputime.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

kernel/sched/cputime.c +3 -3

index 2305ce89a26c..46ed4e1383e2 100644
@@ -740,7 +740,7 @@ void vtime_account_system(struct task_struct *tsk)
 
 	write_seqcount_begin(&vtime->seqcount);
 	/* We might have scheduled out from guest path */
-	if (current->flags & PF_VCPU)
+	if (tsk->flags & PF_VCPU)
 		vtime_account_guest(tsk, vtime);
 	else
 		__vtime_account_system(tsk, vtime);

@@ -783,7 +783,7 @@ void vtime_guest_enter(struct task_struct *tsk)
 	 */
 	write_seqcount_begin(&vtime->seqcount);
 	__vtime_account_system(tsk, vtime);
-	current->flags |= PF_VCPU;
+	tsk->flags |= PF_VCPU;
 	write_seqcount_end(&vtime->seqcount);
 }
 EXPORT_SYMBOL_GPL(vtime_guest_enter);

@@ -794,7 +794,7 @@ void vtime_guest_exit(struct task_struct *tsk)
 
 	write_seqcount_begin(&vtime->seqcount);
 	vtime_account_guest(tsk, vtime);
-	current->flags &= ~PF_VCPU;
+	tsk->flags &= ~PF_VCPU;
 	write_seqcount_end(&vtime->seqcount);
 }
 EXPORT_SYMBOL_GPL(vtime_guest_exit);

Signature · 1 line

2.21.0

Peter Zijlstra

maintainer

2019-09-26 08:14 UTC #1 lore raw

Quoted 21 lines · Frederic Weisbecker

vtime_account_system() assumes that the target task to account cputime to is always the current task. This is most often true indeed except on task switch where we call:
vtime_common_task_switch(prev)
vtime_account_system(prev)
Here prev is the scheduling-out task where we account the cputime to. It doesn't match current that is already the scheduling-in task at this stage of the context switch.
So we end up checking the wrong task flags to determine if we are accounting guest or system time to the previous task.
As a result the wrong task is used to check if the target is running in guest mode. We may then spuriously account or leak either system or guest time on task switch.
Fix this assumption and also turn vtime_guest_enter/exit() to use the task passed in parameter as well to avoid future similar issues.

Quoted 7 lines

Fixes 2a42eb9594a1 ("sched/cputime: Accumulate vtime on top of nsec clocksource")
Signed-off-by Frederic Weisbecker <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Rik van Riel <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Wanpeng Li <[email protected]>
Cc Ingo Molnar <[email protected]>

Thanks!

tip-bot2 for Frederic Weisbecker

merge bot Applied

2019-10-09 13:00 UTC #2 lore raw

In reply to Frederic Weisbecker

The following commit has been merged into the sched/core branch of tip:

Commit-ID:     68e7a4d66b0ce04bf18ff2ffded5596ab3618585
Gitweb:        https://git.kernel.org/tip/68e7a4d66b0ce04bf18ff2ffded5596ab3618585
Author:        Frederic Weisbecker <[email protected]>
AuthorDate:    Wed, 25 Sep 2019 23:42:42 +02:00
Committer:     Ingo Molnar <[email protected]>
CommitterDate: Wed, 09 Oct 2019 12:38:03 +02:00

sched/vtime: Fix guest/system mis-accounting on task switch

vtime_account_system() assumes that the target task to account cputime to is always the current task. This is most often true indeed except on task switch where we call:

vtime_common_task_switch(prev)
vtime_account_system(prev)

Here prev is the scheduling-out task where we account the cputime to. It doesn't match current that is already the scheduling-in task at this stage of the context switch.

So we end up checking the wrong task flags to determine if we are accounting guest or system time to the previous task.

As a result the wrong task is used to check if the target is running in guest mode. We may then spuriously account or leak either system or guest time on task switch.

Fix this assumption and also turn vtime_guest_enter/exit() to use the task passed in parameter as well to avoid future similar issues.

Signed-off-by Frederic Weisbecker <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Rik van Riel <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Wanpeng Li <[email protected]>

Fixes 2a42eb9594a1 ("sched/cputime: Accumulate vtime on top of nsec clocksource")

Link https://lkml.kernel.org/r/[email protected] open here

Signed-off-by Ingo Molnar <[email protected]>

---
 kernel/sched/cputime.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

kernel/sched/cputime.c +3 -3

index 2305ce8..46ed4e1 100644
@@ -740,7 +740,7 @@ void vtime_account_system(struct task_struct *tsk)
 
 	write_seqcount_begin(&vtime->seqcount);
 	/* We might have scheduled out from guest path */
-	if (current->flags & PF_VCPU)
+	if (tsk->flags & PF_VCPU)
 		vtime_account_guest(tsk, vtime);
 	else
 		__vtime_account_system(tsk, vtime);

@@ -783,7 +783,7 @@ void vtime_guest_enter(struct task_struct *tsk)
 	 */
 	write_seqcount_begin(&vtime->seqcount);
 	__vtime_account_system(tsk, vtime);
-	current->flags |= PF_VCPU;
+	tsk->flags |= PF_VCPU;
 	write_seqcount_end(&vtime->seqcount);
 }
 EXPORT_SYMBOL_GPL(vtime_guest_enter);

@@ -794,7 +794,7 @@ void vtime_guest_exit(struct task_struct *tsk)
 
 	write_seqcount_begin(&vtime->seqcount);
 	vtime_account_guest(tsk, vtime);
-	current->flags &= ~PF_VCPU;
+	tsk->flags &= ~PF_VCPU;
 	write_seqcount_end(&vtime->seqcount);
 }
 EXPORT_SYMBOL_GPL(vtime_guest_exit);

tip-bot2 for Frederic Weisbecker

merge bot Applied

2019-10-09 13:00 UTC #3 lore raw

In reply to Frederic Weisbecker

The following commit has been merged into the sched/urgent branch of tip:

Commit-ID:     68e7a4d66b0ce04bf18ff2ffded5596ab3618585
Gitweb:        https://git.kernel.org/tip/68e7a4d66b0ce04bf18ff2ffded5596ab3618585
Author:        Frederic Weisbecker <[email protected]>
AuthorDate:    Wed, 25 Sep 2019 23:42:42 +02:00
Committer:     Ingo Molnar <[email protected]>
CommitterDate: Wed, 09 Oct 2019 12:38:03 +02:00

sched/vtime: Fix guest/system mis-accounting on task switch

vtime_account_system() assumes that the target task to account cputime to is always the current task. This is most often true indeed except on task switch where we call:

vtime_common_task_switch(prev)
vtime_account_system(prev)

Here prev is the scheduling-out task where we account the cputime to. It doesn't match current that is already the scheduling-in task at this stage of the context switch.

So we end up checking the wrong task flags to determine if we are accounting guest or system time to the previous task.

As a result the wrong task is used to check if the target is running in guest mode. We may then spuriously account or leak either system or guest time on task switch.

Fix this assumption and also turn vtime_guest_enter/exit() to use the task passed in parameter as well to avoid future similar issues.

Signed-off-by Frederic Weisbecker <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Rik van Riel <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Wanpeng Li <[email protected]>

Fixes 2a42eb9594a1 ("sched/cputime: Accumulate vtime on top of nsec clocksource")

Link https://lkml.kernel.org/r/[email protected] open here

Signed-off-by Ingo Molnar <[email protected]>

---
 kernel/sched/cputime.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

kernel/sched/cputime.c +3 -3

index 2305ce8..46ed4e1 100644
@@ -740,7 +740,7 @@ void vtime_account_system(struct task_struct *tsk)
 
 	write_seqcount_begin(&vtime->seqcount);
 	/* We might have scheduled out from guest path */
-	if (current->flags & PF_VCPU)
+	if (tsk->flags & PF_VCPU)
 		vtime_account_guest(tsk, vtime);
 	else
 		__vtime_account_system(tsk, vtime);

@@ -783,7 +783,7 @@ void vtime_guest_enter(struct task_struct *tsk)
 	 */
 	write_seqcount_begin(&vtime->seqcount);
 	__vtime_account_system(tsk, vtime);
-	current->flags |= PF_VCPU;
+	tsk->flags |= PF_VCPU;
 	write_seqcount_end(&vtime->seqcount);
 }
 EXPORT_SYMBOL_GPL(vtime_guest_enter);

@@ -794,7 +794,7 @@ void vtime_guest_exit(struct task_struct *tsk)
 
 	write_seqcount_begin(&vtime->seqcount);
 	vtime_account_guest(tsk, vtime);
-	current->flags &= ~PF_VCPU;
+	tsk->flags &= ~PF_VCPU;
 	write_seqcount_end(&vtime->seqcount);
 }
 EXPORT_SYMBOL_GPL(vtime_guest_exit);