Skip to content

idmap: force PRIVATE propagation #2269

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
merged 1 commit into from
Mar 5, 2025
Merged

idmap: force PRIVATE propagation #2269

merged 1 commit into from
Mar 5, 2025

Conversation

giuseppe
Copy link
Member

@giuseppe giuseppe commented Mar 5, 2025

do not leak idmapped mounts to other namespaces, since they are meant to be used privately by overlay.

This is already done with the default configuration, since we have a private mount on top of the graphdriver directory, but it is not the case when skip_home_mount is used.

Closes: https://issues.redhat.com/browse/OCPBUGS-49927

@openshift-ci openshift-ci bot added the approved label Mar 5, 2025
@giuseppe
idmap: force PRIVATE propagation
1fe930f 
do not leak idmapped mounts to other namespaces, since they are meant
to be used privately by overlay.

This is already done with the default configuration, since we have a
private mount on top of the graphdriver directory, but it is not the
case when `skip_home_mount` is used.

Closes: https://issues.redhat.com/browse/OCPBUGS-49927

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
@haircommander
Copy link
Contributor

tested by vendoring into CRI-O and running on openshift and it works for me!
LGTM

flouthoc
flouthoc approved these changes Mar 5, 2025
View reviewed changes
Copy link
Collaborator

@flouthoc flouthoc left a comment

Choose a reason for hiding this comment

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

LGTM

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Mar 5, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc, giuseppe

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

TomSweeneyRedHat
TomSweeneyRedHat reviewed Mar 5, 2025
View reviewed changes
Copy link
Member

@TomSweeneyRedHat TomSweeneyRedHat left a comment

Choose a reason for hiding this comment

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

LGTM

@TomSweeneyRedHat
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Mar 5, 2025
@openshift-merge-bot openshift-merge-bot bot merged commit 465bc75 into containers:main Mar 5, 2025
20 checks passed
@giuseppe
Copy link
Member Author

giuseppe commented Mar 5, 2025

@haircommander do we need to backport it?

@haircommander
Copy link
Contributor

Yes indeed! To 9.6 branches

@giuseppe
Copy link
Member Author

giuseppe commented Mar 6, 2025

/cherry-pick release-1.57

@openshift-cherrypick-robot
Copy link

@giuseppe: new pull request created: #2272

In response to this:

/cherry-pick release-1.57

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Mar 6, 2025
Merged
@bitoku
Copy link

bitoku commented Mar 6, 2025

it is not the case when skip_home_mount is used.

@giuseppe Could you elaborate this? I'd like to know whether other older OCP versions could be affected and whether we have to backport this to older cri-o versions.

@giuseppe
Copy link
Member Author

giuseppe commented Mar 6, 2025

the issue happens only when skip_home_mount is set in the storage.conf file.

If you set it to false, then you don't hit the problem.

@haircommander
Copy link
Contributor

It's not an issue before we run on 9.6, which starts in 4.19

@bitoku
Copy link

bitoku commented Mar 6, 2025

@haircommander Yeah I know. I just wonder if it happens when we set skip_mount_home=true in other versions.

@haircommander
Copy link
Contributor

It's been set that way for a while

@haircommander
Copy link
Contributor

It's some interaction with idmapped overlay which only landed in 9.6 and skip_mount_home

TomSweeneyRedHat added a commit to TomSweeneyRedHat/image that referenced this pull request Mar 7, 2025
@TomSweeneyRedHat
[release-5.34] Bump c/storage to v1.57.2
71bd224 
Bump c/storage to v1.57.2 to force idm map private
propacation per containers/storage#2269

This is targeted for RHEL 9.6/10.0 ZeroDay.

Signed-off-by: tomsweeneyredhat <tsweeney@redhat.com>
@TomSweeneyRedHat TomSweeneyRedHat mentioned this pull request Mar 7, 2025
Merged
@Luap99
Copy link
Member

Luap99 commented Mar 10, 2025

This breaks podman tests

not ok 67 |030| podman run - idmapped mounts in 1292ms
         # tags: ci:parallel
         # (from function `bail-now' in file test/system/[helpers.bash, line 187](https://github.com/containers/podman/blob/bc86898fc200973b2b88838e8512238329eea772/test/system/helpers.bash#L187),
         #  from function `die' in file test/system/[helpers.bash, line 970](https://github.com/containers/podman/blob/bc86898fc200973b2b88838e8512238329eea772/test/system/helpers.bash#L970),
         #  from function `run_podman' in file test/system/[helpers.bash, line 572](https://github.com/containers/podman/blob/bc86898fc200973b2b88838e8512238329eea772/test/system/helpers.bash#L572),
         #  in test file test/system/[030-run.bats, line 1423](https://github.com/containers/podman/blob/bc86898fc200973b2b88838e8512238329eea772/test/system/030-run.bats#L1423))
         #   `run_podman run --security-opt label=disable --rm --uidmap=0:1000:10000 --rootfs $romount:idmap stat -c %u:%g /bin' failed
         #
<+     > # # podman image mount quay.io/libpod/testimage:20241011
<+089ms> # /var/lib/containers/storage/overlay/5fb2677d7366b1f97f4a4d2851f47b11938cf2d6310523a61f43ab71b2b14e13/merged
         #
<+131ms> # # podman image unmount quay.io/libpod/testimage:20241011
<+082ms> # b82e560ed57b77a897379e160371adcf1b000ca885e69c62cbec674777a83850
         #
<+025ms> # # podman run --security-opt label=disable --rm --uidmap=0:1000:10000 --rootfs /tmp/CI_p9hx/podman_bats.SHOJ8z/rootfs:idmap true
         #
<+479ms> # # podman run --security-opt label=disable --rm --uidmap=0:1000:10000 --rootfs /tmp/CI_p9hx/podman_bats.SHOJ8z/rootfs:idmap stat -c %u:%g /bin
<+117ms> # Error: failed to create idmapped mount: mount_setattr /tmp/CI_p9hx/podman_bats.SHOJ8z/rootfs: operation not permitted
<+009ms> # [ rc=126 (** EXPECTED 0 **) ]
         # #/vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
         # #| FAIL: exit code is 126; expected 0
         # #\^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

https://api.cirrus-ci.com/v1/artifact/task/5918086693388288/html/sys-podman-fedora-41-root-host-sqlite.log.html

I reproduced locally and if I revert this part it works again so I am confident that this is caused by this change.

(before backporting fixes it would be nice to run them through the full podman CI first in the future so we know they don't cause regressions)

@Luap99 Luap99 mentioned this pull request Mar 10, 2025
Merged
@TomSweeneyRedHat
Copy link
Member

Just for historical traceability, the fix in c/storage: #2269 was put into RHEL 9.6 ZeroDay with https://issues.redhat.com/browse/RHEL-82509 and RHEL 10.0 ZeroDay with https://issues.redhat.com/browse/RHEL-82511 

@renovate renovate bot mentioned this pull request Jul 5, 2025
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TomSweeneyRedHat TomSweeneyRedHat TomSweeneyRedHat left review comments

@flouthoc flouthoc flouthoc approved these changes

Assignees

@TomSweeneyRedHat TomSweeneyRedHat

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@giuseppe @haircommander @TomSweeneyRedHat @openshift-cherrypick-robot @bitoku @Luap99 @flouthoc