----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/75026/#review226625 -----------------------------------------------------------
> In cgroups2, we want our EBPF file to only grant access to a device if it is > in a cgroup's allow list and not in its deny list. > This means that we need to change our previous logic that exits on the first > match to now check both the allow and deny list of a cgroup > before determining whether access may be granted. Could you make the desired vs current behavior more clear? And explain the motivation for the desired behavior? src/linux/cgroups2.hpp Lines 436-439 (patched) <https://reviews.apache.org/r/75026/#comment314889> The second sentence seems redundant here, the first one is enough although it could be more clear (e.g. "be on" -> "match an entry in" src/linux/cgroups2.cpp Lines 1091-1096 (original), 1091-1096 (patched) <https://reviews.apache.org/r/75026/#comment314893> maybe a note here that we use r6 for allow checking purposes? src/linux/cgroups2.cpp Lines 1120-1122 (original), 1118-1135 (patched) <https://reviews.apache.org/r/75026/#comment314894> Maybe explain this a bit more visually? E.g. // ALLOW BLOCK // CHECK R6 TO JUMP TO DENY BLOCKS // ALLOW BLOCK // CHECK R6 TO JUMP TO DENY BLOCKS // ... // DENY BLOCK // EXIT EARLY // DENY BLOCK // EXIT EARLY // ... // CHECK ALLOWED AND NOT DENIED src/linux/cgroups2.cpp Lines 1164 (patched) <https://reviews.apache.org/r/75026/#comment314897> hm.. why do we need register 6 if we're trying to jump past the allow blocks..? seems we can use the allow and deny trailer blocks to assume that no allows or no denies matched src/linux/cgroups2.cpp Lines 1165 (patched) <https://reviews.apache.org/r/75026/#comment314896> we're jumping here if an allow matches early then jumping again? src/linux/cgroups2.cpp Line 1125 (original), 1176-1177 (patched) <https://reviews.apache.org/r/75026/#comment314895> this looks like another case of non-local reasoning..? `_add_device_checks` needs to know that this adds two instructions afterwards to correctly jump past the exit here? It would be nice if we could avoid that by passing in the jump point? this seems to be accomplished by the `1 +` prefix inside the function? maybe these 2 instruction trailers can go inside the function if we jump to the end of the deny checks? that would also be more symmetric? src/linux/cgroups2.cpp Lines 1203 (patched) <https://reviews.apache.org/r/75026/#comment314890> nit: no need for the leading underscore src/linux/cgroups2.cpp Lines 1143-1151 (original), 1210-1218 (patched) <https://reviews.apache.org/r/75026/#comment314891> Maybe split 5 into 2 sections and re-word the explanation here to clarify how it works now? src/linux/cgroups2.cpp Lines 1230 (patched) <https://reviews.apache.org/r/75026/#comment314892> hm.. the member variable tracking this feels quite tricky and requires the reader to know that the caller MUST call this with all the allows first for this variable to make sense here Maybe instead we can just take in an argument of the deny block jump, and this returns the size of the code generated (or maybe the caller can infer that from the program size change?) - Benjamin Mahler On June 3, 2024, 7:45 p.m., Jason Zhou wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/75026/ > ----------------------------------------------------------- > > (Updated June 3, 2024, 7:45 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Repository: mesos > > > Description > ------- > > In cgroups2, we want our EBPF file to only grant access to a device if it is > in a cgroup's allow list and not in its deny list. > This means that we need to change our previous logic that exits on the first > match to now check both the allow and deny list of a cgroup > before determining whether access may be granted. > > This patch implements the logic change, and removes functions that are no > longer necessary for the DeviceProgram class. > We now pass the entire allow and deny list to a configure function inside the > DeviceProgram object, which will create a ebpf program > with the updated logic and attempt to attach it to the cgroup. > > > Diffs > ----- > > src/linux/cgroups2.hpp 64254d04f65128713cf3489b25bcba42590b6020 > src/linux/cgroups2.cpp 9e2ca2207a4e407fb6b07b6fbf709bbc3b397673 > src/tests/containerizer/cgroups2_tests.cpp > cb1e229f7f40aa71f57417c33fccb2cfb313a1f5 > > > Diff: https://reviews.apache.org/r/75026/diff/4/ > > > Testing > ------- > > All Cgroups2 tests pass i.e. the generated ebpf files pass the verifiers, > tests added for new behavior for when device is on both allow and deny list, > and test that mismatched entries are ignored. > > > Thanks, > > Jason Zhou > >
