Re: [PATCH v6 3/8] x86: add generic function to modify more calls using int3 framework

[Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>]

(2014/01/15 9:33), Steven Rostedt wrote:
> On Tue, 10 Dec 2013 16:42:15 +0100
> Petr Mladek <pmladek@suse.cz> wrote:
> 
>> diff --git a/arch/x86/include/asm/alternative.h b/arch/x86/include/asm/alternative.h
>> index 586747f5f41d..82ffe7e1529c 100644
>> --- a/arch/x86/include/asm/alternative.h
>> +++ b/arch/x86/include/asm/alternative.h
>> @@ -232,4 +232,40 @@ extern int text_poke_bp(void *addr, const void *opcode, size_t len,
>>  extern void text_poke_bp_or_die(void *addr, const void *opcode, size_t len,
>>  				void *handler);
> 
> Small nit. If you can, place comments on the same line as the
> structure field.
>   
>> +struct text_poke_bp_iter {
>> +	/* information used to start iteration from the beginning */
>> +	void *init;
>> +	/* length of the patched instruction */
>> +	size_t len;
>> +	/* details about failure if any */
>> +	int fail_count;
>> +	void *fail_addr;
> 
> The above should have the comments on the same line as the field.
> Something like this:
> 
> 	void		*init;	/* information used to start
> 	iteration from the beginning */
> 
> The comments for the function pointers below are fine.
> 
>> +	/* iteration over entries */
>> +	void *(*start)(void *init);
>> +	void *(*next)(void *prev);
>> +	/* callback to get patched address */
>> +	void *(*get_addr)(void *cur);
>> +	/*
>> +	 * Callbacks to get the patched code. They could return NULL if no
>> +	 * patching is needed; This is useful for example in ftrace.
>> +	 */
>> +	const void *(*get_opcode)(void *cur);
>> +	const void *(*get_old_opcode)(void *cur);
>> +	/*
>> +	 * Optional function that is called when the patching of the given
>> +	 * has finished. It might be NULL if no postprocess is needed.
>> +	 */
>> +	int (*finish)(void *cur);
>> +	/*
>> +	 * Helper function for int3 handler. It decides whether the given IP
>> +	 * is being patched or not.
>> +	 *
>> +	 * Try to implement it as fast as possible. It affects performance
>> +	 * of the system when the patching is in progress.
>> +	 */
>> +	void *(*is_handled)(const unsigned long ip);
>> +};
>> +
>> +extern int text_poke_bp_list(struct text_poke_bp_iter *iter);
>> +
>>  #endif /* _ASM_X86_ALTERNATIVE_H */
>> diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
>> index 6436beec7b0c..8e57ac03a0e8 100644
>> --- a/arch/x86/kernel/alternative.c
>> +++ b/arch/x86/kernel/alternative.c
>> @@ -7,6 +7,7 @@
>>  #include <linux/stringify.h>
>>  #include <linux/kprobes.h>
>>  #include <linux/mm.h>
>> +#include <linux/uaccess.h>
>>  #include <linux/vmalloc.h>
>>  #include <linux/memory.h>
>>  #include <linux/stop_machine.h>
>> @@ -610,8 +611,11 @@ static void run_sync(void)
>>  		on_each_cpu(do_sync_core, NULL, 1);
>>  }
>>  
>> +static char bp_int3;
> 
> bp_int3 is not going to be anything but 0xcc. Let's change that to:
> 
> static char bp_int3 = 0xcc;
> 
> And remove the other initializations.

just a comment.
If it is always 0xcc, it should be a const variable.

Thank you,

-- 
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com