A "NULL pointer dereference " problem in pick_next_task_fair in linux 3.10.0 based

[åå]

hi everybody:

I have encountered a "NULL pointer dereference" problem in
pick_next_task_fair()  in linux 3.10.0 based

static struct task_struct *pick_next_task_fair(struct rq *rq)
{
    struct task_struct *p;
    struct cfs_rq *cfs_rq = &rq->cfs;
    struct sched_entity *se;

    if (!cfs_rq->nr_running)
        return NULL;

    do {
        se = pick_next_entity(cfs_rq);
        set_next_entity(cfs_rq, se);
        cfs_rq = group_cfs_rq(se);
    } while (cfs_rq);

    p = task_of(se);
    if (hrtick_enabled(rq))
        hrtick_start_fair(rq, p);

    return p;
}

In pick_next_entity():

static struct sched_entity *pick_next_entity(struct cfs_rq *cfs_rq)
{
    struct sched_entity *se = __pick_first_entity(cfs_rq);
    struct sched_entity *left = se;

    /*
     * Avoid running the skip buddy, if running something else can
     * be done without getting too unfair.
     */
    if (cfs_rq->skip == se) {
        struct sched_entity *second = __pick_next_entity(se);
        if (second && wakeup_preempt_entity(second, left) < 1)
            se = second;
    }

    /*
     * Prefer last buddy, try to return the CPU to a preempted task.
     */
    if (cfs_rq->last && wakeup_preempt_entity(cfs_rq->last, left) < 1)
        se = cfs_rq->last;

    /*
     * Someone really wants this to run. If it's not unfair, run it.
     */
    if (cfs_rq->next && wakeup_preempt_entity(cfs_rq->next, left) < 1)
        se = cfs_rq->next;

    clear_buddies(cfs_rq, se);

    return se;
}


in pick_next_entity

 1. struct sched_entity *se = __pick_first_entity(cfs_rq);
    This se may be get NULL.

2. Then if cfs_rq->skip is also null, cfs_rq->skip == se.

3. struct sched_entity *second = __pick_next_entity(se);
    This code will lead to the "NULL pointer dereference" because se
is NULL in __pick_next_entity -> rb_next()

The oops is as follow:

[795797.511960] BUG: unable to handle kernel NULL pointer dereference
at 0000000000000010


[795797.622950] task: ffff883f24924560 ti: ffff883f2495c000 task.ti:
ffff883f2495c000

[795797.630541] RIP: 0010:[]  [] rb_next+0x1/0x50

[795797.638153] RSP: 0018:ffff883f2495fdc0  EFLAGS: 00010046

[795797.643575] RAX: 0000000000000000 RBX: 0000000000000000 RCX:
0000000000000000

[795797.650824] RDX: 0000000000000001 RSI: ffff883f7f5f4868 RDI:
0000000000000010

[795797.658068] RBP: ffff883f2495fe08 R08: 0000000000000000 R09:
0000000000000000

[795797.666476] R10: 0000000000000000 R11: ffff883f249f5400 R12:
ffff883d82207200

[795797.674821] R13: 0000000000000000 R14: 0000000000000000 R15:
0000000000000000

[795797.683160] FS:  0000000000000000(0000) GS:ffff883f7f5e0000(0000)
knlGS:0000000000000000

[795797.692460] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033

[795797.699395] CR2: 0000000000000010 CR3: 0000003f1de58000 CR4:
00000000003407e0

[795797.707703] DR0: 0000000000000000 DR1: 0000000000000000 DR2:
0000000000000000

[795797.715992] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7:
0000000000000400

[795797.724267] Stack:

[795797.727416]  ffff883f2495fe08 ffffffff810b87f9 ffff883f2495fe08
ffff883f7f5f47c0

[795797.736040]  ffff883f24924b40 ffff883f7f5f47c0 000000000000003f
ffff883f24924560

[795797.744658]  0000000000000000 ffff883f2495fe68 ffffffff816324ea
ffff883f24924560

[795797.753255] Call Trace:

[795797.756818]  [] ? pick_next_task_fair+0x129/0x1d0

[795797.764207]  [] __schedule+0x12a/0x910

[795797.770622]  [] schedule+0x29/0x70

[795797.776683]  [] smpboot_thread_fn+0xd3/0x1a0

[795797.783599]  [] ? schedule+0x29/0x70

[795797.789807]  [] ? lg_double_unlock+0x90/0x90

[795797.796706]  [] kthread+0xcf/0xe0

[795797.802632]  [] ? kthread_create_on_node+0x140/0x140

[795797.810207]  [] ret_from_fork+0x58/0x90

[795797.816646]  [] ? kthread_create_on_node+0x140/0x140


I find there are something different in upstream (in pick_next_entity )

static struct sched_entity *

pick_next_entity(struct cfs_rq *cfs_rq, struct sched_entity *curr)

{
       struct sched_entity *left = __pick_first_entity(cfs_rq);
        struct sched_entity *se;
        /*
         * If curr is set we have to see if its left of the leftmost entity
         * still in the tree, provided there was anything in the tree at all.
         */
        if (!left || (curr && entity_before(curr, left)))
                left = curr;
        se = left; /* ideally we run the leftmost entity */
        /*
         * Avoid running the skip buddy, if running something else can
         * be done without getting too unfair.
         */
        if (cfs_rq->skip == se) {
                struct sched_entity *second;
                if (se == curr) {
                        second = __pick_first_entity(cfs_rq);
                } else {
                        second = __pick_next_entity(se);
                        if (!second || (curr && entity_before(curr, second)))
                                second = curr;
                }
                if (second && wakeup_preempt_entity(second, left) < 1)
                        se = second;
        }
        /*
         * Prefer last buddy, try to return the CPU to a preempted task.
         */
        if (cfs_rq->last && wakeup_preempt_entity(cfs_rq->last, left) < 1)
                se = cfs_rq->last;
        /*
         * Someone really wants this to run. If it's not unfair, run it.
         */
        if (cfs_rq->next && wakeup_preempt_entity(cfs_rq->next, left) < 1)
                se = cfs_rq->next;
        clear_buddies(cfs_rq, se);
        return se;

}

if the se,curr,cfs_rq->skip are all NULL, the wakeup_preempt_entity()
or  clear_buddies will be also occured the  "NULL pointer dereference"
problem.

I think the following patch can work in the case:

--- a/kernel/sched/fair.c

+++ b/kernel/sched/fair.c

@@ -4639,10 +4639,9 @@
    struct cfs_rq *cfs_rq = &rq->cfs;
    struct sched_entity *se;

-   if (!cfs_rq->nr_running)
-       return NULL;
-
    do {
+       if (!cfs_rq->nr_running)
+           return NULL;
        se = pick_next_entity(cfs_rq);
        set_next_entity(cfs_rq, se);
        cfs_rq = group_cfs_rq(se);



Do I miss some rules or this is a bug?