Re: [PATCH] x86 - Enhance DEBUG_RODATA support - alternatives

[Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>]

* pageexec@freemail.hu (pageexec@freemail.hu) wrote:
> On 6 Mar 2008 at 15:01, Ingo Molnar wrote:
> 
> > +/**
> > + * text_poke - Update instructions on a live kernel
> > + * @addr: address to modify
> > + * @opcode: source of the copy
> > + * @len: length to copy
> > + *
> > + * Only atomic text poke/set should be allowed when not doing early patching.
> > + * It means the size must be writable atomically and the address must be aligned
> > + * in a way that permits an atomic write. It also makes sure we fit on a single
> > + * page.
> > + */
> > +void *__kprobes text_poke(void *addr, const void *opcode, size_t len)
> > +{
> > +	unsigned long flags;
> > +	char *vaddr;
> > +	int nr_pages = 2;
> > +
> > +	BUG_ON(len > sizeof(long));
> > +	BUG_ON((((long)addr + len - 1) & ~(sizeof(long) - 1))
> > +		- ((long)addr & ~(sizeof(long) - 1)));
> > +	{
> > +		struct page *pages[2] = { virt_to_page(addr),
> > +			virt_to_page(addr + PAGE_SIZE) };

I just want to ask for confirmation about two things :

First, that calling this text_poke implementation to modify text in a
module won't fail. Is virt_to_page(addr) ok if addr is in a vmalloc'ed
area ?

Second, that calling virt_to_page(addr + PAGE_SIZE) won't have
undesirable side-effects if addr happens to be in the last page of an
allocated range. It should be ok for the core kernel text, because it is
followed by the kernel rodata, but I am not certain for modules.

If any of these two are not true, then we should add a test for

if (kernel_text_address(addr)) {

around the vmap() allocation and fallback on 

} else
  vaddr = addr;

in the cases where text_poke is called to modify module code.

Mathieu

> > +		if (!pages[1])
> > +			nr_pages = 1;
> > +		vaddr = vmap(pages, nr_pages, VM_MAP, PAGE_KERNEL);
> > +		WARN_ON(!vaddr);
> > +		local_irq_save(flags);
> > +		memcpy(&vaddr[(unsigned long)addr & ~PAGE_MASK], opcode, len);
> > +		local_irq_restore(flags);
> 
> you probably want a BUG_ON instead (or some graceful recovery) else
> the NULL deref will trigger with IRQs off...
> 
> > +		vunmap(vaddr);
> > +	}
> >  	sync_core();
> >  	/* Could also do a CLFLUSH here to speed up CPU recovery; but
> >  	   that causes hangs on some VIA CPUs. */
> > +	return addr;
> >  }
> 
> 

-- 
Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68