ubi: Fix filesystem corruption on detach when fastmap enabled

Quoted 1 line

From d35b7ea298fbd6c9d08b1b7132d43b9289d2b914 Mon Sep 17 00:00:00 2001

From: Martin Townsend <[email protected]>
Date: Fri, 12 Jan 2018 18:59:23 +0000
Subject: [PATCH] ubi: Fix filesystem corruption on detach when fastmap enabled
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

When detaching using "ubi detach" it calls ubi_detach_mtd_dev which calls ubi_update_fastmap twice when fastmap is enabled. The second invocation was corrupting the ubifs as it was called after uif_close. Moved all calls to ubi_wl_close before uif_close.

Signed-off-by Martin Townsend <[email protected]>

---
  drivers/mtd/ubi/build.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

drivers/mtd/ubi/build.c +1 -1

index baf4e2d..795ea34 100644
@@ -1082,9 +1082,9 @@ out_debugfs:
  out_uif:
   get_device(&ubi->dev);
   ubi_assert(ref);
+ ubi_wl_close(ubi);
   uif_close(ubi);
  out_detach:
- ubi_wl_close(ubi);
   ubi_free_internal_volumes(ubi);
   vfree(ubi->vtbl);
  out_free:

Hello Martin,

Am 12.01.2018 um 20:03 schrieb Martin Townsend:

Quoted 38 lines

Quoted 2 lines

From d35b7ea298fbd6c9d08b1b7132d43b9289d2b914 Mon Sep 17 00:00:00 2001

From: Martin Townsend <[email protected]>
Date: Fri, 12 Jan 2018 18:59:23 +0000
Subject: [PATCH] ubi: Fix filesystem corruption on detach when fastmap enabled
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

When detaching using "ubi detach" it calls ubi_detach_mtd_dev which calls ubi_update_fastmap twice when fastmap is enabled. The second invocation was corrupting the ubifs as it was called after uif_close. Moved all calls to ubi_wl_close before uif_close.

Signed-off-by Martin Townsend <[email protected]>

---
  drivers/mtd/ubi/build.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

drivers/mtd/ubi/build.c +1 -1

index baf4e2d..795ea34 100644
@@ -1082,9 +1082,9 @@ out_debugfs:
  out_uif:
   get_device(&ubi->dev);
   ubi_assert(ref);
+ ubi_wl_close(ubi);
   uif_close(ubi);
  out_detach:
- ubi_wl_close(ubi);
   ubi_free_internal_volumes(ubi);
   vfree(ubi->vtbl);
  out_free:

drivers/mtd/ubi/build.c int ubi_detach_mtd_dev(int ubi_num, int anyway) · line 1169

Quoted 10 lines

drivers/mtd/ubi/build.c +1 -1

@@ -1161,9 +1161,9 @@ int ubi_detach_mtd_dev(int ubi_num, int anyway)
   get_device(&ubi->dev);
 
   ubi_debugfs_exit_dev(ubi);
+ ubi_wl_close(ubi);
   uif_close(ubi);
 
- ubi_wl_close(ubi);
   ubi_free_internal_volumes(ubi);
   vfree(ubi->vtbl);
   put_mtd_device(ubi->mtd);

Could you please try the following patch:

drivers/mtd/ubi/fastmap-wl.c +0 -2

index a33d4063e0..2923d21836 100644
@@ -339,8 +339,6 @@ static void ubi_fastmap_close(struct ubi_device *ubi)
 
 #ifndef __UBOOT__
        flush_work(&ubi->fm_work);
-#else
-       update_fastmap_work_fn(ubi);
 #endif
        return_unused_pool_pebs(ubi, &ubi->fm_pool);
        return_unused_pool_pebs(ubi, &ubi->fm_wl_pool);

Your problem is (I think) because U-Boot Code accidentially calls update_fastmap_work_fn(ubi), but we do not need it here anymore, as U-Boot does all UBI work immediately.

bye,
Heiko
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52 Fax: +49-8142-66989-80 Email: hs at denx.de

Hi Heiko,

Quoted 73 lines · Heiko Schocher

Hello Martin,

Am 12.01.2018 um 20:03 schrieb Martin Townsend:

Quoted 38 lines

Quoted 2 lines

From d35b7ea298fbd6c9d08b1b7132d43b9289d2b914 Mon Sep 17 00:00:00 2001

From: Martin Townsend <[email protected]>
Date: Fri, 12 Jan 2018 18:59:23 +0000
Subject: [PATCH] ubi: Fix filesystem corruption on detach when fastmap enabled
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

When detaching using "ubi detach" it calls ubi_detach_mtd_dev which calls ubi_update_fastmap twice when fastmap is enabled. The second invocation was corrupting the ubifs as it was called after uif_close. Moved all calls to ubi_wl_close before uif_close.

Signed-off-by Martin Townsend <[email protected]>

---
   drivers/mtd/ubi/build.c | 4 ++--
   1 file changed, 2 insertions(+), 2 deletions(-)

drivers/mtd/ubi/build.c +1 -1

index baf4e2d..795ea34 100644
@@ -1082,9 +1082,9 @@ out_debugfs:
   out_uif:
    get_device(&ubi->dev);
    ubi_assert(ref);
+ ubi_wl_close(ubi);
    uif_close(ubi);
   out_detach:
- ubi_wl_close(ubi);
    ubi_free_internal_volumes(ubi);
    vfree(ubi->vtbl);
   out_free:

drivers/mtd/ubi/build.c int ubi_detach_mtd_dev(int ubi_num, int anyway) · line 1169

Quoted 10 lines

drivers/mtd/ubi/build.c +1 -1

@@ -1161,9 +1161,9 @@ int ubi_detach_mtd_dev(int ubi_num, int anyway)
    get_device(&ubi->dev);
 
    ubi_debugfs_exit_dev(ubi);
+ ubi_wl_close(ubi);
    uif_close(ubi);
 
- ubi_wl_close(ubi);
    ubi_free_internal_volumes(ubi);
    vfree(ubi->vtbl);
    put_mtd_device(ubi->mtd);

Could you please try the following patch:

drivers/mtd/ubi/fastmap-wl.c +0 -2

index a33d4063e0..2923d21836 100644
@@ -339,8 +339,6 @@ static void ubi_fastmap_close(struct ubi_device *ubi)
 
  #ifndef __UBOOT__
         flush_work(&ubi->fm_work);
-#else
-       update_fastmap_work_fn(ubi);
  #endif
         return_unused_pool_pebs(ubi, &ubi->fm_pool);
         return_unused_pool_pebs(ubi, &ubi->fm_wl_pool);

Your problem is (I think) because U-Boot Code accidentially calls update_fastmap_work_fn(ubi), but we do not need it here anymore, as U-Boot does all UBI work immediately.

Quoted 6 lines

bye,
Heiko
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52 Fax: +49-8142-66989-80 Email: hs at denx.de

That was my original fix so can confirm this also works.

My reasoning for opting for the reordering was: I think the problem is uif_close frees up some UBI data structures so we have to ensure no updating of the filesystem occurs after this. What if ubi_fastmap_close or ubi_wl_close change in future and these changes result in updates to the filesystem, the same problem will occur and for our board it corrupts the UBIFS. So I opted to change the order in build.c.

Hello Martin,

added Richard to cc

Am 15.01.2018 um 13:13 schrieb Martin Townsend:

Quoted 81 lines

Hi Heiko,

Quoted 76 lines · Heiko Schocher

Hello Martin,

Am 12.01.2018 um 20:03 schrieb Martin Townsend:

Quoted 40 lines

Quoted 3 lines

From d35b7ea298fbd6c9d08b1b7132d43b9289d2b914 Mon Sep 17 00:00:00 2001

From: Martin Townsend <[email protected]>
Date: Fri, 12 Jan 2018 18:59:23 +0000
Subject: [PATCH] ubi: Fix filesystem corruption on detach when fastmap enabled
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

When detaching using "ubi detach" it calls ubi_detach_mtd_dev which calls ubi_update_fastmap twice when fastmap is enabled. The second invocation was corrupting the ubifs as it was called after uif_close. Moved all calls to ubi_wl_close before uif_close.

Signed-off-by Martin Townsend <[email protected]>

---
   drivers/mtd/ubi/build.c | 4 ++--
   1 file changed, 2 insertions(+), 2 deletions(-)

drivers/mtd/ubi/build.c +1 -1

index baf4e2d..795ea34 100644
@@ -1082,9 +1082,9 @@ out_debugfs:
   out_uif:
    get_device(&ubi->dev);
    ubi_assert(ref);
+ ubi_wl_close(ubi);
    uif_close(ubi);
   out_detach:
- ubi_wl_close(ubi);
    ubi_free_internal_volumes(ubi);
    vfree(ubi->vtbl);
   out_free:

drivers/mtd/ubi/build.c int ubi_detach_mtd_dev(int ubi_num, int anyway) · line 1169

Quoted 10 lines

drivers/mtd/ubi/build.c +1 -1

@@ -1161,9 +1161,9 @@ int ubi_detach_mtd_dev(int ubi_num, int anyway)
    get_device(&ubi->dev);
 
    ubi_debugfs_exit_dev(ubi);
+ ubi_wl_close(ubi);
    uif_close(ubi);
 
- ubi_wl_close(ubi);
    ubi_free_internal_volumes(ubi);
    vfree(ubi->vtbl);
    put_mtd_device(ubi->mtd);

Could you please try the following patch:

drivers/mtd/ubi/fastmap-wl.c +0 -2

index a33d4063e0..2923d21836 100644
@@ -339,8 +339,6 @@ static void ubi_fastmap_close(struct ubi_device *ubi)
 
  #ifndef __UBOOT__
         flush_work(&ubi->fm_work);
-#else
-       update_fastmap_work_fn(ubi);
  #endif
         return_unused_pool_pebs(ubi, &ubi->fm_pool);
         return_unused_pool_pebs(ubi, &ubi->fm_wl_pool);

Your problem is (I think) because U-Boot Code accidentially calls update_fastmap_work_fn(ubi), but we do not need it here anymore, as U-Boot does all UBI work immediately.

Quoted 9 lines

Quoted 6 lines

bye,
Heiko
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52 Fax: +49-8142-66989-80 Email: hs at denx.de

That was my original fix so can confirm this also works.

Ok, great.

Quoted 7 lines

My reasoning for opting for the reordering was: I think the problem is uif_close frees up some UBI data structures so we have to ensure no updating of the filesystem occurs after this. What if ubi_fastmap_close or ubi_wl_close change in future and these changes result in updates to the filesystem, the same problem will occur and for our board it corrupts the UBIFS. So I opted to change the order in build.c.

Hmm... may Richard can comment here, because your change changes code from linux, so if there is a potentiall problem, we should fix it also in linux (or may this is fixed in mainline linux already?)

BTW: your patch has checkpatch problems, see my weekly tbot tests:

http://xeidos.ddns.net/tbot/id_590/tbot.txt
search for "2018-01-16 02:42" to see the wget command to get your patch
from patchwork
search for "2018-01-16 02:42:19" to see the checkpatch cmd output

[33mWARNING: [0m Possible unwrapped commit description (prefer a maximum 75 chars per line)
#19:
Subject: [PATCH] ubi: Fix filesystem corruption on detach when fastmap enabled

[33mWARNING: [0m please, no spaces at the start of a line #42: FILE: drivers/mtd/ubi/build.c:1085:
+ ubi_wl_close(ubi);$

[33mWARNING: [0m please, no spaces at the start of a line #53: FILE: drivers/mtd/ubi/build.c:1164:
+ ubi_wl_close(ubi);$

Please fix this also in a v2, thanks!

Huch, and search for "2018-01-16 02:42" ... your patch does not apply to mainline U-Boot:

2018-01-16 02:42:20,650:CON    :tbotlib   # tb_ctrl: Applying: ubi: Fix

filesystem corruption on detach when fastmap enabled
Using index info to reconstruct a base tree...
error: patch failed: drivers/mtd/ubi/build.c:1082
error: drivers/mtd/ubi/build.c: patch does not apply
error: Did you hand edit your patch?
It does not apply to blobs recorded in its index.
Patch failed at 0001 ubi: Fix filesystem corruption on detach when fastmap enabled
The copy of the patch that failed is found in: .git/rebase-apply/patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort".

bye,
Heiko
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52 Fax: +49-8142-66989-80 Email: hs at denx.de

Ah. Must be the mail client. Sorry about that I'll setup git send-mail for v2. I'll wait to see what Richard has to say in case there is a better fix.

Heiko, Martin,

Am Dienstag, 16. Januar 2018, 10:11:41 CET schrieb Martin Townsend:

Quoted 3 lines

Ah. Must be the mail client. Sorry about that I'll setup git send-mail for v2. I'll wait to see what Richard has to say in case there is a better fix.

Thanks for letting me know!
Indeed, there is a problem. I'm a little astonished that nobody noticed so far.
Most likely because while detaching UBI in Linux mostly no Fastmap work is scheduled,
and therefore in most cases flush_work(&ubi->fm_work) does nothing.

As you noticed in U-Boot the story is different, you always update the Fastmap upon detach.

Martin, can you please explain what corruption you see?
From reading the code I'd assume that you miss volumes but a full scan would recover everything.

For Linux I suggest this fix:

From 48287459cf8717cffac5aed423937cd7ba4360ab Mon Sep 17 00:00:00 2001
From: Richard Weinberger <[email protected]>
Date: Tue, 16 Jan 2018 14:12:46 +0100
Subject: [PATCH] ubi: fastmap: Don't flush fastmap work on detach

At this point UBI volumes have already been free()'ed and fastmap can no longer access these data structures.

Fixes 74cdaf24004a ("UBI: Fastmap: Fix memory leaks while closing the WL sub-

system")

Signed-off-by Richard Weinberger <[email protected]>

Cc stable at vger.kernel.org

---
 drivers/mtd/ubi/fastmap-wl.c | 1 -
 1 file changed, 1 deletion(-)

drivers/mtd/ubi/fastmap-wl.c +0 -2

index 4f0bd6b4422a..69dd21679a30 100644
@@ -362,7 +362,6 @@ static void ubi_fastmap_close(struct ubi_device *ubi)
 {
        int i;
 
-       flush_work(&ubi->fm_work);
        return_unused_pool_pebs(ubi, &ubi->fm_pool);
        return_unused_pool_pebs(ubi, &ubi->fm_wl_pool);
 
--

2.13.6

In U-Boot you can do the same.

Thanks,
//richard

--
sigma star gmbh - Eduard-Bodem-Gasse 6 - 6020 Innsbruck - Austria ATU66964118 - FN 374287y

Quoted 3 lines

Martin, can you please explain what corruption you see? From reading the code I'd assume that you miss volumes but a full scan would recover everything.

I didn't do much analysis of the corruption I'm afraid. When I tried to reattach the the c->empty flag was set to true and indeed all the EBA tables were reporting an empty filesystem even after a reboot.

Not sure if this was recoverable, how do you trigger a full scan?

Quoted 37 lines

For Linux I suggest this fix:

From 48287459cf8717cffac5aed423937cd7ba4360ab Mon Sep 17 00:00:00 2001
From: Richard Weinberger <[email protected]>
Date: Tue, 16 Jan 2018 14:12:46 +0100
Subject: [PATCH] ubi: fastmap: Don't flush fastmap work on detach

At this point UBI volumes have already been free()'ed and fastmap can no longer access these data structures.

Fixes 74cdaf24004a ("UBI: Fastmap: Fix memory leaks while closing the WL

sub- system")

Signed-off-by Richard Weinberger <[email protected]>

Cc [email protected]

---

 drivers/mtd/ubi/fastmap-wl.c | 1 -
 1 file changed, 1 deletion(-)

drivers/mtd/ubi/fastmap-wl.c +0 -1

index 4f0bd6b4422a..69dd21679a30 100644
@@ -362,7 +362,6 @@ static void ubi_fastmap_close(struct ubi_device *ubi)
 
 {
 
        int i;
 
-       flush_work(&ubi->fm_work);
 

        return_unused_pool_pebs(ubi, &ubi->fm_pool);
        return_unused_pool_pebs(ubi, &ubi->fm_wl_pool);

--
2.13.6

Quoted 1 line

In U-Boot you can do the same.

Richard: This will work but just want to check that ubi_wl_close is supposed to never write out to the filesystem or will never do so in future, if so maybe a something in the comments above ubi_wl_close to ensure this?

Martin,

Am Dienstag, 16. Januar 2018, 15:13:04 CET schrieb Martin Townsend:

Quoted 7 lines

Quoted 3 lines

Martin, can you please explain what corruption you see? From reading the code I'd assume that you miss volumes but a full scan would recover everything.

I didn't do much analysis of the corruption I'm afraid. When I tried to reattach the the c->empty flag was set to true and indeed all the EBA tables were reporting an empty filesystem even after a reboot.

Sounds like what I'd expect.

Quoted 1 line

Not sure if this was recoverable, how do you trigger a full scan?

Booting a kernel without Fastmap support. B-)

Quoted 37 lines

Quoted 37 lines

For Linux I suggest this fix:

From 48287459cf8717cffac5aed423937cd7ba4360ab Mon Sep 17 00:00:00 2001
From: Richard Weinberger <[email protected]>
Date: Tue, 16 Jan 2018 14:12:46 +0100
Subject: [PATCH] ubi: fastmap: Don't flush fastmap work on detach

At this point UBI volumes have already been free()'ed and fastmap can no longer access these data structures.

Fixes 74cdaf24004a ("UBI: Fastmap: Fix memory leaks while closing the WL

sub- system")

Signed-off-by Richard Weinberger <[email protected]>

Cc stable at vger.kernel.org

---

 drivers/mtd/ubi/fastmap-wl.c | 1 -
 1 file changed, 1 deletion(-)

drivers/mtd/ubi/fastmap-wl.c +0 -1

index 4f0bd6b4422a..69dd21679a30 100644
@@ -362,7 +362,6 @@ static void ubi_fastmap_close(struct ubi_device *ubi)
 
 {
 
        int i;
 
-       flush_work(&ubi->fm_work);
 

        return_unused_pool_pebs(ubi, &ubi->fm_pool);
        return_unused_pool_pebs(ubi, &ubi->fm_wl_pool);

--
2.13.6

Quoted 6 lines

Quoted 1 line

In U-Boot you can do the same.

Richard: This will work but just want to check that ubi_wl_close is supposed to never write out to the filesystem or will never do so in future, if so maybe a something in the comments above ubi_wl_close to ensure this?

Hmm, not sure if I got this question.
The filesystem sits above UBI. If we reach ubi_wl_close() all users ontop of UBI are gone. There is no UBIFS mounted anymore.

Thanks,
//richard

--
sigma star gmbh - Eduard-Bodem-Gasse 6 - 6020 Innsbruck - Austria ATU66964118 - FN 374287y