dnsdist: Raise RLIMIT_MEMLOCK automatically when eBPF is requested #11554

yog-singh · 2022-04-17T11:49:45Z

Short Description:

Raise RLIMIT_MEMLOCK automatically when eBPF is requested.

#10566
This PR adds changes to eBPF filter constructor which when invoked automatically raises the RLIMIT_MEMLOCK from 64k to 1024k.
The hard limit for the user needs to be set in /etc/security/limits.conf.

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

rgacogne

The code looks good, I made a few suggestions that are pretty much all around logging, nothing serious. Thanks a lot!

pdns/bpf-filter.cc

pdns/dnsdistdist/dnsdist.service.in

pdns/dnsdistdist/docs/advanced/ebpf.rst

pdns/bpf-filter.cc

rgacogne · 2022-04-17T13:48:54Z

pdns/bpf-filter.cc

@@ -320,6 +322,25 @@ BPFFilter::BPFFilter(std::unordered_map<std::string, MapConfiguration>& configs,
    throw std::runtime_error("Unsupported eBPF map format, the current internal implemenation only supports the legacy format");
  }

+  struct rlimit old_limit;
+  const size_t new_limit_size = 1073741824;


Hmmm, if the target is 1024k, should that number be 1048576? And for the 64k check, 65536 instead of 67108865? If so, I guess we also need to change the $USER hard memlock 1073741824 line in the documentation.

The rlim_cur and rlim_max returned by getrlimit is a multiplied value of 64k, actually multiplying it by 1024. While what we set in limit.conf is exact value as you mentioned. So I should change in the documentation, but will check in the code what the getrlimit function call returns and why is the value different from what I get from ulimit -a

ulimit uses memory sizes in kbytes. getrlimit and setrlimit use bytes.

I'm confused, are we trying to raise the limit to 1024 kilobytes or more? If we are trying to raise it to 1024 kilobytes, I believe we need to replace 1024 * 1024 * 1024 by 1024 * 1024 and, 64 * 1024 * 1024 by 64 * 1024, right?

pdns/dnsdistdist/dnsdist.service.in

pdns/bpf-filter.cc

omoerbeek · 2022-04-29T06:16:15Z

pdns/bpf-filter.cc

+    throw std::runtime_error("Unable to get memory lock limit: " + stringerror());
+  }
+
+  /* Check if the current soft memlock limit is 64k */ 


comments and code do not match. k or M?

omoerbeek · 2022-04-29T06:16:51Z

pdns/bpf-filter.cc

+  /* Check if the current soft memlock limit is 64k */ 
+  if (old_limit.rlim_cur < (64 * 1024 * 1024)) {
+    struct rlimit new_limit;
+    new_limit.rlim_cur = new_limit_size; /* Increase soft limit to 1024k */


Same here: code and comment do not match.

omoerbeek · 2022-04-29T06:19:01Z

pdns/bpf-filter.cc

+  if (old_limit.rlim_cur < (64 * 1024 * 1024)) {
+    struct rlimit new_limit;
+    new_limit.rlim_cur = new_limit_size; /* Increase soft limit to 1024k */
+    new_limit.rlim_max = new_limit_size; /* Increase hard limit to 1024k */


Rasing the hard limit will fail for non-root. Or are we always root at this point?

For eBPF filter I think we need to be root at this point.

We might only have CAP_BPF at this point, but failing is OK as long as we keep going on IMHO.

omoerbeek · 2022-04-29T06:20:15Z

pdns/dnsdistdist/docs/advanced/ebpf.rst

@@ -73,3 +73,6 @@ Since 1.6.0, the default BPF filter set via :func:`setDefaultBPFFilter` will aut
 That feature might require an increase of the memory limit associated to a socket, via the sysctl setting ``net.core.optmem_max``.
 When attaching an eBPF program to a socket, the size of the program is checked against this limit, and the default value might not be enough.
 Large map sizes might also require an increase of ``RLIMIT_MEMLOCK``, which can be done by adding ``LimitMEMLOCK=infinity`` in the systemd unit file.
+


I don't like infinity limits.

We are not going ahead with infinity limit

infinity is still mentioned on line 75.

Also: there is a unit confusion here: the code in bpf-filter.cc uses 1 mega byte (setrlimit's unit is bytes), while the /security/limits.conf example set the limit to 1GB (it uses 1k byte as unit).
Note that LimitMEMLOCK in a systemd unit file uses bytes as unit.

Raise RLIMIT_MEMLOCK automatically when eBPF is requested. This PR adds changes to eBPF filter constructor which when invoked automatically raises the RLIMIT_MEMLOCK from 64k to 1024k. The hard limit for the user needs to be set in `/etc/security/limits.conf`.

omoerbeek · 2022-12-07T08:33:45Z

Rebased to fix conflicts and adapted slightly.

rgacogne · 2022-12-07T08:53:46Z

pdns/bpf-filter.cc

+  }
+
+  /* Check if the current soft memlock limit is 64k */ 
+  if (old_limit.rlim_cur < (64 * 1024)) {


So we will only raise the limit when it is below 64k, right? But we know 64k is not enough, so perhaps it would make sense to try to raise it if it is below 1024k instead?

rgacogne · 2022-12-07T08:54:47Z

pdns/bpf-filter.cc

+  if (old_limit.rlim_cur < (64 * 1024 * 1024)) {
+    struct rlimit new_limit;
+    new_limit.rlim_cur = new_limit_size; /* Increase soft limit to 1024k */
+    new_limit.rlim_max = new_limit_size; /* Increase hard limit to 1024k */


We might only have CAP_BPF at this point, but failing is OK as long as we keep going on IMHO.

rgacogne · 2022-12-07T08:55:03Z

pdns/bpf-filter.cc

+    new_limit.rlim_max = new_limit_size; /* Increase hard limit to 1024k */
+
+    if (setrlimit(RLIMIT_MEMLOCK, &new_limit) != 0) {
+      errlog("Unable to raise the maximum amount of locked memory for eBPF from %d to %d, consider raising RLIMIT_MEMLOCK or setting LimitMEMLOCK=infinity in the systemd unit: %s", old_limit.rlim_cur, new_limit.rlim_cur, stringerror());


Suggested change

errlog("Unable to raise the maximum amount of locked memory for eBPF from %d to %d, consider raising RLIMIT_MEMLOCK or setting LimitMEMLOCK=infinity in the systemd unit: %s", old_limit.rlim_cur, new_limit.rlim_cur, stringerror());

warnlog("Unable to raise the maximum amount of locked memory for eBPF from %d to %d, consider raising RLIMIT_MEMLOCK or setting LimitMEMLOCK=infinity in the systemd unit: %s", old_limit.rlim_cur, new_limit.rlim_cur, stringerror());

rgacogne · 2022-12-07T08:55:43Z

pdns/bpf-filter.cc

+    if (setrlimit(RLIMIT_MEMLOCK, &new_limit) != 0) {
+      errlog("Unable to raise the maximum amount of locked memory for eBPF from %d to %d, consider raising RLIMIT_MEMLOCK or setting LimitMEMLOCK=infinity in the systemd unit: %s", old_limit.rlim_cur, new_limit.rlim_cur, stringerror());
+    }
+    infolog("The current limit of locked memory (soft: %d, hard: %d) is too low for eBPF, trying to raise it to %d", old_limit.rlim_cur, old_limit.rlim_max, new_limit_size);


I think we should move that comment before the setrlimit call, otherwise we might log the failure before the attempt.

rgacogne · 2022-12-07T12:18:35Z

pdns/bpf-filter.cc

+  const rlim_t new_limit_size = 1024 * 1024;
+
+  /* Check if the current soft memlock limit is at least minimal_limit */
+  if (old_limit.rlim_cur < minimal_limit_size) {


I think this is not very useful to raise the limit if the current value is below 64k, as most systems set RLIMIT_MEMLOCK to exactly 64k, and we know this is not enough for most usage (decent-sized maps). How about we try to raise that value to 1024k if it is below 1024k?

That makes sense indeed

rgacogne

Looks very good, thanks!

omoerbeek · 2022-12-08T08:55:29Z

One unrelated CI failure.

rgacogne added enhancement dnsdist labels Apr 17, 2022

rgacogne added this to the dnsdist-1.8.0 milestone Apr 17, 2022

rgacogne self-requested a review April 17, 2022 12:04

rgacogne requested changes Apr 17, 2022

View reviewed changes

yog-singh force-pushed the yog-singh/ddist-ebpf-memlock-limit branch 4 times, most recently from 423cc11 to f24884b Compare April 17, 2022 13:05

yog-singh requested a review from rgacogne April 17, 2022 13:15

rgacogne reviewed Apr 17, 2022

View reviewed changes

pdns/bpf-filter.cc Outdated Show resolved Hide resolved

yog-singh force-pushed the yog-singh/ddist-ebpf-memlock-limit branch from f24884b to 569787b Compare April 17, 2022 13:23

rgacogne reviewed Apr 17, 2022

View reviewed changes

yog-singh force-pushed the yog-singh/ddist-ebpf-memlock-limit branch from 569787b to 5ac199f Compare April 17, 2022 16:54

yog-singh requested a review from rgacogne April 17, 2022 16:55

omoerbeek requested changes Apr 19, 2022

View reviewed changes

pdns/bpf-filter.cc Outdated Show resolved Hide resolved

pdns/bpf-filter.cc Outdated Show resolved Hide resolved

pdns/bpf-filter.cc Outdated Show resolved Hide resolved

yog-singh force-pushed the yog-singh/ddist-ebpf-memlock-limit branch from 5ac199f to e571a15 Compare April 20, 2022 18:47

yog-singh requested a review from omoerbeek April 20, 2022 18:48

omoerbeek reviewed Apr 29, 2022

View reviewed changes

yog-singh force-pushed the yog-singh/ddist-ebpf-memlock-limit branch from e571a15 to 877673c Compare May 29, 2022 19:09

yog-singh requested a review from omoerbeek June 3, 2022 07:49

yog-singh force-pushed the yog-singh/ddist-ebpf-memlock-limit branch from 877673c to 8f29b09 Compare June 16, 2022 03:08

omoerbeek force-pushed the yog-singh/ddist-ebpf-memlock-limit branch from 8f29b09 to d418ba4 Compare December 7, 2022 08:30

Short Description:

dc22be5

Raise RLIMIT_MEMLOCK automatically when eBPF is requested. This PR adds changes to eBPF filter constructor which when invoked automatically raises the RLIMIT_MEMLOCK from 64k to 1024k. The hard limit for the user needs to be set in `/etc/security/limits.conf`.

omoerbeek force-pushed the yog-singh/ddist-ebpf-memlock-limit branch from d418ba4 to dc22be5 Compare December 7, 2022 08:32

rgacogne requested changes Dec 7, 2022

View reviewed changes

Incorporate comments from @rgacogne

ad97958

omoerbeek force-pushed the yog-singh/ddist-ebpf-memlock-limit branch from 1e48578 to ad97958 Compare December 7, 2022 10:08

omoerbeek requested a review from rgacogne December 7, 2022 10:09

rgacogne reviewed Dec 7, 2022

View reviewed changes

Only raise the limit to 1M if the current is lower than 1M

7b6e1bb

omoerbeek requested a review from rgacogne December 7, 2022 13:10

rgacogne approved these changes Dec 8, 2022

View reviewed changes

omoerbeek approved these changes Dec 8, 2022

View reviewed changes

omoerbeek merged commit b82a878 into PowerDNS:master Dec 8, 2022

rgacogne mentioned this pull request Jan 16, 2023

dnsdist: Ponder trying to raise RLIMIT_MEMLOCK automatically when eBPF is requested #10566

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dnsdist: Raise RLIMIT_MEMLOCK automatically when eBPF is requested #11554

dnsdist: Raise RLIMIT_MEMLOCK automatically when eBPF is requested #11554

yog-singh commented Apr 17, 2022 •

edited

Loading

rgacogne left a comment

rgacogne Apr 17, 2022

yog-singh Apr 17, 2022

omoerbeek Apr 17, 2022

rgacogne Apr 25, 2022

omoerbeek Apr 29, 2022

omoerbeek Apr 29, 2022

omoerbeek Apr 29, 2022

yog-singh May 29, 2022 •

edited

Loading

rgacogne Dec 7, 2022

omoerbeek Apr 29, 2022

yog-singh May 29, 2022

omoerbeek Jun 8, 2022

omoerbeek commented Dec 7, 2022

rgacogne Dec 7, 2022

rgacogne Dec 7, 2022

rgacogne Dec 7, 2022

rgacogne Dec 7, 2022

rgacogne Dec 7, 2022

omoerbeek Dec 7, 2022

rgacogne left a comment

omoerbeek commented Dec 8, 2022

	errlog("Unable to raise the maximum amount of locked memory for eBPF from %d to %d, consider raising RLIMIT_MEMLOCK or setting LimitMEMLOCK=infinity in the systemd unit: %s", old_limit.rlim_cur, new_limit.rlim_cur, stringerror());
	warnlog("Unable to raise the maximum amount of locked memory for eBPF from %d to %d, consider raising RLIMIT_MEMLOCK or setting LimitMEMLOCK=infinity in the systemd unit: %s", old_limit.rlim_cur, new_limit.rlim_cur, stringerror());

dnsdist: Raise RLIMIT_MEMLOCK automatically when eBPF is requested #11554

dnsdist: Raise RLIMIT_MEMLOCK automatically when eBPF is requested #11554

Conversation

yog-singh commented Apr 17, 2022 • edited Loading

Short Description:

Checklist

rgacogne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yog-singh May 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omoerbeek commented Dec 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgacogne left a comment

Choose a reason for hiding this comment

omoerbeek commented Dec 8, 2022

yog-singh commented Apr 17, 2022 •

edited

Loading

yog-singh May 29, 2022 •

edited

Loading