Messages in this thread | | | Date | Wed, 9 May 2018 10:41:55 +0100 | From | Mark Rutland <> | Subject | Re: [PATCH 2/3] kcov: prefault the kcov_area |
| |
On Tue, May 08, 2018 at 03:51:58PM -0700, Andrew Morton wrote: > On Fri, 4 May 2018 14:55:34 +0100 Mark Rutland <mark.rutland@arm.com> wrote: > > > On many architectures the vmalloc area is lazily faulted in upon first > > access. This is problematic for KCOV, as __sanitizer_cov_trace_pc > > accesses the (vmalloc'd) kcov_area, and fault handling code may be > > instrumented. If an access to kcov_area faults, this will result in > > mutual recursion through the fault handling code and > > __sanitizer_cov_trace_pc(), eventually leading to stack corruption > > and/or overflow. > > > > We can avoid this by faulting in the kcov_area before > > __sanitizer_cov_trace_pc() is permitted to access it. Once it has been > > faulted in, it will remain present in the process page tables, and will > > not fault again. > > > > ... > > > > --- a/kernel/kcov.c > > +++ b/kernel/kcov.c > > @@ -324,6 +324,17 @@ static int kcov_close(struct inode *inode, struct file *filep) > > return 0; > > } > > > > +static void kcov_fault_in_area(struct kcov *kcov) > > It would be nice to have a comment here explaining why the function > exists. > > umm, this? > > --- a/kernel/kcov.c~kcov-prefault-the-kcov_area-fix-fix > +++ a/kernel/kcov.c > @@ -324,6 +324,10 @@ static int kcov_close(struct inode *inod > return 0; > } > > +/* > + * fault in a lazily-faulted vmalloc area, to avoid recursion issues if the > + * vmalloc fault handler itself is instrumented. > + */
Sounds good to me. Perhaps we want a little more detail, e.g.
/* * Fault in a lazily-faulted vmalloc area before it can be used by * __santizer_cov_trace_pc(), to avoid recursion issues if any code on * the vmalloc fault handling path is instrumented. */
> static void kcov_fault_in_area(struct kcov *kcov)
I also think it might make sense to rename this to kcov_prefault_area(), so that this doesn't sound like a fault handler, but that's not a big deal.
Thanks for handling this!
Mark.
| |