Messages in this thread | | | Date | Tue, 3 Sep 2019 12:56:34 -0400 (EDT) | From | Mathieu Desnoyers <> | Subject | Re: [RFC PATCH 1/3] Fix: sched: task_rcu_dereference: check probe_kernel_address return value |
| |
----- On Sep 3, 2019, at 12:12 PM, Linus Torvalds torvalds@linux-foundation.org wrote:
> On Tue, Sep 3, 2019 at 9:00 AM Mathieu Desnoyers > <mathieu.desnoyers@efficios.com> wrote: >> >> probe_kernel_address can return -EFAULT on error, which leads to use of >> an uninitialized or partially initialized sighand variable. > > I think this comment and this code is actively misleading. > > There is no "uninitialized or partially initialized sighand variable". > That's completely wrong. > > The sighand variable is always completely initialized. It's just that > the check for "is it initialized" is _not_ the return value from > probe_kernel_address(), because that return value is simply not > sufficient. > > So this is just wrong. Don't do it. You're just confusing the issue, > and you're making statments that aren't true in the commit message, > and making the code do a pointless and odd check. > > If you want to change this code for legibility, you should just add a > comment above the probe_kernel_address() about why the return value is > ignored, and why the check _below_ that code verifies the value of > sighand with a different check.
Then I must be misunderstanding something.
probe_kernel_address() is a macro wrapping probe_kernel_read(). mm/maccess.c:probe_kernel_read() calls probe_read_common() mm/maccess.c:probe_read_common() calls __copy_from_user_inatomic()
include/linux/uaccess.h:__copy_from_user_inatomic() documents:
* NOTE: only copy_from_user() zero-pads the destination in case of short copy. * Neither __copy_from_user() nor __copy_from_user_inatomic() zero anything * at all; their callers absolutely must check the return value.
So considering that comment, I suspect the on-stack sighand variable within task_rcu_dereference() can be left either uninitialized or (less likely) partially initialized if probe_kernel_address() returns -EFAULT.
Is there anything else that prevents probe_kernel_address from failing ? If so, why use probe_kernel_address in the first place ?
Thanks,
Mathieu
-- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com
| |