netdev - Re: [RFC iproute2 v6 2/3] ss: pretty-print BPF socket-local storage

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fcee4777-4e46-46c6-8ffd-938b00841958@kernel.org>
Date: Thu, 18 Jan 2024 11:49:33 +0100
From: Matthieu Baerts <matttbe@...nel.org>
To: Quentin Deslandes <qde@...cy.de>, netdev@...r.kernel.org
Cc: Stephen Hemminger <stephen@...workplumber.org>,
 David Ahern <dsahern@...il.com>, Martin KaFai Lau <martin.lau@...nel.org>,
 kernel-team@...a.com
Subject: Re: [RFC iproute2 v6 2/3] ss: pretty-print BPF socket-local storage

Hi Quentin,

Thank you for working on that!

On 18/01/2024 04:15, Quentin Deslandes wrote:
> ss is able to print the map ID(s) for which a given socket has BPF
> socket-local storage defined (using --bpf-maps or --bpf-map-id=). However,
> the actual content of the map remains hidden.
> 
> This change aims to pretty-print the socket-local storage content following
> the socket details, similar to what `bpftool map dump` would do. The exact
> output format is inspired by drgn, while the BTF data processing is similar
> to bpftool's.
> 
> ss will use libbpf's btf_dump__dump_type_data() to ease pretty-printing
> of binary data. This requires out_bpf_sk_storage_print_fn() as a print
> callback function used by btf_dump__dump_type_data(). vout() is also
> introduced, which is similar to out() but accepts a va_list as
> parameter.
> 
> COL_SKSTOR's header is replaced with an empty string, as it doesn't need to
> be printed anymore; it's used as a "virtual" column to refer to the
> socket-local storage dump, which will be printed under the socket information.
> The column's width is fixed to 1, so it doesn't mess up ss' output
> (expect if --oneline is used).

Do you really need this new column, then?

Why not printing that "at the end", in the COL_EXT column, like many
extra and optional bits: TCP Info, CGroups, memory, options, etc.

Here, it seems a bit confusing: if I understand correctly, these extra
and optional bits are handled first, then back to the previous column
you added (COL_SKSTOR) to always iterate over the BPF storages, and
maybe print more stuff only if the new option is given, optionally on
new lines. Would it not print errors even if we didn't ask to display
them, e.g. if the size is different from the expected one?
Would it not be simpler to extend the last column?
If you do that, you will naturally only fetch and iterate over the BPF
storages if it is asked to print something, no?

To be honest, it looks like there are too many options that can be
displayed, and there are probably already enough columns. That's
certainly why no other columns have been added for years. I don't know
why there was an exception for the "Process" one, but OK.
I do think it would be better to have a new "--json" option to structure
the output and ease the parsing, than having workarounds to improve the
output and ease parsing of some options. But that's a more important task :)

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.