This felt unusually risk to me, so I took a look at the patch before
releasing. I found a number of problems:
1. This patch leaks a file descriptor every time the new code path runs.
loop_ctl_fd is opened but not closed.
2. "int devnr = -1" is defined but never used.
3.
> ll->ncur = ioctl(loop_ctl_fd, LOOP_CTL_GET_FREE);
This is wrong. ncur is an integer iterator, not necessarily a loop
number. It keeps state between multiple calls to looplist_next, and
you're clobbering it. It may be the case that it happens not to get used
again when LLFLG_LOOPCTL is set, but that's no reason to re-use an
existing variable for a different purpose. This makes the code much more
difficult to check for unexpected side effects and thus may hide bugs.
4. looplist_next() is called to acquire both free and used loop device
fds (based on LLFLG_USEDONLY/LLFLG_FREEONLY; see looplist_open_dev()).
The fashion in which it does this is quite convulated. Additionally, it
appears designed to be called multiple times (like a C++ iterator). The
new code will always return a new loop device when requested. This is of
course the point of this SRU. However, a consequence of this is that
calls to looplist_next with LLFLG_FREEONLY are now unbounded, and this
may have implications throughout the code. I can't find any, but this is
certainly Regression Potential territory, and IMHO all known use cases
that call looplist_next() should be checked for correct behaviour in SRU
verification.
5. The "mount" and "umount" commands also call functions defined by
lomount.c, so loop-affecting behaviour should be checked in these, too.
It feels like luck to me that this code didn't introduce a bug that I
could find. However, we want to minimise regression risk in SRUs. Even
though I couldn't find a specific bug, issues 1 and 3 above
unnecessarily add to the risk of regression in this SRU, IMHO, and are
easy to mitigate. So I'm marking this bug verification-failed. You might
as well fix issue 2 while you're there. For issues 4 and 5, I'll update
the test plan in the bug description.
** Tags removed: verification-done
** Tags added: verification-failed
** Description changed:
trusty has a very old util-linux which does not yet know about /dev
/loop-control to create arbitrarily many loop devices. This feature was
introduced in Linux 3.1 already (i. e. before precise even). This is a
showstopper for backporting snappy as that needs a lot of loop mounts.
Support for loop-control got introduced later util-linux versions, but
backporting full support for it (for losetup) is too intrusive. We only
need a partial backport for "mount -o loop".
SRU TEST CASE:
First, use up all default 8 loop devices:
$ for i in `seq 8`; do echo $i; sudo losetup --find /etc/issue; done
Now try to use a 9th:
$ dd if=/dev/zero of=/tmp/img bs=1M count=50
$ mkfs.ext2 -F /tmp/img
$ sudo mount -o loop /tmp/img /mnt
With current trusty's "mount" package this will fail with "could not
find any free loop device". With the proposed version, this should
succeed, and "sudo losetup -a" should show "/dev/loop8: ... (/tmp/img)".
Now, reboot, disable loop-control with
sudo mv /dev/loop-control{,.disabled}
and run the test case again. Now "mount -o loop" should fail with "could
not find any free loop device" (as before). Ensure that there are no
hangs, infinite loops, etc.
+ ADDITIONAL REGRESSION CHECKING TEST CASES
+
+ 1. Check that every type of losetup call documented in the losetup
+ manpage still works correctly.
+
+ 2. Check that mount and umount commands that use loop devices still work
+ correctly.
+
REGRESSION POTENTIAL: /dev/loop-control and the corresponding util-linux
support has exited for a long time without known/major issues, so this
should be fairly safe. Also, the patch falls back to the previous
"iterate over loop0 to loop7" behaviour if loop-control is not
available.
--
You received this bug notification because you are a member of Ubuntu
Touch seeded packages, which is subscribed to util-linux in Ubuntu.
https://bugs.launchpad.net/bugs/1640823
Title:
[trusty] mount -o loop is limited to 8 loop devices
Status in util-linux package in Ubuntu:
Fix Released
Status in util-linux source package in Trusty:
Fix Committed
Bug description:
trusty has a very old util-linux which does not yet know about /dev
/loop-control to create arbitrarily many loop devices. This feature
was introduced in Linux 3.1 already (i. e. before precise even). This
is a showstopper for backporting snappy as that needs a lot of loop
mounts.
Support for loop-control got introduced later util-linux versions, but
backporting full support for it (for losetup) is too intrusive. We
only need a partial backport for "mount -o loop".
SRU TEST CASE:
First, use up all default 8 loop devices:
$ for i in `seq 8`; do echo $i; sudo losetup --find /etc/issue; done
Now try to use a 9th:
$ dd if=/dev/zero of=/tmp/img bs=1M count=50
$ mkfs.ext2 -F /tmp/img
$ sudo mount -o loop /tmp/img /mnt
With current trusty's "mount" package this will fail with "could not
find any free loop device". With the proposed version, this should
succeed, and "sudo losetup -a" should show "/dev/loop8: ...
(/tmp/img)".
Now, reboot, disable loop-control with
sudo mv /dev/loop-control{,.disabled}
and run the test case again. Now "mount -o loop" should fail with
"could not find any free loop device" (as before). Ensure that there
are no hangs, infinite loops, etc.
ADDITIONAL REGRESSION CHECKING TEST CASES
1. Check that every type of losetup call documented in the losetup
manpage still works correctly.
2. Check that mount and umount commands that use loop devices still
work correctly.
REGRESSION POTENTIAL: /dev/loop-control and the corresponding util-
linux support has exited for a long time without known/major issues,
so this should be fairly safe. Also, the patch falls back to the
previous "iterate over loop0 to loop7" behaviour if loop-control is
not available.
To manage notifications about this bug go to:
https://bugs.launchpad.net/ubuntu/+source/util-linux/+bug/1640823/+subscriptions
--
Mailing list: https://launchpad.net/~touch-packages
Post to : touch-packages@lists.launchpad.net
Unsubscribe : https://launchpad.net/~touch-packages
More help : https://help.launchpad.net/ListHelp
