[PATCH] mtd-interface: prevent attaching mtd type MTD_UBIVOLUME

[Jan Leupold]

Start swupdate two times from the command line, trying to install
software with handler ubivol. First run will complete, second run will
cause kernel oops (linux-4.0.4 arm) in call of ubi_attach().

The first run will ubi_attach() the ubi partition, a new mtd device of
type MTD_UBIVOLUME is created. Without ubi_detach() this device will
still exist at the beginning of the second run.

With mtd type checking the second and more following runs of swupdate
will succeed.

Signed-off-by: Jan Leupold <leu...@rsi-elektrotechnik.de>
---
corelib/mtd-interface.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/corelib/mtd-interface.c b/corelib/mtd-interface.c
index 96c3f6b..c8a9e45 100644
--- a/corelib/mtd-interface.c
+++ b/corelib/mtd-interface.c
@@ -303,7 +303,8 @@ int scan_mtd_devices (void)
for (i = mtd_info->lowest_mtd_num;
i <= mtd_info->highest_mtd_num; i++) {
if (flash->libubi && !flash->mtd_info[i].skipubi &&
- !flash->mtd_info[i].scanned)
+ !flash->mtd_info[i].scanned &&
+ flash->mtd_info[i].mtd.type != MTD_UBIVOLUME)
scan_ubi_partitions(i);
}
#endif
--
2.1.4


--
_____________________________________________________________
R-S-I Elektrotechnik GmbH & Co. KG
Woelkestrasse 11
D-85301 Schweitenkirchen
Fon: +49 8444 9204-0
Fax: +49 8444 9204-50
www.rsi-elektrotechnik.de

_____________________________________________________________
Amtsgericht Ingolstadt - GmbH: HRB 191328 - KG: HRA 170363
Geschäftsführer: Dr.-Ing. Michael Sorg, Dipl.-Ing. Franz Sorg
USt-IdNr.: DE 128592548

[Stefano Babic]

Hi Jan,

On 10/05/2016 17:54, Jan Leupold wrote:
> Start swupdate two times from the command line, trying to install
> software with handler ubivol. First run will complete, second run will
> cause kernel oops (linux-4.0.4 arm) in call of ubi_attach().

Well, if a user space program can generate a kernel oops, the fix should
be in kernel....

>
> The first run will ubi_attach() the ubi partition, a new mtd device of
> type MTD_UBIVOLUME is created. Without ubi_detach() this device will
> still exist at the beginning of the second run.
>
> With mtd type checking the second and more following runs of swupdate
> will succeed.
>
> Signed-off-by: Jan Leupold <leu...@rsi-elektrotechnik.de>
> ---
> corelib/mtd-interface.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/corelib/mtd-interface.c b/corelib/mtd-interface.c
> index 96c3f6b..c8a9e45 100644
> --- a/corelib/mtd-interface.c
> +++ b/corelib/mtd-interface.c
> @@ -303,7 +303,8 @@ int scan_mtd_devices (void)
> for (i = mtd_info->lowest_mtd_num;
> i <= mtd_info->highest_mtd_num; i++) {
> if (flash->libubi && !flash->mtd_info[i].skipubi &&
> - !flash->mtd_info[i].scanned)
> + !flash->mtd_info[i].scanned &&
> + flash->mtd_info[i].mtd.type != MTD_UBIVOLUME)
> scan_ubi_partitions(i);
> }
> #endif
>

Anyway, I have thought this was fixed with commit
03625bc0abad5b84176c778cec66f162bfc0a6db. A check is done before
attaching a MTD device, and a second attach is avoided:
scan_for_ubi_devices() is called before scan_ubi_partitions(), and the
first one asks with ubi_get_info() if a MTD is already attached.

Best regards,
Stefano Babic

--
=====================================================================
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sba...@denx.de
=====================================================================

[Jan Leupold]

Hi Stefano,

Am 10.05.2016 um 19:11 schrieb Stefano Babic:
> Hi Jan,
>
> On 10/05/2016 17:54, Jan Leupold wrote:
>> Start swupdate two times from the command line, trying to install
>> software with handler ubivol. First run will complete, second run will
>> cause kernel oops (linux-4.0.4 arm) in call of ubi_attach().
> Well, if a user space program can generate a kernel oops, the fix should
> be in kernel....

You are absolutely right, I should check the kernel sources, too.

Maybe I should explain a little more what I have observed:
What we want to do is to update a single ubi volume inside
a ubi partition in /dev/mtd8.
In the first swupdate run ubi_attach("/dev/mtd8")
is executed, all fine and works like a charm.

While executing ubi_attach("/dev/mtd8") another mtd device is created:
/dev/mtd9.
I don't know exactly why this appears as a mtd device, it
must be some kind of representation for the single ubi
volume inside /dev/mtd8 ubi partition.
mtdinfo tells me that the type of /dev/mtd9 is "ubi" (instead
of "nand" for /dev/mtd8). This /dev/mtd9 will not disappear
until ubi_detach() is executed.

On the second run of swupdate the new /dev/mtd9 appears
as "not attached" and so ubi_attach("/dev/mtd9") will be called
... with the corresponding kernel reaction.
That's why I thought the patch has its point: it does not make
sense to scan a ubi volume to contain other ubi volumes, does it?

By the way: I could reproduce the effect with ubiattach
from mtd-utils package ("ubiattach -m 8" followed by
"ubiattach -m 9"). Maybe my mtd-utils is out-of-date? I build
swupdate & mtd-utils using meta-swupdate (master) and poky
(dizzy branch).

Best regards,
Jan

[Stefano Babic]

Hi Jan,

On 10/05/2016 23:37, Jan Leupold wrote:
> Hi Stefano,
>
> Am 10.05.2016 um 19:11 schrieb Stefano Babic:
>> Hi Jan,
>>
>> On 10/05/2016 17:54, Jan Leupold wrote:
>>> Start swupdate two times from the command line, trying to install
>>> software with handler ubivol. First run will complete, second run will
>>> cause kernel oops (linux-4.0.4 arm) in call of ubi_attach().
>> Well, if a user space program can generate a kernel oops, the fix should
>> be in kernel....
> You are absolutely right, I should check the kernel sources, too.

Right.

> Maybe I should explain a little more what I have observed:
> What we want to do is to update a single ubi volume inside
> a ubi partition in /dev/mtd8.
> In the first swupdate run ubi_attach("/dev/mtd8")
> is executed, all fine and works like a charm.

ok

>
> While executing ubi_attach("/dev/mtd8") another mtd device is created:
> /dev/mtd9.

This is something unusual. The MTD subsystem is separate from the UBI
layer, and UBI works on the single MTD device. UBI should not modify the
MTDs and how the device is split. In fact, after a successful ubiattach,
we see /dev/ubictrl and /dev/ubi*, but without changes for MTD. Do you
see MTD9 in /proc/mtd ?

To get a new device, it looks like that mtd_device_register() or
mtd_device_parse_register() in kernel are called.

MTD devices are currently set with DT, and they are not changed.
swupdate does not support repartitioning of MTD device (this leads to
load / unload kernel modules), and partitioning is done only at the UBI
level.

> I don't know exactly why this appears as a mtd device, it
> must be some kind of representation for the single ubi
> volume inside /dev/mtd8 ubi partition.

It should not be. UBI is exported to User Space with a /dev/ubi* and not
/dev/mtd*. I do not know what is happening. It is like /dev/mtd9 is a
generated link (??) to something else (ubi ?).

> mtdinfo tells me that the type of /dev/mtd9 is "ubi" (instead
> of "nand" for /dev/mtd8). This /dev/mtd9 will not disappear
> until ubi_detach() is executed.
>
> On the second run of swupdate the new /dev/mtd9 appears
> as "not attached" and so ubi_attach("/dev/mtd9") will be called
> ... with the corresponding kernel reaction.
> That's why I thought the patch has its point: it does not make
> sense to scan a ubi volume to contain other ubi volumes, does it?

Yes, but that means that /dev/mtd9 is *not* a MTD device as it should
be. swupdate scans volumes just on MTD devices and not on volumes.

>
> By the way: I could reproduce the effect with ubiattach
> from mtd-utils package ("ubiattach -m 8" followed by
> "ubiattach -m 9").

ok - this let's know that there is something strange independently from
the update.

> Maybe my mtd-utils is out-of-date? I build
> swupdate & mtd-utils using meta-swupdate (master) and poky
> (dizzy branch).

This is a weird constellation: dizzy is quite old, and I use master with
Jethro (or at least with fido). Without changing a lot, you can try to
update the mtd-utils in your dizzy branch to a newer version (1.5+).

Best regards,
Stefano

[Stefano Babic]

Hi Jan,

On 11/05/2016 09:00, Stefano Babic wrote:

>> On the second run of swupdate the new /dev/mtd9 appears
>> as "not attached" and so ubi_attach("/dev/mtd9") will be called
>> ... with the corresponding kernel reaction.
>> That's why I thought the patch has its point: it does not make
>> sense to scan a ubi volume to contain other ubi volumes, does it?
>
> Yes, but that means that /dev/mtd9 is *not* a MTD device as it should
> be. swupdate scans volumes just on MTD devices and not on volumes.
>

Have you enabled MTD_UBI_GLUEBI in your kernel ? That could explain the
behavior.

[Jan Leupold]

Hi Stefano,

Am 11.05.2016 um 09:29 schrieb Stefano Babic:

Hi Jan,

On 11/05/2016 09:00, Stefano Babic wrote:

On the second run of swupdate the new /dev/mtd9 appears
as "not attached" and so ubi_attach("/dev/mtd9") will be called
... with the corresponding kernel reaction.
That's why I thought the patch has its point: it does not make
sense to scan a ubi volume to contain other ubi volumes, does it?

Yes, but that means that /dev/mtd9 is *not* a MTD device as it should
be. swupdate scans volumes just on MTD devices and not on volumes.

Have you enabled MTD_UBI_GLUEBI in your kernel ? That could explain the
behavior.

Yes, MTD_UBI_GLUEBI ist set to "y" (for no special reason, will
deactivate it).

Referring to your previous mail:

Yes, I can see /dev/mtd9 in /proc/mtd:

root@tan800:~# cat /proc/mtd
dev:    size   erasesize  name
mtd0: 00100000 00001000 "40000000.sram"
mtd1: 00140000 00020000 "uboot"
mtd2: 00020000 00020000 "uboot_env"
mtd3: 00020000 00020000 "uboot_env_redundant"
mtd4: 00c80000 00020000 "updater"
mtd5: 00200000 00020000 "dummy0"
mtd6: 00200000 00020000 "dummy1"
mtd7: 00200000 00020000 "nv_data"
mtd8: 10000000 00020000 "betrieb"
mtd9: 0f002000 0001f000 "betrieb"

root@tan800:~# mtdinfo --version
mtdinfo 1.5.1

root@tan800:~# mtdinfo /dev/mtd9
mtd9
Name:                           betrieb
Type:                           ubi
Eraseblock size:                126976 bytes, 124.0 KiB
Amount of eraseblocks:          1982 (251666432 bytes, 240.0 MiB)
Minimum input/output unit size: 2048 bytes
Sub-page size:                  2048 bytes
Character device major/minor:   90:18
Bad blocks are allowed:         false
Device is writable:             true

root@tan800:~# ls -l /dev/mtd[8-9]
crw-------    1 root     root       90,  16 May 11 09:26 /dev/mtd8
crw-------    1 root     root       90,  18 May 11 09:26 /dev/mtd9

Best regards,
Jan

-- 
_____________________________________________________________
Jan Leupold
Dr.-Ing.
Entwicklung

Tel.: +49 8444 9204-32
Fax:  +49 8444 9204-50
leu...@rsi-elektrotechnik.de

R-S-I Elektrotechnik GmbH & Co. KG
Woelkestrasse 11
D-85301 Schweitenkirchen

[Stefano Babic]

Hi Jan,

On 11/05/2016 09:49, Jan Leupold wrote:
>> Have you enabled MTD_UBI_GLUEBI in your kernel ? That could explain the
>> behavior.
>
> Yes, MTD_UBI_GLUEBI ist set to "y" (for no special reason, will
> deactivate it).

ok - behavior is clear now.

Fine - I have now understood what is happening. I think it is in any
case safer to merge your patch.

However, it looks like that the case is not adequately in kernel. It
should be interesting to know if this happens with current mainline
(4.5+) or if there is already a patch to fix this issue (I did not find it).

Best regards,
Stefano Babic

[Stefano Babic]

On 10/05/2016 17:54, Jan Leupold wrote:

Applied to master, thanks !