[Bug 1640823] Re: [trusty] mount -o loop is limited to 8 loop devices

[Robie Basak]

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
Foundations Bugs, 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