Until now it has been possible to create overlapping partitions. Go away from that and allow to create partitions only in unallocated areas of a device. This lowers the risk of having inconsistent partitioning and increases the chance that inconsistent partitioning is recognized by the user. We had explicit overlap checking for the environment partition which becomes unnecessary with this change and is removed. Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> --- common/startup.c | 70 +----------------------------------------------- fs/devfs-core.c | 34 +++++++++++++++++++++++ 2 files changed, 35 insertions(+), 69 deletions(-) diff --git a/common/startup.c b/common/startup.c index f72902fc53..f53b73f81a 100644 --- a/common/startup.c +++ b/common/startup.c @@ -68,70 +68,6 @@ static int mount_root(void) fs_initcall(mount_root); #endif -#ifdef CONFIG_ENV_HANDLING -static bool region_overlap(loff_t starta, loff_t lena, - loff_t startb, loff_t lenb) -{ - if (starta + lena <= startb) - return 0; - if (startb + lenb <= starta) - return 0; - return 1; -} - -static int check_overlap(const char *path) -{ - struct cdev *cenv, *cdisk, *cpart; - const char *name; - - name = devpath_to_name(path); - - if (name == path) - /* - * no /dev/ in front, so *path is some file. No need to - * check further. - */ - return 0; - - cenv = cdev_by_name(name); - if (!cenv) - return -EINVAL; - - if (cenv->mtd) - return 0; - - cdisk = cenv->master; - - if (!cdisk) - return 0; - - list_for_each_entry(cpart, &cdisk->partitions, partition_entry) { - if (cpart == cenv) - continue; - - if (region_overlap(cpart->offset, cpart->size, - cenv->offset, cenv->size)) - goto conflict; - } - - return 0; - -conflict: - pr_err("Environment partition (0x%08llx-0x%08llx) " - "overlaps with partition %s (0x%08llx-0x%08llx), not using it\n", - cenv->offset, cenv->offset + cenv->size - 1, - cpart->name, - cpart->offset, cpart->offset + cpart->size - 1); - - return -EINVAL; -} -#else -static int check_overlap(const char *path) -{ - return 0; -} -#endif - static int load_environment(void) { const char *default_environment_path; @@ -143,11 +79,7 @@ static int load_environment(void) defaultenv_load("/env", 0); if (IS_ENABLED(CONFIG_ENV_HANDLING)) { - ret = check_overlap(default_environment_path); - if (ret) - default_environment_path_set(NULL); - else - envfs_load(default_environment_path, "/env", 0); + envfs_load(default_environment_path, "/env", 0); } else { if (IS_ENABLED(CONFIG_DEFAULT_ENVIRONMENT)) pr_notice("No support for persistent environment. Using default environment\n"); diff --git a/fs/devfs-core.c b/fs/devfs-core.c index 30ad0e0508..3715e543e6 100644 --- a/fs/devfs-core.c +++ b/fs/devfs-core.c @@ -301,6 +301,37 @@ int devfs_remove(struct cdev *cdev) return 0; } +static bool region_overlap(loff_t starta, loff_t lena, + loff_t startb, loff_t lenb) +{ + if (starta + lena <= startb) + return 0; + if (startb + lenb <= starta) + return 0; + return 1; +} + +static int check_overlap(struct cdev *cdev, const char *name, loff_t offset, loff_t size) +{ + struct cdev *cpart; + + list_for_each_entry(cpart, &cdev->partitions, partition_entry) { + if (region_overlap(cpart->offset, cpart->size, + offset, size)) + goto conflict; + } + + return 0; + +conflict: + pr_err("New partition %s (0x%08llx-0x%08llx) on %s " + "overlaps with partition %s (0x%08llx-0x%08llx), not creating it\n", + name, offset, offset + size - 1, cpart->name, + cpart->name, cpart->offset, cpart->offset + cpart->size - 1); + + return -EINVAL; +} + static struct cdev *__devfs_add_partition(struct cdev *cdev, const struct devfs_partition *partinfo, loff_t *end) { @@ -336,6 +367,9 @@ static struct cdev *__devfs_add_partition(struct cdev *cdev, return ERR_PTR(-EINVAL); } + if (check_overlap(cdev, partinfo->name, offset, size)) + return ERR_PTR(-EINVAL); + if (IS_ENABLED(CONFIG_MTD) && cdev->mtd) { struct mtd_info *mtd; -- 2.30.2 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
[-- Attachment #1: Type: text/plain, Size: 5360 bytes --] On Mon, Oct 11, 2021 at 09:30:25AM +0200, Sascha Hauer wrote: > Until now it has been possible to create overlapping partitions. Go away > from that and allow to create partitions only in unallocated areas of a > device. This lowers the risk of having inconsistent partitioning and > increases the chance that inconsistent partitioning is recognized by > the user. > > We had explicit overlap checking for the environment partition which > becomes unnecessary with this change and is removed. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> This is now commit 7f9f45b9bfef2ce8706f25c46dcbd109e4181164. Still some feedback: > --- > common/startup.c | 70 +----------------------------------------------- > fs/devfs-core.c | 34 +++++++++++++++++++++++ > 2 files changed, 35 insertions(+), 69 deletions(-) > > diff --git a/common/startup.c b/common/startup.c > index f72902fc53..f53b73f81a 100644 > --- a/common/startup.c > +++ b/common/startup.c > @@ -68,70 +68,6 @@ static int mount_root(void) > fs_initcall(mount_root); > #endif > > -#ifdef CONFIG_ENV_HANDLING > -static bool region_overlap(loff_t starta, loff_t lena, > - loff_t startb, loff_t lenb) > -{ > - if (starta + lena <= startb) > - return 0; > - if (startb + lenb <= starta) > - return 0; > - return 1; > -} > - > -static int check_overlap(const char *path) > -{ > - struct cdev *cenv, *cdisk, *cpart; > - const char *name; > - > - name = devpath_to_name(path); > - > - if (name == path) > - /* > - * no /dev/ in front, so *path is some file. No need to > - * check further. > - */ > - return 0; > - > - cenv = cdev_by_name(name); > - if (!cenv) > - return -EINVAL; > - > - if (cenv->mtd) > - return 0; > - > - cdisk = cenv->master; > - > - if (!cdisk) > - return 0; > - > - list_for_each_entry(cpart, &cdisk->partitions, partition_entry) { > - if (cpart == cenv) > - continue; > - > - if (region_overlap(cpart->offset, cpart->size, > - cenv->offset, cenv->size)) > - goto conflict; > - } > - > - return 0; > - > -conflict: > - pr_err("Environment partition (0x%08llx-0x%08llx) " > - "overlaps with partition %s (0x%08llx-0x%08llx), not using it\n", > - cenv->offset, cenv->offset + cenv->size - 1, > - cpart->name, > - cpart->offset, cpart->offset + cpart->size - 1); > - > - return -EINVAL; > -} > -#else > -static int check_overlap(const char *path) > -{ > - return 0; > -} > -#endif > - > static int load_environment(void) > { > const char *default_environment_path; > @@ -143,11 +79,7 @@ static int load_environment(void) > defaultenv_load("/env", 0); > > if (IS_ENABLED(CONFIG_ENV_HANDLING)) { > - ret = check_overlap(default_environment_path); > - if (ret) > - default_environment_path_set(NULL); > - else > - envfs_load(default_environment_path, "/env", 0); > + envfs_load(default_environment_path, "/env", 0); > } else { > if (IS_ENABLED(CONFIG_DEFAULT_ENVIRONMENT)) > pr_notice("No support for persistent environment. Using default environment\n"); > diff --git a/fs/devfs-core.c b/fs/devfs-core.c > index 30ad0e0508..3715e543e6 100644 > --- a/fs/devfs-core.c > +++ b/fs/devfs-core.c > @@ -301,6 +301,37 @@ int devfs_remove(struct cdev *cdev) > return 0; > } > > +static bool region_overlap(loff_t starta, loff_t lena, > + loff_t startb, loff_t lenb) > +{ > + if (starta + lena <= startb) > + return 0; > + if (startb + lenb <= starta) > + return 0; > + return 1; > +} > + > +static int check_overlap(struct cdev *cdev, const char *name, loff_t offset, loff_t size) > +{ > + struct cdev *cpart; > + > + list_for_each_entry(cpart, &cdev->partitions, partition_entry) { > + if (region_overlap(cpart->offset, cpart->size, > + offset, size)) > + goto conflict; > + } > + > + return 0; > + > +conflict: > + pr_err("New partition %s (0x%08llx-0x%08llx) on %s " > + "overlaps with partition %s (0x%08llx-0x%08llx), not creating it\n", > + name, offset, offset + size - 1, cpart->name, > + cpart->name, cpart->offset, cpart->offset + cpart->size - 1); The first cpart->name should be cdev->name. This warning wrongly triggers for a machine after updating 2021.07.0 -> 2022.05.0 here: bootloader: echo $nand0.partitions 1M(barebox),1M(environment),80M@4480k(root),44672k(var) bootloader: addpart /dev/nand0 0x260000@0x00200000(history) nand0: __devfs_add_partition:422: offset=200000, size=260000, name=nand0.history New partition nand0.history (0x00200000-0x0045ffff) on nand0 overlaps with partition nand0.root (0x00000000-0x04ffffff), not creating it cannot create nand0.history: Invalid argument addpart: Invalid argument The problem seems to be that for mtd partitions(?) the offset is 0 while in reality the offset of the "root" partition is 4480*1024 = 0x460000 (relative to the master mtd device at least). I didn't test, but I guess - if (cenv->mtd) - return 0; in the old check_overlap function made this problem not appear before 7f9f45b9bfef2ce8706f25c46dcbd109e4181164. Do you have a suggestion how to best fix this? Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --]
mtd partitions are separate devices. Their partition info is relative to these subdevices and so offset is 0 for them. This needs to be taken into account for the overlap check. Fixes: 7f9f45b9bfef ("devfs: Do not create overlapping partitions") Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- fs/devfs-core.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/fs/devfs-core.c b/fs/devfs-core.c index 38423e5d1e07..dca5e10191d4 100644 --- a/fs/devfs-core.c +++ b/fs/devfs-core.c @@ -395,9 +395,20 @@ static bool region_overlap(loff_t starta, loff_t lena, static int check_overlap(struct cdev *cdev, const char *name, loff_t offset, loff_t size) { struct cdev *cpart; + loff_t cpart_offset; list_for_each_entry(cpart, &cdev->partitions, partition_entry) { - if (region_overlap(cpart->offset, cpart->size, + cpart_offset = cpart->offset; + + /* + * An mtd partition is represented by a separate cdev and its + * cpart is relative to this one. So its .offset is 0 and we + * have to consult .master_offset to get its offset. + */ + if (cpart->mtd) + cpart_offset = cpart->mtd->master_offset; + + if (region_overlap(cpart_offset, cpart->size, offset, size)) goto conflict; } @@ -408,7 +419,7 @@ conflict: pr_err("New partition %s (0x%08llx-0x%08llx) on %s " "overlaps with partition %s (0x%08llx-0x%08llx), not creating it\n", name, offset, offset + size - 1, cpart->name, - cpart->name, cpart->offset, cpart->offset + cpart->size - 1); + cpart->name, cpart_offset, cpart_offset + cpart->size - 1); return -EINVAL; } -- 2.36.1
cpart->name contains the partition name, not the device name. Use cdev->name instead. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- fs/devfs-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/devfs-core.c b/fs/devfs-core.c index dca5e10191d4..1d8b2664f216 100644 --- a/fs/devfs-core.c +++ b/fs/devfs-core.c @@ -418,7 +418,7 @@ static int check_overlap(struct cdev *cdev, const char *name, loff_t offset, lof conflict: pr_err("New partition %s (0x%08llx-0x%08llx) on %s " "overlaps with partition %s (0x%08llx-0x%08llx), not creating it\n", - name, offset, offset + size - 1, cpart->name, + name, offset, offset + size - 1, cdev->name, cpart->name, cpart_offset, cpart_offset + cpart->size - 1); return -EINVAL; -- 2.36.1
On Mon, Jul 11, 2022 at 11:09:14AM +0200, Uwe Kleine-König wrote: > mtd partitions are separate devices. Their partition info is relative to > these subdevices and so offset is 0 for them. This needs to be taken > into account for the overlap check. > > Fixes: 7f9f45b9bfef ("devfs: Do not create overlapping partitions") > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > fs/devfs-core.c | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) Applied, thanks Sascha > > diff --git a/fs/devfs-core.c b/fs/devfs-core.c > index 38423e5d1e07..dca5e10191d4 100644 > --- a/fs/devfs-core.c > +++ b/fs/devfs-core.c > @@ -395,9 +395,20 @@ static bool region_overlap(loff_t starta, loff_t lena, > static int check_overlap(struct cdev *cdev, const char *name, loff_t offset, loff_t size) > { > struct cdev *cpart; > + loff_t cpart_offset; > > list_for_each_entry(cpart, &cdev->partitions, partition_entry) { > - if (region_overlap(cpart->offset, cpart->size, > + cpart_offset = cpart->offset; > + > + /* > + * An mtd partition is represented by a separate cdev and its > + * cpart is relative to this one. So its .offset is 0 and we > + * have to consult .master_offset to get its offset. > + */ > + if (cpart->mtd) > + cpart_offset = cpart->mtd->master_offset; > + > + if (region_overlap(cpart_offset, cpart->size, > offset, size)) > goto conflict; > } > @@ -408,7 +419,7 @@ conflict: > pr_err("New partition %s (0x%08llx-0x%08llx) on %s " > "overlaps with partition %s (0x%08llx-0x%08llx), not creating it\n", > name, offset, offset + size - 1, cpart->name, > - cpart->name, cpart->offset, cpart->offset + cpart->size - 1); > + cpart->name, cpart_offset, cpart_offset + cpart->size - 1); > > return -EINVAL; > } > -- > 2.36.1 > > > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |