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.

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

storage: Don't allow mounting over other mounted filesystems #19388

Merged
merged 3 commits into from
Oct 5, 2023
Merged

storage: Don't allow mounting over other mounted filesystems #19388

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9866efd
storage: Don't allow mounting over other mounted filesystems
mvollmer Oct 4, 2023
a0e1984
test: Cleanup volume group in one non-destructive test
mvollmer Sep 28, 2023
0407a6e
storage: Validate mount point even with "Format only"
mvollmer Sep 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 35 additions & 12 deletions pkg/storaged/dialog.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1140,10 +1140,18 @@ const UsersPopover = ({ users }) => {
</Popover>);
};

export const TeardownMessage = (usage) => {
function is_expected_unmount(usage, expect_single_unmount) {
return (expect_single_unmount && usage.length == 1 &&
usage[0].usage == "mounted" && usage[0].location == expect_single_unmount);
}

export const TeardownMessage = (usage, expect_single_unmount) => {
if (usage.length == 0)
return null;

if (is_expected_unmount(usage, expect_single_unmount))
return <StopProcessesMessage mount_point={expect_single_unmount} users={usage[0].users} />;

const rows = [];
usage.forEach((use, index) => {
if (use.block) {
Expand Down Expand Up @@ -1178,7 +1186,25 @@ export const TeardownMessage = (usage) => {
</div>);
};

export function init_active_usage_processes(client, usage) {
export function teardown_danger_message(usage, expect_single_unmount) {
if (is_expected_unmount(usage, expect_single_unmount))
return stop_processes_danger_message(usage[0].users);

const usage_with_users = usage.filter(u => u.users);
const n_processes = usage_with_users.reduce((sum, u) => sum + u.users.filter(u => u.pid).length, 0);
const n_services = usage_with_users.reduce((sum, u) => sum + u.users.filter(u => u.unit).length, 0);
if (n_processes > 0 && n_services > 0) {
return _("Related processes and services will be forcefully stopped.");
} else if (n_processes > 0) {
return _("Related processes will be forcefully stopped.");
} else if (n_services > 0) {
return _("Related services will be forcefully stopped.");
Comment on lines +1198 to +1201
Copy link
Contributor

Choose a reason for hiding this comment

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

These 4 added lines are not executed by any test. Details

} else {
return null;
}
}

export function init_active_usage_processes(client, usage, expect_single_unmount) {
return {
title: _("Checking related processes"),
func: dlg => {
Expand All @@ -1191,22 +1217,19 @@ export function init_active_usage_processes(client, usage) {
} else
return Promise.resolve();
}).then(() => {
dlg.set_attribute("Teardown", TeardownMessage(usage));
const usage_with_users = usage.filter(u => u.users);
const n_processes = usage_with_users.reduce((sum, u) => sum + u.users.filter(u => u.pid).length, 0);
const n_services = usage_with_users.reduce((sum, u) => sum + u.users.filter(u => u.unit).length, 0);
if (n_processes > 0 && n_services > 0)
dlg.add_danger(_("Related processes and services will be forcefully stopped."));
else if (n_processes > 0)
dlg.add_danger(_("Related processes will be forcefully stopped."));
else if (n_services > 0)
dlg.add_danger(_("Related services will be forcefully stopped."));
dlg.set_attribute("Teardown", TeardownMessage(usage, expect_single_unmount));
const msg = teardown_danger_message(usage, expect_single_unmount);
if (msg)
dlg.add_danger(msg);
});
}
};
}

export const StopProcessesMessage = ({ mount_point, users }) => {
if (!users || users.length == 0)
return null;

const process_rows = users.filter(u => u.pid).map(u => {
return {
columns: [
Expand Down
3 changes: 1 addition & 2 deletions pkg/storaged/format-dialog.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,8 +274,7 @@ function format_dialog_internal(client, path, start, size, enable_dos_extended,
visible: is_filesystem,
value: old_dir || "",
validate: (val, values, variant) => {
if (variant !== "nomount")
return is_valid_mount_point(client, block, val);
return is_valid_mount_point(client, block, val, variant == "nomount");
}
}),
SelectOne("type", _("Type"),
Expand Down
62 changes: 23 additions & 39 deletions pkg/storaged/fsys-tab.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import { parse_options, unparse_options, extract_option, set_crypto_options, set

import {
dialog_open, TextInput, PassInput, CheckBoxes, SelectOne,
StopProcessesMessage, stop_processes_danger_message
TeardownMessage, init_active_usage_processes
} from "./dialog.jsx";
import { StorageButton, StorageLink } from "./storage-controls.jsx";
import { initial_tab_options, mount_explanation } from "./format-dialog.jsx";
Expand Down Expand Up @@ -96,14 +96,27 @@ function nice_block_name(block) {
return utils.block_name(client.blocks[block.CryptoBackingDevice] || block);
}

export function is_valid_mount_point(client, block, val) {
if (val === "")
return _("Mount point cannot be empty");
export function is_valid_mount_point(client, block, val, format_only, for_fstab) {
if (val === "") {
if (!format_only || for_fstab)
return _("Mount point cannot be empty");
return null;
}

const other_blocks = find_blocks_for_mount_point(client, val, block);
if (other_blocks.length > 0)
return cockpit.format(_("Mount point is already used for $0"),
other_blocks.map(nice_block_name).join(", "));

if (!format_only) {
const children = utils.find_children_for_mount_point(client, val, block);
if (Object.keys(children).length > 0)
return <>
{_("Filesystems are already mounted below this mountpoint.")}
{Object.keys(children).map(m => <div key={m}>{cockpit.format("• $0 on $1", nice_block_name(children[m]), m)}</div>)}
{_("Please unmount them first.")}
</>;
}
}

export function get_cryptobacking_noauto(client, block) {
Expand Down Expand Up @@ -179,7 +192,6 @@ export function mounting_dialog(client, block, mode, forced_options) {
const extra_options = unparse_options(split_options);

const is_filesystem_mounted = is_mounted(client, block);
let mount_point_users = null;

function maybe_update_config(new_dir, new_opts, passphrase, passphrase_type) {
let new_config = null;
Expand Down Expand Up @@ -217,13 +229,6 @@ export function mounting_dialog(client, block, mode, forced_options) {
}
}

function maybe_unmount() {
if (block_fsys && block_fsys.MountPoints.indexOf(utils.encode_filename(old_dir)) >= 0)
return client.unmount_at(old_dir, mount_point_users);
else
return Promise.resolve();
}

function get_block_fsys() {
if (block_fsys)
return Promise.resolve(block_fsys);
Expand Down Expand Up @@ -293,7 +298,7 @@ export function mounting_dialog(client, block, mode, forced_options) {
// backs.

return (utils.reload_systemd()
.then(maybe_unmount)
.then(() => utils.teardown_active_usage(client, usage))
.then(maybe_unlock)
.then(() => {
if (!old_config && new_config)
Expand Down Expand Up @@ -327,7 +332,7 @@ export function mounting_dialog(client, block, mode, forced_options) {
TextInput("mount_point", _("Mount point"),
{
value: old_dir,
validate: val => is_valid_mount_point(client, block, val)
validate: val => is_valid_mount_point(client, block, val, mode == "update" && !is_filesystem_mounted, true)
}),
CheckBoxes("mount_options", _("Mount options"),
{
Expand Down Expand Up @@ -375,17 +380,6 @@ export function mounting_dialog(client, block, mode, forced_options) {
]);
}

let teardown = null;
if (!is_filesystem_mounted && block_fsys && block_fsys.MountPoints.length > 0)
teardown = (
<>
{teardown}
<div className="modal-footer-teardown">
<p>{cockpit.format(_("The filesystem is already mounted at $0. Proceeding will unmount it."),
utils.decode_filename(block_fsys.MountPoints[0]))}</p>
</div>
</>);

const mode_title = {
mount: _("Mount filesystem"),
unmount: _("Unmount filesystem $0"),
Expand Down Expand Up @@ -428,10 +422,12 @@ export function mounting_dialog(client, block, mode, forced_options) {
return Promise.resolve();
}

const usage = utils.get_active_usage(client, block.path);

const dlg = dialog_open({
Title: cockpit.format(mode_title[mode], old_dir),
Fields: fields,
Teardown: teardown,
Teardown: TeardownMessage(usage, old_dir),
update: function (dlg, vals, trigger) {
if (trigger == "at_boot")
dlg.set_options("at_boot", { explanation: mount_explanation[vals.at_boot] });
Expand Down Expand Up @@ -467,19 +463,7 @@ export function mounting_dialog(client, block, mode, forced_options) {
}
},
Inits: [
{
title: _("Checking related processes"),
func: dlg => {
return client.find_mount_users(old_dir, is_filesystem_mounted)
.then(users => {
mount_point_users = users;
if (users.length > 0) {
dlg.set_attribute("Teardown", <StopProcessesMessage mount_point={old_dir} users={users} />);
dlg.add_danger(stop_processes_danger_message(users));
}
});
}
},
init_active_usage_processes(client, usage, old_dir),
(block.IdUsage == "crypto" && mode == "mount")
? init_existing_passphrase(block, true, type => { passphrase_type = type })
: null
Expand Down
6 changes: 2 additions & 4 deletions pkg/storaged/stratis-details.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,8 +328,7 @@ export function stratis_content_rows(client, pool, options) {
TextInput("mount_point", _("Mount point"),
{
validate: (val, values, variant) => {
if (variant !== "nomount")
return is_valid_mount_point(client, null, val);
return is_valid_mount_point(client, null, val, variant == "nomount");
}
}),
CheckBoxes("mount_options", _("Mount options"),
Expand Down Expand Up @@ -514,8 +513,7 @@ function create_fs(client, pool) {
TextInput("mount_point", _("Mount point"),
{
validate: (val, values, variant) => {
if (variant !== "nomount")
return is_valid_mount_point(client, null, val);
return is_valid_mount_point(client, null, val, variant == "nomount");
}
}),
CheckBoxes("mount_options", _("Mount options"),
Expand Down
108 changes: 93 additions & 15 deletions pkg/storaged/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -741,8 +741,34 @@ export function get_children(client, path) {
return children;
}

export function find_children_for_mount_point(client, mount_point, self) {
const children = {};

function is_self(b) {
return self && (b == self || client.blocks[b.CryptoBackingDevice] == self);
}

for (const p in client.blocks) {
const b = client.blocks[p];
const fs = client.blocks_fsys[p];

if (is_self(b))
continue;

if (fs) {
for (const mp of fs.MountPoints) {
const mpd = decode_filename(mp);
if (mpd.length > mount_point.length && mpd.indexOf(mount_point) == 0 && mpd[mount_point.length] == "/")
children[mpd] = b;
}
}
}

return children;
}

export function get_active_usage(client, path, top_action, child_action) {
function get_usage(path, level) {
function get_usage(usage, path, level) {
const block = client.blocks[path];
const fsys = client.blocks_fsys[path];
const mdraid = block && client.mdraids[block.MDRaidMember];
Expand All @@ -752,7 +778,7 @@ export function get_active_usage(client, path, top_action, child_action) {
const stratis_blockdev = block && client.blocks_stratis_blockdev[path];
const stratis_pool = stratis_blockdev && client.stratis_pools[stratis_blockdev.Pool];

const usage = flatten(get_children_for_teardown(client, path).map(p => get_usage(p, level + 1)));
get_children_for_teardown(client, path).map(p => get_usage(usage, p, level + 1));

function get_actions(teardown_action) {
const actions = [];
Expand All @@ -764,16 +790,34 @@ export function get_active_usage(client, path, top_action, child_action) {
return actions;
}

function enter_unmount(block, location, is_top) {
for (const u of usage) {
if (u.usage == 'mounted' && u.location == location) {
if (is_top) {
u.actions = get_actions(_("unmount"));
u.set_noauto = false;
Comment on lines +795 to +798

This comment was marked as resolved.

Sign in to view

}
return;
}
}
usage.push({
level,
block,
usage: 'mounted',
location,
set_noauto: !is_top,
actions: is_top ? get_actions(_("unmount")) : [_("unmount")],
blocking: false
});
}

if (fsys && fsys.MountPoints.length > 0) {
fsys.MountPoints.forEach(mp => {
usage.push({
level,
usage: 'mounted',
block,
location: decode_filename(mp),
actions: get_actions(_("unmount")),
blocking: false,
});
const mpd = decode_filename(mp);
const children = find_children_for_mount_point(client, mpd, null);
for (const c in children)
enter_unmount(children[c], c, false);
enter_unmount(block, mpd, true);
});
} else if (mdraid) {
const active_state = mdraid.ActiveDevices.find(as => as[0] == block.path);
Expand Down Expand Up @@ -828,7 +872,8 @@ export function get_active_usage(client, path, top_action, child_action) {
return usage;
}

let usage = get_usage(path, 0);
let usage = [];
get_usage(usage, path, 0);
Copy link
Member

Choose a reason for hiding this comment

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

Can't say I am super big fan of the indirection of filling in usage.

Copy link
Member Author

Choose a reason for hiding this comment

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

Immutable data structures are overrated. :-)


if (usage.length == 1 && usage[0].level == 0 && usage[0].usage == "none")
usage = [];
Expand All @@ -839,6 +884,37 @@ export function get_active_usage(client, path, top_action, child_action) {
return usage;
}

async function set_fsys_noauto(client, block, mount_point) {
for (const conf of block.Configuration) {
if (conf[0] == "fstab" &&
decode_filename(conf[1].dir.v) == mount_point) {
const options = parse_options(get_block_mntopts(conf[1]));
if (options.indexOf("noauto") >= 0)
continue;
options.push("noauto");
const new_conf = [
"fstab",
Object.assign({ }, conf[1],
{
opts: {
t: 'ay',
v: encode_filename(unparse_options(options))
}
})
];
await block.UpdateConfigurationItem(conf, new_conf, { });
}
}

const crypto_backing = client.blocks[block.CryptoBackingDevice];
if (crypto_backing) {
const crypto_backing_crypto = client.blocks_crypto[crypto_backing.path];
await set_crypto_auto_option(crypto_backing, false);
if (crypto_backing_crypto)
await crypto_backing_crypto.Lock({});
}
}

export function teardown_active_usage(client, usage) {
// The code below is complicated by the fact that the last
// physical volume of a volume group can not be removed
Expand All @@ -849,10 +925,12 @@ export function teardown_active_usage(client, usage) {
// physical volumes here, and it is easiest to catch this
// condition upfront by reshuffling the data structures.

function unmount(mounteds) {
return Promise.all(mounteds.map(m => {
return client.unmount_at(m.location, m.users);
}));
async function unmount(mounteds) {
for (const m of mounteds) {
await client.unmount_at(m.location, m.users);
if (m.set_noauto)
jelly marked this conversation as resolved.
Show resolved Hide resolved
await set_fsys_noauto(client, m.block, m.location);
}
}

function mdraid_remove(members) {
Expand Down
2 changes: 1 addition & 1 deletion test/reference
Open in desktop
Submodule reference updated 5 files
+ − TestStorageMountingLUKS-testOverMounting-overmounting-rejection-dark-pixels.png
+ − TestStorageMountingLUKS-testOverMounting-overmounting-rejection-medium-pixels.png
+ − TestStorageMountingLUKS-testOverMounting-overmounting-rejection-mobile-pixels.png
+ − TestStorageMountingLUKS-testOverMounting-overmounting-rejection-pixels.png
+ − TestStorageMountingLUKS-testOverMounting-overmounting-rejection-rtl-pixels.png
Loading