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

[Mathieu Desnoyers]

----- On Sep 3, 2019, at 1:14 PM, Linus Torvalds torvalds@xxxxxxxxxxxxxxxxxxxx wrote:

> 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.

Indeed, thanks for the explanation. Given that this code will likely be
changed by patchsets submitted by others which will possibly remove the
entire thing, and that it currently works as intended, I do not plan on
submitting any further patch to that function at this stage.

Thanks,

Mathieu



-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com