[Toybox] [PATCH] losetup: fix the race.

[Rob Landley]

On 8/5/19 8:43 PM, enh via Toybox wrote:
> ah, i haven't reproduced that one.
> 
> this patch fixes an issue that was easily reproduced with:

First time I looked at this I got distracted by the unrelated:

-      if (0 <= (i = ioctl(cfd, 0x4C82))) { // LOOP_CTL_GET_FREE
+      if (0 <= (i = ioctl(cfd, LOOP_CTL_GET_FREE))) {

And went "Has that aged out of the 7 year rule now? 7 years ago would be Ubuntu
12.04, which was end of lifed in April of this year, so I should dig up my big
brick of a machine that still has 14.04 installed on it (due to inertia and it
being on a shelf at the moment, except I pulled it down to make a Devuan install
USB stick when I got home with the new SSD), and
https://wiki.ubuntu.com/Releases says 14.04 is still supported through April
2022 so if that needs this maybe it should stay?

So I needed to go run a test, and didn't get around to it before the disk
crashed. And that prevented me from looking to closely at the rest of it until
now...

>     while true; do losetup -sf /etc/passwd & losetup -sf /etc/passwd ; done

The problem is, what if the two losetups are trying to associate to different
files? Your patch returns "success" when we got an errno saying "it didn't
work", and your early return means it never called the SET_STATUS either...
meaning you returned success for a failure.

The thing is, if you replace the & with a && (or just do it twice) you get:

$ sudo ./toybox losetup -sf /etc/passwd
/dev/loop1
x$ sudo ./toybox losetup -sf /etc/passwd
/dev/loop2

So it's not /etc/passwd we're clashing on, it's two calls to /dev/loop-control
returning the same loop device and one of them claiming it, and the other
getting -EBUSY. And THAT means what we need to do is detect EBUSY and loop back
around to loop-control and request another one from that, because ANYBODY on the
system could do this, it's a shared resource with a global namespace.

This patch is not hte right fix. I'll try to come up with something to address
both issues but I _just_ got back to poking at toysh having left off rather
abruptly in mid-thought because "grep -r symbolname /usr/include" came back with
an I/O error in one of the files and I went "Bad Thing! Tangent Now!" and it ate
my week...

> which fails about half the iterations for me without this patch, and zero with.

Yay bug report. Yay adding test. Wrong fix.

And the test should probably loop about ~30 times with 3 asynchronous
backgrounded processes associating and dissocating from a loop device with a
"cat" in between. Modulo that whole block size thing. Hmmm, a 32k file and an
sha1sum thereof is probably a reliable test...

Something vaguely like:

for j in 1 2 3 4 5; do
dd if=/dev/urandom of=test$j
done
for j in 1 2 3 4 5 6 ; do for i in 1 2 3 4 5; do
X=$(losetup -sf test$i) && cmp <(sha1sum<$X) <(sha1sum<test$i) && echo -n y
losetup -d $X
done 2>/dev/null; wait; done
rm test?

And that should check for a string of 30 y's.

Rob