loop.c: kernel_thread() retval check

Willy,

I propose the attached patch (extracted from 2.4.33-ow1) for inclusion into 2.4.34-pre. (Last time I checked, 2.6 needed an equivalent fix, but I haven't produced one yet.)

Basically, the code in drivers/block/loop.c did not check the return value from kernel_thread(). If kernel_thread() would fail, the code would misbehave (IIRC, the invoking process would become unkillable).

An easy way to trigger the bug was to run losetup under strace (as root), and this is also how I tested the error path added with this patch.

This change has been a part of publicly released -ow patches for 8+ months.

There are more instances of kernel_thread() calls that do not check the return value; some of the remaining ones might need to be fixed, too.

Thanks,

Alexander

diff -urpPX nopatch linux-2.4.33/drivers/block/loop.c linux/drivers/block/loop.c
@@ -693,12 +693,23 @@ static int loop_set_fd(struct loop_devic
 	set_blocksize(dev, bs);
 
 	lo->lo_bh = lo->lo_bhtail = NULL;
-	kernel_thread(loop_thread, lo, CLONE_FS | CLONE_FILES | CLONE_SIGHAND);
-	down(&lo->lo_sem);
+	error = kernel_thread(loop_thread, lo,
+	    CLONE_FS | CLONE_FILES | CLONE_SIGHAND);
+	if (error < 0)
+		goto out_clr;
+	down(&lo->lo_sem); /* wait for the thread to start */
 
 	fput(file);
 	return 0;
 
+ out_clr:
+	lo->lo_backing_file = NULL;
+	lo->lo_device = 0;
+	lo->lo_flags = 0;
+	loop_sizes[lo->lo_number] = 0;
+	inode->i_mapping->gfp_mask = lo->old_gfp_mask;
+	lo->lo_state = Lo_unbound;
+	fput(file); /* yes, have to do it twice */
  out_putf:
 	fput(file);
  out:

Hi Alexander,

That's amazing it never got fixed before !
I still remembered this problem being discussed, and finally found the thread :

http://lkml.org/lkml/2003/11/14/55

In fact, no code was proposed and 2.6 got fixed later, then stopped using kernel_thread() so nearly nobody might have noticed it :

http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff_plain;h=3e88c17d404c5787afd5bd1763380317f5ccbf84;hp=22e6c1b39c648850438decd491f62d311800c7db

I've read somewhere that the module loading code might be affected too.

Thanks for this patch, I know this problem caught me at least one !

Cheers,
Willy

I was not aware that this had been discussed before. Bernhard (in the old LKML posting above) seems to imply that having kernel_thread() itself not fail on ptrace would be a sufficient fix, which I don't agree with. There may be other reasons for kernel_thread() to fail, such as the kernel running out of resources; with OpenVZ, kernel_thread() is not allowed from within VEs.

I'm afraid that this does not properly clean things up on error. I just had a look at linux-2.6.17.9/drivers/block/loop.c - it still uses kernel_thread() and has the same "goto out_putf" on error return from kernel_thread(), which appears to not clean things up.

Alexander

Ar Sul, 2006-08-20 am 03:46 +0400, ysgrifennodd Solar Designer:

but should go into 2.6-mm and be tested in 2.6-mm before 2.4 backport.

Alan,

Thanks.

Unfortunately, I do not intend to prepare/test/submit a revision of the patch for 2.6-mm, and I might only work on a 2.6 patch in a few months from now. Unless someone else takes care of this, the fix is likely to get dropped on the floor.

Do you really think that getting this fix into 2.4 should be delayed like that? Please note that the patch affects error path only and that without the patch things misbehave badly whenever the error occurs. I really don't think the patch can make things worse, even if it were buggy (although it's been tested with the error path triggered specifically on 2.4).

Alexander

Ar Llu, 2006-08-21 am 02:34 +0400, ysgrifennodd Solar Designer:

I have no problem with it going into 2.4. Someone should forward port it
- but there should be plenty of people on the lists to do that.

That's so true ! We're relying too much on our own time while many people might be waiting for some little work to start taking a look at the kernel internals !

I think I will be starting to ask for forward porters for the fixed to 2.4 that need to be ported to 2.6 too.

Cheers,
Willy

Well, actually I'd be glad to help. In fact, with this particular 2.4 patch at hand, fixing 2.6 seems incredbly straight-forward (or am I getting ahead of myself?)
However, I wasn't able to reproduce the bug in my system just by running losetup under strace. Maybe 2.6.15-1.2054_FC5 has it patched? If if no one can help me out with this I'll boot into a stock kernel later and try to trigger the bug again.

Cheers,

Julio Auto

You need to make sure that the cleanup code added with the patch matches the loop device initialization preceding the kernel_thread() call. You should not blindly take the cleanup code out of the 2.4 patch and apply it to 2.6 - it might not be correct for 2.6.

No. But you won't be able to reproduce this with strace on 2.6 since 2.6's kernel_thread() uses CLONE_UNTRACED instead of failing on ptrace. You'll probably need to temporarily replace the kernel_thread() call in loop.c with -EAGAIN to comfortably test your cleanup code without forcing the system to run out of resources.

Alexander

Yes, I already had that in mind, but thanks for the worry, anyway.

Thanks for the tip. I'll see what I can do. :)

Cheers,

Julio Auto