If we don't do that, partitions might still be exposed while the underlying device is gone.

Fixes: 2a74930da57f ("mtd: mtdpart: implement proper partition handling") Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com Tested-by: Heiko Schocher hs@denx.de --- Changes in v4: - Test the del_mtd_partitions() ret code and propagate errors - Add a debug() message when del_mtd_partitions() returns an error - Add Heiko's T-b

Changes in v3: - None

Changes in v2: - Fix build failures when CONFIG_MTD_PARTITIONS is not enabled --- drivers/mtd/mtdcore.c | 7 +++++++ include/linux/mtd/mtd.h | 15 +++++++++++++++ 2 files changed, 22 insertions(+)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c index 7a15ded8c883..cb7ca38d0744 100644 --- a/drivers/mtd/mtdcore.c +++ b/drivers/mtd/mtdcore.c @@ -528,6 +528,13 @@ int del_mtd_device(struct mtd_info *mtd) struct mtd_notifier *not; #endif

+ ret = del_mtd_partitions(mtd); + if (ret) { + debug("Failed to delete MTD partitions attached to %s (err %d)\n", + mtd->name, ret); + return ret; + } + mutex_lock(&mtd_table_mutex);

if (idr_find(&mtd_idr, mtd->index) != mtd) { diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h index d20ebd820289..4d0096d9f1d8 100644 --- a/include/linux/mtd/mtd.h +++ b/include/linux/mtd/mtd.h @@ -562,8 +562,23 @@ unsigned mtd_mmap_capabilities(struct mtd_info *mtd); /* drivers/mtd/mtdcore.h */ int add_mtd_device(struct mtd_info *mtd); int del_mtd_device(struct mtd_info *mtd); + +#ifdef CONFIG_MTD_PARTITIONS int add_mtd_partitions(struct mtd_info *, const struct mtd_partition *, int); int del_mtd_partitions(struct mtd_info *); +#else +static inline int add_mtd_partitions(struct mtd_info *mtd, + const struct mtd_partition *parts, + int nparts) +{ + return 0; +} + +static inline int del_mtd_partitions(struct mtd_info *mtd) +{ + return 0; +} +#endif

struct mtd_info *__mtd_next_device(int i); #define mtd_for_each_device(mtd) \

The environment is not guaranteed to contain a valid mtdids variable when called from mtd_search_alternate_name(). Call get_mtdids() instead of env_get("mtdids").

Fixes: ff4afa8a981e ("mtd: uboot: search for an equivalent MTD name with the mtdids") Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com Reviewed-by: Miquel Raynal miquel.raynal@bootlin.com Tested-by: Heiko Schocher hs@denx.de --- Changes in v4: - Add T-b tag

Changes in v3: - None

Changes in v2: - Add Miquel's R-b - Move board_mtdparts_default() prototype def to fix a build failure --- drivers/mtd/mtd_uboot.c | 49 ++++++++++++++++++++--------------------- 1 file changed, 24 insertions(+), 25 deletions(-)

diff --git a/drivers/mtd/mtd_uboot.c b/drivers/mtd/mtd_uboot.c index 6a3e64395de2..c4434d70520d 100644 --- a/drivers/mtd/mtd_uboot.c +++ b/drivers/mtd/mtd_uboot.c @@ -13,6 +13,29 @@

#define MTD_NAME_MAX_LEN 20

+void board_mtdparts_default(const char **mtdids, const char **mtdparts); + +static const char *get_mtdids(void) +{ + __maybe_unused const char *mtdparts = NULL; + const char *mtdids = env_get("mtdids"); + + if (mtdids) + return mtdids; + +#if defined(CONFIG_SYS_MTDPARTS_RUNTIME) + board_mtdparts_default(&mtdids, &mtdparts); +#elif defined(MTDIDS_DEFAULT) + mtdids = MTDIDS_DEFAULT; +#elif defined(CONFIG_MTDIDS_DEFAULT) + mtdids = CONFIG_MTDIDS_DEFAULT; +#endif + + if (mtdids) + env_set("mtdids", mtdids); + + return mtdids; +}

/** * mtd_search_alternate_name - Search an alternate name for @mtdname thanks to @@ -34,7 +57,7 @@ int mtd_search_alternate_name(const char *mtdname, char *altname, const char *mtdids, *equal, *comma, *dev_id, *mtd_id; int dev_id_len, mtd_id_len;

- mtdids = env_get("mtdids"); + mtdids = get_mtdids(); if (!mtdids) return -EINVAL;

@@ -92,30 +115,6 @@ static void mtd_probe_uclass_mtd_devs(void) { } #endif

#if defined(CONFIG_MTD_PARTITIONS) -extern void board_mtdparts_default(const char **mtdids, - const char **mtdparts); - -static const char *get_mtdids(void) -{ - __maybe_unused const char *mtdparts = NULL; - const char *mtdids = env_get("mtdids"); - - if (mtdids) - return mtdids; - -#if defined(CONFIG_SYS_MTDPARTS_RUNTIME) - board_mtdparts_default(&mtdids, &mtdparts); -#elif defined(MTDIDS_DEFAULT) - mtdids = MTDIDS_DEFAULT; -#elif defined(CONFIG_MTDIDS_DEFAULT) - mtdids = CONFIG_MTDIDS_DEFAULT; -#endif - - if (mtdids) - env_set("mtdids", mtdids); - - return mtdids; -}

#define MTDPARTS_MAXLEN 512

The local mtd_name[] variable is limited in size. Return an error if the name passed in mtdparts does not fit in this local var.

Fixes: 5db66b3aee6f ("cmd: mtd: add 'mtd' command") Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com Tested-by: Heiko Schocher hs@denx.de --- Changes in v4: - Add T-b tag

Changes in v3: - None

Changes in v2: - New patch --- drivers/mtd/mtd_uboot.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/mtd_uboot.c b/drivers/mtd/mtd_uboot.c index d551aee20203..0eda36278309 100644 --- a/drivers/mtd/mtd_uboot.c +++ b/drivers/mtd/mtd_uboot.c @@ -222,8 +222,8 @@ int mtd_probe_devices(void) while (mtdparts[0] != '\0') { char mtd_name[MTD_NAME_MAX_LEN], *colon; struct mtd_partition *parts; - int mtd_name_len, nparts; - int ret; + unsigned int mtd_name_len; + int nparts, ret;

colon = strchr(mtdparts, ':'); if (!colon) { @@ -231,7 +231,12 @@ int mtd_probe_devices(void) return -EINVAL; }

- mtd_name_len = colon - mtdparts; + mtd_name_len = (unsigned int)(colon - mtdparts); + if (mtd_name_len + 1 > sizeof(mtd_name)) { + printf("MTD name too long: %s\n", mtdparts); + return -EINVAL; + } + strncpy(mtd_name, mtdparts, mtd_name_len); mtd_name[mtd_name_len] = '\0'; /* Move the pointer forward (including the ':') */

MTD partition creation code is a bit tricky. It tries to figure out when things have changed (either MTD dev list or mtdparts/mtdids vars) and when that happens it first deletes all the partitions that had been previously created and then creates the new ones based on the new mtdparts/mtdids values. But before deleting the old partitions, it ensures that none of the currently registered parts are being used and bails out when that's not the case. So, we end up in a situation where, if at least one MTD dev has one of its partitions used by someone (UBI for instance), the partitions update logic no longer works for other devs.

Rework the code to relax the logic and allow updates of MTD parts on devices that are not being used (we still refuse to updates parts on devices who have at least one of their partitions used by someone).

Fixes: 5db66b3aee6f ("cmd: mtd: add 'mtd' command") Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com Tested-by: Heiko Schocher hs@denx.de --- Changes in v4: - Add T-b tag

Changes in v3: - None

Changes in v2: - New patch

Changes in v3: - Re-create partitions when our last attempt do delete all existing parts failed. This way we can update parts after ubi detach has been called without having to change mtdparts/mtdids. --- drivers/mtd/mtd_uboot.c | 96 +++++++++++++++++++++++++++++------------ drivers/mtd/mtdpart.c | 12 ++++++ include/linux/mtd/mtd.h | 2 + 3 files changed, 82 insertions(+), 28 deletions(-)

diff --git a/drivers/mtd/mtd_uboot.c b/drivers/mtd/mtd_uboot.c index 6a36948b917b..d638f700d041 100644 --- a/drivers/mtd/mtd_uboot.c +++ b/drivers/mtd/mtd_uboot.c @@ -149,6 +149,54 @@ static const char *get_mtdparts(void) return mtdparts; }

+static int mtd_del_parts(struct mtd_info *mtd, bool quiet) +{ + int ret; + + if (!mtd_has_partitions(mtd)) + return 0; + + /* do not delete partitions if they are in use. */ + if (mtd_partitions_used(mtd)) { + if (!quiet) + printf(""%s" partitions still in use, can't delete them\n", + mtd->name); + return -EACCES; + } + + ret = del_mtd_partitions(mtd); + if (ret) + return ret; + + return 1; +} + +static bool mtd_del_all_parts_failed; + +static void mtd_del_all_parts(void) +{ + struct mtd_info *mtd; + int ret = 0; + + mtd_del_all_parts_failed = false; + + /* + * It is not safe to remove entries from the mtd_for_each_device loop + * as it uses idr indexes and the partitions removal is done in bulk + * (all partitions of one device at the same time), so break and + * iterate from start each time a new partition is found and deleted. + */ + do { + mtd_for_each_device(mtd) { + ret = mtd_del_parts(mtd, false); + if (ret > 0) + break; + else if (ret < 0) + mtd_del_all_parts_failed = true; + } + } while (ret > 0); +} + int mtd_probe_devices(void) { static char *old_mtdparts; @@ -156,18 +204,19 @@ int mtd_probe_devices(void) const char *mtdparts = get_mtdparts(); const char *mtdids = get_mtdids(); const char *mtdparts_next = mtdparts; - bool remaining_partitions = true; struct mtd_info *mtd;

mtd_probe_uclass_mtd_devs();

/* - * Check if mtdparts/mtdids changed or if the MTD dev list was updated - * since last call, otherwise: exit + * Check if mtdparts/mtdids changed, if the MTD dev list was updated + * or if our previous attempt to delete existing partititions failed. + * In any of these cases we want to update the partitions, otherwise, + * everything is up-to-date and we can return 0 directly. */ if ((!mtdparts && !old_mtdparts && !mtdids && !old_mtdids) || (mtdparts && old_mtdparts && mtdids && old_mtdids && - !mtd_dev_list_updated() && + !mtd_dev_list_updated() && !mtd_del_all_parts_failed && !strcmp(mtdparts, old_mtdparts) && !strcmp(mtdids, old_mtdids))) return 0; @@ -178,32 +227,12 @@ int mtd_probe_devices(void) old_mtdparts = strdup(mtdparts); old_mtdids = strdup(mtdids);

- /* If at least one partition is still in use, do not delete anything */ - mtd_for_each_device(mtd) { - if (mtd->usecount) { - printf("Partition "%s" already in use, aborting\n", - mtd->name); - return -EACCES; - } - } - /* - * Everything looks clear, remove all partitions. It is not safe to - * remove entries from the mtd_for_each_device loop as it uses idr - * indexes and the partitions removal is done in bulk (all partitions of - * one device at the same time), so break and iterate from start each - * time a new partition is found and deleted. + * Remove all old parts. Note that partition removal can fail in case + * one of the partition is still being used by an MTD user, so this + * does not guarantee that all old partitions are gone. */ - while (remaining_partitions) { - remaining_partitions = false; - mtd_for_each_device(mtd) { - if (!mtd_is_partition(mtd) && mtd_has_partitions(mtd)) { - del_mtd_partitions(mtd); - remaining_partitions = true; - break; - } - } - } + mtd_del_all_parts();

/* * Call mtd_dev_list_updated() to clear updates generated by our own @@ -278,6 +307,17 @@ int mtd_probe_devices(void) } }

+ /* + * Call mtd_del_parts() again, even if it's already been called + * in mtd_del_all_parts(). We need to know if old partitions are + * still around (because they are still being used by someone), + * and if they are, we shouldn't create new partitions, so just + * skip this MTD device and try the next one. + */ + ret = mtd_del_parts(mtd, true); + if (ret < 0) + continue; + /* * Parse the MTD device partitions. It will update the mtdparts * pointer, create an array of parts (that must be freed), and diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c index 4d2ac8107f01..fd8d8e5ea729 100644 --- a/drivers/mtd/mtdpart.c +++ b/drivers/mtd/mtdpart.c @@ -63,6 +63,18 @@ char *kstrdup(const char *s, gfp_t gfp) #define MTD_SIZE_REMAINING (~0LLU) #define MTD_OFFSET_NOT_SPECIFIED (~0LLU)

+bool mtd_partitions_used(struct mtd_info *master) +{ + struct mtd_info *slave; + + list_for_each_entry(slave, &master->partitions, node) { + if (slave->usecount) + return true; + } + + return false; +} + /** * mtd_parse_partition - Parse @mtdparts partition definition, fill @partition * with it and update the @mtdparts string pointer. diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h index 4d0096d9f1d8..cd1f557a2f31 100644 --- a/include/linux/mtd/mtd.h +++ b/include/linux/mtd/mtd.h @@ -366,6 +366,8 @@ static inline bool mtd_has_partitions(const struct mtd_info *mtd) return !list_empty(&mtd->partitions); }

+bool mtd_partitions_used(struct mtd_info *master); + int mtd_ooblayout_ecc(struct mtd_info *mtd, int section, struct mtd_oob_region *oobecc); int mtd_ooblayout_find_eccregion(struct mtd_info *mtd, int eccbyte,

SPI flash based MTD devs can be registered/unregistered at any time through the sf probe command or the spi_flash_free() function.

This commit does not try to fix the root cause as it would probably require rewriting most of the code and have an mtd_info object instance per spi_flash object (not to mention that the the spi-flash layer is likely to be replaced by a spi-nor layer ported from Linux).

Instead, we try to be as safe as can be by checking the code returned by del_mtd_device() and complain loudly when there's nothing we can do about the deregistration failure. When that happens we also reset sf_mtd_info.priv to NULL, and check for NULL pointer in the mtd hooks so that -ENODEV is returned instead of hitting a NULL pointer dereference exception when the MTD instance is later accessed by a user.

Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com Tested-by: Heiko Schocher hs@denx.de --- Changes in v4: - Add T-b tags

Changes in v3: - New patch --- drivers/mtd/spi/sf_mtd.c | 39 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 36 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/spi/sf_mtd.c b/drivers/mtd/spi/sf_mtd.c index aabbc3589435..68c36002bee2 100644 --- a/drivers/mtd/spi/sf_mtd.c +++ b/drivers/mtd/spi/sf_mtd.c @@ -18,6 +18,9 @@ static int spi_flash_mtd_erase(struct mtd_info *mtd, struct erase_info *instr) struct spi_flash *flash = mtd->priv; int err;

+ if (!flash) + return -ENODEV; + instr->state = MTD_ERASING;

err = spi_flash_erase(flash, instr->addr, instr->len); @@ -39,6 +42,9 @@ static int spi_flash_mtd_read(struct mtd_info *mtd, loff_t from, size_t len, struct spi_flash *flash = mtd->priv; int err;

+ if (!flash) + return -ENODEV; + err = spi_flash_read(flash, from, len, buf); if (!err) *retlen = len; @@ -52,6 +58,9 @@ static int spi_flash_mtd_write(struct mtd_info *mtd, loff_t to, size_t len, struct spi_flash *flash = mtd->priv; int err;

+ if (!flash) + return -ENODEV; + err = spi_flash_write(flash, to, len, buf); if (!err) *retlen = len; @@ -76,8 +85,13 @@ int spi_flash_mtd_register(struct spi_flash *flash) { int ret;

- if (sf_mtd_registered) - del_mtd_device(&sf_mtd_info); + if (sf_mtd_registered) { + ret = del_mtd_device(&sf_mtd_info); + if (ret) + return ret; + + sf_mtd_registered = false; + }

sf_mtd_registered = false; memset(&sf_mtd_info, 0, sizeof(sf_mtd_info)); @@ -110,5 +124,24 @@ int spi_flash_mtd_register(struct spi_flash *flash)

void spi_flash_mtd_unregister(void) { - del_mtd_device(&sf_mtd_info); + int ret; + + if (!sf_mtd_registered) + return; + + ret = del_mtd_device(&sf_mtd_info); + if (!ret) { + sf_mtd_registered = false; + return; + } + + /* + * Setting mtd->priv to NULL is the best we can do. Thanks to that, + * the MTD layer can still call mtd hooks without risking a + * use-after-free bug. Still, things should be fixed to prevent the + * spi_flash object from being destroyed when del_mtd_device() fails. + */ + sf_mtd_info.priv = NULL; + printf("Failed to unregister MTD %s and the spi_flash object is going away: you're in deep trouble!", + sf_mtd_info.name); }