Messages in this thread | | | Subject | Re: [PATCH] UBI: dereference after kfree in create_vtbl | From | Artem Bityutskiy <> | Date | Sat, 05 May 2007 16:18:23 +0300 |
| |
On Sat, 2007-05-05 at 17:56 +0530, Satyam Sharma wrote: > > And it is fine to use list_add_tail() directly in vtbl.c. Will be fixed. > Ah, good to know that, but please keep list_add_tail (or whatever is > the implementation abstraction of the various ubi_scan_info lists) > local to scan.c -- you could expose a version of ubi_scan_add_to_list > that does not do kmalloc through scan.h and use that in vtbl.c. That > way you won't lose those debug printk's when adding an eraseblock to a > list, for example, and it's always much cleaner not exposing an > object's implementation innards to others.
It's error path and that print is not really needed. We'll see other complaints in that case. And this is _the only_ place outside scan.c, so it makes sense to use list_add_tail(). We do not really need to hide this behind some other call (ubi_scan_add_to_list())
> Physical eraseblocks are allocated in ubi_scan_add_to_list > (which shouldn't be doing that) Yes, per-PEB scanning information is allocated in ubi_scan_add_to_list() and ubi_scan_add_to_used(). I do not see why it shouldn't be doing that.
> and ubi_scan_add_used (which is a maze) It actually is rather complex because it does a rather complex thing. But any patch/idea to make it simpler is welcome.
> and freed pretty much all over the place It is only freed in ubi_scan_destroy_si(). Yes, there is one exception in create_vtbl, but this is because I did not want to introduce any special version of ubi_scan_add_used().
It does not hurt at all that we do one extra allocation, because it is called _only_ 2 times (once for each volume table copy).
> (because we allocate > new seb's for ourselves to add to the lists, we need to go about > kfree'ing all of them when destroying a ubi_scan_destroy_si too, for > example -- perhaps this driver needs to be told about krefs).
Sorry. not sure what you mean. They are allocated in 2 places, and freed in one, with one exception in vtbl_create() which does not matter much.
> So it > makes life easier if you know there's only one place when/where an > object is born, May be it is, but I have 2 places and do not see any problem.
> if you know that it'll remain alive as long as you > need it and have a reference on it, and if you destroy it a known > single time/location too. I have 1 destroy location. And one exception where I allocate it temporarily and destroy in the same function. And it is done only 2 times and only if one attaches un-formatted flash.
> I wish I could be more specific than this, > but I've only spent a few hours with ubi :-) Thanks for your analysis, it would be helpful if more people did this.
-- Best regards, Artem Bityutskiy (Битюцкий Артём)
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |