Re: [PATCH v6 06/26] perf trace: Copy sockaddr arguments by their length

[Namhyung Kim]

On Wed, Sep 30, 2026 at 09:14:42PM +0200, Arnaldo Carvalho de Melo wrote:
> On Mon, Sep 28, 2026 at 11:25:45AM -0700, Ian Rogers wrote:
> > The BTF augmenter copies sizeof(struct sockaddr), 16 bytes, for the
> > sockaddr arguments of connect, bind and sendto, and runs before the
> > sys_enter_connect and sys_enter_sendto programs that copy their length.
> > An IPv6 address needs 24 or 28 bytes, so the rest was read from past the
> > payload, and now that the printers are bounded only its family is shown.
> > 
> > Copy them as buffers sized by the length argument after them, up to the
> > 32 bytes augmented buffers are limited to. That holds an IPv6 address
> > and doubles the AF_LOCAL path shown.
> > 
> > Fixes: a68fd6a6cdd3 ("perf trace: Collect augmented data using BPF")
> > Assisted-by: Antigravity:gemini-3.1-pro
> > Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>
> > ---
> >  tools/perf/builtin-trace.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> > index 85db74965280..f94745a60f4a 100644
> > --- a/tools/perf/builtin-trace.c
> > +++ b/tools/perf/builtin-trace.c
> > @@ -4159,7 +4159,12 @@ static int trace__bpf_sys_enter_beauty_map(struct trace *trace, int e_machine, i
> >  				continue;
> >  
> >  			bt = sc->arg_fmt[i].type;
> > -			beauty_array[i] = bt->size;
> > +			/* Copy a sockaddr as a buffer sized by the next argument, e.g. addrlen. */
> > +			if (strcmp(name, "sockaddr") == 0 && field->next &&
> > +			    strstr(field->next->name, "len"))
> > +				beauty_array[i] = -((i + 1) + 1);
> > +			else
> > +				beauty_array[i] = bt->size;
> 
> 
> Humm, I thought that this would be called in BPF handlers like:
> 
> SEC("tp/syscalls/sys_enter_connect")
> int sys_enter_connect(struct syscall_enter_args *args)
> {
>         struct augmented_args_payload *augmented_args = augmented_args_payload();
>         const void *sockaddr_arg = (const void *)args->args[1];
>         unsigned int socklen = args->args[2];
>         unsigned int len = sizeof(u64) + sizeof(augmented_args->args); // the size + err in all 'augmented_arg' structs
> 
>         if (augmented_args == NULL)
>                 return 1; /* Failure: don't filter */
> 
>         _Static_assert(is_power_of_2(sizeof(augmented_args->arg.saddr)), "sizeof(augmented_args->arg.saddr) needs to be a power of two");
>         socklen &= sizeof(augmented_args->arg.saddr) - 1;
> 
>         bpf_probe_read_user(&augmented_args->arg.saddr, socklen, sockaddr_arg);
>         augmented_args->arg.size = socklen;
>         augmented_args->arg.err = 0;
> 
>         return augmented__output(args, augmented_args, len + socklen);
> }
> 
> And it knows how many bytes to read by looking at socklen
> (args->args[2]), i.e. not use the generic BPF handler that uses this
> beauty_array, because knowing how many bytes to read in this case is
> dynamic, varies with each syscall, according to one of its arguments :-\
> 
> What am I missing?

I think Ian's patch update the beauty map which is used by
augment_sys_enter() before tail-calling syscall-specific functions.

It'd be great if we cover all syscalls in the BPF skeleton and switch
to the beauty-map and discard the functions.

Thanks,
Namhyung