| Message ID | 1414586758-9972-8-git-send-email-richard@nod.at |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, 2014-10-29 at 13:45 +0100, 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. > It could happen that another thread consumed all PEBs from the > pool and the counter goes beyond ->size. So wl_lock protects the 'pool->*' variables? Could you please add this information to ubi.h. Namely, in the huge comment above 'struct device' we document each lock, and we list the variables the lock protects. > - if (!pool->size || !wl_pool->size || pool->used == pool->size || > - wl_pool->used == wl_pool->size) > +again: > + spin_lock(&ubi->wl_lock); Is it possible to add a little comment here which translates the condition below into English? > + if (!pool->size || !wl_pool->size || pool->used >= pool->size || > + wl_pool->used >= wl_pool->size) { > + spin_unlock(&ubi->wl_lock); > ubi_update_fastmap(ubi); > -
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c index a06e31e..5f8d2ff 100644 --- a/drivers/mtd/ubi/wl.c +++ b/drivers/mtd/ubi/wl.c @@ -626,24 +626,34 @@ 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); + if (!pool->size || !wl_pool->size || 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 || !pool->size) { spin_unlock(&ubi->wl_lock); + if (retried) { + ubi_err("Unable to get a free PEB from user WL pool"); + ret = -ENOSPC; + goto out; + } + retried = 1; + goto again; } + ret = pool->pebs[pool->used++]; + prot_queue_add(ubi, ubi->lookuptbl[ret]); + spin_unlock(&ubi->wl_lock); +out: return ret; }
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/wl.c | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-)