From: Ananth N Mavinakayanahalli <hidden> Date: 2012-06-08 09:33:17
From: Ananth N Mavinakayanahalli <redacted>
On RISC architectures like powerpc, instructions are fixed size.
Instruction analysis on such platforms is just a matter of (insn % 4).
Pass the vaddr at which the uprobe is to be inserted so that
arch_uprobe_analyze_insn() can flag misaligned registration requests.
Changes in V2:
Pass (unsigned long)addr instead of (loff_t)vaddr to
arch_uprobe_analyze_insn(). We need the loff_t vaddr to take care of
the offset of inode:offset pair for large file sizes on 32bit.
Signed-off-by: Ananth N Mavinakaynahalli <redacted>
---
arch/x86/include/asm/uprobes.h | 2 +-
arch/x86/kernel/uprobes.c | 3 ++-
kernel/events/uprobes.c | 2 +-
3 files changed, 4 insertions(+), 3 deletions(-)
Index: linux-3.5-rc1/arch/x86/include/asm/uprobes.h
===================================================================
From: Ananth N Mavinakayanahalli <hidden> Date: 2012-06-08 09:34:56
From: Ananth N Mavinakayanahalli <redacted>
This is the port of uprobes to powerpc. Usage is similar to x86.
One TODO in this port compared to x86 is the uprobe abort_xol() logic.
x86 depends on the thread_struct.trap_nr (absent in powerpc) to determine
if a signal was caused when the uprobed instruction was single-stepped/
emulated, in which case, we reset the instruction pointer to the probed
address and retry the probe again.
Tested on POWER6; I don't see anything here that should stop it from
working on a ppc32; since I don't have access to a ppc32 machine, it
would be good if somoene could verify that part.
[root@xxxx ~]# ./bin/perf probe -x /lib64/libc.so.6 malloc
Added new event:
probe_libc:malloc (on 0xb4860)
You can now use it in all perf tools, such as:
perf record -e probe_libc:malloc -aR sleep 1
[root@xxxx ~]# ./bin/perf record -e probe_libc:malloc -aR sleep 20
[ perf record: Woken up 22 times to write data ]
[ perf record: Captured and wrote 5.843 MB perf.data (~255302 samples) ]
[root@xxxx ~]# ./bin/perf report --stdio
# ========
# captured on: Mon Jun 4 05:26:31 2012
# hostname : xxxx.ibm.com
# os release : 3.4.0-uprobe
# perf version : 3.4.0
# arch : ppc64
# nrcpus online : 4
# nrcpus avail : 4
# cpudesc : POWER6 (raw), altivec supported
# cpuid : 62,769
# total memory : 7310528 kB
# cmdline : /root/bin/perf record -e probe_libc:malloc -aR sleep 20
# event : name = probe_libc:malloc, type = 2, config = 0x124, config1 = 0x0, con
# HEADER_CPU_TOPOLOGY info available, use -I to display
# HEADER_NUMA_TOPOLOGY info available, use -I to display
# ========
#
# Samples: 83K of event 'probe_libc:malloc'
# Event count (approx.): 83484
#
# Overhead Command Shared Object Symbol
# ........ ............ ............. ..........
#
69.05% tar libc-2.12.so [.] malloc
28.57% rm libc-2.12.so [.] malloc
1.32% avahi-daemon libc-2.12.so [.] malloc
0.58% bash libc-2.12.so [.] malloc
0.28% sshd libc-2.12.so [.] malloc
0.08% irqbalance libc-2.12.so [.] malloc
0.05% bzip2 libc-2.12.so [.] malloc
0.04% sleep libc-2.12.so [.] malloc
0.03% multipathd libc-2.12.so [.] malloc
0.01% sendmail libc-2.12.so [.] malloc
0.01% automount libc-2.12.so [.] malloc
V2:
a. arch_uprobe_analyze_insn() now gets unsigned long addr.
b. Verified that mtmsr[d] and rfi[d] are handled correctly by
emulate_step() (no changes to this patch).
Signed-off-by: Ananth N Mavinakayanahalli <redacted>
---
arch/powerpc/Kconfig | 3
arch/powerpc/include/asm/thread_info.h | 4
arch/powerpc/include/asm/uprobes.h | 49 +++++++++
arch/powerpc/kernel/Makefile | 1
arch/powerpc/kernel/signal.c | 6 +
arch/powerpc/kernel/uprobes.c | 164 +++++++++++++++++++++++++++++++++
6 files changed, 226 insertions(+), 1 deletion(-)
Index: linux-3.5-rc1/arch/powerpc/include/asm/thread_info.h
===================================================================
@@ -0,0 +1,164 @@+/*+*User-spaceProbes(UProbes)forpowerpc+*+*Thisprogramisfreesoftware;youcanredistributeitand/ormodify+*itunderthetermsoftheGNUGeneralPublicLicenseaspublishedby+*theFreeSoftwareFoundation;eitherversion2oftheLicense,or+*(atyouroption)anylaterversion.+*+*Thisprogramisdistributedinthehopethatitwillbeuseful,+*butWITHOUTANYWARRANTY;withouteventheimpliedwarrantyof+*MERCHANTABILITYorFITNESSFORAPARTICULARPURPOSE.Seethe+*GNUGeneralPublicLicenseformoredetails.+*+*YoushouldhavereceivedacopyoftheGNUGeneralPublicLicense+*alongwiththisprogram;ifnot,writetotheFreeSoftware+*Foundation,Inc.,59TemplePlace-Suite330,Boston,MA02111-1307,USA.+*+*Copyright(C)IBMCorporation,2007-2012+*+*Adaptedfromthex86portbyAnanthNMavinakayanahalli<ananth@in.ibm.com>+*/+#include<linux/kernel.h>+#include<linux/sched.h>+#include<linux/ptrace.h>+#include<linux/uprobes.h>+#include<linux/uaccess.h>++#include<linux/kdebug.h>+#include<asm/sstep.h>++/**+*arch_uprobe_analyze_insn+*@mm:theprobedaddressspace.+*@arch_uprobe:theprobepointinformation.+*@addr:vaddrtoprobe.+*Return0onsuccessora-venumberonerror.+*/+intarch_uprobe_analyze_insn(structarch_uprobe*auprobe,structmm_struct*mm,unsignedlongaddr)+{+if(addr&0x03)+return-EINVAL;+return0;+}++/*+*arch_uprobe_pre_xol-preparetoexecuteoutofline.+*@auprobe:theprobepointinformation.+*@regs:reflectsthesaveduserstateofcurrenttask.+*/+intarch_uprobe_pre_xol(structarch_uprobe*auprobe,structpt_regs*regs)+{+/* FIXME: We don't support abort_xol on powerpc for now */+regs->nip=current->utask->xol_vaddr;+return0;+}++/**+*uprobe_get_swbp_addr-computeaddressofswbpgivenpost-swbpregs+*@regs:Reflectsthesavedstateofthetaskafterithashitabreakpoint+*instruction.+*Returntheaddressofthebreakpointinstruction.+*/+unsignedlonguprobe_get_swbp_addr(structpt_regs*regs)+{+returninstruction_pointer(regs);+}++/*+*Ifxolinsnitselftrapsandgeneratesasignal(SIGILL/SIGSEGV/etc),+*thendetectthecasewhereasinglesteppedinstructionjumpsbacktoits+*ownaddress.Itisassumedthatanythinglikedo_page_fault/do_trap/etc+*setsthread.trap_nr!=-1.+*+*FIXME:powerpchoweverdoesn'thavethread.trap_nryet.+*+*arch_uprobe_pre_xol/arch_uprobe_post_xolsave/restorethread.trap_nr,+*arch_uprobe_xol_was_trapped()simplychecksthat->trap_nrisnotequalto+*UPROBE_TRAP_NR==-1setbyarch_uprobe_pre_xol().+*/+boolarch_uprobe_xol_was_trapped(structtask_struct*t)+{+/* FIXME: We don't support abort_xol on powerpc for now */+returnfalse;+}++/*+*Calledaftersingle-stepping.ToavoidtheSMPproblemsthatcan+*occurwhenwetemporarilyputbacktheoriginalopcodeto+*single-step,wesingle-steppedacopyoftheinstruction.+*+*Thisfunctionpreparestoresumeexecutionafterthesingle-step.+*/+intarch_uprobe_post_xol(structarch_uprobe*auprobe,structpt_regs*regs)+{+/* FIXME: We don't support abort_xol on powerpc for now */++/*+*Onpowerpc,exceptforloadsandstores,mostinstructions+*includingonesthataltercodeflow(branches,calls,returns)+*areemulatedinthekernel.Wegethereonlyiftheemulation+*supportdoesn'texistandhavetofix-upthenextinstruction+*tobeexecuted.+*/+regs->nip=current->utask->vaddr+MAX_UINSN_BYTES;+return0;+}++/* callback routine for handling exceptions. */+intarch_uprobe_exception_notify(structnotifier_block*self,unsignedlongval,void*data)+{+structdie_args*args=data;+structpt_regs*regs=args->regs;+intret=NOTIFY_DONE;++/* We are only interested in userspace traps */+if(regs&&!user_mode(regs))+returnNOTIFY_DONE;++switch(val){+caseDIE_BPT:+if(uprobe_pre_sstep_notifier(regs))+ret=NOTIFY_STOP;+break;+caseDIE_SSTEP:+if(uprobe_post_sstep_notifier(regs))+ret=NOTIFY_STOP;+default:+break;+}+returnret;+}++/*+*ThisfunctiongetscalledwhenXOLinstructioneithergetstrappedor+*thethreadhasafatalsignal,soresettheinstructionpointertoits+*probedaddress.+*/+voidarch_uprobe_abort_xol(structarch_uprobe*auprobe,structpt_regs*regs)+{+/* FIXME: We don't support abort_xol on powerpc for now */+return;+}++/*+*Seeiftheinstructioncanbeemulated.+*Returnstrueifinstructionwasemulated,falseotherwise.+*/+boolarch_uprobe_skip_sstep(structarch_uprobe*auprobe,structpt_regs*regs)+{+intret;+unsignedintinsn;++memcpy(&insn,auprobe->insn,MAX_UINSN_BYTES);++/*+*emulate_step()returns1iftheinsnwassuccessfullyemulated.+*Forallothercases,weneedtosingle-stepinhardware.+*/+ret=emulate_step(regs,insn);+if(ret>0)+returntrue;++returnfalse;+}
Ananth, Srikar,
I think the patch is correct and I am sorry for nit-picking,
this is really minor.
But,
On 06/08, Ananth N Mavinakayanahalli wrote:
Changes in V2:
Pass (unsigned long)addr
Well, IMHO, this is confusing.
First of all, why do we have this "addr" or even "vaddr"? It should
not exists. We pass it to copy_insn(), but for what?? copy_insn()
should simply use uprobe->offset, the virtual address for this
particular mapping does not matter at all. I am going to send
the cleanup.
Note also that we should move this !UPROBE_COPY_INSN from
install_breakpoint() to somewhere near alloc_uprobe(). This code
is called only once, it looks a bit strange to use the "random" mm
(the first mm vma_prio_tree_foreach() finds) and its mapping to
verify the insn. In fact this is simply not correct and should be
fixed, note that on x86 arch_uprobe_analyze_insn() checks
mm->context.ia32_compat.
IOW, Perhaps uprobe->offset makes more sense?
Just fyi, this conflicts with
"[PATCH 1/3] uprobes: install_breakpoint() should fail if is_swbp_insn() == T"
I sent, but the conflict is trivial.
Oleg.
Note also that we should move this !UPROBE_COPY_INSN from
install_breakpoint() to somewhere near alloc_uprobe().
The main problem is, uprobe_register() doesn't have struct file
for read_mapping_page().
Stupid question. I'm afraid the answer is "no" but I'll ask anyway.
Is it safe to pass filp == NULL to mapping->readpage()? In fact
I do not understand why it needs "struct file*" and I do not see
any example of actual usage. Yes, I didn't try to grep very much
and I understand that the filesystem can do something special.
Say it can use file->private_data...
However. There is read_cache_page_gfp() which does use
a_ops->readpage(filp => NULL), and the comment says nothing about
the riskiness.
If not, is there any other way
uprobe_register(struct inode *inode, loff_t offset) can read the
page at this offset?
And btw, read_mapping_page() accepts "void *data". Why? it uses
filler == a_ops->readpage, it shouldn't accept anything but file
pointer?
Please help, I know nothing about vfs.
Oleg.
Well, IMHO, this is confusing.
First of all, why do we have this "addr" or even "vaddr"? It should
not exists. We pass it to copy_insn(), but for what?? copy_insn()
should simply use uprobe->offset, the virtual address for this
particular mapping does not matter at all. I am going to send
the cleanup.
Yes, we can use uprobe->offset instead of vaddr/addr.
Note also that we should move this !UPROBE_COPY_INSN from
install_breakpoint() to somewhere near alloc_uprobe(). This code
is called only once, it looks a bit strange to use the "random" mm
(the first mm vma_prio_tree_foreach() finds) and its mapping to
verify the insn. In fact this is simply not correct and should be
fixed, note that on x86 arch_uprobe_analyze_insn() checks
The reason we "delay" the copy_insn to the first insert is because
we have to get access to mm. For archs like x86, we want to know if the
executable is 32 bit or not (since we have a different valid set of
instructions for 32 bit and 64 bit). So in effect, if we get access to
struct file corresponding to the inode and if the inode corresponds to
32 bit executable file or 64 bit executable file during register, then
we can move it around alloc_uprobe().
Note also that we should move this !UPROBE_COPY_INSN from
install_breakpoint() to somewhere near alloc_uprobe(). This code
is called only once, it looks a bit strange to use the "random" mm
(the first mm vma_prio_tree_foreach() finds) and its mapping to
verify the insn. In fact this is simply not correct and should be
fixed, note that on x86 arch_uprobe_analyze_insn() checks
The reason we "delay" the copy_insn to the first insert is because
we have to get access to mm. For archs like x86, we want to know if the
executable is 32 bit or not
Yes. And this is wrong afaics.
Once again. This !UPROBE_COPY_INSN code is called only once, and it
uses the "random" mm. After that install_breakpoint() just calls
set_swbp(another_mm) while the insn can be invalid because
another_mm->ia32_compat != mm->ia32_compat.
So in effect, if we get access to
struct file corresponding to the inode and if the inode corresponds to
32 bit executable file or 64 bit executable file during register, then
we can move it around alloc_uprobe().
I don't think this can work. I have another simple fix in mind, I'll
write another email later.
Oleg.
From: Peter Zijlstra <peterz@infradead.org> Date: 2012-06-13 09:58:35
On Mon, 2012-06-11 at 21:09 +0200, Oleg Nesterov wrote:
Stupid question. I'm afraid the answer is "no" but I'll ask anyway.
Is it safe to pass filp =3D=3D NULL to mapping->readpage()? In fact
I do not understand why it needs "struct file*" and I do not see
any example of actual usage.=20
Looking at afs_readpage it looks like its OK to pass in NULL. Same for
nfs_readpage. They use the file, if provided, to avoid some lookups, but
seem to deal with not having it.
This answer by example is of course not authorative nor complete.
Note also that we should move this !UPROBE_COPY_INSN from
install_breakpoint() to somewhere near alloc_uprobe(). This code
is called only once, it looks a bit strange to use the "random" mm
(the first mm vma_prio_tree_foreach() finds) and its mapping to
verify the insn. In fact this is simply not correct and should be
fixed, note that on x86 arch_uprobe_analyze_insn() checks
The reason we "delay" the copy_insn to the first insert is because
we have to get access to mm. For archs like x86, we want to know if the
executable is 32 bit or not
Yes. And this is wrong afaics.
Once again. This !UPROBE_COPY_INSN code is called only once, and it
uses the "random" mm. After that install_breakpoint() just calls
set_swbp(another_mm) while the insn can be invalid because
another_mm->ia32_compat != mm->ia32_compat.
quoted
So in effect, if we get access to
struct file corresponding to the inode and if the inode corresponds to
32 bit executable file or 64 bit executable file during register, then
we can move it around alloc_uprobe().
I don't think this can work. I have another simple fix in mind, I'll
write another email later.
For example. Suppose there is some instruction in /lib64/libc.so which
is valid for 64-bit, but not for 32-bit.
Suppose that a 32-bit application does mmap("/lib64/libc.so", PROT_EXEC).
Now. If vma_prio_tree_foreach() finds this 32-bit mm first, uprobe_register()
fails even if there are other 64-bit applications which could be traced.
Or. uprobe_register() succeeds because it finds a 64-bit mm first, and
then that 32-bit application actually executes the invalid insn.
We can move arch_uprobe_analyze_insn() outside of !UPROBE_COPY_INSN block.
Or, perhaps, validate_insn_bits() should call both
validate_insn_32bits() and validate_insn_64bits(), and set the
UPROBE_VALID_IF_32 / UPROBE_VALID_IF_64 flags. install_breakpoint()
should do the additinal check before set_swbp() and verify that
.ia32_compat matches UPROBE_VALID_IF_*.
What do you think?
Oleg.
On Mon, 2012-06-11 at 21:09 +0200, Oleg Nesterov wrote:
quoted
Stupid question. I'm afraid the answer is "no" but I'll ask anyway.
Is it safe to pass filp == NULL to mapping->readpage()? In fact
I do not understand why it needs "struct file*" and I do not see
any example of actual usage.
Looking at afs_readpage it looks like its OK to pass in NULL. Same for
nfs_readpage. They use the file, if provided, to avoid some lookups, but
seem to deal with not having it.
Yes, and reiser4 does the same.
This answer by example is of course not authorative nor complete.
Yes... perhaps we should simply change this code to use NULL and
collect the bug-reports ;)
Oleg.
Note also that we should move this !UPROBE_COPY_INSN from
install_breakpoint() to somewhere near alloc_uprobe(). This code
is called only once, it looks a bit strange to use the "random" mm
(the first mm vma_prio_tree_foreach() finds) and its mapping to
verify the insn. In fact this is simply not correct and should be
fixed, note that on x86 arch_uprobe_analyze_insn() checks
The reason we "delay" the copy_insn to the first insert is because
we have to get access to mm. For archs like x86, we want to know if the
executable is 32 bit or not
Yes. And this is wrong afaics.
Once again. This !UPROBE_COPY_INSN code is called only once, and it
uses the "random" mm. After that install_breakpoint() just calls
set_swbp(another_mm) while the insn can be invalid because
another_mm->ia32_compat != mm->ia32_compat.
quoted
So in effect, if we get access to
struct file corresponding to the inode and if the inode corresponds to
32 bit executable file or 64 bit executable file during register, then
we can move it around alloc_uprobe().
I don't think this can work. I have another simple fix in mind, I'll
write another email later.
For example. Suppose there is some instruction in /lib64/libc.so which
is valid for 64-bit, but not for 32-bit.
Suppose that a 32-bit application does mmap("/lib64/libc.so", PROT_EXEC).
How correct is it to have a 32 bit binary link to a 64 bit binary/library?
what if the 64 bit binary/executable were to make a jump to a 64 bit
address?
Now. If vma_prio_tree_foreach() finds this 32-bit mm first, uprobe_register()
fails even if there are other 64-bit applications which could be traced.
Or. uprobe_register() succeeds because it finds a 64-bit mm first, and
then that 32-bit application actually executes the invalid insn.
We can move arch_uprobe_analyze_insn() outside of !UPROBE_COPY_INSN block.
Or, perhaps, validate_insn_bits() should call both
validate_insn_32bits() and validate_insn_64bits(), and set the
UPROBE_VALID_IF_32 / UPROBE_VALID_IF_64 flags. install_breakpoint()
should do the additinal check before set_swbp() and verify that
.ia32_compat matches UPROBE_VALID_IF_*.
What do you think?
Lets say we do find a 32 bit app and 64 bit app using the same library
and the underlying instruction is valid for tracing in 64 bit and not 32
bit. So when we are registering, and failed to insert a breakpoint for
the 32 bit app, should we just bail out or should we return a failure?
I would probably prefer to read the underlying file something similar to
what exec does and based on the magic decipher if we should verify for
32 bit instructions or 64 bit instructions.
I didnt find a good way to read the first few bytes just by looking at
the inode. So one option is to read the underlying file at the first
insertion (i.e similar to what we do now .. but instead of depending on
ia32_compat of a random mm, depend on the exec magic)
Better option would be read the underlying file at the register time and
return an error even without attempting an insert if the instruction
wasnt valid. But I am still ignorant on how to do this because we need a
struct file to do this.
--
Thanks and Regards
Srikar
For example. Suppose there is some instruction in /lib64/libc.so which
is valid for 64-bit, but not for 32-bit.
Suppose that a 32-bit application does mmap("/lib64/libc.so", PROT_EXEC).
How correct is it to have a 32 bit binary link to a 64 bit binary/library?
No, I didn't mean this. I guess you misunderstood my point, see below.
quoted
Now. If vma_prio_tree_foreach() finds this 32-bit mm first, uprobe_register()
fails even if there are other 64-bit applications which could be traced.
Or. uprobe_register() succeeds because it finds a 64-bit mm first, and
then that 32-bit application actually executes the invalid insn.
We can move arch_uprobe_analyze_insn() outside of !UPROBE_COPY_INSN block.
Or, perhaps, validate_insn_bits() should call both
validate_insn_32bits() and validate_insn_64bits(), and set the
UPROBE_VALID_IF_32 / UPROBE_VALID_IF_64 flags. install_breakpoint()
should do the additinal check before set_swbp() and verify that
.ia32_compat matches UPROBE_VALID_IF_*.
quoted
What do you think?
Lets say we do find a 32 bit app and 64 bit app using the same library
and the underlying instruction is valid for tracing in 64 bit and not 32
bit. So when we are registering, and failed to insert a breakpoint for
the 32 bit app, should we just bail out or should we return a failure?
I do not really know, I tend to think we should not fail. But this is
another story...
Look. Suppose that a 32-bit app starts after uprobe_register() succeeds.
In this case we have no option, uprobe_mmap()->install_breakpoint()
should "silently" fail. Currently it doesn't, this is one of the reasons
why I think the validation logic is wrong.
And. if install_breakpoint() can fail later anyway (in this case), then
I think uprobe_register() should not fail.
But probably this needs more discussion.
I would probably prefer to read the underlying file something similar to
what exec does and based on the magic decipher if we should verify for
32 bit instructions or 64 bit instructions.
But this can't protect from the malicious user who does
mmap(64-bit-code, PROT_EXEC) from a 32-bit app, and this can confuse
uprobes even if that 32-bit app never tries to actually execute that
64-bit-code.
That is why I think we need the additional (and arch-dependant) check
before every set_swbp(), but arch_uprobe_analyze_insn/etc should not
depend on task/mm/vaddr/whatever.
Oleg.