imagemap: fixes from the v1 review
Subsystems (2)
cover imagemap: fixes from the v1 review
Simon Glass reviewed the v1 posting of the imagemap series ([email protected]) after v2 had already been applied, so his comments arrive against code that is now in next. This series carries the correctness half of them; a second series follows with the structural and cosmetic half.
Tested on sandbox: every commit builds, "ut imagemap" passes 13/13 and survives "ut -r3", and a full test.py run shows the same 11 failures and 20 errors as unmodified next, all of them EFI-secboot tooling, ext4 and config-rebuild tests unrelated to this code. Also built with CONFIG_IMAGEMAP=n.
Daniel Golle (11):
mtd: bind the block device without leaking on failure
cmd: ubi: report a failed UBI block device bind
cmd: ubi: unbind the block device when detaching UBI
boot: imagemap: free the reservation a replaced record owned
boot: imagemap: do not resize a region the caller owns
boot: imagemap: read only the bytes an extend adds
boot: fit: unmap the load address after a storage read
boot: fit: keep the load message for RAM-backed images
boot: bootm: release the imagemap on every failing exit
test: boot: imagemap: detach UBI when the test is done
test: boot: imagemap: reset driver-model state between tests
boot/bootm.c | 9 +++--- boot/image-fit.c | 7 +++-- boot/imagemap.c | 55 ++++++++++++++++++++++++--------- cmd/ubi.c | 67 +++++++++++++++++++++++++++-------------- drivers/mtd/mtdcore.c | 37 ++++++++++++++++------- drivers/mtd/ubi/block.c | 4 ++- include/ubi_uboot.h | 4 +-- test/boot/imagemap.c | 36 ++++++++++++++-------- 8 files changed, 147 insertions(+), 72 deletions(-)
base-commit: 2b8902913c19eb581fb0d567201c09b8fa46ac74
Signature · 1 line
2.55.0
PATCH 1/11 mtd: bind the block device without leaking on failure
add_mtd_device() allocates a slot to hold the mtd_info pointer that mtd_bind() keeps, but drops the return value. mtd_bind() only logs and returns on failure, so the slot leaks. A failed kmalloc() is ignored too, leaving the master without a block device and no hint why its partitions are unreachable.
Move the bind into a helper that frees the slot when mtd_bind() fails and warns with the errno in both cases.
--- drivers/mtd/mtdcore.c | 37 ++++++++++++++++++++++++++----------- 1 file changed, 26 insertions(+), 11 deletions(-)
drivers/mtd/mtdcore.c +26 -11
@@ -396,6 +396,31 @@ static struct device_type mtd_devtype = { };#endif +static void mtd_blk_bind_master (struct mtd_info *mtd) +{ +struct mtd_info **mtdp; +int ret; + +/* +* mtd_bind() keeps the passed pointer, so it needs storage that lives +* as long as the block device; an MTD master is never removed. +*/ + mtdp =kmalloc (sizeof (*mtdp),GFP_KERNEL ); +if (!mtdp) { + ret = -ENOMEM ; + goto warn; + } + + *mtdp = mtd; + ret =mtd_bind (mtd->dev, mtdp); +if (!ret) +return ; + +kfree (mtdp); +warn: +pr_warn ("mtd: %s: cannot bind a block device: %d\n" , mtd->name, ret); +} +/** * add_mtd_device - register an MTD device * @mtd: pointer to new MTD device info structure
@@ -504,18 +529,8 @@ int add_mtd_device(struct mtd_info *mtd)* NAND is excluded: it needs UBI, which brings its own block * device. Partitions inherit the master's block device via the * "mtd" partition type, so only masters are bound. -* -* mtd_bind() keeps a pointer to the passed mtd_info pointer, so -* it needs storage that lives as long as the block device; the -* MTD master is never removed in practice, so a small heap slot -* is fine. */ -struct mtd_info **mtdp =kmalloc (sizeof (*mtdp),GFP_KERNEL ); - -if (mtdp) { - *mtdp = mtd; -mtd_bind (mtd->dev, mtdp); - } +mtd_blk_bind_master (mtd); }/* We _know_ we aren't being removed, because
Signature · 1 line
2.55.0
Re: [PATCH 01/11] mtd: bind the block device without leaking on failure
Hi Daniel,
Quoted 19 lines · Daniel Golle
mtd: bind the block device without leaking on failure
add_mtd_device()allocates a slot to hold themtd_infopointer thatmtd_bind()keeps, but drops the return value.mtd_bind()only logs and returns on failure, so the slot leaks. A failedkmalloc()is ignored too, leaving the master without a block device and no hint why its partitions are unreachable.Move the bind into a helper that frees the slot when
mtd_bind()fails and warns with the errno in both cases.Fixes e8a7453824b7 ("mtd: bind an mtd_blk device for non-NAND MTD masters")Signed-off-by Daniel Golle <[email protected]>
drivers/mtd/mtdcore.c| 37 ++++++++++++++++++++++++++-----------1 file changed, 26 insertions(+), 11 deletions(-)
drivers/mtd/mtdcore.c static struct device_type mtd_devtype = { · line 399
Quoted 4 lines
drivers/mtd/mtdcore.c +4
-0@@ -396,6 +396,31 @@ static struct device_type mtd_devtype = { +/* +* mtd_bind() keeps the passed pointer, so it needs storage that lives +* as long as the block device; an MTD master is never removed. +*/
I don't think this holds for SPI flash. With DM_SPI_FLASH, 'sf probe' calls device_remove() on the existing flash device before probing it again (cmd/sf.c:135). spi_flash_std_remove() then calls spi_flash_mtd_unregister() -> del_mtd_device(), and the new probe calls add_mtd_device() again. Neither del_mtd_device() nor device_remove() unbinds the mtd_blk child, so each 'sf probe' allocates another slot and binds a second mtd_blk device under the same flash device, while the old one still points at the old slot. That leak is more frequent than the mtd_bind() failure fixed here. Please can you either skip the bind when mtd->dev already has a UCLASS_BLK child (device_find_first_child_by_uclass()), or unbind the block device and free the slot in del_mtd_device(), as patch 3 does for UBI? Either way, please update the comment.
drivers/mtd/mtdcore.c static struct device_type mtd_devtype = { · line 398
Quoted 5 lines
drivers/mtd/mtdcore.c +3
-0@@ -396,6 +396,31 @@ static struct device_type mtd_devtype = { +kfree (mtdp); +warn: +pr_warn ("mtd: %s: cannot bind a block device: %d\n" , mtd->name, ret);
mtd_bind() already does pr_err("Cannot create block device\n") on failure, so this prints two lines for one failure. Not a big deal, but you could drop the message from mtd_bind() (the SPI NAND caller in nand/spi/core.c already checks the return value) and keep this one, since it has the name and errno.
Regards,
Simon
PATCH 2/11 cmd: ubi: report a failed UBI block device bind
In reply to Daniel Golleubi_blk_bind_once() drops the ubi_bind() return value, so when the bind fails the attach still reports success and the caller finds no ubi_blk device with nothing to explain it.
Report the errno. The attach itself is not failed: UBI is available either way and the block device is an optional view of it, so a UBIFS user must not lose the partition because the block layer could not be set up.
--- cmd/ubi.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
cmd/ubi.c +7 -1
@@ -676,7 +676,9 @@ int ubi_detach(void)#if CONFIG_IS_ENABLED (UBI_BLOCK )static void ubi_blk_bind_once (void ) { +struct udevice *parent;struct udevice *dev; +int ret;/* * A single ubi_blk device serves all volumes of the attached UBI
@@ -690,7 +692,11 @@ static void ubi_blk_bind_once(void) return; } -ubi_bind (ubi && ubi->mtd && ubi->mtd->dev ? ubi->mtd->dev :dm_root ()); + parent = ubi && ubi->mtd && ubi->mtd->dev ? ubi->mtd->dev :dm_root (); + + ret =ubi_bind (parent); +if (ret) +printf ("Cannot create UBI block device: %d\n" , ret); }#else static inline void ubi_blk_bind_once (void ) { }
Signature · 1 line
2.55.0
Re: [PATCH 02/11] cmd: ubi: report a failed UBI block device bind
Hi Daniel,
Quoted 5 lines · Daniel Golle
cmd: ubi: report a failed UBI block device bind
ubi_blk_bind_once()drops theubi_bind()return value, so when the bind fails the attach still reports success and the caller finds noubi_blkdevice with nothing to explain it.
Not quite. The only failure path in ubi_bind() is blk_create_devicef(), which already calls pr_err("Cannot create block device") at the default LOGLEVEL, so the user does get a message today. What it lacks is the errno and any mention of UBI. Please can you reword this to match?
Quoted 14 lines
Report the errno. The attach itself is not failed: UBI is available either way and the block device is an optional view of it, so a UBIFS user must not lose the partition because the block layer could not be set up.
Fixes dec405d1653a ("cmd: ubi: create a ubi_blk device when attaching UBI")Signed-off-by Daniel Golle <[email protected]>
cmd/ubi.c| 8 +++++++-1 file changed, 7 insertions(+), 1 deletion(-)
cmd/ubi.c static void ubi_blk_bind_once(void) · line 694
Quoted 3 lines
cmd/ubi.c +3
-0@@ -690,7 +692,11 @@ static void ubi_blk_bind_once(void) + ret =ubi_bind (parent); +if (ret) +printf ("Cannot create UBI block device: %d\n" , ret);
A failure now prints two messages, and since the pr_err() in ubi_bind() has no trailing newline they run together:
Cannot create block deviceCannot create UBI block device: -12
I suggest dropping the pr_err() from ubi_bind() so the caller does the reporting, as your patch 1 does in mtd_blk_bind_master(). Patch 3 already changes ubi_bind(), so it could go there or here. Alternatively, keep the message in ubi_bind(), add the errno and a newline, and leave this caller silent. What do you think?
I agree with not failing the attach.
Regards,
Simon
PATCH 3/11 cmd: ubi: unbind the block device when detaching UBI
In reply to Daniel GolleThe ubi_blk device is parented to the MTD device UBI was attached to, but ubi_detach() leaves it bound. After 'ubi part A; ubi detach; ubi part B' the device still hangs off A's MTD while UBI sits on B, so the driver-model tree describes a parent that is no longer in use and A's MTD can no longer be removed. Reads happen to keep working only because get_ubi_device() always returns ubi_devices[0].
Return the new device from ubi_bind(), remember it, and unbind it in ubi_detach() so the next attach binds a fresh one under the right parent. Tracking the device also replaces the scan of every UCLASS_BLK device that was used to find it.
--- cmd/ubi.c | 73 +++++++++++++++++++++++++---------------- drivers/mtd/ubi/block.c | 4 ++- include/ubi_uboot.h | 4 +-- 3 files changed, 49 insertions(+), 32 deletions(-)
cmd/ubi.c +44 -29
@@ -21,6 +21,7 @@#include <nand.h> #include <onenand_uboot.h> #include <dm/device.h> +#include <dm/device-internal.h> #include <dm/devres.h> #include <dm/root.h> #include <dm/uclass-internal.h>
@@ -650,6 +651,47 @@ static int ubi_set_skip_check(const char *volume, bool skip_check)return ubi_change_vtbl_record (ubi, vol->vol_id, &vtbl_rec); } +#if CONFIG_IS_ENABLED (UBI_BLOCK ) +static struct udevice *ubi_blk_dev; + +static void ubi_blk_bind_once (void ) +{ +struct udevice *parent; +int ret; + +/* +* A single ubi_blk device serves all volumes of the attached UBI +* device, the volume being selected through the block descriptor's +* hwpart. It is parented to the MTD device UBI sits on, so it must +* not outlive the attach that created it. +*/ +if (ubi_blk_dev) +return ; + + parent = ubi && ubi->mtd && ubi->mtd->dev ? ubi->mtd->dev :dm_root (); + + ret =ubi_bind (parent, &ubi_blk_dev); +if (ret) +printf ("Cannot create UBI block device: %d\n" , ret); +} + +static void ubi_blk_unbind (void ) +{ +if (!ubi_blk_dev) +return ; + +device_remove (ubi_blk_dev,DM_REMOVE_NORMAL ); +device_unbind (ubi_blk_dev); + ubi_blk_dev =NULL ; +} +#else +static inline void ubi_blk_bind_once (void ) { } + +static inline void ubi_blk_unbind (void ) +{ +} +#endif +int ubi_detach (void ) {#ifdef CONFIG_CMD_UBIFS
@@ -662,6 +704,8 @@ int ubi_detach(void)cmd_ubifs_umount ();#endif +ubi_blk_unbind (); +/* * Call ubi_exit() before re-initializing the UBI subsystem */
@@ -673,35 +717,6 @@ int ubi_detach(void)return 0 ; } -#if CONFIG_IS_ENABLED (UBI_BLOCK ) -static void ubi_blk_bind_once (void ) -{ -struct udevice *parent; -struct udevice *dev; -int ret; - -/* -* A single ubi_blk device serves all volumes of the attached UBI -* device, the volume being selected through the block descriptor's -* hwpart. Bind one when UBI is attached -- unless a previous attach -* already did -- parented to the MTD device UBI sits on. -*/ -for (uclass_find_first_device (UCLASS_BLK , &dev); dev; -uclass_find_next_device (&dev)) { -if (dev->driver ==DM_DRIVER_GET (ubi_blk)) -return ; - } - - parent = ubi && ubi->mtd && ubi->mtd->dev ? ubi->mtd->dev :dm_root (); - - ret =ubi_bind (parent); -if (ret) -printf ("Cannot create UBI block device: %d\n" , ret); -} -#else -static inline void ubi_blk_bind_once (void ) { } -#endif -int ubi_part (const char *part_name,const char *vid_header_offset) {struct mtd_info *mtd;
drivers/mtd/ubi/block.c +3 -1
@@ -11,7 +11,7 @@#include <dm/device.h> #include <dm/device-internal.h> -int ubi_bind (struct udevice *dev) +int ubi_bind (struct udevice *dev,struct udevice **bdevp) {struct blk_desc *bdesc;struct udevice *bdev;
@@ -29,6 +29,8 @@ int ubi_bind(struct udevice *dev) bdesc->bdev = bdev; bdesc->part_type =PART_TYPE_UBI ; + *bdevp = bdev; +return 0 ; }
include/ubi_uboot.h +2 -2
@@ -148,9 +148,9 @@ int cmd_ubifs_mount(const char *vol_name);int cmd_ubifs_umount (void );#if IS_ENABLED (CONFIG_UBI_BLOCK ) -int ubi_bind (struct udevice *dev); +int ubi_bind (struct udevice *dev,struct udevice **bdevp);#else -static inline int ubi_bind (struct udevice *dev) +static inline int ubi_bind (struct udevice *dev,struct udevice **bdevp) {return -EOPNOTSUPP ; }
Signature · 1 line
2.55.0
Re: [PATCH 03/11] cmd: ubi: unbind the block device when detaching UBI
Hi Daniel,
Quoted 30 lines · Daniel Golle
cmd: ubi: unbind the block device when detaching UBI
The
ubi_blkdevice is parented to the MTD device UBI was attached to, butubi_detach()leaves it bound. After 'ubi part A; ubi detach; ubi part B' the device still hangs off A's MTD while UBI sits on B, so the driver-model tree describes a parent that is no longer in use and A's MTD can no longer be removed. Reads happen to keep working only becauseget_ubi_device()always returns ubi_devices[0].Return the new device from
ubi_bind(), remember it, and unbind it inubi_detach()so the next attach binds a fresh one under the right parent. Tracking the device also replaces the scan of everyUCLASS_BLKdevice that was used to find it.Fixes dec405d1653a ("cmd: ubi: create a ubi_blk device when attaching UBI")Signed-off-by Daniel Golle <[email protected]>
cmd/ubi.c| 73 +++++++++++++++++++++++++++++--------------------drivers/mtd/ubi/block.c | 4 ++- include/ubi_uboot.h | 4 +-- 3 files changed, 49 insertions(+), 32 deletions(-)cmd/ubi.c +6
-0@@ -650,6 +651,47 @@ static int ubi_set_skip_check(const char *volume, bool skip_check) +#if CONFIG_IS_ENABLED (UBI_BLOCK ) +static struct udevice *ubi_blk_dev; + +static void ubi_blk_bind_once (void ) +{ +struct udevice *parent;
cmd/ubi.c static int ubi_set_skip_check(const char *volume, bool skip_check) · line 666
Quoted 10 lines
cmd/ubi.c +10
-0+int ret; + +/* +* A single ubi_blk device serves all volumes of the attached UBI +* device, the volume being selected through the block descriptor's +* hwpart. It is parented to the MTD device UBI sits on, so it must +* not outlive the attach that created it. +*/ +if (ubi_blk_dev) +return ;
Driver model can free this device without cmd/ubi.c knowing. If anything unbinds the MTD parent (e.g. 'unbind' on the flash device), DM unbinds the ubi_blk child too, leaving ubi_blk_dev pointing at freed memory. The next ubi_detach() passes it to device_remove(), and the next ubi_part() returns early and never binds a new device.
Patch 11 makes this easier to hit. With UTF_DM the DM tree is torn down after each test, so if imagemap_test_ubiblock() fails an assertion between ubi_part() and the final ubi_detach(), the next run's initial ubi_detach() uses the stale pointer. The UCLASS_BLK scan you are removing does not have this problem, since it can only find devices that still exist.
Please can you keep the scan, and use it in ubi_blk_unbind() too to find the device to unbind? That still fixes the wrong-parent problem without global state. What do you think?
cmd/ubi.c static int ubi_set_skip_check(const char *volume, bool skip_check) · line 653
Quoted 5 lines
cmd/ubi.c +3
-0@@ -650,6 +651,47 @@ static int ubi_set_skip_check(const char *volume, bool skip_check) +device_remove (ubi_blk_dev,DM_REMOVE_NORMAL ); +device_unbind (ubi_blk_dev); + ubi_blk_dev =NULL ;
Both return values are ignored. If device_remove() fails, device_unbind() returns -EINVAL for an activated device, and the pointer is cleared anyway, so the device stays bound under the old parent, which is the case this patch is trying to fix. Please at least log the error.
Regards,
Simon
PATCH 4/11 boot: imagemap: free the reservation a replaced record owned
In reply to Daniel Golleimagemap_record() overwrites an entry that already covers the same image offset, including its lmb_reserved flag. When imagemap_map_to() replaces a region that imagemap_map() had allocated from LMB, the reservation is forgotten with the address that owned it, so imagemap_cleanup() walks a table that no longer mentions it and the memory stays reserved for the rest of the boot.
Release the reservation before the entry is pointed at different memory.
--- boot/imagemap.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-)
boot/imagemap.c +16 -6
@@ -114,12 +114,22 @@ imagemap_record(struct udevice *dev, loff_t img_offset, ulong size,/* Check for an existing entry at the same base that we can extend */ alist_for_each (r, &priv->regions) { -if (r->img_offset == img_offset) { - r->size = size; - r->ram = ram; - r->lmb_reserved = lmb_reserved; -return r; - } +if (r->img_offset != img_offset) +continue ; + +/* +* The entry is about to describe different memory, so give up +* the reservation it owns while its address is still known. +*/ +if (r->lmb_reserved && r->ram != ram) +lmb_free (map_to_sysmem (r->ram), +ALIGN (r->size,ARCH_DMA_MINALIGN ),LMB_NONE ); + + r->size = size; + r->ram = ram; + r->lmb_reserved = lmb_reserved; + +return r; }/* Append new region */
Signature · 1 line
2.55.0
Re: [PATCH 04/11] boot: imagemap: free the reservation a replaced record owned
Hi Daniel,
Quoted 19 lines · Daniel Golle
boot: imagemap: free the reservation a replaced record owned
imagemap_record()overwrites an entry that already covers the same image offset, including itslmb_reservedflag. Whenimagemap_map_to()replaces a region thatimagemap_map()had allocated from LMB, the reservation is forgotten with the address that owned it, soimagemap_cleanup()walks a table that no longer mentions it and the memory stays reserved for the rest of the boot.Release the reservation before the entry is pointed at different memory.
Fixes 9b41c7dbee96 ("boot: add imagemap on-demand loading from storage")Signed-off-by Daniel Golle <[email protected]>
boot/imagemap.c| 22 ++++++++++++++++------1 file changed, 16 insertions(+), 6 deletions(-)
boot/imagemap.c imagemap_record(struct udevice *dev, loff_t img_offset, ulong size, · line 120
Quoted 7 lines
boot/imagemap.c +7
-0@@ -114,12 +114,22 @@ imagemap_record(struct udevice *dev, loff_t img_offset, ulong size, +/* +* The entry is about to describe different memory, so give up +* the reservation it owns while its address is still known. +*/ +if (r->lmb_reserved && r->ram != ram) +lmb_free (map_to_sysmem (r->ram), +ALIGN (r->size,ARCH_DMA_MINALIGN ),LMB_NONE );
This fixes the leak but adds a lifetime problem. imagemap_map() has already given its caller a pointer into the old buffer, which used to stay valid until imagemap_cleanup(). Now the memory goes back to LMB while the caller may still hold the pointer, and a later lmb_alloc() can hand it out again, e.g. for the FDT or ramdisk relocation in bootm. One way to hit this is two FIT images sharing the same external data position, where only one has a load address. The first gets an imagemap buffer, which fit_image_get_data() returns via imagemap_lookup(). The second then calls imagemap_map_to() at the same offset and frees that buffer under the first.
My suggestion is to stop imagemap_map_to() overwriting an LMB-owned entry. If the existing entry is lmb_reserved and the memory differs, append a new entry instead. imagemap_cleanup() still frees the reservation, so nothing leaks, and earlier pointers stay valid until then. What do you think?
boot/imagemap.c imagemap_record(struct udevice *dev, loff_t img_offset, ulong size, · line 116
Quoted 5 lines
boot/imagemap.c +3
-0@@ -114,12 +114,22 @@ imagemap_record(struct udevice *dev, loff_t img_offset, ulong size, + r->size = size; + r->ram = ram; + r->lmb_reserved = lmb_reserved;
Just to check: if r->ram == ram, r->lmb_reserved is true and the new lmb_reserved is false, the flag is cleared and the reservation still leaks. That happens if a caller passes an imagemap-owned pointer as dst to imagemap_map_to(). Unlikely, but the fix is simple: keep the flag when the memory doesn't change. Also, r->size can shrink here, and the lmb_free() in imagemap_cleanup() then uses the smaller size.
boot/imagemap.c imagemap_record(struct udevice *dev, loff_t img_offset, ulong size, · line 114
Quoted 3 lines
boot/imagemap.c +0
-0@@ -114,12 +114,22 @@ imagemap_record(struct udevice *dev, loff_t img_offset, ulong size,/* Check for an existing entry at the same base that we can extend */
This comment no longer fits, since the loop now replaces the entry rather than extending it. The function comment also says the entry is updated only 'if the new size is larger', which the code has never done. Please can you update both?
Please can you also add a test to test/boot/imagemap.c? imagemap_test_lmb_reserve() is a good model: call imagemap_map() then imagemap_map_to() at the same block-aligned offset, then check that the first buffer is no longer reserved after cleanup.
Regards,
Simon
PATCH 5/11 boot: imagemap: do not resize a region the caller owns
In reply to Daniel GolleThe extend path reserves memory at the recorded address and re-reads the region into it. For a region recorded by imagemap_map_to() that address belongs to the caller, which asked for the payload to be placed there:
imagemap reserves memory it was never given and overwrites whatever the caller had put at the destination.
Restrict the extend path to regions allocated from LMB by imagemap itself. A larger request against a caller-owned region now falls through to a fresh allocation, leaving the caller's memory untouched.
--- boot/imagemap.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
boot/imagemap.c +10 -5
@@ -233,13 +233,18 @@ void *imagemap_map(struct udevice *dev, loff_t img_offset, ulong size)*/ alist_for_each (r, &priv->regions) {if (r->img_offset == base_off && r->size < read_size) { +/* +* Only a region this device allocated may be resized; +* one recorded by imagemap_map_to() lives at an address +* the caller chose and owns. +*/ +if (!r->lmb_reserved) +break ; + addr =map_to_sysmem (r->ram); -/* Free old LMB reservation if we own it */ -if (r->lmb_reserved) -lmb_free (addr, -ALIGN (r->size,ARCH_DMA_MINALIGN ), -LMB_NONE ); +lmb_free (addr,ALIGN (r->size,ARCH_DMA_MINALIGN ), +LMB_NONE );/* Try to re-reserve at the same address with new size */ if (lmb_alloc_mem(LMB_MEM_ALLOC_ADDR, 0, &addr,
Signature · 1 line
2.55.0
Re: [PATCH 05/11] boot: imagemap: do not resize a region the caller owns
Hi Daniel,
Quoted 20 lines · Daniel Golle
boot: imagemap: do not resize a region the caller owns
The extend path reserves memory at the recorded address and re-reads the region into it. For a region recorded by
imagemap_map_to()that address belongs to the caller, which asked for the payload to be placed there:
imagemap reserves memory it was never given and overwrites whatever the caller had put at the destination.Restrict the extend path to regions allocated from LMB by imagemap itself. A larger request against a caller-owned region now falls through to a fresh allocation, leaving the caller's memory untouched.
Fixes 9b41c7dbee96 ("boot: add imagemap on-demand loading from storage")Signed-off-by Daniel Golle <[email protected]>
boot/imagemap.c| 15 ++++++++++-----1 file changed, 10 insertions(+), 5 deletions(-)
boot/imagemap.c void *imagemap_map(struct udevice *dev, loff_t img_offset, ulong size) · line 239
Quoted 7 lines
boot/imagemap.c +7
-0@@ -233,13 +233,18 @@ void *imagemap_map(struct udevice *dev, loff_t img_offset, ulong size) +/* +* Only a region this device allocated may be resized; +* one recorded by imagemap_map_to() lives at an address +* the caller chose and owns. +*/ +if (!r->lmb_reserved) +break ;
The logic looks right to me. imagemap_record() dedups on img_offset, so there is at most one match and the break is fine.
Please can you add a test? For example, call imagemap_map_to(dev, 0, 64, dst) with a guard pattern in dst beyond 64 bytes, then imagemap_map(dev, 0, 256), and check that the guard bytes are unchanged, the returned pointer is not dst, and the new region is LMB-reserved. Without a test this is easy to break again.
It may also be worth saying in the commit message that the fall-through replaces the caller's record in the table, via imagemap_record(). A later imagemap_map_to() to the same dst then misses the 'already mapped' shortcut and reads the data again. That is harmless, but not obvious from the description.
boot/imagemap.c void *imagemap_map(struct udevice *dev, loff_t img_offset, ulong size) · line 247
Quoted 2 lines
boot/imagemap.c +2
-0+lmb_free (addr,ALIGN (r->size,ARCH_DMA_MINALIGN ), +LMB_NONE );
BTW, this is not from your patch, but it is the same class of bug the series is fixing. The old reservation is freed here, but r is only updated on success. Both the -ENOMEM and read-failure exits leave r->lmb_reserved true and r->ram pointing at memory that is no longer reserved. imagemap_cleanup() then frees it a second time, which could drop someone else's reservation, and a later imagemap_lookup() can return a pointer into unreserved memory. On the in-place read-failure path, r also still claims r->size valid bytes, although the partial read may have overwritten them. Perhaps those exits should drop the entry or re-reserve the old range? After patch 6, the in-place case could instead shrink the reservation back to old_size, since those bytes are still valid. It could go in this series or the follow-up, whichever you prefer.
Regards,
Simon
PATCH 6/11 boot: imagemap: read only the bytes an extend adds
In reply to Daniel GolleGrowing a region re-reads it from the start even when the reservation was renewed at the same address and the leading bytes are still valid. The translation table exists to avoid re-reading data that is already in RAM, so the common case of probing a FIT header and then mapping the whole structure pays for the header twice.
Read only the range the larger request adds when the region has not moved. A reservation that had to be placed elsewhere still needs the whole range.
--- boot/imagemap.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)
boot/imagemap.c +14 -4
@@ -213,8 +213,10 @@ void *imagemap_map(struct udevice *dev, loff_t img_offset, ulong size)ulong overhead = img_offset & (bl_len -1 );loff_t base_off = img_offset - overhead;ulong read_size =ALIGN (size + overhead, bl_len); +phys_addr_t old_addr;phys_addr_t addr;phys_size_t alloc_size; +ulong old_size;void *base;int ret;struct imagemap_region *r;
@@ -241,9 +243,11 @@ void *imagemap_map(struct udevice *dev, loff_t img_offset, ulong size)if (!r->lmb_reserved)break ; - addr =map_to_sysmem (r->ram); + old_size = r->size; + old_addr =map_to_sysmem (r->ram); + addr = old_addr; -lmb_free (addr,ALIGN (r->size,ARCH_DMA_MINALIGN ), +lmb_free (addr,ALIGN (old_size,ARCH_DMA_MINALIGN ),LMB_NONE );/* Try to re-reserve at the same address with new size */
@@ -261,8 +265,14 @@ void *imagemap_map(struct udevice *dev, loff_t img_offset, ulong size) } base =map_sysmem (addr, alloc_size); - ret =spl_load_region (&priv->info, img_offset, size, - base); +if (addr == old_addr) + ret =spl_load_region (&priv->info, + base_off + old_size, + read_size - old_size, + (char *)base + old_size); +else + ret =spl_load_region (&priv->info, img_offset, + size, base);if (ret <0 ) {log_err ("imagemap: read failed at offset 0x%llx (size 0x%lx): %d\n" , (unsigned long long )img_offset, size, ret);
Signature · 1 line
2.55.0
Re: [PATCH 06/11] boot: imagemap: read only the bytes an extend adds
Hi Daniel,
Quoted 8 lines · Daniel Golle
boot: imagemap: read only the bytes an extend adds
Growing a region re-reads it from the start even when the reservation was renewed at the same address and the leading bytes are still valid. The translation table exists to avoid re-reading data that is already in RAM, so the common case of probing a FIT header and then mapping the whole structure pays for the header twice.
Quoted 5 lines
Read only the range the larger request adds when the region has not moved. A reservation that had to be placed elsewhere still needs the whole range.
Fixes 9b41c7dbee96 ("boot: add imagemap on-demand loading from storage")
This is an optimisation rather than a bug fix: the current code reads more than it needs to, but the result is correct. Please can you drop the Fixes: tag so this is not picked up as a stable fix?
Quoted 3 lines
Read only the range the larger request adds when the region has not moved. A reservation that had to be placed elsewhere still needs the whole range.
Strictly, it doesn't need the whole range from storage. The old reservation has been freed but nothing has written over it, so the old bytes are still in RAM. A memmove() from the old address to the new one would keep them, and copes with the new block overlapping the old one, which is quite likely here. See also below.
Quoted 7 lines
Signed-off-by Daniel Golle <[email protected]>
boot/imagemap.c| 18 ++++++++++++++----1 file changed, 14 insertions(+), 4 deletions(-)
boot/imagemap.c void *imagemap_map(struct udevice *dev, loff_t img_offset, ulong size) · line 272
Quoted 10 lines
boot/imagemap.c +8
-2@@ -261,8 +265,14 @@ void *imagemap_map(struct udevice *dev, loff_t img_offset, ulong size) - ret =spl_load_region (&priv->info, img_offset, size, - base); +if (addr == old_addr) + ret =spl_load_region (&priv->info, + base_off + old_size, + read_size - old_size, + (char *)base + old_size); +else + ret =spl_load_region (&priv->info, img_offset, + size, base);
Just to check: how often is the addr == old_addr case taken in practice? LMB_MEM_ALLOC_ANY allocates top-down, so the FIT header region is normally the last allocation, sitting directly below the previous one or below U-Boot's own reservation. Growing it upwards at the same address runs into that memory and fails, so we end up in the else branch. If so, the 'common case' in the commit message mostly does not benefit. Copying the already-loaded bytes across in the else branch would make the saving apply in both cases.
The comment above the loop still says "re-reading the full range from storage", so please update it too.
For the test, imagemap_test_map_extend() only checks read_count, which is 2 whether the code reads the whole range or just the extra bytes. The mock already records last_off and last_size. Please can you assert that the second read is (64, 192), and add a variant using create_mock_loader_bl() with a block size larger than 1? That would show the new path is really used and the offsets are right for block media.
Regards,
Simon
PATCH 7/11 boot: fit: unmap the load address after a storage read
In reply to Daniel Gollefit_image_load_storage() maps the load address to get a destination for imagemap_map_to() and never unmaps it. The mapping is a no-op on most architectures but not on sandbox, which is where the unit tests run.
Pair the map_sysmem() with an unmap_sysmem() once the read is done.
--- boot/image-fit.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
boot/image-fit.c +3 -1
@@ -2190,6 +2190,7 @@ static int fit_image_load_storage(struct bootm_headers *images, const void *fit,ulong img_load;u8 img_comp =IH_COMP_NONE ;void *mapped; +void *dst;if (CONFIG_IS_ENABLED (FIT_VERITY )) {u8 img_type;
@@ -2222,10 +2223,11 @@ static int fit_image_load_storage(struct bootm_headers *images, const void *fit,if (img_comp ==IH_COMP_NONE && load_op !=FIT_LOAD_IGNORED && !fit_image_get_load (fit, noffset, &img_load)) { -void *dst =map_sysmem (img_load, data_sz); + dst =map_sysmem (img_load, data_sz); mapped =imagemap_map_to (images->imagemap, data_off, data_sz, dst); +unmap_sysmem (dst); }else { mapped =imagemap_map (images->imagemap, data_off, data_sz); }
Signature · 1 line
2.55.0
Re: [PATCH 07/11] boot: fit: unmap the load address after a storage read
Hi Daniel,
Quoted 8 lines · Daniel Golle
boot: fit: unmap the load address after a storage read
fit_image_load_storage()maps the load address to get a destination forimagemap_map_to()and never unmaps it. The mapping is a no-op on most architectures but not on sandbox, which is where the unit tests run.Pair the
map_sysmem()with anunmap_sysmem()once the read is done.
Quoted 7 lines
Fixes 46d32e38ee4e ("boot: fit: support on-demand loading in fit_image_load()")Signed-off-by Daniel Golle <[email protected]>
boot/image-fit.c| 4 +++-1 file changed, 3 insertions(+), 1 deletion(-)The mapping is a no-op on most architectures but not on sandbox, which is where the unit tests run.
Just FYI, on sandbox, unmap_physmem() returns early for anything inside emulated RAM (is_in_sandbox_mem()), and a FIT load address is always in emulated RAM, so in practice it is a no-op there too. I'm fine with adding the unmap for consistency (this is definitely an arcane part of sandbox!), but please can you reword this so it doesn't suggest the tests were leaking something? The Fixes: tag also feels a bit strong, but OK if you want it.
boot/image-fit.c static int fit_image_load_storage(struct bootm_headers *images, const void *fit, · line 2225
Quoted 5 lines
boot/image-fit.c +1
-0@@ -2222,10 +2223,11 @@ static int fit_image_load_storage(struct bootm_headers *images, const void *fit, mapped =imagemap_map_to (images->imagemap, data_off, data_sz, dst); +unmap_sysmem (dst);
imagemap_map_to() saves dst in the region list through imagemap_record(), and fit_image_get_data() later hands that same pointer back through imagemap_lookup(). So this unmaps a pointer the imagemap still holds and keeps giving to callers, which only works because unmap_sysmem() does nothing here. If the unmap is meant to be correct, it should happen when the record is released (e.g. in imagemap_cleanup() for regions where lmb_reserved is false), or the region should store a physical address and map it on lookup. Otherwise I'd suggest leaving the code alone and adding a comment that the mapping lives as long as the imagemap record. What do you think?
boot/image-fit.c static int fit_image_load_storage(struct bootm_headers *images, const void *fit, · line 2191
Quoted 4 lines
boot/image-fit.c +1
-0@@ -2190,6 +2190,7 @@ static int fit_image_load_storage(struct bootm_headers *images, const void *fit,void *mapped; +void *dst;
There's no need to move this to function scope; it is only used inside the if() block. Sorry if I got that wrong in the previous review.
Regards,
Simon
PATCH 8/11 boot: fit: keep the load message for RAM-backed images
In reply to Daniel GolleThe "Loading ... from ... to ..." line is suppressed whenever CONFIG_IMAGEMAP is built in and the source happens to equal the load address. The test is a build-time one, so a board that enables imagemap but boots a FIT from RAM loses the line as well.
Suppress it only when an imagemap is actually in use, which is the one case where the data was read straight to its load address and there is no copy to report.
--- boot/image-fit.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
boot/image-fit.c +2 -1
@@ -2468,7 +2468,8 @@ int fit_image_load(struct bootm_headers *images, ulong addr,return -EXDEV ; } -if (!CONFIG_IS_ENABLED (IMAGEMAP ) || data != load) +/* A storage-backed image was read straight to its load address */ +if (!images->imagemap || data != load)printf (" Loading %s from 0x%08lx to 0x%08lx\n" , prop_name, data, load); }else {
Signature · 1 line
2.55.0
Re: [PATCH 08/11] boot: fit: keep the load message for RAM-backed images
Hi Daniel,
Quoted 19 lines · Daniel Golle
boot: fit: keep the load message for RAM-backed images
The "Loading ... from ... to ..." line is suppressed whenever
CONFIG_IMAGEMAPis built in and the source happens to equal the load address. The test is a build-time one, so a board that enables imagemap but boots a FIT from RAM loses the line as well.Suppress it only when an imagemap is actually in use, which is the one case where the data was read straight to its load address and there is no copy to report.
Fixes 46d32e38ee4e ("boot: fit: support on-demand loading in fit_image_load()")Signed-off-by Daniel Golle <[email protected]>
boot/image-fit.c| 3 ++-1 file changed, 2 insertions(+), 1 deletion(-)
boot/image-fit.c int fit_image_load(struct bootm_headers *images, ulong addr, · line 2469
Quoted 3 lines
boot/image-fit.c +2
-1@@ -2468,7 +2468,8 @@ int fit_image_load(struct bootm_headers *images, ulong addr, -if (!CONFIG_IS_ENABLED (IMAGEMAP ) || data != load) +/* A storage-backed image was read straight to its load address */ +if (!images->imagemap || data != load)
Dropping the CONFIG_IS_ENABLED(IMAGEMAP) term means images->imagemap is read at runtime even when imagemap is not built, which isn't safe for every caller. spl_load_fit_image() declares 'struct bootm_headers images' on the stack and only sets 'verify', so in SPL (there is no SPL_IMAGEMAP, so the old expression was always true) this now tests uninitialised stack memory, and the message can disappear at random when data == load. The storage check further up keeps the build-time guard in front of the pointer test. Please can you do the same here:
if (!CONFIG_IS_ENABLED(IMAGEMAP) || !images->imagemap || data != load)
That keeps the runtime fix for RAM-backed boots with imagemap enabled, and lets the compiler drop the test when it is disabled. Separately, it would be worth zeroing 'images' in spl_load_fit_image() as vbe_common.c does, but that belongs in its own patch.
Regards,
Simon
PATCH 9/11 boot: bootm: release the imagemap on every failing exit
In reply to Daniel GolleThree error paths in bootm_run_states() return directly instead of reaching the err label, so the imagemap device and the LMB reservations its translation table holds survive a failed bootm: the missing OS boot function, an unsupported subcommand, and any error carried out of the earlier states. The first of those also left interrupts disabled.
Route all three through err, which already releases the imagemap and re-enables interrupts.
--- boot/bootm.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
boot/bootm.c +4 -5
@@ -1265,18 +1265,17 @@ int bootm_run_states(struct bootm_info *bmi, int states)/* From now on, we need the OS boot function */ if (ret) -return ret; + goto err; boot_fn =bootm_os_get_boot_func (images->os.os); need_boot_fn = states & (BOOTM_STATE_OS_CMDLINE |BOOTM_STATE_OS_BD_T |BOOTM_STATE_OS_PREP |BOOTM_STATE_OS_FAKE_GO |BOOTM_STATE_OS_GO );if (boot_fn ==NULL && need_boot_fn) { -if (iflag) -enable_interrupts ();printf ("ERROR: booting os '%s' (%d) is not supported\n" ,genimg_get_os_name (images->os.os), images->os.os);bootstage_error (BOOTSTAGE_ID_CHECK_BOOT_OS ); -return 1 ; + ret =1 ; + goto err; }/* Call various other states that are not generally used */
@@ -1318,7 +1317,7 @@ int bootm_run_states(struct bootm_info *bmi, int states)/* Check for unsupported subcommand. */ if (ret) {printf ("subcommand failed (err=%d)\n" , ret); -return ret; + goto err; }/* Now run the OS! We hope this doesn't return */
Signature · 1 line
2.55.0
Re: [PATCH 09/11] boot: bootm: release the imagemap on every failing exit
Hi Daniel,
Quoted 11 lines · Daniel Golle
boot: bootm: release the imagemap on every failing exit
Three error paths in
bootm_run_states()return directly instead of reaching the err label, so the imagemap device and the LMB reservations its translation table holds survive a failed bootm: the missing OS boot function, an unsupported subcommand, and any error carried out of the earlier states. The first of those also left interrupts disabled.Route all three through err, which already releases the imagemap and re-enables interrupts.
Quoted 7 lines
Fixes 46d32e38ee4e ("boot: fit: support on-demand loading in fit_image_load()")Signed-off-by Daniel Golle <[email protected]>
boot/bootm.c| 9 ++++-----1 file changed, 4 insertions(+), 5 deletions(-)The first of those also left interrupts disabled.
Just to check: iflag is set as soon as BOOTM_STATE_LOADOS runs, so a later ramdisk/FDT failure or a failed subcommand also returned with interrupts disabled. All three paths had this problem, not just the first.
boot/bootm.c int bootm_run_states(struct bootm_info *bmi, int states) · line 1267
Quoted 6 lines
boot/bootm.c +1
-1@@ -1265,18 +1265,17 @@ int bootm_run_states(struct bootm_info *bmi, int states)/* From now on, we need the OS boot function */ if (ret) -return ret; + goto err;
The err label does more than release resources. It also handles BOOTM_ERR_RESET / BOOTM_ERR_UNIMPLEMENTED, which was only meant for errors from bootm_load_os(). BOOTM_ERR_RESET is -1, and boot_ramdisk_high() returns -1 on any failure, so with this patch a 'ramdisk - allocation error' or 'in-place initrd alloc failed' now prints 'Resetting the board...' and calls reset_cpu() instead of returning the error. The same applies to the subcommand path below: on ARM, do_bootm_linux() returns -1 for BOOTM_STATE_OS_BD_T and BOOTM_STATE_OS_CMDLINE, so 'bootm bdt' would reset the board. This happens whether or not an imagemap is in use.
Please can you keep the reset/unimplemented handling for the load_os path only? One way is to give that path its own label that falls through to the common cleanup. Another is to move the imagemap release and enable_interrupts() into a separate label after the BOOTM_ERR_* checks, and send these three exits there. What do you think?
boot/bootm.c int bootm_run_states(struct bootm_info *bmi, int states) · line 1320
Quoted 7 lines
boot/bootm.c +1
-1@@ -1318,7 +1317,7 @@ int bootm_run_states(struct bootm_info *bmi, int states)if (ret) {printf ("subcommand failed (err=%d)\n" , ret); -return ret; + goto err; }
See above. It would also be worth adding a sandbox test for a failing bootm subcommand with an imagemap attached, so this path is covered.
Regards,
Simon
PATCH 10/11 test: boot: imagemap: detach UBI when the test is done
In reply to Daniel GolleThe UBI test attaches UBI to the sandbox NAND and leaves it attached, so UBI still holds a reference to the MTD device when the test ends. Running the suite a second time crashes in mtd_partitions_used(): the test calls mtd_probe_devices(), which rebuilds the partition list of a master whose partitions UBI is still using, and the walk then follows a freed entry.
Detach UBI at the end of the test, and again at the start so a device left behind by anything else is released before the partitions are rebuilt.
--- test/boot/imagemap.c | 8 ++++++++ 1 file changed, 8 insertions(+)
test/boot/imagemap.c +8 -0
@@ -592,6 +592,13 @@ static int imagemap_test_ubiblock(struct unit_test_state *uts)u64 off;int i; +/* +* Release any device a previous run attached: UBI keeps a reference to +* the MTD device, and mtd_probe_devices() would then rebuild the +* partition list behind its back. +*/ +ubi_detach (); +mtd_probe_devices (); mtd =get_mtd_device_nm ("nand2" );ut_assert (!IS_ERR_OR_NULL (mtd));
@@ -638,6 +645,7 @@ static int imagemap_test_ubiblock(struct unit_test_state *uts)imagemap_cleanup (imdev);free (wbuf); +ubi_detach ();return 0 ; }
Signature · 1 line
2.55.0
Re: [PATCH 10/11] test: boot: imagemap: detach UBI when the test is done
Hi Daniel,
Quoted 7 lines · Daniel Golle
test: boot: imagemap: detach UBI when the test is done
The UBI test attaches UBI to the sandbox NAND and leaves it attached, so UBI still holds a reference to the MTD device when the test ends. Running the suite a second time crashes in
mtd_partitions_used(): the test callsmtd_probe_devices(), which rebuilds the partition list of a master whose partitions UBI is still using, and the walk then follows a freed entry.
Just to check: mtd_del_parts() checks mtd_partitions_used() precisely so that it does not delete partitions still in use. If the walk follows a freed entry, something has already freed a partition while UBI held it. That looks like an MTD bug which 'ubi part nand2' followed by anything that reprobes could also hit. Please can you explain what frees the entry? If it is a real core bug, I suspect it should be fixed there, with this patch only tidying up the test.
Quoted 13 lines
Detach UBI at the end of the test, and again at the start so a device left behind by anything else is released before the partitions are rebuilt.
Fixes 05c1fbbfa79f ("test: boot: add imagemap unit tests")Signed-off-by Daniel Golle <[email protected]>
test/boot/imagemap.c| 8 ++++++++1 file changed, 8 insertions(+)
test/boot/imagemap.c static int imagemap_test_ubiblock(struct unit_test_state *uts) · line 597
Quoted 6 lines
test/boot/imagemap.c +6
-0@@ -592,6 +592,13 @@ static int imagemap_test_ubiblock(struct unit_test_state *uts) +/* +* Release any device a previous run attached: UBI keeps a reference to +* the MTD device, and mtd_probe_devices() would then rebuild the +* partition list behind its back. +*/ +ubi_detach ();
This only helps if the device was left behind by an earlier run of this same test. Linker-list order puts imagemap_test_mtd_blk before this one, and it also calls mtd_probe_devices(), so if this test fails part-way, the next pass crashes in mtd_blk before this detach is reached.
After the next patch it also acts on freed memory (see my comment on patch 3), and the 'ubi' global is left holding an MTD whose udevice has gone.
test/boot/imagemap.c static int imagemap_test_ubiblock(struct unit_test_state *uts) · line 651
Quoted 9 lines
test/boot/imagemap.c +1
-0@@ -638,6 +645,7 @@ static int imagemap_test_ubiblock(struct unit_test_state *uts)imagemap_cleanup (imdev);free (wbuf); +ubi_detach ();return 0 ; }
This is skipped whenever a ut_assert...() between ubi_part() and here fails, which is exactly the case the start-of-test detach is meant to cover, and wbuf leaks too. Please can you move the body into a helper and have the test function always detach afterwards, something like:
static int imagemap_test_ubiblock(struct unit_test_state *uts)
{
int ret;
ret = do_ubiblock_test(uts);
ubi_detach();
return ret;
}That keeps the teardown on every exit path, so you can drop the detach at the start. What do you think?
Regards,
Simon
PATCH 11/11 test: boot: imagemap: reset driver-model state between tests
In reply to Daniel GolleThe tests bind and probe devices under dm_root() and the storage tests depend on real driver-model state, but none of them asks for the DM init and uninit the test framework provides. A test that fails before imagemap_cleanup() therefore leaves its bound device behind, and the next one trips over the name clash in device_bind_driver().
Pass UTF_DM throughout, and UTF_SCAN_FDT for the two tests that walk device-tree partitions. Those two are also marked UTF_LIVE_TREE: they drive mtd_probe_devices(), which cannot parse the partitions of a master bound from a flat tree.
--- test/boot/imagemap.c | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-)
test/boot/imagemap.c +15 -13
@@ -155,7 +155,7 @@ static int imagemap_test_map_basic(struct unit_test_state *uts)return 0 ; } -IMAGEMAP_TEST (imagemap_test_map_basic,0 ); +IMAGEMAP_TEST (imagemap_test_map_basic,UTF_DM );/* Test: map() returns cached pointer for already-mapped range */ static int imagemap_test_map_cached (struct unit_test_state *uts)
@@ -188,7 +188,7 @@ static int imagemap_test_map_cached(struct unit_test_state *uts)return 0 ; } -IMAGEMAP_TEST (imagemap_test_map_cached,0 ); +IMAGEMAP_TEST (imagemap_test_map_cached,UTF_DM );/* Test: map() returns correct offset within a larger region */ static int imagemap_test_map_offset (struct unit_test_state *uts)
@@ -222,7 +222,7 @@ static int imagemap_test_map_offset(struct unit_test_state *uts)return 0 ; } -IMAGEMAP_TEST (imagemap_test_map_offset,0 ); +IMAGEMAP_TEST (imagemap_test_map_offset,UTF_DM );/* Test: map() re-reads when extending a region to a larger size */ static int imagemap_test_map_extend (struct unit_test_state *uts)
@@ -281,7 +281,7 @@ static int imagemap_test_map_extend(struct unit_test_state *uts)return 0 ; } -IMAGEMAP_TEST (imagemap_test_map_extend,0 ); +IMAGEMAP_TEST (imagemap_test_map_extend,UTF_DM );/* Test: map_to() reads to a specified address and records it */ static int imagemap_test_map_to (struct unit_test_state *uts)
@@ -315,7 +315,7 @@ static int imagemap_test_map_to(struct unit_test_state *uts)return 0 ; } -IMAGEMAP_TEST (imagemap_test_map_to,0 ); +IMAGEMAP_TEST (imagemap_test_map_to,UTF_DM );/* Test: lookup() returns NULL for unmapped ranges */ static int imagemap_test_lookup_miss (struct unit_test_state *uts)
@@ -350,7 +350,7 @@ static int imagemap_test_lookup_miss(struct unit_test_state *uts)return 0 ; } -IMAGEMAP_TEST (imagemap_test_lookup_miss,0 ); +IMAGEMAP_TEST (imagemap_test_lookup_miss,UTF_DM );/* Test: LMB reservations are made for map() allocations */ static int imagemap_test_lmb_reserve (struct unit_test_state *uts)
@@ -383,7 +383,7 @@ static int imagemap_test_lmb_reserve(struct unit_test_state *uts)return 0 ; } -IMAGEMAP_TEST (imagemap_test_lmb_reserve,0 ); +IMAGEMAP_TEST (imagemap_test_lmb_reserve,UTF_DM );/* Test: cleanup() removes the device */ static int imagemap_test_cleanup (struct unit_test_state *uts)
@@ -405,7 +405,7 @@ static int imagemap_test_cleanup(struct unit_test_state *uts)return 0 ; } -IMAGEMAP_TEST (imagemap_test_cleanup,0 ); +IMAGEMAP_TEST (imagemap_test_cleanup,UTF_DM );/* Test: map() with multiple disjoint regions */ static int imagemap_test_multi_region (struct unit_test_state *uts)
@@ -448,7 +448,7 @@ static int imagemap_test_multi_region(struct unit_test_state *uts)return 0 ; } -IMAGEMAP_TEST (imagemap_test_multi_region,0 ); +IMAGEMAP_TEST (imagemap_test_multi_region,UTF_DM );/* Test: read beyond image size returns error */ static int imagemap_test_read_oob (struct unit_test_state *uts)
@@ -474,7 +474,7 @@ static int imagemap_test_read_oob(struct unit_test_state *uts)return 0 ; } -IMAGEMAP_TEST (imagemap_test_read_oob,0 ); +IMAGEMAP_TEST (imagemap_test_read_oob,UTF_DM );/* * Test: block-addressed backend (bl_len > 1) still returns/places the
@@ -514,7 +514,7 @@ static int imagemap_test_blocklen(struct unit_test_state *uts)return 0 ; } -IMAGEMAP_TEST (imagemap_test_blocklen,0 ); +IMAGEMAP_TEST (imagemap_test_blocklen,UTF_DM );#if CONFIG_IS_ENABLED (MTD_BLOCK )/*
@@ -571,7 +571,8 @@ static int imagemap_test_mtd_blk(struct unit_test_state *uts) return 0; } -IMAGEMAP_TEST (imagemap_test_mtd_blk,0 ); + +IMAGEMAP_TEST (imagemap_test_mtd_blk,UTF_DM |UTF_SCAN_FDT |UTF_LIVE_TREE );#endif /* MTD_BLOCK */ #if CONFIG_IS_ENABLED (MTD_UBI ) &&CONFIG_IS_ENABLED (UBI_BLOCK )
@@ -649,5 +650,6 @@ static int imagemap_test_ubiblock(struct unit_test_state *uts) return 0; } -IMAGEMAP_TEST (imagemap_test_ubiblock,0 ); + +IMAGEMAP_TEST (imagemap_test_ubiblock,UTF_DM |UTF_SCAN_FDT |UTF_LIVE_TREE );#endif /* MTD_UBI && UBI_BLOCK */
Signature · 1 line
2.55.0
Re: [PATCH 11/11] test: boot: imagemap: reset driver-model state between tests
Hi Daniel,
Quoted 8 lines · Daniel Golle
test: boot: imagemap: reset driver-model state between tests
The tests bind and probe devices under
dm_root()and the storage tests depend on real driver-model state, but none of them asks for the DM init and uninit the test framework provides. A test that fails beforeimagemap_cleanup()therefore leaves its bound device behind, and the next one trips over the name clash indevice_bind_driver().
Quoted 4 lines
Pass
UTF_DMthroughout, andUTF_SCAN_FDTfor the two tests that walk device-tree partitions. Those two are also markedUTF_LIVE_TREE: they drivemtd_probe_devices(), which cannot parse the partitions of a master bound from a flat tree.
I'm not sure about this. add_mtd_partitions_of() reads the partitions with the ofnode API (mtd_get_ofnode(), ofnode_find_subnode(), ofnode_for_each_subnode()), so I would expect it to work with a flat tree too. What actually fails in the flat-tree run? If the cause is something else, such as the static old_mtdparts / mtd_dev_list_updated() state in mtd_probe_devices() or MTD state left over from the live-tree run, please can you describe that instead? Otherwise UTF_LIVE_TREE just hides the problem.
test/boot/imagemap.c static int imagemap_test_ubiblock(struct unit_test_state *uts) · line 651
Quoted 12 lines
Fixes 05c1fbbfa79f ("test: boot: add imagemap unit tests")Signed-off-by Daniel Golle <[email protected]>
test/boot/imagemap.c| 28 +++++++++++++++-------------1 file changed, 15 insertions(+), 13 deletions(-)test/boot/imagemap.c +2
-1@@ -649,5 +650,6 @@ static int imagemap_test_ubiblock(struct unit_test_state *uts) -IMAGEMAP_TEST (imagemap_test_ubiblock,0 ); + +IMAGEMAP_TEST (imagemap_test_ubiblock,UTF_DM |UTF_SCAN_FDT |UTF_LIVE_TREE );
The motivation is a test that fails part-way, but for this test UTF_DM makes that case worse. If an assertion fails after ubi_part(), the test returns without calling ubi_detach(), and dm_test_post_run() then destroys every uclass. That frees the sand-nand device and its chips, which UBI still points to, as well as the ubi_blk device. nand_unregister() cannot delete the MTD either, since UBI holds a use count. Without UTF_DM, the leading ubi_detach() from patch 10 would clean up properly.
Please can you make sure every failure after ubi_part() calls imagemap_cleanup(), free() and ubi_detach() (see my comment on patch 10), so the DM teardown never runs while UBI is still attached? Alternatively, keep this test out of UTF_DM. What do you think?
test/boot/imagemap.c static int imagemap_test_mtd_blk(struct unit_test_state *uts) · line 572
Quoted 5 lines
test/boot/imagemap.c +2
-1@@ -571,7 +571,8 @@ static int imagemap_test_mtd_blk(struct unit_test_state *uts) -IMAGEMAP_TEST (imagemap_test_mtd_blk,0 ); + +IMAGEMAP_TEST (imagemap_test_mtd_blk,UTF_DM |UTF_SCAN_FDT |UTF_LIVE_TREE );
Regards,
Simon
Hi Daniel,
Quoted 5 lines · Daniel Golle
Simon Glass reviewed the v1 posting of the imagemap series ([email protected]) after v2 had already been applied, so his comments arrive against code that is now in next. This series carries the correctness half of them; a second series follows with the structural and cosmetic half.
Thanks for picking these up so quickly, and for splitting the correctness fixes from the cosmetic ones. This is one of the most tricky series I've looked at in a while!
Patches 4, 5 and 6 change how imagemap behaves but add no tests, so 'ut imagemap' passes both before and after each one. I've suggested a test on each patch.
Quoted 2 lines
Tested on sandbox: every commit builds, "ut imagemap" passes 13/13 and survives "ut -r3"
The -r3 run only exercises the passing path. If the UBI test fails part-way, the MTD table, the attached UBI device and the static ubi_blk_dev from patch 3 all survive the driver-model teardown and point into the freed tree. So I think patches 3, 10 and 11 need a bit more thought. Details are on those patches.
Smaller point: patch 2 adds a 'parent'/'ret' version of ubi_blk_bind_once(), then patch 3 moves it and rewrites most of it. Please consider doing the move first, or folding patch 2's error report into patch 3, so the reader only reviews the function once.
Overall I am not thrilled with how driver model is holding up in this case, e.g. the interaction with UBI and tests. I recently did some work on putting the EFI state into a struct, so we can create a new state for tests. I'd be interested in your thoughts on this, given your effort in putting this series together.
Regards,
Simon
Quoted 6 lines · Daniel Golle
Simon Glass reviewed the v1 posting of the imagemap series ([email protected]) after v2 had already been applied, so his comments arrive against code that is now in next. This series carries the correctness half of them; a second series follows with the structural and cosmetic half.
Quoted 7 lines
Tested on sandbox: every commit builds, "ut imagemap" passes 13/13 and survives "ut -r3", and a full
test.pyrun shows the same 11 failures and 20 errors as unmodified next, all of them EFI-secboot tooling, ext4 and config-rebuild tests unrelated to this code. Also built withCONFIG_IMAGEMAP=n.[...]
Applied to u-boot/main, thanks!
[01/11] mtd: bind the block device without leaking on failure
commit: 10eef5457b925020dde5210c4270ea5f7d983f4f
[02/11] cmd: ubi: report a failed UBI block device bind
commit: 79ca2f85bfe6f0e02987d4ea53a57f4ab0a6e986
[03/11] cmd: ubi: unbind the block device when detaching UBI
commit: 74ee4f18abc81342600dccbf788026e7537d7a4a
[04/11] boot: imagemap: free the reservation a replaced record owned
commit: 0989171be0a4f7c282bd8ff86258b3dc12fcc45d
[05/11] boot: imagemap: do not resize a region the caller owns
commit: 614e9fc19f182c221d372759020900722f9be57b
[06/11] boot: imagemap: read only the bytes an extend adds
commit: 8e5116836a8f49d362cb220149dc559892130338
[07/11] boot: fit: unmap the load address after a storage read
commit: 96270dfb5c7164328c56dc40d665d931f5237976
[08/11] boot: fit: keep the load message for RAM-backed images
commit: 06d6f1705550cc21029669ed42fcce85d6c4170b
[09/11] boot: bootm: release the imagemap on every failing exit
commit: 148c55c66c335b23cfac22d09d9d7ee091c2a50f
[10/11] test: boot: imagemap: detach UBI when the test is done
commit: d828f37d96afcaccfe90c9a0af61f326cd407355
[11/11] test: boot: imagemap: reset driver-model state between tests
commit: 9f938a377f6599422a93b8ef6b2f903dee43f56fSignature · 1 line
Tom