| Message ID | 1381871068-27660-11-git-send-email-dave.long@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2013-10-15 at 17:04 -0400, David Long wrote: > From: "David A. Long" <dave.long@linaro.org> > > Instead of depending on include/asm/kprobes.h to provide a dummy definition > for struct arch_specific_insn, do so in include/linux/kprobes.h. That change description doesn't quite seem to quite make sense to me. Anyway, what we're trying to do with this patch is to allow us to use arch_specific_insn for purposes additional to implementing kprobes. This patch enables that but I'm wary that the kprobes code assumes that ainsn is a struct arch_specific_insn, e.g. in linux/kernel/kprobes.c we have: memcpy(&p->ainsn, &ap->ainsn, sizeof(struct arch_specific_insn)); Now, that code isn't compiled when kprobes isn't configured, but it seams to me to be safer if that was also changed to memcpy(&p->ainsn, &ap->ainsn, sizeof(p->ainsn)); However, I also wonder if we should instead leave arch_specific_insn as a kprobes specific structure and on ARM define it in terms of a new more generic 'struct probe_insn'? The drawback with that is that we'd probably end up with a struct just containing a single member which seems a bit redundant: struct arch_specific_insn { struct probe_insn pinsn; }; Thought's anyone? > > Signed-off-by: David A. Long <dave.long@linaro.org> > --- > include/linux/kprobes.h | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h > index 925eaf2..4b5a74d 100644 > --- a/include/linux/kprobes.h > +++ b/include/linux/kprobes.h > @@ -52,9 +52,6 @@ > > #else /* CONFIG_KPROBES */ > typedef int kprobe_opcode_t; > -struct arch_specific_insn { > - int dummy; > -}; > #endif /* CONFIG_KPROBES */ > > struct kprobe; > @@ -110,7 +107,11 @@ struct kprobe { > kprobe_opcode_t opcode; > > /* copy of the original instruction */ > +#ifdef CONFIG_KPROBES > struct arch_specific_insn ainsn; > +#else > + int ainsn; > +#endif > > /* > * Indicates various status flags.
(2013/10/16 6:04), David Long wrote: > From: "David A. Long" <dave.long@linaro.org> > > Instead of depending on include/asm/kprobes.h to provide a dummy definition > for struct arch_specific_insn, do so in include/linux/kprobes.h. Nak, why do we need ugly #ifdef again there? And also, please cc to the original maintainers. Thank you, > > Signed-off-by: David A. Long <dave.long@linaro.org> > --- > include/linux/kprobes.h | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h > index 925eaf2..4b5a74d 100644 > --- a/include/linux/kprobes.h > +++ b/include/linux/kprobes.h > @@ -52,9 +52,6 @@ > > #else /* CONFIG_KPROBES */ > typedef int kprobe_opcode_t; > -struct arch_specific_insn { > - int dummy; > -}; > #endif /* CONFIG_KPROBES */ > > struct kprobe; > @@ -110,7 +107,11 @@ struct kprobe { > kprobe_opcode_t opcode; > > /* copy of the original instruction */ > +#ifdef CONFIG_KPROBES > struct arch_specific_insn ainsn; > +#else > + int ainsn; > +#endif > > /* > * Indicates various status flags. >
(2013/11/14 2:13), Jon Medhurst (Tixy) wrote: > On Tue, 2013-10-15 at 17:04 -0400, David Long wrote: >> From: "David A. Long" <dave.long@linaro.org> >> >> Instead of depending on include/asm/kprobes.h to provide a dummy definition >> for struct arch_specific_insn, do so in include/linux/kprobes.h. > > That change description doesn't quite seem to quite make sense to me. > > Anyway, what we're trying to do with this patch is to allow us to use > arch_specific_insn for purposes additional to implementing kprobes. This > patch enables that but I'm wary that the kprobes code assumes that ainsn > is a struct arch_specific_insn, e.g. in linux/kernel/kprobes.c we have: > > memcpy(&p->ainsn, &ap->ainsn, sizeof(struct arch_specific_insn)); > > Now, that code isn't compiled when kprobes isn't configured, but it > seams to me to be safer if that was also changed to > > memcpy(&p->ainsn, &ap->ainsn, sizeof(p->ainsn)); This kind of cleanup looks good for me, but I don't agree to change the type of the member (removing is OK) by Kconfig. If you want to change the framework of kprobes and uprobes itself (unification), I'm appreciate to discuss with you and uprobes people, because it will involve all arch dependent code change, *NOT ONLY* the ARM issue. > However, I also wonder if we should instead leave arch_specific_insn as > a kprobes specific structure and on ARM define it in terms of a new more > generic 'struct probe_insn'? The drawback with that is that we'd > probably end up with a struct just containing a single member which > seems a bit redundant: > > struct arch_specific_insn { > struct probe_insn pinsn; > }; I also disagree it. If you have a plan to integrate uprobes and kprobes arch specific code, please share it with us. I'm happy to work with you. There are already many maintainers on each feature who is responsible for it (even it is a piece of code), and scripts/get_maintainers.pl gives you who are. Srikar, Oleg, I think it's a good time to merge such arch_specific mechanism of uprobes and kprobes. Would you think we can do similar thing on x86 too? Thank you,
On Thu, 2013-11-14 at 11:02 +0900, Masami Hiramatsu wrote: > (2013/11/14 2:13), Jon Medhurst (Tixy) wrote: > > On Tue, 2013-10-15 at 17:04 -0400, David Long wrote: > >> From: "David A. Long" <dave.long@linaro.org> > >> > >> Instead of depending on include/asm/kprobes.h to provide a dummy definition > >> for struct arch_specific_insn, do so in include/linux/kprobes.h. > > > > That change description doesn't quite seem to quite make sense to me. > > > > Anyway, what we're trying to do with this patch is to allow us to use > > arch_specific_insn for purposes additional to implementing kprobes. This > > patch enables that but I'm wary that the kprobes code assumes that ainsn > > is a struct arch_specific_insn, e.g. in linux/kernel/kprobes.c we have: > > > > memcpy(&p->ainsn, &ap->ainsn, sizeof(struct arch_specific_insn)); > > > > Now, that code isn't compiled when kprobes isn't configured, but it > > seams to me to be safer if that was also changed to > > > > memcpy(&p->ainsn, &ap->ainsn, sizeof(p->ainsn)); > > This kind of cleanup looks good for me, but I don't agree to change > the type of the member (removing is OK) by Kconfig. Wouldn't that still require an #ifdef CONFIG_KPROBES around ainsn? Admittedly a less ugly one than one to change its type to an int. > If you want to > change the framework of kprobes and uprobes itself (unification), > I'm appreciate to discuss with you and uprobes people, because it > will involve all arch dependent code change, *NOT ONLY* the ARM issue. Well, I don't think the goal wasn't unification as such. For kprobes on ARM we have to decode and simulate pretty much the entire instruction set(s) and the attempt to implement uprobes on ARM have tried to make use of as much of that as possible. The tricky bit has been as to where to try and draw the level of abstraction, and it seems this may well be leaking out of the arch specific arena. Bit of background, Dave Long has been working on ARM uprobes based on Rabin Vincent's earlier work, and I, as author of a large part of the current ARM kprobes code, have been reviewing (not very satisfactorily I admit) the bits that impact that. One of my motivations has been to push the kprobes instruction decoding to be more generic, rather than special casing things to cope with uprobes. This is because I'm aware of the reoccurring theme on the ARM lists that it would be good to not have all the different methods of instruction decoding, for probes, debug and simulation, etc. (I'm sceptical that a one-size-fits-all is possible, but consolidation where practical is good). > > > However, I also wonder if we should instead leave arch_specific_insn as > > a kprobes specific structure and on ARM define it in terms of a new more > > generic 'struct probe_insn'? The drawback with that is that we'd > > probably end up with a struct just containing a single member which > > seems a bit redundant: > > > > struct arch_specific_insn { > > struct probe_insn pinsn; > > }; > > I also disagree it. If you have a plan to integrate uprobes and kprobes > arch specific code, please share it with us. There's not really a 'plan', just an attempt to reuse the instruction decoding code used by kprobes in the implementation of uprobes, i.e. the patch series [1] which this mail thread is in reply to. [1] http://thread.gmane.org/gmane.linux.kernel/1579985 > I'm happy to work with you. > There are already many maintainers on each feature who is responsible for > it (even it is a piece of code), and scripts/get_maintainers.pl gives you > who are. > > > Srikar, Oleg, I think it's a good time to merge such arch_specific mechanism > of uprobes and kprobes. Would you think we can do similar thing on x86 too?
On 11/14/13 09:15, Jon Medhurst (Tixy) wrote: > On Thu, 2013-11-14 at 11:02 +0900, Masami Hiramatsu wrote: >> (2013/11/14 2:13), Jon Medhurst (Tixy) wrote: >>> On Tue, 2013-10-15 at 17:04 -0400, David Long wrote: >>>> From: "David A. Long" <dave.long@linaro.org> >>>> >>>> Instead of depending on include/asm/kprobes.h to provide a dummy definition >>>> for struct arch_specific_insn, do so in include/linux/kprobes.h. >>> >>> That change description doesn't quite seem to quite make sense to me. >>> >>> Anyway, what we're trying to do with this patch is to allow us to use >>> arch_specific_insn for purposes additional to implementing kprobes. This >>> patch enables that but I'm wary that the kprobes code assumes that ainsn >>> is a struct arch_specific_insn, e.g. in linux/kernel/kprobes.c we have: >>> >>> memcpy(&p->ainsn, &ap->ainsn, sizeof(struct arch_specific_insn)); >>> >>> Now, that code isn't compiled when kprobes isn't configured, but it >>> seams to me to be safer if that was also changed to >>> >>> memcpy(&p->ainsn, &ap->ainsn, sizeof(p->ainsn)); >> That does look like an important improvement. >> This kind of cleanup looks good for me, but I don't agree to change >> the type of the member (removing is OK) by Kconfig. > > Wouldn't that still require an #ifdef CONFIG_KPROBES around ainsn? > Admittedly a less ugly one than one to change its type to an int. > It is also possible to make the include of asm/kprobes.h unconditional, although that might only cause the #ifdef to appear in more than one include file. >> If you want to >> change the framework of kprobes and uprobes itself (unification), >> I'm appreciate to discuss with you and uprobes people, because it >> will involve all arch dependent code change, *NOT ONLY* the ARM issue. > > Well, I don't think the goal wasn't unification as such. For kprobes on > ARM we have to decode and simulate pretty much the entire instruction > set(s) and the attempt to implement uprobes on ARM have tried to make > use of as much of that as possible. The tricky bit has been as to where > to try and draw the level of abstraction, and it seems this may well be > leaking out of the arch specific arena. > > Bit of background, Dave Long has been working on ARM uprobes based on > Rabin Vincent's earlier work, and I, as author of a large part of the > current ARM kprobes code, have been reviewing (not very satisfactorily I > admit) the bits that impact that. One of my motivations has been to push > the kprobes instruction decoding to be more generic, rather than special > casing things to cope with uprobes. This is because I'm aware of the > reoccurring theme on the ARM lists that it would be good to not have all > the different methods of instruction decoding, for probes, debug and > simulation, etc. (I'm sceptical that a one-size-fits-all is possible, > but consolidation where practical is good). > >> >>> However, I also wonder if we should instead leave arch_specific_insn as >>> a kprobes specific structure and on ARM define it in terms of a new more >>> generic 'struct probe_insn'? The drawback with that is that we'd >>> probably end up with a struct just containing a single member which >>> seems a bit redundant: >>> >>> struct arch_specific_insn { >>> struct probe_insn pinsn; >>> }; >> >> I also disagree it. If you have a plan to integrate uprobes and kprobes >> arch specific code, please share it with us. > > There's not really a 'plan', just an attempt to reuse the instruction > decoding code used by kprobes in the implementation of uprobes, i.e. the > patch series [1] which this mail thread is in reply to. > > [1] http://thread.gmane.org/gmane.linux.kernel/1579985 > >> I'm happy to work with you. >> There are already many maintainers on each feature who is responsible for >> it (even it is a piece of code), and scripts/get_maintainers.pl gives you >> who are. >> Sorry, that would indeed have told me that you were responsible for include/linux/kprobes.h. >> >> Srikar, Oleg, I think it's a good time to merge such arch_specific mechanism >> of uprobes and kprobes. Would you think we can do similar thing on x86 too? > I welcome input from people who have experience in this area. I have no desire to complicate efforts that might be attempted on other architectures, but the existing code was unique to ARM and the goal here was just to share it on ARM in implementing the currently non-existant ARM uprobes feature. It seems to me the leakage of these changes into generic kprobes code is exceedingly small, and unlikely to be a hinderance to any future work (if any is even needed or planned) supporting uprobes or kprobes on other architectures. In the meantime we have ARM users who have been asking for uprobes support for a while and this is, IMHO, a fairly clean approach to providing it. -dl
(2013/11/14 23:15), Jon Medhurst (Tixy) wrote: > On Thu, 2013-11-14 at 11:02 +0900, Masami Hiramatsu wrote: >> (2013/11/14 2:13), Jon Medhurst (Tixy) wrote: >>> On Tue, 2013-10-15 at 17:04 -0400, David Long wrote: >>>> From: "David A. Long" <dave.long@linaro.org> >>>> >>>> Instead of depending on include/asm/kprobes.h to provide a dummy definition >>>> for struct arch_specific_insn, do so in include/linux/kprobes.h. >>> >>> That change description doesn't quite seem to quite make sense to me. >>> >>> Anyway, what we're trying to do with this patch is to allow us to use >>> arch_specific_insn for purposes additional to implementing kprobes. This >>> patch enables that but I'm wary that the kprobes code assumes that ainsn >>> is a struct arch_specific_insn, e.g. in linux/kernel/kprobes.c we have: >>> >>> memcpy(&p->ainsn, &ap->ainsn, sizeof(struct arch_specific_insn)); >>> >>> Now, that code isn't compiled when kprobes isn't configured, but it >>> seams to me to be safer if that was also changed to >>> >>> memcpy(&p->ainsn, &ap->ainsn, sizeof(p->ainsn)); >> >> This kind of cleanup looks good for me, but I don't agree to change >> the type of the member (removing is OK) by Kconfig. > > Wouldn't that still require an #ifdef CONFIG_KPROBES around ainsn? > Admittedly a less ugly one than one to change its type to an int. Yeah, that's the point. > >> If you want to >> change the framework of kprobes and uprobes itself (unification), >> I'm appreciate to discuss with you and uprobes people, because it >> will involve all arch dependent code change, *NOT ONLY* the ARM issue. > > Well, I don't think the goal wasn't unification as such. For kprobes on > ARM we have to decode and simulate pretty much the entire instruction > set(s) and the attempt to implement uprobes on ARM have tried to make > use of as much of that as possible. The tricky bit has been as to where > to try and draw the level of abstraction, and it seems this may well be > leaking out of the arch specific arena. I see, I've heard that from Sandeepa who are working on arm64 kprobes. His patch series now has generic interface of decoder/simulator. > Bit of background, Dave Long has been working on ARM uprobes based on > Rabin Vincent's earlier work, and I, as author of a large part of the > current ARM kprobes code, have been reviewing (not very satisfactorily I > admit) the bits that impact that. One of my motivations has been to push > the kprobes instruction decoding to be more generic, rather than special > casing things to cope with uprobes. This is because I'm aware of the > reoccurring theme on the ARM lists that it would be good to not have all > the different methods of instruction decoding, for probes, debug and > simulation, etc. (I'm sceptical that a one-size-fits-all is possible, > but consolidation where practical is good). Same as x86, we still have different code base of kprobes and uprobes Fortunately, x86 instruction decoder is separated, but single-stepping and other parts are not well shared. >>> However, I also wonder if we should instead leave arch_specific_insn as >>> a kprobes specific structure and on ARM define it in terms of a new more >>> generic 'struct probe_insn'? The drawback with that is that we'd >>> probably end up with a struct just containing a single member which >>> seems a bit redundant: >>> >>> struct arch_specific_insn { >>> struct probe_insn pinsn; >>> }; >> >> I also disagree it. If you have a plan to integrate uprobes and kprobes >> arch specific code, please share it with us. > > There's not really a 'plan', just an attempt to reuse the instruction > decoding code used by kprobes in the implementation of uprobes, i.e. the > patch series [1] which this mail thread is in reply to. > > [1] http://thread.gmane.org/gmane.linux.kernel/1579985 OK, and I think similar method we can use on x86 too. :) In that case, we may be able to simplify the arch_specific_insn. Thank you,
(2013/11/15 5:33), David Long wrote: > On 11/14/13 09:15, Jon Medhurst (Tixy) wrote: >> On Thu, 2013-11-14 at 11:02 +0900, Masami Hiramatsu wrote: >>> (2013/11/14 2:13), Jon Medhurst (Tixy) wrote: >>>> On Tue, 2013-10-15 at 17:04 -0400, David Long wrote: >>>>> From: "David A. Long" <dave.long@linaro.org> >>>>> >>>>> Instead of depending on include/asm/kprobes.h to provide a dummy definition >>>>> for struct arch_specific_insn, do so in include/linux/kprobes.h. >>>> >>>> That change description doesn't quite seem to quite make sense to me. >>>> >>>> Anyway, what we're trying to do with this patch is to allow us to use >>>> arch_specific_insn for purposes additional to implementing kprobes. This >>>> patch enables that but I'm wary that the kprobes code assumes that ainsn >>>> is a struct arch_specific_insn, e.g. in linux/kernel/kprobes.c we have: >>>> >>>> memcpy(&p->ainsn, &ap->ainsn, sizeof(struct arch_specific_insn)); >>>> >>>> Now, that code isn't compiled when kprobes isn't configured, but it >>>> seams to me to be safer if that was also changed to >>>> >>>> memcpy(&p->ainsn, &ap->ainsn, sizeof(p->ainsn)); >>> > > That does look like an important improvement. Agreed. >>> This kind of cleanup looks good for me, but I don't agree to change >>> the type of the member (removing is OK) by Kconfig. >> >> Wouldn't that still require an #ifdef CONFIG_KPROBES around ainsn? >> Admittedly a less ugly one than one to change its type to an int. >> > > It is also possible to make the include of asm/kprobes.h unconditional, > although that might only cause the #ifdef to appear in more than one > include file. Good point! >>> Srikar, Oleg, I think it's a good time to merge such arch_specific mechanism >>> of uprobes and kprobes. Would you think we can do similar thing on x86 too? >> > > I welcome input from people who have experience in this area. I have no > desire to complicate efforts that might be attempted on other > architectures, but the existing code was unique to ARM and the goal here > was just to share it on ARM in implementing the currently non-existant > ARM uprobes feature. It seems to me the leakage of these changes into > generic kprobes code is exceedingly small, and unlikely to be a > hinderance to any future work (if any is even needed or planned) > supporting uprobes or kprobes on other architectures. In the meantime > we have ARM users who have been asking for uprobes support for a while > and this is, IMHO, a fairly clean approach to providing it. I see. I'd just like to suggest you that your improvement on ONE arch can also be useful idea for the other archs. In that case, there would be better, more efficient way to do that. Since we are on the same (or, next) track, we can learn many things each other. :) Thank you,
On 11/15/13 05:23, Masami Hiramatsu wrote: > I see. I'd just like to suggest you that your improvement on ONE arch > can also be useful idea for the other archs. In that case, there would be > better, more efficient way to do that. > Since we are on the same (or, next) track, we can learn many things each other. :) > > Thank you, > I'd like to suggest there is still no reason not to take the current work (with suggested changes), which interested parties can then further adapt for supporting other/multiple architectures. There is virtually no new architecture-specific code that would have to be redone later. Additionally, it's not clear to me how useful instruction interpretation is to any other architecture. We are doing this only because 32-bit ARM does not support single-stepping. -dl
diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h index 925eaf2..4b5a74d 100644 --- a/include/linux/kprobes.h +++ b/include/linux/kprobes.h @@ -52,9 +52,6 @@ #else /* CONFIG_KPROBES */ typedef int kprobe_opcode_t; -struct arch_specific_insn { - int dummy; -}; #endif /* CONFIG_KPROBES */ struct kprobe; @@ -110,7 +107,11 @@ struct kprobe { kprobe_opcode_t opcode; /* copy of the original instruction */ +#ifdef CONFIG_KPROBES struct arch_specific_insn ainsn; +#else + int ainsn; +#endif /* * Indicates various status flags.