Re: [PATCH] uprobe: Warn if unable to install breakpoint

[Naveen N. Rao]

On 2017/09/01 03:09PM, Michael Ellerman wrote:
> "Naveen N. Rao"  writes:
> 
> > When we try to install a uprobe breakpoint in uprobe_mmap(), we ignore
> > all errors encountered in the process per this comment at the top of
> > the function:
> > /*
> >  * Called from mmap_region/vma_adjust with mm->mmap_sem acquired.
> >  *
> >  * Currently we ignore all errors and always return 0, the callers
> >  * can't handle the failure anyway.
> >  */
> >
> > However, this is very confusing for users since no probe hits are
> > recorded nor is an error logged in dmesg.
> >
> > Fix this by logging an error in dmesg so that users can discover that
> > there was an issue with the uprobe. To facilitate use of uprobe_warn(),
> > we move that function to the top of the file.
> >
> > With this patch, we see a message similar to this in dmesg:
> > [  201.449213] uprobe: uprobe_t:9740 failed to setup probe at 0x95c 
> > (-524)
> >
> > Reported-by: Anton Blanchard 
> > Signed-off-by: Naveen N. Rao 
> > ---
> >  kernel/events/uprobes.c | 21 ++---
> >  1 file changed, 14 insertions(+), 7 deletions(-)
> >
> > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> > index 0e137f98a50c..587c591a535c 100644
> > --- a/kernel/events/uprobes.c
> > +++ b/kernel/events/uprobes.c
> > @@ -112,6 +112,12 @@ struct xol_area {
> > unsigned long   vaddr;  /* Page(s) of 
> > instruction slots */
> >  };
> >  
> > +static void uprobe_warn(struct task_struct *t, const char *msg)
> > +{
> > +   pr_warn("uprobe: %s:%d failed to %s\n",
> > +   current->comm, current->pid, msg);
> 
> That should probably be ratelimited no?

Uprobes can only be installed by root today, so it is not as bad. But, I 
do agree that it is good to ratelimit. I will send a subsequent patch to 
do this.

Thanks for the review,
- Naveen

Re: [PATCH] uprobe: Warn if unable to install breakpoint

[Naveen N. Rao]

On 2017/09/01 03:09PM, Michael Ellerman wrote:
> "Naveen N. Rao"  writes:
> 
> > When we try to install a uprobe breakpoint in uprobe_mmap(), we ignore
> > all errors encountered in the process per this comment at the top of
> > the function:
> > /*
> >  * Called from mmap_region/vma_adjust with mm->mmap_sem acquired.
> >  *
> >  * Currently we ignore all errors and always return 0, the callers
> >  * can't handle the failure anyway.
> >  */
> >
> > However, this is very confusing for users since no probe hits are
> > recorded nor is an error logged in dmesg.
> >
> > Fix this by logging an error in dmesg so that users can discover that
> > there was an issue with the uprobe. To facilitate use of uprobe_warn(),
> > we move that function to the top of the file.
> >
> > With this patch, we see a message similar to this in dmesg:
> > [  201.449213] uprobe: uprobe_t:9740 failed to setup probe at 0x95c 
> > (-524)
> >
> > Reported-by: Anton Blanchard 
> > Signed-off-by: Naveen N. Rao 
> > ---
> >  kernel/events/uprobes.c | 21 ++---
> >  1 file changed, 14 insertions(+), 7 deletions(-)
> >
> > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> > index 0e137f98a50c..587c591a535c 100644
> > --- a/kernel/events/uprobes.c
> > +++ b/kernel/events/uprobes.c
> > @@ -112,6 +112,12 @@ struct xol_area {
> > unsigned long   vaddr;  /* Page(s) of 
> > instruction slots */
> >  };
> >  
> > +static void uprobe_warn(struct task_struct *t, const char *msg)
> > +{
> > +   pr_warn("uprobe: %s:%d failed to %s\n",
> > +   current->comm, current->pid, msg);
> 
> That should probably be ratelimited no?

Uprobes can only be installed by root today, so it is not as bad. But, I 
do agree that it is good to ratelimit. I will send a subsequent patch to 
do this.

Thanks for the review,
- Naveen

Re: [PATCH] uprobe: Warn if unable to install breakpoint

[Michael Ellerman]

"Naveen N. Rao"  writes:

> When we try to install a uprobe breakpoint in uprobe_mmap(), we ignore
> all errors encountered in the process per this comment at the top of
> the function:
> /*
>  * Called from mmap_region/vma_adjust with mm->mmap_sem acquired.
>  *
>  * Currently we ignore all errors and always return 0, the callers
>  * can't handle the failure anyway.
>  */
>
> However, this is very confusing for users since no probe hits are
> recorded nor is an error logged in dmesg.
>
> Fix this by logging an error in dmesg so that users can discover that
> there was an issue with the uprobe. To facilitate use of uprobe_warn(),
> we move that function to the top of the file.
>
> With this patch, we see a message similar to this in dmesg:
> [  201.449213] uprobe: uprobe_t:9740 failed to setup probe at 0x95c (-524)
>
> Reported-by: Anton Blanchard 
> Signed-off-by: Naveen N. Rao 
> ---
>  kernel/events/uprobes.c | 21 ++---
>  1 file changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 0e137f98a50c..587c591a535c 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -112,6 +112,12 @@ struct xol_area {
>   unsigned long   vaddr;  /* Page(s) of 
> instruction slots */
>  };
>  
> +static void uprobe_warn(struct task_struct *t, const char *msg)
> +{
> + pr_warn("uprobe: %s:%d failed to %s\n",
> + current->comm, current->pid, msg);

That should probably be ratelimited no?

cheers

Re: [PATCH] uprobe: Warn if unable to install breakpoint

[Michael Ellerman]

"Naveen N. Rao"  writes:

> When we try to install a uprobe breakpoint in uprobe_mmap(), we ignore
> all errors encountered in the process per this comment at the top of
> the function:
> /*
>  * Called from mmap_region/vma_adjust with mm->mmap_sem acquired.
>  *
>  * Currently we ignore all errors and always return 0, the callers
>  * can't handle the failure anyway.
>  */
>
> However, this is very confusing for users since no probe hits are
> recorded nor is an error logged in dmesg.
>
> Fix this by logging an error in dmesg so that users can discover that
> there was an issue with the uprobe. To facilitate use of uprobe_warn(),
> we move that function to the top of the file.
>
> With this patch, we see a message similar to this in dmesg:
> [  201.449213] uprobe: uprobe_t:9740 failed to setup probe at 0x95c (-524)
>
> Reported-by: Anton Blanchard 
> Signed-off-by: Naveen N. Rao 
> ---
>  kernel/events/uprobes.c | 21 ++---
>  1 file changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 0e137f98a50c..587c591a535c 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -112,6 +112,12 @@ struct xol_area {
>   unsigned long   vaddr;  /* Page(s) of 
> instruction slots */
>  };
>  
> +static void uprobe_warn(struct task_struct *t, const char *msg)
> +{
> + pr_warn("uprobe: %s:%d failed to %s\n",
> + current->comm, current->pid, msg);

That should probably be ratelimited no?

cheers