Skip to content

libct/cgroups/fscommon: add openat2() support#2598

Closed
kolyshkin wants to merge 18 commits into
opencontainers:masterfrom
kolyshkin:cgroups-write-file
Closed

libct/cgroups/fscommon: add openat2() support#2598
kolyshkin wants to merge 18 commits into
opencontainers:masterfrom
kolyshkin:cgroups-write-file

Conversation

@kolyshkin
Copy link
Copy Markdown
Contributor

@kolyshkin kolyshkin commented Sep 23, 2020

This is a naive initial attempt to add openat2 support to libct/cgroups/fscommon, and switch libct/cgroups to use it.

This is now being split into a few more digestible PRs:

@kolyshkin
Copy link
Copy Markdown
Contributor Author

Funny, libcontainer/integration tests fail because now WriteFile do not use O_CREAT (which is obviously not needed for real cgroupfs). Need to think of a way to handle this.

Comment thread libcontainer/cgroups/fscommon/openat2.go
Comment thread libcontainer/cgroups/fscommon/openat2.go
@cyphar
Copy link
Copy Markdown
Member

cyphar commented Sep 23, 2020

Just wanted to mentioned that my long-term plan is to make it easier to use openat2 for this use-case from userspace with libpathrs -- securejoin.SecureJoin is actually quite insecure, and ideally we would use the more complicated userspace path lookup emulation. But I'm totally fine with doing some piece-meal things like this -- since we can easily port it to libpathrs in the future. The nice thing about libpathrs is that we don't need any of the fallback code to live in runc -- it's all handled transparently by libpathrs.

Comment thread libcontainer/cgroups/fscommon/openat2.go
@kolyshkin kolyshkin force-pushed the cgroups-write-file branch 2 times, most recently from 39a5c06 to 802f92e Compare September 25, 2020 03:09
@kolyshkin
Copy link
Copy Markdown
Contributor Author

kolyshkin commented Sep 25, 2020

Pushed a new patchset, slightly better organized/splitted, and with more users converted. Test failures are supposed to be fixed, too.

At this point I'm thinking about maybe changing the whole libct/cg to use per-cgroup fd (rather than fd opened to /sys/fs/cgroup) and openat() (when openat2 is not available) -- this should speed things up because the kernel will have to do less dentry lookups.

1. Don't reconstruct file name, use existing one since it's available.

2. Don't put file name when wrapping the error from ioutil.ReadFile
   since it already has it.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
2. Fix wrapping the error to not have the value as it's already
   part of the error returned from ParseUint.

3. Fix/improve doc.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. Use own ReadFile wrapper instead of ioutils.ReadFile.
   This makes it use the security measures of ReadFile.

2. Improve doc.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. Use GetCgroupParamString as the initial part of both functions are
   the same and we can reuse it. This also gives us whatever security
   measures GetCgroupParamString has (see previous commit).

2. Fix the error wrapping to not add the value, as it is already a part
   of the error returned by ParseUint.

3. Improve docstring.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Document ReadFile and WriteFile. Fix doc for ParseUint to be in
canonical form.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
fscommon.WriteFile is added specifically to work with cgroup files,
and the error it returns does not need to be wrapped.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
fscommon's ReadFile and WriteFile are tailored to cgroupfs,
so let's use them here.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
While at it, change some functions to not be methods of CpusetCgroup as
they don't use any members.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The code to find out memory cgroup root is not really needed,
as 99% of test envrionments will have it at /sys/fs/cgroup/memory.
If not, that means we're either on cgroupv2 or on some very custom
system, so just skip the test.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
Copy link
Copy Markdown
Contributor Author

At this point I'm thinking about maybe changing the whole libct/cg to use per-cgroup fd (rather than fd opened to /sys/fs/cgroup) and openat() (when openat2 is not available) -- this should speed things up because the kernel will have to do less dentry lookups.

This can easily be done on top of this one, so later.

For now, I am going to split this into smaller more digestible PRs.

Move the functionality of opening a cgroup file into a separate
function, OpenFile, which, similar to ReadFile and WriteFile,
use separate dir and file arguments.

Change ReadFile and WriteFile to rely on OpenFile, and use lower-level
read and write instead of ones from ioutil.

It changes the semantics of WriteFile a bit -- it no longer uses
O_CREAT flag. This is good for real cgroup as there is no need to try
creating the files in there, but can potentially break WriteFile users
-- previosly, EPERM error was returned for non-existing files, and
now it's ENOENT.

This also breaks the fs/fs2 unit tests since they write to pseudo-cgroup
files inside a test directory (not to a real cgroup fs), and now
fscommon.WriteFile do not create or truncate files, so we have to add a
variable that is set by the unit tests.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
...and drop os.O_CREATE|os.O_TRUNC as those are definitely not needed.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
In case openat2 is available, it will be used to guarantee
that we're not accessing anything other than cgroupfs[2] files.

In cases when openat2 is not available, or when cgroup has a
non-standard prefix (not "/sys/fs/cgroup", might theoretically happen
on some very old installs and/or very custom systems), fall back to
using securejoin + os.Open like we did before.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@AkihiroSuda
Copy link
Copy Markdown
Member

needs rebase

@kolyshkin
Copy link
Copy Markdown
Contributor Author

kolyshkin commented Oct 29, 2020

Closed as this is now splitted into

and the only last one is not yet merged

@kolyshkin kolyshkin closed this Oct 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants