Re: [PATCH 11/13] tracing/kprobes: Add priv argument to fetch functions

2013-11-04 Thread Namhyung Kim 
On Mon, 4 Nov 2013 17:09:14 +0100, Oleg Nesterov wrote:
> See my replies to 0/13. Lets assume that you agree that get_user_vaddr()
> doesn't need tu->inode.

Okay.

>
> On 10/29, Namhyung Kim wrote:
>>
>> This argument is for passing private data structure to each fetch
>> function and will be used by uprobes.
>
> In this case, why do we need this "void *priv"? It actually becomes
> "bool need_addr_translation".

Right.  I added it because the deref method is used for both of kprobes
and uprobes and it only needed for uprobes.  And I thought if we need
something for kprobes later, it can be reused so make it general.

>
> Can't we avoid it? Can't we just add FETCH_MTD_memory_notranslate ?
> kprobes should use the same methods for FETCH_MTD_memory*, uprobes
> should obviously adjust the addr in FETCH_MTD_memory.
>
> Then (afaics) we need a single change in parse_probe_arg(),
>
>   -   dprm->fetch = t->fetch[FETCH_MTD_memory];
>   +   dprm->fetch = t->fetch[FETCH_MTD_memory_notranslate];
>
> Yes, this will blow *probes_fetch_type_table[], but looks simpler.

Looks good to me too, thanks! :)

>
> And. This way it would be simple to teach parse_probe_arg('@') to use
> _notranslate, say, "@=addr" or whatever.

Yeah, it'll be useful at least for fetching data in anon pages.

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/

Re: [PATCH 11/13] tracing/kprobes: Add priv argument to fetch functions

2013-11-04 Thread Oleg Nesterov 
See my replies to 0/13. Lets assume that you agree that get_user_vaddr()
doesn't need tu->inode.

On 10/29, Namhyung Kim wrote:
>
> This argument is for passing private data structure to each fetch
> function and will be used by uprobes.

In this case, why do we need this "void *priv"? It actually becomes
"bool need_addr_translation".

Can't we avoid it? Can't we just add FETCH_MTD_memory_notranslate ?
kprobes should use the same methods for FETCH_MTD_memory*, uprobes
should obviously adjust the addr in FETCH_MTD_memory.

Then (afaics) we need a single change in parse_probe_arg(),

-   dprm->fetch = t->fetch[FETCH_MTD_memory];
+   dprm->fetch = t->fetch[FETCH_MTD_memory_notranslate];

Yes, this will blow *probes_fetch_type_table[], but looks simpler.

And. This way it would be simple to teach parse_probe_arg('@') to use
_notranslate, say, "@=addr" or whatever.

Oleg.

--
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/

Re: [PATCH 11/13] tracing/kprobes: Add priv argument to fetch functions

2013-11-04 Thread Namhyung Kim 
On Mon, 4 Nov 2013 17:09:14 +0100, Oleg Nesterov wrote:
 See my replies to 0/13. Lets assume that you agree that get_user_vaddr()
 doesn't need tu-inode.

Okay.


 On 10/29, Namhyung Kim wrote:

 This argument is for passing private data structure to each fetch
 function and will be used by uprobes.

 In this case, why do we need this void *priv? It actually becomes
 bool need_addr_translation.

Right.  I added it because the deref method is used for both of kprobes
and uprobes and it only needed for uprobes.  And I thought if we need
something for kprobes later, it can be reused so make it general.


 Can't we avoid it? Can't we just add FETCH_MTD_memory_notranslate ?
 kprobes should use the same methods for FETCH_MTD_memory*, uprobes
 should obviously adjust the addr in FETCH_MTD_memory.

 Then (afaics) we need a single change in parse_probe_arg(),

   -   dprm-fetch = t-fetch[FETCH_MTD_memory];
   +   dprm-fetch = t-fetch[FETCH_MTD_memory_notranslate];

 Yes, this will blow *probes_fetch_type_table[], but looks simpler.

Looks good to me too, thanks! :)


 And. This way it would be simple to teach parse_probe_arg('@') to use
 _notranslate, say, @=addr or whatever.

Yeah, it'll be useful at least for fetching data in anon pages.

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/

Re: [PATCH 11/13] tracing/kprobes: Add priv argument to fetch functions

2013-11-04 Thread Oleg Nesterov 
See my replies to 0/13. Lets assume that you agree that get_user_vaddr()
doesn't need tu-inode.

On 10/29, Namhyung Kim wrote:

 This argument is for passing private data structure to each fetch
 function and will be used by uprobes.

In this case, why do we need this void *priv? It actually becomes
bool need_addr_translation.

Can't we avoid it? Can't we just add FETCH_MTD_memory_notranslate ?
kprobes should use the same methods for FETCH_MTD_memory*, uprobes
should obviously adjust the addr in FETCH_MTD_memory.

Then (afaics) we need a single change in parse_probe_arg(),

-   dprm-fetch = t-fetch[FETCH_MTD_memory];
+   dprm-fetch = t-fetch[FETCH_MTD_memory_notranslate];

Yes, this will blow *probes_fetch_type_table[], but looks simpler.

And. This way it would be simple to teach parse_probe_arg('@') to use
_notranslate, say, @=addr or whatever.

Oleg.

--
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/

Re: [PATCH 11/13] tracing/kprobes: Add priv argument to fetch functions

2013-08-09 Thread Masami Hiramatsu 
(2013/08/09 17:45), Namhyung Kim wrote:
> From: Namhyung Kim 
> 
> This argument is for passing private data structure to each fetch
> function and will be used by uprobes.

OK, I just concern about overhead, but yeah, these fetch functions
are not optimized yet. that's another story. :)
I'd rather increase flexibility in this series.

Acked-by: Masami Hiramatsu 

Thank you!

> Cc: Srikar Dronamraju 
> Cc: Oleg Nesterov 
> Cc: zhangwei(Jovi) 
> Cc: Arnaldo Carvalho de Melo 
> Signed-off-by: Namhyung Kim 
> ---
>  kernel/trace/trace_kprobe.c | 32 ++--
>  kernel/trace/trace_probe.c  | 24 
>  kernel/trace/trace_probe.h  | 19 ++-
>  kernel/trace/trace_uprobe.c |  8 
>  4 files changed, 44 insertions(+), 39 deletions(-)
> 
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index e3f798e41820..01bf5c879144 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -740,7 +740,7 @@ static const struct file_operations kprobe_profile_ops = {
>   */
>  #define DEFINE_FETCH_stack(type) \
>  static __kprobes void FETCH_FUNC_NAME(stack, type)(struct pt_regs *regs,\
> -   void *offset, void *dest) \
> +   void *offset, void *dest, void *priv) \
>  {\
>   *(type *)dest = (type)regs_get_kernel_stack_nth(regs,   \
>   (unsigned int)((unsigned long)offset)); \
> @@ -752,7 +752,7 @@ DEFINE_BASIC_FETCH_FUNCS(stack)
>  
>  #define DEFINE_FETCH_memory(type)\
>  static __kprobes void FETCH_FUNC_NAME(memory, type)(struct pt_regs *regs,\
> -   void *addr, void *dest)   \
> + void *addr, void *dest, void *priv) \
>  {\
>   type retval;\
>   if (probe_kernel_address(addr, retval)) \
> @@ -766,7 +766,7 @@ DEFINE_BASIC_FETCH_FUNCS(memory)
>   * length and relative data location.
>   */
>  static __kprobes void FETCH_FUNC_NAME(memory, string)(struct pt_regs *regs,
> -void *addr, void *dest)
> + void *addr, void *dest, void *priv)
>  {
>   long ret;
>   int maxlen = get_rloc_len(*(u32 *)dest);
> @@ -803,7 +803,7 @@ static __kprobes void FETCH_FUNC_NAME(memory, 
> string)(struct pt_regs *regs,
>  
>  /* Return the length of string -- including null terminal byte */
>  static __kprobes void FETCH_FUNC_NAME(memory, string_size)(struct pt_regs 
> *regs,
> - void *addr, void *dest)
> +void *addr, void *dest, void *priv)
>  {
>   mm_segment_t old_fs;
>   int ret, len = 0;
> @@ -874,11 +874,11 @@ struct symbol_cache *alloc_symbol_cache(const char 
> *sym, long offset)
>  
>  #define DEFINE_FETCH_symbol(type)\
>  __kprobes void FETCH_FUNC_NAME(symbol, type)(struct pt_regs *regs,   \
> -   void *data, void *dest)   \
> + void *data, void *dest, void *priv) \
>  {\
>   struct symbol_cache *sc = data; \
>   if (sc->addr)   \
> - fetch_memory_##type(regs, (void *)sc->addr, dest);  \
> + fetch_memory_##type(regs, (void *)sc->addr, dest, priv);\
>   else\
>   *(type *)dest = 0;  \
>  }
> @@ -924,7 +924,7 @@ __kprobe_trace_func(struct trace_kprobe *tp, struct 
> pt_regs *regs,
>   local_save_flags(irq_flags);
>   pc = preempt_count();
>  
> - dsize = __get_data_size(>p, regs);
> + dsize = __get_data_size(>p, regs, NULL);
>   size = sizeof(*entry) + tp->p.size + dsize;
>  
>   event = trace_event_buffer_lock_reserve(, ftrace_file,
> @@ -935,7 +935,8 @@ __kprobe_trace_func(struct trace_kprobe *tp, struct 
> pt_regs *regs,
>  
>   entry = ring_buffer_event_data(event);
>   entry->ip = (unsigned long)tp->rp.kp.addr;
> - store_trace_args(sizeof(*entry), >p, regs, (u8 *)[1], dsize);
> + store_trace_args(sizeof(*entry), >p, regs, (u8 *)[1], dsize,
> +  NULL);
>  
>   if (!filter_current_check_discard(buffer, call, entry, event))
>   trace_buffer_unlock_commit_regs(buffer, event,
> @@ -972,7 +973,7 @@ __kretprobe_trace_func(struct trace_kprobe *tp, struct 
> kretprobe_instance *ri,
>   local_save_flags(irq_flags);
>

Re: [PATCH 11/13] tracing/kprobes: Add priv argument to fetch functions

2013-08-09 Thread Masami Hiramatsu 
(2013/08/09 17:45), Namhyung Kim wrote:
 From: Namhyung Kim namhyung@lge.com
 
 This argument is for passing private data structure to each fetch
 function and will be used by uprobes.

OK, I just concern about overhead, but yeah, these fetch functions
are not optimized yet. that's another story. :)
I'd rather increase flexibility in this series.

Acked-by: Masami Hiramatsu masami.hiramatsu...@hitachi.com

Thank you!

 Cc: Srikar Dronamraju sri...@linux.vnet.ibm.com
 Cc: Oleg Nesterov o...@redhat.com
 Cc: zhangwei(Jovi) jovi.zhang...@huawei.com
 Cc: Arnaldo Carvalho de Melo a...@ghostprotocols.net
 Signed-off-by: Namhyung Kim namhy...@kernel.org
 ---
  kernel/trace/trace_kprobe.c | 32 ++--
  kernel/trace/trace_probe.c  | 24 
  kernel/trace/trace_probe.h  | 19 ++-
  kernel/trace/trace_uprobe.c |  8 
  4 files changed, 44 insertions(+), 39 deletions(-)
 
 diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
 index e3f798e41820..01bf5c879144 100644
 --- a/kernel/trace/trace_kprobe.c
 +++ b/kernel/trace/trace_kprobe.c
 @@ -740,7 +740,7 @@ static const struct file_operations kprobe_profile_ops = {
   */
  #define DEFINE_FETCH_stack(type) \
  static __kprobes void FETCH_FUNC_NAME(stack, type)(struct pt_regs *regs,\
 -   void *offset, void *dest) \
 +   void *offset, void *dest, void *priv) \
  {\
   *(type *)dest = (type)regs_get_kernel_stack_nth(regs,   \
   (unsigned int)((unsigned long)offset)); \
 @@ -752,7 +752,7 @@ DEFINE_BASIC_FETCH_FUNCS(stack)
  
  #define DEFINE_FETCH_memory(type)\
  static __kprobes void FETCH_FUNC_NAME(memory, type)(struct pt_regs *regs,\
 -   void *addr, void *dest)   \
 + void *addr, void *dest, void *priv) \
  {\
   type retval;\
   if (probe_kernel_address(addr, retval)) \
 @@ -766,7 +766,7 @@ DEFINE_BASIC_FETCH_FUNCS(memory)
   * length and relative data location.
   */
  static __kprobes void FETCH_FUNC_NAME(memory, string)(struct pt_regs *regs,
 -void *addr, void *dest)
 + void *addr, void *dest, void *priv)
  {
   long ret;
   int maxlen = get_rloc_len(*(u32 *)dest);
 @@ -803,7 +803,7 @@ static __kprobes void FETCH_FUNC_NAME(memory, 
 string)(struct pt_regs *regs,
  
  /* Return the length of string -- including null terminal byte */
  static __kprobes void FETCH_FUNC_NAME(memory, string_size)(struct pt_regs 
 *regs,
 - void *addr, void *dest)
 +void *addr, void *dest, void *priv)
  {
   mm_segment_t old_fs;
   int ret, len = 0;
 @@ -874,11 +874,11 @@ struct symbol_cache *alloc_symbol_cache(const char 
 *sym, long offset)
  
  #define DEFINE_FETCH_symbol(type)\
  __kprobes void FETCH_FUNC_NAME(symbol, type)(struct pt_regs *regs,   \
 -   void *data, void *dest)   \
 + void *data, void *dest, void *priv) \
  {\
   struct symbol_cache *sc = data; \
   if (sc-addr)   \
 - fetch_memory_##type(regs, (void *)sc-addr, dest);  \
 + fetch_memory_##type(regs, (void *)sc-addr, dest, priv);\
   else\
   *(type *)dest = 0;  \
  }
 @@ -924,7 +924,7 @@ __kprobe_trace_func(struct trace_kprobe *tp, struct 
 pt_regs *regs,
   local_save_flags(irq_flags);
   pc = preempt_count();
  
 - dsize = __get_data_size(tp-p, regs);
 + dsize = __get_data_size(tp-p, regs, NULL);
   size = sizeof(*entry) + tp-p.size + dsize;
  
   event = trace_event_buffer_lock_reserve(buffer, ftrace_file,
 @@ -935,7 +935,8 @@ __kprobe_trace_func(struct trace_kprobe *tp, struct 
 pt_regs *regs,
  
   entry = ring_buffer_event_data(event);
   entry-ip = (unsigned long)tp-rp.kp.addr;
 - store_trace_args(sizeof(*entry), tp-p, regs, (u8 *)entry[1], dsize);
 + store_trace_args(sizeof(*entry), tp-p, regs, (u8 *)entry[1], dsize,
 +  NULL);
  
   if (!filter_current_check_discard(buffer, call, entry, event))
   trace_buffer_unlock_commit_regs(buffer, event,
 @@ -972,7 +973,7 @@ __kretprobe_trace_func(struct trace_kprobe