Re: [PATCH] x86: Include asm/ptrace.h in syscall_wrapper header

[Jiri Olsa]

On Tue, Oct 18, 2022 at 11:23:21AM -0700, sdf@xxxxxxxxxx wrote:
> On 10/18, Jiri Olsa wrote:
> > From: Jiri Olsa <olsajiri@xxxxxxxxx>
> 
> > With just the forward declaration of the 'struct pt_regs' in
> > syscall_wrapper.h, the syscall stub functions:
> 
> >    __[x64|ia32]_sys_*(struct pt_regs *regs)
> 
> > will have different definition of 'regs' argument in BTF data
> > based on which object file they are defined in.
> 
> > If the syscall's object includes 'struct pt_regs' definition,
> > the BTF argument data will point to 'struct pt_regs' record,
> > like:
> 
> >    [226] STRUCT 'pt_regs' size=168 vlen=21
> >           'r15' type_id=1 bits_offset=0
> >           'r14' type_id=1 bits_offset=64
> >           'r13' type_id=1 bits_offset=128
> >    ...
> 
> > If not, it will point to fwd declaration record:
> 
> >    [15439] FWD 'pt_regs' fwd_kind=struct
> 
> > and make bpf tracing program hooking on those functions unable
> > to access fields from 'struct pt_regs'.
> 
> Is the core issue here is that we can't / don't resolve FWD declarations
> at load time (or dedup time)? I'm assuming 'struct pt_regs' is still
> exposed somewhere in BTF via some other obj file, it's just those
> syscall definitions that have FWD decl?

yes, BTF is generated from dwarf debug info, so it's object based,
and in some objects the regs argument will point to full struct
definition and in some just to forward declaration

> 
> Applying this patch seems fine for now, but also seems fragile. Should
> we instead/also teach verifier/dedup/whatever to resolve those FWD
> declarations?

I'm not sure how hard it'd be to connect forward declarations
to definitions.. it'd need to be probably in dedup or verifier
as you suggest

and I think it'd need to be done just based on struct name search,
so I expect all sort of unexpected problems ;-) other than to be
sure you connect to proper struct

Andrii will have probably better idea if and where this is possible

jirka

> 
> > Including asm/ptrace.h directly in syscall_wrapper.h to make
> > sure all syscalls see 'struct pt_regs' definition and resulted
> > BTF for '__*_sys_*(struct pt_regs *regs)' functions point to
> > actual struct, not just forward declaration.
> 
> > Reported-by: Akihiro HARAI <jharai0815@xxxxxxxxx>
> > Signed-off-by: Jiri Olsa <jolsa@xxxxxxxxxx>
> > ---
> >   arch/x86/include/asm/syscall_wrapper.h | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> > diff --git a/arch/x86/include/asm/syscall_wrapper.h
> > b/arch/x86/include/asm/syscall_wrapper.h
> > index 59358d1bf880..fd2669b1cb2d 100644
> > --- a/arch/x86/include/asm/syscall_wrapper.h
> > +++ b/arch/x86/include/asm/syscall_wrapper.h
> > @@ -6,7 +6,7 @@
> >   #ifndef _ASM_X86_SYSCALL_WRAPPER_H
> >   #define _ASM_X86_SYSCALL_WRAPPER_H
> 
> > -struct pt_regs;
> > +#include <asm/ptrace.h>
> 
> >   extern long __x64_sys_ni_syscall(const struct pt_regs *regs);
> >   extern long __ia32_sys_ni_syscall(const struct pt_regs *regs);
> > --
> > 2.37.3
>