Skip to content

Fix cgroupv2 bpf-devices issues #10757

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.

Sign up for GitHub

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

Jump to bottom
Merged
poettering merged 2 commits into from
Nov 13, 2018
Merged

Fix cgroupv2 bpf-devices issues #10757

poettering merged 2 commits into from
Nov 13, 2018

Conversation

@phrdina
Copy link
Contributor

@phrdina phrdina commented Nov 13, 2018

This pull request solves two issues with cgroupv2 bpf-devices:

  1. detection code is broken and it will never detect whether bpf-devices is supported or not
  2. there is a security issue in the way how we update BPF programs while adding/removing allowed devices

@phrdina
bpf-devices: fix cgroup v2 devices detection
0b82cd2 
If cgroup v2 bpf devices is supported we need to return 1, not -1.

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
@poettering
Copy link
Member

poettering commented Nov 13, 2018

/cc @rgushchin

@poettering
Copy link
Member

poettering commented Nov 13, 2018

@Antique travis CI detected that the flags variable is now unused. It's right, that variable should be removed. Could you make such a change, please?

lgtm otherwise

@poettering poettering added cgroups pid1 reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Nov 13, 2018
@phrdina
bpf-devices: fix order of removing and adding BPF programs
2af3eed 
The current code has multiple issues and it should never be done like
that.  If someone updates list of allowed devices we should attach new
program before we remove the old one for two reasons:

1. It takes some time to attach new program so there is a period of time
when all devices are allowed.

2. BPF programs have limit for number of instructions (4096) and if user
adds a lot of devices we might hit the instruction limit and the new
program will not be accepted which will result in allow all devices
because the old program was already removed.

In order to attach the new program before we remove the old one we need
to use BPF_F_ALLOW_MULTI flag every time.

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
@phrdina
Copy link
Contributor Author

phrdina commented Nov 13, 2018

@Antique travis CI detected that the flags variable is now unused. It's right, that variable should be removed. Could you make such a change, please?

lgtm otherwise

Thanks, fixed now. I've completely missed that one.

@poettering poettering added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Nov 13, 2018
@rgushchin
Copy link
Contributor

rgushchin commented Nov 13, 2018

@Antique , thanks for fixing this!
Re second patch: then you might want to fix bpf firewall as well (see bpf_firewall_install(), where from I mostly stole the code with this approach).
We don't necessary have to attach second program and use BPF_F_ALLOW_MULTI, we can just attach a new program without detaching the previous one, should work too; and the switch will be atomic, so might be less confusing for the userspace.

@poettering poettering merged commit 862d9d9 into systemd:master Nov 13, 2018
facebook-github-bot pushed a commit to facebookarchive/rpm-backports that referenced this pull request Dec 11, 2018
@davide125 @facebook-github-bot
rpms: backport some more cgroup2 fixes for systemd
d545d6e 
Summary:
Rebase two fixes onto the version merged upstream:
systemd/systemd#10507
systemd/systemd#10567
and backport a few more:
systemd/systemd#10411
systemd/systemd#10493
systemd/systemd#10757
systemd/systemd#10876

These are almost all cgroup2 related.

Reviewed By: cdown

Differential Revision: D13351498

fbshipit-source-id: 87c8428d48dbb0eb2ae7d34f7381fff88f83872f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

cgroups good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed pid1

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@phrdina @poettering @rgushchin