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

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

On 1/12/24 6:04 AM, Quentin Deslandes wrote:
> +static struct rtattr *bpf_map_opts_alloc_rta(void)
> +{
> +	struct rtattr *stgs_rta, *fd_rta;
> +	size_t total_size;
> +	unsigned int i;
> +	void *buf;
> +
> +	/* If bpf_map_opts.show_all == true, then bpf_map_opts.nr_maps == 0. We
> +	 * will send an empty message to the kernel, which will return all the
> +	 * socket-local data attached to a socket, no matter their map ID. */
> +	total_size = RTA_LENGTH(RTA_LENGTH(sizeof(int)) * bpf_map_opts.nr_maps);

I have been trying the patch in some heavier traffic machines because I am over 
excited :)

The "--bpf-maps" result is pretty flaky. It does not always print all the 
sk_storage_map.

This line has a bug when using with the "--bpf-maps" cmd opts. The nr_maps will 
become non-zero and will end up not printing all sk_storage_map. Take a look at 
the inet_show_netlink() and there is a "goto again" case.

It really has to test with the bpf_map_opts.show_all here.


> +	buf = malloc(total_size);
> +	if (!buf)
> +		return NULL;
> +
> +	stgs_rta = buf;
> +	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;
> +}
> +

[ ... ]

> @@ -3564,13 +3767,14 @@ static int sockdiag_send(int family, int fd, int protocol, struct filter *f)
>   {
>   	struct sockaddr_nl nladdr = { .nl_family = AF_NETLINK };
>   	DIAG_REQUEST(req, struct inet_diag_req_v2 r);
> +	struct rtattr *bpf_stgs_rta = NULL;
>   	char    *bc = NULL;
>   	int	bclen;
>   	__u32	proto;
>   	struct msghdr msg;
>   	struct rtattr rta_bc;
>   	struct rtattr rta_proto;
> -	struct iovec iov[5];
> +	struct iovec iov[6];
>   	int iovlen = 1;
> 
>   	if (family == PF_UNSPEC)
> @@ -3623,6 +3827,19 @@ static int sockdiag_send(int family, int fd, int protocol, struct filter *f)
>   		iovlen += 2;
>   	}
> 
> +#ifdef HAVE_LIBBPF
> +	if (bpf_map_opts_is_enabled()) {
> +		bpf_stgs_rta = bpf_map_opts_alloc_rta();
> +		if (!bpf_stgs_rta) {
> +			fprintf(stderr, "ss: cannot alloc request for --bpf-map\n");
> +			return -1;
> +		}
> +
> +		iov[iovlen++] = (struct iovec){ bpf_stgs_rta, bpf_stgs_rta->rta_len };
> +		req.nlh.nlmsg_len += bpf_stgs_rta->rta_len;
> +	}
> +#endif
> +
>   	msg = (struct msghdr) {
>   		.msg_name = (void *)&nladdr,
>   		.msg_namelen = sizeof(nladdr),
> @@ -3631,10 +3848,13 @@ static int sockdiag_send(int family, int fd, int protocol, struct filter *f)
>   	};
> 
>   	if (sendmsg(fd, &msg, 0) < 0) {
> +		free(bpf_stgs_rta);
>   		close(fd);
>   		return -1;
>   	}
> 
> +	free(bpf_stgs_rta);
> +
>   	return 0;
>   }