netdev - Re: [PATCH iproute2 v8 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: <9610f33d-ee98-4b95-a776-a203d83401bf@naccy.de>
Date: Wed, 21 Feb 2024 12:00:00 +0100
From: Quentin Deslandes <qde@...cy.de>
To: David Ahern <dsahern@...il.com>, netdev@...r.kernel.org
Cc: Martin KaFai Lau <martin.lau@...nel.org>,
 Stephen Hemminger <stephen@...workplumber.org>, kernel-team@...a.com,
 Matthieu Baerts <matttbe@...nel.org>
Subject: Re: [PATCH iproute2 v8 1/3] ss: add support for BPF socket-local
 storage

On 2024-02-21 10:51, Quentin Deslandes wrote:
> On 2024-02-18 18:39, David Ahern wrote:
>> On 2/14/24 1:42 AM, Quentin Deslandes wrote:
>>> +	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;
>>> +	}
>>
>> ss.c: In function ‘bpf_map_opts_load_info’:
>> ss.c:3448:33: warning: implicit declaration of function
>> ‘libbpf_bpf_map_type_str’ [-Wimplicit-function-declaration]
>>  3448 |                         optarg, libbpf_bpf_map_type_str(info.type));
>>       |                                 ^~~~~~~~~~~~~~~~~~~~~~~
>> ss.c:3447:68: warning: format ‘%s’ expects argument of type ‘char *’,
>> but argument 4 has type ‘int’ [-Wformat=]
>>  3447 |                 fprintf(stderr, "ss: BPF map with ID %s has type
>> '%s', expecting 'sk_storage'\n",
>>       |                                                                   ~^
>>       |                                                                    |
>>       |
>>   char *
>>       |                                                                   %d
>>  3448 |                         optarg, libbpf_bpf_map_type_str(info.type));
>>       |                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>       |                                 |
>>       |                                 int
>>     CC       lnstat_util.o
>>     LINK     lnstat
>>     LINK     ss
>> /usr/bin/ld: ss.o: in function `main':
>>
>>
>> Ubuntu 22.04 has libbpf-0.5 installed. I suspect version hook is needed.
>> e.g., something like this (but with the relevant version numbers):
>>
>> #if (LIBBPF_MAJOR_VERSION > 0) || (LIBBPF_MINOR_VERSION >= 7)
> 
> After checking, all the libbpf symbols I use require at least libbpf-0.5, except
> for this one which was introduced in libbpf-1.0. Hence, I will print the type ID
> instead of the string. IMO printing the string representation of the type doesn't
> add enough value to justify adding a version hook.
> 
> However, I see the minimum required version for libbpf is 0.1, but this
> series requires 0.5. I would check the version in ss and #error if
> the requirements are not met, but I'm not sure this is the right way to do it.
> What do you think?

I've settled on a slightly different solution: the BPF socket-local code is gated
by ENABLE_BPF_SKSTORAGE_SUPPORT instead of HAVE_LIBBPF. If libbpf-0.5+ is available,
and HAVE_LIBBPF is defined, ENABLE_BPF_SKSTORAGE_SUPPORT will be defined in ss,
enabling this feature. If HAVE_LIBBPF is defined but libbpf-0.5+ is not available,
a compile warning will be printed, ENABLE_BPF_SKSTORAGE_SUPPORT won't be defined in
ss, and ss will be compiled without socket-local storage support.

This will ensure that:
- Features relying on HAVE_LIBBPF in ss don't have to comply with the same requirements
  as the BPF socket-local storage support (because iproute2 only requires libbpf-0.1+).
- This change won't prevent iproute2 as a whole from being compiled.

This seems much more reasonable than using #error and failing the whole build. I'll
send a v9 with these changes.

Regards,
Quentin Deslandes