FInal kprobes rollup patches

From f87fb3155500f85ffbafc01d6f5b8dc2a3f6184c Mon Sep 17 00:00:00 2001

From 4a048bf0c67558b8170dcde7c8395f6c35592f0b Mon Sep 17 00:00:00 2001

From 83fde2864abcc34ef8bc575a22b5f8e76477b64b Mon Sep 17 00:00:00 2001

From 323525be495dc545158fe43e072523d994253c39 Mon Sep 17 00:00:00 2001

From e7fd0efa2e7860012d0a625c1fda3174ae200dc5 Mon Sep 17 00:00:00 2001

From 7fe6bd404ae0c51ec0bd334ca28a1f17f247a099 Mon Sep 17 00:00:00 2001

Further unification work. There is a possible behavior change on X86_32 here.

is_IF_modifier(p->opcode)

to

is_IF_modifier(p->ainsn.insn)

Which should be equivalent, but is not purely cosmetic as the rest of the unification so far.

Quoted 7 lines · Harvey Harrison

Further unification work. There is a possible behavior change on X86_32 here.

is_IF_modifier(p->opcode)

to

Quoted 4 lines

is_IF_modifier(p->ainsn.insn)

Which should be equivalent, but is not purely cosmetic as the rest of the unification so far.

hm, could you split this into two, the pure-equivalence and the possibly-modifying patch? (that way any potential breakage becomes bisectable) Same end result, just two commits instead of one.

also, did you manage to run/test kprobes (on 32-bit or 64-bit x86), and it worked fine?

If you compare this memcpy from arch_prepare_kprobe in 32/64 bit I'm almost sure the X86_32 version should be

... + sizeof(kprobe_opcode_t)

not

... * sizeof(kprobe_opcode_t)

Quoted 8 lines · Harvey Harrison

If you compare this memcpy from arch_prepare_kprobe in 32/64 bit I'm almost sure the X86_32 version should be

... + sizeof(kprobe_opcode_t)

not

... * sizeof(kprobe_opcode_t)

good point. I've Cc:-ed the top authors of kprobes.c. Could anyone confirm (or deny) that Harvey found a real kprobes bug here?

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Hi Harvey and Ingo,

I'm working on another version of patches for unification. Currently cleaning up the patches.
http://sources.redhat.com/ml/systemtap/2007-q4/msg00457.html
I'll cleanup and repost it today.

Masami,

Quoted 2 lines · Masami Hiramatsu

Hi Harvey and Ingo,

Quoted 4 lines

I'm working on another version of patches for unification. Currently cleaning up the patches.
http://sources.redhat.com/ml/systemtap/2007-q4/msg00457.html
I'll cleanup and repost it today.

cool! Please Cc: lkml and Harvey as well so that there's less overlap in unification work - Harvey spent quite some time unifying and cleaning up the kprobes code during the past week.

So i think we could/should use Harvey's latest series as a base, those are pretty finegrained already. Note that they break 64-bit kprobes though, with such a config:

CONFIG_KPROBES=y
CONFIG_NET_TCPPROBE=y

so it crashes with an int3 in the TCP code. It's probably some trivial typo somewhere, as 32-bit works fine.

the coordinates for x86.git#mm can be found below - that tree already includes Harvey's latest kprobes series. I'll try to bisect the 64-bit breakage now.

Ingo

------------------>
git-clone git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git linux-2.6.git cd linux-2.6.git
git-branch x86
git-checkout x86
git-pull git://git.kernel.org/pub/scm/linux/kernel/git/x86/linux-2.6-x86.git mm

(do subsequent pulls via "git-pull --force", as we frequently rebase the git tree. NOTE: this might override your own local changes, so do this only if you dont mind about losing thse changes in that tree.)

Quoted 3 lines

cool! Please Cc: lkml and Harvey as well so that there's less overlap in unification work - Harvey spent quite some time unifying and cleaning up the kprobes code during the past week.

Should I rewrite it based on current git tree?
My patch includes 3 part of patches.
- 2 Bugfix patches (which is not merged yet.)
- 2 booster patches (ditto)
- 2 unification patches (most of this patches are already done by Harvey's patch)

Quoted 8 lines · Masami Hiramatsu

Quoted 3 lines

cool! Please Cc: lkml and Harvey as well so that there's less overlap in unification work - Harvey spent quite some time unifying and cleaning up the kprobes code during the past week.

Should I rewrite it based on current git tree?
My patch includes 3 part of patches.
- 2 Bugfix patches (which is not merged yet.)
- 2 booster patches (ditto)
- 2 unification patches (most of this patches are already done by Harvey's patch)

would it be easier/more robust to first did the unification patches and then get the bugfixes and new features in? That would give us your bugfixes and new features on both 32-bit and 64-bit at the same time.

feel free to do whichever approach you prefer - but it would be nice to preserve the unification and cleanup work done by Harvey.

btw., is any of your bugfixes 2.6.24 material?

Ingo

Quoted 9 lines · Masami Hiramatsu

Quoted 3 lines

cool! Please Cc: lkml and Harvey as well so that there's less overlap in unification work - Harvey spent quite some time unifying and cleaning up the kprobes code during the past week.

Should I rewrite it based on current git tree?
My patch includes 3 part of patches.
- 2 Bugfix patches (which is not merged yet.)
- 2 booster patches (ditto)
- 2 unification patches (most of this patches are already done by Harvey's patch)

would it be easier/more robust to first did the unification patches and then get the bugfixes and new features in? That would give us your bugfixes and new features on both 32-bit and 64-bit at the same time.

feel free to do whichever approach you prefer - but it would be nice to preserve the unification and cleanup work done by Harvey.

btw., is any of your bugfixes 2.6.24 material?

Quoted 9 lines

If you compare this memcpy from arch_prepare_kprobe in 32/64 bit I'm almost sure the X86_32 version should be

... + sizeof(kprobe_opcode_t)

not

... * sizeof(kprobe_opcode_t)

good point. I've Cc:-ed the top authors of kprobes.c. Could anyone confirm (or deny) that Harvey found a real kprobes bug here?

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

* Ingo Molnar <[email protected]> [2007-12-15 14:12:04]:

Hi Ingo, Harvey

In file include/asm-x86/kprobes_32.h
typedef u8 kprobe_opcode_t;
hence sizeof(kprobe_opcode_t) turns out to be 1.

Hence

memcpy(p->ainsn.insn, p->addr, MAX_INSN_SIZE * sizeof(kprobe_opcode_t)); is correct.

Quoted 5 lines · Srikar Dronamraju

* Ingo Molnar <[email protected]> [2007-12-15 14:12:04]:

Hi Ingo, Harvey

Quoted 9 lines

In file include/asm-x86/kprobes_32.h
typedef u8 kprobe_opcode_t;
hence sizeof(kprobe_opcode_t) turns out to be 1.

Hence

memcpy(p->ainsn.insn, p->addr, MAX_INSN_SIZE * sizeof(kprobe_opcode_t)); is correct.

OK, but this would be much clearer to adopt the X86_64 way, define MAX_INSN_SIZE one smaller and make this line:

/* Copy original instruction plus space for 1 byte relative jump */ memcpy(p->ainsn.insn, p->addr, MAX_INSN_SIZE + sizeof(kprobe_opcode_t));

See the first patch of my cleanup work that unified MAX_INSN_SIZE and you'll see why this jumped out.

Harvey

Hi Harvey,
If you mention about a relative jump which is inserted by resume_execution(), I think you might misunderstand that relative jump.

The size of that relative jump, which will be embedded by kprobe-booster, is 5-bytes(not 1 byte). So it needs 5 bytes space.
And we decided not to expand MAX_INSN_SIZE when we developed the booster. The reasons are:
- it is supplemental feature(just accelerating kprobes), if we have no space, we can disable it.
- 5 bytes are big enough compared with 15(=MAX_INSN_SIZE)
- the lengths of most of instructions are less than 10 bytes.

Additionally, MAX_INSN_SIZE is used in kernel/kprobes.c to allocate an instruction buffer which will be assigned to p->ainsn.insn. Since the instruction buffer size is MAX_INSN_SIZE, you can not copy instructions more than MAX_INSN_SIZE.

BTW, in my patch, I unified MAX_INSN_SIZE to bigger one(16). I think it is enough for us.

Quoted 18 lines · Masami Hiramatsu

Hi Harvey,
If you mention about a relative jump which is inserted by resume_execution(), I think you might misunderstand that relative jump.

The size of that relative jump, which will be embedded by kprobe-booster, is 5-bytes(not 1 byte). So it needs 5 bytes space.
And we decided not to expand MAX_INSN_SIZE when we developed the booster. The reasons are:
- it is supplemental feature(just accelerating kprobes), if we have no space, we can disable it.
- 5 bytes are big enough compared with 15(=MAX_INSN_SIZE)
- the lengths of most of instructions are less than 10 bytes.

Additionally, MAX_INSN_SIZE is used in kernel/kprobes.c to allocate an instruction buffer which will be assigned to p->ainsn.insn. Since the instruction buffer size is MAX_INSN_SIZE, you can not copy instructions more than MAX_INSN_SIZE.

Quoted 3 lines

BTW, in my patch, I unified MAX_INSN_SIZE to bigger one(16). I think it is enough for us.

I went with 15 in mine, I thought it made the code a little more readable, but I will defer if you think 16 is better. If you want me to send the whole series to you, let me know.

I just sent out a series of 4 patches equivalent to your patches 1-4/6 but based on my already unified kprobes.c/h, You may want to check your handling of restored registers in trampoline_probe_handler which I found when rebasing yours on top of my cleanups. Not sure if this is important, but it was a difference I found.

X86_32:
regs->cs = __KERNEL_CS | get_kernel_rpl();

yours:
regs->cs = __KERNEL_CS;

Hi Harvey,
Before porting, could you tell me what differences are important to you? We can discuss about it.

Quoted 6 lines

I just sent out a series of 4 patches equivalent to your patches 1-4/6 but based on my already unified kprobes.c/h, You may want to check your handling of restored registers in trampoline_probe_handler which I found when rebasing yours on top of my cleanups. Not sure if this is important, but it was a difference I found.

Quoted 5 lines

X86_32:
regs->cs = __KERNEL_CS | get_kernel_rpl();

yours:
regs->cs = __KERNEL_CS;

Because of kretprobe's compatibility, on x86-32 cs should be set rpl(). But get_kernel_rpl() does not exist on x86-64.

Quoted 10 lines · Masami Hiramatsu

Hi Harvey,
Before porting, could you tell me what differences are important to you? We can discuss about it.

Quoted 6 lines

I just sent out a series of 4 patches equivalent to your patches 1-4/6 but based on my already unified kprobes.c/h, You may want to check your handling of restored registers in trampoline_probe_handler which I found when rebasing yours on top of my cleanups. Not sure if this is important, but it was a difference I found.

Quoted 8 lines

Quoted 5 lines

X86_32:
regs->cs = __KERNEL_CS | get_kernel_rpl();

yours:
regs->cs = __KERNEL_CS;

Because of kretprobe's compatibility, on x86-32 cs should be set rpl(). But get_kernel_rpl() does not exist on x86-64.

I've already ported it and sent it to you. It's not really important to me I just think my fine-grained patches may be of some use to see where the differences between X86_32/64 ended up being. Your patches end up being just about entirely removal of ifdefs when rebased onto my patches, so it's at least a good secondary check of your patches even if mine don't go in. Your patches end up being much smaller against my version too.

I like my version slightly better because the remaining ifdefs (wrmsr, etc) and others could be done in a few more small patches that are more easily reviewable than your large final unification patch.

But, you know the code better than I....

Harvey

Hi Harvey,

Quoted 5 lines · Harvey Harrison

Quoted 3 lines · Masami Hiramatsu

Hi Harvey,
Before porting, could you tell me what differences are important to you? We can discuss about it.

Quoted 7 lines

I've already ported it and sent it to you. It's not really important to me I just think my fine-grained patches may be of some use to see where the differences between X86_32/64 ended up being. Your patches end up being just about entirely removal of ifdefs when rebased onto my patches, so it's at least a good secondary check of your patches even if mine don't go in. Your patches end up being much smaller against my version too.

OK, I'll review that.

Quoted 1 line

Quoted 3 lines

I like my version slightly better because the remaining ifdefs (wrmsr, etc) and others could be done in a few more small patches that are more easily reviewable than your large final unification patch.

I agreed that your patches are including some goodness. So let us merge it into one.

Quoted 19 lines · Masami Hiramatsu

Hi Harvey,

Quoted 11 lines · Harvey Harrison

Quoted 3 lines · Masami Hiramatsu

Hi Harvey,
Before porting, could you tell me what differences are important to you? We can discuss about it.

I've already ported it and sent it to you. It's not really important to me I just think my fine-grained patches may be of some use to see where the differences between X86_32/64 ended up being. Your patches end up being just about entirely removal of ifdefs when rebased onto my patches, so it's at least a good secondary check of your patches even if mine don't go in. Your patches end up being much smaller against my version too.

OK, I'll review that.

Quoted 3 lines

I like my version slightly better because the remaining ifdefs (wrmsr, etc) and others could be done in a few more small patches that are more easily reviewable than your large final unification patch.

Quoted 4 lines

I agreed that your patches are including some goodness. So let us merge it into one.

OK, I'll take the last bits of your patches 5/6 that aren't already cleaned up and send out a unified patchset for you to add your acked/signed off by/reviewed by as appropriate.

These are:

(quoted diff) +0 -2

-add stack_addr() macro
-I prefer the table defintion macros in mine as it avoids the need to

cast the pointer passed to test_bit, but if you want them to be u32 as in your patch, I can change it.

(quoted diff) +0 -3

-wrmsr/wrmsrl - use wrmsr() for both
-call is_IF_modifier with p->ainsn.insn in both
-check casting of jprobe_saved_sp, I get some compile warnings currently

with pointer comparisons to signed/unsigned types.

That will eliminate nearly all of the remaining ifdefs in my version, let me work through this and I'll send out a set for review.

CHeers,

Harvey

Quoted 6 lines

These are:

(quoted diff) +0 -2

-add stack_addr() macro
-I prefer the table defintion macros in mine as it avoids the need to

cast the pointer passed to test_bit, but if you want them to be u32 as in your patch, I can change it.

please do so. we'd like to reduce ifdefs as less as possible:-)

Quoted 4 lines

(quoted diff) +0 -3

-wrmsr/wrmsrl - use wrmsr() for both
-call is_IF_modifier with p->ainsn.insn in both
-check casting of jprobe_saved_sp, I get some compile warnings currently

with pointer comparisons to signed/unsigned types.

Could you also add below?
- fix some comments (it clarifies the meanings of the code)
- add fix_riprel(). this useful to reduce ifdefs.
- expand reenter_kprobe(). I think it treat above two blocks.
- reassignment of regs->ip in kprobe_handler can be unified to "regs->ip = (unsigned long)addr;"

Quoted 4 lines · Masami Hiramatsu

Review

Quoted 4 lines

These are:

(quoted diff) +0 -2

-add stack_addr() macro
-I prefer the table defintion macros in mine as it avoids the need

to

Quoted 8 lines

Quoted 2 lines

cast the pointer passed to test_bit, but if you want them to be u32 as in your patch, I can change it.

please do so. we'd like to reduce ifdefs as less as possible:-)

Review

Quoted 3 lines

(quoted diff) +0 -3

-wrmsr/wrmsrl - use wrmsr() for both
-call is_IF_modifier with p->ainsn.insn in both
-check casting of jprobe_saved_sp, I get some compile warnings

currently

Quoted 8 lines

Quoted 1 line

with pointer comparisons to signed/unsigned types.

Could you also add below?
- fix some comments (it clarifies the meanings of the code)
- add fix_riprel(). this useful to reduce ifdefs.
- expand reenter_kprobe(). I think it treat above two blocks.
- reassignment of regs->ip in kprobe_handler can be unified to "regs->ip = (unsigned long)addr;"

Oh, I forgot to point out important thing.
- please make bugfix patches first. I think my bugfix patches need to go upstream before unification. It would cause some crashes.

Quoted 9 lines

If you compare this memcpy from arch_prepare_kprobe in 32/64 bit I'm almost sure the X86_32 version should be

... + sizeof(kprobe_opcode_t)

not

... * sizeof(kprobe_opcode_t)

good point. I've Cc:-ed the top authors of kprobes.c. Could anyone confirm (or deny) that Harvey found a real kprobes bug here?

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/