UBI scanning

[Arnout Vandecappelle]

I have yet another problem with the UBI. [Angelo in Cc since he's been dealing
with UBI as well recently.]

If UBI is enabled in the config, then all MTD devices will be scanned and will
be attempted to attach as UBI partitions before the images are installed. This
attaching is problematic for a few reasons.

1. Attaching is not a neutral operation. E.g. if the MTD partition is erased, a
UBI superblock will be written to it. Or if it actually contains a UBI system,
it may enter recovery or scrubbing.

2. Partitions that contain something else than UBI will trigger nasty warnings
in the kernel logs. Also the swupdate log itself contains ERROR traces.

3. Attempting to attach a large partition can take a relatively long time and
unnecessarily slows down swupdate.

In particular the first one I consider bad enough to call it a bug.

There is already a workaround, since you can specify in the config or on the
command line with MTD partitions to blacklist, and recently CONFIG_UBIWHITELIST
was added. But that is really a hack IMO.

In addition, in most cases it's not needed to do this. Either the UBI partition
is already attached and the volume to replace is already present, so attaching
it is not needed. Or the sw-description explicitly specifies on which partition
the UBI volume has to be created with the 'device=' option, so there is no need
to attempt to attach any other partition.

So, to improve this situation I see a few possibilities.

A. In scan_ubi_partitions(), only try attaching if there is a partition
definition that needs it. This is a bit tricky for a few reasons:

a. As far as I understand, the network streaming code will call
scan_mtd_devices() before receiving and parsing the sw-description. I guess that
can be worked around by moving scan_mtd_devices() into the STREAM_WAIT_SIGNATURE
-> STREAM_DATA transition, but I can't say I fully understand that code.

b. To know if a partition definition needs it, we really should check *both*
the partitions definition *and* the images definition - a partition that is not
referred to by a 'type = "ubi"' image doesn't need to be scanned.

B. Remove automatic attaching entirely. The OS is expected to attach UBI
partitions autonomously. Probably, however, there are use cases where attaching
automatically is useful, otherwise Stefano wouldn't have added the feature :-).
Note that scanning is still needed to determine on which UBI device the volume
should be created. If this path is chosen, it should probably be done under a
(default y) Kconfig option.

C. Make automatic attaching a command-line option. This will require existing
users to update their scripts to add this option, so perhaps it should be the
reverse. Could also be coded as --blacklist=all.

D. Only attach if the device to attach is explicitly specify with the 'device='
option in the partitions list. I think this is the cleanest solution that
*should* satisfy all use cases, but it may require existing sw-description files
to be updated. So the "legacy" behaviour should perhaps be made available under
a Kconfig option as well.


I'm prepared to implement any of those proposals, but preferably only one of
them :-). Also, for option A or D, I'll need some help with the network
streaming code. Perhaps Stefano can refactor it first?


Regards,
Arnout


--
Arnout Vandecappelle arnout at mind be
Senior Embedded Software Architect +32-16-286500
Essensium/Mind http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint: 7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Stefano Babic]

Hi Arnout,

On 16/08/2018 15:49, Arnout Vandecappelle wrote:
> I have yet another problem with the UBI. [Angelo in Cc since he's been dealing
> with UBI as well recently.]
>
> If UBI is enabled in the config, then all MTD devices will be scanned and will
> be attempted to attach as UBI partitions before the images are installed. This
> attaching is problematic for a few reasons.
>
> 1. Attaching is not a neutral operation. E.g. if the MTD partition is erased, a
> UBI superblock will be written to it. Or if it actually contains a UBI system,
> it may enter recovery or scrubbing.
>
> 2. Partitions that contain something else than UBI will trigger nasty warnings
> in the kernel logs. Also the swupdate log itself contains ERROR traces.

This is in the best case - it can also make your device unbootable.

If just the first sector is empty, but then there is a bootloader, UBI
startw to write the UBI header and this overwrites the bootloader.

>
> 3. Attempting to attach a large partition can take a relatively long time and
> unnecessarily slows down swupdate.

True, but it is without consequences.

>
> In particular the first one I consider bad enough to call it a bug.
>
> There is already a workaround, since you can specify in the config or on the
> command line with MTD partitions to blacklist, and recently CONFIG_UBIWHITELIST
> was added. But that is really a hack IMO.

Up now, this was considered the solution. You specify at buildtime via
config or at runtime (via command line parameters) which MTD devices are
allowed to be scanned.

>
> In addition, in most cases it's not needed to do this. Either the UBI partition
> is already attached and the volume to replace is already present, so attaching
> it is not needed. Or the sw-description explicitly specifies on which partition
> the UBI volume has to be created with the 'device=' option, so there is no need
> to attempt to attach any other partition.

You are estimating to have just one sw-description (or SWU image) for
your product. In many cases, there are many sw-description each of them
bound to a different image for the product (different features, and so
on). Let's say, one SWU is the "standard" case.

>
> So, to improve this situation I see a few possibilities.
>
> A. In scan_ubi_partitions(), only try attaching if there is a partition
> definition that needs it. This is a bit tricky for a few reasons:
>
> a. As far as I understand, the network streaming code will call
> scan_mtd_devices() before receiving and parsing the sw-description. I guess that
> can be worked around by moving scan_mtd_devices() into the STREAM_WAIT_SIGNATURE
> -> STREAM_DATA transition, but I can't say I fully understand that code.
>
> b. To know if a partition definition needs it, we really should check *both*
> the partitions definition *and* the images definition - a partition that is not
> referred to by a 'type = "ubi"' image doesn't need to be scanned.

This is bad. I considered in design to have *no* dependencies among any
installer. That means, the UBI installer does not depend in any way to
the partitions. Each handler receives as input the image to be
installed, and should not take care of the rest. Each installer, that
means, each handler, is atomic and just care of the image to be installed.

The second point is that it is not true (or not always true) that we
need to attach a partition if it is put into sw-description. Let think
to a wrong sw-description where a partition entry is set for the
bootloader. If sw-description becomes the "master" to decide which
partition should be scanned, the result is that UBI is maybe attached to
the bootloader. This can brick the device. If the MTDs to be scanned is
decided at build time, that means via black - or whitelist, this does
not happen: SWUpdate reports that it cannot find the volume and stops.

The fact that MTDs are scanned moves the responsibility outside
sw-description avoids this case. Even a bad SWU with entries for wrong
devices cannot make this kind of damages.

>
> B. Remove automatic attaching entirely. The OS is expected to attach UBI
> partitions autonomously.

This is not true in many projects. And generally, it is not true when
rescue is running.

> Probably, however, there are use cases where attaching
> automatically is useful, otherwise Stefano wouldn't have added the feature :-).

Exactly.

> Note that scanning is still needed to determine on which UBI device the volume
> should be created. If this path is chosen, it should probably be done under a
> (default y) Kconfig option.

I could agree to disable the scanning via a CONFIG_, but letting the
current behavior as default.

>
> C. Make automatic attaching a command-line option. This will require existing
> users to update their scripts to add this option, so perhaps it should be the
> reverse. Could also be coded as --blacklist=all.

As far as I understand, this is like two lines above, just with command
line parameters. And yes, a blacklist=all could be added.

>
> D. Only attach if the device to attach is explicitly specify with the 'device='
> option in the partitions list.

See above - I repute the device (the hardware) is the master, not
sw-description.

And there are many projects where a "partitions" is not used and not
desired: volumes are set only in the factory. If something went wrong,
it is considered (for these projects) as a hardware issue and device
must be sent back.

Of course, there are other projects where the device must try to repair
itself. I get anyy sort of (different) requirements and I cannot say
that there is a standard way to do.

> I think this is the cleanest solution that
> *should* satisfy all use cases, but it may require existing sw-description files
> to be updated. So the "legacy" behaviour should perhaps be made available under
> a Kconfig option as well.
>
>
> I'm prepared to implement any of those proposals, but preferably only one of
> them :-). Also, for option A or D, I'll need some help with the network
> streaming code. Perhaps Stefano can refactor it first?

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

[Arnout Vandecappelle]

Hi Stefano,

Thank you for your feedback.

It would be nice to hear from other UBI users on the list as well...

On 16/08/2018 19:24, Stefano Babic wrote:
> Hi Arnout,
>
> On 16/08/2018 15:49, Arnout Vandecappelle wrote:
>> I have yet another problem with the UBI. [Angelo in Cc since he's been dealing
>> with UBI as well recently.]
>>
>> If UBI is enabled in the config, then all MTD devices will be scanned and will
>> be attempted to attach as UBI partitions before the images are installed. This
>> attaching is problematic for a few reasons.
>>
>> 1. Attaching is not a neutral operation. E.g. if the MTD partition is erased, a
>> UBI superblock will be written to it. Or if it actually contains a UBI system,
>> it may enter recovery or scrubbing.
>>
>> 2. Partitions that contain something else than UBI will trigger nasty warnings
>> in the kernel logs. Also the swupdate log itself contains ERROR traces.
>
> This is in the best case - it can also make your device unbootable.
>
> If just the first sector is empty, but then there is a bootloader, UBI
> startw to write the UBI header and this overwrites the bootloader.

Yes, that's why I consider case #1 to be a bug.

>> 3. Attempting to attach a large partition can take a relatively long time and
>> unnecessarily slows down swupdate.
>
> True, but it is without consequences.
>
>>
>> In particular the first one I consider bad enough to call it a bug.
>>
>> There is already a workaround, since you can specify in the config or on the
>> command line with MTD partitions to blacklist, and recently CONFIG_UBIWHITELIST
>> was added. But that is really a hack IMO.
>
> Up now, this was considered the solution. You specify at buildtime via
> config or at runtime (via command line parameters) which MTD devices are
> allowed to be scanned.

The problem with that is twofold.

1. The MTD device numbering is not stable (may depend on the kernel version);
solved by allowing black/whitelist to be specified by name in addition to number.

2. You may want to use the same sw-update executable (and same .swu files) for
different hardware with different MTD layout. Specifying MTD by name would also
solve this issue (it would solve it for me).

So this would add an option E, to allow black/whitelist to be specified by name
in addition to by number.

>> In addition, in most cases it's not needed to do this. Either the UBI partition
>> is already attached and the volume to replace is already present, so attaching
>> it is not needed. Or the sw-description explicitly specifies on which partition
>> the UBI volume has to be created with the 'device=' option, so there is no need
>> to attempt to attach any other partition.
>
> You are estimating to have just one sw-description (or SWU image) for
> your product. In many cases, there are many sw-description each of them
> bound to a different image for the product (different features, and so
> on). Let's say, one SWU is the "standard" case.
>
>>
>> So, to improve this situation I see a few possibilities.
>>
>> A. In scan_ubi_partitions(), only try attaching if there is a partition
>> definition that needs it. This is a bit tricky for a few reasons:
>>
>> a. As far as I understand, the network streaming code will call
>> scan_mtd_devices() before receiving and parsing the sw-description. I guess that
>> can be worked around by moving scan_mtd_devices() into the STREAM_WAIT_SIGNATURE
>> -> STREAM_DATA transition, but I can't say I fully understand that code.
>>
>> b. To know if a partition definition needs it, we really should check *both*
>> the partitions definition *and* the images definition - a partition that is not
>> referred to by a 'type = "ubi"' image doesn't need to be scanned.
>
>
> This is bad. I considered in design to have *no* dependencies among any
> installer. That means, the UBI installer does not depend in any way to
> the partitions. Each handler receives as input the image to be
> installed, and should not take care of the rest. Each installer, that
> means, each handler, is atomic and just care of the image to be installed.

So if I understand correctly, what you mean is that the partitions section in
the .swu file should have been handled entirely by the time we get to the
handlers, and any effect of the partitions section should be represented in the
internal data model, correct? That indeed seems to be how it is working at the
moment. Makes sense. So option A is not an option then.

> The second point is that it is not true (or not always true) that we
> need to attach a partition if it is put into sw-description. Let think
> to a wrong sw-description where a partition entry is set for the
> bootloader. If sw-description becomes the "master" to decide which
> partition should be scanned, the result is that UBI is maybe attached to
> the bootloader. This can brick the device.

But that's exactly what happens now, right? Except in case you blacklist or
whitelist at build time, just calling swupdate will brick your device (even if
it is a completely correct .swu file). scan_ubi_volumes() is called
unconditionally and doesn't seem to depend at all on the .swu file.

> If the MTDs to be scanned is
> decided at build time, that means via black - or whitelist, this does
> not happen: SWUpdate reports that it cannot find the volume and stops.
>
> The fact that MTDs are scanned moves the responsibility outside
> sw-description avoids this case. Even a bad SWU with entries for wrong
> devices cannot make this kind of damages.

Agreed.

>> B. Remove automatic attaching entirely. The OS is expected to attach UBI
>> partitions autonomously.
>
> This is not true in many projects. And generally, it is not true when
> rescue is running.

That doesn't make sense for me. IMO, in any sane setup, you have a single UBI
partition and you attach it early on during boot (both in rescue and in
operational). Putting the responsibility at the OS level makes it much easier to
make hardware-specific variances, I think.

But since I can't imagine all the use cases in the world, I can accept that
this is a use case that needs to be supported.

>
>> Probably, however, there are use cases where attaching
>> automatically is useful, otherwise Stefano wouldn't have added the feature :-).
>
> Exactly.
>
>> Note that scanning is still needed to determine on which UBI device the volume
>> should be created. If this path is chosen, it should probably be done under a
>> (default y) Kconfig option.
>
> I could agree to disable the scanning via a CONFIG_, but letting the
> current behavior as default.

Should I conclude from all of the above and below that you want me to implement
option B?

>> C. Make automatic attaching a command-line option. This will require existing
>> users to update their scripts to add this option, so perhaps it should be the
>> reverse. Could also be coded as --blacklist=all.
>
> As far as I understand, this is like two lines above, just with command
> line parameters. And yes, a blacklist=all could be added.

Indeed it is the same as option B but at runtime. I don't like this option very
much, though, because it still requires correct command-line options to operate
correctly, which makes it more difficult to create a generic update script.

>> D. Only attach if the device to attach is explicitly specify with the 'device='
>> option in the partitions list.
>
> See above - I repute the device (the hardware) is the master, not
> sw-description.
>
> And there are many projects where a "partitions" is not used and not
> desired: volumes are set only in the factory. If something went wrong,
> it is considered (for these projects) as a hardware issue and device
> must be sent back.

The ability to create new volumes during update is essential for me. I have had
many situations in the past where after a few years you need something more than
we had originally. For example, back in the day when ARM platforms moved to DTB,
you suddenly need a DTB volume as well. One key feature of swupdate for me is
that it is possible to do such changes with a simple .swu file instead of
requiring error-prone homegrown update scripts.

> Of course, there are other projects where the device must try to repair
> itself. I get anyy sort of (different) requirements and I cannot say
> that there is a standard way to do.

So for me self-repairing is not a requirement (it's nice to have, but in
practice when it is needed it probably won't work correctly anyway). The
requirement is to have the possibility to change as much as possible during update.

>
>> I think this is the cleanest solution that
>> *should* satisfy all use cases, but it may require existing sw-description files
>> to be updated. So the "legacy" behaviour should perhaps be made available under
>> a Kconfig option as well.
>>
>>
>> I'm prepared to implement any of those proposals, but preferably only one of
>> them :-). Also, for option A or D, I'll need some help with the network
>> streaming code. Perhaps Stefano can refactor it first?

Could you make it explicit which option you'd prefer me to implement? B, C, D
or the new E?

Regards,
Arnout

>
>
> Best regards,
> Stefano Babic
>

--

[Arnout Vandecappelle]

On 21/08/2018 13:02, Arnout Vandecappelle wrote:
> Could you make it explicit which option you'd prefer me to implement? B, C, D
> or the new E?

 So I've implemented option B [1].


 Regards,
 Arnout

[1] http://patchwork.ozlabs.org/project/swupdate/list/?series=61770

[Stefano Babic]

On 22/08/2018 12:35, Arnout Vandecappelle wrote:
>
> On 21/08/2018 13:02, Arnout Vandecappelle wrote:
>> Could you make it explicit which option you'd prefer me to implement? B, C, D
>> or the new E?
>
>
>  So I've implemented option B [1].
>

Fine with me.

Best regards,
Stefano Babic

>
>  Regards,
>  Arnout
>
> [1] http://patchwork.ozlabs.org/project/swupdate/list/?series=61770
>


--