From 66ae5130a0ecf72b42503abe68c865e97ab512fd Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 11 Jun 2018 12:03:35 +0200 Subject: [PATCH 1/6] blockdev-util: let's initialize return parameter on success We document the rule that return values >= 0 of functions are supposed to indicate success, and that in case of success all return parameters should be initialized. Let's actually do so. Just a tiny coding style fix-up. --- src/basic/blockdev-util.c | 1 + src/basic/btrfs-util.c | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/basic/blockdev-util.c b/src/basic/blockdev-util.c index b15b8cf770..6495a71658 100644 --- a/src/basic/blockdev-util.c +++ b/src/basic/blockdev-util.c @@ -81,6 +81,7 @@ int get_block_device(const char *path, dev_t *dev) { if (F_TYPE_EQUAL(sfs.f_type, BTRFS_SUPER_MAGIC)) return btrfs_get_block_device(path, dev); + *dev = 0; return 0; } diff --git a/src/basic/btrfs-util.c b/src/basic/btrfs-util.c index 7720d98009..32a022f7ce 100644 --- a/src/basic/btrfs-util.c +++ b/src/basic/btrfs-util.c @@ -277,8 +277,10 @@ int btrfs_get_block_device_fd(int fd, dev_t *dev) { return -errno; /* We won't do this for btrfs RAID */ - if (fsi.num_devices != 1) + if (fsi.num_devices != 1) { + *dev = 0; return 0; + } for (id = 1; id <= fsi.max_id; id++) { struct btrfs_ioctl_dev_info_args di = { From 880f09bd91b84580fb61da3561d3830f8a951aa9 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 11 Jun 2018 12:06:27 +0200 Subject: [PATCH 2/6] blockdev: split out actual DM sysfs code of get_block_device_harder() into function of its own That way we can use it in code that already acquired a dev_t from some source. --- src/basic/blockdev-util.c | 69 +++++++++++++++++++-------------------- src/basic/blockdev-util.h | 1 + 2 files changed, 34 insertions(+), 36 deletions(-) diff --git a/src/basic/blockdev-util.c b/src/basic/blockdev-util.c index 6495a71658..0b090bb2ed 100644 --- a/src/basic/blockdev-util.c +++ b/src/basic/blockdev-util.c @@ -85,35 +85,22 @@ int get_block_device(const char *path, dev_t *dev) { return 0; } -int get_block_device_harder(const char *path, dev_t *dev) { +int block_get_originating(dev_t dt, dev_t *ret) { _cleanup_closedir_ DIR *d = NULL; _cleanup_free_ char *t = NULL; char p[SYS_BLOCK_PATH_MAX("/slaves")]; struct dirent *de, *found = NULL; - const char *q; unsigned maj, min; - dev_t dt; + const char *q; int r; - assert(path); - assert(dev); - - /* Gets the backing block device for a file system, and - * handles LUKS encrypted file systems, looking for its - * immediate parent, if there is one. */ - - r = get_block_device(path, &dt); - if (r <= 0) - return r; + /* For the specified block device tries to chase it through the layers, in case LUKS-style DM stacking is used, + * trying to find the next underlying layer. */ xsprintf_sys_block_path(p, "/slaves", dt); d = opendir(p); - if (!d) { - if (errno == ENOENT) - goto fallback; - + if (!d) return -errno; - } FOREACH_DIRENT_ALL(de, d, return -errno) { @@ -141,34 +128,28 @@ int get_block_device_harder(const char *path, dev_t *dev) { return -ENOMEM; r = read_one_line_file(u, &a); - if (r < 0) { - log_debug_errno(r, "Failed to read %s: %m", u); - goto fallback; - } + if (r < 0) + return log_debug_errno(r, "Failed to read %s: %m", u); r = read_one_line_file(v, &b); - if (r < 0) { - log_debug_errno(r, "Failed to read %s: %m", v); - goto fallback; - } + if (r < 0) + return log_debug_errno(r, "Failed to read %s: %m", v); /* Check if the parent device is the same. If not, then the two backing devices are on * different physical devices, and we don't support that. */ if (!streq(a, b)) - goto fallback; + return -ENOTUNIQ; } found = de; } if (!found) - goto fallback; + return -ENOENT; q = strjoina(p, "/", found->d_name, "/dev"); r = read_one_line_file(q, &t); - if (r == -ENOENT) - goto fallback; if (r < 0) return r; @@ -176,12 +157,28 @@ int get_block_device_harder(const char *path, dev_t *dev) { return -EINVAL; if (maj == 0) - goto fallback; + return -ENOENT; + + *ret = makedev(maj, min); + return 1; +} + +int get_block_device_harder(const char *path, dev_t *ret) { + int r; + + assert(path); + assert(ret); + + /* Gets the backing block device for a file system, and handles LUKS encrypted file systems, looking for its + * immediate parent, if there is one. */ + + r = get_block_device(path, ret); + if (r <= 0) + return r; + + r = block_get_originating(*ret, ret); + if (r < 0) + log_debug_errno(r, "Failed to chase block device '%s', ignoring: %m", path); - *dev = makedev(maj, min); - return 1; - -fallback: - *dev = dt; return 1; } diff --git a/src/basic/blockdev-util.h b/src/basic/blockdev-util.h index 500fbcdff1..34cbffd98f 100644 --- a/src/basic/blockdev-util.h +++ b/src/basic/blockdev-util.h @@ -19,6 +19,7 @@ xsprintf(buf, "/sys/dev/block/%u:%u%s", major(devno), minor(devno), strempty(suffix)) int block_get_whole_disk(dev_t d, dev_t *ret); +int block_get_originating(dev_t d, dev_t *ret); int get_block_device(const char *path, dev_t *dev); From 45c2e0685463b9d47fb647b1a94467fefd21b05c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 11 Jun 2018 12:17:32 +0200 Subject: [PATCH 3/6] cgroup: beef up device lookup logic for block devices Let's chase block devices through btrfs and LUKS like we do elsewhere. --- src/core/cgroup.c | 37 ++++++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/src/core/cgroup.c b/src/core/cgroup.c index adc3e4ab2d..eb350d7132 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -11,6 +11,7 @@ #include "alloc-util.h" #include "blockdev-util.h" #include "bpf-firewall.h" +#include "btrfs-util.h" #include "bus-error.h" #include "cgroup-util.h" #include "cgroup.h" @@ -306,30 +307,36 @@ void cgroup_context_dump(CGroupContext *c, FILE* f, const char *prefix) { } } -static int lookup_block_device(const char *p, dev_t *dev) { +static int lookup_block_device(const char *p, dev_t *ret) { struct stat st; + int r; assert(p); - assert(dev); + assert(ret); if (stat(p, &st) < 0) - return log_warning_errno(errno, "Couldn't stat device %s: %m", p); + return log_warning_errno(errno, "Couldn't stat device '%s': %m", p); if (S_ISBLK(st.st_mode)) - *dev = st.st_rdev; - else if (major(st.st_dev) != 0) { - /* If this is not a device node then find the block - * device this file is stored on */ - *dev = st.st_dev; - - /* If this is a partition, try to get the originating - * block device */ - (void) block_get_whole_disk(*dev, dev); - } else { - log_warning("%s is not a block device and file system block device cannot be determined or is not local.", p); - return -ENODEV; + *ret = st.st_rdev; + else if (major(st.st_dev) != 0) + *ret = st.st_dev; /* If this is not a device node then use the block device this file is stored on */ + else { + /* If this is btrfs, getting the backing block device is a bit harder */ + r = btrfs_get_block_device(p, ret); + if (r < 0 && r != -ENOTTY) + return log_warning_errno(r, "Failed to determine block device backing btrfs file system '%s': %m", p); + if (r == -ENOTTY) { + log_warning("'%s' is not a block device node, and file system block device cannot be determined or is not local.", p); + return -ENODEV; + } } + /* If this is a LUKS device, try to get the originating block device */ + (void) block_get_originating(*ret, ret); + + /* If this is a partition, try to get the originating block device */ + (void) block_get_whole_disk(*ret, ret); return 0; } From 9d5e9b4add4d22dedc9d3286bfae9687a2a7c641 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 11 Jun 2018 12:19:01 +0200 Subject: [PATCH 4/6] cgroup: relax checks for block device cgroup settings This drops needless safety checks that ensure we only reference block devices for blockio/io settings. The backing code was already able to accept regular file system paths too, in which case the backing device node of that file system would be used. Hence, let's drop the artificial restrictions and open up this underlying functionality. --- src/core/dbus-cgroup.c | 20 ++++++++------------ src/core/load-fragment.c | 28 ++-------------------------- 2 files changed, 10 insertions(+), 38 deletions(-) diff --git a/src/core/dbus-cgroup.c b/src/core/dbus-cgroup.c index d170f6ad9f..194b5379dc 100644 --- a/src/core/dbus-cgroup.c +++ b/src/core/dbus-cgroup.c @@ -687,9 +687,8 @@ int bus_cgroup_set_property( while ((r = sd_bus_message_read(message, "(st)", &path, &u64)) > 0) { - if (!path_startswith(path, "/dev") && - !path_startswith(path, "/run/systemd/inaccessible/")) - return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Path %s specified in %s= is not a device file in /dev", name, path); + if (!path_is_normalized(path)) + return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Path '%s' specified in %s= is not normalized.", name, path); if (!UNIT_WRITE_FLAGS_NOOP(flags)) { CGroupIODeviceLimit *a = NULL, *b; @@ -775,9 +774,8 @@ int bus_cgroup_set_property( while ((r = sd_bus_message_read(message, "(st)", &path, &weight)) > 0) { - if (!path_startswith(path, "/dev") && - !path_startswith(path, "/run/systemd/inaccessible/")) - return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Path %s specified in %s= is not a device file in /dev", name, path); + if (!path_is_normalized(path)) + return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Path '%s' specified in %s= is not normalized.", name, path); if (!CGROUP_WEIGHT_IS_OK(weight) || weight == CGROUP_WEIGHT_INVALID) return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "IODeviceWeight= value out of range"); @@ -861,9 +859,8 @@ int bus_cgroup_set_property( while ((r = sd_bus_message_read(message, "(st)", &path, &u64)) > 0) { - if (!path_startswith(path, "/dev") && - !path_startswith(path, "/run/systemd/inaccessible/")) - return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Path %s specified in %s= is not a device file in /dev", name, path); + if (!path_is_normalized(path)) + return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Path '%s' specified in %s= is not normalized.", name, path); if (!UNIT_WRITE_FLAGS_NOOP(flags)) { CGroupBlockIODeviceBandwidth *a = NULL, *b; @@ -961,9 +958,8 @@ int bus_cgroup_set_property( while ((r = sd_bus_message_read(message, "(st)", &path, &weight)) > 0) { - if (!path_startswith(path, "/dev") && - !path_startswith(path, "/run/systemd/inaccessible/")) - return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Path %s specified in %s= is not a device file in /dev", name, path); + if (!path_is_normalized(path)) + return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Path '%s' specified in %s= is not normalized.", name, path); if (!CGROUP_BLKIO_WEIGHT_IS_OK(weight) || weight == CGROUP_BLKIO_WEIGHT_INVALID) return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "BlockIODeviceWeight= out of range"); diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index fb5b742fcf..2d8525c4bd 100644 --- a/src/core/load-fragment.c +++ b/src/core/load-fragment.c @@ -3317,12 +3317,6 @@ int config_parse_io_device_weight( if (r < 0) return 0; - if (!path_startswith(resolved, "/dev") && - !path_startswith(resolved, "/run/systemd/inaccessible/")) { - log_syntax(unit, LOG_ERR, filename, line, 0, "Invalid device node path '%s', ignoring.", resolved); - return 0; - } - r = cg_weight_parse(p, &u); if (r < 0) { log_syntax(unit, LOG_ERR, filename, line, r, "IO weight '%s' invalid, ignoring: %m", p); @@ -3400,15 +3394,9 @@ int config_parse_io_limit( if (r < 0) return 0; - if (!path_startswith(resolved, "/dev") && - !path_startswith(resolved, "/run/systemd/inaccessible/")) { - log_syntax(unit, LOG_ERR, filename, line, 0, "Invalid device node path '%s', ignoring.", resolved); - return 0; - } - - if (streq("infinity", p)) { + if (streq("infinity", p)) num = CGROUP_LIMIT_MAX; - } else { + else { r = parse_size(p, 1000, &num); if (r < 0 || num <= 0) { log_syntax(unit, LOG_ERR, filename, line, 0, "Invalid IO limit '%s', ignoring.", p); @@ -3497,12 +3485,6 @@ int config_parse_blockio_device_weight( if (r < 0) return 0; - if (!path_startswith(resolved, "/dev") && - !path_startswith(resolved, "/run/systemd/inaccessible/")) { - log_syntax(unit, LOG_ERR, filename, line, 0, "Invalid device node path '%s'. Ignoring.", resolved); - return 0; - } - r = cg_blkio_weight_parse(p, &u); if (r < 0) { log_syntax(unit, LOG_ERR, filename, line, r, "Invalid block IO weight '%s', ignoring: %m", p); @@ -3581,12 +3563,6 @@ int config_parse_blockio_bandwidth( if (r < 0) return 0; - if (!path_startswith(resolved, "/dev") && - !path_startswith(resolved, "/run/systemd/inaccessible/")) { - log_syntax(unit, LOG_ERR, filename, line, 0, "Invalid device node path '%s', ignoring.", resolved); - return 0; - } - r = parse_size(p, 1000, &bytes); if (r < 0 || bytes <= 0) { log_syntax(unit, LOG_ERR, filename, line, r, "Invalid Block IO Bandwidth '%s', ignoring.", p); From 57e84e7535f9b3578313ecdf9f33c8348217a268 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 11 Jun 2018 12:22:58 +0200 Subject: [PATCH 5/6] core: rework how we validate DeviceAllow= settings Let's make sure we don't validate "char-*" and "block-*" expressions as paths. --- src/basic/path-util.c | 29 +++++++++++++++++++++++++---- src/basic/path-util.h | 4 +++- src/core/dbus-cgroup.c | 9 +++------ src/core/load-fragment.c | 16 +++++++++------- 4 files changed, 40 insertions(+), 18 deletions(-) diff --git a/src/basic/path-util.c b/src/basic/path-util.c index f36301ccf5..062b95182c 100644 --- a/src/basic/path-util.c +++ b/src/basic/path-util.c @@ -893,10 +893,31 @@ bool is_device_path(const char *path) { path_startswith(path, "/sys/"); } -bool is_deviceallow_pattern(const char *path) { - return path_startswith(path, "/dev/") || - startswith(path, "block-") || - startswith(path, "char-"); +bool valid_device_node_path(const char *path) { + + /* Some superficial checks whether the specified path is a valid device node path, all without looking at the + * actual device node. */ + + if (!PATH_STARTSWITH_SET(path, "/dev/", "/run/systemd/inaccessible/")) + return false; + + if (endswith(path, "/")) /* can't be a device node if it ends in a slash */ + return false; + + return path_is_normalized(path); +} + +bool valid_device_allow_pattern(const char *path) { + assert(path); + + /* Like valid_device_node_path(), but also allows full-subsystem expressions, like DeviceAllow= and DeviceDeny= + * accept it */ + + if (startswith(path, "block-") || + startswith(path, "char-")) + return true; + + return valid_device_node_path(path); } int systemd_installation_has_version(const char *root, unsigned minimal_version) { diff --git a/src/basic/path-util.h b/src/basic/path-util.h index d8a923d3cd..486046f861 100644 --- a/src/basic/path-util.h +++ b/src/basic/path-util.h @@ -147,7 +147,9 @@ char *file_in_same_dir(const char *path, const char *filename); bool hidden_or_backup_file(const char *filename) _pure_; bool is_device_path(const char *path); -bool is_deviceallow_pattern(const char *path); + +bool valid_device_node_path(const char *path); +bool valid_device_allow_pattern(const char *path); int systemd_installation_has_version(const char *root, unsigned minimal_version); diff --git a/src/core/dbus-cgroup.c b/src/core/dbus-cgroup.c index 194b5379dc..4eb7226979 100644 --- a/src/core/dbus-cgroup.c +++ b/src/core/dbus-cgroup.c @@ -1059,15 +1059,12 @@ int bus_cgroup_set_property( while ((r = sd_bus_message_read(message, "(ss)", &path, &rwm)) > 0) { - if ((!is_deviceallow_pattern(path) && - !path_startswith(path, "/run/systemd/inaccessible/")) || - strpbrk(path, WHITESPACE)) - return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "DeviceAllow= requires device node"); + if (!valid_device_allow_pattern(path) || strpbrk(path, WHITESPACE)) + return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "DeviceAllow= requires device node or pattern"); if (isempty(rwm)) rwm = "rwm"; - - if (!in_charset(rwm, "rwm")) + else if (!in_charset(rwm, "rwm")) return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "DeviceAllow= requires combination of rwm flags"); if (!UNIT_WRITE_FLAGS_NOOP(flags)) { diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index 2d8525c4bd..935f6be807 100644 --- a/src/core/load-fragment.c +++ b/src/core/load-fragment.c @@ -3234,14 +3234,16 @@ int config_parse_device_allow( return 0; } - r = path_simplify_and_warn(resolved, 0, unit, filename, line, lvalue); - if (r < 0) - return 0; + if (!startswith(resolved, "block-") && !startswith(resolved, "char-")) { - if (!is_deviceallow_pattern(resolved) && - !path_startswith(resolved, "/run/systemd/inaccessible/")) { - log_syntax(unit, LOG_ERR, filename, line, 0, "Invalid device node path '%s', ignoring.", resolved); - return 0; + r = path_simplify_and_warn(resolved, 0, unit, filename, line, lvalue); + if (r < 0) + return 0; + + if (!valid_device_node_path(resolved)) { + log_syntax(unit, LOG_ERR, filename, line, 0, "Invalid device node path '%s', ignoring.", resolved); + return 0; + } } if (!isempty(p) && !in_charset(p, "rwm")) { From bae47ba7d250866019212f9411e5462b480d31b8 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 11 Jun 2018 12:32:37 +0200 Subject: [PATCH 6/6] tree-wide: use PATH_STARTSWITH_SET() at two more places --- src/basic/path-util.c | 6 ++---- src/core/device.c | 3 +-- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/src/basic/path-util.c b/src/basic/path-util.c index 062b95182c..2e68de9e63 100644 --- a/src/basic/path-util.c +++ b/src/basic/path-util.c @@ -886,11 +886,9 @@ bool hidden_or_backup_file(const char *filename) { bool is_device_path(const char *path) { - /* Returns true on paths that refer to a device, either in - * sysfs or in /dev */ + /* Returns true on paths that likely refer to a device, either by path in sysfs or to something in /dev */ - return path_startswith(path, "/dev/") || - path_startswith(path, "/sys/"); + return PATH_STARTSWITH_SET(path, "/dev/", "/sys/"); } bool valid_device_node_path(const char *path) { diff --git a/src/core/device.c b/src/core/device.c index 915991bf90..3219c850ed 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -546,8 +546,7 @@ static int device_process_new(Manager *m, struct udev_device *dev) { /* Don't bother with the /dev/block links */ p = udev_list_entry_get_name(item); - if (path_startswith(p, "/dev/block/") || - path_startswith(p, "/dev/char/")) + if (PATH_STARTSWITH_SET(p, "/dev/block/", "/dev/char/")) continue; /* Verify that the symlink in the FS actually belongs