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

init.d/root: add support for 'shared' fstab option on / #526

Merged
merged 1 commit into from
Jul 28, 2024

Conversation

martinetd
Copy link
Contributor

containers on linux might require filesystems to be mounted with
different propagation than the kernel default of 'private':
by setting 'shared' in fstab for / options, one can now make the
fs hierarchy shared.

Note we use 'rshared' to make other existing mounts shared as well
because the setting is contagious and it seemed more logical to
behave as if the setting was set on / immediately (and thus inherited
by other mounts)

This fixes #525.

@hosaka
Copy link

hosaka commented Nov 10, 2022

This should fix the warning message for rootless podman/buildah: containers/buildah#3726

WARN[0000] "/" is not a shared mount, this could cause issues or missing mounts with rootless containers

@martinetd
Copy link
Contributor Author

This should fix the warning message for rootless podman/buildah: containers/buildah#3726

Yes, that's the main reason I want this :)

(well, that and shared mounts can actually be useful for containers -- with volume mounted as shared it's possible to have an administration container that can issue mount commands for slave, less trusted containers)

@nekopsykose
Copy link
Contributor

was there anything more to do here to allow it to be merged? looks pretty cut and dry to me

@martinetd
Copy link
Contributor Author

For reference, I've been using something equivalent to this for a bit over a year now without any issue. I'll be more than happy to remove my kludge if/when this is merged :)

@daiaji
Copy link

daiaji commented Jun 25, 2023

Is that the problem? Strange mistake.

@vampywiz17
Copy link

I hope it will merged soon :)

Now i use /var/lib/docker folder all of shared container mounts. This folder shared by default, if docker installed :D

@vapier
Copy link
Member

vapier commented Sep 6, 2023

the mount(8) man page says it recognizes the various types in fstab (private, slave, shared, unbindable, rprivate, rslave, rshared, runbindable). we shouldn't treat "shared" as "rshared". if you want --make-rshared, then you need to use "rshared" in fstab, not "shared".

the man page also says that since util-linux 2.23, the fstab propagation flags are respected by mount. i'm guessing this doesn't work for / because the kernel mounted it for us. does mount -o remount / not apply the share settings ? i don't have a system setup atm to easily test, or spelunk code.

@martinetd
Copy link
Contributor Author

martinetd commented Sep 6, 2023

The rationale for using rshared when shared is set on /, is that it is supposed to be inherited -- but since we're setting rootflags late (after other mount points have been made) then we need rshared to behave as if it had been shared from the start. (otherwise you end up with something weird where part of the tree is shared but it's not really clear to users what)

Happy to update the PR to change that though, it's a detail really.

As for mount -o remount / iirc it didn't affect the sharing previously, will retry tomorrow

@thetredev
Copy link

Just tested the change in an Alpine 3.18 VM (amd64, system disk). Works fine:

$ cat /etc/fstab
UUID=....	/	ext4	rw,relatime,shared 0 1
....

$ podman run --rm hello-world

Hello from Docker!
....

@btreecat
Copy link

btreecat commented Apr 3, 2024

@vapier If the PR was changed to use the rshared flag in /etc/fstab so that it was matching the call to mount --make-rshared / would that be satisfactory for merging? Is there additional work you would like to see done first before the PR from @martinetd is accepted?

I would love to see this fixed upstream for everyone! Thank you.

@martinetd
Copy link
Contributor Author

Ah, I realized I forgot to reply to a point:

As for mount -o remount / iirc it didn't affect the sharing previously, will retry tomorrow

This doesn't just have no effect, mount -o doesn't seem to allow passing shared or other similar options:

alpine:~# mount -o remount,shared / 
mount: mounting /dev/vda3 on / failed: Invalid argument

re: fstab flag, as said before I still think it makes more sense as it is, but happy to do the updating if that's what you want.

@Raboo
Copy link

Raboo commented May 6, 2024

Perhaps there should be a shared and a rshared option? So people explicitly knows that rshared is recursive and shared is not.

@martinetd
Copy link
Contributor Author

Perhaps there should be a shared and a rshared option? So people explicitly knows that rshared is recursive and shared is not.

This was brought up before so I'll just quote my reply:

The rationale for using rshared when shared is set on /, is that it is supposed to be inherited -- but since we're setting rootflags late (after other mount points have been made) then we need rshared to behave as if it had been shared from the start. (otherwise you end up with something weird where part of the tree is shared but it's not really clear to users what)

To rephrase myself, the shared setting will be inherited by future mounts, so depending on the timing of the "make root shared" you'll get something implementation specific with a simple --make-shared, which I find it hard to believe anyone would want.

It's an easy change so given the above, if someone still think that makes sense I'll be happy to update this PR to at least rename shared to rshared to make it more obvious that it's what we're doing; please re-ask if that's the case.

@Raboo
Copy link

Raboo commented May 6, 2024

Oh sorry, missed that. I actually don't care that much about the details, I just want it implemented.

@btreecat
Copy link

@williamh or @vapier Is there anything outstanding preventing this PR from being accepted?

@btreecat
Copy link

Just hoping we don't let this die on the vine. @williamh @vapier

@btreecat
Copy link

@navi-desu Could you possibly provide some feedback here as to how we can git this PR merged?

@navi-desu
Copy link
Member

navi-desu commented Jul 27, 2024

i'm looking over shared mounts, since we run this at boot and remount doesn't seem to apply the shared status, --make-rshared would make sense, otherwise any mounts existing before the script is called would not have the propagation applied, so the question is, can a mount exist before the script is called?

tho, both shared and rshared are mount options, wouldn't it make sense for us to match on both?

also, we call fstabinfo -o / right above this one, setting a variable to it might be better? avoid querying twice (as i assume fstab won't change between calls)

@martinetd
Copy link
Contributor Author

martinetd commented Jul 27, 2024

can a mount exist before the script is called?

yes, mounts like /dev are made by the kernel before this if no initrd, and if there is an initrd anything can happen.

I don't think shared without r makes sense here precisely because we don't know the state the fs will be in here (need to check script dependencies with e.g. sysfs cgroup etc services that all happen at boot and have no reason to have a dependency on this), but if that helps merging I don't really care so happy to handle both, it's just more code that probably won't be used much

fstabinfo -o /

happy to assign it to a variable, yes; will update the patch next week

@navi-desu
Copy link
Member

I don't think shared without r makes sense here precisely because we don't know the state the fs will be in here (need to check script dependencies with e.g. sysfs cgroup etc services that all happen at boot and have no reason to have a dependency on this), but if that helps merging I don't really care so happy to handle both, it's just more code that probably won't be used much

got it, and i agree then, since anything can happen, it's more consistent to reapply it.

btw by handling both i just mean *,shared,*|*,rshared,* since reading the man page, both seem valid for fstab

@martinetd
Copy link
Contributor Author

btw by handling both i just mean ,shared,|,rshared, since reading the man page, both seem valid for fstab

ah, hadn't understood it that way -- this is probably fine/welcome, I've fixed it up along with the variable and a trivial rebase (can't really re-test until next week but this is probably fine; will repost next week after checking it still works as of master)

@navi-desu
Copy link
Member

nit: root_info is a better name than slash_info

in anyway, we can merge this as soon as it's tested again. it seems fine to me but i don't know how to properly reproduce the scenario this fixes, i can test as well if there's any steps to reproduce

@FintasticMan
Copy link

I can confirm that this change does fix the issues linked. I tested on Alpine Linux 3.20.

containers on linux might require filesystems to be mounted with
different propagation than the kernel default of 'private':
by setting 'shared' in fstab for / options, one can now make the
fs hierarchy shared.

Note we use 'rshared' to make other existing mounts shared as well
because the setting is contagious and it seemed more logical to
behave as if the setting was set on / immediately (and thus inherited
by other mounts)

This fixes OpenRC#525.
@martinetd
Copy link
Contributor Author

nit: root_info is a better name than slash_info

renamed. It's actually mount options so root_opts.

i can test as well if there's any steps to reproduce

findmnt -o target,propagation should list mount points as shared if the option is set; this affects containers e.g. mount namespaces so if you unshare -m --propagation unchanged and change something (new mount/umount) it'll be visible out of the unshare process if and only if the parent mount is shared (note unshare and container technos will all make mounts private by default when creating a mount namespace; sharing needs explicit sharing on the root namespace and when creating the child namespace which is why this option is needed)

@navi-desu navi-desu merged commit 6bfef87 into OpenRC:master Jul 28, 2024
5 checks passed
@navi-desu
Copy link
Member

thanks!

@jaitaiwan
Copy link

Any idea when this will be in a tagged release? Doesn't look like it's in 0.54.2 afaict.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add option to make mountpoints shared