From ff27ef4b598ca6b26381c6abc124641a228f0d85 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 1 Sep 2022 19:08:10 +0200 Subject: [PATCH 1/9] loop: convert impossibe EBADF cases into asserts --- src/shared/loop-util.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/shared/loop-util.c b/src/shared/loop-util.c index def2fa89fbb..724e05b7d2f 100644 --- a/src/shared/loop-util.c +++ b/src/shared/loop-util.c @@ -878,7 +878,9 @@ static int resize_partition(int partition_fd, uint64_t offset, uint64_t size) { int loop_device_refresh_size(LoopDevice *d, uint64_t offset, uint64_t size) { struct loop_info64 info; + assert(d); + assert(d->fd >= 0); /* Changes the offset/start of the loop device relative to the beginning of the underlying file or * block device. If this loop device actually refers to a partition and not a loopback device, we'll @@ -886,9 +888,6 @@ int loop_device_refresh_size(LoopDevice *d, uint64_t offset, uint64_t size) { * * If either offset or size is UINT64_MAX we won't change that parameter. */ - if (d->fd < 0) - return -EBADF; - if (d->nr < 0) /* not a loopback device */ return resize_partition(d->fd, offset, size); @@ -924,12 +923,10 @@ int loop_device_flock(LoopDevice *d, int operation) { int loop_device_sync(LoopDevice *d) { assert(d); + assert(d->fd >= 0); /* We also do this implicitly in loop_device_unref(). Doing this explicitly here has the benefit that * we can check the return value though. */ - if (d->fd < 0) - return -EBADF; - return RET_NERRNO(fsync(d->fd)); } From 3a6ed1e19d2929270e8cd11375d5d34072336450 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 1 Sep 2022 12:32:48 +0200 Subject: [PATCH 2/9] loop-util: when clearing a loopback device delete partitions first, and take BSD lock Whenever we release a loopback device, let's first synchronously delete all partitions, so that we know that's complete and not done asynchronously in the background. Take a BSD lock on the device while doing so, so that udev won't make the devices busy while we do this. --- src/shared/loop-util.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/src/shared/loop-util.c b/src/shared/loop-util.c index 724e05b7d2f..d6999ffa757 100644 --- a/src/shared/loop-util.c +++ b/src/shared/loop-util.c @@ -697,6 +697,8 @@ int loop_device_make_by_path( } LoopDevice* loop_device_unref(LoopDevice *d) { + int r; + if (!d) return NULL; @@ -706,9 +708,23 @@ LoopDevice* loop_device_unref(LoopDevice *d) { log_debug_errno(errno, "Failed to sync loop block device, ignoring: %m"); if (d->nr >= 0 && !d->relinquished) { - if (ioctl(d->fd, LOOP_CLR_FD) < 0) - log_debug_errno(errno, "Failed to clear loop device: %m"); + /* We are supposed to clear the loopback device. Let's do this synchronously: lock + * the device, manually remove all partitions and then clear it. This should ensure + * udev doesn't concurrently access the devices, and we can be reasonably sure that + * once we are done here the device is cleared and all its partition children + * removed. Note that we lock our primary device fd here (and not a separate locking + * fd, as we do during allocation, since we want to keep the lock all the way through + * the LOOP_CLR_FD, but that call would fail if we had more than one fd open.) */ + if (flock(d->fd, LOCK_EX) < 0) + log_debug_errno(errno, "Failed to lock loop block device, ignoring: %m"); + + r = block_device_remove_all_partitions(d->fd); + if (r < 0) + log_debug_errno(r, "Failed to remove partitions of loopback block device, ignoring: %m"); + + if (ioctl(d->fd, LOOP_CLR_FD) < 0) + log_debug_errno(errno, "Failed to clear loop device, ignoring: %m"); } safe_close(d->fd); From e8383058b2378e516b1a6c092b323537624db475 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 1 Sep 2022 14:59:56 +0200 Subject: [PATCH 3/9] dissect: don't remove partitions explicitly on umount anymore We do that now automatically when releasing the loopback device, hence we can drop the redundant try entirely. --- src/dissect/dissect.c | 23 +++++------------------ 1 file changed, 5 insertions(+), 18 deletions(-) diff --git a/src/dissect/dissect.c b/src/dissect/dissect.c index 17291cf22f6..0c33cacdbad 100644 --- a/src/dissect/dissect.c +++ b/src/dissect/dissect.c @@ -864,7 +864,7 @@ static int action_umount(const char *path) { _cleanup_free_ char *canonical = NULL, *devname = NULL; _cleanup_(loop_device_unrefp) LoopDevice *d = NULL; dev_t devno; - int r, k; + int r; fd = chase_symlinks_and_open(path, NULL, 0, O_DIRECTORY, &canonical); if (fd == -ENOTDIR) @@ -907,25 +907,12 @@ static int action_umount(const char *path) { loop_device_unrelinquish(d); if (arg_rmdir) { - k = RET_NERRNO(rmdir(canonical)); - if (k < 0) - log_error_errno(k, "Failed to remove mount directory '%s': %m", canonical); - } else - k = 0; - - /* Before loop_device_unrefp() kicks in, let's explicitly remove all the partition subdevices of the - * loop device. We do this to ensure that all traces of the loop device are gone by the time this - * command exits. */ - r = block_device_remove_all_partitions(d->fd); - if (r == -EBUSY) { - log_error_errno(r, "One or more partitions of '%s' are busy, ignoring", devname); - r = 0; + r = RET_NERRNO(rmdir(canonical)); + if (r < 0) + return log_error_errno(r, "Failed to remove mount directory '%s': %m", canonical); } - if (r < 0) - log_error_errno(r, "Failed to remove one or more partitions of '%s': %m", devname); - - return k < 0 ? k : r; + return 0; } static int run(int argc, char *argv[]) { From 234c2e16e5686d1a49fe84e534d4edb2db8d3719 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 1 Sep 2022 12:38:53 +0200 Subject: [PATCH 4/9] dissect: drop partition removal code This reverts a major chunk of 75d7e04eb4662a814c26010d447eed8a862f5ec1 Now that the loopback device code already destroys the partitions we don't have to do this here anymore. I am sure the right place to delete the partitions is in the loopback code, since we really only should do that for loopback devices, see bug #24431, and not on "real" block devices. I am also not convinced dropping partitions the dissection logic doesn't care about is a good idea, after all. The dissection stuff should probably not consider itself the "owner" of the block devices it analyzes, but take a more passive role: figure out what is what, but not modify it. Fixes: #24431 --- src/core/namespace.c | 1 - src/dissect/dissect.c | 2 - src/gpt-auto-generator/gpt-auto-generator.c | 2 - src/shared/dissect-image.c | 47 +++------------------ src/shared/dissect-image.h | 4 -- src/sysext/sysext.c | 1 - 6 files changed, 5 insertions(+), 52 deletions(-) diff --git a/src/core/namespace.c b/src/core/namespace.c index d774467658b..aaafdd8d0f1 100644 --- a/src/core/namespace.c +++ b/src/core/namespace.c @@ -2432,7 +2432,6 @@ int setup_namespace( } } - dissected_image_relinquish(dissected_image); loop_device_relinquish(loop_device); } else if (root_directory) { diff --git a/src/dissect/dissect.c b/src/dissect/dissect.c index 0c33cacdbad..858ed6a8f97 100644 --- a/src/dissect/dissect.c +++ b/src/dissect/dissect.c @@ -685,7 +685,6 @@ static int action_mount(DissectedImage *m, LoopDevice *d) { return log_error_errno(r, "Failed to relinquish DM devices: %m"); } - dissected_image_relinquish(m); loop_device_relinquish(d); return 0; } @@ -738,7 +737,6 @@ static int action_copy(DissectedImage *m, LoopDevice *d) { return log_error_errno(r, "Failed to relinquish DM devices: %m"); } - dissected_image_relinquish(m); loop_device_relinquish(d); if (arg_action == ACTION_COPY_FROM) { diff --git a/src/gpt-auto-generator/gpt-auto-generator.c b/src/gpt-auto-generator/gpt-auto-generator.c index a95f384ecbc..bd16ae333c9 100644 --- a/src/gpt-auto-generator/gpt-auto-generator.c +++ b/src/gpt-auto-generator/gpt-auto-generator.c @@ -772,8 +772,6 @@ static int enumerate_partitions(dev_t devnum) { r = k; } - dissected_image_relinquish(m); - return r; } diff --git a/src/shared/dissect-image.c b/src/shared/dissect-image.c index 4782330a0c3..45218944e01 100644 --- a/src/shared/dissect-image.c +++ b/src/shared/dissect-image.c @@ -150,20 +150,9 @@ static void check_partition_flags( } #endif -static void dissected_partition_done(int fd, DissectedPartition *p) { - assert(fd >= 0); +static void dissected_partition_done(DissectedPartition *p) { assert(p); -#if HAVE_BLKID - if (p->node && p->partno > 0 && !p->relinquished) { - int r; - - r = block_device_remove_partition(fd, p->node, p->partno); - if (r < 0) - log_debug_errno(r, "BLKPG_DEL_PARTITION failed, ignoring: %m"); - } -#endif - free(p->fstype); free(p->node); free(p->label); @@ -312,14 +301,9 @@ int dissect_image( return -ENOMEM; *m = (DissectedImage) { - .fd = -1, .has_init_system = -1, }; - m->fd = fcntl(fd, F_DUPFD_CLOEXEC, 3); - if (m->fd < 0) - return -errno; - r = sd_device_get_sysname(d, &sysname); if (r < 0) return log_debug_errno(r, "Failed to get device sysname: %m"); @@ -775,14 +759,10 @@ int dissect_image( * scheme in OS images. */ if (!PARTITION_DESIGNATOR_VERSIONED(designator) || - strverscmp_improved(m->partitions[designator].label, label) >= 0) { - r = block_device_remove_partition(fd, node, nr); - if (r < 0) - log_debug_errno(r, "BLKPG_DEL_PARTITION failed, ignoring: %m"); + strverscmp_improved(m->partitions[designator].label, label) >= 0) continue; - } - dissected_partition_done(fd, m->partitions + designator); + dissected_partition_done(m->partitions + designator); } if (fstype) { @@ -852,12 +832,8 @@ int dissect_image( const char *sid, *options = NULL; /* First one wins */ - if (m->partitions[PARTITION_XBOOTLDR].found) { - r = block_device_remove_partition(fd, node, nr); - if (r < 0) - log_debug_errno(r, "BLKPG_DEL_PARTITION failed, ignoring: %m"); + if (m->partitions[PARTITION_XBOOTLDR].found) continue; - } sid = blkid_partition_get_uuid(pp); if (sid) @@ -1171,9 +1147,8 @@ DissectedImage* dissected_image_unref(DissectedImage *m) { return NULL; for (PartitionDesignator i = 0; i < _PARTITION_DESIGNATOR_MAX; i++) - dissected_partition_done(m->fd, m->partitions + i); + dissected_partition_done(m->partitions + i); - safe_close(m->fd); free(m->image_name); free(m->hostname); strv_free(m->machine_info); @@ -1183,16 +1158,6 @@ DissectedImage* dissected_image_unref(DissectedImage *m) { return mfree(m); } -void dissected_image_relinquish(DissectedImage *m) { - assert(m); - - /* Partitions are automatically removed when the underlying loop device is closed. We just need to - * make sure we don't try to remove the partitions early. */ - - for (PartitionDesignator i = 0; i < _PARTITION_DESIGNATOR_MAX; i++) - m->partitions[i].relinquished = true; -} - static int is_loop_device(const char *path) { char s[SYS_BLOCK_PATH_MAX("/../loop/")]; struct stat st; @@ -3046,7 +3011,6 @@ int mount_image_privately_interactively( return log_error_errno(r, "Failed to relinquish DM devices: %m"); } - dissected_image_relinquish(dissected_image); loop_device_relinquish(d); *ret_directory = TAKE_PTR(created_dir); @@ -3209,7 +3173,6 @@ int verity_dissect_and_mount( return log_debug_errno(r, "Failed to relinquish decrypted image: %m"); } - dissected_image_relinquish(dissected_image); loop_device_relinquish(loop_device); return 0; diff --git a/src/shared/dissect-image.h b/src/shared/dissect-image.h index 5230189c163..55bb8a1dadc 100644 --- a/src/shared/dissect-image.h +++ b/src/shared/dissect-image.h @@ -31,7 +31,6 @@ struct DissectedPartition { char *mount_options; uint64_t size; uint64_t offset; - bool relinquished; }; typedef enum PartitionDesignator { @@ -204,8 +203,6 @@ typedef enum DissectImageFlags { } DissectImageFlags; struct DissectedImage { - int fd; /* Backing fd */ - bool encrypted:1; bool has_verity:1; /* verity available in image, but not necessarily used */ bool has_verity_sig:1; /* pkcs#7 signature embedded in image */ @@ -261,7 +258,6 @@ int dissect_image_and_warn(int fd, const char *name, const VeritySettings *verit DissectedImage* dissected_image_unref(DissectedImage *m); DEFINE_TRIVIAL_CLEANUP_FUNC(DissectedImage*, dissected_image_unref); -void dissected_image_relinquish(DissectedImage *m); int dissected_image_decrypt(DissectedImage *m, const char *passphrase, const VeritySettings *verity, DissectImageFlags flags, DecryptedImage **ret); int dissected_image_decrypt_interactively(DissectedImage *m, const char *passphrase, const VeritySettings *verity, DissectImageFlags flags, DecryptedImage **ret); diff --git a/src/sysext/sysext.c b/src/sysext/sysext.c index a8593cec4de..2f6cf22d2e8 100644 --- a/src/sysext/sysext.c +++ b/src/sysext/sysext.c @@ -585,7 +585,6 @@ static int merge_subprocess(Hashmap *images, const char *workspace) { return log_error_errno(r, "Failed to relinquish DM devices: %m"); } - dissected_image_relinquish(m); loop_device_relinquish(d); break; } From 7f52206a2bc128f9ae8306db43aa6e2f7d916f82 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 1 Sep 2022 15:00:30 +0200 Subject: [PATCH 5/9] loop-util: rework how we lock loopback block devices Let's rework how we lock loopback block devices in two ways: 1. Lock a separate fd, instead of the main block device fd. We already did that for our internal locking when allocating loopback block devices, but do so for the exposed locking (i.e. loop_device_flock()), too, so that the lock is independent of the main fd we actually use of IO. 2. Instead of locking the device during allocation of the loopback device, then unlocking it (which will make udev run), and then re-locking things if we need, let's instead just keep the lock the whole time, to make things a bit safer and faster, and not have to wait for udev at all. This is done by adding a "lock_op" parameter to loop device allocation functions that declares the initial state of the lock, and is one of LOCK_UN/LOCK_SH/LOCK_EX. This change also shortens a lot of code, since we allocate + immediately lock loopback devices pretty much everywhere. --- src/core/namespace.c | 7 +-- src/dissect/dissect.c | 13 +---- src/home/homework-luks.c | 10 ++-- src/nspawn/nspawn.c | 8 +-- src/partition/repart.c | 12 +---- src/portable/portable.c | 6 +-- src/shared/discover-image.c | 8 +-- src/shared/dissect-image.c | 11 +--- src/shared/loop-util.c | 103 +++++++++++++++++++++++++++++------- src/shared/loop-util.h | 7 +-- src/sysext/sysext.c | 5 +- src/test/test-loop-block.c | 12 +---- 12 files changed, 105 insertions(+), 97 deletions(-) diff --git a/src/core/namespace.c b/src/core/namespace.c index aaafdd8d0f1..831dcaa81ec 100644 --- a/src/core/namespace.c +++ b/src/core/namespace.c @@ -2058,16 +2058,11 @@ int setup_namespace( root_image, FLAGS_SET(dissect_image_flags, DISSECT_IMAGE_DEVICE_READ_ONLY) ? O_RDONLY : -1 /* < 0 means writable if possible, read-only as fallback */, FLAGS_SET(dissect_image_flags, DISSECT_IMAGE_NO_PARTITION_TABLE) ? 0 : LO_FLAGS_PARTSCAN, + LOCK_SH, &loop_device); if (r < 0) return log_debug_errno(r, "Failed to create loop device for root image: %m"); - /* Make sure udevd won't issue BLKRRPART (which might flush out the loaded partition table) - * while we are still trying to mount things */ - r = loop_device_flock(loop_device, LOCK_SH); - if (r < 0) - return log_debug_errno(r, "Failed to lock loopback device with LOCK_SH: %m"); - r = dissect_image( loop_device->fd, &verity, diff --git a/src/dissect/dissect.c b/src/dissect/dissect.c index 858ed6a8f97..f0094a390f7 100644 --- a/src/dissect/dissect.c +++ b/src/dissect/dissect.c @@ -885,14 +885,10 @@ static int action_umount(const char *path) { return log_error_errno(r, "Failed to get devname of block device " DEVNUM_FORMAT_STR ": %m", DEVNUM_FORMAT_VAL(devno)); - r = loop_device_open(devname, 0, &d); + r = loop_device_open(devname, 0, LOCK_EX, &d); if (r < 0) return log_error_errno(r, "Failed to open loop device '%s': %m", devname); - r = loop_device_flock(d, LOCK_EX); - if (r < 0) - return log_error_errno(r, "Failed to lock loop device '%s': %m", devname); - /* We've locked the loop device, now we're ready to unmount. To allow the unmount to succeed, we have * to close the O_PATH fd we opened earlier. */ fd = safe_close(fd); @@ -944,16 +940,11 @@ static int run(int argc, char *argv[]) { arg_image, FLAGS_SET(arg_flags, DISSECT_IMAGE_DEVICE_READ_ONLY) ? O_RDONLY : O_RDWR, FLAGS_SET(arg_flags, DISSECT_IMAGE_NO_PARTITION_TABLE) ? 0 : LO_FLAGS_PARTSCAN, + LOCK_SH, &d); if (r < 0) return log_error_errno(r, "Failed to set up loopback device for %s: %m", arg_image); - /* Make sure udevd doesn't issue BLKRRPART underneath us thus making devices disappear in the middle, - * that we assume already are there. */ - r = loop_device_flock(d, LOCK_SH); - if (r < 0) - return log_error_errno(r, "Failed to lock loopback device: %m"); - r = dissect_image_and_warn( d->fd, arg_image, diff --git a/src/home/homework-luks.c b/src/home/homework-luks.c index 68a5ac9c2e3..eea282fe12b 100644 --- a/src/home/homework-luks.c +++ b/src/home/homework-luks.c @@ -1284,7 +1284,7 @@ int home_setup_luks( return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Failed to determine backing device for DM %s.", setup->dm_name); if (!setup->loop) { - r = loop_device_open(n, O_RDWR, &setup->loop); + r = loop_device_open(n, O_RDWR, LOCK_UN, &setup->loop); if (r < 0) return log_error_errno(r, "Failed to open loopback device %s: %m", n); } @@ -1378,7 +1378,7 @@ int home_setup_luks( return r; } - r = loop_device_make(setup->image_fd, O_RDWR, offset, size, 0, &setup->loop); + r = loop_device_make(setup->image_fd, O_RDWR, offset, size, 0, LOCK_UN, &setup->loop); if (r == -ENOENT) { log_error_errno(r, "Loopback block device support is not available on this system."); return -ENOLINK; /* make recognizable */ @@ -2299,7 +2299,7 @@ int home_create_luks( log_info("Writing of partition table completed."); - r = loop_device_make(setup->image_fd, O_RDWR, partition_offset, partition_size, 0, &setup->loop); + r = loop_device_make(setup->image_fd, O_RDWR, partition_offset, partition_size, 0, LOCK_EX, &setup->loop); if (r < 0) { if (r == -ENOENT) { /* this means /dev/loop-control doesn't exist, i.e. we are in a container * or similar and loopback bock devices are not available, return a @@ -2311,10 +2311,6 @@ int home_create_luks( return log_error_errno(r, "Failed to set up loopback device for %s: %m", setup->temporary_image_path); } - r = loop_device_flock(setup->loop, LOCK_EX); /* make sure udev won't read before we are done */ - if (r < 0) - return log_error_errno(r, "Failed to take lock on loop device: %m"); - log_info("Setting up loopback device %s completed.", setup->loop->node ?: ip); r = luks_format(setup->loop->node, diff --git a/src/nspawn/nspawn.c b/src/nspawn/nspawn.c index 75af4c58f91..45e138d48a9 100644 --- a/src/nspawn/nspawn.c +++ b/src/nspawn/nspawn.c @@ -5746,19 +5746,13 @@ static int run(int argc, char *argv[]) { arg_image, arg_read_only ? O_RDONLY : O_RDWR, FLAGS_SET(dissect_image_flags, DISSECT_IMAGE_NO_PARTITION_TABLE) ? 0 : LO_FLAGS_PARTSCAN, + LOCK_SH, &loop); if (r < 0) { log_error_errno(r, "Failed to set up loopback block device: %m"); goto finish; } - /* Take a LOCK_SH lock on the device, so that udevd doesn't issue BLKRRPART in our back */ - r = loop_device_flock(loop, LOCK_SH); - if (r < 0) { - log_error_errno(r, "Failed to take lock on loopback block device: %m"); - goto finish; - } - r = dissect_image_and_warn( loop->fd, arg_image, diff --git a/src/partition/repart.c b/src/partition/repart.c index 815e81052b9..bb95c0aab53 100644 --- a/src/partition/repart.c +++ b/src/partition/repart.c @@ -2789,14 +2789,10 @@ static int context_copy_blocks(Context *context) { assert_se((whole_fd = fdisk_get_devfd(context->fdisk_context)) >= 0); if (p->encrypt != ENCRYPT_OFF) { - r = loop_device_make(whole_fd, O_RDWR, p->offset, p->new_size, 0, &d); + r = loop_device_make(whole_fd, O_RDWR, p->offset, p->new_size, 0, LOCK_EX, &d); if (r < 0) return log_error_errno(r, "Failed to make loopback device of future partition %" PRIu64 ": %m", p->partno); - r = loop_device_flock(d, LOCK_EX); - if (r < 0) - return log_error_errno(r, "Failed to lock loopback device: %m"); - r = partition_encrypt(context, p, d->node, &cd, &encrypted, &encrypted_dev_fd); if (r < 0) return log_error_errno(r, "Failed to encrypt device: %m"); @@ -3038,14 +3034,10 @@ static int context_mkfs(Context *context) { /* Loopback block devices are not only useful to turn regular files into block devices, but * also to cut out sections of block devices into new block devices. */ - r = loop_device_make(fd, O_RDWR, p->offset, p->new_size, 0, &d); + r = loop_device_make(fd, O_RDWR, p->offset, p->new_size, 0, LOCK_EX, &d); if (r < 0) return log_error_errno(r, "Failed to make loopback device of future partition %" PRIu64 ": %m", p->partno); - r = loop_device_flock(d, LOCK_EX); - if (r < 0) - return log_error_errno(r, "Failed to lock loopback device: %m"); - if (p->encrypt != ENCRYPT_OFF) { r = partition_encrypt(context, p, d->node, &cd, &encrypted, &encrypted_dev_fd); if (r < 0) diff --git a/src/portable/portable.c b/src/portable/portable.c index 256362355c0..0f6c71042eb 100644 --- a/src/portable/portable.c +++ b/src/portable/portable.c @@ -333,7 +333,7 @@ static int portable_extract_by_path( assert(path); - r = loop_device_make_by_path(path, O_RDONLY, LO_FLAGS_PARTSCAN, &d); + r = loop_device_make_by_path(path, O_RDONLY, LO_FLAGS_PARTSCAN, LOCK_SH, &d); if (r == -EISDIR) { _cleanup_free_ char *image_name = NULL; @@ -359,10 +359,6 @@ static int portable_extract_by_path( /* We now have a loopback block device, let's fork off a child in its own mount namespace, mount it * there, and extract the metadata we need. The metadata is sent from the child back to us. */ - r = loop_device_flock(d, LOCK_SH); - if (r < 0) - return log_debug_errno(r, "Failed to acquire lock on loopback block device: %m"); - BLOCK_SIGNALS(SIGCHLD); r = mkdtemp_malloc("/tmp/inspect-XXXXXX", &tmpdir); diff --git a/src/shared/discover-image.c b/src/shared/discover-image.c index ea0afb81a50..3733cf06b2f 100644 --- a/src/shared/discover-image.c +++ b/src/shared/discover-image.c @@ -1192,13 +1192,7 @@ int image_read_metadata(Image *i) { _cleanup_(loop_device_unrefp) LoopDevice *d = NULL; _cleanup_(dissected_image_unrefp) DissectedImage *m = NULL; - r = loop_device_make_by_path(i->path, O_RDONLY, LO_FLAGS_PARTSCAN, &d); - if (r < 0) - return r; - - /* Make sure udevd doesn't issue BLKRRPART in the background which might make our partitions - * disappear temporarily. */ - r = loop_device_flock(d, LOCK_SH); + r = loop_device_make_by_path(i->path, O_RDONLY, LO_FLAGS_PARTSCAN, LOCK_SH, &d); if (r < 0) return r; diff --git a/src/shared/dissect-image.c b/src/shared/dissect-image.c index 45218944e01..6b3a18c011f 100644 --- a/src/shared/dissect-image.c +++ b/src/shared/dissect-image.c @@ -2966,15 +2966,11 @@ int mount_image_privately_interactively( image, FLAGS_SET(flags, DISSECT_IMAGE_DEVICE_READ_ONLY) ? O_RDONLY : O_RDWR, FLAGS_SET(flags, DISSECT_IMAGE_NO_PARTITION_TABLE) ? 0 : LO_FLAGS_PARTSCAN, + LOCK_SH, &d); if (r < 0) return log_error_errno(r, "Failed to set up loopback device for %s: %m", image); - /* Make sure udevd doesn't issue BLKRRPART behind our backs */ - r = loop_device_flock(d, LOCK_SH); - if (r < 0) - return r; - r = dissect_image_and_warn(d->fd, image, &verity, NULL, d->diskseq, d->uevent_seqnum_not_before, d->timestamp_not_before, flags, &dissected_image); if (r < 0) return r; @@ -3081,14 +3077,11 @@ int verity_dissect_and_mount( src_fd >= 0 ? FORMAT_PROC_FD_PATH(src_fd) : src, -1, verity.data_path ? 0 : LO_FLAGS_PARTSCAN, + LOCK_SH, &loop_device); if (r < 0) return log_debug_errno(r, "Failed to create loop device for image: %m"); - r = loop_device_flock(loop_device, LOCK_SH); - if (r < 0) - return log_debug_errno(r, "Failed to lock loop device: %m"); - r = dissect_image( loop_device->fd, &verity, diff --git a/src/shared/loop-util.c b/src/shared/loop-util.c index d6999ffa757..50177aed27c 100644 --- a/src/shared/loop-util.c +++ b/src/shared/loop-util.c @@ -117,13 +117,28 @@ static int device_has_block_children(sd_device *d) { return !!sd_device_enumerator_get_device_first(e); } +static int open_lock_fd(int primary_fd, int operation) { + int lock_fd; + + assert(primary_fd >= 0); + + lock_fd = fd_reopen(primary_fd, O_RDWR|O_CLOEXEC|O_NONBLOCK|O_NOCTTY); + if (lock_fd < 0) + return lock_fd; + if (flock(lock_fd, operation) < 0) + return -errno; + + return lock_fd; +} + static int loop_configure( int fd, int nr, const struct loop_config *c, bool *try_loop_configure, uint64_t *ret_seqnum_not_before, - usec_t *ret_timestamp_not_before) { + usec_t *ret_timestamp_not_before, + int *ret_lock_fd) { _cleanup_(sd_device_unrefp) sd_device *d = NULL; _cleanup_free_ char *sysname = NULL; @@ -152,11 +167,9 @@ static int loop_configure( * long time udev would possibly never run on it again, even though the fd is unlocked, simply * because we never close() it. It also has the nice benefit we can use the _cleanup_close_ logic to * automatically release the lock, after we are done. */ - lock_fd = fd_reopen(fd, O_RDWR|O_CLOEXEC|O_NONBLOCK|O_NOCTTY); + lock_fd = open_lock_fd(fd, LOCK_EX); if (lock_fd < 0) return lock_fd; - if (flock(lock_fd, LOCK_EX) < 0) - return -errno; /* Let's see if the device is really detached, i.e. currently has no associated partition block * devices. On various kernels (such as 5.8) it is possible to have a loopback block device that @@ -247,12 +260,7 @@ static int loop_configure( goto fail; } - if (ret_seqnum_not_before) - *ret_seqnum_not_before = seqnum; - if (ret_timestamp_not_before) - *ret_timestamp_not_before = timestamp; - - return 0; + goto success; } } @@ -317,10 +325,13 @@ static int loop_configure( log_debug_errno(errno, "Failed to enable direct IO mode on loopback device /dev/loop%i, ignoring: %m", nr); } +success: if (ret_seqnum_not_before) *ret_seqnum_not_before = seqnum; if (ret_timestamp_not_before) *ret_timestamp_not_before = timestamp; + if (ret_lock_fd) + *ret_lock_fd = TAKE_FD(lock_fd); return 0; @@ -373,9 +384,10 @@ static int loop_device_make_internal( uint64_t offset, uint64_t size, uint32_t loop_flags, + int lock_op, LoopDevice **ret) { - _cleanup_close_ int direct_io_fd = -1; + _cleanup_close_ int direct_io_fd = -1, lock_fd = -1; _cleanup_free_ char *loopdev = NULL; bool try_loop_configure = true; struct loop_config config; @@ -393,6 +405,12 @@ static int loop_device_make_internal( return -errno; if (S_ISBLK(st.st_mode)) { + if (lock_op != LOCK_UN) { + lock_fd = open_lock_fd(fd, lock_op); + if (lock_fd < 0) + return lock_fd; + } + if (ioctl(fd, LOOP_GET_STATUS64, &config.info) >= 0) { /* Oh! This is a loopback device? That's interesting! */ @@ -430,6 +448,7 @@ static int loop_device_make_internal( return -ENOMEM; *d = (LoopDevice) { .fd = TAKE_FD(copy), + .lock_fd = TAKE_FD(lock_fd), .nr = nr, .node = TAKE_PTR(loopdev), .relinquished = true, /* It's not allocated by us, don't destroy it when this object is freed */ @@ -519,7 +538,7 @@ static int loop_device_make_internal( if (!ERRNO_IS_DEVICE_ABSENT(errno)) return -errno; } else { - r = loop_configure(loop, nr, &config, &try_loop_configure, &seqnum, ×tamp); + r = loop_configure(loop, nr, &config, &try_loop_configure, &seqnum, ×tamp, &lock_fd); if (r >= 0) { loop_with_fd = TAKE_FD(loop); break; @@ -583,11 +602,26 @@ static int loop_device_make_internal( if (r < 0 && r != -EOPNOTSUPP) return r; + switch (lock_op & ~LOCK_NB) { + case LOCK_EX: /* Already in effect */ + break; + case LOCK_SH: /* Downgrade */ + if (flock(lock_fd, lock_op) < 0) + return -errno; + break; + case LOCK_UN: /* Release */ + lock_fd = safe_close(lock_fd); + break; + default: + assert_not_reached(); + } + d = new(LoopDevice, 1); if (!d) return -ENOMEM; *d = (LoopDevice) { .fd = TAKE_FD(loop_with_fd), + .lock_fd = TAKE_FD(lock_fd), .node = TAKE_PTR(loopdev), .nr = nr, .devno = st.st_rdev, @@ -622,6 +656,7 @@ int loop_device_make( uint64_t offset, uint64_t size, uint32_t loop_flags, + int lock_op, LoopDevice **ret) { assert(fd >= 0); @@ -633,6 +668,7 @@ int loop_device_make( offset, size, loop_flags_mangle(loop_flags), + lock_op, ret); } @@ -640,6 +676,7 @@ int loop_device_make_by_path( const char *path, int open_flags, uint32_t loop_flags, + int lock_op, LoopDevice **ret) { int r, basic_flags, direct_flags, rdwr_flags; @@ -693,7 +730,7 @@ int loop_device_make_by_path( direct ? "enabled" : "disabled", direct != (direct_flags != 0) ? " (O_DIRECT was requested but not supported)" : ""); - return loop_device_make_internal(fd, open_flags, 0, 0, loop_flags, ret); + return loop_device_make_internal(fd, open_flags, 0, 0, loop_flags, lock_op, ret); } LoopDevice* loop_device_unref(LoopDevice *d) { @@ -702,6 +739,8 @@ LoopDevice* loop_device_unref(LoopDevice *d) { if (!d) return NULL; + d->lock_fd = safe_close(d->lock_fd); + if (d->fd >= 0) { /* Implicitly sync the device, since otherwise in-flight blocks might not get written */ if (fsync(d->fd) < 0) @@ -768,8 +807,13 @@ void loop_device_unrelinquish(LoopDevice *d) { d->relinquished = false; } -int loop_device_open(const char *loop_path, int open_flags, LoopDevice **ret) { - _cleanup_close_ int loop_fd = -1; +int loop_device_open( + const char *loop_path, + int open_flags, + int lock_op, + LoopDevice **ret) { + + _cleanup_close_ int loop_fd = -1, lock_fd = -1; _cleanup_free_ char *p = NULL; struct loop_info64 info; struct stat st; @@ -798,6 +842,12 @@ int loop_device_open(const char *loop_path, int open_flags, LoopDevice **ret) { } else nr = -1; + if ((lock_op & ~LOCK_NB) != LOCK_UN) { + lock_fd = open_lock_fd(loop_fd, lock_op); + if (lock_fd < 0) + return lock_fd; + } + p = strdup(loop_path); if (!p) return -ENOMEM; @@ -808,6 +858,7 @@ int loop_device_open(const char *loop_path, int open_flags, LoopDevice **ret) { *d = (LoopDevice) { .fd = TAKE_FD(loop_fd), + .lock_fd = TAKE_FD(lock_fd), .nr = nr, .node = TAKE_PTR(p), .relinquished = true, /* It's not ours, don't try to destroy it when this object is freed */ @@ -929,12 +980,28 @@ int loop_device_refresh_size(LoopDevice *d, uint64_t offset, uint64_t size) { } int loop_device_flock(LoopDevice *d, int operation) { + assert(IN_SET(operation & ~LOCK_NB, LOCK_UN, LOCK_SH, LOCK_EX)); assert(d); - if (d->fd < 0) - return -EBADF; + /* When unlocking just close the lock fd */ + if ((operation & ~LOCK_NB) == LOCK_UN) { + d->lock_fd = safe_close(d->lock_fd); + return 0; + } - return RET_NERRNO(flock(d->fd, operation)); + /* If we had no lock fd so far, create one and lock it right-away */ + if (d->lock_fd < 0) { + assert(d->fd >= 0); + + d->lock_fd = open_lock_fd(d->fd, operation); + if (d->lock_fd < 0) + return d->lock_fd; + + return 0; + } + + /* Otherwise change the current lock mode on the existing fd */ + return RET_NERRNO(flock(d->lock_fd, operation)); } int loop_device_sync(LoopDevice *d) { diff --git a/src/shared/loop-util.h b/src/shared/loop-util.h index a33d7e3e59d..f5c37383eac 100644 --- a/src/shared/loop-util.h +++ b/src/shared/loop-util.h @@ -10,6 +10,7 @@ typedef struct LoopDevice LoopDevice; struct LoopDevice { int fd; + int lock_fd; int nr; dev_t devno; char *node; @@ -19,9 +20,9 @@ struct LoopDevice { usec_t timestamp_not_before; /* CLOCK_MONOTONIC timestamp taken immediately before attaching the loopback device, or USEC_INFINITY if we don't know */ }; -int loop_device_make(int fd, int open_flags, uint64_t offset, uint64_t size, uint32_t loop_flags, LoopDevice **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); +int loop_device_make(int fd, int open_flags, uint64_t offset, uint64_t size, uint32_t loop_flags, int lock_op, LoopDevice **ret); +int loop_device_make_by_path(const char *path, int open_flags, uint32_t loop_flags, int lock_op, LoopDevice **ret); +int loop_device_open(const char *loop_path, int open_flags, int lock_op, LoopDevice **ret); LoopDevice* loop_device_unref(LoopDevice *d); DEFINE_TRIVIAL_CLEANUP_FUNC(LoopDevice*, loop_device_unref); diff --git a/src/sysext/sysext.c b/src/sysext/sysext.c index 2f6cf22d2e8..6e533f335c4 100644 --- a/src/sysext/sysext.c +++ b/src/sysext/sysext.c @@ -534,14 +534,11 @@ static int merge_subprocess(Hashmap *images, const char *workspace) { img->path, O_RDONLY, FLAGS_SET(flags, DISSECT_IMAGE_NO_PARTITION_TABLE) ? 0 : LO_FLAGS_PARTSCAN, + LOCK_SH, &d); if (r < 0) return log_error_errno(r, "Failed to set up loopback device for %s: %m", img->path); - r = loop_device_flock(d, LOCK_SH); - if (r < 0) - return log_error_errno(r, "Failed to lock loopback device: %m"); - r = dissect_image_and_warn( d->fd, img->path, diff --git a/src/test/test-loop-block.c b/src/test/test-loop-block.c index 9179ef5d608..fb5e0a2fc99 100644 --- a/src/test/test-loop-block.c +++ b/src/test/test-loop-block.c @@ -49,16 +49,13 @@ static void* thread_func(void *ptr) { assert_se(mkdtemp_malloc(NULL, &mounted) >= 0); - r = loop_device_make(fd, O_RDONLY, 0, UINT64_MAX, LO_FLAGS_PARTSCAN, &loop); + r = loop_device_make(fd, O_RDONLY, 0, UINT64_MAX, LO_FLAGS_PARTSCAN, LOCK_SH, &loop); if (r < 0) log_error_errno(r, "Failed to allocate loopback device: %m"); assert_se(r >= 0); log_notice("Acquired loop device %s, will mount on %s", loop->node, mounted); - /* Let's make sure udev doesn't call BLKRRPART in the background, while we try to mount the device. */ - assert_se(loop_device_flock(loop, LOCK_SH) >= 0); - r = dissect_image(loop->fd, NULL, NULL, loop->diskseq, loop->uevent_seqnum_not_before, loop->timestamp_not_before, DISSECT_IMAGE_READ_ONLY, &dissected); if (r < 0) log_error_errno(r, "Failed dissect loopback device %s: %m", loop->node); @@ -215,7 +212,7 @@ static int run(int argc, char *argv[]) { assert_se(pclose(sfdisk) == 0); sfdisk = NULL; - assert_se(loop_device_make(fd, O_RDWR, 0, UINT64_MAX, LO_FLAGS_PARTSCAN, &loop) >= 0); + assert_se(loop_device_make(fd, O_RDWR, 0, UINT64_MAX, LO_FLAGS_PARTSCAN, LOCK_EX, &loop) >= 0); #if HAVE_BLKID _cleanup_(dissected_image_unrefp) DissectedImage *dissected = NULL; @@ -223,11 +220,6 @@ static int run(int argc, char *argv[]) { pthread_t threads[arg_n_threads]; sd_id128_t id; - /* Take an explicit lock while we format the file systems, in accordance with - * https://systemd.io/BLOCK_DEVICE_LOCKING/. We don't want udev to interfere and probe while we write - * or even issue BLKRRPART or similar while we are working on this. */ - assert_se(loop_device_flock(loop, LOCK_EX) >= 0); - assert_se(dissect_image(loop->fd, NULL, NULL, loop->diskseq, loop->uevent_seqnum_not_before, loop->timestamp_not_before, 0, &dissected) >= 0); assert_se(dissected->partitions[PARTITION_ESP].found); From 247738b4f52665d54858355816e9ec911eee17f5 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 1 Sep 2022 15:17:01 +0200 Subject: [PATCH 6/9] loop-util: drop code to attach empty file Back when I wrote this code I wasn't aware of BLKPG and what it can do. Hence I came up with this hack to attach an empty file to delete all partitions. But today we can do better with BLKPG: let's just explicitly remove all partitions, and then try again. --- src/shared/loop-util.c | 63 +++++++++--------------------------------- 1 file changed, 13 insertions(+), 50 deletions(-) diff --git a/src/shared/loop-util.c b/src/shared/loop-util.c index 50177aed27c..81682994de6 100644 --- a/src/shared/loop-util.c +++ b/src/shared/loop-util.c @@ -173,10 +173,9 @@ static int loop_configure( /* Let's see if the device is really detached, i.e. currently has no associated partition block * devices. On various kernels (such as 5.8) it is possible to have a loopback block device that - * superficially is detached but still has partition block devices associated for it. They only go - * away when the device is reattached. (Yes, LOOP_CLR_FD doesn't work then, because officially - * nothing is attached and LOOP_CTL_REMOVE doesn't either, since it doesn't care about partition - * block devices. */ + * superficially is detached but still has partition block devices associated for it. Let's then + * manually remove the partitions via BLKPG, and tell the caller we did that via EUCLEAN, so they try + * again. */ r = device_has_block_children(d); if (r < 0) return r; @@ -187,8 +186,14 @@ static int loop_configure( if (r > 0) return -EBUSY; - return -EUCLEAN; /* Bound but children? Tell caller to reattach something so that the - * partition block devices are gone too. */ + /* Unbound but has children? Remove all partitions, and report this to the caller, to try + * again, and count this as an attempt. */ + + r = block_device_remove_all_partitions(fd); + if (r < 0) + return r; + + return -EUCLEAN; } if (*try_loop_configure) { @@ -340,44 +345,6 @@ fail: return r; } -static int attach_empty_file(int loop, int nr) { - _cleanup_close_ int fd = -1; - - /* So here's the thing: on various kernels (5.8 at least) loop block devices might enter a state - * where they are detached but nonetheless have partitions, when used heavily. Accessing these - * partitions results in immediatey IO errors. There's no pretty way to get rid of them - * again. Neither LOOP_CLR_FD nor LOOP_CTL_REMOVE suffice (see above). What does work is to - * reassociate them with a new fd however. This is what we do here hence: we associate the devices - * with an empty file (i.e. an image that definitely has no partitions). We then immediately clear it - * again. This suffices to make the partitions go away. Ugly but appears to work. */ - - log_debug("Found unattached loopback block device /dev/loop%i with partitions. Attaching empty file to remove them.", nr); - - fd = open_tmpfile_unlinkable(NULL, O_RDONLY); - if (fd < 0) - return fd; - - if (flock(loop, LOCK_EX) < 0) - return -errno; - - if (ioctl(loop, LOOP_SET_FD, fd) < 0) - return -errno; - - if (ioctl(loop, LOOP_SET_STATUS64, &(struct loop_info64) { - .lo_flags = LO_FLAGS_READ_ONLY| - LO_FLAGS_AUTOCLEAR| - LO_FLAGS_PARTSCAN, /* enable partscan, so that the partitions really go away */ - }) < 0) - return -errno; - - if (ioctl(loop, LOOP_CLR_FD) < 0) - return -errno; - - /* The caller is expected to immediately close the loopback device after this, so that the BSD lock - * is released, and udev sees the changes. */ - return 0; -} - static int loop_device_make_internal( int fd, int open_flags, @@ -543,12 +510,8 @@ static int loop_device_make_internal( loop_with_fd = TAKE_FD(loop); break; } - if (r == -EUCLEAN) { - /* Make left-over partition disappear hack (see above) */ - r = attach_empty_file(loop, nr); - if (r < 0 && r != -EBUSY) - return r; - } else if (r != -EBUSY) + if (!IN_SET(r, -EBUSY, -EUCLEAN)) /* Busy, or some left-over partition devices that + * were cleaned up. */ return r; } From 87862cc2b4abb9564f7e0365ac515dc9020a54e4 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 1 Sep 2022 15:42:32 +0200 Subject: [PATCH 7/9] loop-util: close lock fd before trying LOOP_CLR_FD in failure path If the loopback device is open more than once LOOP_CLR_FD will fail, hence close the lock fd first explicitly, so there's definitely only one fd left. --- src/shared/loop-util.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/shared/loop-util.c b/src/shared/loop-util.c index 81682994de6..6cb370e9507 100644 --- a/src/shared/loop-util.c +++ b/src/shared/loop-util.c @@ -341,6 +341,9 @@ success: return 0; fail: + /* Close the lock fd explicitly before clearing the loopback block device, since an additional open + * fd would block the clearing to succeed */ + lock_fd = safe_close(lock_fd); (void) ioctl(fd, LOOP_CLR_FD); return r; } From 4c1d50e65cbf9c6320dbd76938a01b8f899c264e Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 1 Sep 2022 15:57:10 +0200 Subject: [PATCH 8/9] loop-util: lock the control device around clearing the loopback device and deleting it This mirrors what we already do during allocation. We lock the control device first, and then release the block device and then delete it. This makes things substantially more robust as long all participants do such locking: we won't attempt to delete a block device somebody else already is using. --- src/shared/loop-util.c | 51 ++++++++++++++++++++++++++---------------- 1 file changed, 32 insertions(+), 19 deletions(-) diff --git a/src/shared/loop-util.c b/src/shared/loop-util.c index 6cb370e9507..62e8969bed9 100644 --- a/src/shared/loop-util.c +++ b/src/shared/loop-util.c @@ -490,7 +490,10 @@ static int loop_device_make_internal( * unnecessary busywork less likely. Note that this is just something we do to optimize our * own code (and whoever else decides to use LOCK_EX locks for this), taking this lock is not * necessary, it just means it's less likely we have to iterate through this loop again and - * again if our own code races against our own code. */ + * again if our own code races against our own code. + * + * Note: our lock protocol is to take the /dev/loop-control lock first, and the block device + * lock second, if both are taken, and always in this order, to avoid ABBA locking issues. */ if (flock(control, LOCK_EX) < 0) return -errno; @@ -700,13 +703,31 @@ int loop_device_make_by_path( } LoopDevice* loop_device_unref(LoopDevice *d) { + _cleanup_close_ int control = -1; int r; if (!d) return NULL; + /* Release any lock we might have on the device first. We want to open+lock the /dev/loop-control + * device below, but our lock protocol says that if both control and block device locks are taken, + * the control lock needs to be taken first, the block device lock second — in order to avoid ABBA + * locking issues. Moreover, we want to issue LOOP_CLR_FD on the block device further down, and that + * would fail if we had another fd open to the device. */ d->lock_fd = safe_close(d->lock_fd); + /* Let's open the control device early, and lock it, so that we can release our block device and + * delete it in a synchronized fashion, and allocators won't needlessly see the block device as free + * while we are about to delete it. */ + if (d->nr >= 0 && !d->relinquished) { + 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, cannot remove loop device '%s', ignoring: %m", strna(d->node)); + else if (flock(control, LOCK_EX) < 0) + log_debug_errno(errno, "Failed to lock loop control device, ignoring: %m"); + } + + /* Then let's release the loopback block device */ if (d->fd >= 0) { /* Implicitly sync the device, since otherwise in-flight blocks might not get written */ if (fsync(d->fd) < 0) @@ -735,25 +756,17 @@ LoopDevice* loop_device_unref(LoopDevice *d) { safe_close(d->fd); } - if (d->nr >= 0 && !d->relinquished) { - _cleanup_close_ int control = -1; - - control = open("/dev/loop-control", O_RDWR|O_CLOEXEC|O_NOCTTY|O_NONBLOCK); - if (control < 0) - 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; - } - (void) usleep(50 * USEC_PER_MSEC); + /* Now that the block device is released, let's also try to remove it */ + if (control >= 0) + for (unsigned n_attempts = 0;;) { + if (ioctl(control, LOOP_CTL_REMOVE, d->nr) >= 0) + break; + if (errno != EBUSY || ++n_attempts >= 64) { + log_debug_errno(errno, "Failed to remove device %s: %m", strna(d->node)); + break; } - } + (void) usleep(50 * USEC_PER_MSEC); + } free(d->node); return mfree(d); From 7cb349f0cae22cc9b0b17d8a52ca0fcd0435160f Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 1 Sep 2022 21:34:58 +0200 Subject: [PATCH 9/9] loop-util: make clearer how LoopDevice objects that do not encapsulate an actual loopback device are set up --- src/shared/loop-util.c | 4 ++-- src/shared/loop-util.h | 5 ++++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/shared/loop-util.c b/src/shared/loop-util.c index 62e8969bed9..32941c7fa1f 100644 --- a/src/shared/loop-util.c +++ b/src/shared/loop-util.c @@ -719,7 +719,7 @@ LoopDevice* loop_device_unref(LoopDevice *d) { /* Let's open the control device early, and lock it, so that we can release our block device and * delete it in a synchronized fashion, and allocators won't needlessly see the block device as free * while we are about to delete it. */ - if (d->nr >= 0 && !d->relinquished) { + if (!LOOP_DEVICE_IS_FOREIGN(d) && !d->relinquished) { 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, cannot remove loop device '%s', ignoring: %m", strna(d->node)); @@ -733,7 +733,7 @@ LoopDevice* loop_device_unref(LoopDevice *d) { if (fsync(d->fd) < 0) log_debug_errno(errno, "Failed to sync loop block device, ignoring: %m"); - if (d->nr >= 0 && !d->relinquished) { + if (!LOOP_DEVICE_IS_FOREIGN(d) && !d->relinquished) { /* We are supposed to clear the loopback device. Let's do this synchronously: lock * the device, manually remove all partitions and then clear it. This should ensure * udev doesn't concurrently access the devices, and we can be reasonably sure that diff --git a/src/shared/loop-util.h b/src/shared/loop-util.h index f5c37383eac..bdbdac60eb0 100644 --- a/src/shared/loop-util.h +++ b/src/shared/loop-util.h @@ -11,7 +11,7 @@ typedef struct LoopDevice LoopDevice; struct LoopDevice { int fd; int lock_fd; - int nr; + int nr; /* The loopback device index (i.e. 4 for /dev/loop4); if this object encapsulates a non-loopback block device, set to -1 */ dev_t devno; char *node; bool relinquished; @@ -20,6 +20,9 @@ struct LoopDevice { usec_t timestamp_not_before; /* CLOCK_MONOTONIC timestamp taken immediately before attaching the loopback device, or USEC_INFINITY if we don't know */ }; +/* Returns true if LoopDevice object is not actually a loopback device but some other block device we just wrap */ +#define LOOP_DEVICE_IS_FOREIGN(d) ((d)->nr < 0) + int loop_device_make(int fd, int open_flags, uint64_t offset, uint64_t size, uint32_t loop_flags, int lock_op, LoopDevice **ret); int loop_device_make_by_path(const char *path, int open_flags, uint32_t loop_flags, int lock_op, LoopDevice **ret); int loop_device_open(const char *loop_path, int open_flags, int lock_op, LoopDevice **ret);