Re: [RFC PATCH 1/3] Fix: sched: task_rcu_dereference: check probe_kernel_address return value

[Linus Torvalds]

On Tue, Sep 3, 2019 at 9:56 AM Mathieu Desnoyers
<mathieu.desnoyers@xxxxxxxxxxxx> wrote:
>
> Then I must be misunderstanding something.
>
> probe_kernel_address() is a macro wrapping probe_kernel_read().

Don't look at probe_kernel_address().

As long as you only look at that, you will be missing the big picture.

Instead, look at the code below it:

        /*
         * Pairs with atomic_dec_and_test() in put_task_struct(). If this task
         * was already freed we can not miss the preceding update of this
         * pointer.
         */
        smp_rmb();
        if (unlikely(task != READ_ONCE(*ptask)))
                goto retry;


That code is the code that verifies "ok, the pointer was valid over
the whole sequence, so the probe_kernel_address() must have succeeded"

So the code *does* check for success, but it does so using a
*stronger* check than the return value of probe_kernel_address().

If the task on the runqueue hasn't changed, then the
probe_kernel_read() cannot have failed.

But the reverse test is not true: if the probe_kernel_read()
succeeded, that doesn't guarantee that the value we read was
consistent.

So the check for failure is there, and the check that does exist is
the correct and stronger check.

Which is why checking the return value of probe_kernel_read() is
immaterial and pointless.

But a comment about this above the probe_kernel_read() may indeed be
worth it, since it seems to be unclear to so many people.

The code basically just wants to do a kernel memory access, knowing
that it's speculative. And the _only_ reason for using
probe_kernel_read() is that with DEBUG_PAGEALLOC you might have a page
fault on the speculative access.

But we do the speculation verification check afterwards, and that's
the important part.

                      Linus