Re: [PATCH 2/3] ss: add support for BPF socket-local storage

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

On 11/27/23 6:30 PM, Quentin Deslandes wrote:
> diff --git a/misc/ss.c b/misc/ss.c
> index 09dc1f37..5b255ce3 100644
> --- a/misc/ss.c
> +++ b/misc/ss.c
> @@ -51,6 +51,11 @@
>   #include <linux/tls.h>
>   #include <linux/mptcp.h>
>   
> +#ifdef HAVE_LIBBPF
> +#include <bpf/bpf.h>
> +#include <bpf/libbpf.h>
> +#endif
> +
>   #if HAVE_RPC
>   #include <rpc/rpc.h>
>   #include <rpc/xdr.h>
> @@ -101,6 +106,7 @@ enum col_id {
>   	COL_RADDR,
>   	COL_RSERV,
>   	COL_PROC,
> +	COL_SKSTOR,
>   	COL_EXT,
>   	COL_MAX
>   };
> @@ -130,6 +136,7 @@ static struct column columns[] = {
>   	{ ALIGN_RIGHT,	"Peer Address:",	" ",	0, 0, 0 },
>   	{ ALIGN_LEFT,	"Port",			"",	0, 0, 0 },
>   	{ ALIGN_LEFT,	"Process",		"",	0, 0, 0 },
> +	{ ALIGN_LEFT,	"Socket storage",	"",	1, 0, 0 },
>   	{ ALIGN_LEFT,	"",			"",	0, 0, 0 },
>   };
>   
> @@ -3368,6 +3375,222 @@ static void parse_diag_msg(struct nlmsghdr *nlh, struct sockstat *s)
>   	memcpy(s->remote.data, r->id.idiag_dst, s->local.bytelen);
>   }
>   
> +#ifdef HAVE_LIBBPF
> +
> +#define MAX_NR_BPF_MAP_ID_OPTS 32
> +
> +struct btf;
> +
> +static struct bpf_map_opts {
> +	unsigned int nr_maps;
> +	struct bpf_sk_storage_map_info {
> +		unsigned int id;
> +		int fd;
> +	} maps[MAX_NR_BPF_MAP_ID_OPTS];
> +	bool show_all;
> +	struct btf *kernel_btf;
> +} bpf_map_opts;
> +
> +static void bpf_map_opts_mixed_error(void)
> +{
> +	fprintf(stderr,
> +		"ss: --bpf-maps and --bpf-map-id cannot be used together\n");
> +}
> +
> +static int bpf_map_opts_add_all(void)
> +{
> +	unsigned int i;
> +	unsigned int fd;
> +	uint32_t id = 0;
> +	int r;
> +
> +	if (bpf_map_opts.nr_maps) {
> +		bpf_map_opts_mixed_error();
> +		return -1;
> +	}
> +
> +	while (1) {
> +		struct bpf_map_info info = {};
> +		uint32_t len = sizeof(info);
> +
> +		r = bpf_map_get_next_id(id, &id);
> +		if (r) {
> +			if (errno == ENOENT)
> +				break;
> +
> +			fprintf(stderr, "ss: failed to fetch BPF map ID\n");
> +			goto err;
> +		}
> +
> +		fd = bpf_map_get_fd_by_id(id);
> +		if (fd == -1) {

The map might be gone. Check for errno == -ENOENT and "continue;" instead of 
"goto err;".

> +			fprintf(stderr, "ss: cannot get fd for BPF map ID %u%s\n",
> +				id, errno == EPERM ?
> +				": missing root permissions, CAP_BPF, or CAP_SYS_ADMIN" : "");
> +			goto err;
> +		}
> +
> +		r = bpf_obj_get_info_by_fd(fd, &info, &len);
> +		if (r) {
> +			fprintf(stderr, "ss: failed to get info for BPF map ID %u\n",
> +				id);
> +			close(fd);
> +			goto err;
> +		}
> +
> +		if (info.type != BPF_MAP_TYPE_SK_STORAGE) {
> +			close(fd);
> +			continue;
> +		}
> +
> +		if (bpf_map_opts.nr_maps == MAX_NR_BPF_MAP_ID_OPTS) {
> +			fprintf(stderr, "ss: too many (> %u) BPF socket-local storage maps found, skipping map ID %u\n",
> +				MAX_NR_BPF_MAP_ID_OPTS, id);
> +			close(fd);
> +			continue;
> +		}
> +
> +		bpf_map_opts.maps[bpf_map_opts.nr_maps].id = id;
> +		bpf_map_opts.maps[bpf_map_opts.nr_maps++].fd = fd;

Not sure how the ss takes care of the fd/memory resources before process exit.

May be the fd(s) need a close() at some point?

> +	}
> +
> +	bpf_map_opts.show_all = true;
> +
> +	return 0;
> +
> +err:
> +	for (i = 0; i < bpf_map_opts.nr_maps; ++i)
> +		close(bpf_map_opts.maps[i].fd);
> +
> +	return -1;
> +}
> +
> +static int bpf_map_opts_add_id(const char *optarg)
> +{
> +	struct bpf_map_info info = {};
> +	uint32_t len = sizeof(info);
> +	size_t optarg_len;
> +	unsigned long id;
> +	unsigned int i;
> +	char *end;
> +	int fd;
> +	int r;
> +
> +	if (bpf_map_opts.show_all) {
> +		bpf_map_opts_mixed_error();
> +		return -1;
> +	}
> +
> +	optarg_len = strlen(optarg);
> +	id = strtoul(optarg, &end, 0);
> +	if (end != optarg + optarg_len || id == 0 || id > UINT32_MAX) {

id >= INT32_MAX

> +		fprintf(stderr, "ss: invalid BPF map ID %s\n", optarg);
> +		return -1;
> +	}
> +
> +	for (i = 0; i < bpf_map_opts.nr_maps; i++) {
> +		if (bpf_map_opts.maps[i].id == id)
> +			return 0;
> +	}
> +
> +	if (bpf_map_opts.nr_maps == MAX_NR_BPF_MAP_ID_OPTS) {
> +		fprintf(stderr, "ss: too many (> %u) BPF socket-local storage maps found, skipping map ID %lu\n",
> +			MAX_NR_BPF_MAP_ID_OPTS, id);
> +		return 0;
> +	}
> +
> +	fd = bpf_map_get_fd_by_id(id);
> +	if (fd == -1) {
> +		fprintf(stderr, "ss: cannot get fd for BPF map ID %lu%s\n",
> +			id, errno == EPERM ?
> +			": missing root permissions, CAP_BPF, or CAP_SYS_ADMIN" : "");
> +		return -1;
> +	}
> +
> +	r = bpf_obj_get_info_by_fd(fd, &info, &len);
> +	if (r) {
> +		fprintf(stderr, "ss: failed to get info for BPF map ID %lu\n", id);
> +		close(fd);
> +		return -1;
> +	}
> +
> +	if (info.type != BPF_MAP_TYPE_SK_STORAGE) {
> +		fprintf(stderr, "ss: BPF map with ID %s has type '%s', expecting 'sk_storage'\n",
> +			optarg, libbpf_bpf_map_type_str(info.type));
> +		close(fd);
> +		return -1;
> +	}
> +
> +	bpf_map_opts.maps[bpf_map_opts.nr_maps].id = id;
> +	bpf_map_opts.maps[bpf_map_opts.nr_maps++].fd = fd;
> +
> +	return 0;
> +}
> +
> +static inline bool bpf_map_opts_is_enabled(void)
> +{
> +	return bpf_map_opts.nr_maps;
> +}
> +
> +static struct rtattr *bpf_map_opts_alloc_rta(void)
> +{
> +	size_t total_size = RTA_LENGTH(RTA_LENGTH(sizeof(int)) * bpf_map_opts.nr_maps);
> +	struct rtattr *stgs_rta, *fd_rta;
> +	unsigned int i;
> +	void *buf;
> +
> +	stgs_rta = malloc(RTA_LENGTH(0));

stgs_rta is malloc()-ed here.

> +	stgs_rta->rta_len = RTA_LENGTH(0);
> +	stgs_rta->rta_type = INET_DIAG_REQ_SK_BPF_STORAGES | NLA_F_NESTED;
> +
> +	buf = malloc(total_size);
> +	if (!buf)
> +		return NULL;
> +
> +	stgs_rta = buf;

and then overwriteen by buf. doesn't look right.

> +	stgs_rta->rta_type = INET_DIAG_REQ_SK_BPF_STORAGES | NLA_F_NESTED;
> +	stgs_rta->rta_len = total_size;
> +
> +	buf = RTA_DATA(stgs_rta);
> +	for (i = 0; i < bpf_map_opts.nr_maps; i++) {
> +		int *fd;
> +
> +		fd_rta = buf;
> +		fd_rta->rta_type = SK_DIAG_BPF_STORAGE_REQ_MAP_FD;
> +		fd_rta->rta_len = RTA_LENGTH(sizeof(int));
> +
> +		fd = RTA_DATA(fd_rta);
> +		*fd = bpf_map_opts.maps[i].fd;
> +
> +		buf += fd_rta->rta_len;
> +	}
> +
> +	return stgs_rta;
> +}