From 14bb274d3fddd645568c2eb679743ed8529024ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 5 Dec 2019 18:19:06 +0100 Subject: [PATCH 1/9] networkd: check return value CID 1408497. --- src/network/networkd-brvlan.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/network/networkd-brvlan.c b/src/network/networkd-brvlan.c index 970065d32ad..41f09287f2b 100644 --- a/src/network/networkd-brvlan.c +++ b/src/network/networkd-brvlan.c @@ -180,7 +180,9 @@ int br_vlan_configure(Link *link, uint16_t pvid, uint32_t *br_vid_bitmap, uint32 /* master needs flag self */ if (!link->network->bridge) { flags = BRIDGE_FLAGS_SELF; - sd_netlink_message_append_data(req, IFLA_BRIDGE_FLAGS, &flags, sizeof(uint16_t)); + r = sd_netlink_message_append_data(req, IFLA_BRIDGE_FLAGS, &flags, sizeof(uint16_t)); + if (r < 0) + return log_link_error_errno(link, r, "Could not open IFLA_BRIDGE_FLAGS: %m"); } /* add vlan info */ From 1163a2e98a89335c2a9151d48c1f318765e74a18 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 6 Dec 2019 10:39:14 +0100 Subject: [PATCH 2/9] shared/loop-util: operate on the right fd 'loop' is always -1 at this point in the code. --- src/shared/loop-util.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/shared/loop-util.c b/src/shared/loop-util.c index acf3eae2d72..05b0d51d979 100644 --- a/src/shared/loop-util.c +++ b/src/shared/loop-util.c @@ -44,7 +44,7 @@ int loop_device_make_full( return -errno; if (S_ISBLK(st.st_mode)) { - if (ioctl(loop, LOOP_GET_STATUS64, &info) >= 0) { + if (ioctl(fd, LOOP_GET_STATUS64, &info) >= 0) { /* Oh! This is a loopback device? That's interesting! */ #if HAVE_VALGRIND_MEMCHECK_H From ba5450f4119c75ad9e3214bf4edeecb4d3f55a06 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 6 Dec 2019 10:40:20 +0100 Subject: [PATCH 3/9] shared/loop-util: fix leak of fd in error path --- src/shared/loop-util.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/shared/loop-util.c b/src/shared/loop-util.c index 05b0d51d979..c3d8f81d087 100644 --- a/src/shared/loop-util.c +++ b/src/shared/loop-util.c @@ -58,7 +58,7 @@ int loop_device_make_full( } if (offset == 0 && IN_SET(size, 0, UINT64_MAX)) { - int copy; + _cleanup_close_ int copy = -1; /* If this is already a block device, store a copy of the fd as it is */ @@ -71,7 +71,7 @@ int loop_device_make_full( return -ENOMEM; *d = (LoopDevice) { - .fd = copy, + .fd = TAKE_FD(copy), .nr = nr, .node = TAKE_PTR(loopdev), .relinquished = true, /* It's not allocated by us, don't destroy it when this object is freed */ From 6b2a8b80b4bf0da6634e1cf7f36a1bf8f2431364 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 6 Dec 2019 10:56:49 +0100 Subject: [PATCH 4/9] shared/loop-util: drop inline function with one use --- src/shared/loop-util.c | 2 +- src/shared/loop-util.h | 4 ---- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/src/shared/loop-util.c b/src/shared/loop-util.c index c3d8f81d087..2b17df6b5d0 100644 --- a/src/shared/loop-util.c +++ b/src/shared/loop-util.c @@ -163,7 +163,7 @@ int loop_device_make_by_path(const char *path, int open_flags, uint32_t loop_fla if (fd < 0) return -errno; - return loop_device_make(fd, open_flags, loop_flags, ret); + return loop_device_make_full(fd, open_flags, 0, 0, loop_flags, ret); } LoopDevice* loop_device_unref(LoopDevice *d) { diff --git a/src/shared/loop-util.h b/src/shared/loop-util.h index 5156b46ad61..a9c9a647a57 100644 --- a/src/shared/loop-util.h +++ b/src/shared/loop-util.h @@ -15,10 +15,6 @@ struct LoopDevice { }; int loop_device_make_full(int fd, int open_flags, uint64_t offset, uint64_t size, uint32_t loop_flags, LoopDevice **ret); -static inline int loop_device_make(int fd, int open_flags, uint32_t loop_flags, LoopDevice **ret) { - return loop_device_make_full(fd, open_flags, 0, 0, loop_flags, ret); -} - int loop_device_make_by_path(const char *path, int open_flags, uint32_t loop_flags, LoopDevice **ret); int loop_device_open(const char *loop_path, int open_flags, LoopDevice **ret); From e8af3bfd635cf6e51ec0d0f57cd4b48715f9b0da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 6 Dec 2019 11:10:10 +0100 Subject: [PATCH 5/9] shared/loop-util: fix error handling in loop_device_make_full() The function no longer returns the fd. This complicated semantics, because it wasn't clear what holds the ownership: the return value or the output parameter. There were no users of the fd in the return value, so let's simplify things conceptually and only return the fd once. Reduce the scope of variables. LOOP_CLR_FD was called on the wrong fd. Let's use a cleanup function to make this automatic and reduce chances of a mixup in the future. CID 1408498. --- src/shared/loop-util.c | 57 +++++++++++++++++++----------------------- 1 file changed, 26 insertions(+), 31 deletions(-) diff --git a/src/shared/loop-util.c b/src/shared/loop-util.c index 2b17df6b5d0..c2d28a348ce 100644 --- a/src/shared/loop-util.c +++ b/src/shared/loop-util.c @@ -20,6 +20,13 @@ #include "stat-util.h" #include "stdio-util.h" +static void cleanup_clear_loop_close(int *fd) { + if (*fd >= 0) { + (void) ioctl(*fd, LOOP_CLR_FD); + (void) safe_close(*fd); + } +} + int loop_device_make_full( int fd, int open_flags, @@ -28,9 +35,7 @@ int loop_device_make_full( uint32_t loop_flags, LoopDevice **ret) { - _cleanup_close_ int control = -1, loop = -1; _cleanup_free_ char *loopdev = NULL; - unsigned n_attempts = 0; struct loop_info64 info; LoopDevice *d = NULL; struct stat st; @@ -69,7 +74,6 @@ int loop_device_make_full( d = new(LoopDevice, 1); if (!d) return -ENOMEM; - *d = (LoopDevice) { .fd = TAKE_FD(copy), .nr = nr, @@ -86,13 +90,18 @@ int loop_device_make_full( return r; } + _cleanup_close_ int control = -1; + _cleanup_(cleanup_clear_loop_close) int loop_with_fd = -1; + control = open("/dev/loop-control", O_RDWR|O_CLOEXEC|O_NOCTTY|O_NONBLOCK); if (control < 0) return -errno; /* Loop around LOOP_CTL_GET_FREE, since at the moment we attempt to open the returned device it might * be gone already, taken by somebody else racing against us. */ - for (;;) { + for (unsigned n_attempts = 0;;) { + _cleanup_close_ int loop = -1; + nr = ioctl(control, LOOP_CTL_GET_FREE); if (nr < 0) return -errno; @@ -103,17 +112,16 @@ int loop_device_make_full( loop = open(loopdev, O_CLOEXEC|O_NONBLOCK|O_NOCTTY|open_flags); if (loop < 0) return -errno; - if (ioctl(loop, LOOP_SET_FD, fd) < 0) { - if (errno != EBUSY) - return -errno; - - if (++n_attempts >= 64) /* Give up eventually */ - return -EBUSY; - } else + if (ioctl(loop, LOOP_SET_FD, fd) >= 0) { + loop_with_fd = TAKE_FD(loop); break; + } + if (errno != EBUSY) + return -errno; + if (++n_attempts >= 64) /* Give up eventually */ + return -EBUSY; loopdev = mfree(loopdev); - loop = safe_close(loop); } info = (struct loop_info64) { @@ -123,33 +131,20 @@ int loop_device_make_full( .lo_sizelimit = size == UINT64_MAX ? 0 : size, }; - if (ioctl(loop, LOOP_SET_STATUS64, &info) < 0) { - r = -errno; - goto fail; - } + if (ioctl(loop_with_fd, LOOP_SET_STATUS64, &info) < 0) + return -errno; d = new(LoopDevice, 1); - if (!d) { - r = -ENOMEM; - goto fail; - } - + if (!d) + return -ENOMEM; *d = (LoopDevice) { - .fd = TAKE_FD(loop), + .fd = TAKE_FD(loop_with_fd), .node = TAKE_PTR(loopdev), .nr = nr, }; *ret = d; - return d->fd; - -fail: - if (fd >= 0) - (void) ioctl(fd, LOOP_CLR_FD); - if (d && d->fd >= 0) - (void) ioctl(d->fd, LOOP_CLR_FD); - - return r; + return 0; } int loop_device_make_by_path(const char *path, int open_flags, uint32_t loop_flags, LoopDevice **ret) { From f2d9213fee0fe0197845734e7c0554b1f8afc6d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 6 Dec 2019 11:35:57 +0100 Subject: [PATCH 6/9] shared/loop-util: spin on LOOP_CTL_REMOVE If we call LOOP_CLR_FD and LOOP_CTL_REMOVE too rapidly, the kernel cannot deal with that (5.3.13-300.fc31.x86_64 running on dual core Intel(R) Core(TM) i7-6500U CPU @ 2.50GHz). $ sudo strace -eioctl build/test-dissect-image /tmp/foobar3.img ioctl(3, TCGETS, 0x7ffcee47de20) = -1 ENOTTY (Inappropriate ioctl for device) ioctl(4, LOOP_CTL_GET_FREE) = 9 ioctl(5, LOOP_SET_FD, 3) = 0 ioctl(5, LOOP_SET_STATUS64, {lo_offset=0, lo_number=0, lo_flags=LO_FLAGS_READ_ONLY|LO_FLAGS_AUTOCLEAR|LO_FLAGS_PARTSCAN, lo_file_name="", ...}) = 0 ioctl(5, BLKGETSIZE64, [299999744]) = 0 ioctl(5, CDROM_GET_CAPABILITY, 0) = -1 EINVAL (Invalid argument) ioctl(5, BLKSSZGET, [512]) = 0 Waiting for device (parent + 0 partitions) to appear... Found root partition, writable of type btrfs at #-1 (/dev/block/7:9) ioctl(5, LOOP_CLR_FD) = 0 ioctl(3, LOOP_CTL_REMOVE, 9) = -1 EBUSY (Device or resource busy) Failed to remove loop device: Device or resource busy This seems to be clear race condition, and attaching strace is generally enough to "win" the race. But even with strace attached, we will fail occasionally. Let's wait a bit and retry. With the wait, on my machine, the second attempt always succeeds: ... Found root partition, writable of type btrfs at #-1 (/dev/block/7:9) ioctl(5, LOOP_CLR_FD) = 0 ioctl(3, LOOP_CTL_REMOVE, 9) = -1 EBUSY (Device or resource busy) ioctl(3, LOOP_CTL_REMOVE, 9) = 9 +++ exited with 0 +++ Without the wait, all 64 attempts will occasionally fail. --- src/shared/loop-util.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/src/shared/loop-util.c b/src/shared/loop-util.c index c2d28a348ce..8b4d13e6164 100644 --- a/src/shared/loop-util.c +++ b/src/shared/loop-util.c @@ -11,6 +11,7 @@ #include #include #include +#include #include "alloc-util.h" #include "fd-util.h" @@ -19,6 +20,7 @@ #include "parse-util.h" #include "stat-util.h" #include "stdio-util.h" +#include "string-util.h" static void cleanup_clear_loop_close(int *fd) { if (*fd >= 0) { @@ -166,7 +168,6 @@ LoopDevice* loop_device_unref(LoopDevice *d) { return NULL; if (d->fd >= 0) { - if (d->nr >= 0 && !d->relinquished) { if (ioctl(d->fd, LOOP_CLR_FD) < 0) log_debug_errno(errno, "Failed to clear loop device: %m"); @@ -181,11 +182,19 @@ LoopDevice* loop_device_unref(LoopDevice *d) { control = open("/dev/loop-control", O_RDWR|O_CLOEXEC|O_NOCTTY|O_NONBLOCK); if (control < 0) - log_debug_errno(errno, "Failed to open loop control device: %m"); - else { - if (ioctl(control, LOOP_CTL_REMOVE, d->nr) < 0) - log_debug_errno(errno, "Failed to remove loop device: %m"); - } + log_warning_errno(errno, + "Failed to open loop control device, cannot remove loop device %s: %m", + strna(d->node)); + else + for (unsigned n_attempts = 0;;) { + if (ioctl(control, LOOP_CTL_REMOVE, d->nr) >= 0) + break; + if (errno != EBUSY || ++n_attempts >= 64) { + log_warning_errno(errno, "Failed to remove device %s: %m", strna(d->node)); + break; + } + usleep(50 * USEC_PER_MSEC); + } } free(d->node); From a97abb30e7ebaf5cfa07e3815909d7c0c4416e6c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 6 Dec 2019 11:55:20 +0100 Subject: [PATCH 7/9] shared/efi-loader: add some debugging statements Should make it easier to figure out why some operations fail... --- src/shared/efi-loader.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/shared/efi-loader.c b/src/shared/efi-loader.c index 3d1df09907f..108f31d502b 100644 --- a/src/shared/efi-loader.c +++ b/src/shared/efi-loader.c @@ -562,17 +562,20 @@ int efi_loader_get_boot_usec(usec_t *firmware, usec_t *loader) { r = read_usec(EFI_VENDOR_LOADER, "LoaderTimeInitUSec", &x); if (r < 0) - return r; + return log_debug_errno(r, "Failed to read LoaderTimeInitUSec: %m"); r = read_usec(EFI_VENDOR_LOADER, "LoaderTimeExecUSec", &y); if (r < 0) - return r; + return log_debug_errno(r, "Failed to read LoaderTimeExecUSec: %m"); if (y == 0 || y < x) - return -EIO; + return log_debug_errno(SYNTHETIC_ERRNO(EIO), + "Bad LoaderTimeInitUSec=%"PRIu64", LoaderTimeExecUSec=%" PRIu64"; refusing.", + x, y); if (y > USEC_PER_HOUR) - return -EIO; + return log_debug_errno(SYNTHETIC_ERRNO(EIO), + "LoaderTimeExecUSec=%"PRIu64" too large, refusing.", x); *firmware = x; *loader = y; From 35b9eb0a72b6254568a294f0ebd011da20958a64 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 6 Dec 2019 12:13:34 +0100 Subject: [PATCH 8/9] basic/efivars: do not return EIO if an efivar read is shorten than fstat size On my machine stat returns size 22, but only 20 bytes are read: openat(AT_FDCWD, "/sys/firmware/efi/efivars/LoaderTimeInitUSec-4a67b082-0a4c-41cf-b6c7-440b29bb8c4f", O_RDONLY|O_NOCTTY|O_CLOEXEC) = 3 fstat(3, {st_mode=S_IFREG|0644, st_size=22, ...}) = 0 read(3, "\6\0\0\0", 4) = 4 read(3, "7\0001\0001\0003\0005\0002\0007\0\0\0", 18) = 16 Failed to read LoaderTimeInitUSec: Input/output error Let's just accept that the kernel is returning inconsistent results. It seems to happen two only two variables on my machine: /sys/firmware/efi/efivars/LoaderTimeInitUSec-4a67b082-0a4c-41cf-b6c7-440b29bb8c4f /sys/firmware/efi/efivars/LoaderTimeMenuUSec-4a67b082-0a4c-41cf-b6c7-440b29bb8c4f so it might be related to the way we write them. --- src/basic/efivars.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/basic/efivars.c b/src/basic/efivars.c index 7b14c062df1..bfde67a883b 100644 --- a/src/basic/efivars.c +++ b/src/basic/efivars.c @@ -90,13 +90,14 @@ int efi_get_variable( n = read(fd, buf, (size_t) st.st_size - 4); if (n < 0) return -errno; - if (n != st.st_size - 4) - return -EIO; + assert(n <= st.st_size - 4); /* Always NUL terminate (2 bytes, to protect UTF-16) */ - ((char*) buf)[st.st_size - 4] = 0; - ((char*) buf)[st.st_size - 4 + 1] = 0; - } + ((char*) buf)[n - 4] = 0; + ((char*) buf)[n - 4 + 1] = 0; + } else + /* Assume that the reported size is accurate */ + n = st.st_size - 4; /* Note that efivarfs interestingly doesn't require ftruncate() to update an existing EFI variable * with a smaller value. */ @@ -108,7 +109,7 @@ int efi_get_variable( *ret_value = TAKE_PTR(buf); if (ret_size) - *ret_size = (size_t) st.st_size - 4; + *ret_size = n; return 0; } From 01813148619cb4b78427abe1cb8ccbe2b9916a03 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 15 Dec 2019 20:58:59 +0100 Subject: [PATCH 9/9] shared/loop-util: spin on open() returning ENOENT too https://github.com/systemd/systemd/pull/14261#discussion_r355001559 --- src/shared/loop-util.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/src/shared/loop-util.c b/src/shared/loop-util.c index 8b4d13e6164..6cb45f1f1bc 100644 --- a/src/shared/loop-util.c +++ b/src/shared/loop-util.c @@ -112,14 +112,20 @@ int loop_device_make_full( return -ENOMEM; loop = open(loopdev, O_CLOEXEC|O_NONBLOCK|O_NOCTTY|open_flags); - if (loop < 0) - return -errno; - if (ioctl(loop, LOOP_SET_FD, fd) >= 0) { - loop_with_fd = TAKE_FD(loop); - break; + if (loop < 0) { + /* Somebody might've gotten the same number from the kernel, used the device, + * and called LOOP_CTL_REMOVE on it. Let's retry with a new number. */ + if (errno != ENOENT) + return -errno; + } else { + if (ioctl(loop, LOOP_SET_FD, fd) >= 0) { + loop_with_fd = TAKE_FD(loop); + break; + } + if (errno != EBUSY) + return -errno; } - if (errno != EBUSY) - return -errno; + if (++n_attempts >= 64) /* Give up eventually */ return -EBUSY;