On 1/29/24 19:44, Masami Hiramatsu (Google) wrote:
> On Sun, 28 Jan 2024 15:25:59 -0600
> Jinghao Jia <jingh...@illinois.edu> 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
