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

Tue Jan 16 09:11:41 UTC 2018

Hi,

On Tue, Jan 16, 2018 at 5:43 AM, Heiko Schocher <hs at denx.de> wrote:
> 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
>
>
> [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".
>

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.

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