Messages in this thread | | | Date | Sat, 5 May 2007 17:56:50 +0530 | From | "Satyam Sharma" <> | Subject | Re: [PATCH] UBI: dereference after kfree in create_vtbl |
| |
On 5/5/07, Artem Bityutskiy <dedekind@infradead.org> wrote: > [...] > On Sat, 2007-05-05 at 09:25 +0530, Satyam Sharma wrote: > > Artem would have to step in here to verify if there really is a good > > reason why we kmalloc a fresh ubi_scan_leb every time we want to add > > one to a list. > Particularly in vtbl.c there is no good reason. Leftover of itsy-bitsy > units. I'll make ubi_scan_add_to_list static, as well as > ubi_scan_add_used(). And I'll rename them to something shorter. They are > only useful in scan.c. > > 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.
> > >though this likely requires a > > major cleanup of this driver w.r.t. ubi_scan_leb lifetime semantics. > What is wrong with the semantics, please be more specific.
It's wrong to be allocating and freeing any object in more than one place . Physical eraseblocks are allocated in ubi_scan_add_to_list (which shouldn't be doing that) and ubi_scan_add_used (which is a maze) and freed pretty much all over the place (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). So it makes life easier if you know there's only one place when/where an object is born, 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 wish I could be more specific than this, but I've only spent a few hours with ubi :-) - 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/
| |