Re: [RFC v8 PATCH 2/5] uprobes: introduce has_uprobes helper

[Yang Shi]

On 8/22/18 8:07 AM, Srikar Dronamraju wrote:
> * Vlastimil Babka <vbabka@xxxxxxx> [2018-08-22 12:55:59]:
>
> > On 08/15/2018 08:49 PM, Yang Shi wrote:
> > > We need check if mm or vma has uprobes in the following patch to check
> > > if a vma could be unmapped with holding read mmap_sem. The checks and
> > > pre-conditions used by uprobe_munmap() look just suitable for this
> > > purpose.
> > >
> > > Extracting those checks into a helper function, has_uprobes().
> > >
> > > Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> > > Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> > > Cc: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
> > > Cc: Alexander Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx>
> > > Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
> > > Cc: Namhyung Kim <namhyung@xxxxxxxxxx>
> > > Cc: Vlastimil Babka <vbabka@xxxxxxx>
> > > Signed-off-by: Yang Shi <yang.shi@xxxxxxxxxxxxxxxxx>
> > > ---
> > >   include/linux/uprobes.h |  7 +++++++
> > >   kernel/events/uprobes.c | 23 ++++++++++++++++-------
> > >   2 files changed, 23 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
> > > index 0a294e9..418764e 100644
> > > --- a/include/linux/uprobes.h
> > > +++ b/include/linux/uprobes.h
> > > @@ -149,6 +149,8 @@ struct uprobes_state {
> > >   extern bool arch_uprobe_ignore(struct arch_uprobe *aup, struct pt_regs *regs);
> > >   extern void arch_uprobe_copy_ixol(struct page *page, unsigned long vaddr,
> > >   					 void *src, unsigned long len);
> > > +extern bool has_uprobes(struct vm_area_struct *vma, unsigned long start,
> > > +			unsigned long end);
> > >   #else /* !CONFIG_UPROBES */
> > >   struct uprobes_state {
> > >   };
> > > @@ -203,5 +205,10 @@ static inline void uprobe_copy_process(struct task_struct *t, unsigned long flag
> > >   static inline void uprobe_clear_state(struct mm_struct *mm)
> > >   {
> > >   }
> > > +static inline bool has_uprobes(struct vm_area_struct *vma, unsigned long start,
> > > +			       unsgined long end)
> > > +{
> > > +	return false;
> > > +}
> > >   #endif /* !CONFIG_UPROBES */
> > >   #endif	/* _LINUX_UPROBES_H */
> > > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> > > index aed1ba5..568481c 100644
> > > --- a/kernel/events/uprobes.c
> > > +++ b/kernel/events/uprobes.c
> > > @@ -1114,22 +1114,31 @@ int uprobe_mmap(struct vm_area_struct *vma)
> > >   	return !!n;
> > >   }
> > >   
> > > -/*
> > > - * Called in context of a munmap of a vma.
> > > - */
> > > -void uprobe_munmap(struct vm_area_struct *vma, unsigned long start, unsigned long end)
> > > +bool
> > > +has_uprobes(struct vm_area_struct *vma, unsigned long start, unsigned long end)
> > The name is not really great...
> I too feel the name is not apt.
> Can you make this vma_has_uprobes and convert the current
> vma_has_uprobes to __vma_has_uprobes?

It sounds good to me.

> > >   {
> > >   	if (no_uprobe_events() || !valid_vma(vma, false))
> > > -		return;
> > > +		return false;
> > >   
> > >   	if (!atomic_read(&vma->vm_mm->mm_users)) /* called by mmput() ? */
> > > -		return;
> > > +		return false;
> > >   
> > >   	if (!test_bit(MMF_HAS_UPROBES, &vma->vm_mm->flags) ||
> > >   	     test_bit(MMF_RECALC_UPROBES, &vma->vm_mm->flags))
> > This means that vma might have uprobes, but since RECALC is already set,
> > we don't need to set it again. That's different from "has uprobes".
> >
> > Perhaps something like vma_needs_recalc_uprobes() ?
> >
> > But I also worry there might be a race where we initially return false
> > because of MMF_RECALC_UPROBES, then the flag is cleared while vma's
> > still have uprobes, then we downgrade mmap_sem and skip uprobe_munmap().
> > Should be checked if e.g. mmap_sem and vma visibility changes protects
> > this case from happening.
> That is a very good observation.
>
> One think we can probably do is pass an extra parameter to
> has_uprobes(), depending on which we should skip this check.
> such that when we call from uprobes_munmap(), we continue as is
> but when calling from do_munmap_zap_rlock(), we skip the check.

Yes, it sounds good to solve the race issue. When we need decide if we 
should jump to regular path for uprobes mapping, we don't check if 
MMF_RECALC_UPROBES is set or not. It looks not harmful to set this flag 
twice.

Thanks,
Yang

> > > -		return;
> > > +		return false;
> > >   
> > >   	if (vma_has_uprobes(vma, start, end))
> > > +		return true;
> > > +
> > > +	return false;
> > Simpler:
> > 	return vma_has_uprobes(vma, start, end);
> >
> > > +}
> > > +
> > > +/*
> > > + * Called in context of a munmap of a vma.
> > > + */
> > > +void uprobe_munmap(struct vm_area_struct *vma, unsigned long start, unsigned long end)
> > > +{
> > > +	if (has_uprobes(vma, start, end))
> > >   		set_bit(MMF_RECALC_UPROBES, &vma->vm_mm->flags);
> > >   }