[PATCH] ARM: mm: Facilitate debugging CONFIG_KUSER_HELPERS disabled

[PATCH] ARM: mm: Facilitate debugging CONFIG_KUSER_HELPERS disabled

[Florian Fainelli]

Some software such as perf makes unconditional use of the special
[vectors] page which is only provided when CONFIG_KUSER_HELPERS is
enabled in the kernel.

Facilitate the debugging of such situations by printing a debug message
to the kernel log showing the task name and the faulting address.

Suggested-by: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 arch/arm/mm/fault.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c
index f4ea4c62c613..f17471fbc1c4 100644
--- a/arch/arm/mm/fault.c
+++ b/arch/arm/mm/fault.c
@@ -173,6 +173,11 @@ __do_user_fault(struct task_struct *tsk, unsigned long addr,
 		show_regs(regs);
 	}
 #endif
+#ifndef CONFIG_KUSER_HELPERS
+	if ((sig == SIGSEGV) && ((addr & PAGE_MASK) == 0xffff0000))
+		printk(KERN_DEBUG "%s: CONFIG_KUSER_HELPERS disabled at 0x%08lx\n",
+		       tsk->comm, addr);
+#endif
 
 	tsk->thread.address = addr;
 	tsk->thread.error_code = fsr;
-- 
2.17.1

Re: [PATCH] ARM: mm: Facilitate debugging CONFIG_KUSER_HELPERS disabled

[Eric W. Biederman]

Florian Fainelli <f.fainelli@gmail.com> writes:

> Some software such as perf makes unconditional use of the special
> [vectors] page which is only provided when CONFIG_KUSER_HELPERS is
> enabled in the kernel.
>
> Facilitate the debugging of such situations by printing a debug message
> to the kernel log showing the task name and the faulting address.

Can't someone trigger this segv deliberately and spam the kerne log?
Doesn't this need something like printk_ratelimit or something to ensure
this message only gets printed once?

Eric

> Suggested-by: Russell King <rmk+kernel@armlinux.org.uk>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  arch/arm/mm/fault.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c
> index f4ea4c62c613..f17471fbc1c4 100644
> --- a/arch/arm/mm/fault.c
> +++ b/arch/arm/mm/fault.c
> @@ -173,6 +173,11 @@ __do_user_fault(struct task_struct *tsk, unsigned long addr,
>  		show_regs(regs);
>  	}
>  #endif
> +#ifndef CONFIG_KUSER_HELPERS
> +	if ((sig == SIGSEGV) && ((addr & PAGE_MASK) == 0xffff0000))
> +		printk(KERN_DEBUG "%s: CONFIG_KUSER_HELPERS disabled at 0x%08lx\n",
> +		       tsk->comm, addr);
> +#endif
>  
>  	tsk->thread.address = addr;
>  	tsk->thread.error_code = fsr;

Re: [PATCH] ARM: mm: Facilitate debugging CONFIG_KUSER_HELPERS disabled

[Souptick Joarder]

On Thu, Oct 25, 2018 at 11:40 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>
> Some software such as perf makes unconditional use of the special
> [vectors] page which is only provided when CONFIG_KUSER_HELPERS is
> enabled in the kernel.
>
> Facilitate the debugging of such situations by printing a debug message
> to the kernel log showing the task name and the faulting address.
>
> Suggested-by: Russell King <rmk+kernel@armlinux.org.uk>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  arch/arm/mm/fault.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c
> index f4ea4c62c613..f17471fbc1c4 100644
> --- a/arch/arm/mm/fault.c
> +++ b/arch/arm/mm/fault.c
> @@ -173,6 +173,11 @@ __do_user_fault(struct task_struct *tsk, unsigned long addr,
>                 show_regs(regs);
>         }
>  #endif
> +#ifndef CONFIG_KUSER_HELPERS

Just have one doubt, if the condition is "#ifdef CONFIG_KUSER_HELPER"
as commit message suggests the scenario is valid when CONFIG_KUSER_HELPER
is enabled ? No ?

> +       if ((sig == SIGSEGV) && ((addr & PAGE_MASK) == 0xffff0000))
> +               printk(KERN_DEBUG "%s: CONFIG_KUSER_HELPERS disabled at 0x%08lx\n",
> +                      tsk->comm, addr);
> +#endif
>
>         tsk->thread.address = addr;
>         tsk->thread.error_code = fsr;
> --
> 2.17.1
>

Re: [PATCH] ARM: mm: Facilitate debugging CONFIG_KUSER_HELPERS disabled

[Florian Fainelli]

On 10/25/18 11:30 AM, Souptick Joarder wrote:
> On Thu, Oct 25, 2018 at 11:40 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>>
>> Some software such as perf makes unconditional use of the special
>> [vectors] page which is only provided when CONFIG_KUSER_HELPERS is
>> enabled in the kernel.
>>
>> Facilitate the debugging of such situations by printing a debug message
>> to the kernel log showing the task name and the faulting address.
>>
>> Suggested-by: Russell King <rmk+kernel@armlinux.org.uk>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>  arch/arm/mm/fault.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c
>> index f4ea4c62c613..f17471fbc1c4 100644
>> --- a/arch/arm/mm/fault.c
>> +++ b/arch/arm/mm/fault.c
>> @@ -173,6 +173,11 @@ __do_user_fault(struct task_struct *tsk, unsigned long addr,
>>                 show_regs(regs);
>>         }
>>  #endif
>> +#ifndef CONFIG_KUSER_HELPERS
> 
> Just have one doubt, if the condition is "#ifdef CONFIG_KUSER_HELPER"
> as commit message suggests the scenario is valid when CONFIG_KUSER_HELPER
> is enabled ? No ?

#ifndef CONFIG_KUSER_HELPERS is what is intended here, when that option
is not enabled, there is no [vectors] page provided in a program's
virtual address space, so accesses to that virtual address will cause a
fault which we are catching here.

When CONFIG_KUSER_HELPERS is enabled, every program gets a valid vectors
page in its virtual address space, and accessing that address would not
cause a fault, since the page is there.

Does this clarify the intent of this commit?

> 
>> +       if ((sig == SIGSEGV) && ((addr & PAGE_MASK) == 0xffff0000))
>> +               printk(KERN_DEBUG "%s: CONFIG_KUSER_HELPERS disabled at 0x%08lx\n",
>> +                      tsk->comm, addr);
>> +#endif
>>
>>         tsk->thread.address = addr;
>>         tsk->thread.error_code = fsr;
>> --
>> 2.17.1
>>


-- 
Florian

Re: [PATCH] ARM: mm: Facilitate debugging CONFIG_KUSER_HELPERS disabled

[Florian Fainelli]

On 10/25/18 11:25 AM, Eric W. Biederman wrote:
> Florian Fainelli <f.fainelli@gmail.com> writes:
> 
>> Some software such as perf makes unconditional use of the special
>> [vectors] page which is only provided when CONFIG_KUSER_HELPERS is
>> enabled in the kernel.
>>
>> Facilitate the debugging of such situations by printing a debug message
>> to the kernel log showing the task name and the faulting address.
> 
> Can't someone trigger this segv deliberately and spam the kerne log?
> Doesn't this need something like printk_ratelimit or something to ensure
> this message only gets printed once?

Yes, good point, thanks.
-- 
Florian

Re: [PATCH] ARM: mm: Facilitate debugging CONFIG_KUSER_HELPERS disabled

[Souptick Joarder]

On Fri, Oct 26, 2018 at 12:18 AM Florian Fainelli <f.fainelli@gmail.com> wrote:
>
> On 10/25/18 11:30 AM, Souptick Joarder wrote:
> > On Thu, Oct 25, 2018 at 11:40 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
> >>
> >> Some software such as perf makes unconditional use of the special
> >> [vectors] page which is only provided when CONFIG_KUSER_HELPERS is
> >> enabled in the kernel.
> >>
> >> Facilitate the debugging of such situations by printing a debug message
> >> to the kernel log showing the task name and the faulting address.
> >>
> >> Suggested-by: Russell King <rmk+kernel@armlinux.org.uk>
> >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> >> ---
> >>  arch/arm/mm/fault.c | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> >> diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c
> >> index f4ea4c62c613..f17471fbc1c4 100644
> >> --- a/arch/arm/mm/fault.c
> >> +++ b/arch/arm/mm/fault.c
> >> @@ -173,6 +173,11 @@ __do_user_fault(struct task_struct *tsk, unsigned long addr,
> >>                 show_regs(regs);
> >>         }
> >>  #endif
> >> +#ifndef CONFIG_KUSER_HELPERS
> >
> > Just have one doubt, if the condition is "#ifdef CONFIG_KUSER_HELPER"
> > as commit message suggests the scenario is valid when CONFIG_KUSER_HELPER
> > is enabled ? No ?
>
> #ifndef CONFIG_KUSER_HELPERS is what is intended here, when that option
> is not enabled, there is no [vectors] page provided in a program's
> virtual address space, so accesses to that virtual address will cause a
> fault which we are catching here.
>
> When CONFIG_KUSER_HELPERS is enabled, every program gets a valid vectors
> page in its virtual address space, and accessing that address would not
> cause a fault, since the page is there.
>
> Does this clarify the intent of this commit?

Yes, It's clear. It might be good to add little more description on
commit message here.
But it's your choice on taking this input :-)
>
> >
> >> +       if ((sig == SIGSEGV) && ((addr & PAGE_MASK) == 0xffff0000))
> >> +               printk(KERN_DEBUG "%s: CONFIG_KUSER_HELPERS disabled at 0x%08lx\n",
> >> +                      tsk->comm, addr);
> >> +#endif
> >>
> >>         tsk->thread.address = addr;
> >>         tsk->thread.error_code = fsr;
> >> --
> >> 2.17.1
> >>
>
>
> --
> Florian

Re: [PATCH] ARM: mm: Facilitate debugging CONFIG_KUSER_HELPERS disabled

[Russell King - ARM Linux]

On Fri, Oct 26, 2018 at 12:00:09AM +0530, Souptick Joarder wrote:
> On Thu, Oct 25, 2018 at 11:40 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
> >
> > Some software such as perf makes unconditional use of the special
> > [vectors] page which is only provided when CONFIG_KUSER_HELPERS is
> > enabled in the kernel.
> >
> > Facilitate the debugging of such situations by printing a debug message
> > to the kernel log showing the task name and the faulting address.
> >
> > Suggested-by: Russell King <rmk+kernel@armlinux.org.uk>
> > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> > ---
> >  arch/arm/mm/fault.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c
> > index f4ea4c62c613..f17471fbc1c4 100644
> > --- a/arch/arm/mm/fault.c
> > +++ b/arch/arm/mm/fault.c
> > @@ -173,6 +173,11 @@ __do_user_fault(struct task_struct *tsk, unsigned long addr,
> >                 show_regs(regs);
> >         }
> >  #endif
> > +#ifndef CONFIG_KUSER_HELPERS
> 
> Just have one doubt, if the condition is "#ifdef CONFIG_KUSER_HELPER"
> as commit message suggests the scenario is valid when CONFIG_KUSER_HELPER
> is enabled ? No ?
> 
> > +       if ((sig == SIGSEGV) && ((addr & PAGE_MASK) == 0xffff0000))
> > +               printk(KERN_DEBUG "%s: CONFIG_KUSER_HELPERS disabled at 0x%08lx\n",
> > +                      tsk->comm, addr);
> > +#endif

The idea is to print a message when we get a SEGV _and_ the faulting
address is in the vectors page _and_ CONFIG_KUSER_HELPERS is _not_ set
(which makes the page inaccessible.)  The message points users to
enable it and/or why the application has failed.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up