linux-kernel - Re: [uml-devel] [PATCH 4/5] UML

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <200610011910.37805.blaisorblade@yahoo.it>
Date:	Sun, 1 Oct 2006 19:10:37 +0200
From:	Blaisorblade <blaisorblade@...oo.it>
To:	user-mode-linux-devel@...ts.sourceforge.net
Cc:	Jeff Dike <jdike@...toit.com>, akpm@...l.org,
	linux-kernel@...r.kernel.org
Subject: Re: [uml-devel] [PATCH 4/5] UML - Close file descriptor leaks

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@...toit.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 
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/