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 <[email protected]>
--- 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:
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 <[email protected]>
Acked-by: Solar Designer <[email protected]>
> --- 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
On Mon, 28 Aug 2006 07:55:56 +0400
Solar Designer <[email protected]> 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 <[email protected]>
>
> Acked-by: Solar Designer <[email protected]>
>
> > --- 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
On 8/28/06, Solar Designer <[email protected]> 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
On 8/28/06, Andrew Morton <[email protected]> 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
Quoting Julio Auto ([email protected]):
> On 8/28/06, Andrew Morton <[email protected]> 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 <[email protected]>
---
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