ebpf: replace the existing program on update #352

giuseppe · 2020-05-05T17:05:59Z

on a container update, make sure the existing eBPF program is
completely replaced.

Signed-off-by: Giuseppe Scrivano [email protected]

AkihiroSuda · 2020-05-07T04:44:14Z

src/libcrun/ebpf.c

    }

  memset (&attr, 0, sizeof (attr));
  attr.attach_type = BPF_CGROUP_DEVICE;
  attr.target_fd = dirfd;
  attr.attach_bpf_fd = fd;
+#ifdef BPF_F_REPLACE


Please add kernel requirement (v5.6) in the code comment
torvalds/linux@7dd68b3

thanks, I am going to rework the patch and handle better concurrent updates

also, I'll have to move the check at runtime, not build time

rhatdan · 2020-05-11T17:38:29Z

LGTM
@AkihiroSuda PTAL

AkihiroSuda · 2020-05-15T09:16:07Z

src/libcrun/ebpf.c

+  const int MAX_ATTEMPTS = 20;
+  int attempt;
+
+  for (attempt = 0;; attempt++)


maybe attempt = 0; attempt < MAX_ATTEMPTS; attempt++ for readability

I tried to rewrite using the condition inside of the for loop but there is some extra logic that decides whether to retry or not.

AkihiroSuda · 2020-05-15T09:16:54Z

cc @cyphar PTAL

AkihiroSuda · 2020-05-19T13:12:55Z

@cyphar LGTY?

giuseppe · 2020-05-21T16:15:32Z

fine to merge?

cyphar

This is okay as a first draft, but I think we should rethink how we're generating these programs. It might be much nicer in the long term to store the device rules in an eBPF map (which we can atomically swap out if we use a map-of-maps and BPF_F_LOCK). That way we would never have to replace the program, and could just update the eBPF map whenever we needed to. It would also allow us to read the current rule-set from userspace.

cyphar · 2020-05-21T16:13:29Z

src/libcrun/ebpf.c

+        {
+#ifdef BPF_F_REPLACE
+          memset (&attr, 0, sizeof (attr));
+          attr.prog_id = progs[0];


This logic (replacing whatever program is attached) and the corresponding limitations (not being able to replace a program if there is more than one) seems a bit fishy. I think a much better solution would be to either pin the program, or to store the id and load-time into some state file (which I assume crun has) and then to use that to fetch the original program.

To be fair, it's unlikely anybody else will be attaching BPF_CGROUP_DEVICE programs to a container but this seems a bit fishy to me.

cyphar · 2020-05-21T16:14:33Z

src/libcrun/ebpf.c

+              return crun_make_error (err, errno, "cannot open existing eBPF program");
+            }
+#else
+          return crun_make_error (err, 0, "eBPF program already configured");


If BPF_F_REPLACE isn't supported -- which is the case for most kernels -- you should insert the new program then remove the old one. It won't be atomic, but it'll be much better than this.

cyphar · 2020-05-21T16:17:00Z

Also you mentioned you'd do a BPF_F_REPLACE check at runtime -- did you want to include that in this PR?

giuseppe · 2020-05-21T19:51:15Z

This is okay as a first draft, but I think we should rethink how we're generating these programs. It might be much nicer in the long term to store the device rules in an eBPF map (which we can atomically swap out if we use a map-of-maps and BPF_F_LOCK). That way we would never have to replace the program, and could just update the eBPF map whenever we needed to. It would also allow us to read the current rule-set from userspace.

Thanks for the review I had initially tried to implement the devices controller using an eBPF map, but I had issues dealing with wildcards and I am not sure how we could handle them without enumeration or using a more complex map of maps kind of data structure. Do you have any suggestions for it?

In the current implementation, I've assumed that there is a single writer/owner for the cgroup, so there should never been two programs installed. That is the reason for the weird check.

Multiple writers were somehow supported with pinning in the previous versions of this PR but there were also some potential race conditions with multiple crun update running at the same time.

In this version I've tried to deal with concurrent updates and avoid any potential of race conditions.

Currently there is no fallback when BPF_F_REPLACE is missing, just a nicer error message.

Would the fallback for BPF_F_REPLACE look like the following?

get a file lock
read old ID from status or pinned path
write new program
delete old program by ID
if the delete fails: try to delete the new installed program and goto 7
store the new ID/pin it
release the lock

What I don't like is that if something goes wrong we can end up with multiple eBPF programs installed that we lose track of.

giuseppe · 2020-05-23T21:14:01Z

I've changed the implementation and pushed a new version.

It still assumes that crun is the only writer to the cgroup, any other eBPF program is removed on an update.

BPF_F_REPLACE is used only when there is a single program already installed for the cgroup.

In any other case, now crun first stores what programs are already installed for the cgroup, attaches the new program, then it removes all the programs previously installed. Multiple crun update racing with each other are still safe since there is always at least one program attached to the cgroup.

AkihiroSuda · 2020-05-27T23:09:32Z

@cyphar LGTY?

AkihiroSuda · 2020-06-04T02:30:39Z

@cyphar PTAL

Signed-off-by: Giuseppe Scrivano <[email protected]>

on a container update, make sure the existing eBPF programs are completely replaced. Signed-off-by: Giuseppe Scrivano <[email protected]>

giuseppe · 2020-06-10T15:55:47Z

@rhatdan this is ready to go, if there are other comments we can address them later.

rhatdan · 2020-06-11T11:47:03Z

LGTM

giuseppe force-pushed the replace-device branch 2 times, most recently from 4e6d583 to d9aeb6b Compare May 6, 2020 07:38

giuseppe mentioned this pull request May 6, 2020

cgroup: devices updates appear to be broken opencontainers/runc#2366

Closed

2 tasks

giuseppe force-pushed the replace-device branch from 8c6bcb7 to dcda87e Compare May 6, 2020 12:04

AkihiroSuda reviewed May 7, 2020

View reviewed changes

giuseppe marked this pull request as draft May 7, 2020 07:25

giuseppe force-pushed the replace-device branch 3 times, most recently from e00aba6 to 84949c9 Compare May 10, 2020 20:08

giuseppe marked this pull request as ready for review May 10, 2020 20:09

AkihiroSuda reviewed May 15, 2020

View reviewed changes

AkihiroSuda approved these changes May 19, 2020

View reviewed changes

cyphar reviewed May 21, 2020

View reviewed changes

giuseppe force-pushed the replace-device branch 3 times, most recently from b31711d to 42adc81 Compare May 23, 2020 21:06

giuseppe force-pushed the replace-device branch from 42adc81 to 9a81b07 Compare May 24, 2020 12:00

.lgtm.yml: new file

981f3ba

Signed-off-by: Giuseppe Scrivano <[email protected]>

giuseppe force-pushed the replace-device branch from 9a81b07 to 94085c4 Compare June 6, 2020 11:22

ebpf: replace the existing programs on update

87b33ab

on a container update, make sure the existing eBPF programs are completely replaced. Signed-off-by: Giuseppe Scrivano <[email protected]>

giuseppe force-pushed the replace-device branch from 94085c4 to 87b33ab Compare June 7, 2020 11:47

rhatdan merged commit f09475b into containers:master Jun 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ebpf: replace the existing program on update #352

ebpf: replace the existing program on update #352

giuseppe commented May 5, 2020

AkihiroSuda May 7, 2020

giuseppe May 7, 2020

giuseppe May 7, 2020

rhatdan commented May 11, 2020

AkihiroSuda May 15, 2020

giuseppe May 15, 2020

AkihiroSuda commented May 15, 2020

AkihiroSuda commented May 19, 2020

giuseppe commented May 21, 2020

cyphar left a comment

cyphar May 21, 2020

cyphar May 21, 2020

cyphar commented May 21, 2020

giuseppe commented May 21, 2020

giuseppe commented May 23, 2020

AkihiroSuda commented May 27, 2020

AkihiroSuda commented Jun 4, 2020

giuseppe commented Jun 10, 2020

rhatdan commented Jun 11, 2020

ebpf: replace the existing program on update #352

ebpf: replace the existing program on update #352

Conversation

giuseppe commented May 5, 2020

AkihiroSuda May 7, 2020

Choose a reason for hiding this comment

giuseppe May 7, 2020

Choose a reason for hiding this comment

giuseppe May 7, 2020

Choose a reason for hiding this comment

rhatdan commented May 11, 2020

AkihiroSuda May 15, 2020

Choose a reason for hiding this comment

giuseppe May 15, 2020

Choose a reason for hiding this comment

AkihiroSuda commented May 15, 2020

AkihiroSuda commented May 19, 2020

giuseppe commented May 21, 2020

cyphar left a comment

Choose a reason for hiding this comment

cyphar May 21, 2020

Choose a reason for hiding this comment

cyphar May 21, 2020

Choose a reason for hiding this comment

cyphar commented May 21, 2020

giuseppe commented May 21, 2020

giuseppe commented May 23, 2020

AkihiroSuda commented May 27, 2020

AkihiroSuda commented Jun 4, 2020

giuseppe commented Jun 10, 2020

rhatdan commented Jun 11, 2020