On Mon, 29 Jan 2024 20:50:39 -0600 Jinghao Jia <[email protected]> wrote:
> On 1/29/24 19:44, Masami Hiramatsu (Google) wrote: > > On Sun, 28 Jan 2024 15:25:59 -0600 > > Jinghao Jia <[email protected]> wrote: > > > >>>> /* Check if paddr is at an instruction boundary */ > >>>> static int can_probe(unsigned long paddr) > >>>> { > >>>> @@ -294,6 +310,16 @@ static int can_probe(unsigned long paddr) > >>>> #endif > >>>> addr += insn.length; > >>>> } > >>>> + __addr = recover_probed_instruction(buf, addr); > >>>> + if (!__addr) > >>>> + return 0; > >>>> + > >>>> + if (insn_decode_kernel(&insn, (void *)__addr) < 0) > >>>> + return 0; > >>>> + > >>>> + if (is_exception_insn(&insn)) > >>>> + return 0; > >>>> + > >>> > >>> Please don't put this outside of decoding loop. You should put these in > >>> the loop which decodes the instruction from the beginning of the function. > >>> Since the x86 instrcution is variable length, can_probe() needs to check > >>> whether that the address is instruction boundary and decodable. > >>> > >>> Thank you, > >> > >> If my understanding is correct then this is trying to decode the kprobe > >> target instruction, given that it is after the main decoding loop. Here I > >> hoisted the decoding logic out of the if(IS_ENABLED(CONFIG_CFI_CLANG)) > >> block so that we do not need to decode the same instruction twice. I left > >> the main decoding loop unchanged so it is still decoding the function from > >> the start and should handle instruction boundaries. Are there any caveats > >> that I missed? > > > > Ah, sorry I misread the patch. You're correct! > > This is a good place to do that. > > > > But hmm, I think we should add another patch to check the addr == paddr > > soon after the loop so that we will avoid decoding. > > > > Thank you, > > > > Yes, that makes sense to me. At the same time, I'm also thinking about > changing the return type of can_probe() to bool, since we are just using > int as bool in this context. Yes, that is also a good change :) Thank you, > > --Jinghao > > >> > >> --Jinghao > >> > >>> > >>>> if (IS_ENABLED(CONFIG_CFI_CLANG)) { > >>>> /* > >>>> * The compiler generates the following instruction > >>>> sequence > >>>> @@ -308,13 +334,6 @@ static int can_probe(unsigned long paddr) > >>>> * Also, these movl and addl are used for showing > >>>> expected > >>>> * type. So those must not be touched. > >>>> */ > >>>> - __addr = recover_probed_instruction(buf, addr); > >>>> - if (!__addr) > >>>> - return 0; > >>>> - > >>>> - if (insn_decode_kernel(&insn, (void *)__addr) < 0) > >>>> - return 0; > >>>> - > >>>> if (insn.opcode.value == 0xBA) > >>>> offset = 12; > >>>> else if (insn.opcode.value == 0x3) > >>>> -- > >>>> 2.43.0 > >>>> > >>> > >>> > > > > -- Masami Hiramatsu (Google) <[email protected]>
