[PATCH] fastboot: inform user if barebox update falls back to raw copy

[PATCH] fastboot: inform user if barebox update falls back to raw copy

[Ahmad Fatoum]

Fastboot code determines whether to go into barebox update handler by
checking whether filetype_is_barebox_image() returns true for the image.
This is different from the barebox_update command which leaves it to the
individual handlers to decide whether an image is suitable.

Coupled with eMMC boot partitions, this could lead to
`barebox_update -t mmc` updating the boot partition, while a
`fastboot flash bbu-mmc` will write the image to the user area instead.

It may be worth breaking backwards compatibility here and refuse update
in that case, but for now at least alert the user that a raw copy is
being attempted.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 common/fastboot.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/common/fastboot.c b/common/fastboot.c
index f8ed40c86e00..50ea8421d92c 100644
--- a/common/fastboot.c
+++ b/common/fastboot.c
@@ -673,7 +673,7 @@ static void cb_flash(struct fastboot *fb, const char *cmd)
 		goto out;
 	}
 
-	if (IS_ENABLED(CONFIG_BAREBOX_UPDATE) && filetype_is_barebox_image(filetype)) {
+	if (IS_ENABLED(CONFIG_BAREBOX_UPDATE)) {
 		void *buf;
 		struct bbu_handler *handler;
 		struct bbu_data data = {
@@ -681,6 +681,17 @@ static void cb_flash(struct fastboot *fb, const char *cmd)
 			.flags = BBU_FLAG_YES,
 		};
 
+		if (!filetype_is_barebox_image(filetype)) {
+			if (strstarts(fentry->name, "bbu-")) {
+				fastboot_tx_print(fb, FASTBOOT_MSG_INFO,
+						  "This is _not_ a barebox image...");
+				fastboot_tx_print(fb, FASTBOOT_MSG_INFO,
+						  "Falling back to raw copy to %s", filename);
+			}
+
+			goto copy;
+		}
+
 		handler = bbu_find_handler_by_device(data.devicefile);
 		if (!handler)
 			goto copy;
-- 
2.30.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] fastboot: inform user if barebox update falls back to raw copy

[Ahmad Fatoum]

Hello Sascha,

On 31.05.22 11:35, Ahmad Fatoum wrote:
> Fastboot code determines whether to go into barebox update handler by
> checking whether filetype_is_barebox_image() returns true for the image.
> This is different from the barebox_update command which leaves it to the
> individual handlers to decide whether an image is suitable.
> 
> Coupled with eMMC boot partitions, this could lead to
> `barebox_update -t mmc` updating the boot partition, while a
> `fastboot flash bbu-mmc` will write the image to the user area instead.
> 
> It may be worth breaking backwards compatibility here and refuse update
> in that case, but for now at least alert the user that a raw copy is
> being attempted.

Any comments?

> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  common/fastboot.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/common/fastboot.c b/common/fastboot.c
> index f8ed40c86e00..50ea8421d92c 100644
> --- a/common/fastboot.c
> +++ b/common/fastboot.c
> @@ -673,7 +673,7 @@ static void cb_flash(struct fastboot *fb, const char *cmd)
>  		goto out;
>  	}
>  
> -	if (IS_ENABLED(CONFIG_BAREBOX_UPDATE) && filetype_is_barebox_image(filetype)) {
> +	if (IS_ENABLED(CONFIG_BAREBOX_UPDATE)) {
>  		void *buf;
>  		struct bbu_handler *handler;
>  		struct bbu_data data = {
> @@ -681,6 +681,17 @@ static void cb_flash(struct fastboot *fb, const char *cmd)
>  			.flags = BBU_FLAG_YES,
>  		};
>  
> +		if (!filetype_is_barebox_image(filetype)) {
> +			if (strstarts(fentry->name, "bbu-")) {
> +				fastboot_tx_print(fb, FASTBOOT_MSG_INFO,
> +						  "This is _not_ a barebox image...");
> +				fastboot_tx_print(fb, FASTBOOT_MSG_INFO,
> +						  "Falling back to raw copy to %s", filename);
> +			}
> +
> +			goto copy;
> +		}
> +
>  		handler = bbu_find_handler_by_device(data.devicefile);
>  		if (!handler)
>  			goto copy;


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

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] fastboot: inform user if barebox update falls back to raw copy

[Sascha Hauer]

On Tue, May 31, 2022 at 11:35:37AM +0200, Ahmad Fatoum wrote:
> Fastboot code determines whether to go into barebox update handler by
> checking whether filetype_is_barebox_image() returns true for the image.
> This is different from the barebox_update command which leaves it to the
> individual handlers to decide whether an image is suitable.
> 
> Coupled with eMMC boot partitions, this could lead to
> `barebox_update -t mmc` updating the boot partition, while a
> `fastboot flash bbu-mmc` will write the image to the user area instead.
> 
> It may be worth breaking backwards compatibility here and refuse update
> in that case, but for now at least alert the user that a raw copy is
> being attempted.
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  common/fastboot.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/common/fastboot.c b/common/fastboot.c
> index f8ed40c86e00..50ea8421d92c 100644
> --- a/common/fastboot.c
> +++ b/common/fastboot.c
> @@ -673,7 +673,7 @@ static void cb_flash(struct fastboot *fb, const char *cmd)
>  		goto out;
>  	}
>  
> -	if (IS_ENABLED(CONFIG_BAREBOX_UPDATE) && filetype_is_barebox_image(filetype)) {
> +	if (IS_ENABLED(CONFIG_BAREBOX_UPDATE)) {


>  		void *buf;
>  		struct bbu_handler *handler;
>  		struct bbu_data data = {
> @@ -681,6 +681,17 @@ static void cb_flash(struct fastboot *fb, const char *cmd)
>  			.flags = BBU_FLAG_YES,
>  		};
>  
> +		if (!filetype_is_barebox_image(filetype)) {
> +			if (strstarts(fentry->name, "bbu-")) {
> +				fastboot_tx_print(fb, FASTBOOT_MSG_INFO,
> +						  "This is _not_ a barebox image...");
> +				fastboot_tx_print(fb, FASTBOOT_MSG_INFO,
> +						  "Falling back to raw copy to %s", filename);
> +			}
> +
> +			goto copy;
> +		}

The handlers starting with "bbu-" are registered from the barebox update
code, so falling back to copy is clearly wrong.

The logic to enter this barebox_update path should be:

	if (IS_ENABLED(CONFIG_BAREBOX_UPDATE) &&
	    (filetype_is_barebox_image(filetype) || strstarts(fentry->name, "bbu-")))

That way we are sure that barebox_update is called when we need it. Also
behaviour is consistent to what barebox_update does on the command line.

Sascha

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

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] fastboot: inform user if barebox update falls back to raw copy

[Ahmad Fatoum]

Hello Sascha,

On 09.06.22 09:19, Sascha Hauer wrote:
> On Tue, May 31, 2022 at 11:35:37AM +0200, Ahmad Fatoum wrote:
>> Fastboot code determines whether to go into barebox update handler by
>> checking whether filetype_is_barebox_image() returns true for the image.
>> This is different from the barebox_update command which leaves it to the
>> individual handlers to decide whether an image is suitable.
>>
>> Coupled with eMMC boot partitions, this could lead to
>> `barebox_update -t mmc` updating the boot partition, while a
>> `fastboot flash bbu-mmc` will write the image to the user area instead.
>>
>> It may be worth breaking backwards compatibility here and refuse update
>> in that case, but for now at least alert the user that a raw copy is
>> being attempted.
>>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>>  common/fastboot.c | 13 ++++++++++++-
>>  1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/common/fastboot.c b/common/fastboot.c
>> index f8ed40c86e00..50ea8421d92c 100644
>> --- a/common/fastboot.c
>> +++ b/common/fastboot.c
>> @@ -673,7 +673,7 @@ static void cb_flash(struct fastboot *fb, const char *cmd)
>>  		goto out;
>>  	}
>>  
>> -	if (IS_ENABLED(CONFIG_BAREBOX_UPDATE) && filetype_is_barebox_image(filetype)) {
>> +	if (IS_ENABLED(CONFIG_BAREBOX_UPDATE)) {
> 
> 
>>  		void *buf;
>>  		struct bbu_handler *handler;
>>  		struct bbu_data data = {
>> @@ -681,6 +681,17 @@ static void cb_flash(struct fastboot *fb, const char *cmd)
>>  			.flags = BBU_FLAG_YES,
>>  		};
>>  
>> +		if (!filetype_is_barebox_image(filetype)) {
>> +			if (strstarts(fentry->name, "bbu-")) {
>> +				fastboot_tx_print(fb, FASTBOOT_MSG_INFO,
>> +						  "This is _not_ a barebox image...");
>> +				fastboot_tx_print(fb, FASTBOOT_MSG_INFO,
>> +						  "Falling back to raw copy to %s", filename);
>> +			}
>> +
>> +			goto copy;
>> +		}
> 
> The handlers starting with "bbu-" are registered from the barebox update
> code, so falling back to copy is clearly wrong.
> 
> The logic to enter this barebox_update path should be:
> 
> 	if (IS_ENABLED(CONFIG_BAREBOX_UPDATE) &&
> 	    (filetype_is_barebox_image(filetype) || strstarts(fentry->name, "bbu-")))
> 
> That way we are sure that barebox_update is called when we need it. Also
> behaviour is consistent to what barebox_update does on the command line.

This is not consistent with barebox_update behavior. Calling it from command line
will leave it to update handlers to determine whether an image is a barebox
image. filetype_is_barebox_image() is exclusively used for fastboot.

I don't like the current behavior either, but I am wary of breaking existing
users, so I figured an info message would be a stop-gap solution for now.

You can add a

  pr_warn("fastboot: deprecated raw copy fallback for barebox_update\n");

for now. What do you think?

Cheers,
Ahmad

> 
> Sascha
> 


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

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] fastboot: inform user if barebox update falls back to raw copy

[Sascha Hauer]

On Thu, Jun 09, 2022 at 10:21:46AM +0200, Ahmad Fatoum wrote:
> Hello Sascha,
> 
> On 09.06.22 09:19, Sascha Hauer wrote:
> > On Tue, May 31, 2022 at 11:35:37AM +0200, Ahmad Fatoum wrote:
> >> Fastboot code determines whether to go into barebox update handler by
> >> checking whether filetype_is_barebox_image() returns true for the image.
> >> This is different from the barebox_update command which leaves it to the
> >> individual handlers to decide whether an image is suitable.
> >>
> >> Coupled with eMMC boot partitions, this could lead to
> >> `barebox_update -t mmc` updating the boot partition, while a
> >> `fastboot flash bbu-mmc` will write the image to the user area instead.
> >>
> >> It may be worth breaking backwards compatibility here and refuse update
> >> in that case, but for now at least alert the user that a raw copy is
> >> being attempted.
> >>
> >> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> >> ---
> >>  common/fastboot.c | 13 ++++++++++++-
> >>  1 file changed, 12 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/common/fastboot.c b/common/fastboot.c
> >> index f8ed40c86e00..50ea8421d92c 100644
> >> --- a/common/fastboot.c
> >> +++ b/common/fastboot.c
> >> @@ -673,7 +673,7 @@ static void cb_flash(struct fastboot *fb, const char *cmd)
> >>  		goto out;
> >>  	}
> >>  
> >> -	if (IS_ENABLED(CONFIG_BAREBOX_UPDATE) && filetype_is_barebox_image(filetype)) {
> >> +	if (IS_ENABLED(CONFIG_BAREBOX_UPDATE)) {
> > 
> > 
> >>  		void *buf;
> >>  		struct bbu_handler *handler;
> >>  		struct bbu_data data = {
> >> @@ -681,6 +681,17 @@ static void cb_flash(struct fastboot *fb, const char *cmd)
> >>  			.flags = BBU_FLAG_YES,
> >>  		};
> >>  
> >> +		if (!filetype_is_barebox_image(filetype)) {
> >> +			if (strstarts(fentry->name, "bbu-")) {
> >> +				fastboot_tx_print(fb, FASTBOOT_MSG_INFO,
> >> +						  "This is _not_ a barebox image...");
> >> +				fastboot_tx_print(fb, FASTBOOT_MSG_INFO,
> >> +						  "Falling back to raw copy to %s", filename);
> >> +			}
> >> +
> >> +			goto copy;
> >> +		}
> > 
> > The handlers starting with "bbu-" are registered from the barebox update
> > code, so falling back to copy is clearly wrong.
> > 
> > The logic to enter this barebox_update path should be:
> > 
> > 	if (IS_ENABLED(CONFIG_BAREBOX_UPDATE) &&
> > 	    (filetype_is_barebox_image(filetype) || strstarts(fentry->name, "bbu-")))
> > 
> > That way we are sure that barebox_update is called when we need it. Also
> > behaviour is consistent to what barebox_update does on the command line.
> 
> This is not consistent with barebox_update behavior. Calling it from command line
> will leave it to update handlers to determine whether an image is a barebox
> image. filetype_is_barebox_image() is exclusively used for fastboot.

With my suggestion we also leave it to the update handlers to decide
whether an image is suitable or not.

Sascha

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

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox