Skip to content

Conversation

@blue42u
Copy link
Contributor

@blue42u blue42u commented Oct 29, 2025

Currently perf_event_open is only allowed if both CAP_SYS_ADMIN and CAP_PERFMON are enabled. CAP_SYS_ADMIN is a very overloaded capability and is best avoided. This PR enables perf_event_open if either (or both) capabilities are enabled. In particular, this enables a container to profile itself by only enabling CAP_PERFMON.

This change does not deny anything new, nor does it enable perf_event_open by default. In summary:

Capabilities perf_event_open return (before) perf_event_open return (after)
CAP_PERFMON + CAP_SYS_ADMIN No error No error
CAP_PERFMON EPERM No error
CAP_SYS_ADMIN ENOSYS No error
(none of the above) EPERM EPERM

Previously perf_event_open was only allowed if both CAP_SYS_ADMIN and
CAP_PERFMON were granted. CAP_SYS_ADMIN in particular is a very
overloaded capability and is best avoided. This commit enables
perf_event_open if either (or both) capabilities are set, in particular
this enables containers with only CAP_PERFMON to profile itself.

This change does not deny anything new, nor does it enable
perf_event_open by default.

Signed-off-by: Jonathon Anderson <anderson.jonathonm@gmail.com>
@github-actions github-actions bot added the common Related to "common" package label Oct 29, 2025
podmanbot pushed a commit to podmanbot/buildah that referenced this pull request Oct 29, 2025
@podmanbot
Copy link

✅ A new PR has been created in buildah to vendor these changes: containers/buildah#6461

@mtrmac
Copy link
Contributor

mtrmac commented Oct 30, 2025

@giuseppe PTAL. Historically I can see containers/common@daa81f1 was already supposed to enable this, which makes me worried about this PR.

@Luap99
Copy link
Member

Luap99 commented Oct 30, 2025

cc @martinetd

@giuseppe
Copy link
Member

@giuseppe PTAL. Historically I can see containers/common@daa81f1 was already supposed to enable this, which makes me worried about this PR.

this new PR enables it with CAP_PERFMON which seems to me like a good idea

Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@martinetd martinetd left a comment

Choose a reason for hiding this comment

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

Oww, I had managed to screw up that old commit in two different places, sorry for that mess.
This diff looks good to me, both the moving from eperm-if-not-sysadmin to allow-if-sysadmin and deny-if-not-sysadmin-or-perfmon instead of deny-if-not-sysadmin-or-bpf parts are sound.

Just in case I also had a new look at the other commits of that old PR ( https://github.com/containers/common/pull/2040/commits ) and the only similar commit was about bpf, which looks correct to me, so I'm a bit confused about how this got so bad... Sorry again, and thanks for the cc!

Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

LGTM

@Luap99 Luap99 merged commit 4584282 into containers:main Oct 30, 2025
18 checks passed
@mtrmac
Copy link
Contributor

mtrmac commented Oct 30, 2025

Thanks @blue42u and @martinetd !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

common Related to "common" package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants