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
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/1640823
Title:
[trusty] mount -o loop is limited to 8 loop devices
To manage notifications about this bug go to:
https://bugs.launchpad.net/ubuntu/+source/util-linux/+bug/1640823/+subscriptions
--
ubuntu-bugs mailing list
[email protected]
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
