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

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

[Martin Townsend]

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

[Heiko Schocher]

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

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

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

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

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

[Martin Townsend]

Hi,

On Tue, Jan 16, 2018 at 5:43 AM, Heiko Schocher <hs@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@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@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@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

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

[Richard Weinberger]

Heiko, Martin,

Am Dienstag, 16. Januar 2018, 10:11:41 CET schrieb Martin Townsend:
> 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:

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

[Martin Townsend]

Hi Richard,

On Tue, Jan 16, 2018 at 1:22 PM, Richard Weinberger
<richard@sigma-star.at> wrote:
> Heiko, Martin,
>
> Am Dienstag, 16. Januar 2018, 10:11:41 CET schrieb Martin Townsend:
>> 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.

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?

>
> For Linux I suggest this fix:
>
> From 48287459cf8717cffac5aed423937cd7ba4360ab Mon Sep 17 00:00:00 2001
> From: Richard Weinberger <richard@nod.at>
> 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 <richard@nod.at>
> Cc: stable at vger.kernel.org
> ---
>  drivers/mtd/ubi/fastmap-wl.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/mtd/ubi/fastmap-wl.c b/drivers/mtd/ubi/fastmap-wl.c
> index 4f0bd6b4422a..69dd21679a30 100644
> --- a/drivers/mtd/ubi/fastmap-wl.c
> +++ b/drivers/mtd/ubi/fastmap-wl.c
> @@ -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

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?

Re: [PATCH] ubi: Fix filesystem corruption on detach when fastmap enabled

[Richard Weinberger]

Martin,

Am Dienstag, 16. Januar 2018, 15:13:04 CET schrieb Martin Townsend:
> > 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.

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

Booting a kernel without Fastmap support. B-)

> > For Linux I suggest this fix:
> > 
> > From 48287459cf8717cffac5aed423937cd7ba4360ab Mon Sep 17 00:00:00 2001
> > From: Richard Weinberger <richard@nod.at>
> > 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 <richard@nod.at>
> > Cc: stable@vger.kernel.org
> > ---
> > 
> >  drivers/mtd/ubi/fastmap-wl.c | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/drivers/mtd/ubi/fastmap-wl.c b/drivers/mtd/ubi/fastmap-wl.c
> > index 4f0bd6b4422a..69dd21679a30 100644
> > --- a/drivers/mtd/ubi/fastmap-wl.c
> > +++ b/drivers/mtd/ubi/fastmap-wl.c
> > @@ -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.
>
> 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

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

[Richard Weinberger]

Martin,

Am Dienstag, 16. Januar 2018, 15:13:04 CET schrieb Martin Townsend:
> > 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.

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

Booting a kernel without Fastmap support. B-)

> > For Linux I suggest this fix:
> > 
> > From 48287459cf8717cffac5aed423937cd7ba4360ab Mon Sep 17 00:00:00 2001
> > From: Richard Weinberger <richard@nod.at>
> > 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 <richard@nod.at>
> > Cc: stable at vger.kernel.org
> > ---
> > 
> >  drivers/mtd/ubi/fastmap-wl.c | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/drivers/mtd/ubi/fastmap-wl.c b/drivers/mtd/ubi/fastmap-wl.c
> > index 4f0bd6b4422a..69dd21679a30 100644
> > --- a/drivers/mtd/ubi/fastmap-wl.c
> > +++ b/drivers/mtd/ubi/fastmap-wl.c
> > @@ -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.
>
> 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

Re: [PATCH] ubi: Fix filesystem corruption on detach when fastmap enabled

[Martin Townsend]

Hi,


On Wed, Jan 17, 2018 at 3:47 PM, Richard Weinberger
<richard@sigma-star.at> wrote:
> Martin,
>
> Am Dienstag, 16. Januar 2018, 15:13:04 CET schrieb Martin Townsend:
>> > 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.
>
>> Not sure if this was recoverable, how do you trigger a full scan?
>
> Booting a kernel without Fastmap support. B-)
>
>> > For Linux I suggest this fix:
>> >
>> > From 48287459cf8717cffac5aed423937cd7ba4360ab Mon Sep 17 00:00:00 2001
>> > From: Richard Weinberger <richard@nod.at>
>> > 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 <richard@nod.at>
>> > Cc: stable@vger.kernel.org
>> > ---
>> >
>> >  drivers/mtd/ubi/fastmap-wl.c | 1 -
>> >  1 file changed, 1 deletion(-)
>> >
>> > diff --git a/drivers/mtd/ubi/fastmap-wl.c b/drivers/mtd/ubi/fastmap-wl.c
>> > index 4f0bd6b4422a..69dd21679a30 100644
>> > --- a/drivers/mtd/ubi/fastmap-wl.c
>> > +++ b/drivers/mtd/ubi/fastmap-wl.c
>> > @@ -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.
>>
>> 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 for clarifying, I'll submit a version 2 patch that replicates
this for U-Boot.

> Thanks,
> //richard
>
> --
> sigma star gmbh - Eduard-Bodem-Gasse 6 - 6020 Innsbruck - Austria
> ATU66964118 - FN 374287y

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

[Martin Townsend]

Hi,


On Wed, Jan 17, 2018 at 3:47 PM, Richard Weinberger
<richard@sigma-star.at> wrote:
> Martin,
>
> Am Dienstag, 16. Januar 2018, 15:13:04 CET schrieb Martin Townsend:
>> > 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.
>
>> Not sure if this was recoverable, how do you trigger a full scan?
>
> Booting a kernel without Fastmap support. B-)
>
>> > For Linux I suggest this fix:
>> >
>> > From 48287459cf8717cffac5aed423937cd7ba4360ab Mon Sep 17 00:00:00 2001
>> > From: Richard Weinberger <richard@nod.at>
>> > 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 <richard@nod.at>
>> > Cc: stable at vger.kernel.org
>> > ---
>> >
>> >  drivers/mtd/ubi/fastmap-wl.c | 1 -
>> >  1 file changed, 1 deletion(-)
>> >
>> > diff --git a/drivers/mtd/ubi/fastmap-wl.c b/drivers/mtd/ubi/fastmap-wl.c
>> > index 4f0bd6b4422a..69dd21679a30 100644
>> > --- a/drivers/mtd/ubi/fastmap-wl.c
>> > +++ b/drivers/mtd/ubi/fastmap-wl.c
>> > @@ -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.
>>
>> 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 for clarifying, I'll submit a version 2 patch that replicates
this for U-Boot.

> Thanks,
> //richard
>
> --
> sigma star gmbh - Eduard-Bodem-Gasse 6 - 6020 Innsbruck - Austria
> ATU66964118 - FN 374287y