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,
>
> --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]>
