[RFC PATCH 0/4] UBI: Fastmap: Some cleanup and check if a vol exists when attaching

[RFC PATCH 0/4] UBI: Fastmap: Some cleanup and check if a vol exists when attaching

[Sheng Yong]

Hi, folks,

PATCH 1-3 do some cleanup.

PATCH 4 checks if a volume already exists when fastmap doing attach.
ubi_write_fastmap() does not guarantee if two volumes have the same vol
ID. So fastmap should check it when attaching, and if something is wrong,
scan_all() could try to attach instead of fastmap.

thanks,
Sheng

Sheng Yong (4):
  UBI: Fastmap: Use max() to get the larger value
  UBI: Fastmap: Remove unnecessary `\'
  UBI: Fastmap: Rename variables to make them meaningful
  UBI: Fastmap: Do not add vol if it already exists

 drivers/mtd/ubi/build.c   |  4 +--
 drivers/mtd/ubi/fastmap.c | 81 +++++++++++++++++++++++++----------------------
 2 files changed, 46 insertions(+), 39 deletions(-)

-- 
1.8.3.4

[RFC PATCH 1/4] UBI: Fastmap: Use max() to get the larger value

[Sheng Yong]

Signed-off-by: Sheng Yong <shengyong1@huawei.com>
---
 drivers/mtd/ubi/build.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index 9690cf9..90a03c8 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -947,8 +947,8 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
 	 */
 	ubi->fm_pool.max_size = min(((int)mtd_div_by_eb(ubi->mtd->size,
 		ubi->mtd) / 100) * 5, UBI_FM_MAX_POOL_SIZE);
-	if (ubi->fm_pool.max_size < UBI_FM_MIN_POOL_SIZE)
-		ubi->fm_pool.max_size = UBI_FM_MIN_POOL_SIZE;
+	ubi->fm_pool.max_size = max(ubi->fm_pool.max_size,
+		UBI_FM_MIN_POOL_SIZE);
 
 	ubi->fm_wl_pool.max_size = ubi->fm_pool.max_size / 2;
 	ubi->fm_disabled = !fm_autoconvert;
-- 
1.8.3.4

[RFC PATCH 2/4] UBI: Fastmap: Remove unnecessary `\'

[Sheng Yong]

Signed-off-by: Sheng Yong <shengyong1@huawei.com>
---
 drivers/mtd/ubi/fastmap.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index 02a6de2..696a4f9 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -88,13 +88,13 @@ size_t ubi_calc_fm_size(struct ubi_device *ubi)
 {
 	size_t size;
 
-	size = sizeof(struct ubi_fm_sb) + \
-		sizeof(struct ubi_fm_hdr) + \
-		sizeof(struct ubi_fm_scan_pool) + \
-		sizeof(struct ubi_fm_scan_pool) + \
-		(ubi->peb_count * sizeof(struct ubi_fm_ec)) + \
-		(sizeof(struct ubi_fm_eba) + \
-		(ubi->peb_count * sizeof(__be32))) + \
+	size = sizeof(struct ubi_fm_sb) +
+		sizeof(struct ubi_fm_hdr) +
+		sizeof(struct ubi_fm_scan_pool) +
+		sizeof(struct ubi_fm_scan_pool) +
+		(ubi->peb_count * sizeof(struct ubi_fm_ec)) +
+		(sizeof(struct ubi_fm_eba) +
+		(ubi->peb_count * sizeof(__be32))) +
 		sizeof(struct ubi_fm_volhdr) * UBI_MAX_VOLUMES;
 	return roundup(size, ubi->leb_size);
 }
-- 
1.8.3.4

[RFC PATCH 3/4] UBI: Fastmap: Rename variables to make them meaningful

[Sheng Yong]

s/fmpl1/fmpl
s/fmpl2/fmpl_wl

Add "WL" to the error message when wrong WL pool magic number is detected.

Signed-off-by: Sheng Yong <shengyong1@huawei.com>
---
 drivers/mtd/ubi/fastmap.c | 58 +++++++++++++++++++++++------------------------
 1 file changed, 29 insertions(+), 29 deletions(-)

diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index 696a4f9..e3d8294 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -601,7 +601,7 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
 	struct ubi_ainf_peb *aeb, *tmp_aeb, *_tmp_aeb;
 	struct ubi_fm_sb *fmsb;
 	struct ubi_fm_hdr *fmhdr;
-	struct ubi_fm_scan_pool *fmpl1, *fmpl2;
+	struct ubi_fm_scan_pool *fmpl, *fmpl_wl;
 	struct ubi_fm_ec *fmec;
 	struct ubi_fm_volhdr *fmvhdr;
 	struct ubi_fm_eba *fm_eba;
@@ -631,30 +631,30 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
 		goto fail_bad;
 	}
 
-	fmpl1 = (struct ubi_fm_scan_pool *)(fm_raw + fm_pos);
-	fm_pos += sizeof(*fmpl1);
+	fmpl = (struct ubi_fm_scan_pool *)(fm_raw + fm_pos);
+	fm_pos += sizeof(*fmpl);
 	if (fm_pos >= fm_size)
 		goto fail_bad;
-	if (be32_to_cpu(fmpl1->magic) != UBI_FM_POOL_MAGIC) {
+	if (be32_to_cpu(fmpl->magic) != UBI_FM_POOL_MAGIC) {
 		ubi_err(ubi, "bad fastmap pool magic: 0x%x, expected: 0x%x",
-			be32_to_cpu(fmpl1->magic), UBI_FM_POOL_MAGIC);
+			be32_to_cpu(fmpl->magic), UBI_FM_POOL_MAGIC);
 		goto fail_bad;
 	}
 
-	fmpl2 = (struct ubi_fm_scan_pool *)(fm_raw + fm_pos);
-	fm_pos += sizeof(*fmpl2);
+	fmpl_wl = (struct ubi_fm_scan_pool *)(fm_raw + fm_pos);
+	fm_pos += sizeof(*fmpl_wl);
 	if (fm_pos >= fm_size)
 		goto fail_bad;
-	if (be32_to_cpu(fmpl2->magic) != UBI_FM_POOL_MAGIC) {
-		ubi_err(ubi, "bad fastmap pool magic: 0x%x, expected: 0x%x",
-			be32_to_cpu(fmpl2->magic), UBI_FM_POOL_MAGIC);
+	if (be32_to_cpu(fmpl_wl->magic) != UBI_FM_POOL_MAGIC) {
+		ubi_err(ubi, "bad fastmap WL pool magic: 0x%x, expected: 0x%x",
+			be32_to_cpu(fmpl_wl->magic), UBI_FM_POOL_MAGIC);
 		goto fail_bad;
 	}
 
-	pool_size = be16_to_cpu(fmpl1->size);
-	wl_pool_size = be16_to_cpu(fmpl2->size);
-	fm->max_pool_size = be16_to_cpu(fmpl1->max_size);
-	fm->max_wl_pool_size = be16_to_cpu(fmpl2->max_size);
+	pool_size = be16_to_cpu(fmpl->size);
+	wl_pool_size = be16_to_cpu(fmpl_wl->size);
+	fm->max_pool_size = be16_to_cpu(fmpl->max_size);
+	fm->max_wl_pool_size = be16_to_cpu(fmpl_wl->max_size);
 
 	if (pool_size > UBI_FM_MAX_POOL_SIZE || pool_size < 0) {
 		ubi_err(ubi, "bad pool size: %i", pool_size);
@@ -796,11 +796,11 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
 		}
 	}
 
-	ret = scan_pool(ubi, ai, fmpl1->pebs, pool_size, &max_sqnum, &free);
+	ret = scan_pool(ubi, ai, fmpl->pebs, pool_size, &max_sqnum, &free);
 	if (ret)
 		goto fail;
 
-	ret = scan_pool(ubi, ai, fmpl2->pebs, wl_pool_size, &max_sqnum, &free);
+	ret = scan_pool(ubi, ai, fmpl_wl->pebs, wl_pool_size, &max_sqnum, &free);
 	if (ret)
 		goto fail;
 
@@ -1083,7 +1083,7 @@ static int ubi_write_fastmap(struct ubi_device *ubi,
 	void *fm_raw;
 	struct ubi_fm_sb *fmsb;
 	struct ubi_fm_hdr *fmh;
-	struct ubi_fm_scan_pool *fmpl1, *fmpl2;
+	struct ubi_fm_scan_pool *fmpl, *fmpl_wl;
 	struct ubi_fm_ec *fec;
 	struct ubi_fm_volhdr *fvh;
 	struct ubi_fm_eba *feba;
@@ -1141,25 +1141,25 @@ static int ubi_write_fastmap(struct ubi_device *ubi,
 	erase_peb_count = 0;
 	vol_count = 0;
 
-	fmpl1 = (struct ubi_fm_scan_pool *)(fm_raw + fm_pos);
-	fm_pos += sizeof(*fmpl1);
-	fmpl1->magic = cpu_to_be32(UBI_FM_POOL_MAGIC);
-	fmpl1->size = cpu_to_be16(ubi->fm_pool.size);
-	fmpl1->max_size = cpu_to_be16(ubi->fm_pool.max_size);
+	fmpl = (struct ubi_fm_scan_pool *)(fm_raw + fm_pos);
+	fm_pos += sizeof(*fmpl);
+	fmpl->magic = cpu_to_be32(UBI_FM_POOL_MAGIC);
+	fmpl->size = cpu_to_be16(ubi->fm_pool.size);
+	fmpl->max_size = cpu_to_be16(ubi->fm_pool.max_size);
 
 	for (i = 0; i < ubi->fm_pool.size; i++) {
-		fmpl1->pebs[i] = cpu_to_be32(ubi->fm_pool.pebs[i]);
+		fmpl->pebs[i] = cpu_to_be32(ubi->fm_pool.pebs[i]);
 		set_seen(ubi, ubi->fm_pool.pebs[i], seen_pebs);
 	}
 
-	fmpl2 = (struct ubi_fm_scan_pool *)(fm_raw + fm_pos);
-	fm_pos += sizeof(*fmpl2);
-	fmpl2->magic = cpu_to_be32(UBI_FM_POOL_MAGIC);
-	fmpl2->size = cpu_to_be16(ubi->fm_wl_pool.size);
-	fmpl2->max_size = cpu_to_be16(ubi->fm_wl_pool.max_size);
+	fmpl_wl = (struct ubi_fm_scan_pool *)(fm_raw + fm_pos);
+	fm_pos += sizeof(*fmpl_wl);
+	fmpl_wl->magic = cpu_to_be32(UBI_FM_POOL_MAGIC);
+	fmpl_wl->size = cpu_to_be16(ubi->fm_wl_pool.size);
+	fmpl_wl->max_size = cpu_to_be16(ubi->fm_wl_pool.max_size);
 
 	for (i = 0; i < ubi->fm_wl_pool.size; i++) {
-		fmpl2->pebs[i] = cpu_to_be32(ubi->fm_wl_pool.pebs[i]);
+		fmpl_wl->pebs[i] = cpu_to_be32(ubi->fm_wl_pool.pebs[i]);
 		set_seen(ubi, ubi->fm_wl_pool.pebs[i], seen_pebs);
 	}
 
-- 
1.8.3.4

[RFC PATCH 4/4] UBI: Fastmap: Do not add vol if it already exists

[Sheng Yong]

When writing fastmap, there is no guarantee that two same vol ID are
written to flash by mistake. Although this unlikely happens, we still
should check it when attaching.

Signed-off-by: Sheng Yong <shengyong1@huawei.com>
---
 drivers/mtd/ubi/fastmap.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index e3d8294..9eee904 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -192,8 +192,10 @@ static struct ubi_ainf_volume *add_vol(struct ubi_attach_info *ai, int vol_id,
 
 		if (vol_id > av->vol_id)
 			p = &(*p)->rb_left;
-		else
+		else if (vol_id < av->vol_id)
 			p = &(*p)->rb_right;
+		else
+			return ERR_PTR(-EINVAL);
 	}
 
 	av = kmalloc(sizeof(struct ubi_ainf_volume), GFP_KERNEL);
@@ -748,6 +750,11 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
 
 		if (!av)
 			goto fail_bad;
+		if (PTR_ERR(av) == -EINVAL) {
+			ubi_err(ubi, "volume (ID %i) already exists",
+				fmvhdr->vol_id);
+			goto fail_bad;
+		}
 
 		ai->vols_found++;
 		if (ai->highest_vol_id < be32_to_cpu(fmvhdr->vol_id))
-- 
1.8.3.4

Re: [RFC PATCH 4/4] UBI: Fastmap: Do not add vol if it already exists

[Richard Weinberger]

Am 21.05.2015 um 11:56 schrieb Sheng Yong:
> When writing fastmap, there is no guarantee that two same vol ID are
> written to flash by mistake. Although this unlikely happens, we still
> should check it when attaching.

Not sure if I understand the error scenario.
How can this happen?

Did you hit that care or is this theoretical?

Thanks,
//richard

Re: [RFC PATCH 4/4] UBI: Fastmap: Do not add vol if it already exists

[Richard Weinberger]

Am 21.05.2015 um 12:16 schrieb Richard Weinberger:
> Am 21.05.2015 um 11:56 schrieb Sheng Yong:
>> When writing fastmap, there is no guarantee that two same vol ID are
>> written to flash by mistake. Although this unlikely happens, we still
>> should check it when attaching.
> 
> Not sure if I understand the error scenario.
> How can this happen?

...thought a bit more about that.
I think your fix is correct but unless I'm very
mistaken one can hit the issue only by modifying the fastmap
by hand.

Thanks,
//richard

Re: [RFC PATCH 4/4] UBI: Fastmap: Do not add vol if it already exists

[Sheng Yong]

Hi, Richard

On 5/22/2015 4:19 AM, Richard Weinberger wrote:
> Am 21.05.2015 um 12:16 schrieb Richard Weinberger:
>> Am 21.05.2015 um 11:56 schrieb Sheng Yong:
>>> When writing fastmap, there is no guarantee that two same vol ID are
>>> written to flash by mistake. Although this unlikely happens, we still
>>> should check it when attaching.
>>
>> Not sure if I understand the error scenario.
>> How can this happen?
I didn't hit the case, this was just found by codewalking. After I read
more code and though about more unclean reboot and ECC scenario, I agree
with you that there is no way to have two same vol_id saved in different
slots in ubi->volumes[], unless modify it by hand :( .  Thank you for
pointing it out.

thanks,
Sheng
> 
> ...thought a bit more about that.
> I think your fix is correct but unless I'm very
> mistaken one can hit the issue only by modifying the fastmap
> by hand.
> 
> Thanks,
> //richard
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
> 
> 

Re: [RFC PATCH 4/4] UBI: Fastmap: Do not add vol if it already exists

[Richard Weinberger]

Am 26.05.2015 um 10:53 schrieb Sheng Yong:
> Hi, Richard
> 
> On 5/22/2015 4:19 AM, Richard Weinberger wrote:
>> Am 21.05.2015 um 12:16 schrieb Richard Weinberger:
>>> Am 21.05.2015 um 11:56 schrieb Sheng Yong:
>>>> When writing fastmap, there is no guarantee that two same vol ID are
>>>> written to flash by mistake. Although this unlikely happens, we still
>>>> should check it when attaching.
>>>
>>> Not sure if I understand the error scenario.
>>> How can this happen?
> I didn't hit the case, this was just found by codewalking. After I read
> more code and though about more unclean reboot and ECC scenario, I agree
> with you that there is no way to have two same vol_id saved in different
> slots in ubi->volumes[], unless modify it by hand :( .  Thank you for
> pointing it out.

Okay. :)
Please resend your patch with a proper changelog where you explain
that this issue can only be triggered by manually editing the on-flash
data.

Thanks,
//richard