Coverity (CID 1614) spotted new_seb being dereferenced after kfree() in
create_vtbl's write_error path.
Signed-off-by: Florin Malita <[email protected]>
---
vtbl.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/mtd/ubi/vtbl.c b/drivers/mtd/ubi/vtbl.c
index b6fd6bb..91e3619 100644
--- a/drivers/mtd/ubi/vtbl.c
+++ b/drivers/mtd/ubi/vtbl.c
@@ -317,14 +317,13 @@ retry:
return err;
write_error:
- kfree(new_seb);
- /* May be this physical eraseblock went bad, try to pick another one */
- if (++tries <= 5) {
+ /* Maybe this physical eraseblock went bad, try to pick another one */
+ if (++tries <= 5)
err = ubi_scan_add_to_list(si, new_seb->pnum, new_seb->ec,
&si->corr);
- if (!err)
- goto retry;
- }
+ kfree(new_seb);
+ if (!err)
+ goto retry;
out_free:
ubi_free_vid_hdr(ubi, vid_hdr);
return err;
On Thu, 2007-05-03 at 11:49 -0400, Florin Malita wrote:
> Coverity (CID 1614) spotted new_seb being dereferenced after kfree() in
> create_vtbl's write_error path.
Applied with minor trailing white-space cleanup, thanks.
--
Best regards,
Artem Bityutskiy (Битюцкий Артём)
Hi Florin, Artem,
On 5/3/07, Florin Malita <[email protected]> wrote:
> [...]
> write_error:
> - kfree(new_seb);
> - /* May be this physical eraseblock went bad, try to pick another one */
> - if (++tries <= 5) {
> + /* Maybe this physical eraseblock went bad, try to pick another one */
> + if (++tries <= 5)
> err = ubi_scan_add_to_list(si, new_seb->pnum, new_seb->ec,
> &si->corr);
> - if (!err)
> - goto retry;
> - }
> + kfree(new_seb);
> + if (!err)
> + goto retry;
Eeks ... no, wait. You found a (two, actually) bug alright, but fixed
it wrong. When we fail a write, we *must* add it to the corrupted list
and _then_ attempt to retry. So, the "if (++tries <= 5)" applies to
"if (!err) goto retry;" and not to the ubi_scan_add_to_list(). The
difference is quite subtle here ...
The correct fix should actually be as follows: (Artem, this is diffed
on the original vtbl.c)
---
Fix write_error logic in drivers/mtd/ubi/vtbl.c:create_vtbl(). On a
write failure, we add the corrupted physical eraseblock to the
corrupted list, and then attempt to retry (upto a maximum of 5 times).
Also, fix an oops by pushing kfree(new_seb) below after dereferencing
it for ubi_scan_add_to_list().
Signed-off-by: Satyam Sharma <[email protected]>
Signed-off-by: Florin Malita <[email protected]>
---
drivers/mtd/ubi/vtbl.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
---
diff -ruNp a/drivers/mtd/ubi/vtbl.c b/drivers/mtd/ubi/vtbl.c
--- a/drivers/mtd/ubi/vtbl.c 2007-04-20 18:42:17.000000000 +0530
+++ b/drivers/mtd/ubi/vtbl.c 2007-05-05 03:01:32.000000000 +0530
@@ -317,14 +317,12 @@ retry:
return err;
write_error:
- kfree(new_seb);
/* May be this physical eraseblock went bad, try to pick another one */
- if (++tries <= 5) {
- err = ubi_scan_add_to_list(si, new_seb->pnum, new_seb->ec,
- &si->corr);
+ err = ubi_scan_add_to_list(si, new_seb->pnum, new_seb->ec, &si->corr);
+ kfree(new_seb);
+ if (++tries <= 5)
if (!err)
goto retry;
- }
out_free:
ubi_free_vid_hdr(ubi, vid_hdr);
return err;
Hi Satyam,
Satyam Sharma wrote:
> Eeks ... no, wait. You found a (two, actually) bug alright, but fixed
> it wrong. When we fail a write, we *must* add it to the corrupted list
> and _then_ attempt to retry. So, the "if (++tries <= 5)" applies to
> "if (!err) goto retry;" and not to the ubi_scan_add_to_list(). The
> difference is quite subtle here ...
Not being familiar with the code, I was specifically trying to preserve
the old semantics and only address the use-after-free issue. So if there
was another bug... well, I guess I succeeded at preserving it ;)
> The correct fix should actually be as follows: (Artem, this is diffed
> on the original vtbl.c)
[snip]
> + err = ubi_scan_add_to_list(si, new_seb->pnum, new_seb->ec,
> &si->corr);
> + kfree(new_seb);
> + if (++tries <= 5)
> if (!err)
> goto retry;
There's a side effect to this change: by unconditionally overwriting err
we lose the original error code. Then if we're exceeding the number of
retries we'll end up returning 0 which is probably not what you want.
Return code aside, it seems the only thing ubi_scan_add_to_list() is
doing is allocate a new struct ubi_scan_leb, initialize some fields with
values passed from new_seb and then add it to the desired list. But
copying new_seb to a newly allocated structure and then immediately
freeing the old one seems redundant - why not just add new_seb to the
corrupted list and be done? Then we don't have to deal with allocation
failures in an error path anymore - something like this (diff against
the original code):
Signed-off-by: Florin Malita <[email protected]>
---
diff --git a/drivers/mtd/ubi/vtbl.c b/drivers/mtd/ubi/vtbl.c
index b6fd6bb..2ad2d59 100644
--- a/drivers/mtd/ubi/vtbl.c
+++ b/drivers/mtd/ubi/vtbl.c
@@ -317,14 +317,10 @@ retry:
return err;
write_error:
- kfree(new_seb);
- /* May be this physical eraseblock went bad, try to pick another one */
- if (++tries <= 5) {
- err = ubi_scan_add_to_list(si, new_seb->pnum, new_seb->ec,
- &si->corr);
- if (!err)
- goto retry;
- }
+ /* Maybe this physical eraseblock went bad, try to pick another one */
+ list_add_tail(&new_seb->u.list, &si->corr);
+ if (++tries <= 5)
+ goto retry;
out_free:
ubi_free_vid_hdr(ubi, vid_hdr);
return err;
On 5/5/07, Florin Malita <[email protected]> wrote:
> Hi Satyam,
>
> Satyam Sharma wrote:
> > Eeks ... no, wait. You found a (two, actually) bug alright, but fixed
> > it wrong. When we fail a write, we *must* add it to the corrupted list
> > and _then_ attempt to retry. So, the "if (++tries <= 5)" applies to
> > "if (!err) goto retry;" and not to the ubi_scan_add_to_list(). The
> > difference is quite subtle here ...
>
> Not being familiar with the code, I was specifically trying to preserve
> the old semantics and only address the use-after-free issue. So if there
> was another bug... well, I guess I succeeded at preserving it ;)
>
> > The correct fix should actually be as follows: (Artem, this is diffed
> > on the original vtbl.c)
> [snip]
> > + err = ubi_scan_add_to_list(si, new_seb->pnum, new_seb->ec,
> > &si->corr);
> > + kfree(new_seb);
> > + if (++tries <= 5)
> > if (!err)
> > goto retry;
>
> There's a side effect to this change: by unconditionally overwriting err
> we lose the original error code. Then if we're exceeding the number of
> retries we'll end up returning 0 which is probably not what you want.
You're absolutely right. We must preserve and return the original
(write error) return code and not the spurious ENOMEM / 0 that would
come from ubi_scan_add_to_list. I had noticed this too, but this time
_I_ preserved the (flawed) old semantics which leads to this problem.
Wow ... so you actually found 3 bugs here in ~3 lines of code :-)
I tried going deeper into this, but the lifetime semantics of a struct
ubi_scan_leb in this driver are quite ... weird indeed.
> Return code aside, it seems the only thing ubi_scan_add_to_list() is
> doing is allocate a new struct ubi_scan_leb, initialize some fields with
> values passed from new_seb and then add it to the desired list. But
> copying new_seb to a newly allocated structure and then immediately
> freeing the old one seems redundant - why not just add new_seb to the
> corrupted list and be done? Then we don't have to deal with allocation
> failures in an error path anymore - something like this (diff against
> the original code):
Again, I saw that too, but would still prefer using the higher level
function ubi_scan_add_to_list() to add to the corrupted list, but with
a different identifier for the return value to avoid overwriting err.
list_add_tail seems best left as an implementation detail below
ubi_scan_add_to_list(), IMO. So if it fails in the error path, we'd
have to return with the original (write error) return value and the
ENOMEM sort of goes ... unreturned. Alas!
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. If possible, the best solution would be to change
ubi_scan_add_to_list() to take in a valid struct ubi_scan_leb and just
add that to the specified list (using list_add_tail or whatever) --
and leave allocation up to callers, though this likely requires a
major cleanup of this driver w.r.t. ubi_scan_leb lifetime semantics.
ubi_scan_add_used() was such a horror!
If adding an existing ubi_scan_leb is fine, but we can't really change
ubi_scan_add_to_list right now, then I'd much rather see a
__ubi_scan_add_to_list() that does the list_add_tail (and those
associated debug printk messages: btw are those printk's the only
reason why we're carrying that ubi_scan_info into
ubi_scan_add_to_list?). So ubi_scan_add_to_list() just wraps a kmalloc
over it -- that way, callers who want a new eraseblock alloced before
adding it to a list can use ubi_scan_add_to_list() and those that
already hold a valid one (like us) could use __ubi_scan_add_to_list()
directly.
I see Andrew merged the previous fix, so this is on top of that,
though I'm not sure if this half-solution should actually go in --
hopefully that verbose comment / changelog will irritate Artem enough
to fix all this for good :-)
---
drivers/mtd/ubi/vtbl.c:create_vtbl() uses ubi_scan_add_to_list() in
the write error path. That can itself return with an ENOMEM error, in
which case we give up but return from create_vtbl() with the original
write error.
A robust solution would be to fix ubi_scan_add_to_list() to just take
in a valid eraseblock and simply add it to the specified list using
list_add_tail, and thus never fail. This would likely require a major
cleanup of this driver.
Alternatively, we could introduce and use a void
__ubi_scan_add_to_list() here that does this and make
ubi_scan_add_to_list() simply wrap a kmalloc over it, to avoid
changing existing users.
Signed-off-by: Satyam Sharma <[email protected]>
---
drivers/mtd/ubi/vtbl.c | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
---
diff -ruNp a/drivers/mtd/ubi/vtbl.c b/drivers/mtd/ubi/vtbl.c
--- a/drivers/mtd/ubi/vtbl.c 2007-05-05 06:17:04.000000000 +0530
+++ b/drivers/mtd/ubi/vtbl.c 2007-05-05 09:23:43.000000000 +0530
@@ -260,7 +260,7 @@ bad:
static int create_vtbl(const struct ubi_device *ubi, struct ubi_scan_info *si,
int copy, void *vtbl)
{
- int err, tries = 0;
+ int add_err, err, tries = 0;
static struct ubi_vid_hdr *vid_hdr;
struct ubi_scan_volume *sv;
struct ubi_scan_leb *new_seb, *old_seb = NULL;
@@ -317,12 +317,19 @@ retry:
return err;
write_error:
- /* May be this physical eraseblock went bad, try to pick another one */
- err = ubi_scan_add_to_list(si, new_seb->pnum, new_seb->ec, &si->corr);
+ /*
+ * May be this physical eraseblock went bad, so add it to the corrupted
+ * list. Note that ubi_scan_add_to_list() allocates a new eraseblock
+ * and could fail with ENOMEM. In that case we give up and return with
+ * the original write error -- the ENOMEM is effectively lost.
+ */
+ add_err = ubi_scan_add_to_list(si, new_seb->pnum, new_seb->ec, &si->corr);
kfree(new_seb);
+ if (add_err)
+ goto out_free;
+ /* Try to pick another one */
if (++tries <= 5)
- if (!err)
- goto retry;
+ goto retry;
out_free:
ubi_free_vid_hdr(ubi, vid_hdr);
return err;
Hi,
thanks for finding bugs in this patch. Although this path will likely
never happen, this is good to have it bug-free.
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.
> If possible, the best solution would be to change
> ubi_scan_add_to_list() to take in a valid struct ubi_scan_leb and just
> add that to the specified list (using list_add_tail or whatever) --
> and leave allocation up to callers,
In scan.c it is useful because _all_ callers have to allocate it. vtbl.c
is the only place which does not need it. I'll fix this.
> >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.
I'll fix this shortly.
--
Best regards,
Artem Bityutskiy (Битюцкий Артём)
On 5/5/07, Artem Bityutskiy <[email protected]> wrote:
> On Sat, 2007-05-05 at 19:18 +0530, Satyam Sharma wrote:
> > Well, you're developing / maintaining this right now, so it's your
> > call. Though I bet most people would find keeping that list_add_tail
> > local to scan.c more tasteful.
>
> I do not think so. If you are interested, try to find "UBI take 2"
> patches in lkml. Look how it looked liked. It consisted of many
> independent units and units could access other units _only_ via
> interfaces. I would do what you say there.
But what you're doing now is *worse*, don't you see it? You _think_
that you are not exporting any add_to_list function from scan.c
because you removed the wrapper from scan.h, but then you open-code it
anyway in create_vtbl (yes, that list_add _is_ the *only* meaningful
code in add_to_list; it just so _happens_ that most of its callers
till now also wanted to allocate a ubi_scan_leb too at that time so
you pushed that too into add_to_list rather than do it in the callers
-- via a separate alloc_leb, for example). Now that means there is an
*undocumented* use of the "add to list" _functionality_ outside of
scan.c anyway and which wouldn't even come up if someone tries to
analyse / overhaul the code some day in the future. I know it's just
_one_ exception, but still, I'm a heckler for style.
> Read Teo's comments - I actually now agree with them. And after I had
> changed UBI i got rid of several thousands lines of code, and the code
> became simpler.
>
> So, my argument is:
> 1. It makes no sense to introduce one more non-static function to _just_
> encapsulate list_add_tail and _just_ for one caller.
Well, introducing one more non-static function is *not* what I
originally had in mind (it was proposed only as a temporary measure
till all users of ubi_scan_add_to_list could be updated to use the
other version, explained below).
I actually planned to _replace_ ubi_scan_add_to_list with another
version that does *not* allocate memory (the __ubi_scan_add_to_list I
proposed above), and leave allocation up to its *callers* who
therefore pass a valid ubi_scan_leb to it. But that is where I ran
into ubi_scan_add_used().
> 2. It is _C_, it is _kernel_, and it is OK sometimes _not_ to follow
> computer since rules.
Ah, well, forget it. It's all a matter of taste and maintainability. I
guess in the end it needs to be fine with you.
On Sat, 2007-05-05 at 19:18 +0530, Satyam Sharma wrote:
> Well, you're developing / maintaining this right now, so it's your
> call. Though I bet most people would find keeping that list_add_tail
> local to scan.c more tasteful.
I do not think so. If you are interested, try to find "UBI take 2"
patches in lkml. Look how it looked liked. It consisted of many
independent units and units could access other units _only_ via
interfaces. I would do what you say there.
Read Teo's comments - I actually now agree with them. And after I had
changed UBI i got rid of several thousands lines of code, and the code
became simpler.
So, my argument is:
1. It makes no sense to introduce one more non-static function to _just_
encapsulate list_add_tail and _just_ for one caller.
2. It is _C_, it is _kernel_, and it is OK sometimes _not_ to follow
computer since rules.
> I wish you'd commented it better than "This function returns zero in
> case of success and a negative error code in case of failure." in that
> case :-)
Agreed, I'll add more comments, thanks.
> Again, you're developing and maintaining this right now, so it's your
> call. Though it would be easier on you if you remove these exceptions
> that could be quite easily removed, actually.
I do not see any nice way to do this. If you suggest one, I will do.
--
Best regards,
Artem Bityutskiy (Битюцкий Артём)
On 5/5/07, Artem Bityutskiy <[email protected]> wrote:
> [...]
> I've put the fix here:
> http://git.infradead.org/?p=users/dedekind/ubi-2.6.git;a=commit;h=5125237efb6a3309fbf5b9a7a21aaf716787f2a2
> write_error:
> if (err == -EIO && ++tries <= 5) {
> /*
> * Probably this physical eraseblock went bad, try to pick
> * another one.
> */
> list_add_tail(&new_seb->u.list, &si->corr);
> goto retry;
> }
> kfree(new_seb);
> out_free:
> ubi_free_vid_hdr(ubi, vid_hdr);
> return err;
Ummm ...
1. "if (err == -EIO)" applies to adding new_seb to the corrupted list,
and not to retrying. We wouldn't want _not_ to retry if there's some
other error, or would we?
2. "if (++tries <= 5)" applies to "goto retry" and not to adding
new_seb to the corrupted list. If we hit write failure for the 5th
time and err == -EIO, we should still be adding it to corrupted list,
but not retry, of course. Otherwise we would add the first 4 write
failure (with -EIO) eraseblocks to si->corr, but the 5th _similar_
case is ... just freed?
So:
write_error:
/*
* Probably this physical eraseblock went bad, add it to the
* corrupted list. Else, just release it.
*/
if (err == -EIO)
/* list_add_tail(&new_seb->u.list, &si->corr); */
__ubi_scan_add_to_list(si, new_seb, &si->corr);
else
kfree(new_seb);
/* Try to pick another one */
if (++tries <= 5)
goto retry;
out_free:
ubi_free_vid_hdr(ubi, vid_hdr);
return err;
And in case you want to avoid the list_add_tail there, the following
(I'm fine with whichever way, of course):
diff -ruNp a/drivers/mtd/ubi/scan.c b/drivers/mtd/ubi/scan.c
--- a/drivers/mtd/ubi/scan.c 2007-05-05 10:52:47.000000000 +0530
+++ b/drivers/mtd/ubi/scan.c 2007-05-05 11:47:26.000000000 +0530
@@ -55,29 +55,37 @@ static int paranoid_check_si(const struc
static struct ubi_ec_hdr *ech;
static struct ubi_vid_hdr *vidh;
-int ubi_scan_add_to_list(struct ubi_scan_info *si, int pnum, int ec,
- struct list_head *list)
+void __ubi_scan_add_to_list(struct ubi_scan_info *si,
+ struct ubi_scan_leb *seb,
+ struct list_head *list)
{
- struct ubi_scan_leb *seb;
-
if (list == &si->free)
- dbg_bld("add to free: PEB %d, EC %d", pnum, ec);
+ dbg_bld("add to free: PEB %d, EC %d", seb->pnum, seb->ec);
else if (list == &si->erase)
- dbg_bld("add to erase: PEB %d, EC %d", pnum, ec);
+ dbg_bld("add to erase: PEB %d, EC %d", seb->pnum, seb->ec);
else if (list == &si->corr)
- dbg_bld("add to corrupted: PEB %d, EC %d", pnum, ec);
+ dbg_bld("add to corrupted: PEB %d, EC %d", seb->pnum, seb->ec);
else if (list == &si->alien)
- dbg_bld("add to alien: PEB %d, EC %d", pnum, ec);
+ dbg_bld("add to alien: PEB %d, EC %d", seb->pnum, seb->ec);
else
BUG();
+ list_add_tail(&seb->u.list, list);
+}
+
+int ubi_scan_add_to_list(struct ubi_scan_info *si, int pnum, int ec,
+ struct list_head *list)
+{
+ struct ubi_scan_leb *seb;
+
seb = kmalloc(sizeof(struct ubi_scan_leb), GFP_KERNEL);
if (unlikely(!seb))
return -ENOMEM;
seb->pnum = pnum;
seb->ec = ec;
- list_add_tail(&seb->u.list, list);
+ __ubi_scan_add_to_list(si, seb, list);
+
return 0;
}
diff -ruNp a/drivers/mtd/ubi/scan.h b/drivers/mtd/ubi/scan.h
--- a/drivers/mtd/ubi/scan.h 2007-05-05 10:55:24.000000000 +0530
+++ b/drivers/mtd/ubi/scan.h 2007-05-05 10:55:01.000000000 +0530
@@ -147,6 +147,9 @@ static inline void ubi_scan_move_to_list
list_add_tail(&seb->u.list, list);
}
+void __ubi_scan_add_to_list(struct ubi_scan_info *si,
+ struct ubi_scan_leb *seb,
+ struct list_head *list);
int ubi_scan_add_to_list(struct ubi_scan_info *si, int pnum, int ec,
struct list_head *list);
int ubi_scan_add_used(const struct ubi_device *ubi, struct ubi_scan_info *si,
On 5/5/07, Artem Bityutskiy <[email protected]> wrote:
> 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())
Well, you're developing / maintaining this right now, so it's your
call. Though I bet most people would find keeping that list_add_tail
local to scan.c more tasteful.
> > 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.
I wish you'd commented it better than "This function returns zero in
case of success and a negative error code in case of failure." in that
case :-)
> 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.
Again, you're developing and maintaining this right now, so it's your
call. Though it would be easier on you if you remove these exceptions
that could be quite easily removed, actually.
Thanks,
Satyam
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 (Битюцкий Артём)
On 5/5/07, Artem Bityutskiy <[email protected]> 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 :-)
On Sat, 2007-05-05 at 19:02 +0530, Satyam Sharma wrote:
> > write_error:
> > if (err == -EIO && ++tries <= 5) {
> > /*
> > * Probably this physical eraseblock went bad, try to pick
> > * another one.
> > */
> > list_add_tail(&new_seb->u.list, &si->corr);
> > goto retry;
> > }
> > kfree(new_seb);
> > out_free:
> > ubi_free_vid_hdr(ubi, vid_hdr);
> > return err;
>
> Ummm ...
>
> 1. "if (err == -EIO)" applies to adding new_seb to the corrupted list,
> and not to retrying. We wouldn't want _not_ to retry if there's some
> other error, or would we?
In case of other error - no, we do not want to retry. Only in case of
-EIO because we just might have hit a new badblock, which is unlikely,
but possible.
If it is anything else then -EIO, then we just return an error and
_refuse_ to attach this MTD device. In this case it does not matter
where we add new_seb. We just drop it. We free all allocated data
structures.
> 2. "if (++tries <= 5)" applies to "goto retry" and not to adding
> new_seb to the corrupted list. If we hit write failure for the 5th
> time and err == -EIO, we should still be adding it to corrupted list,
> but not retry, of course. Otherwise we would add the first 4 write
> failure (with -EIO) eraseblocks to si->corr, but the 5th _similar_
> case is ... just freed?
If we hit -EIO more then five times, there is probably something _really
bad_ with this MTD device and we _refuse_ attaching it. We return error,
and every data structure is freed. It does not matter if we add new_seb
anywhere or not. It is anyway just freed.
--
Best regards,
Artem Bityutskiy (Битюцкий Артём)
On Sat, 2007-05-05 at 09:25 +0530, Satyam Sharma wrote:
> Again, I saw that too, but would still prefer using the higher level
> function ubi_scan_add_to_list() to add to the corrupted list, but with
> a different identifier for the return value to avoid overwriting err.
> list_add_tail seems best left as an implementation detail below
> ubi_scan_add_to_list(), IMO. So if it fails in the error path, we'd
> have to return with the original (write error) return value and the
> ENOMEM sort of goes ... unreturned. Alas!
I've put the fix here:
http://git.infradead.org/?p=users/dedekind/ubi-2.6.git;a=commit;h=5125237efb6a3309fbf5b9a7a21aaf716787f2a2
--
Best regards,
Artem Bityutskiy (Битюцкий Артём)