netdev - Re: [PATCH v4 1/3] ss: add support for 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: <75243f6c-f316-47f5-89ae-f203174cd0ad@linux.dev>
Date: Fri, 12 Jan 2024 14:50:41 -0800
From: Martin KaFai Lau <martin.lau@...ux.dev>
To: Quentin Deslandes <qde@...cy.de>
Cc: David Ahern <dsahern@...il.com>, Martin KaFai Lau
 <martin.lau@...nel.org>, kernel-team@...a.com, netdev@...r.kernel.org
Subject: Re: [PATCH v4 1/3] ss: add support for BPF socket-local storage

On 1/12/24 6:04 AM, Quentin Deslandes wrote:
> +static int bpf_map_opts_load_info(unsigned int map_id)
> +{
> +	struct bpf_map_info info = {};
> +	uint32_t len = sizeof(info);
> +	int fd;
> +	int r;
> +
> +	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, map_id);
> +		return 0;
> +	}
> +
> +	fd = bpf_map_get_fd_by_id(map_id);
> +	if (fd == -1) {

I also just noticed libbpf returns -errno (from libbpf_err_errno()), so better 
check for < 0 here.

> +		if (errno == -ENOENT)
> +			return 0;
> +
> +		fprintf(stderr, "ss: cannot get fd for BPF map ID %u%s\n",
> +			map_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 %u\n",
> +			map_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 = map_id;
> +	bpf_map_opts.maps[bpf_map_opts.nr_maps++].fd = fd;
> +
> +	return 0;
> +}
> +
> +static struct bpf_sk_storage_map_info *bpf_map_opts_get_info(
> +	unsigned int map_id)
> +{
> +	unsigned int i;
> +	int r;
> +
> +	for (i = 0; i < bpf_map_opts.nr_maps; ++i) {
> +		if (bpf_map_opts.maps[i].id == map_id)
> +			return &bpf_map_opts.maps[i];
> +	}
> +
> +	r = bpf_map_opts_load_info(map_id);
> +	if (r)
> +		return NULL;
> +
> +	return &bpf_map_opts.maps[bpf_map_opts.nr_maps - 1];
> +}
> +
> +static int bpf_map_opts_add_id(const char *optarg)
> +{
> +	size_t optarg_len;
> +	unsigned long id;
> +	char *end;
> +
> +	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) {
> +		fprintf(stderr, "ss: invalid BPF map ID %s\n", optarg);
> +		return -1;
> +	}
> +
> +	// Force lazy loading of the map's data.
> +	if (!bpf_map_opts_get_info(id))
> +		return -ENOENT;

nit. may be also "return -1;" here to be consistent with the above error returns.

Other than the minor nits, lgtm, you can carry my ack in the next spin:

Acked-by: Martin KaFai Lau <martin.lau@...nel.org>