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

[Ian Rogers]

On Thu, Oct 1, 2026 at 2:07 AM Arnaldo Carvalho de Melo <acme@xxxxxxxxxx> wrote:
>
> On Wed, Sep 30, 2026 at 04:08:17PM -0700, Namhyung Kim wrote:
> > 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:
> > > > +++ 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;
>
> > > 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.
>
> I was missing the convention that a negative size means read some
> other argument with a cap, as Ian explained in his response.
>
> So checking if a syscall arg is of type sockaddr (or if the name is
> always sockaddr as Ian did above) and the next arg has name "len", then
> we can set the beauty_array[index_of_sockaddr_arg] = -index_of_len_arg,
> that extra + 1 looks odd, but must be part of the convention too.

Thanks for merging the series!

Yes, the two + 1s are:
- (i + 1): the 0-based index of the length argument right after the
sockaddr argument.
- -(j + 1): beauty_array's 1-based negative encoding (so arg 0 is -1
rather than 0), which augment_arg() decodes with index = -(size + 1).

> Since we have it there already and we may not have BTF and BTF isn't yet
> a hard requirement, we leave the fallbacks in place for the time being?

Agreed, keeping the fallbacks for now makes sense. (Also, in
trace__bpf_sys_enter_beauty_map(), only the struct/union size lookup
actually uses trace->btf; strings, buffers and sockaddr only use the
tracepoint format fields, so moving the trace->btf check into the
bt->size branch would let those work without BTF too.)

> I guess that clamp can be made bigger, 64 maybe? Applying the series
> now.
[ ... ]
> And copying the sockaddr parameters according to its addrlen or the cap
> in the augmented bpf, that probably needs to be a bit bigger for local
> sockets:
>
>   ⬢ [acme@toolbx perf-tools-next]$ echo -n /var/run/.heim_org.h5l.kcm-soc | wc -c
>   30
>   ⬢ [acme@toolbx perf-tools-next]$ ls -la /var/run/.heim_org.h5l.kcm-socket
>   srw-rw-rw-. 1 nobody nobody 0 Sep 14 23:13 /var/run/.heim_org.h5l.kcm-socket

Right, 32 bytes is 2 bytes of sa_family plus 30 bytes of sun_path, so
"/var/run/.heim_org.h5l.kcm-socket" lost its last 3 characters.
augmented_arg->value already has room for PATH_MAX (4096) bytes, so
raising TRACE_AUG_MAX_BUF to 64 (or 128, i.e. SS_MAXSIZE, which covers
all 110 bytes of struct sockaddr_un) works without changing the map
layout. Since syscall_arg__scnprintf_buf() prints all
augmented_arg->size bytes for write(), if we want to keep write()
buffers at 32 bytes while giving sockaddr up to 128 bytes, we could
either cap syscall_arg__scnprintf_buf() at 32 or use a separate
negative range in beauty_array for sockaddr. Happy to send a follow-up
patch whichever way you prefer.

Thanks,
Ian