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

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

[Julio Auto]

Hi all,

this is my porting (to 2.6.x) of the loop.c issue reported and patched
by Solar Designer, to whom all credits of the original idea to the
patch go (more info in the original "[PATCH] loop.c: kernel_thread()
retval check" e-mail thread).

Honestly, I couldn't test it on other computers, but mine. But the
tests were made against a stock (unmodified) 2.6.17.9 kernel and the
patch works like it should. Nevertheless, a second thought/review is
always appreciated.

Cheers,

    Julio Auto

Signed-off-by: Julio Auto <mindvortex@gmail.com>

--- drivers/block/loop.c.orig	2006-08-23 11:44:51.000000000 -0700
+++ drivers/block/loop.c	2006-08-24 00:33:54.000000000 -0700
@@ -841,10 +841,20 @@ static int loop_set_fd(struct loop_devic

 	error = kernel_thread(loop_thread, lo, CLONE_KERNEL);
 	if (error < 0)
-		goto out_putf;
+		goto out_clr;
 	wait_for_completion(&lo->lo_done);
 	return 0;

+ out_clr:
+	lo->lo_device = NULL;
+	lo->lo_flags = 0;
+	lo->lo_backing_file = NULL;
+	set_capacity(disks[lo->lo_number], 0);
+	invalidate_bdev(bdev, 0);
+	bd_set_size(bdev, 0);
+	mapping_set_gfp_mask(mapping, lo->old_gfp_mask);
+	lo->lo_state = Lo_unbound;
+
  out_putf:
 	fput(file);
  out:

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

[Solar Designer]

On Thu, Aug 24, 2006 at 03:41:00AM -0300, Julio Auto wrote:
> this is my porting (to 2.6.x) of the loop.c issue reported and patched
> by Solar Designer, to whom all credits of the original idea to the
> patch go (more info in the original "[PATCH] loop.c: kernel_thread()
> retval check" e-mail thread).

The patch looks good to me, although I did not test it.

> Honestly, I couldn't test it on other computers, but mine. But the
> tests were made against a stock (unmodified) 2.6.17.9 kernel and the
> patch works like it should. Nevertheless, a second thought/review is
> always appreciated.

I think that testing this on a single machine is fine, but it is
preferable that you also check for any resource leaks.  That is, replace
the kernel_thread() call with -EAGAIN, then run losetup in a loop and
see whether the system possibly leaks a resource.  I did apply this sort
of testing to my original 2.4 patch.

> Signed-off-by: Julio Auto <mindvortex@gmail.com>

Acked-by: Solar Designer <solar@openwall.com>

> --- drivers/block/loop.c.orig	2006-08-23 11:44:51.000000000 -0700
> +++ drivers/block/loop.c	2006-08-24 00:33:54.000000000 -0700
> @@ -841,10 +841,20 @@ static int loop_set_fd(struct loop_devic
> 
> 	error = kernel_thread(loop_thread, lo, CLONE_KERNEL);
> 	if (error < 0)
> -		goto out_putf;
> +		goto out_clr;
> 	wait_for_completion(&lo->lo_done);
> 	return 0;
> 
> + out_clr:
> +	lo->lo_device = NULL;
> +	lo->lo_flags = 0;
> +	lo->lo_backing_file = NULL;
> +	set_capacity(disks[lo->lo_number], 0);
> +	invalidate_bdev(bdev, 0);
> +	bd_set_size(bdev, 0);
> +	mapping_set_gfp_mask(mapping, lo->old_gfp_mask);
> +	lo->lo_state = Lo_unbound;
> +
>  out_putf:
> 	fput(file);
>  out:

Thanks,

Alexander

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

[Andrew Morton]

On Mon, 28 Aug 2006 07:55:56 +0400
Solar Designer <solar@openwall.com> wrote:

> On Thu, Aug 24, 2006 at 03:41:00AM -0300, Julio Auto wrote:
> > this is my porting (to 2.6.x) of the loop.c issue reported and patched
> > by Solar Designer, to whom all credits of the original idea to the
> > patch go (more info in the original "[PATCH] loop.c: kernel_thread()
> > retval check" e-mail thread).
> 
> The patch looks good to me, although I did not test it.
> 
> > Honestly, I couldn't test it on other computers, but mine. But the
> > tests were made against a stock (unmodified) 2.6.17.9 kernel and the
> > patch works like it should. Nevertheless, a second thought/review is
> > always appreciated.
> 
> I think that testing this on a single machine is fine, but it is
> preferable that you also check for any resource leaks.  That is, replace
> the kernel_thread() call with -EAGAIN, then run losetup in a loop and
> see whether the system possibly leaks a resource.  I did apply this sort
> of testing to my original 2.4 patch.
> 
> > Signed-off-by: Julio Auto <mindvortex@gmail.com>
> 
> Acked-by: Solar Designer <solar@openwall.com>
> 
> > --- drivers/block/loop.c.orig	2006-08-23 11:44:51.000000000 -0700
> > +++ drivers/block/loop.c	2006-08-24 00:33:54.000000000 -0700
> > @@ -841,10 +841,20 @@ static int loop_set_fd(struct loop_devic
> > 
> > 	error = kernel_thread(loop_thread, lo, CLONE_KERNEL);
> > 	if (error < 0)
> > -		goto out_putf;
> > +		goto out_clr;
> > 	wait_for_completion(&lo->lo_done);
> > 	return 0;
> > 
> > + out_clr:
> > +	lo->lo_device = NULL;
> > +	lo->lo_flags = 0;
> > +	lo->lo_backing_file = NULL;
> > +	set_capacity(disks[lo->lo_number], 0);
> > +	invalidate_bdev(bdev, 0);
> > +	bd_set_size(bdev, 0);
> > +	mapping_set_gfp_mask(mapping, lo->old_gfp_mask);
> > +	lo->lo_state = Lo_unbound;
> > +
> >  out_putf:
> > 	fput(file);
> >  out:
> 

The plan is to stop using the deprecated kernel_thread() in loop altogether.

Please review

ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.18-rc4/2.6.18-rc4-mm3/broken-out/kthread-convert-loopc-to-kthread.patch

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

[Julio Auto]

On 8/28/06, Andrew Morton <akpm@osdl.org> wrote:
> The plan is to stop using the deprecated kernel_thread() in loop altogether.
>
> Please review
>
> >ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.18-rc4/2.6.18-rc4-mm3/broken-out/kthread-convert-loopc-to-kthread.patch
>

Well, the reason behind this patch is just adding a little bit of
cleanup code (ie., cleaning the 'lo' object) to the case when the
kernel thread creation fails, regardless of what implementation
loop_set_fd() uses to accomplish this.
In fact, I think the concept it's still usable after applying the
patch you mentioned, since after changing kernel_thread() for
kthread_create(), the only cleanup code added is setting lo->lo_thread
to NULL.

Cheers,

    Julio Auto

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

[Serge E. Hallyn]

Quoting Julio Auto (mindvortex@gmail.com):
> On 8/28/06, Andrew Morton <akpm@osdl.org> wrote:
> >The plan is to stop using the deprecated kernel_thread() in loop 
> >altogether.
> >
> >Please review
> >
> >>ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.18-rc4/2.6.18-rc4-mm3/broken-out/kthread-convert-loopc-to-kthread.patch
> >
> 
> Well, the reason behind this patch is just adding a little bit of
> cleanup code (ie., cleaning the 'lo' object) to the case when the
> kernel thread creation fails, regardless of what implementation
> loop_set_fd() uses to accomplish this.
> In fact, I think the concept it's still usable after applying the
> patch you mentioned, since after changing kernel_thread() for
> kthread_create(), the only cleanup code added is setting lo->lo_thread
> to NULL.
> 
> Cheers,
> 
>    Julio Auto

The attached patch does the same resource checks in the kthread version
from 2.6.18-rc4-mm2.

thanks,
-serge

Subject: [PATCH] loop: forward-port resource leak checks from Solar

Forward port of the patch by Solar and ported by Julio, ported
to the -mm tree.

Compiles, boots, and passes my looptorturetest.sh.

Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>

---

 drivers/block/loop.c |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

1a32f47ff67796d8ee8d088d3ba6f54e834e6004
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index ced8a78..c2e1862 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -826,13 +826,22 @@ static int loop_set_fd(struct loop_devic
 						lo->lo_number);
 	if (IS_ERR(lo->lo_thread)) {
 		error = PTR_ERR(lo->lo_thread);
-		lo->lo_thread = NULL;
-		goto out_putf;
+		goto out_clr;
 	}
 	lo->lo_state = Lo_bound;
 	wake_up_process(lo->lo_thread);
 	return 0;
 
+out_clr:
+	lo->lo_thread = NULL;
+	lo->lo_device = NULL;
+	lo->lo_backing_file = NULL;
+	lo->lo_flags = 0;
+	set_capacity(disks[lo->lo_number], 0);
+	invalidate_bdev(bdev, 0);
+	bd_set_size(bdev, 0);
+	mapping_set_gfp_mask(mapping, lo->old_gfp_mask);
+	lo->lo_state = Lo_unbound;
  out_putf:
 	fput(file);
  out:
-- 
1.1.6

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

[Julio Auto]

On 8/28/06, Solar Designer <solar@openwall.com> wrote:
> I think that testing this on a single machine is fine, but it is
> preferable that you also check for any resource leaks.  That is, replace
> the kernel_thread() call with -EAGAIN, then run losetup in a loop and
> see whether the system possibly leaks a resource.  I did apply this sort
> of testing to my original 2.4 patch.
>

That was exactly the approach I took. :)


    Julio Auto