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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
