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

[Arnaldo Carvalho de Melo <acme@kernel.org>]

On Thu, Oct 01, 2026 at 10:17:24AM -0700, Ian Rogers wrote:
> On Thu, Oct 1, 2026 at 2:07 AM Arnaldo Carvalho de Melo <acme@kernel.org> 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!

Good stuff, you're welcome to send more ;-) :-)
 
> 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).

would be good to have some comment here or there about it.
 
> > 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.

I think we need to cover the max size for sockaddr since it doesn't add
space costs to what we have already, if we want to have a shorter, by
default, capture for write, then we need another knob for that, one that
applies to write and other user->kernel typeless payloads.

Thanks,

- Arnaldo