Messages in this thread |  | | From | Linus Torvalds <> | Date | Fri, 30 Aug 2019 09:21:31 -0700 | Subject | Re: [BUG] Use of probe_kernel_address() in task_rcu_dereference() without checking return value |
| |
On Fri, Aug 30, 2019 at 9:10 AM Oleg Nesterov <oleg@redhat.com> wrote: > > > Yes, please see > > [PATCH 2/3] introduce probe_slab_address() > https://lore.kernel.org/lkml/20141027195425.GC11736@redhat.com/ > > 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
|  |