Re: [RFC V2] mm: Generalize notify_page_fault()
On 06/05/2019 04:53 PM, Matthew Wilcox wrote: > On Wed, Jun 05, 2019 at 09:19:22PM +1000, Michael Ellerman wrote: >> Anshuman Khandual writes: >>> Similar notify_page_fault() definitions are being used by architectures >>> duplicating much of the same code. This attempts to unify them into a >>> single implementation, generalize it and then move it to a common place. >>> kprobes_built_in() can detect CONFIG_KPROBES, hence notify_page_fault() >>> need not be wrapped again within CONFIG_KPROBES. Trap number argument can >>> now contain upto an 'unsigned int' accommodating all possible platforms. >> ... >> >> You've changed several of the architectures from something like above, >> where it disables preemption around the call into the below: >> >> >> Which skips everything if we're preemptible. Is that an equivalent >> change? If so can you please explain why in more detail. > > See the discussion in v1 of this patch, which you were cc'd on. > > I agree the description here completely fails to mention why the change. > It should mention commit a980c0ef9f6d8c. I will update the commit message to include an explanation for this new preempt behavior in the generic definition.
Re: [RFC V2] mm: Generalize notify_page_fault()
On 06/05/2019 04:49 PM, Michael Ellerman wrote:
> Anshuman Khandual writes:
>> Similar notify_page_fault() definitions are being used by architectures
>> duplicating much of the same code. This attempts to unify them into a
>> single implementation, generalize it and then move it to a common place.
>> kprobes_built_in() can detect CONFIG_KPROBES, hence notify_page_fault()
>> need not be wrapped again within CONFIG_KPROBES. Trap number argument can
>> now contain upto an 'unsigned int' accommodating all possible platforms.
> ...
>> diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c
>> index 58f69fa..1bc3b18 100644
>> --- a/arch/arm/mm/fault.c
>> +++ b/arch/arm/mm/fault.c
>> @@ -30,28 +30,6 @@
>>
>> #ifdef CONFIG_MMU
>>
>> -#ifdef CONFIG_KPROBES
>> -static inline int notify_page_fault(struct pt_regs *regs, unsigned int fsr)
>> -{
>> -int ret = 0;
>> -
>> -if (!user_mode(regs)) {
>> -/* kprobe_running() needs smp_processor_id() */
>> -preempt_disable();
>> -if (kprobe_running() && kprobe_fault_handler(regs, fsr))
>> -ret = 1;
>> -preempt_enable();
>> -}
>> -
>> -return ret;
>> -}
>> -#else
>
> You've changed several of the architectures from something like above,
> where it disables preemption around the call into the below:
>
>> +int __kprobes notify_page_fault(struct pt_regs *regs, unsigned int trap)
>> +{
>> +int ret = 0;
>> +
>> +/*
>> + * To be potentially processing a kprobe fault and to be allowed
>> + * to call kprobe_running(), we have to be non-preemptible.
>> + */
>> +if (kprobes_built_in() && !preemptible() && !user_mode(regs)) {
>> +if (kprobe_running() && kprobe_fault_handler(regs, trap))
>> +ret = 1;
>> +}
>> +return ret;
>> +}
>
> Which skips everything if we're preemptible. Is that an equivalent
Right.
> change? If so can you please explain why in more detail.
It is probably not an equivalent change. The following explanation is extracted
from
RFC V1 discussions (https://patchwork.kernel.org/patch/10968273/). Will explain
the
rational for this behavior change in the commit message next time around.
a980c0ef9f6d ("x86/kprobes: Refactor kprobes_fault() like
kprobe_exceptions_notify()")
b506a9d08bae ("x86: code clarification patch to Kprobes arch code")
In particular the later one (b506a9d08bae). It explains how the invoking context
in itself should be non-preemptible for the kprobes processing context
irrespective
of whether kprobe_running() or perhaps smp_processor_id() is safe or not. Hence
it
does not make much sense to continue when original invoking context is
preemptible.
Instead just bail out earlier. This seems to be making more sense than preempt
disable-enable pair. If there are no concerns about this change from other
platforms,
I will change the preemption behavior in proposed generic function next time
around.
Do you see any concern changing preempt behavior in the x86 way ?
>
> Also why not have it return bool?
Just that all architectures (except powerpc) had 'int' as return type. But we
can
change that to 'bool'.
Re: [RFC V2] mm: Generalize notify_page_fault()
On 06/05/2019 03:23 AM, Matthew Wilcox wrote:
> On Tue, Jun 04, 2019 at 12:04:06PM +0530, Anshuman Khandual wrote:
>> +++ b/arch/x86/mm/fault.c
>> @@ -46,23 +46,6 @@ kmmio_fault(struct pt_regs *regs, unsigned long addr)
>> return 0;
>> }
>>
>> -static nokprobe_inline int kprobes_fault(struct pt_regs *regs)
>> -{
> ...
>> -}
>
>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>> index 0e8834a..c5a8dcf 100644
>> --- a/include/linux/mm.h
>> +++ b/include/linux/mm.h
>> @@ -1778,6 +1778,7 @@ static inline int pte_devmap(pte_t pte)
>> }
>> #endif
>>
>> +int notify_page_fault(struct pt_regs *regs, unsigned int trap);
>
> Why is it now out-of-line?
Did not get it. AFAICS it is the same from last version and does not cross
80 characters limit on that line.
>
>> +++ b/mm/memory.c
>> +int __kprobes notify_page_fault(struct pt_regs *regs, unsigned int trap)
>> +{
>> +int ret = 0;
>> +
>> +/*
>> + * To be potentially processing a kprobe fault and to be allowed
>> + * to call kprobe_running(), we have to be non-preemptible.
>> + */
>> +if (kprobes_built_in() && !preemptible() && !user_mode(regs)) {
>> +if (kprobe_running() && kprobe_fault_handler(regs, trap))
>> +ret = 1;
>> +}
>> +return ret;
>> +}
>> +
>
> I would argue this should be in kprobes.h as a static nokprobe_inline.
We can do that. Though it will be a stand alone (not inside #ifdef) as it
already takes care of CONFIG_KPROBES via kprobes_built_in(). Will change
it and in which case the above declaration in mm.h would not be required.
Re: [RFC V2] mm: Generalize notify_page_fault()
On Wed, Jun 05, 2019 at 09:19:22PM +1000, Michael Ellerman wrote: > Anshuman Khandual writes: > > Similar notify_page_fault() definitions are being used by architectures > > duplicating much of the same code. This attempts to unify them into a > > single implementation, generalize it and then move it to a common place. > > kprobes_built_in() can detect CONFIG_KPROBES, hence notify_page_fault() > > need not be wrapped again within CONFIG_KPROBES. Trap number argument can > > now contain upto an 'unsigned int' accommodating all possible platforms. > ... > > You've changed several of the architectures from something like above, > where it disables preemption around the call into the below: > > > Which skips everything if we're preemptible. Is that an equivalent > change? If so can you please explain why in more detail. See the discussion in v1 of this patch, which you were cc'd on. I agree the description here completely fails to mention why the change. It should mention commit a980c0ef9f6d8c. > Also why not have it return bool? > > cheers >
Re: [RFC V2] mm: Generalize notify_page_fault()
Anshuman Khandual writes:
> Similar notify_page_fault() definitions are being used by architectures
> duplicating much of the same code. This attempts to unify them into a
> single implementation, generalize it and then move it to a common place.
> kprobes_built_in() can detect CONFIG_KPROBES, hence notify_page_fault()
> need not be wrapped again within CONFIG_KPROBES. Trap number argument can
> now contain upto an 'unsigned int' accommodating all possible platforms.
...
> diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c
> index 58f69fa..1bc3b18 100644
> --- a/arch/arm/mm/fault.c
> +++ b/arch/arm/mm/fault.c
> @@ -30,28 +30,6 @@
>
> #ifdef CONFIG_MMU
>
> -#ifdef CONFIG_KPROBES
> -static inline int notify_page_fault(struct pt_regs *regs, unsigned int fsr)
> -{
> - int ret = 0;
> -
> - if (!user_mode(regs)) {
> - /* kprobe_running() needs smp_processor_id() */
> - preempt_disable();
> - if (kprobe_running() && kprobe_fault_handler(regs, fsr))
> - ret = 1;
> - preempt_enable();
> - }
> -
> - return ret;
> -}
> -#else
You've changed several of the architectures from something like above,
where it disables preemption around the call into the below:
> +int __kprobes notify_page_fault(struct pt_regs *regs, unsigned int trap)
> +{
> + int ret = 0;
> +
> + /*
> + * To be potentially processing a kprobe fault and to be allowed
> + * to call kprobe_running(), we have to be non-preemptible.
> + */
> + if (kprobes_built_in() && !preemptible() && !user_mode(regs)) {
> + if (kprobe_running() && kprobe_fault_handler(regs, trap))
> + ret = 1;
> + }
> + return ret;
> +}
Which skips everything if we're preemptible. Is that an equivalent
change? If so can you please explain why in more detail.
Also why not have it return bool?
cheers
Re: [RFC V2] mm: Generalize notify_page_fault()
On Tue, Jun 04, 2019 at 12:04:06PM +0530, Anshuman Khandual wrote:
> +++ b/arch/x86/mm/fault.c
> @@ -46,23 +46,6 @@ kmmio_fault(struct pt_regs *regs, unsigned long addr)
> return 0;
> }
>
> -static nokprobe_inline int kprobes_fault(struct pt_regs *regs)
> -{
...
> -}
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 0e8834a..c5a8dcf 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1778,6 +1778,7 @@ static inline int pte_devmap(pte_t pte)
> }
> #endif
>
> +int notify_page_fault(struct pt_regs *regs, unsigned int trap);
Why is it now out-of-line?
> +++ b/mm/memory.c
> +int __kprobes notify_page_fault(struct pt_regs *regs, unsigned int trap)
> +{
> + int ret = 0;
> +
> + /*
> + * To be potentially processing a kprobe fault and to be allowed
> + * to call kprobe_running(), we have to be non-preemptible.
> + */
> + if (kprobes_built_in() && !preemptible() && !user_mode(regs)) {
> + if (kprobe_running() && kprobe_fault_handler(regs, trap))
> + ret = 1;
> + }
> + return ret;
> +}
> +
I would argue this should be in kprobes.h as a static nokprobe_inline.
Re: [RFC V2] mm: Generalize notify_page_fault()
On 06/04/2019 12:24 PM, Peter Zijlstra wrote:
> On Tue, Jun 04, 2019 at 12:04:06PM +0530, Anshuman Khandual wrote:
>> diff --git a/mm/memory.c b/mm/memory.c
>> index ddf20bd..b6bae8f 100644
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -52,6 +52,7 @@
>> #include
>> #include
>> #include
>> +#include
>> #include
>> #include
>> #include
>> @@ -141,6 +142,21 @@ static int __init init_zero_pfn(void)
>> core_initcall(init_zero_pfn);
>>
>>
>> +int __kprobes notify_page_fault(struct pt_regs *regs, unsigned int trap)
>> +{
>> +int ret = 0;
>> +
>> +/*
>> + * To be potentially processing a kprobe fault and to be allowed
>> + * to call kprobe_running(), we have to be non-preemptible.
>> + */
>> +if (kprobes_built_in() && !preemptible() && !user_mode(regs)) {
>> +if (kprobe_running() && kprobe_fault_handler(regs, trap))
>> +ret = 1;
>> +}
>> +return ret;
>> +}
>
> That thing should be called kprobe_page_fault() or something,
> notify_page_fault() is a horribly crap name for this function.
Agreed. kprobe_page_fault() sounds good.
Re: [RFC V2] mm: Generalize notify_page_fault()
On Tue, Jun 04, 2019 at 12:04:06PM +0530, Anshuman Khandual wrote:
> diff --git a/mm/memory.c b/mm/memory.c
> index ddf20bd..b6bae8f 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -52,6 +52,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -141,6 +142,21 @@ static int __init init_zero_pfn(void)
> core_initcall(init_zero_pfn);
>
>
> +int __kprobes notify_page_fault(struct pt_regs *regs, unsigned int trap)
> +{
> + int ret = 0;
> +
> + /*
> + * To be potentially processing a kprobe fault and to be allowed
> + * to call kprobe_running(), we have to be non-preemptible.
> + */
> + if (kprobes_built_in() && !preemptible() && !user_mode(regs)) {
> + if (kprobe_running() && kprobe_fault_handler(regs, trap))
> + ret = 1;
> + }
> + return ret;
> +}
That thing should be called kprobe_page_fault() or something,
notify_page_fault() is a horribly crap name for this function.
