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

config: drop /sys/dev/block from masked paths #2278

Merged

Conversation

giuseppe
Copy link
Member

there is no real advantage in masking this path, since this information is already available under /sys/devices. In fact, the files under /sys/dev/block are just symlinks.

[root@953139a2851f /]# ls -l /sys/dev/block/252:0 lrwxrwxrwx. 1 root root 0 Dec 17 12:47 /sys/dev/block/252:0 -> ../../devices/virtual/block/zram0

Fixes: #2277
Fixes: https://issues.redhat.com/browse/RHEL-3115

there is no real advantage in masking this path, since this
information is already available under /sys/devices.  In fact, the
files under /sys/dev/block are just symlinks.

[root@953139a2851f /]# ls -l /sys/dev/block/252\:0
lrwxrwxrwx. 1 root root 0 Dec 17 12:47 /sys/dev/block/252:0 -> ../../devices/virtual/block/zram0

Fixes: containers#2277
Fixes: https://issues.redhat.com/browse/RHEL-3115

Signed-off-by: Giuseppe Scrivano <[email protected]>
Copy link
Contributor

openshift-ci bot commented Dec 17, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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

Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

@rhatdan
Copy link
Member

rhatdan commented Dec 17, 2024

I thought this originally was something about lsblk causing issues where apps would think they had access to blk devices and then blew up with permission denied, and this caused tools to no longer think the devices was there. Although I might be misremembering.

Anyways I am fine with this change.

LGTM

@giuseppe
Copy link
Member Author

lsblk works fine after this change:

# podman run --rm -it registry.access.redhat.com/ubi9/ubi:latest lsblk
[no output]
# bin/podman run --rm -it registry.access.redhat.com/ubi9/ubi:latest lsblk
NAME        MAJ:MIN RM   SIZE RO TYPE MOUNTPOINTS
zram0       252:0    0     8G  0 disk [SWAP]
nvme0n1     259:0    0 476.9G  0 disk 
|-nvme0n1p1 259:1    0   600M  0 part 
|-nvme0n1p2 259:2    0     1G  0 part 
`-nvme0n1p3 259:3    0 475.4G  0 part 

@rhatdan
Copy link
Member

rhatdan commented Dec 17, 2024

Right that is my point, the complaint was that it reported blk devices which from the containers point of view, were not actionable, so the code within the container attempts to do something with them, which generates a permission denied. If as in the current behaviour, id sees no blk devices it continues on happily.

@grooverdan
Copy link

I totally support the unmasking because of problems exposed in containers/podman#13708.

Its origins where https://bugzilla.redhat.com/show_bug.cgi?id=1772993 related to exposing host storage information inside an unprivileged container. The masking of the symlinks in /sys/dev/block didn't actually achieve this (mentioned previous - #2277, except where lsblk was considered the definitive exposure) because the expose was still present in /sys/devices. With /sys being a ro,nodev mount, the expose isn't prevented.

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, does docker mask any of these paths?

@giuseppe
Copy link
Member Author

giuseppe commented Jan 8, 2025

Docker doesn't:

# docker --version
Docker version 27.4.1, build 1.fc42

# docker run --rm alpine ls /sys/dev/block
252:0
252:1
252:2

@Luap99
Copy link
Member

Luap99 commented Jan 8, 2025

/lgtm

@rhatdan
Copy link
Member

rhatdan commented Jan 8, 2025

/lgtm

@openshift-merge-bot openshift-merge-bot bot merged commit 7601d27 into containers:main Jan 8, 2025
13 of 15 checks passed
Luap99 added a commit to Luap99/buildah that referenced this pull request Jan 21, 2025
The c/common defaults were changed to no longer mask this path[1]. As
such we need to remove it from this test.

[1] containers/common#2278

Signed-off-by: Paul Holzinger <[email protected]>
Luap99 added a commit to Luap99/buildah that referenced this pull request Jan 22, 2025
The c/common defaults were changed to no longer mask this path[1]. As
such we need to remove it from this test.

[1] containers/common#2278

Signed-off-by: Paul Holzinger <[email protected]>
Luap99 added a commit to Luap99/buildah that referenced this pull request Jan 23, 2025
The c/common defaults were changed to no longer mask this path[1]. As
such we need to remove it from this test.

[1] containers/common#2278

Signed-off-by: Paul Holzinger <[email protected]>
Luap99 added a commit to Luap99/buildah that referenced this pull request Jan 24, 2025
The c/common defaults were changed to no longer mask this path[1]. As
such we need to remove it from this test.

[1] containers/common#2278

Signed-off-by: Paul Holzinger <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

remove mask on /sys/dev/block
4 participants