linux-kernel - Re: [RFC][ PATCH -tip v2 2/7] kprobes: introducing generic insn

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4A492D86.5040604@redhat.com>
Date:	Mon, 29 Jun 2009 17:09:26 -0400
From:	Masami Hiramatsu <mhiramat@...hat.com>
To:	Steven Rostedt <rostedt@...dmis.org>
CC:	Ingo Molnar <mingo@...e.hu>,
	Ananth N Mavinakayanahalli <ananth@...ibm.com>,
	lkml <linux-kernel@...r.kernel.org>,
	systemtap <systemtap@...rces.redhat.com>,
	DLE <dle-develop@...ts.sourceforge.net>,
	Jim Keniston <jkenisto@...ibm.com>,
	Srikar Dronamraju <srikar@...ux.vnet.ibm.com>,
	Christoph Hellwig <hch@...radead.org>,
	Frederic Weisbecker <fweisbec@...il.com>,
	"H. Peter Anvin" <hpa@...or.com>,
	Anders Kaseorg <andersk@...lice.com>,
	Tim Abbott <tabbott@...lice.com>
Subject: Re: [RFC][ PATCH -tip v2 2/7] kprobes: introducing generic insn_slot
  framework

Thank you for reviewing.

Steven Rostedt wrote:
> Hi Masami,
> 
> I'm currently traveling so my responses are very slow this week.
> 
> 
> On Mon, 22 Jun 2009, Masami Hiramatsu wrote:
> 
>> Make insn_slot framework support various size slots.
>> Current insn_slot just supports one-size instruction buffer slot. However,
>> kprobes jump optimization needs larger size buffers.
>>
>> Signed-off-by: Masami Hiramatsu <mhiramat@...hat.com>
>> Cc: Ananth N Mavinakayanahalli <ananth@...ibm.com>
>> Cc: Ingo Molnar <mingo@...e.hu>
>> Cc: Jim Keniston <jkenisto@...ibm.com>
>> Cc: Srikar Dronamraju <srikar@...ux.vnet.ibm.com>
>> Cc: Christoph Hellwig <hch@...radead.org>
>> Cc: Steven Rostedt <rostedt@...dmis.org>
>> Cc: Frederic Weisbecker <fweisbec@...il.com>
>> Cc: H. Peter Anvin <hpa@...or.com>
>> Cc: Anders Kaseorg <andersk@...lice.com>
>> Cc: Tim Abbott <tabbott@...lice.com>
>> ---
>>
>>  kernel/kprobes.c |   96 +++++++++++++++++++++++++++++++++---------------------
>>  1 files changed, 58 insertions(+), 38 deletions(-)
>>
>> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
>> index 0b68fdc..bc9cfd0 100644
>> --- a/kernel/kprobes.c
>> +++ b/kernel/kprobes.c
>> @@ -100,26 +100,38 @@ static struct kprobe_blackpoint kprobe_blacklist[] = {
>>   * stepping on the instruction on a vmalloced/kmalloced/data page
>>   * is a recipe for disaster
>>   */
>> -#define INSNS_PER_PAGE	(PAGE_SIZE/(MAX_INSN_SIZE * sizeof(kprobe_opcode_t)))
>> -
>>  struct kprobe_insn_page {
>>  	struct list_head list;
>>  	kprobe_opcode_t *insns;		/* Page of instruction slots */
>> -	char slot_used[INSNS_PER_PAGE];
>>  	int nused;
>>  	int ngarbage;
>> +	char slot_used[1];
> 
> I would recommend using [] instead of [1], that would help other 
> developers know that it is a variable array.

Sure.

[...]
>> -	list_for_each_entry(kip, &kprobe_insn_pages, list) {
>> -		if (kip->nused < INSNS_PER_PAGE) {
>> +	list_for_each_entry(kip, &c->pages, list) {
>> +		if (kip->nused < slots_per_page(c)) {
>>  			int i;
>> -			for (i = 0; i < INSNS_PER_PAGE; i++) {
>> +			for (i = 0; i < slots_per_page(c); i++) {
>>  				if (kip->slot_used[i] == SLOT_CLEAN) {
>>  					kip->slot_used[i] = SLOT_USED;
>>  					kip->nused++;
>> -					return kip->insns + (i * MAX_INSN_SIZE);
>> +					return kip->insns + (i * c->insn_size);
>>  				}
>>  			}
>> -			/* Surprise!  No unused slots.  Fix kip->nused. */
>> -			kip->nused = INSNS_PER_PAGE;
>> +			/* kip->nused is broken. */
>> +			BUG();
> 
> Does this deserve a bug, or can we get away with a WARN and find a way to 
> fail nicely? Is it already too late to recover?

No, WARN() is enough here.

> 
>>  		}
>>  	}
>>  
>>  	/* If there are any garbage slots, collect it and try again. */
>> -	if (kprobe_garbage_slots && collect_garbage_slots() == 0) {
>> +	if (c->nr_garbage && collect_garbage_slots(c) == 0)
>>  		goto retry;
>> -	}
>> +
>>  	/* All out of space.  Need to allocate a new page. Use slot 0. */
>> -	kip = kmalloc(sizeof(struct kprobe_insn_page), GFP_KERNEL);
>> +	kip = kmalloc(sizeof(struct kprobe_insn_page) + slots_per_page(c) - 1,
> 
> Why the '- 1'?  Is it because of the char [1] above?
> 
> Would be better to make the size of the kprobe_insn_page a macro:
> 
> #define KPROBE_INSN_SIZE offsetof(struct kbrobe_insn_page, slot_used)
> 
> and then you can do the following:
> 
> 	kip = kmalloc(KPROBE_INSN_SIZE + slots_per_page(c));

Good idea!

Thanks


-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

e-mail: mhiramat@...hat.com

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