[PATCH] kallsyms: let kallsyms_on_each_match_symbol match symbols exactly

[PATCH] kallsyms: let kallsyms_on_each_match_symbol match symbols exactly

[Song Liu]

With CONFIG_LTO_CLANG, kallsyms.c:cleanup_symbol_name() removes symbols
suffixes during comparison. This is problematic for livepatch, as
kallsyms_on_each_match_symbol may find multiple matches for the same
symbol, and fail with:

  livepatch: unresolvable ambiguity for symbol 'xxx' in object 'yyy'

Make kallsyms_on_each_match_symbol() to match symbols exactly. Since
livepatch is the only user of kallsyms_on_each_match_symbol(), this
change is safe.

Signed-off-by: Song Liu <song@kernel.org>
---
 kernel/kallsyms.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index 77747391f49b..2ab459b43084 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -187,7 +187,7 @@ static bool cleanup_symbol_name(char *s)
 	return false;
 }
 
-static int compare_symbol_name(const char *name, char *namebuf)
+static int compare_symbol_name(const char *name, char *namebuf, bool match_exactly)
 {
 	int ret;
 
@@ -195,7 +195,7 @@ static int compare_symbol_name(const char *name, char *namebuf)
 	if (!ret)
 		return ret;
 
-	if (cleanup_symbol_name(namebuf) && !strcmp(name, namebuf))
+	if (!match_exactly && cleanup_symbol_name(namebuf) && !strcmp(name, namebuf))
 		return 0;
 
 	return ret;
@@ -213,7 +213,8 @@ static unsigned int get_symbol_seq(int index)
 
 static int kallsyms_lookup_names(const char *name,
 				 unsigned int *start,
-				 unsigned int *end)
+				 unsigned int *end,
+				 bool match_exactly)
 {
 	int ret;
 	int low, mid, high;
@@ -228,7 +229,7 @@ static int kallsyms_lookup_names(const char *name,
 		seq = get_symbol_seq(mid);
 		off = get_symbol_offset(seq);
 		kallsyms_expand_symbol(off, namebuf, ARRAY_SIZE(namebuf));
-		ret = compare_symbol_name(name, namebuf);
+		ret = compare_symbol_name(name, namebuf, match_exactly);
 		if (ret > 0)
 			low = mid + 1;
 		else if (ret < 0)
@@ -245,7 +246,7 @@ static int kallsyms_lookup_names(const char *name,
 		seq = get_symbol_seq(low - 1);
 		off = get_symbol_offset(seq);
 		kallsyms_expand_symbol(off, namebuf, ARRAY_SIZE(namebuf));
-		if (compare_symbol_name(name, namebuf))
+		if (compare_symbol_name(name, namebuf, match_exactly))
 			break;
 		low--;
 	}
@@ -257,7 +258,7 @@ static int kallsyms_lookup_names(const char *name,
 			seq = get_symbol_seq(high + 1);
 			off = get_symbol_offset(seq);
 			kallsyms_expand_symbol(off, namebuf, ARRAY_SIZE(namebuf));
-			if (compare_symbol_name(name, namebuf))
+			if (compare_symbol_name(name, namebuf, match_exactly))
 				break;
 			high++;
 		}
@@ -277,7 +278,7 @@ unsigned long kallsyms_lookup_name(const char *name)
 	if (!*name)
 		return 0;
 
-	ret = kallsyms_lookup_names(name, &i, NULL);
+	ret = kallsyms_lookup_names(name, &i, NULL, false);
 	if (!ret)
 		return kallsyms_sym_address(get_symbol_seq(i));
 
@@ -312,7 +313,7 @@ int kallsyms_on_each_match_symbol(int (*fn)(void *, unsigned long),
 	int ret;
 	unsigned int i, start, end;
 
-	ret = kallsyms_lookup_names(name, &start, &end);
+	ret = kallsyms_lookup_names(name, &start, &end, true);
 	if (ret)
 		return 0;
 
-- 
2.34.1

Re: [PATCH] kallsyms: let kallsyms_on_each_match_symbol match symbols exactly

[Leizhen (ThunderTown)]

On 2023/6/16 1:00, Song Liu wrote:
> With CONFIG_LTO_CLANG, kallsyms.c:cleanup_symbol_name() removes symbols
> suffixes during comparison. This is problematic for livepatch, as
> kallsyms_on_each_match_symbol may find multiple matches for the same
> symbol, and fail with:
> 
>   livepatch: unresolvable ambiguity for symbol 'xxx' in object 'yyy'

Did you forget to specify 'old_sympos'? When there are multiple symbols with
the same name, we need to specify the sequence number of the symbols to be
matched.

> 
> Make kallsyms_on_each_match_symbol() to match symbols exactly. Since
> livepatch is the only user of kallsyms_on_each_match_symbol(), this
> change is safe.
> 
> Signed-off-by: Song Liu <song@kernel.org>
> ---
>  kernel/kallsyms.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> index 77747391f49b..2ab459b43084 100644
> --- a/kernel/kallsyms.c
> +++ b/kernel/kallsyms.c
> @@ -187,7 +187,7 @@ static bool cleanup_symbol_name(char *s)
>  	return false;
>  }
>  
> -static int compare_symbol_name(const char *name, char *namebuf)
> +static int compare_symbol_name(const char *name, char *namebuf, bool match_exactly)
>  {
>  	int ret;
>  
> @@ -195,7 +195,7 @@ static int compare_symbol_name(const char *name, char *namebuf)
>  	if (!ret)
>  		return ret;
>  
> -	if (cleanup_symbol_name(namebuf) && !strcmp(name, namebuf))
> +	if (!match_exactly && cleanup_symbol_name(namebuf) && !strcmp(name, namebuf))
>  		return 0;
>  
>  	return ret;
> @@ -213,7 +213,8 @@ static unsigned int get_symbol_seq(int index)
>  
>  static int kallsyms_lookup_names(const char *name,
>  				 unsigned int *start,
> -				 unsigned int *end)
> +				 unsigned int *end,
> +				 bool match_exactly)
>  {
>  	int ret;
>  	int low, mid, high;
> @@ -228,7 +229,7 @@ static int kallsyms_lookup_names(const char *name,
>  		seq = get_symbol_seq(mid);
>  		off = get_symbol_offset(seq);
>  		kallsyms_expand_symbol(off, namebuf, ARRAY_SIZE(namebuf));
> -		ret = compare_symbol_name(name, namebuf);
> +		ret = compare_symbol_name(name, namebuf, match_exactly);
>  		if (ret > 0)
>  			low = mid + 1;
>  		else if (ret < 0)
> @@ -245,7 +246,7 @@ static int kallsyms_lookup_names(const char *name,
>  		seq = get_symbol_seq(low - 1);
>  		off = get_symbol_offset(seq);
>  		kallsyms_expand_symbol(off, namebuf, ARRAY_SIZE(namebuf));
> -		if (compare_symbol_name(name, namebuf))
> +		if (compare_symbol_name(name, namebuf, match_exactly))
>  			break;
>  		low--;
>  	}
> @@ -257,7 +258,7 @@ static int kallsyms_lookup_names(const char *name,
>  			seq = get_symbol_seq(high + 1);
>  			off = get_symbol_offset(seq);
>  			kallsyms_expand_symbol(off, namebuf, ARRAY_SIZE(namebuf));
> -			if (compare_symbol_name(name, namebuf))
> +			if (compare_symbol_name(name, namebuf, match_exactly))
>  				break;
>  			high++;
>  		}
> @@ -277,7 +278,7 @@ unsigned long kallsyms_lookup_name(const char *name)
>  	if (!*name)
>  		return 0;
>  
> -	ret = kallsyms_lookup_names(name, &i, NULL);
> +	ret = kallsyms_lookup_names(name, &i, NULL, false);
>  	if (!ret)
>  		return kallsyms_sym_address(get_symbol_seq(i));
>  
> @@ -312,7 +313,7 @@ int kallsyms_on_each_match_symbol(int (*fn)(void *, unsigned long),
>  	int ret;
>  	unsigned int i, start, end;
>  
> -	ret = kallsyms_lookup_names(name, &start, &end);
> +	ret = kallsyms_lookup_names(name, &start, &end, true);
>  	if (ret)
>  		return 0;
>  
> 

-- 
Regards,
  Zhen Lei

Re: [PATCH] kallsyms: let kallsyms_on_each_match_symbol match symbols exactly

[Song Liu]

> On Jun 15, 2023, at 7:19 PM, Leizhen (ThunderTown) <thunder.leizhen@huawei.com> wrote:
> 
> On 2023/6/16 1:00, Song Liu wrote:
>> With CONFIG_LTO_CLANG, kallsyms.c:cleanup_symbol_name() removes symbols
>> suffixes during comparison. This is problematic for livepatch, as
>> kallsyms_on_each_match_symbol may find multiple matches for the same
>> symbol, and fail with:
>> 
>>  livepatch: unresolvable ambiguity for symbol 'xxx' in object 'yyy'
> 
> Did you forget to specify 'old_sympos'? When there are multiple symbols with
> the same name, we need to specify the sequence number of the symbols to be
> matched.


old_sympos is indeed 0 here. However, the issue with CONFIG_LTO_CLANG 
is different. Here is an example:

$ grep bpf_verifier_vlog /proc/kallsyms
ffffffff81549f60 t bpf_verifier_vlog
ffffffff8268b430 d bpf_verifier_vlog._entry
ffffffff8282a958 d bpf_verifier_vlog._entry_ptr
ffffffff82e12a1f d bpf_verifier_vlog.__already_done

kallsyms_on_each_match_symbol matches "bpf_verifier_vlog" to all of 
these because of cleanup_symbol_name(). IOW, we only have one 
function called bpf_verifier_vlog, but kallsyms_on_each_match_symbol() 
matches it to bpf_verifier_vlog.*. 

Does this make sense?

Thanks,
Song

Re: [PATCH] kallsyms: let kallsyms_on_each_match_symbol match symbols exactly

[Leizhen (ThunderTown)]

On 2023/6/16 13:01, Song Liu wrote:
> 
> 
>> On Jun 15, 2023, at 7:19 PM, Leizhen (ThunderTown) <thunder.leizhen@huawei.com> wrote:
>>
>> On 2023/6/16 1:00, Song Liu wrote:
>>> With CONFIG_LTO_CLANG, kallsyms.c:cleanup_symbol_name() removes symbols
>>> suffixes during comparison. This is problematic for livepatch, as
>>> kallsyms_on_each_match_symbol may find multiple matches for the same
>>> symbol, and fail with:
>>>
>>>  livepatch: unresolvable ambiguity for symbol 'xxx' in object 'yyy'
>>
>> Did you forget to specify 'old_sympos'? When there are multiple symbols with
>> the same name, we need to specify the sequence number of the symbols to be
>> matched.
> 
> 
> old_sympos is indeed 0 here. However, the issue with CONFIG_LTO_CLANG 
> is different. Here is an example:
> 
> $ grep bpf_verifier_vlog /proc/kallsyms
> ffffffff81549f60 t bpf_verifier_vlog
> ffffffff8268b430 d bpf_verifier_vlog._entry
> ffffffff8282a958 d bpf_verifier_vlog._entry_ptr
> ffffffff82e12a1f d bpf_verifier_vlog.__already_done
> 
> kallsyms_on_each_match_symbol matches "bpf_verifier_vlog" to all of 
> these because of cleanup_symbol_name(). IOW, we only have one 
> function called bpf_verifier_vlog, but kallsyms_on_each_match_symbol() 
> matches it to bpf_verifier_vlog.*. 
> 
> Does this make sense?

Sorry. I mistakenly thought you were operating a static function.

These suffixes are not mentioned in the comments in the function
cleanup_symbol_name(). So I didn't notice it.

> 
> Thanks,
> Song
> 
> 
> .
> 

-- 
Regards,
  Zhen Lei

Re: [PATCH] kallsyms: let kallsyms_on_each_match_symbol match symbols exactly

[Leizhen (ThunderTown)]

On 2023/6/16 16:11, Leizhen (ThunderTown) wrote:
> 
> 
> On 2023/6/16 13:01, Song Liu wrote:
>>
>>
>>> On Jun 15, 2023, at 7:19 PM, Leizhen (ThunderTown) <thunder.leizhen@huawei.com> wrote:
>>>
>>> On 2023/6/16 1:00, Song Liu wrote:
>>>> With CONFIG_LTO_CLANG, kallsyms.c:cleanup_symbol_name() removes symbols
>>>> suffixes during comparison. This is problematic for livepatch, as
>>>> kallsyms_on_each_match_symbol may find multiple matches for the same
>>>> symbol, and fail with:
>>>>
>>>>  livepatch: unresolvable ambiguity for symbol 'xxx' in object 'yyy'
>>>
>>> Did you forget to specify 'old_sympos'? When there are multiple symbols with
>>> the same name, we need to specify the sequence number of the symbols to be
>>> matched.
>>
>>
>> old_sympos is indeed 0 here. However, the issue with CONFIG_LTO_CLANG 
>> is different. Here is an example:
>>
>> $ grep bpf_verifier_vlog /proc/kallsyms
>> ffffffff81549f60 t bpf_verifier_vlog
>> ffffffff8268b430 d bpf_verifier_vlog._entry
>> ffffffff8282a958 d bpf_verifier_vlog._entry_ptr
>> ffffffff82e12a1f d bpf_verifier_vlog.__already_done
>>
>> kallsyms_on_each_match_symbol matches "bpf_verifier_vlog" to all of 
>> these because of cleanup_symbol_name(). IOW, we only have one 
>> function called bpf_verifier_vlog, but kallsyms_on_each_match_symbol() 
>> matches it to bpf_verifier_vlog.*. 
>>
>> Does this make sense?
> 
> Sorry. I mistakenly thought you were operating a static function.
> 
> These suffixes are not mentioned in the comments in the function
> cleanup_symbol_name(). So I didn't notice it.
We can keep these three suffixes on the kallsyms tool end.

> 
>>
>> Thanks,
>> Song
>>
>>
>> .
>>
> 

-- 
Regards,
  Zhen Lei

Re: [PATCH] kallsyms: let kallsyms_on_each_match_symbol match symbols exactly

[Leizhen (ThunderTown)]

On 2023/6/16 16:43, Leizhen (ThunderTown) wrote:
> 
> 
> On 2023/6/16 16:11, Leizhen (ThunderTown) wrote:
>>
>>
>> On 2023/6/16 13:01, Song Liu wrote:
>>>
>>>
>>>> On Jun 15, 2023, at 7:19 PM, Leizhen (ThunderTown) <thunder.leizhen@huawei.com> wrote:
>>>>
>>>> On 2023/6/16 1:00, Song Liu wrote:
>>>>> With CONFIG_LTO_CLANG, kallsyms.c:cleanup_symbol_name() removes symbols
>>>>> suffixes during comparison. This is problematic for livepatch, as
>>>>> kallsyms_on_each_match_symbol may find multiple matches for the same
>>>>> symbol, and fail with:
>>>>>
>>>>>  livepatch: unresolvable ambiguity for symbol 'xxx' in object 'yyy'
>>>>
>>>> Did you forget to specify 'old_sympos'? When there are multiple symbols with
>>>> the same name, we need to specify the sequence number of the symbols to be
>>>> matched.
>>>
>>>
>>> old_sympos is indeed 0 here. However, the issue with CONFIG_LTO_CLANG 
>>> is different. Here is an example:
>>>
>>> $ grep bpf_verifier_vlog /proc/kallsyms
>>> ffffffff81549f60 t bpf_verifier_vlog
>>> ffffffff8268b430 d bpf_verifier_vlog._entry
>>> ffffffff8282a958 d bpf_verifier_vlog._entry_ptr
>>> ffffffff82e12a1f d bpf_verifier_vlog.__already_done
>>>
>>> kallsyms_on_each_match_symbol matches "bpf_verifier_vlog" to all of 
>>> these because of cleanup_symbol_name(). IOW, we only have one 
>>> function called bpf_verifier_vlog, but kallsyms_on_each_match_symbol() 
>>> matches it to bpf_verifier_vlog.*. 
>>>
>>> Does this make sense?
>>
>> Sorry. I mistakenly thought you were operating a static function.
>>
>> These suffixes are not mentioned in the comments in the function
>> cleanup_symbol_name(). So I didn't notice it.
> We can keep these three suffixes on the kallsyms tool end.

And modify cleanup_symbol_name() not to cleanup these three suffixes.

> 
>>
>>>
>>> Thanks,
>>> Song
>>>
>>>
>>> .
>>>
>>
> 

-- 
Regards,
  Zhen Lei

Re: [PATCH] kallsyms: let kallsyms_on_each_match_symbol match symbols exactly

[Song Liu]

> On Jun 16, 2023, at 1:52 AM, Leizhen (ThunderTown) <thunder.leizhen@huawei.com> wrote:
> 
> 
> 
> On 2023/6/16 16:43, Leizhen (ThunderTown) wrote:
>> 
>> 
>> On 2023/6/16 16:11, Leizhen (ThunderTown) wrote:
>>> 
>>> 
>>> On 2023/6/16 13:01, Song Liu wrote:
>>>> 
>>>> 
>>>>> On Jun 15, 2023, at 7:19 PM, Leizhen (ThunderTown) <thunder.leizhen@huawei.com> wrote:
>>>>> 
>>>>> On 2023/6/16 1:00, Song Liu wrote:
>>>>>> With CONFIG_LTO_CLANG, kallsyms.c:cleanup_symbol_name() removes symbols
>>>>>> suffixes during comparison. This is problematic for livepatch, as
>>>>>> kallsyms_on_each_match_symbol may find multiple matches for the same
>>>>>> symbol, and fail with:
>>>>>> 
>>>>>> livepatch: unresolvable ambiguity for symbol 'xxx' in object 'yyy'
>>>>> 
>>>>> Did you forget to specify 'old_sympos'? When there are multiple symbols with
>>>>> the same name, we need to specify the sequence number of the symbols to be
>>>>> matched.
>>>> 
>>>> 
>>>> old_sympos is indeed 0 here. However, the issue with CONFIG_LTO_CLANG 
>>>> is different. Here is an example:
>>>> 
>>>> $ grep bpf_verifier_vlog /proc/kallsyms
>>>> ffffffff81549f60 t bpf_verifier_vlog
>>>> ffffffff8268b430 d bpf_verifier_vlog._entry
>>>> ffffffff8282a958 d bpf_verifier_vlog._entry_ptr
>>>> ffffffff82e12a1f d bpf_verifier_vlog.__already_done
>>>> 
>>>> kallsyms_on_each_match_symbol matches "bpf_verifier_vlog" to all of 
>>>> these because of cleanup_symbol_name(). IOW, we only have one 
>>>> function called bpf_verifier_vlog, but kallsyms_on_each_match_symbol() 
>>>> matches it to bpf_verifier_vlog.*. 
>>>> 
>>>> Does this make sense?
>>> 
>>> Sorry. I mistakenly thought you were operating a static function.
>>> 
>>> These suffixes are not mentioned in the comments in the function
>>> cleanup_symbol_name(). So I didn't notice it.
>> We can keep these three suffixes on the kallsyms tool end.
> 
> And modify cleanup_symbol_name() not to cleanup these three suffixes.

I think livepatch should match symbols exactly anyway, so it is not 
necessary to expose more details in cleanup_symbol_name()?

Thanks,
Song

Re: [PATCH] kallsyms: let kallsyms_on_each_match_symbol match symbols exactly

[Leizhen (ThunderTown)]

On 2023/6/16 1:00, Song Liu wrote:
> With CONFIG_LTO_CLANG, kallsyms.c:cleanup_symbol_name() removes symbols
> suffixes during comparison. This is problematic for livepatch, as
> kallsyms_on_each_match_symbol may find multiple matches for the same
> symbol, and fail with:
> 
>   livepatch: unresolvable ambiguity for symbol 'xxx' in object 'yyy'
> 
> Make kallsyms_on_each_match_symbol() to match symbols exactly. Since
> livepatch is the only user of kallsyms_on_each_match_symbol(), this
> change is safe.
> 
> Signed-off-by: Song Liu <song@kernel.org>
> ---
>  kernel/kallsyms.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> index 77747391f49b..2ab459b43084 100644
> --- a/kernel/kallsyms.c
> +++ b/kernel/kallsyms.c
> @@ -187,7 +187,7 @@ static bool cleanup_symbol_name(char *s)
>  	return false;
>  }
>  
> -static int compare_symbol_name(const char *name, char *namebuf)
> +static int compare_symbol_name(const char *name, char *namebuf, bool match_exactly)
>  {
>  	int ret;
>  
> @@ -195,7 +195,7 @@ static int compare_symbol_name(const char *name, char *namebuf)
>  	if (!ret)
>  		return ret;
>  
> -	if (cleanup_symbol_name(namebuf) && !strcmp(name, namebuf))
> +	if (!match_exactly && cleanup_symbol_name(namebuf) && !strcmp(name, namebuf))

This may affect the lookup of static functions.

>  		return 0;
>  
>  	return ret;
> @@ -213,7 +213,8 @@ static unsigned int get_symbol_seq(int index)
>  
>  static int kallsyms_lookup_names(const char *name,
>  				 unsigned int *start,
> -				 unsigned int *end)
> +				 unsigned int *end,
> +				 bool match_exactly)
>  {
>  	int ret;
>  	int low, mid, high;
> @@ -228,7 +229,7 @@ static int kallsyms_lookup_names(const char *name,
>  		seq = get_symbol_seq(mid);
>  		off = get_symbol_offset(seq);
>  		kallsyms_expand_symbol(off, namebuf, ARRAY_SIZE(namebuf));
> -		ret = compare_symbol_name(name, namebuf);
> +		ret = compare_symbol_name(name, namebuf, match_exactly);
>  		if (ret > 0)
>  			low = mid + 1;
>  		else if (ret < 0)
> @@ -245,7 +246,7 @@ static int kallsyms_lookup_names(const char *name,
>  		seq = get_symbol_seq(low - 1);
>  		off = get_symbol_offset(seq);
>  		kallsyms_expand_symbol(off, namebuf, ARRAY_SIZE(namebuf));
> -		if (compare_symbol_name(name, namebuf))
> +		if (compare_symbol_name(name, namebuf, match_exactly))
>  			break;
>  		low--;
>  	}
> @@ -257,7 +258,7 @@ static int kallsyms_lookup_names(const char *name,
>  			seq = get_symbol_seq(high + 1);
>  			off = get_symbol_offset(seq);
>  			kallsyms_expand_symbol(off, namebuf, ARRAY_SIZE(namebuf));
> -			if (compare_symbol_name(name, namebuf))
> +			if (compare_symbol_name(name, namebuf, match_exactly))
>  				break;
>  			high++;
>  		}
> @@ -277,7 +278,7 @@ unsigned long kallsyms_lookup_name(const char *name)
>  	if (!*name)
>  		return 0;
>  
> -	ret = kallsyms_lookup_names(name, &i, NULL);
> +	ret = kallsyms_lookup_names(name, &i, NULL, false);
>  	if (!ret)
>  		return kallsyms_sym_address(get_symbol_seq(i));
>  
> @@ -312,7 +313,7 @@ int kallsyms_on_each_match_symbol(int (*fn)(void *, unsigned long),
>  	int ret;
>  	unsigned int i, start, end;
>  
> -	ret = kallsyms_lookup_names(name, &start, &end);
> +	ret = kallsyms_lookup_names(name, &start, &end, true);
>  	if (ret)
>  		return 0;
>  
> 

-- 
Regards,
  Zhen Lei

Re: [PATCH] kallsyms: let kallsyms_on_each_match_symbol match symbols exactly

[Song Liu]

> On Jun 16, 2023, at 2:31 AM, Leizhen (ThunderTown) <thunder.leizhen@huawei.com> wrote:
> 
> 
> 
> On 2023/6/16 1:00, Song Liu wrote:
>> With CONFIG_LTO_CLANG, kallsyms.c:cleanup_symbol_name() removes symbols
>> suffixes during comparison. This is problematic for livepatch, as
>> kallsyms_on_each_match_symbol may find multiple matches for the same
>> symbol, and fail with:
>> 
>>  livepatch: unresolvable ambiguity for symbol 'xxx' in object 'yyy'
>> 
>> Make kallsyms_on_each_match_symbol() to match symbols exactly. Since
>> livepatch is the only user of kallsyms_on_each_match_symbol(), this
>> change is safe.
>> 
>> Signed-off-by: Song Liu <song@kernel.org>
>> ---
>> kernel/kallsyms.c | 17 +++++++++--------
>> 1 file changed, 9 insertions(+), 8 deletions(-)
>> 
>> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
>> index 77747391f49b..2ab459b43084 100644
>> --- a/kernel/kallsyms.c
>> +++ b/kernel/kallsyms.c
>> @@ -187,7 +187,7 @@ static bool cleanup_symbol_name(char *s)
>> return false;
>> }
>> 
>> -static int compare_symbol_name(const char *name, char *namebuf)
>> +static int compare_symbol_name(const char *name, char *namebuf, bool match_exactly)
>> {
>> int ret;
>> 
>> @@ -195,7 +195,7 @@ static int compare_symbol_name(const char *name, char *namebuf)
>> if (!ret)
>> return ret;
>> 
>> - if (cleanup_symbol_name(namebuf) && !strcmp(name, namebuf))
>> + if (!match_exactly && cleanup_symbol_name(namebuf) && !strcmp(name, namebuf))
> 
> This may affect the lookup of static functions.

I am not following why would this be a problem. Could you give an 
example of it?

Thanks,
Song

Re: [PATCH] kallsyms: let kallsyms_on_each_match_symbol match symbols exactly

[Leizhen (ThunderTown)]

On 2023/6/17 1:37, Song Liu wrote:
> 
> 
>> On Jun 16, 2023, at 2:31 AM, Leizhen (ThunderTown) <thunder.leizhen@huawei.com> wrote:
>>
>>
>>
>> On 2023/6/16 1:00, Song Liu wrote:
>>> With CONFIG_LTO_CLANG, kallsyms.c:cleanup_symbol_name() removes symbols
>>> suffixes during comparison. This is problematic for livepatch, as
>>> kallsyms_on_each_match_symbol may find multiple matches for the same
>>> symbol, and fail with:
>>>
>>>  livepatch: unresolvable ambiguity for symbol 'xxx' in object 'yyy'
>>>
>>> Make kallsyms_on_each_match_symbol() to match symbols exactly. Since
>>> livepatch is the only user of kallsyms_on_each_match_symbol(), this
>>> change is safe.
>>>
>>> Signed-off-by: Song Liu <song@kernel.org>
>>> ---
>>> kernel/kallsyms.c | 17 +++++++++--------
>>> 1 file changed, 9 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
>>> index 77747391f49b..2ab459b43084 100644
>>> --- a/kernel/kallsyms.c
>>> +++ b/kernel/kallsyms.c
>>> @@ -187,7 +187,7 @@ static bool cleanup_symbol_name(char *s)
>>> return false;
>>> }
>>>
>>> -static int compare_symbol_name(const char *name, char *namebuf)
>>> +static int compare_symbol_name(const char *name, char *namebuf, bool match_exactly)
>>> {
>>> int ret;
>>>
>>> @@ -195,7 +195,7 @@ static int compare_symbol_name(const char *name, char *namebuf)
>>> if (!ret)
>>> return ret;
>>>
>>> - if (cleanup_symbol_name(namebuf) && !strcmp(name, namebuf))
>>> + if (!match_exactly && cleanup_symbol_name(namebuf) && !strcmp(name, namebuf))
>>
>> This may affect the lookup of static functions.
> 
> I am not following why would this be a problem. Could you give an 
> example of it?

Here are the comments in cleanup_symbol_name(). If the compiler adds a suffix to the
static function, but we do not remove the suffix, will the symbol match fail?

	/*
	 * LLVM appends various suffixes for local functions and variables that
	 * must be promoted to global scope as part of LTO.  This can break
	 * hooking of static functions with kprobes. '.' is not a valid
	 * character in an identifier in C. Suffixes observed:
	 * - foo.llvm.[0-9a-f]+
	 * - foo.[0-9a-f]+
	 */

> 
> Thanks,
> Song
> 
> .
> 

-- 
Regards,
  Zhen Lei

Re: [PATCH] kallsyms: let kallsyms_on_each_match_symbol match symbols exactly

[Song Liu]

On Sun, Jun 18, 2023 at 8:32 PM Leizhen (ThunderTown)
<thunder.leizhen@huawei.com> wrote:
>
>
>
> On 2023/6/17 1:37, Song Liu wrote:
> >
> >
> >> On Jun 16, 2023, at 2:31 AM, Leizhen (ThunderTown) <thunder.leizhen@huawei.com> wrote:
> >>
> >>
> >>
> >> On 2023/6/16 1:00, Song Liu wrote:
> >>> With CONFIG_LTO_CLANG, kallsyms.c:cleanup_symbol_name() removes symbols
> >>> suffixes during comparison. This is problematic for livepatch, as
> >>> kallsyms_on_each_match_symbol may find multiple matches for the same
> >>> symbol, and fail with:
> >>>
> >>>  livepatch: unresolvable ambiguity for symbol 'xxx' in object 'yyy'
> >>>
> >>> Make kallsyms_on_each_match_symbol() to match symbols exactly. Since
> >>> livepatch is the only user of kallsyms_on_each_match_symbol(), this
> >>> change is safe.
> >>>
> >>> Signed-off-by: Song Liu <song@kernel.org>
> >>> ---
> >>> kernel/kallsyms.c | 17 +++++++++--------
> >>> 1 file changed, 9 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> >>> index 77747391f49b..2ab459b43084 100644
> >>> --- a/kernel/kallsyms.c
> >>> +++ b/kernel/kallsyms.c
> >>> @@ -187,7 +187,7 @@ static bool cleanup_symbol_name(char *s)
> >>> return false;
> >>> }
> >>>
> >>> -static int compare_symbol_name(const char *name, char *namebuf)
> >>> +static int compare_symbol_name(const char *name, char *namebuf, bool match_exactly)
> >>> {
> >>> int ret;
> >>>
> >>> @@ -195,7 +195,7 @@ static int compare_symbol_name(const char *name, char *namebuf)
> >>> if (!ret)
> >>> return ret;
> >>>
> >>> - if (cleanup_symbol_name(namebuf) && !strcmp(name, namebuf))
> >>> + if (!match_exactly && cleanup_symbol_name(namebuf) && !strcmp(name, namebuf))
> >>
> >> This may affect the lookup of static functions.
> >
> > I am not following why would this be a problem. Could you give an
> > example of it?
>
> Here are the comments in cleanup_symbol_name(). If the compiler adds a suffix to the
> static function, but we do not remove the suffix, will the symbol match fail?
>
>         /*
>          * LLVM appends various suffixes for local functions and variables that
>          * must be promoted to global scope as part of LTO.  This can break
>          * hooking of static functions with kprobes. '.' is not a valid
>          * character in an identifier in C. Suffixes observed:
>          * - foo.llvm.[0-9a-f]+
>          * - foo.[0-9a-f]+
>          */

I think livepatch will not fail, as the tool chain should already match the
suffix for the function being patched. If the tool chain failed to do so,
livepatch can fail for other reasons (missing symbols, etc.)

Thanks,
Song

Re: [PATCH] kallsyms: let kallsyms_on_each_match_symbol match symbols exactly

[Petr Mladek]

On Sun 2023-06-18 22:05:19, Song Liu wrote:
> On Sun, Jun 18, 2023 at 8:32 PM Leizhen (ThunderTown)
> <thunder.leizhen@huawei.com> wrote:
> >
> >
> >
> > On 2023/6/17 1:37, Song Liu wrote:
> > >
> > >
> > >> On Jun 16, 2023, at 2:31 AM, Leizhen (ThunderTown) <thunder.leizhen@huawei.com> wrote:
> > >>
> > >>
> > >>
> > >> On 2023/6/16 1:00, Song Liu wrote:
> > >>> With CONFIG_LTO_CLANG, kallsyms.c:cleanup_symbol_name() removes symbols
> > >>> suffixes during comparison. This is problematic for livepatch, as
> > >>> kallsyms_on_each_match_symbol may find multiple matches for the same
> > >>> symbol, and fail with:
> > >>>
> > >>>  livepatch: unresolvable ambiguity for symbol 'xxx' in object 'yyy'
> > >>>
> > >>> Make kallsyms_on_each_match_symbol() to match symbols exactly. Since
> > >>> livepatch is the only user of kallsyms_on_each_match_symbol(), this
> > >>> change is safe.
> > >>>
> > >>> Signed-off-by: Song Liu <song@kernel.org>
> > >>> ---
> > >>> kernel/kallsyms.c | 17 +++++++++--------
> > >>> 1 file changed, 9 insertions(+), 8 deletions(-)
> > >>>
> > >>> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> > >>> index 77747391f49b..2ab459b43084 100644
> > >>> --- a/kernel/kallsyms.c
> > >>> +++ b/kernel/kallsyms.c
> > >>> @@ -187,7 +187,7 @@ static bool cleanup_symbol_name(char *s)
> > >>> return false;
> > >>> }
> > >>>
> > >>> -static int compare_symbol_name(const char *name, char *namebuf)
> > >>> +static int compare_symbol_name(const char *name, char *namebuf, bool match_exactly)
> > >>> {
> > >>> int ret;
> > >>>
> > >>> @@ -195,7 +195,7 @@ static int compare_symbol_name(const char *name, char *namebuf)
> > >>> if (!ret)
> > >>> return ret;
> > >>>
> > >>> - if (cleanup_symbol_name(namebuf) && !strcmp(name, namebuf))
> > >>> + if (!match_exactly && cleanup_symbol_name(namebuf) && !strcmp(name, namebuf))
> > >>
> > >> This may affect the lookup of static functions.
> > >
> > > I am not following why would this be a problem. Could you give an
> > > example of it?
> >
> > Here are the comments in cleanup_symbol_name(). If the compiler adds a suffix to the
> > static function, but we do not remove the suffix, will the symbol match fail?
> >
> >         /*
> >          * LLVM appends various suffixes for local functions and variables that
> >          * must be promoted to global scope as part of LTO.  This can break
> >          * hooking of static functions with kprobes. '.' is not a valid
> >          * character in an identifier in C. Suffixes observed:
> >          * - foo.llvm.[0-9a-f]+
> >          * - foo.[0-9a-f]+
> >          */
> 
> I think livepatch will not fail, as the tool chain should already match the
> suffix for the function being patched. If the tool chain failed to do so,
> livepatch can fail for other reasons (missing symbols, etc.)

cleanup_symbol_name() has been added by the commit 8b8e6b5d3b013b0bd8
("kallsyms: strip ThinLTO hashes from static functions"). The
motivation is that user space tools pass the symbol names found
in sources. They do not know about the "random" suffix added
by the "random" compiler.

While livepatching might want to work with the full symbol names.
It helps to locate avoid duplication and find the right symbol.

At least, this should be beneficial for kpatch tool which works directly
with the generated symbols.

Well, in theory, the cleaned symbol names might be useful for
source-based livepatches. But there might be problem to
distinguish different symbols with the same name and symbols
duplicated because of inlining. Well, we tend to livepatch
the caller anyway.

Best Regards,
Petr

Re: [PATCH] kallsyms: let kallsyms_on_each_match_symbol match symbols exactly

[Song Liu]

> On Jun 19, 2023, at 4:32 AM, Petr Mladek <pmladek@suse.com> wrote:
> 
> On Sun 2023-06-18 22:05:19, Song Liu wrote:
>> On Sun, Jun 18, 2023 at 8:32 PM Leizhen (ThunderTown)
>> <thunder.leizhen@huawei.com> wrote:

[...]

>>>>>> 
>>>>>> @@ -195,7 +195,7 @@ static int compare_symbol_name(const char *name, char *namebuf)
>>>>>> if (!ret)
>>>>>> return ret;
>>>>>> 
>>>>>> - if (cleanup_symbol_name(namebuf) && !strcmp(name, namebuf))
>>>>>> + if (!match_exactly && cleanup_symbol_name(namebuf) && !strcmp(name, namebuf))
>>>>> 
>>>>> This may affect the lookup of static functions.
>>>> 
>>>> I am not following why would this be a problem. Could you give an
>>>> example of it?
>>> 
>>> Here are the comments in cleanup_symbol_name(). If the compiler adds a suffix to the
>>> static function, but we do not remove the suffix, will the symbol match fail?
>>> 
>>>        /*
>>>         * LLVM appends various suffixes for local functions and variables that
>>>         * must be promoted to global scope as part of LTO.  This can break
>>>         * hooking of static functions with kprobes. '.' is not a valid
>>>         * character in an identifier in C. Suffixes observed:
>>>         * - foo.llvm.[0-9a-f]+
>>>         * - foo.[0-9a-f]+
>>>         */
>> 
>> I think livepatch will not fail, as the tool chain should already match the
>> suffix for the function being patched. If the tool chain failed to do so,
>> livepatch can fail for other reasons (missing symbols, etc.)
> 
> cleanup_symbol_name() has been added by the commit 8b8e6b5d3b013b0bd8
> ("kallsyms: strip ThinLTO hashes from static functions"). The
> motivation is that user space tools pass the symbol names found
> in sources. They do not know about the "random" suffix added
> by the "random" compiler.

I am not quite sure how tracing tools would work without knowing about
what the compiler did to the code. But I guess we are not addressing
that part here. 

> 
> While livepatching might want to work with the full symbol names.
> It helps to locate avoid duplication and find the right symbol.
> 
> At least, this should be beneficial for kpatch tool which works directly
> with the generated symbols.
> 
> Well, in theory, the cleaned symbol names might be useful for
> source-based livepatches. But there might be problem to
> distinguish different symbols with the same name and symbols
> duplicated because of inlining. Well, we tend to livepatch
> the caller anyway.

I am not quite following the direction here. Do we need more 
work for this patch?

Thanks,
Song

Re: [PATCH] kallsyms: let kallsyms_on_each_match_symbol match symbols exactly

[Petr Mladek]

On Tue 2023-06-20 22:36:14, Song Liu wrote:
> > On Jun 19, 2023, at 4:32 AM, Petr Mladek <pmladek@suse.com> wrote:
> > 
> > On Sun 2023-06-18 22:05:19, Song Liu wrote:
> >> On Sun, Jun 18, 2023 at 8:32 PM Leizhen (ThunderTown)
> >> <thunder.leizhen@huawei.com> wrote:
> 
> [...]
> 
> >>>>>> 
> >>>>>> @@ -195,7 +195,7 @@ static int compare_symbol_name(const char *name, char *namebuf)
> >>>>>> if (!ret)
> >>>>>> return ret;
> >>>>>> 
> >>>>>> - if (cleanup_symbol_name(namebuf) && !strcmp(name, namebuf))
> >>>>>> + if (!match_exactly && cleanup_symbol_name(namebuf) && !strcmp(name, namebuf))
> >>>>> 
> >>>>> This may affect the lookup of static functions.
> >>>> 
> >>>> I am not following why would this be a problem. Could you give an
> >>>> example of it?
> >>> 
> >>> Here are the comments in cleanup_symbol_name(). If the compiler adds a suffix to the
> >>> static function, but we do not remove the suffix, will the symbol match fail?
> >>> 
> >>>        /*
> >>>         * LLVM appends various suffixes for local functions and variables that
> >>>         * must be promoted to global scope as part of LTO.  This can break
> >>>         * hooking of static functions with kprobes. '.' is not a valid
> >>>         * character in an identifier in C. Suffixes observed:
> >>>         * - foo.llvm.[0-9a-f]+
> >>>         * - foo.[0-9a-f]+
> >>>         */
> >> 
> >> I think livepatch will not fail, as the tool chain should already match the
> >> suffix for the function being patched. If the tool chain failed to do so,
> >> livepatch can fail for other reasons (missing symbols, etc.)
> > 
> > cleanup_symbol_name() has been added by the commit 8b8e6b5d3b013b0bd8
> > ("kallsyms: strip ThinLTO hashes from static functions"). The
> > motivation is that user space tools pass the symbol names found
> > in sources. They do not know about the "random" suffix added
> > by the "random" compiler.
> 
> I am not quite sure how tracing tools would work without knowing about
> what the compiler did to the code. But I guess we are not addressing
> that part here.

I expect that the tracers access only symbols that are unique even
after removing the extra suffix.

Otherwise they would have the same problem even without suffix.
Each symbol create its own entry in kallsyms. There might be
more static symbols of the same name. This is why there is
"old_sympos" in struct klp_func. See also klp-convert,
https://lore.kernel.org/r/20230306140824.3858543-1-joe.lawrence@redhat.com

Fortunately, the same names are rare and "old_sympos" is used
only rarely. This is why it probably works for the tracers as well.


> > While livepatching might want to work with the full symbol names.
> > It helps to locate avoid duplication and find the right symbol.
> > 
> > At least, this should be beneficial for kpatch tool which works directly
> > with the generated symbols.
> > 
> > Well, in theory, the cleaned symbol names might be useful for
> > source-based livepatches. But there might be problem to
> > distinguish different symbols with the same name and symbols
> > duplicated because of inlining. Well, we tend to livepatch
> > the caller anyway.
> 
> I am not quite following the direction here. Do we need more 
> work for this patch?

Good question. I primary tried to add more details so that
we better understand the problem.

Honestly, I do not know the answer. I am neither familiar with
kpatch nor with clang. Let me think loudly.

kpatch produce livepatches by comparing binaries of the original
and fixed kernel. It adds a symbol into the livepatch when
the same symbol has different code in the two binaries.

So one important question is how clang generates the suffix.
Is the suffix the same in every build? Is it the same even
after the function gets modified by a fix?

If the suffix is always the same then then the full symbol name
would be better for kpatch. kpatch would get it for free.
And kpatch would not longer need to use "old_sympos".

But if the suffix is different then kpatch has a problem.
kpatch would need to match symbols with different suffixes.
It would be easy for symbols which are unique after removing
the suffix. But it would be tricky for comparing symbols
which do not have an unique name. kpatch would need to find
which suffix in the original binary matches an other suffix
in the fixed binary. In this case, it might be easier
to use the stripped symbol names.

And the suffix might be problematic also for source based
livepatches. They define struct klp_func in sources so
they would need to hardcode the suffix there. It might
be easy to keep using the stripped name and "old_sympos".

I expect that this patch actually breaks the livepatch
selftests when the kernel is compiled with clang LTO.

Best Regards,
Petr

Re: [PATCH] kallsyms: let kallsyms_on_each_match_symbol match symbols exactly

[Song Liu]

> On Jun 21, 2023, at 1:52 AM, Petr Mladek <pmladek@suse.com> wrote:
> 
> On Tue 2023-06-20 22:36:14, Song Liu wrote:
>>> On Jun 19, 2023, at 4:32 AM, Petr Mladek <pmladek@suse.com> wrote:
>>> 
>>> On Sun 2023-06-18 22:05:19, Song Liu wrote:
>>>> On Sun, Jun 18, 2023 at 8:32 PM Leizhen (ThunderTown)
>>>> <thunder.leizhen@huawei.com> wrote:
>> 
>> [...]

[...]

>> 
>> I am not quite following the direction here. Do we need more 
>> work for this patch?
> 
> Good question. I primary tried to add more details so that
> we better understand the problem.
> 
> Honestly, I do not know the answer. I am neither familiar with
> kpatch nor with clang. Let me think loudly.
> 
> kpatch produce livepatches by comparing binaries of the original
> and fixed kernel. It adds a symbol into the livepatch when
> the same symbol has different code in the two binaries.
> 
> So one important question is how clang generates the suffix.
> Is the suffix the same in every build? Is it the same even
> after the function gets modified by a fix?
> 
> If the suffix is always the same then then the full symbol name
> would be better for kpatch. kpatch would get it for free.
> And kpatch would not longer need to use "old_sympos".

This is pretty complicated. 

1. clang with LTO does not use the suffix to eliminated duplicated
kallsyms, so old_sympos is still needed. Here is an example:

# grep init_once /proc/kallsyms
ffffffff8120ba80 t init_once.llvm.14172910296636650566
ffffffff8120ba90 t inode_init_once
ffffffff813ea5d0 t bpf_user_rnd_init_once
ffffffff813fd5b0 t init_once.llvm.17912494158778303782
ffffffff813ffbf0 t init_once
ffffffff813ffc60 t init_once
ffffffff813ffc70 t init_once
ffffffff813ffcd0 t init_once
ffffffff813ffce0 t init_once

There are two "init_once" with suffix, but there are also ones 
without them. 

2. kpatch-build does "Building original source", "Building patched 
source", and then do binary diff of the two. From our experiments, 
the suffix doesn't change between the two builds. However, we need
to match the build environment (path of kernel source, etc.) to 
make sure suffix from kpatch matches the kernel. 

3. The goal of this patch is NOT to resolve different suffix by 
llvm (.llvm.[0-9]+). Instead, we are trying fix issues like:

#  grep bpf_verifier_vlog /proc/kallsyms
ffffffff81549f60 t bpf_verifier_vlog
ffffffff8268b430 d bpf_verifier_vlog._entry
ffffffff8282a958 d bpf_verifier_vlog._entry_ptr
ffffffff82e12a1f d bpf_verifier_vlog.__already_done

With existing code, compare_symbol_name() will match 
bpf_verifier_vlog to all these with CONFIG_LTO_CLANG.

We can probably teach compare_symbol_name() to not to match
these suffix, as Zhen suggested. 

If this is not ideal, I am open to suggestions that can solve
the problem. 

> But if the suffix is different then kpatch has a problem.
> kpatch would need to match symbols with different suffixes.
> It would be easy for symbols which are unique after removing
> the suffix. But it would be tricky for comparing symbols
> which do not have an unique name. kpatch would need to find
> which suffix in the original binary matches an other suffix
> in the fixed binary. In this case, it might be easier
> to use the stripped symbol names.
> 
> And the suffix might be problematic also for source based
> livepatches. They define struct klp_func in sources so
> they would need to hardcode the suffix there. It might
> be easy to keep using the stripped name and "old_sympos".
> 
> I expect that this patch actually breaks the livepatch
> selftests when the kernel is compiled with clang LTO.

Not really. This patch passes livepatch selftests. 

Thanks,
Song

Re: [PATCH] kallsyms: let kallsyms_on_each_match_symbol match symbols exactly

[Yonghong Song]

On 6/21/23 12:18 PM, Song Liu wrote:
> 
> 
>> On Jun 21, 2023, at 1:52 AM, Petr Mladek <pmladek@suse.com> wrote:
>>
>> On Tue 2023-06-20 22:36:14, Song Liu wrote:
>>>> On Jun 19, 2023, at 4:32 AM, Petr Mladek <pmladek@suse.com> wrote:
>>>>
>>>> On Sun 2023-06-18 22:05:19, Song Liu wrote:
>>>>> On Sun, Jun 18, 2023 at 8:32 PM Leizhen (ThunderTown)
>>>>> <thunder.leizhen@huawei.com> wrote:
>>>
>>> [...]
> 
> [...]
> 
>>>
>>> I am not quite following the direction here. Do we need more
>>> work for this patch?
>>
>> Good question. I primary tried to add more details so that
>> we better understand the problem.
>>
>> Honestly, I do not know the answer. I am neither familiar with
>> kpatch nor with clang. Let me think loudly.
>>
>> kpatch produce livepatches by comparing binaries of the original
>> and fixed kernel. It adds a symbol into the livepatch when
>> the same symbol has different code in the two binaries.
>>
>> So one important question is how clang generates the suffix.
>> Is the suffix the same in every build? Is it the same even
>> after the function gets modified by a fix?
>>
>> If the suffix is always the same then then the full symbol name
>> would be better for kpatch. kpatch would get it for free.
>> And kpatch would not longer need to use "old_sympos".
> 
> This is pretty complicated.
> 
> 1. clang with LTO does not use the suffix to eliminated duplicated
> kallsyms, so old_sympos is still needed. Here is an example:
> 
> # grep init_once /proc/kallsyms
> ffffffff8120ba80 t init_once.llvm.14172910296636650566
> ffffffff8120ba90 t inode_init_once
> ffffffff813ea5d0 t bpf_user_rnd_init_once
> ffffffff813fd5b0 t init_once.llvm.17912494158778303782
> ffffffff813ffbf0 t init_once
> ffffffff813ffc60 t init_once
> ffffffff813ffc70 t init_once
> ffffffff813ffcd0 t init_once
> ffffffff813ffce0 t init_once
> 
> There are two "init_once" with suffix, but there are also ones
> without them.

This is correct. At LTO mode, when a static function/variable
is promoted to the global. The '.llvm.<hash>' is added to the
static function/variable name to form a global function name.
The '<hash>' here is computed based on IR of the compiled file
before LTO. So if one file is not modified, then <hash>
won't change after additional code change in other files.

> 
> 2. kpatch-build does "Building original source", "Building patched
> source", and then do binary diff of the two. From our experiments,
> the suffix doesn't change between the two builds. However, we need
> to match the build environment (path of kernel source, etc.) to
> make sure suffix from kpatch matches the kernel.

The goal here is to generate the same IR (hence <hash>) if
file content is not changed. This way, <hash> value will
change only for those changed files.

> 
> 3. The goal of this patch is NOT to resolve different suffix by
> llvm (.llvm.[0-9]+). Instead, we are trying fix issues like:
> 
> #  grep bpf_verifier_vlog /proc/kallsyms
> ffffffff81549f60 t bpf_verifier_vlog
> ffffffff8268b430 d bpf_verifier_vlog._entry
> ffffffff8282a958 d bpf_verifier_vlog._entry_ptr
> ffffffff82e12a1f d bpf_verifier_vlog.__already_done

Note that these symbols also exist non-LTO mode.

For example, with my particular config, I have


$ grep bpf_verifier_vlog System.map
ffffffff812f9370 T bpf_verifier_vlog
ffffffff82afa440 d bpf_verifier_vlog._entry
ffffffff83404218 d bpf_verifier_vlog._entry_ptr
$ grep bpf_output System.map
ffffffff81359c70 t perf_event_bpf_output
ffffffff821eeba0 t bpf_output
ffffffff82eec720 d bpf_output._entry
ffffffff83412c50 d bpf_output._entry_ptr
ffffffff84b0f334 d bpf_output.__already_done

bpf_output() is defined in net/core/lwt_bpf.c.

The original function:

static int bpf_output(struct net *net, struct sock *sk, struct sk_buff *skb)
{
         struct dst_entry *dst = skb_dst(skb);
         struct bpf_lwt *bpf;
         int ret;

         bpf = bpf_lwt_lwtunnel(dst->lwtstate);
         if (bpf->out.prog) {
                 ret = run_lwt_bpf(skb, &bpf->out, dst, NO_REDIRECT);
                 if (ret < 0)
                         return ret;
         }

         if (unlikely(!dst->lwtstate->orig_output)) {
                 pr_warn_once("orig_output not set on dst for prog %s\n",
                              bpf->out.name);
                 kfree_skb(skb);
                 return -EINVAL;
         }

         return dst->lwtstate->orig_output(net, sk, skb);
}


The function after preprocess:

static int bpf_output(struct net *net, struct sock *sk, struct sk_buff *skb)
{
  struct dst_entry *dst = skb_dst(skb);
  struct bpf_lwt *bpf;
  int ret;

  bpf = bpf_lwt_lwtunnel(dst->lwtstate);
  if (bpf->out.prog) {
   ret = run_lwt_bpf(skb, &bpf->out, dst, false);
   if (ret < 0)
    return ret;
  }

  if (__builtin_expect(!!(!dst->lwtstate->orig_output), 0)) {
   ({ bool __ret_do_once = !!(true); if (({ static bool 
__attribute__((__section__(".data.once"))) __already_done; bool 
__ret_cond = !!(__ret_do_once); bool __ret_once = false; if 
(__builtin_expect(!!(__ret_cond && !__already_done), 0)) { 
__already_done = true; __ret_once = true; } 
__builtin_expect(!!(__ret_once), 0); })) ({ do { if 
(__builtin_constant_p("\001" "4" "orig_output not set on dst for prog 
%s\n") && __builtin_constant_p(((void *)0))) { static const struct 
pi_entry _entry __attribute__((__used__)) = { .fmt = 
__builtin_constant_p("\001" "4" "orig_output not set on dst for prog 
%s\n") ? ("\001" "4" "orig_output not set on dst for prog %s\n") : 
((void *)0), .func = __func__, .file = "net/core/lwt_bpf.c", .line = 
154, .level = __builtin_constant_p(((void *)0)) ? (((void *)0)) : ((void 
*)0), .subsys_fmt_prefix = ((void *)0), }; static const struct pi_entry 
*_entry_ptr __attribute__((__used__)) 
__attribute__((__section__(".printk_index"))) = &_entry; } } while (0); 
_printk("\001" "4" "orig_output not set on dst for prog %s\n", 
bpf->out.name); }); __builtin_expect(!!(__ret_do_once), 0); });

   kfree_skb(skb);
   return -22;
  }

  return dst->lwtstate->orig_output(net, sk, skb);
}

In the above particular case, pr_warn_once() introduced
three static variables inside the funciton '__already_done',
'_entry' and '_entry_ptr'. To differentiate from
other potential static variables inside other functions,
these static variables are renamed to
<func_name>.<static_variable_name> in the above.

> 
> With existing code, compare_symbol_name() will match
> bpf_verifier_vlog to all these with CONFIG_LTO_CLANG.
> 
> We can probably teach compare_symbol_name() to not to match
> these suffix, as Zhen suggested.

This might not be a scalable solution unless there is a
way to capture usage of static variable inside the function
with some tools and feed them into an auto generated table
to be used by live patching.

Current comment in cleanup_symbol_name():

===
         /*
          * LLVM appends various suffixes for local functions and 
variables that
          * must be promoted to global scope as part of LTO.  This can break
          * hooking of static functions with kprobes. '.' is not a valid
          * character in an identifier in C. Suffixes observed:
          * - foo.llvm.[0-9a-f]+
          * - foo.[0-9a-f]+
          */
===

Based on my earlier study from llvm15 and llvm17, I only
observed 'foo.llvm.[0-9a-f]+'. Maybe earlier version of llvm
lto generates 'foo.[0-9a-f]+' format.

Note that CONFIG_CLANG_VERSION should be in the .config file
if the kernel is built with clang, which could be used
to further differentiate suffix format if necessary.

> 
> If this is not ideal, I am open to suggestions that can solve
> the problem.
> 
>> But if the suffix is different then kpatch has a problem.
>> kpatch would need to match symbols with different suffixes.
>> It would be easy for symbols which are unique after removing
>> the suffix. But it would be tricky for comparing symbols
>> which do not have an unique name. kpatch would need to find
>> which suffix in the original binary matches an other suffix
>> in the fixed binary. In this case, it might be easier
>> to use the stripped symbol names.
>>
>> And the suffix might be problematic also for source based
>> livepatches. They define struct klp_func in sources so
>> they would need to hardcode the suffix there. It might
>> be easy to keep using the stripped name and "old_sympos".
>>
>> I expect that this patch actually breaks the livepatch
>> selftests when the kernel is compiled with clang LTO.
> 
> Not really. This patch passes livepatch selftests.
> 
> Thanks,
> Song
> 

Re: [PATCH] kallsyms: let kallsyms_on_each_match_symbol match symbols exactly

[Petr Mladek]

Hi,

I have added people mentioned in commits which modified
cleanup_symbol_name() in kallsyms.c.

I think that stripping ".*" suffix does not work for static
variables defined locally from symbol does always work,
see below.



On Wed 2023-06-21 15:34:27, Yonghong Song wrote:
> On 6/21/23 12:18 PM, Song Liu wrote:
> > > On Jun 21, 2023, at 1:52 AM, Petr Mladek <pmladek@suse.com> wrote:
> > > On Tue 2023-06-20 22:36:14, Song Liu wrote:
> > > > > On Jun 19, 2023, at 4:32 AM, Petr Mladek <pmladek@suse.com> wrote:
> > > > > On Sun 2023-06-18 22:05:19, Song Liu wrote:
> > > > > > On Sun, Jun 18, 2023 at 8:32 PM Leizhen (ThunderTown)
> > > > > > <thunder.leizhen@huawei.com> wrote:
> > > > I am not quite following the direction here. Do we need more
> > > > work for this patch?
> > > 
> > > Good question. I primary tried to add more details so that
> > > we better understand the problem.
> > > 
> > > Honestly, I do not know the answer. I am neither familiar with
> > > kpatch nor with clang.

> > This is pretty complicated.
> > 
> > 1. clang with LTO does not use the suffix to eliminated duplicated
> > kallsyms, so old_sympos is still needed. Here is an example:
> > 
> > # grep init_once /proc/kallsyms
> > ffffffff8120ba80 t init_once.llvm.14172910296636650566
> > ffffffff8120ba90 t inode_init_once
> > ffffffff813ea5d0 t bpf_user_rnd_init_once
> > ffffffff813fd5b0 t init_once.llvm.17912494158778303782
> > ffffffff813ffbf0 t init_once
> > ffffffff813ffc60 t init_once
> > ffffffff813ffc70 t init_once
> > ffffffff813ffcd0 t init_once
> > ffffffff813ffce0 t init_once
> > 
> > There are two "init_once" with suffix, but there are also ones
> > without them.
> 
> This is correct. At LTO mode, when a static function/variable
> is promoted to the global. The '.llvm.<hash>' is added to the
> static function/variable name to form a global function name.
> The '<hash>' here is computed based on IR of the compiled file
> before LTO. So if one file is not modified, then <hash>
> won't change after additional code change in other files.

OK, so the ".llvm.<hash>" suffix is added when a symbol is promoted
from static to global. Right?

> > 2. kpatch-build does "Building original source", "Building patched
> > source", and then do binary diff of the two. From our experiments,
> > the suffix doesn't change between the two builds. However, we need
> > to match the build environment (path of kernel source, etc.) to
> > make sure suffix from kpatch matches the kernel.
>
> The goal here is to generate the same IR (hence <hash>) if
> file content is not changed. This way, <hash> value will
> change only for those changed files.

Hmm, how does kpatch match the fixed functions? They are modified
so that they should get different IR (hash). Or do I miss
anything, please?

> > 3. The goal of this patch is NOT to resolve different suffix by
> > llvm (.llvm.[0-9]+). Instead, we are trying fix issues like:
> > 
> > #  grep bpf_verifier_vlog /proc/kallsyms
> > ffffffff81549f60 t bpf_verifier_vlog
> > ffffffff8268b430 d bpf_verifier_vlog._entry
> > ffffffff8282a958 d bpf_verifier_vlog._entry_ptr
> > ffffffff82e12a1f d bpf_verifier_vlog.__already_done

And <function>.<symbol> notation seems to be used for static symbols
defined inside a function.

I guess that it is used when the symbols stay statics. It would
probably get additional ".llvm.<hash>" when it got promoted
from static to global. But this probably never happens.

Do I get it correctly?

It means that we have two different types of name changes:

  1. .llvm.<hash> suffix

     If we remove this suffix then we will not longer distinguish
     symbols which stayed static and which were promoted to global
     ones.

     The result should be basically the same as without LTO.
     Some symbols might have duplicated name. But most symbols
     would have an unique one.


  2. <function>.<symbol> name

     In this case, <symbol> is _not_ suffix. It is actually
     the original symbol name.

     The extra thing is the <function>. prefix.

     These static variables seem to have special handling even
     with gcc without LTO. gcc adds an extra id instead,
     for example:

	$> nm vmlinux | grep " _entry_ptr" | head
	ffffffff82a2e800 d _entry_ptr.100135
	ffffffff82a2e7f8 d _entry_ptr.100178
	ffffffff82a32e70 d _entry_ptr.100798
	ffffffff82a1e240 d _entry_ptr.10342
	ffffffff82a33930 d _entry_ptr.104764
	ffffffff82a339c8 d _entry_ptr.104830
	ffffffff82a33928 d _entry_ptr.104871
	ffffffff82a33920 d _entry_ptr.104877
	ffffffff82a33918 d _entry_ptr.104893
	ffffffff82a339c0 d _entry_ptr.104905

	$> nm vmlinux | grep panic_console_dropped
	ffffffff853618e0 b panic_console_dropped.54158


Effect from the tracers POV?

  1. .llvm.<hash> suffix

     The names without the .llvm.<hash> suffix are the same as without
     LTO. This is probably why commit 8b8e6b5d3b013b0b ("kallsyms: strip
     ThinLTO hashes from static functions") worked. The tracers probably
     wanted to access only the symbols with uniqueue names


  2. <function>.<symbol> name

     The name without the .<symbol> suffix is the same as the function
     name. The result are duplicated function names.

     I do not understand why this was not a problem for tracers.
     Note that this is pretty common. _entry and _entry_ptr are
     added into any function calling printk().

     It seems to be working only by chance. Maybe, the tracers always
     take the first matched symbol. And the function name, without
     any suffix, is always the first one in the sorted list.


Effect from livepatching POV:

  1. .llvm.<hash> suffix

      Comparing the full symbol name looks fragile to me because
      the <hash> might change.

      IMHO, it would be better to compare the names without
      the .llvm.<hash> suffix even for livepatches.


   2. <function>.<symbol> name

      The removal of <.symbol> suffix is a bad idea. The livepatch
      code is not able to distinguish the symbol of the <function>
      and static variables defined in this function.

      IMHO, it would be better to compare the full
       <function>.<symbol> name.


Result:

IMHO, cleanup_symbol_name() should remove only .llwn.* suffix.
And it should be used for both tracers and livepatching.

Does this makes any sense?

Best Regards,
Petr

Re: [PATCH] kallsyms: let kallsyms_on_each_match_symbol match symbols exactly

[Yonghong Song]

On 6/22/23 6:35 AM, Petr Mladek wrote:
> Hi,
> 
> I have added people mentioned in commits which modified
> cleanup_symbol_name() in kallsyms.c.
> 
> I think that stripping ".*" suffix does not work for static
> variables defined locally from symbol does always work,
> see below.
> 
> 
> 
> On Wed 2023-06-21 15:34:27, Yonghong Song wrote:
>> On 6/21/23 12:18 PM, Song Liu wrote:
>>>> On Jun 21, 2023, at 1:52 AM, Petr Mladek <pmladek@suse.com> wrote:
>>>> On Tue 2023-06-20 22:36:14, Song Liu wrote:
>>>>>> On Jun 19, 2023, at 4:32 AM, Petr Mladek <pmladek@suse.com> wrote:
>>>>>> On Sun 2023-06-18 22:05:19, Song Liu wrote:
>>>>>>> On Sun, Jun 18, 2023 at 8:32 PM Leizhen (ThunderTown)
>>>>>>> <thunder.leizhen@huawei.com> wrote:
>>>>> I am not quite following the direction here. Do we need more
>>>>> work for this patch?
>>>>
>>>> Good question. I primary tried to add more details so that
>>>> we better understand the problem.
>>>>
>>>> Honestly, I do not know the answer. I am neither familiar with
>>>> kpatch nor with clang.
> 
>>> This is pretty complicated.
>>>
>>> 1. clang with LTO does not use the suffix to eliminated duplicated
>>> kallsyms, so old_sympos is still needed. Here is an example:
>>>
>>> # grep init_once /proc/kallsyms
>>> ffffffff8120ba80 t init_once.llvm.14172910296636650566
>>> ffffffff8120ba90 t inode_init_once
>>> ffffffff813ea5d0 t bpf_user_rnd_init_once
>>> ffffffff813fd5b0 t init_once.llvm.17912494158778303782
>>> ffffffff813ffbf0 t init_once
>>> ffffffff813ffc60 t init_once
>>> ffffffff813ffc70 t init_once
>>> ffffffff813ffcd0 t init_once
>>> ffffffff813ffce0 t init_once
>>>
>>> There are two "init_once" with suffix, but there are also ones
>>> without them.
>>
>> This is correct. At LTO mode, when a static function/variable
>> is promoted to the global. The '.llvm.<hash>' is added to the
>> static function/variable name to form a global function name.
>> The '<hash>' here is computed based on IR of the compiled file
>> before LTO. So if one file is not modified, then <hash>
>> won't change after additional code change in other files.
> 
> OK, so the ".llvm.<hash>" suffix is added when a symbol is promoted
> from static to global. Right?

Right at lest for llvm >= 15.
There are an alternative format '.llvm.<file_path>' suffix with
a more involved compilation process.
 
https://github.com/llvm/llvm-project/blob/main/llvm/test/ThinLTO/X86/promote-local-name.ll

But for kernel lto build, yes, only '.llvm.<hash>'.

> 
>>> 2. kpatch-build does "Building original source", "Building patched
>>> source", and then do binary diff of the two. From our experiments,
>>> the suffix doesn't change between the two builds. However, we need
>>> to match the build environment (path of kernel source, etc.) to
>>> make sure suffix from kpatch matches the kernel.
>>
>> The goal here is to generate the same IR (hence <hash>) if
>> file content is not changed. This way, <hash> value will
>> change only for those changed files.
> 
> Hmm, how does kpatch match the fixed functions? They are modified
> so that they should get different IR (hash). Or do I miss
> anything, please?

If the static function are promoted to global function and the file
containing static function changed, then that modified static
function will appear to be a *new* function. Live change should
be able to do it, right?

> 
>>> 3. The goal of this patch is NOT to resolve different suffix by
>>> llvm (.llvm.[0-9]+). Instead, we are trying fix issues like:
>>>
>>> #  grep bpf_verifier_vlog /proc/kallsyms
>>> ffffffff81549f60 t bpf_verifier_vlog
>>> ffffffff8268b430 d bpf_verifier_vlog._entry
>>> ffffffff8282a958 d bpf_verifier_vlog._entry_ptr
>>> ffffffff82e12a1f d bpf_verifier_vlog.__already_done
> 
> And <function>.<symbol> notation seems to be used for static symbols
> defined inside a function.

That is correct.

> 
> I guess that it is used when the symbols stay statics. It would
> probably get additional ".llvm.<hash>" when it got promoted
> from static to global. But this probably never happens.

I have not see a case like this yet.

> 
> Do I get it correctly?

yes, that is correct.

> 
> It means that we have two different types of name changes:
> 
>    1. .llvm.<hash> suffix
> 
>       If we remove this suffix then we will not longer distinguish
>       symbols which stayed static and which were promoted to global
>       ones.
> 
>       The result should be basically the same as without LTO.
>       Some symbols might have duplicated name. But most symbols
>       would have an unique one.
> 
> 
>    2. <function>.<symbol> name
> 
>       In this case, <symbol> is _not_ suffix. It is actually
>       the original symbol name.
> 
>       The extra thing is the <function>. prefix.
> 
>       These static variables seem to have special handling even
>       with gcc without LTO. gcc adds an extra id instead,
>       for example:
> 
> 	$> nm vmlinux | grep " _entry_ptr" | head
> 	ffffffff82a2e800 d _entry_ptr.100135
> 	ffffffff82a2e7f8 d _entry_ptr.100178
> 	ffffffff82a32e70 d _entry_ptr.100798
> 	ffffffff82a1e240 d _entry_ptr.10342
> 	ffffffff82a33930 d _entry_ptr.104764
> 	ffffffff82a339c8 d _entry_ptr.104830
> 	ffffffff82a33928 d _entry_ptr.104871
> 	ffffffff82a33920 d _entry_ptr.104877
> 	ffffffff82a33918 d _entry_ptr.104893
> 	ffffffff82a339c0 d _entry_ptr.104905
> 
> 	$> nm vmlinux | grep panic_console_dropped
> 	ffffffff853618e0 b panic_console_dropped.54158

IIRC, yes, these 'id' might change if source code changed.

> 
> 
> Effect from the tracers POV?
> 
>    1. .llvm.<hash> suffix
> 
>       The names without the .llvm.<hash> suffix are the same as without
>       LTO. This is probably why commit 8b8e6b5d3b013b0b ("kallsyms: strip
>       ThinLTO hashes from static functions") worked. The tracers probably
>       wanted to access only the symbols with uniqueue names
> 
> 
>    2. <function>.<symbol> name
> 
>       The name without the .<symbol> suffix is the same as the function
>       name. The result are duplicated function names.
> 
>       I do not understand why this was not a problem for tracers.
>       Note that this is pretty common. _entry and _entry_ptr are
>       added into any function calling printk().
> 
>       It seems to be working only by chance. Maybe, the tracers always
>       take the first matched symbol. And the function name, without
>       any suffix, is always the first one in the sorted list.

Note this only happens in LTO mode. Maybe lto kernel is not used
wide enough to discover this issue?

> 
> 
> Effect from livepatching POV:
> 
>    1. .llvm.<hash> suffix
> 
>        Comparing the full symbol name looks fragile to me because
>        the <hash> might change.
> 
>        IMHO, it would be better to compare the names without
>        the .llvm.<hash> suffix even for livepatches.
> 
> 
>     2. <function>.<symbol> name
> 
>        The removal of <.symbol> suffix is a bad idea. The livepatch
>        code is not able to distinguish the symbol of the <function>
>        and static variables defined in this function.
> 
>        IMHO, it would be better to compare the full
>         <function>.<symbol> name.
> 
> 
> Result:
> 
> IMHO, cleanup_symbol_name() should remove only .llwn.* suffix.
> And it should be used for both tracers and livepatching.
> 
> Does this makes any sense?

Song, does this fix the problem?

I only checked llvm15 and llvm17, not sure what kind of
suffix'es used for early llvm (>= llvm11).
Nick, could you help answer this question? What kind
of suffix are used for lto when promoting a local symbol
to a global one, considering all versions of llvm >= 11
since llvm 11 is the minimum supported version for kernel build.

> 
> Best Regards,
> Petr

Re: [PATCH] kallsyms: let kallsyms_on_each_match_symbol match symbols exactly

[Yonghong Song]

On 6/22/23 1:33 PM, Song Liu wrote:
> 
> 
>> On Jun 22, 2023, at 9:10 AM, Yonghong Song <yhs@meta.com> wrote:
> 
> [...]
> 
>>
>>> Effect from the tracers POV?
>>>    1. .llvm.<hash> suffix
>>>       The names without the .llvm.<hash> suffix are the same as without
>>>       LTO. This is probably why commit 8b8e6b5d3b013b0b ("kallsyms: strip
>>>       ThinLTO hashes from static functions") worked. The tracers probably
>>>       wanted to access only the symbols with uniqueue names
>>>    2. <function>.<symbol> name
>>>       The name without the .<symbol> suffix is the same as the function
>>>       name. The result are duplicated function names.
>>>       I do not understand why this was not a problem for tracers.
>>>       Note that this is pretty common. _entry and _entry_ptr are
>>>       added into any function calling printk().
>>>       It seems to be working only by chance. Maybe, the tracers always
>>>       take the first matched symbol. And the function name, without
>>>       any suffix, is always the first one in the sorted list.
>>
>> Note this only happens in LTO mode. Maybe lto kernel is not used
>> wide enough to discover this issue?
> 
> I think this is because all these <function>.<symbol> are data, while
> tracers are looking for functions.
> 
>>
>>> Effect from livepatching POV:
>>>    1. .llvm.<hash> suffix
>>>        Comparing the full symbol name looks fragile to me because
>>>        the <hash> might change.
>>>        IMHO, it would be better to compare the names without
>>>        the .llvm.<hash> suffix even for livepatches.
>>>     2. <function>.<symbol> name
>>>        The removal of <.symbol> suffix is a bad idea. The livepatch
>>>        code is not able to distinguish the symbol of the <function>
>>>        and static variables defined in this function.
>>>        IMHO, it would be better to compare the full
>>>         <function>.<symbol> name.
>>> Result:
>>> IMHO, cleanup_symbol_name() should remove only .llwn.* suffix.
>>> And it should be used for both tracers and livepatching.
>>> Does this makes any sense?
>>
>> Song, does this fix the problem?
> 
> I think this should work. We also see .str.<num.>llvm.<hash>.
> But those should not matter.

For some string constants, llvm may create a local symbol with
name like '.str'. If there are more than one such local
symbols, they become '.str.<num>'. Then when they got promoted
to global they become '.str.<num>.llvm.<hash>'.

> 
> Thanks,
> Song
> 
>>
>> I only checked llvm15 and llvm17, not sure what kind of
>> suffix'es used for early llvm (>= llvm11).
>> Nick, could you help answer this question? What kind
>> of suffix are used for lto when promoting a local symbol
>> to a global one, considering all versions of llvm >= 11
>> since llvm 11 is the minimum supported version for kernel build.
> 

Re: [PATCH] kallsyms: let kallsyms_on_each_match_symbol match symbols exactly

[Nick Desaulniers]

On Thu, Jun 22, 2023 at 9:11 AM Yonghong Song <yhs@meta.com> wrote:
>
>
>
> On 6/22/23 6:35 AM, Petr Mladek wrote:
> > Hi,
> >
> > I have added people mentioned in commits which modified
> > cleanup_symbol_name() in kallsyms.c.
> >
> > I think that stripping ".*" suffix does not work for static
> > variables defined locally from symbol does always work,
> > see below.
> >
> >
> >
> > On Wed 2023-06-21 15:34:27, Yonghong Song wrote:
> >> On 6/21/23 12:18 PM, Song Liu wrote:
> >>>> On Jun 21, 2023, at 1:52 AM, Petr Mladek <pmladek@suse.com> wrote:
> >>>> On Tue 2023-06-20 22:36:14, Song Liu wrote:
> >>>>>> On Jun 19, 2023, at 4:32 AM, Petr Mladek <pmladek@suse.com> wrote:
> >>>>>> On Sun 2023-06-18 22:05:19, Song Liu wrote:
> >>>>>>> On Sun, Jun 18, 2023 at 8:32 PM Leizhen (ThunderTown)
> >>>>>>> <thunder.leizhen@huawei.com> wrote:
> >>>>> I am not quite following the direction here. Do we need more
> >>>>> work for this patch?
> >>>>
> >>>> Good question. I primary tried to add more details so that
> >>>> we better understand the problem.
> >>>>
> >>>> Honestly, I do not know the answer. I am neither familiar with
> >>>> kpatch nor with clang.
> >
> >>> This is pretty complicated.
> >>>
> >>> 1. clang with LTO does not use the suffix to eliminated duplicated
> >>> kallsyms, so old_sympos is still needed. Here is an example:
> >>>
> >>> # grep init_once /proc/kallsyms
> >>> ffffffff8120ba80 t init_once.llvm.14172910296636650566
> >>> ffffffff8120ba90 t inode_init_once
> >>> ffffffff813ea5d0 t bpf_user_rnd_init_once
> >>> ffffffff813fd5b0 t init_once.llvm.17912494158778303782
> >>> ffffffff813ffbf0 t init_once
> >>> ffffffff813ffc60 t init_once
> >>> ffffffff813ffc70 t init_once
> >>> ffffffff813ffcd0 t init_once
> >>> ffffffff813ffce0 t init_once
> >>>
> >>> There are two "init_once" with suffix, but there are also ones
> >>> without them.
> >>
> >> This is correct. At LTO mode, when a static function/variable
> >> is promoted to the global. The '.llvm.<hash>' is added to the
> >> static function/variable name to form a global function name.
> >> The '<hash>' here is computed based on IR of the compiled file
> >> before LTO. So if one file is not modified, then <hash>
> >> won't change after additional code change in other files.
> >
> > OK, so the ".llvm.<hash>" suffix is added when a symbol is promoted
> > from static to global. Right?
>
> Right at lest for llvm >= 15.
> There are an alternative format '.llvm.<file_path>' suffix with
> a more involved compilation process.
>
> https://github.com/llvm/llvm-project/blob/main/llvm/test/ThinLTO/X86/promote-local-name.ll
>
> But for kernel lto build, yes, only '.llvm.<hash>'.
>
> >
> >>> 2. kpatch-build does "Building original source", "Building patched
> >>> source", and then do binary diff of the two. From our experiments,
> >>> the suffix doesn't change between the two builds. However, we need
> >>> to match the build environment (path of kernel source, etc.) to
> >>> make sure suffix from kpatch matches the kernel.
> >>
> >> The goal here is to generate the same IR (hence <hash>) if
> >> file content is not changed. This way, <hash> value will
> >> change only for those changed files.
> >
> > Hmm, how does kpatch match the fixed functions? They are modified
> > so that they should get different IR (hash). Or do I miss
> > anything, please?
>
> If the static function are promoted to global function and the file
> containing static function changed, then that modified static
> function will appear to be a *new* function. Live change should
> be able to do it, right?
>
> >
> >>> 3. The goal of this patch is NOT to resolve different suffix by
> >>> llvm (.llvm.[0-9]+). Instead, we are trying fix issues like:
> >>>
> >>> #  grep bpf_verifier_vlog /proc/kallsyms
> >>> ffffffff81549f60 t bpf_verifier_vlog
> >>> ffffffff8268b430 d bpf_verifier_vlog._entry
> >>> ffffffff8282a958 d bpf_verifier_vlog._entry_ptr
> >>> ffffffff82e12a1f d bpf_verifier_vlog.__already_done
> >
> > And <function>.<symbol> notation seems to be used for static symbols
> > defined inside a function.
>
> That is correct.
>
> >
> > I guess that it is used when the symbols stay statics. It would
> > probably get additional ".llvm.<hash>" when it got promoted
> > from static to global. But this probably never happens.
>
> I have not see a case like this yet.
>
> >
> > Do I get it correctly?
>
> yes, that is correct.
>
> >
> > It means that we have two different types of name changes:
> >
> >    1. .llvm.<hash> suffix
> >
> >       If we remove this suffix then we will not longer distinguish
> >       symbols which stayed static and which were promoted to global
> >       ones.
> >
> >       The result should be basically the same as without LTO.
> >       Some symbols might have duplicated name. But most symbols
> >       would have an unique one.
> >
> >
> >    2. <function>.<symbol> name
> >
> >       In this case, <symbol> is _not_ suffix. It is actually
> >       the original symbol name.
> >
> >       The extra thing is the <function>. prefix.
> >
> >       These static variables seem to have special handling even
> >       with gcc without LTO. gcc adds an extra id instead,
> >       for example:
> >
> >       $> nm vmlinux | grep " _entry_ptr" | head
> >       ffffffff82a2e800 d _entry_ptr.100135
> >       ffffffff82a2e7f8 d _entry_ptr.100178
> >       ffffffff82a32e70 d _entry_ptr.100798
> >       ffffffff82a1e240 d _entry_ptr.10342
> >       ffffffff82a33930 d _entry_ptr.104764
> >       ffffffff82a339c8 d _entry_ptr.104830
> >       ffffffff82a33928 d _entry_ptr.104871
> >       ffffffff82a33920 d _entry_ptr.104877
> >       ffffffff82a33918 d _entry_ptr.104893
> >       ffffffff82a339c0 d _entry_ptr.104905
> >
> >       $> nm vmlinux | grep panic_console_dropped
> >       ffffffff853618e0 b panic_console_dropped.54158
>
> IIRC, yes, these 'id' might change if source code changed.
>
> >
> >
> > Effect from the tracers POV?
> >
> >    1. .llvm.<hash> suffix
> >
> >       The names without the .llvm.<hash> suffix are the same as without
> >       LTO. This is probably why commit 8b8e6b5d3b013b0b ("kallsyms: strip
> >       ThinLTO hashes from static functions") worked. The tracers probably
> >       wanted to access only the symbols with uniqueue names
> >
> >
> >    2. <function>.<symbol> name
> >
> >       The name without the .<symbol> suffix is the same as the function
> >       name. The result are duplicated function names.
> >
> >       I do not understand why this was not a problem for tracers.
> >       Note that this is pretty common. _entry and _entry_ptr are
> >       added into any function calling printk().
> >
> >       It seems to be working only by chance. Maybe, the tracers always
> >       take the first matched symbol. And the function name, without
> >       any suffix, is always the first one in the sorted list.
>
> Note this only happens in LTO mode. Maybe lto kernel is not used
> wide enough to discover this issue?

Likely.

>
> >
> >
> > Effect from livepatching POV:
> >
> >    1. .llvm.<hash> suffix
> >
> >        Comparing the full symbol name looks fragile to me because
> >        the <hash> might change.
> >
> >        IMHO, it would be better to compare the names without
> >        the .llvm.<hash> suffix even for livepatches.
> >
> >
> >     2. <function>.<symbol> name
> >
> >        The removal of <.symbol> suffix is a bad idea. The livepatch
> >        code is not able to distinguish the symbol of the <function>
> >        and static variables defined in this function.
> >
> >        IMHO, it would be better to compare the full
> >         <function>.<symbol> name.
> >
> >
> > Result:
> >
> > IMHO, cleanup_symbol_name() should remove only .llwn.* suffix.
> > And it should be used for both tracers and livepatching.
> >
> > Does this makes any sense?
>
> Song, does this fix the problem?
>
> I only checked llvm15 and llvm17, not sure what kind of
> suffix'es used for early llvm (>= llvm11).
> Nick, could you help answer this question? What kind
> of suffix are used for lto when promoting a local symbol
> to a global one, considering all versions of llvm >= 11
> since llvm 11 is the minimum supported version for kernel build.

For ToT for this case, the call chain backtrace looks like:

ModuleSummaryIndex::getGlobalNameForLocal
FunctionImportGlobalProcessing::getPromotedName
FunctionImportGlobalProcessing::processGlobalForThinLTO

This has been the case since release/11.x.

LLVM uses different mangling schemes for different places. For
example, function specialization (that occurs when sinking a constant
into a function) may rename a function from foo to something like
foo.42 where a dot followed by a monotonically increasing counter is
used. Numbers before may be missing from other symbols (where's .41?)
if they are DCE'd (perhaps because they were inlined and have no more
callers).  That is done by:

ValueSymbolTable::makeUniqueName which is eventually called by
FunctionSpecializer::createSpecialization.

That is the cause of common warnings from modpost.

There are likely numerous other special manglings done through llvm.
The above two are slightly more generic, but other passes may do
something more ad-hoc.

>
> >
> > Best Regards,
> > Petr



-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] kallsyms: let kallsyms_on_each_match_symbol match symbols exactly

[Yonghong Song]

On 6/23/23 10:43 AM, Nick Desaulniers wrote:
> On Thu, Jun 22, 2023 at 9:11 AM Yonghong Song <yhs@meta.com> wrote:
>>
>>
>>
>> On 6/22/23 6:35 AM, Petr Mladek wrote:
>>> Hi,
>>>
>>> I have added people mentioned in commits which modified
>>> cleanup_symbol_name() in kallsyms.c.
>>>
>>> I think that stripping ".*" suffix does not work for static
>>> variables defined locally from symbol does always work,
>>> see below.
>>>
>>>
>>>
>>> On Wed 2023-06-21 15:34:27, Yonghong Song wrote:
>>>> On 6/21/23 12:18 PM, Song Liu wrote:
>>>>>> On Jun 21, 2023, at 1:52 AM, Petr Mladek <pmladek@suse.com> wrote:
>>>>>> On Tue 2023-06-20 22:36:14, Song Liu wrote:
>>>>>>>> On Jun 19, 2023, at 4:32 AM, Petr Mladek <pmladek@suse.com> wrote:
>>>>>>>> On Sun 2023-06-18 22:05:19, Song Liu wrote:
>>>>>>>>> On Sun, Jun 18, 2023 at 8:32 PM Leizhen (ThunderTown)
>>>>>>>>> <thunder.leizhen@huawei.com> wrote:
>>>>>>> I am not quite following the direction here. Do we need more
>>>>>>> work for this patch?
>>>>>>
>>>>>> Good question. I primary tried to add more details so that
>>>>>> we better understand the problem.
>>>>>>
>>>>>> Honestly, I do not know the answer. I am neither familiar with
>>>>>> kpatch nor with clang.
>>>
>>>>> This is pretty complicated.
>>>>>
>>>>> 1. clang with LTO does not use the suffix to eliminated duplicated
>>>>> kallsyms, so old_sympos is still needed. Here is an example:
>>>>>
>>>>> # grep init_once /proc/kallsyms
>>>>> ffffffff8120ba80 t init_once.llvm.14172910296636650566
>>>>> ffffffff8120ba90 t inode_init_once
>>>>> ffffffff813ea5d0 t bpf_user_rnd_init_once
>>>>> ffffffff813fd5b0 t init_once.llvm.17912494158778303782
>>>>> ffffffff813ffbf0 t init_once
>>>>> ffffffff813ffc60 t init_once
>>>>> ffffffff813ffc70 t init_once
>>>>> ffffffff813ffcd0 t init_once
>>>>> ffffffff813ffce0 t init_once
>>>>>
>>>>> There are two "init_once" with suffix, but there are also ones
>>>>> without them.
>>>>
>>>> This is correct. At LTO mode, when a static function/variable
>>>> is promoted to the global. The '.llvm.<hash>' is added to the
>>>> static function/variable name to form a global function name.
>>>> The '<hash>' here is computed based on IR of the compiled file
>>>> before LTO. So if one file is not modified, then <hash>
>>>> won't change after additional code change in other files.
>>>
>>> OK, so the ".llvm.<hash>" suffix is added when a symbol is promoted
>>> from static to global. Right?
>>
>> Right at lest for llvm >= 15.
>> There are an alternative format '.llvm.<file_path>' suffix with
>> a more involved compilation process.
>>
>> https://github.com/llvm/llvm-project/blob/main/llvm/test/ThinLTO/X86/promote-local-name.ll
>>
>> But for kernel lto build, yes, only '.llvm.<hash>'.
>>
>>>
>>>>> 2. kpatch-build does "Building original source", "Building patched
>>>>> source", and then do binary diff of the two. From our experiments,
>>>>> the suffix doesn't change between the two builds. However, we need
>>>>> to match the build environment (path of kernel source, etc.) to
>>>>> make sure suffix from kpatch matches the kernel.
>>>>
>>>> The goal here is to generate the same IR (hence <hash>) if
>>>> file content is not changed. This way, <hash> value will
>>>> change only for those changed files.
>>>
>>> Hmm, how does kpatch match the fixed functions? They are modified
>>> so that they should get different IR (hash). Or do I miss
>>> anything, please?
>>
>> If the static function are promoted to global function and the file
>> containing static function changed, then that modified static
>> function will appear to be a *new* function. Live change should
>> be able to do it, right?
>>
>>>
>>>>> 3. The goal of this patch is NOT to resolve different suffix by
>>>>> llvm (.llvm.[0-9]+). Instead, we are trying fix issues like:
>>>>>
>>>>> #  grep bpf_verifier_vlog /proc/kallsyms
>>>>> ffffffff81549f60 t bpf_verifier_vlog
>>>>> ffffffff8268b430 d bpf_verifier_vlog._entry
>>>>> ffffffff8282a958 d bpf_verifier_vlog._entry_ptr
>>>>> ffffffff82e12a1f d bpf_verifier_vlog.__already_done
>>>
>>> And <function>.<symbol> notation seems to be used for static symbols
>>> defined inside a function.
>>
>> That is correct.
>>
>>>
>>> I guess that it is used when the symbols stay statics. It would
>>> probably get additional ".llvm.<hash>" when it got promoted
>>> from static to global. But this probably never happens.
>>
>> I have not see a case like this yet.
>>
>>>
>>> Do I get it correctly?
>>
>> yes, that is correct.
>>
>>>
>>> It means that we have two different types of name changes:
>>>
>>>     1. .llvm.<hash> suffix
>>>
>>>        If we remove this suffix then we will not longer distinguish
>>>        symbols which stayed static and which were promoted to global
>>>        ones.
>>>
>>>        The result should be basically the same as without LTO.
>>>        Some symbols might have duplicated name. But most symbols
>>>        would have an unique one.
>>>
>>>
>>>     2. <function>.<symbol> name
>>>
>>>        In this case, <symbol> is _not_ suffix. It is actually
>>>        the original symbol name.
>>>
>>>        The extra thing is the <function>. prefix.
>>>
>>>        These static variables seem to have special handling even
>>>        with gcc without LTO. gcc adds an extra id instead,
>>>        for example:
>>>
>>>        $> nm vmlinux | grep " _entry_ptr" | head
>>>        ffffffff82a2e800 d _entry_ptr.100135
>>>        ffffffff82a2e7f8 d _entry_ptr.100178
>>>        ffffffff82a32e70 d _entry_ptr.100798
>>>        ffffffff82a1e240 d _entry_ptr.10342
>>>        ffffffff82a33930 d _entry_ptr.104764
>>>        ffffffff82a339c8 d _entry_ptr.104830
>>>        ffffffff82a33928 d _entry_ptr.104871
>>>        ffffffff82a33920 d _entry_ptr.104877
>>>        ffffffff82a33918 d _entry_ptr.104893
>>>        ffffffff82a339c0 d _entry_ptr.104905
>>>
>>>        $> nm vmlinux | grep panic_console_dropped
>>>        ffffffff853618e0 b panic_console_dropped.54158
>>
>> IIRC, yes, these 'id' might change if source code changed.
>>
>>>
>>>
>>> Effect from the tracers POV?
>>>
>>>     1. .llvm.<hash> suffix
>>>
>>>        The names without the .llvm.<hash> suffix are the same as without
>>>        LTO. This is probably why commit 8b8e6b5d3b013b0b ("kallsyms: strip
>>>        ThinLTO hashes from static functions") worked. The tracers probably
>>>        wanted to access only the symbols with uniqueue names
>>>
>>>
>>>     2. <function>.<symbol> name
>>>
>>>        The name without the .<symbol> suffix is the same as the function
>>>        name. The result are duplicated function names.
>>>
>>>        I do not understand why this was not a problem for tracers.
>>>        Note that this is pretty common. _entry and _entry_ptr are
>>>        added into any function calling printk().
>>>
>>>        It seems to be working only by chance. Maybe, the tracers always
>>>        take the first matched symbol. And the function name, without
>>>        any suffix, is always the first one in the sorted list.
>>
>> Note this only happens in LTO mode. Maybe lto kernel is not used
>> wide enough to discover this issue?
> 
> Likely.
> 
>>
>>>
>>>
>>> Effect from livepatching POV:
>>>
>>>     1. .llvm.<hash> suffix
>>>
>>>         Comparing the full symbol name looks fragile to me because
>>>         the <hash> might change.
>>>
>>>         IMHO, it would be better to compare the names without
>>>         the .llvm.<hash> suffix even for livepatches.
>>>
>>>
>>>      2. <function>.<symbol> name
>>>
>>>         The removal of <.symbol> suffix is a bad idea. The livepatch
>>>         code is not able to distinguish the symbol of the <function>
>>>         and static variables defined in this function.
>>>
>>>         IMHO, it would be better to compare the full
>>>          <function>.<symbol> name.
>>>
>>>
>>> Result:
>>>
>>> IMHO, cleanup_symbol_name() should remove only .llwn.* suffix.
>>> And it should be used for both tracers and livepatching.
>>>
>>> Does this makes any sense?
>>
>> Song, does this fix the problem?
>>
>> I only checked llvm15 and llvm17, not sure what kind of
>> suffix'es used for early llvm (>= llvm11).
>> Nick, could you help answer this question? What kind
>> of suffix are used for lto when promoting a local symbol
>> to a global one, considering all versions of llvm >= 11
>> since llvm 11 is the minimum supported version for kernel build.
> 
> For ToT for this case, the call chain backtrace looks like:
> 
> ModuleSummaryIndex::getGlobalNameForLocal
> FunctionImportGlobalProcessing::getPromotedName
> FunctionImportGlobalProcessing::processGlobalForThinLTO
> 
> This has been the case since release/11.x.
> 
> LLVM uses different mangling schemes for different places. For
> example, function specialization (that occurs when sinking a constant
> into a function) may rename a function from foo to something like
> foo.42 where a dot followed by a monotonically increasing counter is
> used. Numbers before may be missing from other symbols (where's .41?)
> if they are DCE'd (perhaps because they were inlined and have no more
> callers).  That is done by:
> 
> ValueSymbolTable::makeUniqueName which is eventually called by
> FunctionSpecializer::createSpecialization.
> 
> That is the cause of common warnings from modpost.
> 
> There are likely numerous other special manglings done through llvm.
> The above two are slightly more generic, but other passes may do
> something more ad-hoc.

Thanks for the information. I checked 
FunctionSpecializer::createSpecialization and this also happens
without LTO. But here, we are discussing at LTO mode.
See 
https://github.com/torvalds/linux/blob/master/kernel/kallsyms.c#L166-L188

Let us say, without LTO, through function specializer,
two functions are generated:
    foo
    foo.58
and live patching for 'foo' can be done correctly
since live patching is able to match 'foo' precisely.

Now, the kernel is built with LTO, and two functions
are still generated
    foo
    foo.58
With current code, live patching searching a 'foo'
function and both function 'foo' and 'foo.58' will
be returned. If live patching intends to search
'foo.58' and actually there will be a mismatch.

There are also another case we discovered above,
    foo
    foo._entry (static variable inside foo)
    foo._entry_ptr (static variable inside foo)

These three symbols will be generated without LTO
and it works fine for tracing and live patching.

But at LTO mode, searching 'foo' will get
three symbols, and at least live patching
won't work any more.

Do you really have a use case that
foo.* (excluding foo.llvm.*) cause a problem?
Could you share it?

> 
>>
>>>
>>> Best Regards,
>>> Petr
> 
> 
>