* [PATCH] sched: Do not re-read h_load_next during hierarchical load calculation
@ 2019-03-19 9:35 Mel Gorman
2019-03-19 11:38 ` Valentin Schneider
2019-03-19 12:06 ` Peter Zijlstra
0 siblings, 2 replies; 6+ messages in thread
From: Mel Gorman @ 2019-03-19 9:35 UTC (permalink / raw)
To: Ingo Molnar, Peter Zijlstra; +Cc: linux-kernel
A NULL pointer dereference bug was reported on a distribution kernel but
the same issue should be present on mainline kernel. It occured on s390
but should not be arch-specific. A partial oops looks like
[775277.408564] Unable to handle kernel pointer dereference in virtual kernel address space
...
[775277.408759] Call Trace:
[775277.408763] ([<0002c11c56899c61>] 0x2c11c56899c61)
[775277.408766] [<0000000000177bb4>] try_to_wake_up+0xfc/0x450
[775277.408773] [<000003ff81ede872>] vhost_poll_wakeup+0x3a/0x50 [vhost]
[775277.408777] [<0000000000194ae4>] __wake_up_common+0xbc/0x178
[775277.408779] [<0000000000194f86>] __wake_up_common_lock+0x9e/0x160
[775277.408780] [<00000000001950de>] __wake_up_sync_key+0x4e/0x60
[775277.408785] [<00000000005d911e>] sock_def_readable+0x5e/0x98
The bug hits any time between 1 hour to 3 days. The dereference occurs
in update_cfs_rq_h_load when accumulating h_load. The problem is that
cfq_rq->h_load_next is not protected by any locking and can be updated
by parallel calls to task_h_load. Depending on the compiler, code may be
generated that re-reads cfq_rq->h_load_next after the check for NULL and
then oops when reading se->avg.load_avg. The dissassembly showed that it
was possible to reread h_load_next after the check for NULL.
While this does not appear to be an issue for later compilers, it's still
an accident if the correct code is generated. Full locking in this path
would have high overhead so this patch uses READ_ONCE to read h_load_next
only once and check for NULL before dereferencing. It was confirmed that
there were no further oops after 10 days of testing.
Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
---
kernel/sched/fair.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 310d0637fe4b..34aeb40e69d2 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -7726,7 +7726,7 @@ static void update_cfs_rq_h_load(struct cfs_rq *cfs_rq)
cfs_rq->last_h_load_update = now;
}
- while ((se = cfs_rq->h_load_next) != NULL) {
+ while ((se = READ_ONCE(cfs_rq->h_load_next)) != NULL) {
load = cfs_rq->h_load;
load = div64_ul(load * se->avg.load_avg,
cfs_rq_load_avg(cfs_rq) + 1);
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] sched: Do not re-read h_load_next during hierarchical load calculation
2019-03-19 9:35 [PATCH] sched: Do not re-read h_load_next during hierarchical load calculation Mel Gorman
@ 2019-03-19 11:38 ` Valentin Schneider
2019-03-19 12:30 ` Mel Gorman
2019-03-19 12:06 ` Peter Zijlstra
1 sibling, 1 reply; 6+ messages in thread
From: Valentin Schneider @ 2019-03-19 11:38 UTC (permalink / raw)
To: Mel Gorman, Ingo Molnar, Peter Zijlstra; +Cc: linux-kernel
Hi,
On 19/03/2019 09:35, Mel Gorman wrote:
> A NULL pointer dereference bug was reported on a distribution kernel but
> the same issue should be present on mainline kernel. It occured on s390
> but should not be arch-specific. A partial oops looks like
>
> [775277.408564] Unable to handle kernel pointer dereference in virtual kernel address space
> ...
> [775277.408759] Call Trace:
> [775277.408763] ([<0002c11c56899c61>] 0x2c11c56899c61)
> [775277.408766] [<0000000000177bb4>] try_to_wake_up+0xfc/0x450
> [775277.408773] [<000003ff81ede872>] vhost_poll_wakeup+0x3a/0x50 [vhost]
> [775277.408777] [<0000000000194ae4>] __wake_up_common+0xbc/0x178
> [775277.408779] [<0000000000194f86>] __wake_up_common_lock+0x9e/0x160
> [775277.408780] [<00000000001950de>] __wake_up_sync_key+0x4e/0x60
> [775277.408785] [<00000000005d911e>] sock_def_readable+0x5e/0x98
>
> The bug hits any time between 1 hour to 3 days. The dereference occurs
> in update_cfs_rq_h_load when accumulating h_load. The problem is that
> cfq_rq->h_load_next is not protected by any locking and can be updated
> by parallel calls to task_h_load. Depending on the compiler, code may be
> generated that re-reads cfq_rq->h_load_next after the check for NULL and
> then oops when reading se->avg.load_avg. The dissassembly showed that it
> was possible to reread h_load_next after the check for NULL.
>
> While this does not appear to be an issue for later compilers, it's still
> an accident if the correct code is generated. Full locking in this path
> would have high overhead so this patch uses READ_ONCE to read h_load_next
> only once and check for NULL before dereferencing. It was confirmed that
> there were no further oops after 10 days of testing.
>
Does that also want a
Fixes: 685207963be9 ("sched: Move h_load calculation to task_h_load()")
Cc: stable@vger.kernel.org
?
Other than that, the change looks sane to me.
Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
> Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
> ---
> kernel/sched/fair.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 310d0637fe4b..34aeb40e69d2 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -7726,7 +7726,7 @@ static void update_cfs_rq_h_load(struct cfs_rq *cfs_rq)
> cfs_rq->last_h_load_update = now;
> }
>
> - while ((se = cfs_rq->h_load_next) != NULL) {
> + while ((se = READ_ONCE(cfs_rq->h_load_next)) != NULL) {
> load = cfs_rq->h_load;
> load = div64_ul(load * se->avg.load_avg,
> cfs_rq_load_avg(cfs_rq) + 1);
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] sched: Do not re-read h_load_next during hierarchical load calculation
2019-03-19 11:38 ` Valentin Schneider
@ 2019-03-19 12:30 ` Mel Gorman
2019-03-19 12:36 ` Valentin Schneider
0 siblings, 1 reply; 6+ messages in thread
From: Mel Gorman @ 2019-03-19 12:30 UTC (permalink / raw)
To: Valentin Schneider; +Cc: Ingo Molnar, Peter Zijlstra, linux-kernel
On Tue, Mar 19, 2019 at 11:38:25AM +0000, Valentin Schneider wrote:
> Hi,
>
> On 19/03/2019 09:35, Mel Gorman wrote:
> > A NULL pointer dereference bug was reported on a distribution kernel but
> > the same issue should be present on mainline kernel. It occured on s390
> > but should not be arch-specific. A partial oops looks like
> >
> > [775277.408564] Unable to handle kernel pointer dereference in virtual kernel address space
> > ...
> > [775277.408759] Call Trace:
> > [775277.408763] ([<0002c11c56899c61>] 0x2c11c56899c61)
> > [775277.408766] [<0000000000177bb4>] try_to_wake_up+0xfc/0x450
> > [775277.408773] [<000003ff81ede872>] vhost_poll_wakeup+0x3a/0x50 [vhost]
> > [775277.408777] [<0000000000194ae4>] __wake_up_common+0xbc/0x178
> > [775277.408779] [<0000000000194f86>] __wake_up_common_lock+0x9e/0x160
> > [775277.408780] [<00000000001950de>] __wake_up_sync_key+0x4e/0x60
> > [775277.408785] [<00000000005d911e>] sock_def_readable+0x5e/0x98
> >
> > The bug hits any time between 1 hour to 3 days. The dereference occurs
> > in update_cfs_rq_h_load when accumulating h_load. The problem is that
> > cfq_rq->h_load_next is not protected by any locking and can be updated
> > by parallel calls to task_h_load. Depending on the compiler, code may be
> > generated that re-reads cfq_rq->h_load_next after the check for NULL and
> > then oops when reading se->avg.load_avg. The dissassembly showed that it
> > was possible to reread h_load_next after the check for NULL.
> >
> > While this does not appear to be an issue for later compilers, it's still
> > an accident if the correct code is generated. Full locking in this path
> > would have high overhead so this patch uses READ_ONCE to read h_load_next
> > only once and check for NULL before dereferencing. It was confirmed that
> > there were no further oops after 10 days of testing.
> >
>
> Does that also want a
>
> Fixes: 685207963be9 ("sched: Move h_load calculation to task_h_load()")
> Cc: stable@vger.kernel.org
>
> ?
>
> Other than that, the change looks sane to me.
>
> Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
>
Thanks. I'll add the Fixes because it does look like the old code would
have been ok. Thanks for the review!
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] sched: Do not re-read h_load_next during hierarchical load calculation
2019-03-19 12:30 ` Mel Gorman
@ 2019-03-19 12:36 ` Valentin Schneider
0 siblings, 0 replies; 6+ messages in thread
From: Valentin Schneider @ 2019-03-19 12:36 UTC (permalink / raw)
To: Mel Gorman; +Cc: Ingo Molnar, Peter Zijlstra, linux-kernel
On 19/03/2019 12:30, Mel Gorman wrote:
[...]
>>
>> Does that also want a
>>
>> Fixes: 685207963be9 ("sched: Move h_load calculation to task_h_load()")
>> Cc: stable@vger.kernel.org
>>
>> ?
>>
>> Other than that, the change looks sane to me.
>>
>> Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
>>
>
> Thanks. I'll add the Fixes because it does look like the old code would
> have been ok. Thanks for the review!
>
Well it's not worth much because I completely skipped that we *didn't*
already have a WRITE_ONCE(), which you do need as Peter mentioned. But I
do think stable will be happy to get this one :)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] sched: Do not re-read h_load_next during hierarchical load calculation
2019-03-19 9:35 [PATCH] sched: Do not re-read h_load_next during hierarchical load calculation Mel Gorman
2019-03-19 11:38 ` Valentin Schneider
@ 2019-03-19 12:06 ` Peter Zijlstra
2019-03-19 12:33 ` Mel Gorman
1 sibling, 1 reply; 6+ messages in thread
From: Peter Zijlstra @ 2019-03-19 12:06 UTC (permalink / raw)
To: Mel Gorman; +Cc: Ingo Molnar, linux-kernel
On Tue, Mar 19, 2019 at 09:35:18AM +0000, Mel Gorman wrote:
> A NULL pointer dereference bug was reported on a distribution kernel but
> the same issue should be present on mainline kernel. It occured on s390
> but should not be arch-specific. A partial oops looks like
>
> [775277.408564] Unable to handle kernel pointer dereference in virtual kernel address space
> ...
> [775277.408759] Call Trace:
> [775277.408763] ([<0002c11c56899c61>] 0x2c11c56899c61)
> [775277.408766] [<0000000000177bb4>] try_to_wake_up+0xfc/0x450
> [775277.408773] [<000003ff81ede872>] vhost_poll_wakeup+0x3a/0x50 [vhost]
> [775277.408777] [<0000000000194ae4>] __wake_up_common+0xbc/0x178
> [775277.408779] [<0000000000194f86>] __wake_up_common_lock+0x9e/0x160
> [775277.408780] [<00000000001950de>] __wake_up_sync_key+0x4e/0x60
> [775277.408785] [<00000000005d911e>] sock_def_readable+0x5e/0x98
>
> The bug hits any time between 1 hour to 3 days. The dereference occurs
> in update_cfs_rq_h_load when accumulating h_load. The problem is that
> cfq_rq->h_load_next is not protected by any locking and can be updated
> by parallel calls to task_h_load.
Hurpmh, right.
> Depending on the compiler, code may be
> generated that re-reads cfq_rq->h_load_next after the check for NULL and
> then oops when reading se->avg.load_avg. The dissassembly showed that it
> was possible to reread h_load_next after the check for NULL.
>
> While this does not appear to be an issue for later compilers, it's still
> an accident if the correct code is generated. Full locking in this path
> would have high overhead so this patch uses READ_ONCE to read h_load_next
> only once and check for NULL before dereferencing. It was confirmed that
> there were no further oops after 10 days of testing.
>
> Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
> ---
> kernel/sched/fair.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 310d0637fe4b..34aeb40e69d2 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -7726,7 +7726,7 @@ static void update_cfs_rq_h_load(struct cfs_rq *cfs_rq)
> cfs_rq->last_h_load_update = now;
> }
>
> - while ((se = cfs_rq->h_load_next) != NULL) {
> + while ((se = READ_ONCE(cfs_rq->h_load_next)) != NULL) {
> load = cfs_rq->h_load;
> load = div64_ul(load * se->avg.load_avg,
> cfs_rq_load_avg(cfs_rq) + 1);
Where there is a READ_ONCE there should also be a corresponding
WRITE_ONCE(). Otherwise the compiler can still screw us over by doing
store-tearing.
So something like the below. But looking at this, we probably also want
ONCE treatment on cfs_rq->h_load itself, but that's another patch.
And I think we can do something with cfs_rq->last_h_load_update.
---
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index fdab7eb6f351..40bd1e27b1b7 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -7784,10 +7784,10 @@ static void update_cfs_rq_h_load(struct cfs_rq *cfs_rq)
if (cfs_rq->last_h_load_update == now)
return;
- cfs_rq->h_load_next = NULL;
+ WRITE_ONCE(cfs_rq->h_load_next, NULL);
for_each_sched_entity(se) {
cfs_rq = cfs_rq_of(se);
- cfs_rq->h_load_next = se;
+ WRITE_ONCE(cfs_rq->h_load_next, se);
if (cfs_rq->last_h_load_update == now)
break;
}
@@ -7797,7 +7797,7 @@ static void update_cfs_rq_h_load(struct cfs_rq *cfs_rq)
cfs_rq->last_h_load_update = now;
}
- while ((se = cfs_rq->h_load_next) != NULL) {
+ while ((se = READ_ONCE(cfs_rq->h_load_next)) != NULL) {
load = cfs_rq->h_load;
load = div64_ul(load * se->avg.load_avg,
cfs_rq_load_avg(cfs_rq) + 1);
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] sched: Do not re-read h_load_next during hierarchical load calculation
2019-03-19 12:06 ` Peter Zijlstra
@ 2019-03-19 12:33 ` Mel Gorman
0 siblings, 0 replies; 6+ messages in thread
From: Mel Gorman @ 2019-03-19 12:33 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: Ingo Molnar, linux-kernel
On Tue, Mar 19, 2019 at 01:06:09PM +0100, Peter Zijlstra wrote:
> > ---
> > kernel/sched/fair.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index 310d0637fe4b..34aeb40e69d2 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -7726,7 +7726,7 @@ static void update_cfs_rq_h_load(struct cfs_rq *cfs_rq)
> > cfs_rq->last_h_load_update = now;
> > }
> >
> > - while ((se = cfs_rq->h_load_next) != NULL) {
> > + while ((se = READ_ONCE(cfs_rq->h_load_next)) != NULL) {
> > load = cfs_rq->h_load;
> > load = div64_ul(load * se->avg.load_avg,
> > cfs_rq_load_avg(cfs_rq) + 1);
>
> Where there is a READ_ONCE there should also be a corresponding
> WRITE_ONCE(). Otherwise the compiler can still screw us over by doing
> store-tearing.
>
> So something like the below. But looking at this, we probably also want
> ONCE treatment on cfs_rq->h_load itself, but that's another patch.
>
> And I think we can do something with cfs_rq->last_h_load_update.
>
Ok, I hadn't taken into account the possibility of store tearing but you're
right, it is a possibility depending on the architecture and your patch
is better. While there are some potential issues around h_load, I had
treated it as data that is approximate and only considered the potential
NULL dereference to be relevant.
I'll send a v2 shortly.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-03-19 12:36 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-03-19 9:35 [PATCH] sched: Do not re-read h_load_next during hierarchical load calculation Mel Gorman
2019-03-19 11:38 ` Valentin Schneider
2019-03-19 12:30 ` Mel Gorman
2019-03-19 12:36 ` Valentin Schneider
2019-03-19 12:06 ` Peter Zijlstra
2019-03-19 12:33 ` Mel Gorman
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.