Re: [RFC dwarves] syscall functions in BTF

[Alan Maguire <alan.maguire@oracle.com>]

On 10/03/2023 15:03, Arnaldo Carvalho de Melo wrote:
> Em Fri, Mar 10, 2023 at 12:43:31PM +0000, Alan Maguire escreveu:
>> On 10/03/2023 10:07, Jiri Olsa wrote:
>>> hi,
>>> with latest pahole fixes we get rid of some syscall functions (with
>>> __x64_sys_ prefix) and it seems to fall down to 2 cases:
>>>
>>> - weak syscall functions generated in kernel/sys_ni.c prevent these syscalls
>>>   to be generated in BTF. The reason is the __COND_SYSCALL macro uses
>>>   '__unused' for regs argument:
>>>
>>>         #define __COND_SYSCALL(abi, name)                                      \
>>>                __weak long __##abi##_##name(const struct pt_regs *__unused);   \
>>>                __weak long __##abi##_##name(const struct pt_regs *__unused)    \
>>>                {                                                               \
>>>                        return sys_ni_syscall();                                \
>>>                }
>>>
>>>   and having weak function with different argument name will rule out the
>>>   syscall from BTF functions
>>>
>>>   the patch below workarounds this by using the same argument name,
>>>   but I guess the real fix would be to check the whole type not just
>>>   the argument name.. or ignore weak function if there's non weak one
>>>
>>>   I guess there will be more cases like this in kernel
>>>
>>>
>  
>> Thanks for the report Jiri! I'm working on reusing the dwarves_fprintf.c
>> code to use string comparisons of function prototypes (minus parameter names!)
>> instead as a more robust comparison.  Hope to have something working soon..
> 
> Humm, that could be an option, a simple strcmp after snprintf'ing the
> function prototype, but there is also the type__compare_members_types()
> approach, used to order types in pahole, the same could be done for
> function prototypes?
> 
> I.e. to compare a function prototype for functions with the same name we
> would check its return value type, the number of arguments and then each
> of the arguments, continuing to consider the names as an heuristic that
> functions with all being so far equal having different argument names
> may indicate different functions, but if there is no name in both
> functions, look at its type instead, where we then would use
> type__compare_members_types() for structs/unions?
> 

We could do that too alright; the thing I like about stringifying the
prototype is we get a clear description when we hit a mismatch
(in verbose mode):

function mismatch for 'alloc_buffer'('alloc_buffer'): 'struct debug_buffer * ()(struct usb_bus *, ssize_t (*)(struct debug_buffer *))' != 'struct debug_buffer * ()(struct ohci_hcd *, ssize_t (*)(struct debug_buffer *))'

...and we can re-use the existing support for function display, which works great!
Luckily we don't have to do this very often; I counted 2113 prototype comparisons,
of which only 49 mismatches were detected. Thanks!

Alan

> - Arnaldo
>   
>>> - we also do not get any syscall with no arguments, because they are
>>>   generated as aliases to __do_<syscall> function:
>>>
>>>         $ nm ./vmlinux | grep _sys_fork
>>>         ffffffff81174890 t __do_sys_fork
>>>         ffffffff81174890 T __ia32_sys_fork
>>>         ffffffff81174880 T __pfx___x64_sys_fork
>>>         ffffffff81174890 T __x64_sys_fork
>>>
>>>   with:
>>>         #define __SYS_STUB0(abi, name)                                          \
>>>                 long __##abi##_##name(const struct pt_regs *regs);              \
>>>                 ALLOW_ERROR_INJECTION(__##abi##_##name, ERRNO);                 \
>>>                 long __##abi##_##name(const struct pt_regs *regs)               \
>>>                         __alias(__do_##name);
>>>
>>>   the problem seems to be that there's no DWARF data for aliased symbol,
>>>   so pahole won't see any __x64_sys_fork record
>>>   I'm not sure how to fix this one
>>>
>>
>> Is this one a new issue, or did you just spot it when looking at the other case?
>>
>> Thanks!
>>
>> Alan
>>
>>>   technically we can always connect to __do_sys_fork, but we'd need to
>>>   have special cases for such syscalls.. would be great to have all with
>>>   '__x64_sys_' prefix
>>>
>>>
>>> thoughts?
>>>
>>> thanks,
>>> jirka
>>>
>>>
>>> ---
>>> diff --git a/arch/x86/include/asm/syscall_wrapper.h b/arch/x86/include/asm/syscall_wrapper.h
>>> index fd2669b1cb2d..e02dab630577 100644
>>> --- a/arch/x86/include/asm/syscall_wrapper.h
>>> +++ b/arch/x86/include/asm/syscall_wrapper.h
>>> @@ -80,8 +80,8 @@ extern long __ia32_sys_ni_syscall(const struct pt_regs *regs);
>>>  	}
>>>  
>>>  #define __COND_SYSCALL(abi, name)					\
>>> -	__weak long __##abi##_##name(const struct pt_regs *__unused);	\
>>> -	__weak long __##abi##_##name(const struct pt_regs *__unused)	\
>>> +	__weak long __##abi##_##name(const struct pt_regs *regs);	\
>>> +	__weak long __##abi##_##name(const struct pt_regs *regs)	\
>>>  	{								\
>>>  		return sys_ni_syscall();				\
>>>  	}
>>>
>