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

[Paolo Giarrusso]

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