Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Docker container ID parsing with cgroupfs driver #293

Merged
merged 6 commits into from
Jul 1, 2024

Conversation

vadorovsky
Copy link
Member

The format of cgroup path is different for systemd and cgroupfs drivers.

For systemd driver, it's

0::/system.slice/docker-d4ea646fc22c701dbb146e52db4e9125dcca2eebb2f5552f90fedbb28a0f0716.scope

For cgroupfs driver, it's

0::/docker/2cf2e0be458a80acb354c953f7bb03de5d2d277dfcb8ebaa6575f95668a0c15f

Handle both formats.

vadorovsky and others added 3 commits June 26, 2024 10:54
The format of cgroup path is different for systemd and cgroupfs
drivers.

For systemd driver, it's

```
0::/system.slice/docker-d4ea646fc22c701dbb146e52db4e9125dcca2eebb2f5552f90fedbb28a0f0716.scope
```

For cgroupfs driver, it's

```
0::/docker/2cf2e0be458a80acb354c953f7bb03de5d2d277dfcb8ebaa6575f95668a0c15f
```

Handle both formats.
Before this change, we were trying to handle offsets by doing pointer
arithmetics (like `str + offset` where `str` is `char*` and `offset`
an integer).

Verifier in kernels <6.0 was not happy with that and was complaining
about unchecked bounds.

Fix that by making `buffer_append_str` aware of offsets and doing all
the bound checks there.
* It's cleaner.
* Using `char` array was triggering verifier errors on kernel 6.0.
@banditopazzo
Copy link
Member

Seems good to me.

I would like to make one final consideration: looking at the history the commit fix: Handle offsets and their bounds in buffer_append_str didn't solve the verifier issue, but it was solved in the next commit fix: Use container_id_buffer also for parent_buf. so is it possible to revert to the previous function signature of the function buffer_append_str? I'm ok if it's the only way to solve the verifier issue, I'm only asking if we can keep the signature less weird

@vadorovsky
Copy link
Member Author

@banditopazzo There were two separate verifier issues which both of these commits were solving. 😅

fixed the verifier error on kernel 5.13
fixed an another verifier error on kernel 6.0

if you check out to a change before both of commits, integration tests fail for both 5.13 and 6.0. I initially didn't notice the latter, but that's the case.

If you check out to the first commit, the tests for 6.0 will fail.

And I think that both changes make sense (regardless of verifier errors) - the first one makes the offset checks more reliable in the whole helper, the second one uses your buffer struct consequently. :)

Copy link
Member

@banditopazzo banditopazzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

@vadorovsky vadorovsky merged commit 9f837a3 into main Jul 1, 2024
21 checks passed
@vadorovsky vadorovsky deleted the vadorovsky/docker-openrc branch July 1, 2024 09:54
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.

2 participants