[PATCH 4/5] UML - Close file descriptor leaks

[PATCH 4/5] UML - Close file descriptor leaks

[Jeff Dike]

Close two file descriptor leaks, one in the ubd driver and one to
/proc/mounts.  The ubd driver bug also leaked some vmalloc space.
The /proc/mounts leak was a descriptor that was just never closed.

Signed-off-by: Jeff Dike <jdike@addtoit.com>
---

 arch/um/drivers/ubd_kern.c |    9 ++-------
 arch/um/os-Linux/mem.c     |    6 +++++-
 2 files changed, 7 insertions(+), 8 deletions(-)

Index: linux-2.6.18-mm/arch/um/drivers/ubd_kern.c
===================================================================
--- linux-2.6.18-mm.orig/arch/um/drivers/ubd_kern.c	2006-09-26 16:28:58.000000000 -0400
+++ linux-2.6.18-mm/arch/um/drivers/ubd_kern.c	2006-09-26 16:31:24.000000000 -0400
@@ -667,18 +667,15 @@ static int ubd_add(int n)
 	if(dev->file == NULL)
 		goto out;
 
-	if (ubd_open_dev(dev))
-		goto out;
-
 	err = ubd_file_size(dev, &dev->size);
 	if(err < 0)
-		goto out_close;
+		goto out;
 
 	dev->size = ROUND_BLOCK(dev->size);
 
 	err = ubd_new_disk(MAJOR_NR, dev->size, n, &ubd_gendisk[n]);
 	if(err)
-		goto out_close;
+		goto out;
 
 	if(fake_major != MAJOR_NR)
 		ubd_new_disk(fake_major, dev->size, n,
@@ -690,8 +687,6 @@ static int ubd_add(int n)
 		make_ide_entries(ubd_gendisk[n]->disk_name);
 
 	err = 0;
-out_close:
-	ubd_close(dev);
 out:
 	return err;
 }
Index: linux-2.6.18-mm/arch/um/os-Linux/mem.c
===================================================================
--- linux-2.6.18-mm.orig/arch/um/os-Linux/mem.c	2006-09-26 16:28:50.000000000 -0400
+++ linux-2.6.18-mm/arch/um/os-Linux/mem.c	2006-09-26 16:31:24.000000000 -0400
@@ -132,6 +132,9 @@ err:
 	else if(found < 0)
 		printf("read returned errno %d\n", -found);
 
+out:
+	close(fd);
+
 	return;
 
 found:
@@ -141,11 +144,12 @@ found:
 
 	if(strncmp(buf, "tmpfs", strlen("tmpfs"))){
 		printf("not tmpfs\n");
-		return;
+		goto out;
 	}
 
 	printf("OK\n");
 	default_tmpdir = "/dev/shm";
+	goto out;
 }
 
 /*

Re: [uml-devel] [PATCH 4/5] UML - Close file descriptor leaks

[Blaisorblade]

On Wednesday 27 September 2006 19:57, Jeff Dike wrote:
> Close two file descriptor leaks, one in the ubd driver and one to
> /proc/mounts.  The ubd driver bug also leaked some vmalloc space.
> The /proc/mounts leak was a descriptor that was just never closed.
For AKPM:
NACK on the ubd driver part. It adds a bugs and does fix the one you found in 
the right point. ACK on the other hunk.

Now, Andrew, you can ignore what follows if you want:

Jeff, please explain me the exact ubd driver bug - I believe that the open 
must be done there, that it is balanced by ubd_close(), and that the leak fix 
should be done differently. I've done huge changes to the UBD driver and I'll 
send them you briefly for your tree (they work but they're not yet in a 
perfect shape).

For what I can gather from your description and code, the leak you diagnosed 
is a bug in ubd_open_dev(), and is valid for any call to it: generally, if an 
_open function fails it should leave nothing to cleanup, and in particular 
the corresponding _close should not be called (this is the implicit standard 
I've seen in Linux). However, I did not note the ubd_open_dev() leak, and I'm 
fixing it now.

Btw, ubd_open_dev() and ubd_close() are matching functions, while ubd_close() 
does not match ubd_open(), so I renamed ubd_close -> ubd_close_dev.

About the UBD changes, they're mainly about making it SMP-ready. I've done 
also a number of other changes (for instance I removed the allocation in 
fork+execvp() by reimplementing the latter, and fixed the usage of sigio 
spinlocks), so the only big remaining bug I recall is that writes to consoles 
should be lock-free.

When there are sleep-inside-spinlock warnings inside line_open(), for 
instance, the resulting printk hangs because the line spinlock is already 
held (this is, in particular, due to um_request_irq in 
write_sigio_workaround(), and I fixed it).
However, console write driver methods must be lock-free (specified in 
Documentation/tty.txt); I fixed the warnings but I wanted to fix the hang 
caused by them.

> Signed-off-by: Jeff Dike <jdike@addtoit.com>
> ---
>
>  arch/um/drivers/ubd_kern.c |    9 ++-------
>  arch/um/os-Linux/mem.c     |    6 +++++-
>  2 files changed, 7 insertions(+), 8 deletions(-)
>
> Index: linux-2.6.18-mm/arch/um/drivers/ubd_kern.c
> ===================================================================
> --- linux-2.6.18-mm.orig/arch/um/drivers/ubd_kern.c	2006-09-26
> 16:28:58.000000000 -0400 +++
> linux-2.6.18-mm/arch/um/drivers/ubd_kern.c	2006-09-26 16:31:24.000000000
> -0400 @@ -667,18 +667,15 @@ static int ubd_add(int n)
>  	if(dev->file == NULL)
>  		goto out;
>
> -	if (ubd_open_dev(dev))
> -		goto out;
> -
>  	err = ubd_file_size(dev, &dev->size);
>  	if(err < 0)
> -		goto out_close;
> +		goto out;
>
>  	dev->size = ROUND_BLOCK(dev->size);
>
>  	err = ubd_new_disk(MAJOR_NR, dev->size, n, &ubd_gendisk[n]);
>  	if(err)
> -		goto out_close;
> +		goto out;
>
>  	if(fake_major != MAJOR_NR)
>  		ubd_new_disk(fake_major, dev->size, n,
> @@ -690,8 +687,6 @@ static int ubd_add(int n)
>  		make_ide_entries(ubd_gendisk[n]->disk_name);
>
>  	err = 0;
> -out_close:
> -	ubd_close(dev);
>  out:
>  	return err;
>  }

-- 
Inform me of my mistakes, so I can keep imitating Homer Simpson's "Doh!".
Paolo Giarrusso, aka Blaisorblade
http://www.user-mode-linux.org/~blaisorblade
Chiacchiera con i tuoi amici in tempo reale! 
 http://it.yahoo.com/mail_it/foot/*http://it.messenger.yahoo.com 

Re: [uml-devel] [PATCH 4/5] UML - Close file descriptor leaks

[Jeff Dike]

On Sun, Oct 01, 2006 at 07:10:37PM +0200, Blaisorblade wrote:
> NACK on the ubd driver part. It adds a bugs and does fix the one you found in
> the right point. ACK on the other hunk.
> 
> Now, Andrew, you can ignore what follows if you want:
> 
> Jeff, please explain me the exact ubd driver bug - I believe that the open 
> must be done there, that it is balanced by ubd_close(), and that the leak fix
> should be done differently. 

The code as it was was just wrong.  There was a call to ubd_open_dev
at the start and a matching ubd_close at the end.  So far, so good,
since ubd_close inverts ubd_open_dev.  However, neither manipulates
dev->count (this is done by ubd_open/ubd_release) and the call to
ubd_add_disk indirectly calls ubd_open, which, since dev->count is
still zero redoes the initialization, leaking the descriptor and
vmalloc space allocated by the first ubd_open_dev call.

The fix is simple - there is no need for ubd_add to call ubd_open_dev
or ubd_close (ubd_file_size doesn't require the device be opened), so
the calls can simply be deleted.  With the non-count-changing
device-opening call gone, the leaks just disappear.

There are multiple things wrong with the code, many of which are still
there, but I don't see this as being an argument against this change.
It eliminates some useless code, which is good from both a maintenance
and performance standpoint.  However, leaks aside, the main benefit of
this change is that eliminates the one call to ubd_open_dev outside of
ubd_open, and thus opening stuff on the host and allocating memory is
always inside a check of dev->open.

> I've done huge changes to the UBD driver and I'll
> send them you briefly for your tree (they work but they're not yet in a 
> perfect shape).

OK, just make sure you preserve (or add) this property.

> For what I can gather from your description and code, the leak you diagnosed 
> is a bug in ubd_open_dev(), and is valid for any call to it: 

No, the bug is doing the work of opening the device outside a check of
the device refcount.

> generally, if an
> _open function fails it should leave nothing to cleanup, and in particular 
> the corresponding _close should not be called (this is the implicit standard 
> I've seen in Linux). 

This is true - however the _open function was succeeding, so it's
irrelevant in this case.

> Btw, ubd_open_dev() and ubd_close() are matching functions, while ubd_close()
> does not match ubd_open(), so I renamed ubd_close -> ubd_close_dev.

Yes, this is one of the things wrong with the current code - the names
are messed up.

				Jeff

Re: [uml-devel] [PATCH 4/5] UML - Close file descriptor leaks

[Paolo Giarrusso]

Jeff Dike <jdike@addtoit.com> ha scritto: 

> On Sun, Oct 01, 2006 at 07:10:37PM +0200, Blaisorblade wrote:
> > NACK on the ubd driver part. It adds a bugs and does fix the one
> you found in
> > the right point. ACK on the other hunk.

I'm answering away from the code (limited Internet access) so I'll
have to look better some things. The patch has been merged, I'll care
for that in any case (avoiding conflicts with my changes as needed).

> However, neither manipulates
> dev->count (this is done by ubd_open/ubd_release) and the call to
> ubd_add_disk indirectly calls ubd_open, which, since dev->count is
> still zero redoes the initialization, leaking the descriptor and
> vmalloc space allocated by the first ubd_open_dev call.

Ah, ok. I fixed exactly this - now ubd_open_dev and ubd_close_dev are
called only through ubd_{get,put}_dev, which manage the refcount. I
did this also for other reasons - the refcount makes "close while it
is being used" impossible, so solves "mutual exclusion without
locking" (like the network layer state machine does for network
devices, this is used in general for fds; I'll write something about
this).

> The fix is simple - there is no need for ubd_add to call
> ubd_open_dev
> or ubd_close (ubd_file_size doesn't require the device be opened),

I know ubd_file_size doesn't require opening, I thought (and still
think) that call was for error checking. I'll have to re-read the
code now that I saw your point.
> so
> the calls can simply be deleted.  With the non-count-changing
> device-opening call gone, the leaks just disappear.

> There are multiple things wrong with the code, many of which are
> still
> there, but I don't see this as being an argument against this
> change.
> It eliminates some useless code, which is good from both a
> maintenance
> and performance standpoint.  However, leaks aside, the main benefit
> of
> this change is that eliminates the one call to ubd_open_dev outside
> of
> ubd_open, and thus opening stuff on the host and allocating memory
> is
> always inside a check of dev->open.

Well, this may simplify locking, so it may be interesting - I'll see.

> > I've done huge changes to the UBD driver and I'll
> > send them you briefly for your tree (they work but they're not
> yet in a 
> > perfect shape).

> OK, just make sure you preserve (or add) this property.

> > For what I can gather from your description and code, the leak
> you diagnosed 
> > is a bug in ubd_open_dev(), and is valid for any call to it: 

> No, the bug is doing the work of opening the device outside a check
> of
> the device refcount.

Ok, but the one I saw exists (in the error: label), (but I have the
patch for it).

__________________________________________________
Do You Yahoo!?
Poco spazio e tanto spam? Yahoo! Mail ti protegge dallo spam e ti da tanto spazio gratuito per i tuoi file e i messaggi 
http://mail.yahoo.it