Re: [PATCH] loop.c: kernel_thread() retval check

Willy Tarreau
2006-08-20T07:32:20Z

Hi Alexander,

On Sun, Aug 20, 2006 at 03:46:29AM +0400, Solar Designer wrote:
> 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.

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

> 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.

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

> Thanks,
>
> Alexander

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

Cheers,
Willy

> diff -urpPX nopatch linux-2.4.33/drivers/block/loop.c linux/drivers/block/loop.c
> --- linux-2.4.33/drivers/block/loop.c Fri Jun 3 04:26:42 2005
> +++ linux/drivers/block/loop.c Sat Aug 12 08:51:47 2006
> @@ -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);