Re: [PATCH v3 1/8] x86: allow to handle errors in text_poke function family

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

(2013/11/14 19:41), Petr Mladek wrote:
> The text_poke functions called BUG() in case of error. This was too strict.
> There are situations when the system is still usable even when the patching
> has failed, for example when enabling the dynamic ftrace.
> 
> This commit modifies text_poke, text_poke_early, and text_poke_bp functions
> to return an error code instead calling BUG(). The code is returned instead
> of the patched address. The address was just copied from the first parameter,
> so it was no extra information. It has not been used anywhere yet.
> 
> The commit also modifies the few locations where text_poke functions were used
> and the error code has to be handled now. It just passes the error code if
> there already is an existing error handling, for example in
> kgdb_arch_set_breakpoint. It calls BUG() in the other locations.
> 
> Note that BUG() still need to be called in text_poke_bp when the code already is
> partially modified but the operation can not be finished.

Mostly this looks good for me, see below.

> @@ -566,7 +576,8 @@ void *__kprobes text_poke(void *addr, const void *opcode, size_t len)
>  		WARN_ON(!PageReserved(pages[0]));
>  		pages[1] = virt_to_page(addr + PAGE_SIZE);
>  	}
> -	BUG_ON(!pages[0]);
> +	if (unlikely(!pages[0]))
> +		return -EFAULT;
>  	local_irq_save(flags);
>  	set_fixmap(FIX_TEXT_POKE0, page_to_phys(pages[0]));
>  	if (pages[1])
> @@ -580,10 +591,11 @@ void *__kprobes text_poke(void *addr, const void *opcode, size_t len)
>  	sync_core();
>  	/* Could also do a CLFLUSH here to speed up CPU recovery; but
>  	   that causes hangs on some VIA CPUs. */
> -	for (i = 0; i < len; i++)
> -		BUG_ON(((char *)addr)[i] != ((char *)opcode)[i]);
>  	local_irq_restore(flags);
> -	return addr;
> +	for (i = 0; i < len; i++)
> +		if (((char *)addr)[i] != ((char *)opcode)[i])
> +			return -EPERM;
> +	return 0;

Hmm, IMHO, it would be better to keep the old one, because it actually failed to modify
as expected for some reason (not only permission error). And we are not sure the
code is same as old one. (only compared with what we expected)

Thank you,

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