Re: [PATCH] devfs: Do not create overlapping partitions

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

[-- 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 --]