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

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

Cheers, Martin