[PATCH v1] android_ab: don't ignore ab_control_store return code

[PATCH v1] android_ab: don't ignore ab_control_store return code

[Alexey Romanov]

ab_control_store() can return an error if writing to disk fails.
In this case, we have to pass the error code to the caller.

Signed-off-by: Alexey Romanov <avromanov@salutedevices.com>
---
 boot/android_ab.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/boot/android_ab.c b/boot/android_ab.c
index 73b55c196c..5a3152dd53 100644
--- a/boot/android_ab.c
+++ b/boot/android_ab.c
@@ -337,7 +337,15 @@ int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info,
 
 	if (store_needed) {
 		abc->crc32_le = ab_control_compute_crc(abc);
-		ab_control_store(dev_desc, part_info, abc, 0);
+		ret = ab_control_store(dev_desc, part_info, abc, 0);
+		if (ret < 0) {
+#if ANDROID_AB_BACKUP_OFFSET
+			free(backup_abc);
+#endif
+			free(abc);
+			log_err("ANDROID: failed to store boot control block: %d\n", ret);
+			return ret;
+		}
 	}
 
 #if ANDROID_AB_BACKUP_OFFSET
@@ -346,8 +354,14 @@ int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info,
 	 * to the backup offset
 	 */
 	if (memcmp(backup_abc, abc, sizeof(*abc)) != 0) {
-		ab_control_store(dev_desc, part_info, abc,
+		ret = ab_control_store(dev_desc, part_info, abc,
 				 ANDROID_AB_BACKUP_OFFSET);
+		if (ret < 0) {
+			free(backup_abc);
+			free(abc);
+			log_err("ANDROID: failed to store boot control block: %d\n", ret);
+			return ret;
+		}
 	}
 	free(backup_abc);
 #endif
-- 
2.39.2

Re: [PATCH v1] android_ab: don't ignore ab_control_store return code

[Alexey Romanov]

Hello!

Really sorry for the noise, but please, take a look at my patch :)

On Tue, Nov 28, 2023 at 02:26:34PM +0300, Alexey Romanov wrote:
> ab_control_store() can return an error if writing to disk fails.
> In this case, we have to pass the error code to the caller.
> 
> Signed-off-by: Alexey Romanov <avromanov@salutedevices.com>
> ---
>  boot/android_ab.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/boot/android_ab.c b/boot/android_ab.c
> index 73b55c196c..5a3152dd53 100644
> --- a/boot/android_ab.c
> +++ b/boot/android_ab.c
> @@ -337,7 +337,15 @@ int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info,
>  
>  	if (store_needed) {
>  		abc->crc32_le = ab_control_compute_crc(abc);
> -		ab_control_store(dev_desc, part_info, abc, 0);
> +		ret = ab_control_store(dev_desc, part_info, abc, 0);
> +		if (ret < 0) {
> +#if ANDROID_AB_BACKUP_OFFSET
> +			free(backup_abc);
> +#endif
> +			free(abc);
> +			log_err("ANDROID: failed to store boot control block: %d\n", ret);
> +			return ret;
> +		}
>  	}
>  
>  #if ANDROID_AB_BACKUP_OFFSET
> @@ -346,8 +354,14 @@ int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info,
>  	 * to the backup offset
>  	 */
>  	if (memcmp(backup_abc, abc, sizeof(*abc)) != 0) {
> -		ab_control_store(dev_desc, part_info, abc,
> +		ret = ab_control_store(dev_desc, part_info, abc,
>  				 ANDROID_AB_BACKUP_OFFSET);
> +		if (ret < 0) {
> +			free(backup_abc);
> +			free(abc);
> +			log_err("ANDROID: failed to store boot control block: %d\n", ret);
> +			return ret;
> +		}
>  	}
>  	free(backup_abc);
>  #endif
> -- 
> 2.39.2
> 

-- 
Thank you,
Alexey

Re: [PATCH v1] android_ab: don't ignore ab_control_store return code

[Mattijs Korpershoek]

Hi Alexey,

Thank you for your patch.

I'm looping in the Android AB reviewers and maintainers (Sam and Igor).

They were not automatically add due to a stale MAINTAINERS entry[1]

[1] https://lore.kernel.org/all/20231220-maintainers-fix-ab-v1-1-dd16fc5285ed@baylibre.com/

On Tue, Nov 28, 2023 at 14:26, Alexey Romanov <avromanov@salutedevices.com> wrote:

> ab_control_store() can return an error if writing to disk fails.
> In this case, we have to pass the error code to the caller.
>
> Signed-off-by: Alexey Romanov <avromanov@salutedevices.com>
> ---
>  boot/android_ab.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/boot/android_ab.c b/boot/android_ab.c
> index 73b55c196c..5a3152dd53 100644
> --- a/boot/android_ab.c
> +++ b/boot/android_ab.c
> @@ -337,7 +337,15 @@ int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info,
>  
>  	if (store_needed) {
>  		abc->crc32_le = ab_control_compute_crc(abc);
> -		ab_control_store(dev_desc, part_info, abc, 0);
> +		ret = ab_control_store(dev_desc, part_info, abc, 0);
> +		if (ret < 0) {
> +#if ANDROID_AB_BACKUP_OFFSET
> +			free(backup_abc);
> +#endif
> +			free(abc);
> +			log_err("ANDROID: failed to store boot control block: %d\n", ret);

There is no point in logging here, because ab_control_store() already
logs:

	ret = blk_dwrite(dev_desc, part_info->start + abc_offset, abc_blocks,
			 abc);
	if (IS_ERR_VALUE(ret)) {
		log_err("ANDROID: Could not write back the misc partition\n");
		return -EIO;
	}

> +			return ret;
> +		}
>  	}
>  
>  #if ANDROID_AB_BACKUP_OFFSET
> @@ -346,8 +354,14 @@ int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info,
>  	 * to the backup offset
>  	 */
>  	if (memcmp(backup_abc, abc, sizeof(*abc)) != 0) {
> -		ab_control_store(dev_desc, part_info, abc,
> +		ret = ab_control_store(dev_desc, part_info, abc,
>  				 ANDROID_AB_BACKUP_OFFSET);
> +		if (ret < 0) {
> +			free(backup_abc);
> +			free(abc);
> +			log_err("ANDROID: failed to store boot control block: %d\n", ret);

Same here

With that addressed:

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

> +			return ret;
> +		}
>  	}
>  	free(backup_abc);
>  #endif
> -- 
> 2.39.2