Re: [PATCH v3 2/2] ss: pretty-print BPF socket-local storage

[Martin KaFai Lau <martin.lau@linux.dev>]

On 12/20/23 5:23 AM, Quentin Deslandes wrote:
> diff --git a/misc/ss.c b/misc/ss.c
> index 689972d7..6e1ddfa5 100644
> --- a/misc/ss.c
> +++ b/misc/ss.c
> @@ -51,8 +51,13 @@
>   #include <linux/tls.h>
>   #include <linux/mptcp.h>
>   
> +#ifdef HAVE_LIBBPF
> +#include <linux/btf.h>

nit. Instead of adding another "#ifdef HAVE_LIBBPF", move this line to the same 
"#ifdef HAVE_LIBBPF" below?

> +#endif
> +
>   #ifdef HAVE_LIBBPF
>   #include <bpf/bpf.h>
> +#include <bpf/btf.h>
>   #include <bpf/libbpf.h>
>   #endif
>   

[ ... ]

> +static int bpf_maps_opts_load_btf(struct bpf_map_info *info, struct btf **btf)
> +{
> +	if (info->btf_value_type_id) {
> +		*btf = btf__load_from_kernel_by_id(info->btf_id);
> +		if (!*btf) {
> +			fprintf(stderr, "ss: failed to load BTF for map ID %u\n",
> +				info->id);
> +			return -1;
> +		}
> +	} else {
> +		*btf = NULL;
> +	}
> +
> +	return 0;
> +}
> +
>   static int bpf_map_opts_add_all(void)
>   {
>   	unsigned int i;
> @@ -3418,6 +3452,7 @@ static int bpf_map_opts_add_all(void)
>   	while (1) {
>   		struct bpf_map_info info = {};
>   		uint32_t len = sizeof(info);
> +		struct btf *btf;
>   
>   		r = bpf_map_get_next_id(id, &id);
>   		if (r) {
> @@ -3462,8 +3497,18 @@ static int bpf_map_opts_add_all(void)
>   			continue;
>   		}
>   
> +		r = bpf_maps_opts_load_btf(&info, &btf);
> +		if (r) {
> +			fprintf(stderr, "ss: failed to get BTF data for BPF map ID: %u\n",
> +				id);

This will be a duplicated fprintf(stderr) with the bpf_maps_opts_load_btf() 
above. Remove either one of them.

The same goes for the bpf_map_opts_add_id().

[ ... ]

> +static void out_bpf_sk_storage(int map_id, const void *data, size_t len)
> +{
> +	uint32_t type_id;
> +	const struct bpf_sk_storage_map_info *map_info;
> +	struct btf_dump *dump;
> +	struct btf_dump_type_data_opts opts = {
> +		.sz = sizeof(struct btf_dump_type_data_opts),
> +		.indent_str = SK_STORAGE_INDENT_STR,
> +		.indent_level = 2,
> +		.emit_zeroes = 1
> +	};
> +	struct btf_dump_opts dopts = {
> +		.sz = sizeof(struct btf_dump_opts)
> +	};
> +	int r;
> +
> +	map_info = bpf_map_opts_get_info(map_id);
> +	if (!map_info) {
> +		fprintf(stderr, "map_id: %d: missing map info", map_id);

With the 'total_size = RTA_LENGTH(0)' change during show_all == true case in 
patch 1, this fprintf(stderr) should be removed. A new bpf_sk_storage_map has 
just been created after the ss has started which is fine to skip. The next ss 
run will be able to show it.

In the future, it could be improved to give it another try here to get the btf 
info of this new map on-demand.

> +		return;
> +	}
> +
> +	if (map_info->info.value_size != len) {
> +		fprintf(stderr, "map_id: %d: invalid value size, expecting %u, got %lu\n",
> +			map_id, map_info->info.value_size, len);
> +		return;
> +	}
> +
> +	type_id = map_info->info.btf_value_type_id;
> +
> +	dump = btf_dump__new(map_info->btf, out_bpf_sk_storage_print_fn, NULL, &dopts);

nit. just noticed this one also. Instead of recreating the "dump" obj for each 
printed sk, can it be created once and stored in "struct bpf_sk_storage_map_info 
{ ... } maps[MAX_NR_BPF_MAP_ID_OPTS];"?

> +	if (!dump) {
> +		fprintf(stderr, "Failed to create btf_dump object\n");
> +		return;
> +	}
> +
> +	out(SK_STORAGE_INDENT_STR "map_id: %d [\n", map_id);
> +	r = btf_dump__dump_type_data(dump, type_id, data, len, &opts);
> +	if (r < 0)
> +		out(SK_STORAGE_INDENT_STR SK_STORAGE_INDENT_STR "failed to dump data: %d", r);
> +	out("\n" SK_STORAGE_INDENT_STR "]");
> +
> +	btf_dump__free(dump);
> +}
> +
>   static void show_sk_bpf_storages(struct rtattr *bpf_stgs)
>   {
>   	struct rtattr *tb[SK_DIAG_BPF_STORAGE_MAX + 1], *bpf_stg;
> -	unsigned int rem;
> +	unsigned int rem, map_id;
> +	struct rtattr *value;
>   
>   	for (bpf_stg = RTA_DATA(bpf_stgs), rem = RTA_PAYLOAD(bpf_stgs);
>   		RTA_OK(bpf_stg, rem); bpf_stg = RTA_NEXT(bpf_stg, rem)) {
> @@ -3605,8 +3731,13 @@ static void show_sk_bpf_storages(struct rtattr *bpf_stgs)
>   			(struct rtattr *)bpf_stg);
>   
>   		if (tb[SK_DIAG_BPF_STORAGE_MAP_ID]) {
> -			out("map_id:%u",
> -				rta_getattr_u32(tb[SK_DIAG_BPF_STORAGE_MAP_ID]));
> +			out("\n");
> +
> +			map_id = rta_getattr_u32(tb[SK_DIAG_BPF_STORAGE_MAP_ID]);
> +			value = tb[SK_DIAG_BPF_STORAGE_MAP_VALUE];
> +
> +			out_bpf_sk_storage(map_id, RTA_DATA(value),
> +				RTA_PAYLOAD(value));
>   		}
>   	}
>   }
> @@ -6004,6 +6135,11 @@ int main(int argc, char *argv[])
>   		}
>   	}
>   
> +	if (oneline && (bpf_map_opts.nr_maps || bpf_map_opts.show_all)) {

This will not compile if HAVE_LIBBPF is not set. Please test with the 
HAVE_LIBBPF is false condition.

May be revisit my earlier suggestion to create a few helper functions here when 
HAVE_LIBBPF is not set instead of adding "#ifdef HAVE_LIBBPF" here. Something like:

#ifdef HAVE_LIBBPF
static bool bpf_map_opts_is_enabled(void)
{
         return bpf_map_opts.nr_maps;
}
#else
static bool bpf_map_opts_is_enabled(void)
{
         return false;
}
#endif


> +		fprintf(stderr, "ss: --oneline, --bpf-maps, and --bpf-map-id are incompatible\n");
> +		exit(-1);
> +	}
> +
>   	if (show_processes || show_threads || show_proc_ctx || show_sock_ctx)
>   		user_ent_hash_build();
>