Received-SPF: pass (google.com: domain of linux-kernel+bounces-209166-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <57e499a4-e26d-148f-317d-233e873d11b4@huawei.com>
Date: Tue, 11 Jun 2024 09:56:51 +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: [RFC PATCH] ftrace: Skip __fentry__ location of overridden weak
 functions
Content-Language: en-US
To: Peter Zijlstra <peterz@infradead.org>
CC: <rostedt@goodmis.org>, <mcgrof@kernel.org>, <mhiramat@kernel.org>,
	<mark.rutland@arm.com>, <mathieu.desnoyers@efficios.com>,
	<jpoimboe@kernel.org>, <linux-modules@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, <linux-trace-kernel@vger.kernel.org>,
	<bpf@vger.kernel.org>
References: <20240607115211.734845-1-zhengyejian1@huawei.com>
 <20240607150228.GR8774@noisy.programming.kicks-ass.net>
From: Zheng Yejian <zhengyejian1@huawei.com>
In-Reply-To: <20240607150228.GR8774@noisy.programming.kicks-ass.net>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit

On 2024/6/7 23:02, Peter Zijlstra wrote:
> On Fri, Jun 07, 2024 at 07:52:11PM +0800, Zheng Yejian wrote:
>> ftrace_location() was changed to not only return the __fentry__ location
>> when called for the __fentry__ location, but also when called for the
>> sym+0 location after commit aebfd12521d9 ("x86/ibt,ftrace: Search for
>> __fentry__ location"). That is, if sym+0 location is not __fentry__,
>> ftrace_location() would find one over the entire size of the sym.
>>
>> However, there is case that more than one __fentry__ exist in the sym
>> range (described below) and ftrace_location() would find wrong __fentry__
>> location by binary searching, which would cause its users like livepatch/
>> kprobe/bpf to not work properly on this sym!
>>
>> The case is that, based on current compiler behavior, suppose:
>>   - function A is followed by weak function B1 in same binary file;
>>   - weak function B1 is overridden by function B2;
>> Then in the final binary file:
>>   - symbol B1 will be removed from symbol table while its instructions are
>>     not removed;
>>   - __fentry__ of B1 will be still in __mcount_loc table;
>>   - function size of A is computed by substracting the symbol address of
>>     A from its next symbol address (see kallsyms_lookup_size_offset()),
>>     but because symbol info of B1 is removed, the next symbol of A is
>>     originally the next symbol of B1. See following example, function
>>     sizeof A will be (symbol_address_C - symbol_address_A):
>>
>>       symbol_address_A
>>       symbol_address_B1 (Not in symbol table)
>>       symbol_address_C
>>
>> The weak function issue has been discovered in commit b39181f7c690
>> ("ftrace: Add FTRACE_MCOUNT_MAX_OFFSET to avoid adding weak function")
>> but it didn't resolve the issue in ftrace_location().
>>
>> There may be following resolutions:
> 
> Oh gawd, sodding weak functions again.
> 
> I would suggest changing scipts/kallsyms.c to emit readily identifiable
> symbol names for all the weak junk, eg:
> 
>    __weak_junk_NNNNN
> 

Sorry for the late reply, I just had a long noon holiday :>

scripts/kallsyms.c is compiled and used to handle symbols in vmlinux.o
or vmlinux.a, see kallsyms_step() in scripts/link-vmlinux.sh, those
overridden weak symbols has been removed from symbol table of vmlinux.o
or vmlinux.a. But we can found those symbols from original xx/xx.o file,
for example, the weak free_initmem() in in init/main.c is overridden,
its symbol is not in vmlinx but is still in init/main.o .

How about traversing all origin xx/xx.o and finding all weak junk symbols ?

> That instantly fixes the immediate problem and Steve's horrid hack can
> go away.
> 

Yes, this can be done in same patch series.

> Additionally, I would add a boot up pass that would INT3 fill all such
> functions and remove/invalidate all
> static_call/static_jump/fentry/alternative entry that is inside of them.
> 
> 
> 

--

Thanks,
Zheng Yejian