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.
--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
>>>>
>>>
>>>
>
>
OpenPGP_signature.asc
Description: OpenPGP digital signature
