Received-SPF: pass (google.com: domain of linux-kernel+bounces-134753-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <d3e65876-0730-98a5-f89e-d7841797de31@huawei.com>
Date: Mon, 8 Apr 2024 11:29:04 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.0
Subject: Re: [PATCH] kprobes: Fix possible warn in __arm_kprobe_ftrace()
Content-Language: en-US
To: "Masami Hiramatsu (Google)" <mhiramat@kernel.org>
CC: <naveen.n.rao@linux.ibm.com>, <anil.s.keshavamurthy@intel.com>,
	<davem@davemloft.net>, <linux-kernel@vger.kernel.org>,
	<linux-trace-kernel@vger.kernel.org>
References: <20240407035904.2556645-1-zhengyejian1@huawei.com>
 <20240408115038.b0c85767bf1f249eccc32fff@kernel.org>
From: Zheng Yejian <zhengyejian1@huawei.com>
In-Reply-To: <20240408115038.b0c85767bf1f249eccc32fff@kernel.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit

On 2024/4/8 10:50, Masami Hiramatsu (Google) wrote:
> On Sun, 7 Apr 2024 11:59:04 +0800
> Zheng Yejian <zhengyejian1@huawei.com> wrote:
> 
>> There is once warn in __arm_kprobe_ftrace() on:
>>
>>   ret = ftrace_set_filter_ip(ops, (unsigned long)p->addr, 0, 0);
>>   if (WARN_ONCE(..., "Failed to arm kprobe-ftrace at %pS (error %d)\n", ...)
>>     return ret;
>>
>> This warning is due to 'p->addr' is not a valid ftrace_location and
>> that invalid 'p->addr' was bypassed in check_kprobe_address_safe():
> 
> Ah, this is a guard warning to detect changes on ftrace_set_filter() and/or
> preparation steps. (A kind of debug message.)
> The ftrace address check is done by check_ftrace_location() at the beginning of
> check_kprobe_address_safe(). At that point, ftrace_location(addr) == addr should
> return true if the module is loaded. But there is a small window that we check
> the ftrace_location() and get the module refcount, even if the "addr" was ftrace
> location, the module is unloaded and failed to get the module refcount and fail
> to register the kprobe.

Thanks, I'll complete the description in V2.

> 
>>     check_kprobe_address_safe() {
>>       ...
>>       // 1. p->addr is in some module, this check passed
>>       is_module_text_address((unsigned long) p->addr)
>>       ...
>>       // 2. If the moudle is removed, the *probed_mod is NULL, but then
>>       //    the return value would still be 0 !!!
>>       *probed_mod = __module_text_address((unsigned long) p->addr);
>>       ...
>>     }
>>
>> So adjust the module text check to fix it.
> 
> This seems just optimizing code (skip the 2nd module search), right?

Yes, optimze more than fix, but this can still avoid latter warn like in
__arm_kprobe_ftrace()

> Also some comments needs to be updated.

Will do in V2.

> 
>>
>> Signed-off-by: Zheng Yejian <zhengyejian1@huawei.com>
>> ---
>>   kernel/kprobes.c | 13 +++++++++----
>>   1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
>> index 9d9095e81792..e0612cc3e2a3 100644
>> --- a/kernel/kprobes.c
>> +++ b/kernel/kprobes.c
>> @@ -1567,10 +1567,16 @@ static int check_kprobe_address_safe(struct kprobe *p,
>>   	jump_label_lock();
>>   	preempt_disable();
>>
> 
> 	/* Ensure the address is in a text area, and find a module if exists. */
>    
>> +	*probed_mod = NULL;
>> +	if (!core_kernel_text((unsigned long) p->addr)) {
>> +		*probed_mod = __module_text_address((unsigned long) p->addr);
>> +		if (!(*probed_mod)) {
>> +			ret = -EINVAL;
>> +			goto out;
>> +		}
> 
> nit: this should get the module refcount soon after getting probed_mod.
> (I think this should be an atomic operation, but we don't have such interface.)

Yes, it's better to get the module refcount right here, but suppose the
module is expected to be unloaded soon, we don't really need to make the 
registration succeed.

> 
>> +	}
> 
>>   	/* Ensure it is not in reserved area nor out of text */
> 
> 	/* Ensure it is not in reserved area. */
> 
>> -	if (!(core_kernel_text((unsigned long) p->addr) ||
>> -	    is_module_text_address((unsigned long) p->addr)) ||
> 
> Note that this part is doing same thing. If the probe address is !kernel_text
> and !module_text, it will be rejected.
> 

Yes.

>> -	    in_gate_area_no_mm((unsigned long) p->addr) ||
>> +	if (in_gate_area_no_mm((unsigned long) p->addr) ||
>>   	    within_kprobe_blacklist((unsigned long) p->addr) ||
>>   	    jump_label_text_reserved(p->addr, p->addr) ||
>>   	    static_call_text_reserved(p->addr, p->addr) ||
>> @@ -1581,7 +1587,6 @@ static int check_kprobe_address_safe(struct kprobe *p,
>>   	}
>>   
>>   	/* Check if 'p' is probing a module. */
> 
> 	/* Get module refcount and reject __init functions for loaded modules. */
> 
>> -	*probed_mod = __module_text_address((unsigned long) p->addr);
>>   	if (*probed_mod) {
>>   		/*
>>   		 * We must hold a refcount of the probed module while updating
>> -- 
>> 2.25.1
>>
> 
> Thank you,
> 

--

Thank you,
Zheng Yejian