> On July 24, 2024, 10:15 p.m., Benjamin Mahler wrote: > > src/slave/containerizer/device_manager/device_manager.cpp > > Lines 177-197 (patched) > > <https://reviews.apache.org/r/75099/diff/4/?file=2291847#file2291847line177> > > > > Ok so this assumes that we've normalized the lists, i.e. > > > > 1. wildcard allow entries must not be adding permissions to > > non-wildcard allow entries: > > > > `b 1:* m` > > `b 1:2 rw` > > is not allowed because the wildcard is granting m access, this must be > > normalized to: > > > > `b 1:* m` > > `b 1:2 rwm` > > > > 2. Ditto for deny entries. > > > > It would be good to codify this more clearly, e.g.: > > > > 1. CgroupDeviceAccess is only constructable in a normalized way (e.g. > > Try<CgroupDeviceAccess> CgroupDeviceAccess::create(...) function that > > rejects invalid. > > > > 2. Adding a CHECK(normalized()) inside this function before we run this > > logic. And there's a normalize() function that can be called on it as well > > by whoever is editing the struct.
We add validation here, the normalize() function is addded in a separate patch here: https://reviews.apache.org/r/75104/ - Jason ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/75099/#review226716 ----------------------------------------------------------- On July 25, 2024, 7:45 p.m., Jason Zhou wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/75099/ > ----------------------------------------------------------- > > (Updated July 25, 2024, 7:45 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Repository: mesos > > > Description > ------- > > Currently we assume that a device state is normalized before using it > for generating ebpf files. However, we have not been enforcing these > constraints on the device access state. > > We enforce some basic validation on cgroups2::configure on the state > to ensure that we are able to generate a correct ebpf program. > An allow or deny list is 'normalized' iff everything below are true: > 1. No Entry can have no accesses specified > 2. No two entries on the same list can have the same type, major & minor > numbers. > 3. No two entries on the same list can be encompassed by the other > entry. > > This patch adds helpers to check if a device state is normalized, > and will only allow users to create new CgroupDeviceAccess instances > using a helper that checks that the allow and deny lists are normalized. > A new helper function is added to check if an entry would be granted > access, and requires the state to be normalized. > > > Diffs > ----- > > src/linux/cgroups2.hpp accaebdaddc85acdd96b87161ea441c77b025099 > src/linux/cgroups2.cpp cb3c425a46f33f5434f870c03dd7de5be3331ece > src/slave/containerizer/device_manager/device_manager.hpp > 7c8523d8bdddb8e95c47e1812b48520296680ad6 > src/slave/containerizer/device_manager/device_manager.cpp > 4c9b86393f0809e08d79b6354940826bd56116f2 > src/tests/containerizer/cgroups2_tests.cpp > c73045632f1bc102d42852b9095a4fe6e11839bb > src/tests/device_manager_tests.cpp 54d464e97c8fd179128239a6757b16dfa0147c54 > > > Diff: https://reviews.apache.org/r/75099/diff/5/ > > > Testing > ------- > > Added tests for DeviceManager::CgroupDeviceAccess::is_access_granted. Tests > passed. > > > Thanks, > > Jason Zhou > >
