| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <50180a93-b555-087e-2376-3bf46fabafb6@huawei.com>
Date: Mon, 22 Jul 2024 09:29:07 +0800
From: Zheng Yejian <zhengyejian1@...wei.com>
To: Masahiro Yamada <masahiroy@...nel.org>
CC: <rostedt@...dmis.org>, <mhiramat@...nel.org>, <mark.rutland@....com>,
<mpe@...erman.id.au>, <npiggin@...il.com>, <christophe.leroy@...roup.eu>,
<naveen.n.rao@...ux.ibm.com>, <tglx@...utronix.de>, <mingo@...hat.com>,
<bp@...en8.de>, <dave.hansen@...ux.intel.com>, <x86@...nel.org>,
<hpa@...or.com>, <mcgrof@...nel.org>, <mathieu.desnoyers@...icios.com>,
<nathan@...nel.org>, <nicolas@...sle.eu>, <kees@...nel.org>,
<james.clark@....com>, <kent.overstreet@...ux.dev>, <yhs@...com>,
<jpoimboe@...nel.org>, <peterz@...radead.org>,
<linux-kernel@...r.kernel.org>, <linux-trace-kernel@...r.kernel.org>,
<linuxppc-dev@...ts.ozlabs.org>, <linux-modules@...r.kernel.org>,
<linux-kbuild@...r.kernel.org>, <bpf@...r.kernel.org>
Subject: Re: [PATCH 2/6] kallsyms: Emit symbol at the holes in the text
On 2024/7/20 22:14, Masahiro Yamada wrote:
> On Thu, Jul 18, 2024 at 12:45 PM Zheng Yejian <zhengyejian1@...wei.com> wrote:
>>
>> On 2024/7/16 16:33, Masahiro Yamada wrote:
>>> On Thu, Jun 13, 2024 at 10:36 PM Zheng Yejian <zhengyejian1@...wei.com> wrote:
>>>>
>>>> When a weak type function is overridden, its symbol will be removed
>>>> from the symbol table, but its code will not be removed. Besides,
>>>> due to lacking of size for kallsyms, kernel compute function size by
>>>> substracting its symbol address from its next symbol address (see
>>>> kallsyms_lookup_size_offset()). These will cause that size of some
>>>> function is computed to be larger than it actually is, just because
>>>> symbol of its following weak function is removed.
>>>>
>>>> This issue also causes multiple __fentry__ locations to be counted in
>>>> the some function scope, and eventually causes ftrace_location() to find
>>>> wrong __fentry__ location. It was reported in
>>>> Link: https://lore.kernel.org/all/20240607115211.734845-1-zhengyejian1@huawei.com/
>>>>
>>>> Peter suggested to change scipts/kallsyms.c to emit readily
>>>> identifiable symbol names for all the weak junk, eg:
>>>>
>>>> __weak_junk_NNNNN
>>>>
>>>> The name of this kind symbol needs some discussion, but it's temporarily
>>>> called "__hole_symbol_XXXXX" in this patch:
>>>> 1. Pass size info to scripts/kallsyms (see mksysmap());
>>>> 2. Traverse sorted function symbols, if one function address plus its
>>>> size less than next function address, it means there's a hole, then
>>>> emit a symbol "__hole_symbol_XXXXX" there which type is 't'.
>>>>
>>>> After this patch, the effect is as follows:
>>>>
>>>> $ cat /proc/kallsyms | grep -A 3 do_one_initcall
>>>> ffffffff810021e0 T do_one_initcall
>>>> ffffffff8100245e t __hole_symbol_XXXXX
>>>> ffffffff810024a0 t __pfx_rootfs_init_fs_context
>>>>
>>>> Suggested-by: Peter Zijlstra <peterz@...radead.org>
>>>> Signed-off-by: Zheng Yejian <zhengyejian1@...wei.com>
>>>
>>>
>>>
>>> With my quick test, "t__hole_symbol_XXXXX" was encoded
>>> into the following 10-byte stream.
>>>
>>> .byte 0x09, 0x94, 0xbf, 0x18, 0xf3, 0x3d, 0xce, 0xd1, 0xd1, 0x58
>>>
>>>
>>>
>>> Now "t__hole_symbol_XXXXX" is the most common symbol name.
>>> However, 10 byte is consumed for every instance of
>>> "t__hole_symbol_XXXXX".
>>>
>>> This is much less efficient thanI had expected,
>>> although I did not analyze the logic of this inefficiency.
>>>
>> Hi, Masahiro!
>>
>> In my local test, "t__hole_symbol_XXXXX" was finally encoded
>> into just one byte. See "kallsyms_token_table" in the .tmp_vmlinux.kallsyms2.S:
>>
>> kallsyms_token_table:
>> [...]
>> .asciz "t__hole_symbol_XXXXX"
>> .asciz "hole_symbol_XXXXX"
>> .asciz "e_symbol_XXXXX"
>> .asciz "XXXXX"
>> .asciz "XXX"
>> .asciz "e_symbol_"
>> .asciz "ymbol_"
>> .asciz "ymb"
>> .asciz "hol"
>> .asciz "ol_"
>> .asciz "pfx"
>> .asciz "pf"
>> .asciz "e_s"
>> .asciz "ym"
>> .asciz "t__"
>> .asciz "_s"
>> .asciz "ol"
>> .asciz "__"
>> .asciz "XX"
>>
>> But it would still takes up several tokens due to substrings of
>> "t__hole_symbol_XXXXX" would also become the most common ones.
>> After this patch, the number of "t__hole_symbol_XXXXX" will be ~30% of the total.
>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>> ---
>>>> scripts/kallsyms.c | 101 +++++++++++++++++++++++++++++++++++++++-
>>>> scripts/link-vmlinux.sh | 4 +-
>>>> scripts/mksysmap | 2 +-
>>>> 3 files changed, 102 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/scripts/kallsyms.c b/scripts/kallsyms.c
>>>> index 6559a9802f6e..5c4cde864a04 100644
>>>> --- a/scripts/kallsyms.c
>>>> +++ b/scripts/kallsyms.c
>>>> @@ -35,6 +35,7 @@
>>>> struct sym_entry {
>>>> struct sym_entry *next;
>>>> unsigned long long addr;
>>>> + unsigned long long size;
>>>> unsigned int len;
>>>> unsigned int seq;
>>>> unsigned int start_pos;
>>>> @@ -74,6 +75,7 @@ static int token_profit[0x10000];
>>>> static unsigned char best_table[256][2];
>>>> static unsigned char best_table_len[256];
>>>>
>>>> +static const char hole_symbol[] = "__hole_symbol_XXXXX";
>>>>
>>>> static void usage(void)
>>>> {
>>>> @@ -130,8 +132,16 @@ static struct sym_entry *read_symbol(FILE *in, char **buf, size_t *buf_len)
>>>> size_t len;
>>>> ssize_t readlen;
>>>> struct sym_entry *sym;
>>>> + unsigned long long size = 0;
>>>>
>>>> errno = 0;
>>>> + /*
>>>> + * Example of expected symbol format:
>>>> + * 1. symbol with size info:
>>>> + * ffffffff81000070 00000000000001d7 T __startup_64
>>>> + * 2. symbol without size info:
>>>> + * 0000000002a00000 A text_size
>>>> + */
>>>> readlen = getline(buf, buf_len, in);
>>>> if (readlen < 0) {
>>>> if (errno) {
>>>> @@ -145,9 +155,24 @@ static struct sym_entry *read_symbol(FILE *in, char **buf, size_t *buf_len)
>>>> (*buf)[readlen - 1] = 0;
>>>>
>>>> addr = strtoull(*buf, &p, 16);
>>>> + if (*buf == p || *p++ != ' ') {
>>>> + fprintf(stderr, "line format error: unable to parse address\n");
>>>> + exit(EXIT_FAILURE);
>>>> + }
>>>> +
>>>> + if (*p == '0') {
>>>> + char *str = p;
>>>>
>>>> - if (*buf == p || *p++ != ' ' || !isascii((type = *p++)) || *p++ != ' ') {
>>>> - fprintf(stderr, "line format error\n");
>>>> + size = strtoull(str, &p, 16);
>>>> + if (str == p || *p++ != ' ') {
>>>> + fprintf(stderr, "line format error: unable to parse size\n");
>>>> + exit(EXIT_FAILURE);
>>>> + }
>>>> + }
>>>> +
>>>> + type = *p++;
>>>> + if (!isascii(type) || *p++ != ' ') {
>>>> + fprintf(stderr, "line format error: unable to parse type\n");
>>>> exit(EXIT_FAILURE);
>>>> }
>>>>
>>>> @@ -182,6 +207,7 @@ static struct sym_entry *read_symbol(FILE *in, char **buf, size_t *buf_len)
>>>> exit(EXIT_FAILURE);
>>>> }
>>>> sym->addr = addr;
>>>> + sym->size = size;
>>>> sym->len = len;
>>>> sym->sym[0] = type;
>>>> strcpy(sym_name(sym), name);
>>>> @@ -795,6 +821,76 @@ static void sort_symbols(void)
>>>> qsort(table, table_cnt, sizeof(table[0]), compare_symbols);
>>>> }
>>>>
>>>> +static int may_exist_hole_after_symbol(const struct sym_entry *se)
>>>
>>>
>>> The return type should be bool.
>>>
>>
>> Yes!
>>
>>>
>>>
>>>> +{
>>>> + char type = se->sym[0];
>>>> +
>>>> + /* Only check text symbol or weak symbol */
>>>> + if (type != 't' && type != 'T' &&
>>>> + type != 'w' && type != 'W')
>>>> + return 0;
>>>> + /* Symbol without size has no hole */
>>>> + return se->size != 0;
>>>> +}
>>>> +
>>>> +static struct sym_entry *gen_hole_symbol(unsigned long long addr)
>>>> +{
>>>> + struct sym_entry *sym;
>>>> + static size_t len = sizeof(hole_symbol);
>>>> +
>>>> + /* include type field */
>>>> + sym = malloc(sizeof(*sym) + len + 1);
>>>> + if (!sym) {
>>>> + fprintf(stderr, "unable to allocate memory for hole symbol\n");
>>>> + exit(EXIT_FAILURE);
>>>> + }
>>>> + sym->addr = addr;
>>>> + sym->size = 0;
>>>> + sym->len = len;
>>>> + sym->sym[0] = 't';
>>>> + strcpy(sym_name(sym), hole_symbol);
>>>> + sym->percpu_absolute = 0;
>>>> + return sym;
>>>> +}
>>>> +
>>>> +static void emit_hole_symbols(void)
>>>> +{
>>>> + unsigned int i, pos, nr_emit;
>>>> + struct sym_entry **new_table;
>>>> + unsigned int new_cnt;
>>>> +
>>>> + nr_emit = 0;
>>>> + for (i = 0; i < table_cnt - 1; i++) {
>>>> + if (may_exist_hole_after_symbol(table[i]) &&
>>>> + table[i]->addr + table[i]->size < table[i+1]->addr)
>>>> + nr_emit++;
>>>> + }
>>>> + if (!nr_emit)
>>>> + return;
>>>> +
>>>> + new_cnt = table_cnt + nr_emit;
>>>> + new_table = malloc(sizeof(*new_table) * new_cnt);
>>>
>>>
>>> Do you need to allocate another huge table?
>>>
>>> You can use realloc() to append the room for nr_emit
>>> if you iterate the table in the reverse order.
>>>
>>
>> Yes, it would be much better. If it turns out to be the
>> "emit hole symbol" solution, I'll change it to that in the next version,
>> actually, I forgot to mark this series as "RFC".
>
>
> "__hole_symbol_XXXXX" is too much.
>
> You can use the empty symbol type/name as a special case
> to represent the hole.
>
Ok, I'll try it in v2.
>
--
Thanks,
Zheng Yejian
Powered by blists - more mailing lists