use BPF_MAP_TYPE_LPM_TRIE for range matching

[Y7n05h]

Short description

#9690

Checklist

I have:

• read the CONTRIBUTING.md document
• compiled this code
• tested this code
• included documentation (including possible behaviour changes)
• documented the code
• added or modified regression test(s)
• added or modified unit test(s)
• checked that this code was merged to master

[Y7n05h]

Like the size of the three maps specified in newBPFFilter, the size of the two newly added maps in blockRange also needs to be specified.
It may not be good practice to add two extra parameters to newBPFFilter to specify the size of the two maps.

[rgacogne]

It may not be good practice to add two extra parameters to newBPFFilter to specify the size of the two maps.

It might be time to use a table-based approach for these parameters instead?

[Y7n05h]

It might be time to use a table-based approach for these parameters instead?

Can you elaborate? I didn't understand what you meant.

[rgacogne]

Can you elaborate? I didn't understand what you meant.

Yes, my bad, I should have included an example right away! If you take a look at, for example, setWebserverConfig https://dnsdist.org/reference/config.html#setWebserverConfig you will see that it takes a Lua associative table instead of a fixed number of parameters. That way we can be much more flexible in what we accept, and add parameters without breaking compatibility with existing configurations.
So I think we have two options for newBPFFilter, we can either:

• add an additional, optional associative table after the existing parameters: newBPFFilter(v4Parameters, v6Parameters, qnamesParameters [, optionalTable])
• or replace all parameters by a single associative table: newBPFFilter(table)

The second one is a lot cleaner but would break compatibility with existing configurations. I'm fine with doing that in 1.8.0 if needed.

[Y7n05h]

Can you elaborate? I didn't understand what you meant.

Yes, my bad, I should have included an example right away! If you take a look at, for example, setWebserverConfig https://dnsdist.org/reference/config.html#setWebserverConfig you will see that it takes a Lua associative table instead of a fixed number of parameters. That way we can be much more flexible in what we accept, and add parameters without breaking compatibility with existing configurations. So I think we have two options for newBPFFilter, we can either:

• add an additional, optional associative table after the existing parameters: newBPFFilter(v4Parameters, v6Parameters, qnamesParameters [, optionalTable])
• or replace all parameters by a single associative table: newBPFFilter(table)

The second one is a lot cleaner but would break compatibility with existing configurations. I'm fine with doing that in 1.8.0 if needed.

I prefer the second one. Compatibility is necessary, but newBPFFilter has extended the 4th parameter in a similar way. It might not be nice to add new optional parameters after optional parameters.

If we add an optional table for newBPFFilter then newBPFFilter will become:

newBPFFilter(v4Parameters, v6Parameters, qnamesParameters [,external] [,optionalTable])

Or we could merge the 4th parameter with the 5th parameter, but this is also not consistent with the previous program. In this case, let's put all the parameters into the table.

[rgacogne]

In this case, let's put all the parameters into the table.

Agreed, that seems to be the best option!

[Y7n05h]

Why do we need use different types of value depending on format == MapFormat::Legacy. Why doesn't MapFormat::Legacy use CounterAndActionValue as value?

pdns/pdns/bpf-filter.cc

Lines 211 to 246 in 053a020

	if (format == MapFormat::Legacy) {
	switch (d_config.d_type) {
	case MapType::IPv4:
	keySize = sizeof(uint32_t);
	valueSize = sizeof(uint64_t);
	break;
	case MapType::IPv6:
	keySize = sizeof(KeyV6);
	valueSize = sizeof(uint64_t);
	break;
	case MapType::QNames:
	keySize = sizeof(QNameKey);
	valueSize = sizeof(QNameValue);
	break;
	default:
	throw std::runtime_error("Unsupported eBPF map type: " + std::to_string(static_cast<uint8_t>(d_config.d_type)));
	}
	}
	else {
	switch (d_config.d_type) {
	case MapType::IPv4:
	keySize = sizeof(uint32_t);
	valueSize = sizeof(CounterAndActionValue);
	break;
	case MapType::IPv6:
	keySize = sizeof(KeyV6);
	valueSize = sizeof(CounterAndActionValue);
	break;
	case MapType::QNames:
	keySize = sizeof(QNameAndQTypeKey);
	valueSize = sizeof(CounterAndActionValue);
	break;
	default:
	throw std::runtime_error("Unsupported eBPF map type: " + std::to_string(static_cast<uint8_t>(d_config.d_type)));
	}
	}

[rgacogne]

Why do we need use different types of value depending on format == MapFormat::Legacy. Why doesn't MapFormat::Legacy use CounterAndActionValue as value?

It's quite frustrating! It comes from the limitations on eBPF socket filter programs in older kernels, where the number of eBPF instructions was very limited, so I couldn't get a composite key based qname + qtype to work and had to use that weird construct where the key is the qname only. We could get rid of that on newer kernels, but that would mean that all of a sudden eBPF filtering would not work on older kernels, which I would like to avoid for a bit longer, at least until major distributions no longer support these kernel versions.
The non-legacy format does not care about that, in part because XDP requires a more recent kernel anyway but also because I'm OK with requiring a recent kernel for new features, as opposed to breaking existing features.
By the way, it means that we will need to be able to build on kernel where LPM trie support does not exist, but of course that feature will not be enabled then.

[rgacogne]

This is really great work, thank you! I have made a few comments and I think there are a couple points we might have to change, but I'm very happy with that pull request 👍

[rgacogne]

I use linux-zen in ArchLinux.

Kernel version: 5.17.5-zen1-1-zen

I test d3d121d and b1419e5 by executing sudo python xdp.py . They work fine for me.

Thanks, that's very helpful! I'm using Arch's hardened kernel, I'll try with -zen.

[rgacogne]

I did not manage to make it work last time I tried, but I will take the time to test this properly this week.

[Y7n05h]

I just tested xdp.py on the linux-zen 5.18.1.zen1-1 and linux 5.18.1.arch1-1 kernels of ArchLinux, respectively, and he works fine. But when testing on linux-hardened 5.17.12.gardened2-1, I encountered this error:

unknown func bpf_probe_read#4

I think this is a linux-hardened specific problem, probably related to the kernel configuration used by linux-hardened.

[rgacogne]

I finally managed to test this properly, sorry again it took me so long.
The code is very good, and apart from a couple issues I added comments for it works pretty well. Thanks a lot!

[rgacogne]

That looks very good, I made a few comments but my feeling is that is pull request is almost done, nice job!

[rgacogne]

I missed it but there is a build error when we build without HAVE_EBPF:

bpf-filter.cc:922:32: error: use of undeclared identifier 'CounterAndActionValue'
  std::vector<std::pair<Netmask, CounterAndActionValue>> BPFFilter::getRangeRule(){
  depbase=`echo dnsdist-console.o | sed 's|[^/]*$|.deps/&|;s|\.o$||'`;\
                                 ^
  bpf-filter.cc:924:10: error: no viable conversion from returned value of type 'std::vector<std::pair<Netmask, CounterAndActionValue>>' to function return type 'int'
    return result;
           ^~~~~~
  2 errors generated.

[Y7n05h]

I missed it but there is a build error when we build without HAVE_EBPF:

Can you post how you built it? I did not successfully reproduce it. The commands I used is:

git clean -fXd
autoreconf -i
./configure --prefix=/usr --sysconfdir=/etc --localstatedir=/var --with-gnutls --with-libsodium --with-libssl --with-nghttp2 --with-re2 --enable-dnstap --enable-dns-over-tls --enable-dns-over-https --enable-dnscrypt
make -j16

[rgacogne]

I missed it but there is a build error when we build without HAVE_EBPF:

Can you post how you built it? I did not successfully reproduce it. The commands I used is:

It's one of the checks in our continuous integration, but the option we are interested in is here is ./configure --without-ebpf

[rgacogne]

Looks good, I'm not 100% sure why the documentation is failing to build but I suggested a few changes that might help.

[rgacogne]

The latest CI failure is unrelated to this PR (Google resolvers were not in a good mood: AXFR-out zone 'example.com', client '127.0.0.1', error resolving for ALIAS google-public-dns-a.google.com., aborting AXFR).

[rgacogne]

Thanks a lot!