Re: [BUG] Use of probe_kernel_address() in task_rcu_dereference() without checking return value

[Linus Torvalds]

On Fri, Aug 30, 2019 at 9:10 AM Oleg Nesterov <oleg@xxxxxxxxxx> wrote:
>
>
> Yes, please see
>
>         [PATCH 2/3] introduce probe_slab_address()
>         https://lore.kernel.org/lkml/20141027195425.GC11736@xxxxxxxxxx/
>
> I sent 5 years ago ;) Do you think
>
>         /*
>          * Same as probe_kernel_address(), but @addr must be the valid pointer
>          * to a slab object, potentially freed/reused/unmapped.
>          */
>         #ifdef CONFIG_DEBUG_PAGEALLOC
>         #define probe_slab_address(addr, retval)        \
>                 probe_kernel_address(addr, retval)
>         #else
>         #define probe_slab_address(addr, retval)        \
>                 ({                                                      \
>                         (retval) = *(typeof(retval) *)(addr);           \
>                         0;                                              \
>                 })
>         #endif
>
> can work?

Ugh. I would much rather handle the general case, because honestly,
tracing has had a lot of issues with our hacky "probe_kernel_read()"
stuff that bases itself on user addresses.

It's also one of the few remaining users of "set_fs()" in core code,
and we really should try to get rid of those.

So your code would work for this particular case, but not for other
cases that can trap simply because the pointer isn't reliable (tracing
being the main case for that - but if the source of the pointer itself
might have been free'd, you would also have that situation).

So I'd really prefer to have something a bit fancier. On most
architectures, doing a good exception fixup for kernel addresses is
really not that hard.

On x86, for example, we actually have *exactly* that. The
"__get_user_asm()" macro is basically it. It purely does a load
instruction from an unchecked address.

(It's a really odd syntax, but you could remove the __chk_user_ptr()
from the __get_user_size() macro, and now you'd have basically a "any
regular size kernel access with exception handlng").

But yes, your hack is I guess optimal for this particular case where
you simply can depend on "we know the pointer was valid, we just don't
know if it was freed".

Hmm. Don't we RCU-free the task struct? Because then we don't even
need to care about CONFIG_DEBUG_PAGEALLOC. We can just always access
the pointer as long as we have the RCU read lock. We do that in other
cases.

                    Linus