On Mon, 4 Nov 2013 16:01:12 +0100, Oleg Nesterov wrote:
> On 11/04, Namhyung Kim wrote:
>>
>> On Sat, 2 Nov 2013 16:54:58 +0100, Oleg Nesterov wrote:
>> >
>> > This does not look right to me.
>> >
>> > - get_user_vaddr() is costly, it does vma_interval_tree_foreach() under
>> > ->i_mmap_mutex.
>>
>> Hmm.. yes, I think this is not needed. I guess it should lookup a
>> proper vma in current->mm with mmap_sem read-locked.
>>
>> >
>> > - this only allows to read the data from the same binary.
>>
>> Right. This is also an unnecessary restriction. We should be able to
>> access data in other binary.
>
> Yes... but this needs another discussion. In general, we simply can not
> do this with the suggested syntax.
Agreed.
>
> Say you want to probe this "foo" binary and dump "stdin" from libc.so.
> You can't do this. You simply can't know where libc.so will be mmaped.
>
> But: if we attach the event to the already running process, or if we
> disable the randomization, then we can probably do this, see below.
>
> Or the syntax should be "name=probe @file/addr" or something like this.
Okay. Let's call this kind of thing "cross-fetch" (or a better name can
be suggested). This is more complex situation and needs more discussion
as you said. So let's skip the discussion for now. :)
>
>> > - in particular, you can't read the data from bss
>>
>> I can't understand why.. The bss region should also be in a same vma of
>> normal data, no?
>
> No, no. bss is mmaped anonymously, at least in general. See set_brk() in
> load_elf().
Ah, thanks for the pointer. I also need to say that I'm not familiar
with the code base.
Looking at the code, it seems to add a anon mapping iff the bss region
spans on two or more pages - that's why I missed it from my simple
test. :/
>
>> I thought the gcc somehow aligns data to next page boundary.
>
> And perhaps it even should, my system is old. But this doesn't really
> matter, the process itself can create another mapping.
Right.
>
>> But if
>> it's not the case, we need to recognize which is the proper one..
>>
>> Simply preferring a writable vma to a read-only vma is what's came to my
>> head now. Do you have an idea?
>
> So far I think that trace_uprobes.c should not play games with vma. At all.
Yes, playing with vma is fragile. But otherwise how can we get the
address from the file+offset in random processes?
>
>> > -------------------------------------------------------------------------------
>> > Can't we simply implement get_user_vaddr() as
>> >
>> > static void __user *get_user_vaddr(unsigned long addr, struct
>> > trace_uprobe *tu)
>> > {
>> > void __user *vaddr = (void __force __user *)addr;
>> >
>> > /* A NULL tu means that we already got the vaddr */
>> > if (tu)
>> > vaddr += (current->mm->start_data & PAGE_MASK);
>> >
>> > return vaddr;
>> > }
>> >
>> > ?
>> >
>> > I did this change, and now the test-case above works. And it also works
>> > with "cc -pie -fPIC",
>> >
>> > # nm foo | grep -w global
>> > 0000000000200c9c D global
>> >
>> > # perf probe -x ./foo -a "func var=@0xc9c:u32"
>> > # perf record -e probe_foo:func ./foo
>> > ...
>> > # perf script | tail -1
>> > foo 576 [001] 475.519940: probe_foo:func: (7ffe95ca3814)
>> > var=4321
>> >
>> > What do you think?
>>
>> This can only work with the probes fetching data from the executable,
>> right? But as I said it should support any other binaries too.
>
> See above, we can't in general read other binaries.
Okay, I need to clarify my words. I'm not saying about "cross-fetch"
here, what I wanted to say is adding a probe in some dso and fetch data
from the dso.
Primary usecase I have in mind is supporting SDTs in the perf probe
tool. Currently many libraries including glibc add tracepoints (SDTs)
within themselves to be traced/profilied easily.
You can see Hemant's work on this here:
https://lkml.org/lkml/2013/10/18/274
>
> But: if we know know where it is mmapped we can do this, just we need
> to calculate the right addr passed to trace_uprobes.
>
> Or: we should support both absolute and relative addresses, this is what
> I was going to discuss later.
But I guess this "specifying address directly" is hard to apply to
multiple processes - like system-wide tracing in perf.
>
>> static void __user *get_user_vaddr(unsigned long addr, struct trace_uprobe
>> *tu)
>> {
>> unsigned long pgoff = addr >> PAGE_SHIFT;
>> struct vm_area_struct *vma, *orig_vma = NULL;
>> unsigned long vaddr = 0;
>>
>> if (tu == NULL) {
>> /* A NULL tu means that we already got the vaddr */
>> return (void __force __user *) addr;
>> }
>>
>> down_read(¤t->mm->mmap_sem);
>>
>> vma = current->mm->mmap;
>
> Cough, it can be null if another thread does munmap(0, TASK_SIZE) ;)
>
> But this doesn't matter.
:)
>
>> do {
>> if (!vma->vm_file || vma->vm_file->f_inode != tu->inode) {
>> /*
>> * We found read-only mapping for this inode.
>> * (provided that all mappings for this inode
>> * have consecutive addresses)
>> */
>> if (orig_vma)
>> break;
>> continue;
>> }
>>
>> if (vma->vm_pgoff > pgoff ||
>> (vma->vm_pgoff + vma_pages(vma) <= pgoff))
>> continue;
>>
>> orig_vma = vma;
>>
>> /*
>> * We prefer writable mapping over read-only since
>> * data is usually in read/write memory region. But
>> * in case of read-only data, it only can be found in
>> * read-only mapping so we save orig_vma and check
>> * whether it also has writable mapping.
>> */
>> if (vma->vm_flags & VM_WRITE)
>> break;
>> } while ((vma = vma->vm_next) != NULL);
>>
>> if (orig_vma)
>> vaddr = offset_to_vaddr(orig_vma, addr);
>>
>> up_read(¤t->mm->mmap_sem);
>>
>> return (void __force __user *) vaddr;
>> }
>
> For what? Why it is better then my suggestion?
Just to support fetching (not cross-fetching!) from other binaries
(dsos) other than an executable.
>
> How it can read bss? How it can read the data from other binaries?
Yes, it'd fail if bss resides in a separate vma. :-/
>
> How we can trust the result? This code relies on some guesses and
> none of them are "strict".
>
> If nothing else, elf can have the arbitrary number of mmaped sections,
> this can't work in general?
These two are still problems to be solved.
Thanks,
Namhyung
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
