[U-Boot] [PATCH] ubi: Fix filesystem corruption on detach when fastmap enabled

[Heiko Schocher]

Hello Martin,

added Richard to cc

Am 15.01.2018 um 13:13 schrieb Martin Townsend:
> Hi Heiko,
> 
> 
> On Mon, Jan 15, 2018 at 11:30 AM, Heiko Schocher <hs at denx.de> wrote:
>> Hello Martin,
>>
>>
>> Am 12.01.2018 um 20:03 schrieb Martin Townsend:
>>>>
>>>>  From d35b7ea298fbd6c9d08b1b7132d43b9289d2b914 Mon Sep 17 00:00:00 2001
>>>
>>> From: Martin Townsend <mtownsend1973 at gmail.com>
>>> 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 <mtownsend1973 at gmail.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 baf4e2d..795ea34 100644
>>> --- a/drivers/mtd/ubi/build.c
>>> +++ b/drivers/mtd/ubi/build.c
>>> @@ -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:
>>> @@ -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:
>>
>> diff --git a/drivers/mtd/ubi/fastmap-wl.c b/drivers/mtd/ubi/fastmap-wl.c
>> index a33d4063e0..2923d21836 100644
>> --- a/drivers/mtd/ubi/fastmap-wl.c
>> +++ b/drivers/mtd/ubi/fastmap-wl.c
>> @@ -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
> 
> That was my original fix so can confirm this also works.

Ok, great.

> 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


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

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

WARNING: 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