kpartx -d fails with image paths longer than 63 characters

[Simo Punnonen]

[Impact]
Users of kpartx to load disk images, possibly multiple images with the same file name (but in different paths).

[Test case]
See below, also see https://bugs.launchpad.net/ubuntu/+source/multipath-tools/+bug/1469143/comments/3

[Regression potential]
This changes makes the matching for loaded images more robust, and so has a very limited risk of regression. Since there is a call to stat() introduced, one may notice a slowdown if the stat() call blocks for a reason, and the operation will fail if the stat() call fails for any reason, since the device and inode are now required.

---

$ apt-show-versions multipath-tools
multipath-tools:amd64/vivid 0.4.9-3ubuntu12 uptodate

Reproduce:
Mount an image from a path longer than 63 chars succeeds:
$ sudo kpartx -av asfd1asdf2asdf3asdf4asdf5asdf6asfd7asdf8asdf9asf10asdf11asdf12asdf13/disk.img
add map loop0p1 (252:0): 0 409600 linear /dev/loop0 2048
add map loop0p2 (252:1): 0 2 linear /dev/loop0 411648
add map loop0p5 : 0 819200 linear /dev/loop0 413696
add map loop0p6 : 0 819200 linear /dev/loop0 1234944
add map loop0p7 : 0 819200 linear /dev/loop0 2056192
add map loop0p8 : 0 1316864 linear /dev/loop0 2877440

but dismounting fails:
$ sudo kpartx -dv asfd1asdf2asdf3asdf4asdf5asdf6asfd7asdf8asdf9asf10asdf11asdf12asdf13/disk.img

strace shows that the parameter on the dismount appears to get cut at 63 chars:
ioctl(3, DM_LIST_VERSIONS, 0x15b89b0) = 0
stat("asfd1asdf2asdf3asdf4asdf5asdf6asfd7asdf8asdf9asf10asdf11asdf12asdf13/disk.img", {st_mode=S_IFREG|0644, st_size=2147483648, ...}) = 0
stat("/dev/loop0", {st_mode=S_IFBLK|0660, st_rdev=makedev(7, 0), ...}) = 0
open("/dev/loop0", O_RDONLY) = 4
ioctl(4, LOOP_GET_STATUS, {number=0, offset=0, flags=0, name="asfd1asdf2asdf3asdf4asdf5asdf6asfd7asdf8asdf9asf10asdf11asdf12a", ...}) = 0
close(4) = 0
stat("/dev/loop1", {st_mode=S_IFBLK|0660, st_rdev=makedev(7, 1), ...}) = 0
open("/dev/loop1", O_RDONLY) = 4
ioctl(4, LOOP_GET_STATUS, {number=0, offset=0, flags=0, name="asfd1asdf2asdf3asdf4asdf5asdf6asfd7asdf8asdf9asf10asdf11asdf12a", ...}) = -1 ENXIO (No such device or address)

if the path is 63 chars or less, the dismount also succeeds.

This is quickly becomes an issue if you want to use full disk paths in your shell scripts.

[Launchpad Janitor (janitor)]

Status changed to 'Confirmed' because the bug affects multiple users.

[Jorge Niedbalski (niedbalski)]

I can reproduce this on precise+. I've submitted the wily branch for review.

[Simo Punnonen (simo-punnonen)]

If i understand correctly, the fix is to compare only the first 63 chars of the path.

With kpartx the precise file path matters in resolving the correct loop device, the suggested solution has the following consequence:

Assuming all loop devices are available, these commands succeed and mount their respective images to /dev/loop0 and /dev/loop1

$ sudo kpartx -av asfd1asdf2asdf3asdf4asdf5asdf6asfd7asdf8asdf9asf10asdf11asdf12asdf13/disk.img
$ sudo kpartx -av asfd1asdf2asdf3asdf4asdf5asdf6asfd7asdf8asdf9asf10asdf11asdf12asdf14/disk.img

Now, if i issue the following command:
$ sudo kpartx -dv asfd1asdf2asdf3asdf4asdf5asdf6asfd7asdf8asdf9asf10asdf11asdf12asdf14/disk.img

With the fix, if you only match the first 63 characters on dismount you likely end up dismounting the wrong one (probably loop0 as it's the first hit as the loops are iterated), as only the part of the path exceeding 63 chars is significant.

Probably something more elegant is required to fix this problem.

[Simo Punnonen (simo-punnonen)]

is the lo_name which gets stored in the loop_info structure actually required to be an actual file path or is it enough to be an arbitrary string identifier?

If it doesn't have to be a file path, could it be for instance a sha1 hash of the provided path? That fits into 40 chars and is relatively fast to calculate. A sha1 identifier would make it really hard to collide with other loop device identifiers _unless_ you pass and use the same relative image path in two separate instances (which is also an existing problem).

Alternatively it would be nice if the lo_name identifier wasn't actually based on path but some unique property of the mounted file, that way you would only need to point to an image, regardless of the path and kpartx could find if that specific image is mounted or not.

[Jorge Niedbalski (niedbalski)]

Simo

Go ahead an propose the patch you are thinking on.

Best!

On Monday, June 29, 2015, Simo Punnonen <email address hidden> wrote:

> is the lo_name which gets stored in the loop_info structure actually
> required to be an actual file path or is it enough to be an arbitrary
> string identifier?
>
> If it doesn't have to be a file path, could it be for instance a sha1
> hash of the provided path? That fits into 40 chars and is relatively
> fast to calculate. A sha1 identifier would make it really hard to
> collide with other loop device identifiers _unless_ you pass and use the
> same relative image path in two separate instances (which is also an
> existing problem).
>
> Alternatively it would be nice if the lo_name identifier wasn't actually
> based on path but some unique property of the mounted file, that way you
> would only need to point to an image, regardless of the path and kpartx
> could find if that specific image is mounted or not.
>
> --
> You received this bug notification because you are a bug assignee.
> https://bugs.launchpad.net/bugs/1469143
>
> Title:
> kpartx -d fails with image paths longer than 63 characters
>
> Status in multipath-tools package in Ubuntu:
> In Progress
> Status in multipath-tools source package in Precise:
> New
> Status in multipath-tools source package in Trusty:
> New
> Status in multipath-tools source package in Utopic:
> New
> Status in multipath-tools source package in Vivid:
> New
>
> Bug description:
> $ apt-show-versions multipath-tools
> multipath-tools:amd64/vivid 0.4.9-3ubuntu12 uptodate
>
> Reproduce:
> Mount an image from a path longer than 63 chars succeeds:
> $ sudo kpartx -av
> asfd1asdf2asdf3asdf4asdf5asdf6asfd7asdf8asdf9asf10asdf11asdf12asdf13/disk.img
> add map loop0p1 (252:0): 0 409600 linear /dev/loop0 2048
> add map loop0p2 (252:1): 0 2 linear /dev/loop0 411648
> add map loop0p5 : 0 819200 linear /dev/loop0 413696
> add map loop0p6 : 0 819200 linear /dev/loop0 1234944
> add map loop0p7 : 0 819200 linear /dev/loop0 2056192
> add map loop0p8 : 0 1316864 linear /dev/loop0 2877440
>
> but dismounting fails:
> $ sudo kpartx -dv
> asfd1asdf2asdf3asdf4asdf5asdf6asfd7asdf8asdf9asf10asdf11asdf12asdf13/disk.img
>
> strace shows that the parameter on the dismount appears to get cut at 63
> chars:
> ioctl(3, DM_LIST_VERSIONS, 0x15b89b0) = 0
>
> stat("asfd1asdf2asdf3asdf4asdf5asdf6asfd7asdf8asdf9asf10asdf11asdf12asdf13/disk.img",
> {st_mode=S_IFREG|0644, st_size=2147483648, ...}) = 0
> stat("/dev/loop0", {st_mode=S_IFBLK|0660, st_rdev=makedev(7, 0), ...}) =
> 0
> open("/dev/loop0", O_RDONLY) = 4
> ioctl(4, LOOP_GET_STATUS, {number=0, offset=0, flags=0,
> name="asfd1asdf2asdf3asdf4asdf5asdf6asfd7asdf8asdf9asf10asdf11asdf12a",
> ...}) = 0
> close(4) = 0
> stat("/dev/loop1", {st_mode=S_IFBLK|0660, st_rdev=makedev(7, 1), ...}) =
> 0
> open("/dev/loop1", O_RDONLY) = 4
> ioctl(4, LOOP_GET_STATUS, {number=0, offset=0, flags=0,
> name="asfd1asdf2asdf3asdf4asdf5asdf6asfd7asdf8asdf9asf10asdf11asdf12a",
> ...}) = -1 ENXIO (No such device or address)
>
> if the path is 63 chars or les...

[Risto Kankkunen (risto-kankkunen-i)]

In addition to not handling long paths, kpartx also fails to handle relative paths correctly. Here's an example:

for d in 1 2
do
  mkdir $d && (
    cd $d &&
    dd if=/dev/zero of=disk.img seek=8k count=1 2> /dev/null &&
    (echo n;echo;echo;echo;echo;echo w) | fdisk disk.img >/dev/null &&
    kpartx -av disk.img
  )
done

Expected result:
add map loop1p1 (254:0): 0 6145 linear /dev/loop0 2048
add map loop2p1 (254:1): 0 6145 linear /dev/loop1 2048

Actual result:
add map loop1p1 (254:0): 0 6145 linear /dev/loop0 2048
add map loop1p1 (254:0): 0 6145 linear /dev/loop0 2048

Currently kpartx end up modifying the existing binding instead of creating a new one.

Both the long path problem and the relative path problem are caused by kpartx trying to use the binding name to store the image file path and assuming that the name would uniquely identify a binding.

The correct way is to use the device and inode numbers of the image files to identify which loop mount is attached to which image file. I have attached a patch which does exactly this.

[Simo Punnonen (simo-punnonen)]

Thanks Risto!

[Jorge Niedbalski (niedbalski)]

@simo-punnonen, @risto-kankkunen-i

Makes much more sense to use the device/inode for reference. Please could you guys
send this patch to upstream via the <email address hidden> mailing list?

Once the patch is there I can prepare an Ubuntu patch containing the fix.

Thanks.

[Risto Kankkunen (risto-kankkunen-i)]

I have sent the patch to the mailing list, no comments yet though.

https://www.redhat.com/archives/dm-devel/2015-July/msg00037.html

[Simo Punnonen (simo-punnonen)]

Looks like the upstream people don't seem to be very responsive to handle this case. Is there any way to kick it forward?
Also, could ubuntu still fix this properly while redhat sits on it?

[Mathieu Trudel-Lapierre (cyphermox)]

Unassigning Jorge, I will sponsor this fix.

[Simo Punnonen (simo-punnonen)]

Thank you Mathieu

[Launchpad Janitor (janitor)]

This bug was fixed in the package multipath-tools - 0.5.0-7ubuntu5

---------------
multipath-tools (0.5.0-7ubuntu5) wily; urgency=medium

  * debian/patches/0014-kpartx-long-path.patch: have kpartx match loopback
    files by device and inode rather than by path, as paths are not complete
    enough to do specific matching for long paths (> 64 chars) or relative
    paths. (LP: #1469143)

 -- Mathieu Trudel-Lapierre <email address hidden> Thu, 17 Sep 2015 11:10:38 -0400

[Brian Murray (brian-murray)]

Hello Simo, or anyone else affected,

Accepted multipath-tools into vivid-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/multipath-tools/0.4.9-3ubuntu12.15.04.2 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, and change the tag from verification-needed to verification-done. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed. In either case, details of your testing will help us make a better decision.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance!

[Ubuntu Foundations Team Bug Bot (crichton)]

The fix for this bug has been awaiting testing feedback in the -proposed repository for vivid for more than 90 days. Please test this fix and update the bug appropriately with the results. In the event that the fix for this bug is still not verified 15 days from now, the package will be removed from the -proposed repository.

[Simo Punnonen (simo-punnonen)]

Tested the above scenario on vivid, appears to work

[Simo Punnonen (simo-punnonen)]

Not sure how the process here works, is this enough or do i need to change some status somewhere?

[Martin Pitt (pitti)]

The version of multipath-tools in the proposed pocket of Vivid that was purported to fix this bug report has been removed because the bugs that were to be fixed by the upload were not verified in a timely (105 days) fashion.

[Simo Punnonen (simo-punnonen)]

I verified the bugs months ago, however i'm not familiar with how this process here works and nobody reacted to my question.

The package works, please integrate it.

[Simo Punnonen (simo-punnonen)]

I can't seem to be able to change the state of the bug myself to 'tested' in any way. Everything's grayed out.

[Simo Punnonen (simo-punnonen)]

How do i change tags?

[Simo Punnonen (simo-punnonen)]

Found it, this UI could really use some work