lkml.org 
[lkml]   [2019]   [Mar]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] sched: Do not re-read h_load_next during hierarchical load calculation
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!

\
 
 \ /
  Last update: 2019-03-19 13:31    [W:0.049 / U:0.300 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site