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

Update destroy.sh - refuse to destroy jail with mounted filesystem #749

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

tschettervictor
Copy link
Collaborator

@tschettervictor tschettervictor commented Nov 24, 2024

This PR fixes an issue where files could be deleted when bastille attempts to destroy a jail that still has mounted filesystems when using ZFS. It will throw an error and exit if it detects a filesystem is still mounted inside the jail.

To test

# bastille create temp 14.0-RELEASE 192.168.1.10 lo1
...
# mkdir test
# cp /usr/bin/less test/
# mkdir /usr/local/bastille/jails/temp/root/test
# bastille mount temp $(realpath test) test
[temp]:
Added: /root/admin/bastille/test /usr/local/bastille/jails/temp/root/test nullfs ro 0 0
# /usr/local/bastille/jails/temp/root/test/less -f /dev/stdin &
# bastille destroy force temp
rdr-anchor not found in pf.conf
[temp]:
temp: removed
umount: unmount of /usr/local/bastille/jails/temp/root/test failed: Device busy
jail: temp: /sbin/umount -t nullfs /usr/local/bastille/jails/temp/root/test: failed

Deleting Jail: temp.
Jail has mounted filesystems:
/usr/local/bastille/jails/temp/root/test

With the PR in place, bastille will error upon finding existing mounts.

@tschettervictor
Copy link
Collaborator Author

This won't work.
Since jails are always mounted, there will always be a mount point present, and jails can never be destroyed with this.

This will allow the jail root to be mounted when destroying a jail, but if anything under 'root' is still mounted, it will exit.
@tschettervictor
Copy link
Collaborator Author

Tested and working as expected now.

@bmac2
Copy link
Collaborator

bmac2 commented Dec 8, 2024

need testing on this one to merge
@yaazkal @cedwards

@tschettervictor
Copy link
Collaborator Author

tschettervictor commented Dec 8, 2024

The concept is this.

Jails have their mount points defined in the fstab file, but are also mounted and can be found using the mount command. Any mount points that follow "/root" need to be unmounted before destroying the jail.

But there is also the root dataset of the jail itself that does need to be destroyed, that's why we grep for "${bastille_jail_base}/root/" with the trailing slash in our mount points. If we would not include the trailing slash, we would end up also find the root dataset of the jail, and could never delete a jail

@bmac2
Copy link
Collaborator

bmac2 commented Dec 26, 2024

give me an example of how you are mounting filesystems inside your jail. I want to make sure I am doing it the way you are for testing.

@tschettervictor
Copy link
Collaborator Author

#662

Basically, sometimes a mount point will fail to unmount, and the destroy command will then delete content inside the mounted directory.

This PR will grep for any mount points below root/ of the jail, which should normally already be unmounted when the jail is stopped.

Easiest way to test is to mount a directory into the jail, copy a binary into it, and run the binary from the host.

Then stop the jail and try to destroy it.

With this PR it should fail because it detects an existing mount point.

Without the PR it destroys all the data inside the mount point.

@tschettervictor
Copy link
Collaborator Author

I could also add something I've added to my own fork, which will attempt to force unmount any mounts before it does the destroy.

It's basically an additional safety feature.

@bmac2
Copy link
Collaborator

bmac2 commented Jan 1, 2025

tested and as long as tghere is a running process in a mount, it will not destroy the jail, but it will stop it and throw a message that there is a mounted filesystem.

@yaazkal test this one also yourself. This is the 3rd one ready for you.

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.

2 participants