Control: tag -1 patch

On Sun, 03 Jul 2016 22:49:18 +0200 Emmanuel Kasper <[email protected]>
wrote:
> Since the 0.6.1-* version of kpartx, the '-d' option does not work anymore.
> 
> Simple test case:
> 
> # add loopdevice and partition mappings, freedos.raw is here a disk image
> kpartx -av freedos.raw
> add map loop0p1 (253:0): 0 262017 linear 7:0 63
> 
> # ask kpartx to detach the loop device
> kpartx -d freedos.raw
> 
> # but the loop device is still there
> file --special-files --dereference /dev/loop0 
> /dev/loop0: DOS/MBR boot sector; partition 1 : ID=0xe, active, start-CHS 
> (0x0,1,1), end-CHS (0x13,15,63), startsector 63, 262017 sectors
> 
> losetup --list 
> NAME       SIZELIMIT OFFSET AUTOCLEAR RO BACK-FILE                            
>       DIO
> /dev/loop0         0      0         0  0 
> /home/manu/Projects/emul/i386/kvm/freedos.raw

I ran into this bug when creating bootable VM images using
vmdebootstrap. I noticed that all images, except the first one created
after a reboot, failed to boot.

The problem is that the code that is supposed to detect whether kpartx
is dealing with a loop-device-backed regular file is broken.

Assuming kpartx -d blockdev.img is called, the following happens in
kpartx.c:

>         if (stat(device, &buf)) {
>                 printf("failed to stat() %s\n", device);
>                 exit (1);
>         }

buf gets set with the status of device, which is blockdev.img.

>         if (S_ISREG (buf.st_mode)) {
>                 /* already looped file ? */
>                 loopdev = find_loop_by_file(device);

This identifies the backing loop device of blockdev.img. Let's assume
that the device is /dev/loop0.

>
>                 if (!loopdev && what == DELETE)
>                         exit (0);
>
>                 if (!loopdev) {
>                         loopdev = find_unused_loop_device();
>
>                         if (set_loop(loopdev, device, 0, &ro)) {
>                                 fprintf(stderr, "can't set up loop\n");
>                                 exit (1);
>                         }
>                         loopcreated = 1;
>                 }

The above is not executed, as loopdev has been previously set.

>                 device = loopdev;

device no longer points to "blockdev.img", but to "/dev/loop0".

>
>                 if (stat(device, &buf)) {
>                         printf("failed to stat() %s\n", device);
>                         exit (1);
>                 }
>         }

buf gets set again, but this time with the status of /dev/loop0.

Later:
>                         if (S_ISREG (buf.st_mode)) {
                                      ^^^
With buf being the status of /dev/loop0, S_ISREG evaluates to false.
Therefore, the following code which would normally remove the backing
loop device does not get called.

>                                 if (del_loop(device)) {
>                                         if (verbose)
>                                                 printf("can't del loop: %s\n",
>                                                         device);
>                                         exit(1);
>                                 }
>                                 printf("loop deleted : %s\n", device);
>                         }
>                         break;

I've sent the attached patch to the upstream mailing list, but it should
also apply cleanly to the Debian version, albeit with some offset.

Regards,
Christian
From 979f67eac9c697638cd0dda7867fb868752dd961 Mon Sep 17 00:00:00 2001
From: Christian Kastner <[email protected]>
Date: Tue, 12 Jul 2016 23:16:47 +0200
Subject: [PATCH] kpartx: Fix check whether to detach loop device

When invoking kpartx -d $FOO with $FOO being a regular file, the intention is
to also detach the loop device backing $FOO using del_loop(), but this
currently doesn't happen, as can be seen by calling losetup -a afterwards.

This is because the check for the type of $FOO, prior to the del_loop() call, is
broken. S_ISREG() is called on a buffer that initially did contain the status of
$FOO, but was later overwritten with the status of the backing loop device.

This replaces the S_ISREG() test with a check for whether the loop device path
is not NULL. If it isn't, then we know that it contains the correct path, and
del_loop() can be called safely on that.

Reported as Debian bug #829496; that bug also has more information.

Signed-off-by: Christian Kastner <[email protected]>
---
 kpartx/kpartx.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c
index 4de13fa..b7b4047 100644
--- a/kpartx/kpartx.c
+++ b/kpartx/kpartx.c
@@ -482,14 +482,14 @@ main(int argc, char **argv){
 					printf("del devmap : %s\n", partname);
 			}
 
-			if (S_ISREG (buf.st_mode)) {
-				if (del_loop(device)) {
+			if (loopdev) {
+				if (del_loop(loopdev)) {
 					if (verbose)
 						printf("can't del loop : %s\n",
-							device);
+							loopdev);
 					exit(1);
 				}
-				printf("loop deleted : %s\n", device);
+				printf("loop deleted : %s\n", loopdev);
 			}
 			break;
 
-- 
2.8.1

Attachment: signature.asc
Description: OpenPGP digital signature

Reply via email to