Re: [PATCH 12/13] tracing/uprobes: Add more fetch functions

[Steven Rostedt <rostedt@goodmis.org>]

On Mon, 4 Nov 2013 17:44:31 +0100
Oleg Nesterov <oleg@redhat.com> wrote:

> On 11/04, Namhyung Kim wrote:
> >
> > On Thu, 31 Oct 2013 19:22:18 +0100, Oleg Nesterov wrote:
> > > On 10/29, Namhyung Kim wrote:
> > >>
> > >> +static void __user *get_user_vaddr(unsigned long addr, struct trace_uprobe *tu)
> > >> +{
> > >> +	unsigned long pgoff = addr >> PAGE_SHIFT;
> > >> +	struct vm_area_struct *vma;
> > >> +	struct address_space *mapping;
> > >> +	unsigned long vaddr = 0;
> > >> +
> > >> +	if (tu == NULL) {
> > >> +		/* A NULL tu means that we already got the vaddr */
> > >> +		return (void __force __user *) addr;
> > >> +	}
> > >> +
> > >> +	mapping = tu->inode->i_mapping;
> > >> +
> > >> +	mutex_lock(&mapping->i_mmap_mutex);
> > >> +	vma_interval_tree_foreach(vma, &mapping->i_mmap, pgoff, pgoff) {
> > >> +		if (vma->vm_mm != current->mm)
> > >> +			continue;
> > >> +		if (!(vma->vm_flags & VM_READ))
> > >> +			continue;
> > >> +
> > >> +		vaddr = offset_to_vaddr(vma, addr);
> > >> +		break;
> > >> +	}
> > >> +	mutex_unlock(&mapping->i_mmap_mutex);
> > >> +
> > >> +	WARN_ON_ONCE(vaddr == 0);
> > >
> > > Hmm. But unless I missed something this "addr" passed as an argument can
> > > be wrong? And if nothing else this or another thread can unmap the vma?
> >
> > You mean WARN_ON_ONCE here is superfluous?  I admit that it should
> > protect concurrent vma [un]mappings.  Please see my reply in other
> > thread for a new approach.
> 
> Whatever we do this address can be unmapped. For example, just because of
> @invalid_address passed to trace_uprobe.c.
> 
> We do not really care, copy_from_user() should fail. But we should not
> WARN() in this case.
> 

I agree, the WARN_ON_ONCE() above looks like it's uncalled for.
WARN()ings should only be used when an anomaly in the kernel logic is
detected. Can this trigger on bad input from user space, or something
else that userspace does? (a race with unmapping memory?). If so, error
out to the user process, but do not call any of the WARN() functions.

-- Steve