| Message ID | 1414586758-9972-3-git-send-email-richard@nod.at |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, 2014-10-29 at 13:45 +0100, Richard Weinberger wrote: > + ubi->lookuptbl[e2->pnum] = NULL; > kmem_cache_free(ubi_wl_entry_slab, e2); Since these 2 must always go together, they really deserve a separate helper function.
Hi Richard On 10/29/2014 2:45 PM, Richard Weinberger wrote: > In some error paths the WL sub-system gives up on a PEB > and frees it's ubi_wl_entry struct but does not set > the entry in ubi->lookuptbl to NULL. > Fastmap can stumble over such a stale pointer as it uses > ubi->lookuptbl to find all PEBs. > > Fix this by setting the pointers to free'd ubi_wl_entry to NULL. There are 2 more places: tree_destroy() and protection_queue_destroy() where ubi_wl_entry is released. Both functions used on power down so all should be good as is, just wanted to make sure you didn't add ubi->lookuptbl[e2->pnum] = NULL there on purpose. Thanks, Tanya Brokhman
Tanya, Am 04.12.2014 um 12:00 schrieb Tanya Brokhman: > Hi Richard > > On 10/29/2014 2:45 PM, Richard Weinberger wrote: >> In some error paths the WL sub-system gives up on a PEB >> and frees it's ubi_wl_entry struct but does not set >> the entry in ubi->lookuptbl to NULL. >> Fastmap can stumble over such a stale pointer as it uses >> ubi->lookuptbl to find all PEBs. >> >> Fix this by setting the pointers to free'd ubi_wl_entry to NULL. > > There are 2 more places: > tree_destroy() and protection_queue_destroy() where ubi_wl_entry is released. Both functions used on power down so all should be good as is, just wanted to make sure you didn't add > ubi->lookuptbl[e2->pnum] = NULL there on purpose. I fear you're looking at the old patch series. Please have a look at: "[PATCH 3/6] UBI: Fix stale pointers in ubi->lookuptbl" sent on 30.11.2014. Thanks, //richard
On 12/4/2014 1:04 PM, Richard Weinberger wrote: > Tanya, > > Am 04.12.2014 um 12:00 schrieb Tanya Brokhman: >> Hi Richard >> >> On 10/29/2014 2:45 PM, Richard Weinberger wrote: >>> In some error paths the WL sub-system gives up on a PEB >>> and frees it's ubi_wl_entry struct but does not set >>> the entry in ubi->lookuptbl to NULL. >>> Fastmap can stumble over such a stale pointer as it uses >>> ubi->lookuptbl to find all PEBs. >>> >>> Fix this by setting the pointers to free'd ubi_wl_entry to NULL. >> >> There are 2 more places: >> tree_destroy() and protection_queue_destroy() where ubi_wl_entry is released. Both functions used on power down so all should be good as is, just wanted to make sure you didn't add >> ubi->lookuptbl[e2->pnum] = NULL there on purpose. > > I fear you're looking at the old patch series. > Please have a look at: > "[PATCH 3/6] UBI: Fix stale pointers in ubi->lookuptbl" sent on 30.11.2014. > > Thanks, > //richard > oh, you're right. Started from the oldest. now I see you released v2. Sorry. So I should be looking at "Fastmap update v2 (pile 1-7)", right? Thanks, Tanya Brokhman
Am 04.12.2014 um 13:37 schrieb Tanya Brokhman: > On 12/4/2014 1:04 PM, Richard Weinberger wrote: >> Tanya, >> >> Am 04.12.2014 um 12:00 schrieb Tanya Brokhman: >>> Hi Richard >>> >>> On 10/29/2014 2:45 PM, Richard Weinberger wrote: >>>> In some error paths the WL sub-system gives up on a PEB >>>> and frees it's ubi_wl_entry struct but does not set >>>> the entry in ubi->lookuptbl to NULL. >>>> Fastmap can stumble over such a stale pointer as it uses >>>> ubi->lookuptbl to find all PEBs. >>>> >>>> Fix this by setting the pointers to free'd ubi_wl_entry to NULL. >>> >>> There are 2 more places: >>> tree_destroy() and protection_queue_destroy() where ubi_wl_entry is released. Both functions used on power down so all should be good as is, just wanted to make sure you didn't add >>> ubi->lookuptbl[e2->pnum] = NULL there on purpose. >> >> I fear you're looking at the old patch series. >> Please have a look at: >> "[PATCH 3/6] UBI: Fix stale pointers in ubi->lookuptbl" sent on 30.11.2014. >> >> Thanks, >> //richard >> > > oh, you're right. Started from the oldest. now I see you released v2. Sorry. So I should be looking at "Fastmap update v2 (pile 1-7)", right? Correct. :) Thanks, //richard
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c index 6654f191..a12c54b 100644 --- a/drivers/mtd/ubi/wl.c +++ b/drivers/mtd/ubi/wl.c @@ -1212,9 +1212,12 @@ static int wear_leveling_worker(struct ubi_device *ubi, struct ubi_work *wrk, err = do_sync_erase(ubi, e1, vol_id, lnum, 0); if (err) { + ubi->lookuptbl[e1->pnum] = NULL; kmem_cache_free(ubi_wl_entry_slab, e1); - if (e2) + if (e2) { + ubi->lookuptbl[e2->pnum] = NULL; kmem_cache_free(ubi_wl_entry_slab, e2); + } goto out_ro; } @@ -1227,6 +1230,7 @@ static int wear_leveling_worker(struct ubi_device *ubi, struct ubi_work *wrk, e2->pnum, vol_id, lnum); err = do_sync_erase(ubi, e2, vol_id, lnum, 0); if (err) { + ubi->lookuptbl[e2->pnum] = NULL; kmem_cache_free(ubi_wl_entry_slab, e2); goto out_ro; } @@ -1266,6 +1270,7 @@ out_not_moved: ubi_free_vid_hdr(ubi, vid_hdr); err = do_sync_erase(ubi, e2, vol_id, lnum, torture); if (err) { + ubi->lookuptbl[e2->pnum] = NULL; kmem_cache_free(ubi_wl_entry_slab, e2); goto out_ro; } @@ -1285,6 +1290,8 @@ out_error: spin_unlock(&ubi->wl_lock); ubi_free_vid_hdr(ubi, vid_hdr); + ubi->lookuptbl[e1->pnum] = NULL; + ubi->lookuptbl[e2->pnum] = NULL; kmem_cache_free(ubi_wl_entry_slab, e1); kmem_cache_free(ubi_wl_entry_slab, e2); @@ -1428,6 +1435,7 @@ static int erase_worker(struct ubi_device *ubi, struct ubi_work *wl_wrk, if (shutdown) { dbg_wl("cancel erasure of PEB %d EC %d", pnum, e->ec); kfree(wl_wrk); + ubi->lookuptbl[e->pnum] = NULL; kmem_cache_free(ubi_wl_entry_slab, e); return 0; } @@ -1474,6 +1482,7 @@ static int erase_worker(struct ubi_device *ubi, struct ubi_work *wl_wrk, return err; } + ubi->lookuptbl[e->pnum] = NULL; kmem_cache_free(ubi_wl_entry_slab, e); if (err != -EIO) /* @@ -1912,6 +1921,7 @@ int ubi_wl_init(struct ubi_device *ubi, struct ubi_attach_info *ai) ubi_assert(!ubi_is_fm_block(ubi, e->pnum)); ubi->lookuptbl[e->pnum] = e; if (schedule_erase(ubi, e, aeb->vol_id, aeb->lnum, 0)) { + ubi->lookuptbl[e->pnum] = NULL; kmem_cache_free(ubi_wl_entry_slab, e); goto out_free; }
In some error paths the WL sub-system gives up on a PEB and frees it's ubi_wl_entry struct but does not set the entry in ubi->lookuptbl to NULL. Fastmap can stumble over such a stale pointer as it uses ubi->lookuptbl to find all PEBs. Fix this by setting the pointers to free'd ubi_wl_entry to NULL. Signed-off-by: Richard Weinberger <richard@nod.at> --- drivers/mtd/ubi/wl.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)