--read-write-tmpfs=false should set /dev/* tmpfs to readonly#12954
--read-write-tmpfs=false should set /dev/* tmpfs to readonly#12954rhatdan wants to merge 1 commit intocontainers:mainfrom
Conversation
|
@giuseppe When I do this I attempt to make /dev, /dev/tty, /dev/shm, /dev/mqueue as read-only but crun blows up with ./bin/podman run -ti --read-only --read-only-tmpfs=false alpine sh |
|
I've fixed an issue in crun so it works when I've tried with runc and it fails as well, so I think it should be optional to set |
|
this seems to help with your PR: containers/crun#859 |
|
@kolyshkin WDYT? |
|
I think if we leave any tmpfs writable, we don't solve the problem. |
|
Fixes: #12937 |
|
As @giuseppe mentioned earlier, runc can't work with ro /dev, but this fix is easy opencontainers/runc#3345 (and very similar to containers/crun#857). One other thing, @rhatdan, I also find it very confusing how |
|
@kolyshkin I am not crazy about adding another function, Since I believe the original goal of --read-only-tmpfs (A badly named option, which we should probably change) was to make the entire container read-only. (At least the way I think about it). |
|
Changed the option to read-write-tmpfs. |
|
This PR is waiting for an updated released version of runc, which will allow it to work with /dev set to read/only mode. |
Depending on how soon you need it. We can certainly release runc 1.1.1, either now (= soon) with this change alone, or a bit later, waiting for a few more fixes to pile up. |
Backport PR: opencontainers/runc#3355 |
|
Well since this is a post 4.0 feature, it is not needed immediately, but soon. |
ecc6563 to
9329fca
Compare
23268a9 to
8f10141
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhatdan The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Friendly ping. @rhatdan, are you on it? |
|
I look at it once in a while and restart it hoping for a miracle. But I have no idea why this does not work on Ubuntu. |
|
Hrmmm, you're using recent VM images too 😕 I see this, which is probably telling:
Have you tried running any of those tests or failing commands manually under |
|
Nope, Have thought about it, but never have time. Others can take if over and drive to conclusion... |
This was fixed in opencontainers/runc#3345 which was backported to runc 1.1 in opencontainers/runc#3355, and the fix made its way to runc 1.1.1. What runc version is used in the test? |
|
@cevich would know for sure, but I think it is using the current released versions of runc in Ubuntu. |
|
One of the failing Ubuntu 22.04 tasks reports: Checking if there's an update available...There are 28 package updates available, none of them are runc. We have an option to bring in a custom-built version, but generally the distro-provided packages would be preferred. Is there a way we can tell if cc: @lsm5 FYI for now. |
68c331b to
28e0ef1
Compare
eac20a3 to
1f312b6
Compare
|
This is my fault, #16240. Can you run |
edsantiago
left a comment
There was a problem hiding this comment.
Lots of little nits, sorry for not catching them earlier.
There was a problem hiding this comment.
When false, Podman the entire container
Does not parse. I think Podman is the part that needs to go, but I'm not sure what the intention was.
Did you deliberately un-underscore the directory names? Our guidelines say:
Strings of characters or numbers can be highlighted with
backticks. Paths of any kind must be highlighted.
(which suggests that backticks are preferred, underscores are ok, but leaving them unadorned is not).
test/system/030-run.bats
Outdated
There was a problem hiding this comment.
Inconsistent indentation: some of these are tabs, some are eight spaces.
test/system/030-run.bats
Outdated
There was a problem hiding this comment.
I have a find-obsolete-skips script that I run periodically. Could you add "FIXME:" to this and the above skip/Skips ? That will bring these to my attention when I run the script next time.
test/system/030-run.bats
Outdated
There was a problem hiding this comment.
leftover from copy/paste (hence misleading, because it's never used). Please remove.
cmd/podman/containers/create.go
Outdated
There was a problem hiding this comment.
I think this would be clearer as "...is only valid if", or "...is only meaningful when..."
There was a problem hiding this comment.
(commenting on code that github does not show by default, so please click the uparrow-on-bar to see lines 473-475). Those lines read:
The best way to handle this is to mount tmpfs directories on /run and /tmp.
This looks like a holdover from a previous age: the implication is that /run and /tmp are also read-only. I haven't taken the time to go through history, but is it possible that the read-only-tmpfs option (the one that is being renamed here) was added after this was written, and that this used to be true but no longer is? Can you re-review lines 473-475 and see if they should be rewritten, maybe something like
Read-only containers still mount
/run,/tmp,/var/tmp,/dev, and/dev/shmas read-write tmpfs. Use--read-write-tmpfs=falseto protect eventmpfsdirectories, and--tmpfs=PATHto explicitly limit the read-write tmpfs filesystems.
(Idea only. That reads horribly. My documentation-writing brain is not fully engaged yet).
1551a20 to
970f3b8
Compare
There was a problem hiding this comment.
I find this hard to parse, and am really confused by the discrepancies between the first list of "these are tmpfs" and the second. I'm just going to hope that @TomSweeneyRedHat will offer his always-helpful input. Otherwise my (much less refined) thinking goes something like:
Only valid in combination with **--read-only**. Default: **true**.
Normally a container run with **read-only** will mount the following `tmpfs` filesystems read/write:
[ complete enumeration of those filesystems, possibly with a reference to where in the code that list is found ]
With **--read-write-tmpfs=false**, even those filesystems are mounted read-only.Change the --read-only-tmpfs option to --read-write-tmpfs since this makes sense and the old name was doing the exact opposite. Fixes: containers#12937 Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
|
Replaced by #16545 |
Change the --read-only-tmpfs option to --read-write-tmpfs since
this makes sense and the old name was doing the exact opposite.
Fixes: #12937
Signed-off-by: Daniel J Walsh dwalsh@redhat.com