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: <20061001220727.GA5194@ccure.user-mode-linux.org>
Date:	Sun, 1 Oct 2006 18:07:27 -0400
From:	Jeff Dike <jdike@...toit.com>
To:	Blaisorblade <blaisorblade@...oo.it>
Cc:	user-mode-linux-devel@...ts.sourceforge.net, akpm@...l.org,
	linux-kernel@...r.kernel.org
Subject: Re: [uml-devel] [PATCH 4/5] UML - Close file descriptor leaks

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