| Message ID | 1416835236-25185-5-git-send-email-richard@nod.at |
|---|---|
| State | Accepted |
| Headers | show |
On 11/24/2014 3:20 PM, Richard Weinberger wrote: > ubi_wl_get_peb() has two problems, it reads the pool > size and usage counters without any protection. > While reading one value would be perfectly fine it reads multiple > values and compares them. This is racy and can lead to incorrect > pool handling. > Furthermore ubi_update_fastmap() is called without wl_lock held, > before incrementing the used counter it needs to be checked again. I didn't see where you fixed the ubi_update_fastmap issue you just mentioned. > It could happen that another thread consumed all PEBs from the > pool and the counter goes beyond ->size. > > Signed-off-by: Richard Weinberger <richard@nod.at> > --- > drivers/mtd/ubi/ubi.h | 3 ++- > drivers/mtd/ubi/wl.c | 34 +++++++++++++++++++++++----------- > 2 files changed, 25 insertions(+), 12 deletions(-) > > diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h > index 04c4c05..d672412 100644 > --- a/drivers/mtd/ubi/ubi.h > +++ b/drivers/mtd/ubi/ubi.h > @@ -439,7 +439,8 @@ struct ubi_debug_info { > * @pq_head: protection queue head > * @wl_lock: protects the @used, @free, @pq, @pq_head, @lookuptbl, @move_from, > * @move_to, @move_to_put @erase_pending, @wl_scheduled, @works, > - * @erroneous, @erroneous_peb_count, and @fm_work_scheduled fields > + * @erroneous, @erroneous_peb_count, @fm_work_scheduled, @fm_pool, > + * and @fm_wl_pool fields > * @move_mutex: serializes eraseblock moves > * @work_sem: used to wait for all the scheduled works to finish and prevent > * new works from being submitted > diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c > index cb2e571..7730b97 100644 > --- a/drivers/mtd/ubi/wl.c > +++ b/drivers/mtd/ubi/wl.c > @@ -629,24 +629,36 @@ void ubi_refill_pools(struct ubi_device *ubi) > */ > int ubi_wl_get_peb(struct ubi_device *ubi) > { > - int ret; > + int ret, retried = 0; > struct ubi_fm_pool *pool = &ubi->fm_pool; > struct ubi_fm_pool *wl_pool = &ubi->fm_wl_pool; > > - if (!pool->size || !wl_pool->size || pool->used == pool->size || > - wl_pool->used == wl_pool->size) > +again: > + spin_lock(&ubi->wl_lock); > + /* We check here also for the WL pool because at this point we can > + * refill the WL pool synchronous. */ > + if (pool->used == pool->size || wl_pool->used == wl_pool->size) { > + spin_unlock(&ubi->wl_lock); > ubi_update_fastmap(ubi); > - > - /* we got not a single free PEB */ > - if (!pool->size) > - ret = -ENOSPC; > - else { > spin_lock(&ubi->wl_lock); > - ret = pool->pebs[pool->used++]; > - prot_queue_add(ubi, ubi->lookuptbl[ret]); > + } > + > + if (pool->used == pool->size) { Im confused about this "if" condition. You just tested pool->used == pool->size in the previous "if". If in the previous if pool->used != pool->size and wl_pool->used != wl_pool->size, you didn't enter, the lock is still held so pool->used != pool->size still. If in the previos "if" wl_pool->used == wl_pool->size was true nd tou released the lock, ubi_update_fastmap(ubi) was called, which refills the pools. So again, if pools were refilled pool->used would be 0 here and pool->size > 0. So in both cases I don't see how at this point pool->used == pool->size could ever be true? > spin_unlock(&ubi->wl_lock); > + if (retried) { > + ubi_err(ubi, "Unable to get a free PEB from user WL pool"); > + ret = -ENOSPC; > + goto out; > + } > + retried = 1; Why did you decide to retry in this function? and why only 1 retry attempt? I'm not against it, trying to understand the logic. > + goto again; > } > > + ubi_assert(pool->used < pool->size); > + ret = pool->pebs[pool->used++]; > + prot_queue_add(ubi, ubi->lookuptbl[ret]); > + spin_unlock(&ubi->wl_lock); > +out: > return ret; > } > > @@ -659,7 +671,7 @@ static struct ubi_wl_entry *get_peb_for_wl(struct ubi_device *ubi) > struct ubi_fm_pool *pool = &ubi->fm_wl_pool; > int pnum; > > - if (pool->used == pool->size || !pool->size) { > + if (pool->used == pool->size) { > /* We cannot update the fastmap here because this > * function is called in atomic context. > * Let's fail here and refill/update it as soon as possible. */ > Thanks, Tanya Brokhman
Tanya, Am 05.12.2014 um 14:09 schrieb Tanya Brokhman: > On 11/24/2014 3:20 PM, Richard Weinberger wrote: >> ubi_wl_get_peb() has two problems, it reads the pool >> size and usage counters without any protection. >> While reading one value would be perfectly fine it reads multiple >> values and compares them. This is racy and can lead to incorrect >> pool handling. >> Furthermore ubi_update_fastmap() is called without wl_lock held, >> before incrementing the used counter it needs to be checked again. > > I didn't see where you fixed the ubi_update_fastmap issue you just mentioned. This is exactly what you're questioning below. We have to recheck as the pool counter could have changed. >> It could happen that another thread consumed all PEBs from the >> pool and the counter goes beyond ->size. >> >> Signed-off-by: Richard Weinberger <richard@nod.at> >> --- >> drivers/mtd/ubi/ubi.h | 3 ++- >> drivers/mtd/ubi/wl.c | 34 +++++++++++++++++++++++----------- >> 2 files changed, 25 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h >> index 04c4c05..d672412 100644 >> --- a/drivers/mtd/ubi/ubi.h >> +++ b/drivers/mtd/ubi/ubi.h >> @@ -439,7 +439,8 @@ struct ubi_debug_info { >> * @pq_head: protection queue head >> * @wl_lock: protects the @used, @free, @pq, @pq_head, @lookuptbl, @move_from, >> * @move_to, @move_to_put @erase_pending, @wl_scheduled, @works, >> - * @erroneous, @erroneous_peb_count, and @fm_work_scheduled fields >> + * @erroneous, @erroneous_peb_count, @fm_work_scheduled, @fm_pool, >> + * and @fm_wl_pool fields >> * @move_mutex: serializes eraseblock moves >> * @work_sem: used to wait for all the scheduled works to finish and prevent >> * new works from being submitted >> diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c >> index cb2e571..7730b97 100644 >> --- a/drivers/mtd/ubi/wl.c >> +++ b/drivers/mtd/ubi/wl.c >> @@ -629,24 +629,36 @@ void ubi_refill_pools(struct ubi_device *ubi) >> */ >> int ubi_wl_get_peb(struct ubi_device *ubi) >> { >> - int ret; >> + int ret, retried = 0; >> struct ubi_fm_pool *pool = &ubi->fm_pool; >> struct ubi_fm_pool *wl_pool = &ubi->fm_wl_pool; >> >> - if (!pool->size || !wl_pool->size || pool->used == pool->size || >> - wl_pool->used == wl_pool->size) >> +again: >> + spin_lock(&ubi->wl_lock); >> + /* We check here also for the WL pool because at this point we can >> + * refill the WL pool synchronous. */ >> + if (pool->used == pool->size || wl_pool->used == wl_pool->size) { >> + spin_unlock(&ubi->wl_lock); >> ubi_update_fastmap(ubi); >> - >> - /* we got not a single free PEB */ >> - if (!pool->size) >> - ret = -ENOSPC; >> - else { >> spin_lock(&ubi->wl_lock); >> - ret = pool->pebs[pool->used++]; >> - prot_queue_add(ubi, ubi->lookuptbl[ret]); >> + } >> + >> + if (pool->used == pool->size) { > > Im confused about this "if" condition. You just tested pool->used == pool->size in the previous "if". If in the previous if pool->used != pool->size and wl_pool->used != > wl_pool->size, you didn't enter, the lock is still held so pool->used != pool->size still. If in the previos "if" wl_pool->used == wl_pool->size was true nd tou released the lock, > ubi_update_fastmap(ubi) was called, which refills the pools. So again, if pools were refilled pool->used would be 0 here and pool->size > 0. > > So in both cases I don't see how at this point pool->used == pool->size could ever be true? If we enter the "if (pool->used == pool->size || wl_pool->used == wl_pool->size) {" branch we unlock wl_lock and call ubi_update_fastmap(). Another thread can enter ubi_wl_get_peb() and alter the pool counter. So we have to recheck the counter after taking wl_lock again. >> spin_unlock(&ubi->wl_lock); >> + if (retried) { >> + ubi_err(ubi, "Unable to get a free PEB from user WL pool"); >> + ret = -ENOSPC; >> + goto out; >> + } >> + retried = 1; > > Why did you decide to retry in this function? and why only 1 retry attempt? I'm not against it, trying to understand the logic. Because failing immediately with -ENOSPC is not nice. Before we do that I'll give UBI a second chance to produce a free PEB. Thanks, //richard
Hi Richard On 12/5/2014 3:20 PM, Richard Weinberger wrote: > Tanya, > > Am 05.12.2014 um 14:09 schrieb Tanya Brokhman: >> On 11/24/2014 3:20 PM, Richard Weinberger wrote: >>> ubi_wl_get_peb() has two problems, it reads the pool >>> size and usage counters without any protection. >>> While reading one value would be perfectly fine it reads multiple >>> values and compares them. This is racy and can lead to incorrect >>> pool handling. >>> Furthermore ubi_update_fastmap() is called without wl_lock held, >>> before incrementing the used counter it needs to be checked again. >> >> I didn't see where you fixed the ubi_update_fastmap issue you just mentioned. > > This is exactly what you're questioning below. > We have to recheck as the pool counter could have changed. > Oh, I understood the commit msg a bit differently, but now I see that it was my mistake. thanks! >>> It could happen that another thread consumed all PEBs from the >>> pool and the counter goes beyond ->size. >>> >>> Signed-off-by: Richard Weinberger <richard@nod.at> >>> --- >>> drivers/mtd/ubi/ubi.h | 3 ++- >>> drivers/mtd/ubi/wl.c | 34 +++++++++++++++++++++++----------- >>> 2 files changed, 25 insertions(+), 12 deletions(-) >>> >>> diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h >>> index 04c4c05..d672412 100644 >>> --- a/drivers/mtd/ubi/ubi.h >>> +++ b/drivers/mtd/ubi/ubi.h >>> @@ -439,7 +439,8 @@ struct ubi_debug_info { >>> * @pq_head: protection queue head >>> * @wl_lock: protects the @used, @free, @pq, @pq_head, @lookuptbl, @move_from, >>> * @move_to, @move_to_put @erase_pending, @wl_scheduled, @works, >>> - * @erroneous, @erroneous_peb_count, and @fm_work_scheduled fields >>> + * @erroneous, @erroneous_peb_count, @fm_work_scheduled, @fm_pool, >>> + * and @fm_wl_pool fields >>> * @move_mutex: serializes eraseblock moves >>> * @work_sem: used to wait for all the scheduled works to finish and prevent >>> * new works from being submitted >>> diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c >>> index cb2e571..7730b97 100644 >>> --- a/drivers/mtd/ubi/wl.c >>> +++ b/drivers/mtd/ubi/wl.c >>> @@ -629,24 +629,36 @@ void ubi_refill_pools(struct ubi_device *ubi) >>> */ >>> int ubi_wl_get_peb(struct ubi_device *ubi) >>> { >>> - int ret; >>> + int ret, retried = 0; >>> struct ubi_fm_pool *pool = &ubi->fm_pool; >>> struct ubi_fm_pool *wl_pool = &ubi->fm_wl_pool; >>> >>> - if (!pool->size || !wl_pool->size || pool->used == pool->size || >>> - wl_pool->used == wl_pool->size) >>> +again: >>> + spin_lock(&ubi->wl_lock); >>> + /* We check here also for the WL pool because at this point we can >>> + * refill the WL pool synchronous. */ >>> + if (pool->used == pool->size || wl_pool->used == wl_pool->size) { >>> + spin_unlock(&ubi->wl_lock); >>> ubi_update_fastmap(ubi); >>> - >>> - /* we got not a single free PEB */ >>> - if (!pool->size) >>> - ret = -ENOSPC; >>> - else { >>> spin_lock(&ubi->wl_lock); >>> - ret = pool->pebs[pool->used++]; >>> - prot_queue_add(ubi, ubi->lookuptbl[ret]); >>> + } >>> + >>> + if (pool->used == pool->size) { >> >> Im confused about this "if" condition. You just tested pool->used == pool->size in the previous "if". If in the previous if pool->used != pool->size and wl_pool->used != >> wl_pool->size, you didn't enter, the lock is still held so pool->used != pool->size still. If in the previos "if" wl_pool->used == wl_pool->size was true nd tou released the lock, >> ubi_update_fastmap(ubi) was called, which refills the pools. So again, if pools were refilled pool->used would be 0 here and pool->size > 0. >> >> So in both cases I don't see how at this point pool->used == pool->size could ever be true? > > If we enter the "if (pool->used == pool->size || wl_pool->used == wl_pool->size) {" branch we unlock wl_lock and call ubi_update_fastmap(). > Another thread can enter ubi_wl_get_peb() and alter the pool counter. So we have to recheck the counter after taking wl_lock again. hmmm... ok. Perhaps a comment could be added in the code to explain this case in a few words? > >>> spin_unlock(&ubi->wl_lock); >>> + if (retried) { >>> + ubi_err(ubi, "Unable to get a free PEB from user WL pool"); >>> + ret = -ENOSPC; >>> + goto out; >>> + } >>> + retried = 1; >> >> Why did you decide to retry in this function? and why only 1 retry attempt? I'm not against it, trying to understand the logic. > > Because failing immediately with -ENOSPC is not nice. Why not? this is what was done before.... I think what I really bothers me in this case is that you don't sleep, you branch immediately to retry again, so the chances that there will be context switch and free pebs appear aren't that high. I'm used to functions using some sort of "retry" logic to sleep before retrying. Of course sleeping isn't a good idea here. That's why the "retry" bugs me a bit. Before we do that I'll give UBI a second chance to produce a free PEB. > > Thanks, > //richard > Thanks, Tanya Brokhman
Tanya, Am 05.12.2014 um 17:54 schrieb Tanya Brokhman: > Hi Richard > > On 12/5/2014 3:20 PM, Richard Weinberger wrote: >> Tanya, >> >> Am 05.12.2014 um 14:09 schrieb Tanya Brokhman: >>> On 11/24/2014 3:20 PM, Richard Weinberger wrote: >>>> ubi_wl_get_peb() has two problems, it reads the pool >>>> size and usage counters without any protection. >>>> While reading one value would be perfectly fine it reads multiple >>>> values and compares them. This is racy and can lead to incorrect >>>> pool handling. >>>> Furthermore ubi_update_fastmap() is called without wl_lock held, >>>> before incrementing the used counter it needs to be checked again. >>> >>> I didn't see where you fixed the ubi_update_fastmap issue you just mentioned. >> >> This is exactly what you're questioning below. >> We have to recheck as the pool counter could have changed. >> > > Oh, I understood the commit msg a bit differently, but now I see that it was my mistake. thanks! > >>>> It could happen that another thread consumed all PEBs from the >>>> pool and the counter goes beyond ->size. >>>> >>>> Signed-off-by: Richard Weinberger <richard@nod.at> >>>> --- >>>> drivers/mtd/ubi/ubi.h | 3 ++- >>>> drivers/mtd/ubi/wl.c | 34 +++++++++++++++++++++++----------- >>>> 2 files changed, 25 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h >>>> index 04c4c05..d672412 100644 >>>> --- a/drivers/mtd/ubi/ubi.h >>>> +++ b/drivers/mtd/ubi/ubi.h >>>> @@ -439,7 +439,8 @@ struct ubi_debug_info { >>>> * @pq_head: protection queue head >>>> * @wl_lock: protects the @used, @free, @pq, @pq_head, @lookuptbl, @move_from, >>>> * @move_to, @move_to_put @erase_pending, @wl_scheduled, @works, >>>> - * @erroneous, @erroneous_peb_count, and @fm_work_scheduled fields >>>> + * @erroneous, @erroneous_peb_count, @fm_work_scheduled, @fm_pool, >>>> + * and @fm_wl_pool fields >>>> * @move_mutex: serializes eraseblock moves >>>> * @work_sem: used to wait for all the scheduled works to finish and prevent >>>> * new works from being submitted >>>> diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c >>>> index cb2e571..7730b97 100644 >>>> --- a/drivers/mtd/ubi/wl.c >>>> +++ b/drivers/mtd/ubi/wl.c >>>> @@ -629,24 +629,36 @@ void ubi_refill_pools(struct ubi_device *ubi) >>>> */ >>>> int ubi_wl_get_peb(struct ubi_device *ubi) >>>> { >>>> - int ret; >>>> + int ret, retried = 0; >>>> struct ubi_fm_pool *pool = &ubi->fm_pool; >>>> struct ubi_fm_pool *wl_pool = &ubi->fm_wl_pool; >>>> >>>> - if (!pool->size || !wl_pool->size || pool->used == pool->size || >>>> - wl_pool->used == wl_pool->size) >>>> +again: >>>> + spin_lock(&ubi->wl_lock); >>>> + /* We check here also for the WL pool because at this point we can >>>> + * refill the WL pool synchronous. */ >>>> + if (pool->used == pool->size || wl_pool->used == wl_pool->size) { >>>> + spin_unlock(&ubi->wl_lock); >>>> ubi_update_fastmap(ubi); >>>> - >>>> - /* we got not a single free PEB */ >>>> - if (!pool->size) >>>> - ret = -ENOSPC; >>>> - else { >>>> spin_lock(&ubi->wl_lock); >>>> - ret = pool->pebs[pool->used++]; >>>> - prot_queue_add(ubi, ubi->lookuptbl[ret]); >>>> + } >>>> + >>>> + if (pool->used == pool->size) { >>> >>> Im confused about this "if" condition. You just tested pool->used == pool->size in the previous "if". If in the previous if pool->used != pool->size and wl_pool->used != >>> wl_pool->size, you didn't enter, the lock is still held so pool->used != pool->size still. If in the previos "if" wl_pool->used == wl_pool->size was true nd tou released the lock, >>> ubi_update_fastmap(ubi) was called, which refills the pools. So again, if pools were refilled pool->used would be 0 here and pool->size > 0. >>> >>> So in both cases I don't see how at this point pool->used == pool->size could ever be true? >> >> If we enter the "if (pool->used == pool->size || wl_pool->used == wl_pool->size) {" branch we unlock wl_lock and call ubi_update_fastmap(). >> Another thread can enter ubi_wl_get_peb() and alter the pool counter. So we have to recheck the counter after taking wl_lock again. > > hmmm... ok. Perhaps a comment could be added in the code to explain this case in a few words? > Makes sense, I'll add a comment. >>>> spin_unlock(&ubi->wl_lock); >>>> + if (retried) { >>>> + ubi_err(ubi, "Unable to get a free PEB from user WL pool"); >>>> + ret = -ENOSPC; >>>> + goto out; >>>> + } >>>> + retried = 1; >>> >>> Why did you decide to retry in this function? and why only 1 retry attempt? I'm not against it, trying to understand the logic. >> >> Because failing immediately with -ENOSPC is not nice. > > Why not? this is what was done before.... The behavior from before was not good. If we return here a -ENOSPC it is not because we ran out of free PEBs, it is because the pool contains no free PEBs and needs refilling. As between refilling the pool and requesting a fresh PEB from it another thread could "steal" all PEBs we retry. > I think what I really bothers me in this case is that you don't sleep, you branch immediately to retry again, so the chances that there will be context switch and free pebs appear > aren't that high. > I'm used to functions using some sort of "retry" logic to sleep before retrying. Of course sleeping isn't a good idea here. That's why the "retry" bugs me a bit. You mean a cond_resched()? This retry-logic is common pattern in UBI. For exmaple see ubi_wl_put_peb(). Thanks, //richard
On 12/5/2014 11:08 PM, Richard Weinberger wrote: > >>>>> spin_unlock(&ubi->wl_lock); >>>>> + if (retried) { >>>>> + ubi_err(ubi, "Unable to get a free PEB from user WL pool"); >>>>> + ret = -ENOSPC; >>>>> + goto out; >>>>> + } >>>>> + retried = 1; >>>> >>>> Why did you decide to retry in this function? and why only 1 retry attempt? I'm not against it, trying to understand the logic. >>> >>> Because failing immediately with -ENOSPC is not nice. >> >> Why not? this is what was done before.... > > The behavior from before was not good. > If we return here a -ENOSPC it is not because we ran out of free PEBs, it is because the pool contains > no free PEBs and needs refilling. > As between refilling the pool and requesting a fresh PEB from it another thread could "steal" all PEBs > we retry. > >> I think what I really bothers me in this case is that you don't sleep, you branch immediately to retry again, so the chances that there will be context switch and free pebs appear >> aren't that high. >> I'm used to functions using some sort of "retry" logic to sleep before retrying. Of course sleeping isn't a good idea here. That's why the "retry" bugs me a bit. > > You mean a cond_resched()? > This retry-logic is common pattern in UBI. For exmaple see ubi_wl_put_peb(). you're right. didn't pay much attention to ubi_wl_put_peb() before. don't like it there either :) perhaps we can rethink this later for both cases. > > Thanks, > //richard > Thanks, Tanya Brokhman
Am 07.12.2014 um 08:36 schrieb Tanya Brokhman: > On 12/5/2014 11:08 PM, Richard Weinberger wrote: > >> >>>>>> spin_unlock(&ubi->wl_lock); >>>>>> + if (retried) { >>>>>> + ubi_err(ubi, "Unable to get a free PEB from user WL pool"); >>>>>> + ret = -ENOSPC; >>>>>> + goto out; >>>>>> + } >>>>>> + retried = 1; >>>>> >>>>> Why did you decide to retry in this function? and why only 1 retry attempt? I'm not against it, trying to understand the logic. >>>> >>>> Because failing immediately with -ENOSPC is not nice. >>> >>> Why not? this is what was done before.... >> >> The behavior from before was not good. >> If we return here a -ENOSPC it is not because we ran out of free PEBs, it is because the pool contains >> no free PEBs and needs refilling. >> As between refilling the pool and requesting a fresh PEB from it another thread could "steal" all PEBs >> we retry. >> >>> I think what I really bothers me in this case is that you don't sleep, you branch immediately to retry again, so the chances that there will be context switch and free pebs appear >>> aren't that high. >>> I'm used to functions using some sort of "retry" logic to sleep before retrying. Of course sleeping isn't a good idea here. That's why the "retry" bugs me a bit. >> >> You mean a cond_resched()? >> This retry-logic is common pattern in UBI. For exmaple see ubi_wl_put_peb(). > > you're right. didn't pay much attention to ubi_wl_put_peb() before. don't like it there either :) > perhaps we can rethink this later for both cases. If there is room for improvement I'm all open for an extra patch set all over UBI. :-) Thanks, //richard
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h index 04c4c05..d672412 100644 --- a/drivers/mtd/ubi/ubi.h +++ b/drivers/mtd/ubi/ubi.h @@ -439,7 +439,8 @@ struct ubi_debug_info { * @pq_head: protection queue head * @wl_lock: protects the @used, @free, @pq, @pq_head, @lookuptbl, @move_from, * @move_to, @move_to_put @erase_pending, @wl_scheduled, @works, - * @erroneous, @erroneous_peb_count, and @fm_work_scheduled fields + * @erroneous, @erroneous_peb_count, @fm_work_scheduled, @fm_pool, + * and @fm_wl_pool fields * @move_mutex: serializes eraseblock moves * @work_sem: used to wait for all the scheduled works to finish and prevent * new works from being submitted diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c index cb2e571..7730b97 100644 --- a/drivers/mtd/ubi/wl.c +++ b/drivers/mtd/ubi/wl.c @@ -629,24 +629,36 @@ void ubi_refill_pools(struct ubi_device *ubi) */ int ubi_wl_get_peb(struct ubi_device *ubi) { - int ret; + int ret, retried = 0; struct ubi_fm_pool *pool = &ubi->fm_pool; struct ubi_fm_pool *wl_pool = &ubi->fm_wl_pool; - if (!pool->size || !wl_pool->size || pool->used == pool->size || - wl_pool->used == wl_pool->size) +again: + spin_lock(&ubi->wl_lock); + /* We check here also for the WL pool because at this point we can + * refill the WL pool synchronous. */ + if (pool->used == pool->size || wl_pool->used == wl_pool->size) { + spin_unlock(&ubi->wl_lock); ubi_update_fastmap(ubi); - - /* we got not a single free PEB */ - if (!pool->size) - ret = -ENOSPC; - else { spin_lock(&ubi->wl_lock); - ret = pool->pebs[pool->used++]; - prot_queue_add(ubi, ubi->lookuptbl[ret]); + } + + if (pool->used == pool->size) { spin_unlock(&ubi->wl_lock); + if (retried) { + ubi_err(ubi, "Unable to get a free PEB from user WL pool"); + ret = -ENOSPC; + goto out; + } + retried = 1; + goto again; } + ubi_assert(pool->used < pool->size); + ret = pool->pebs[pool->used++]; + prot_queue_add(ubi, ubi->lookuptbl[ret]); + spin_unlock(&ubi->wl_lock); +out: return ret; } @@ -659,7 +671,7 @@ static struct ubi_wl_entry *get_peb_for_wl(struct ubi_device *ubi) struct ubi_fm_pool *pool = &ubi->fm_wl_pool; int pnum; - if (pool->used == pool->size || !pool->size) { + if (pool->used == pool->size) { /* We cannot update the fastmap here because this * function is called in atomic context. * Let's fail here and refill/update it as soon as possible. */
ubi_wl_get_peb() has two problems, it reads the pool size and usage counters without any protection. While reading one value would be perfectly fine it reads multiple values and compares them. This is racy and can lead to incorrect pool handling. Furthermore ubi_update_fastmap() is called without wl_lock held, before incrementing the used counter it needs to be checked again. It could happen that another thread consumed all PEBs from the pool and the counter goes beyond ->size. Signed-off-by: Richard Weinberger <richard@nod.at> --- drivers/mtd/ubi/ubi.h | 3 ++- drivers/mtd/ubi/wl.c | 34 +++++++++++++++++++++++----------- 2 files changed, 25 insertions(+), 12 deletions(-)