Subject: Re: [PATCH 2/3] x86/kprobes: Fix __recover_optprobed_insn check
 optimizing logic
To:     "Masami Hiramatsu (Google)" <mhiramat@kernel.org>
CC:     <tglx@linutronix.de>, <mingo@redhat.com>, <bp@alien8.de>,
        <dave.hansen@linux.intel.com>, <x86@kernel.org>, <hpa@zytor.com>,
        <naveen.n.rao@linux.ibm.com>, <anil.s.keshavamurthy@intel.com>,
        <davem@davemloft.net>, <ast@kernel.org>, <peterz@infradead.org>,
        <linux-kernel@vger.kernel.org>,
        <linux-trace-kernel@vger.kernel.org>
References: <20230215115430.236046-1-yangjihong1@huawei.com>
 <20230215115430.236046-3-yangjihong1@huawei.com>
 <20230216000849.f0e332ebf35150cb5d5c4a29@kernel.org>
From:   Yang Jihong <yangjihong1@huawei.com>
Message-ID: <403950ec-b4d1-e0e1-2c30-742e2bd905f8@huawei.com>
Date:   Thu, 16 Feb 2023 10:53:48 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.6.1
MIME-Version: 1.0
In-Reply-To: <20230216000849.f0e332ebf35150cb5d5c4a29@kernel.org>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hello Masami,

On 2023/2/15 23:08, Masami Hiramatsu (Google) wrote:
> On Wed, 15 Feb 2023 19:54:29 +0800
> Yang Jihong <yangjihong1@huawei.com> wrote:
> 
>> Since the following commit:
>>
>>    commit f66c0447cca1 ("kprobes: Set unoptimized flag after unoptimizing code")
>>
>> modified the update timing of the KPROBE_FLAG_OPTIMIZED, a optimized_kprobe
>> may be in the optimizing or unoptimizing state when op.kp->flags
>> has KPROBE_FLAG_OPTIMIZED and op->list is not empty.
>>
>> The __recover_optprobed_insn check logic is incorrect, a kprobe in the
>> unoptimizing state may be incorrectly determined as unoptimizing.
>> As a result, incorrect instructions are copied.
> 
> Ah, good catch!
> 
>>
>> The optprobe_queued_unopt function needs to be exported for invoking in
>> arch directory.
>>
>> Fixes: f66c0447cca1 ("kprobes: Set unoptimized flag after unoptimizing code")
> 
> Cc: stable@vger.kernel.org

OK, will add in next version.

> 
>> Signed-off-by: Yang Jihong <yangjihong1@huawei.com>
>> ---
>>   arch/x86/kernel/kprobes/opt.c | 4 ++--
>>   include/linux/kprobes.h       | 1 +
>>   kernel/kprobes.c              | 2 +-
>>   3 files changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c
>> index e57e07b0edb6..3718d6863555 100644
>> --- a/arch/x86/kernel/kprobes/opt.c
>> +++ b/arch/x86/kernel/kprobes/opt.c
>> @@ -46,8 +46,8 @@ unsigned long __recover_optprobed_insn(kprobe_opcode_t *buf, unsigned long addr)
>>   		/* This function only handles jump-optimized kprobe */
>>   		if (kp && kprobe_optimized(kp)) {
>>   			op = container_of(kp, struct optimized_kprobe, kp);
>> -			/* If op->list is not empty, op is under optimizing */
>> -			if (list_empty(&op->list))
>> +			/* If op is [un]optimized or under unoptimizing */
> 
> Hmm, this is a bit confusing comment. If it is unoptimized, the kprobe_optimized() returns false.
> Thus the comment should be /* If op is optimized or under unoptimizing */.
> 

OK, will fix in next version.

Thanks,
Yang.