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

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

On 12/8/23 6:57 AM, Quentin Deslandes wrote:
> +static inline bool bpf_map_opts_is_enabled(void)

nit.

This is the only "inline" usage in this file. I would avoid it and depend on the 
compiler to decide.

> +{
> +	return bpf_map_opts.nr_maps;
> +}
> +

[ ... ]

>   static int inet_show_sock(struct nlmsghdr *nlh,
>   			  struct sockstat *s)
>   {
> @@ -3381,8 +3620,8 @@ static int inet_show_sock(struct nlmsghdr *nlh,
>   	struct inet_diag_msg *r = NLMSG_DATA(nlh);
>   	unsigned char v6only = 0;
>   
> -	parse_rtattr(tb, INET_DIAG_MAX, (struct rtattr *)(r+1),
> -		     nlh->nlmsg_len - NLMSG_LENGTH(sizeof(*r)));
> +	parse_rtattr_flags(tb, INET_DIAG_MAX, (struct rtattr *)(r+1),
> +		nlh->nlmsg_len - NLMSG_LENGTH(sizeof(*r)), NLA_F_NESTED);
>   
>   	if (tb[INET_DIAG_PROTOCOL])
>   		s->type = rta_getattr_u8(tb[INET_DIAG_PROTOCOL]);
> @@ -3479,6 +3718,11 @@ static int inet_show_sock(struct nlmsghdr *nlh,
>   	}
>   	sctp_ino = s->ino;
>   
> +	if (tb[INET_DIAG_SK_BPF_STORAGES]) {
> +		field_set(COL_SKSTOR);
> +		show_sk_bpf_storages(tb[INET_DIAG_SK_BPF_STORAGES]);
> +	}
> +
>   	return 0;
>   }
>   
> @@ -3560,13 +3804,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)
> @@ -3619,6 +3864,17 @@ static int sockdiag_send(int family, int fd, int protocol, struct filter *f)
>   		iovlen += 2;
>   	}
>   
> +	if (bpf_map_opts_is_enabled()) {

This will have compiler error when HAVE_LIBBPF is not set.

> +		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;
> +	}
> +
>   	msg = (struct msghdr) {
>   		.msg_name = (void *)&nladdr,
>   		.msg_namelen = sizeof(nladdr),

[ ... ]

> @@ -5712,6 +5982,16 @@ int main(int argc, char *argv[])
>   		case OPT_INET_SOCKOPT:
>   			show_inet_sockopt = 1;
>   			break;
> +#ifdef HAVE_LIBBPF
> +		case OPT_BPF_MAPS:
> +			if (bpf_map_opts_add_all())
> +				exit(1);
> +			break;
> +		case OPT_BPF_MAP_ID:
> +			if (bpf_map_opts_add_id(optarg))
> +				exit(1);
> +			break;
> +#endif
>   		case 'h':
>   			help();
>   		case '?':
> @@ -5810,6 +6090,9 @@ int main(int argc, char *argv[])
>   	if (!(current_filter.states & (current_filter.states - 1)))
>   		columns[COL_STATE].disabled = 1;
>   
> +	if (bpf_map_opts.nr_maps)

same here when HAVE_LIBBPF is not set

> +		columns[COL_SKSTOR].disabled = 0;
> +
>   	if (show_header)
>   		print_header();
>   
> @@ -5845,6 +6128,7 @@ int main(int argc, char *argv[])
>   
>   	if (show_processes || show_threads || show_proc_ctx || show_sock_ctx)
>   		user_ent_destroy();
> +	bpf_map_opts_destroy();

same here.

A #ifdef is needed.
Another option is to create an empty or always-return-false function.

>   
>   	render();
>