[OpenWrt-Devel,v2] package/base-files: caldata: work around dd's limitation

tl;dr: dd will silently truncate the output if reading from special
files (e.g. sysfs attributes) with a too large bs parameter.

This problem was exposed on some RouterBOARD ipq40xx devices which use a
caldata payload which is larger than PAGE_SIZE, contrary to all other
currently supported RouterBOARD devices: the caldata would fail to
properly load with the current scripts.

Background: dd doesn't seem to correctly handle read() results that
return less than requested data. sysfs attributes have a kernel exchange
buffer which is at most PAGE_SIZE big, so only 1 page can be read() at a
time. In this case, if bs is larger than PAGE_SIZE, dd will silently
truncate blocks to PAGE_SIZE. With the current scripts using bs=<size>
count=1, the data is truncated to PAGE_SIZE as soon as the requested
<size> exceeds this value.

This commit works around this problem by using `cat` in the caldata
routines that can read from a file (routines that read from mtd devices
are untouched). cat correctly handles partial read requests. The output
is then piped to dd with the same parameters as before, to ensure that
the resulting file remains exactly the same.

This is a simple workaround, the downside is that it uses a pipe and one
more executable, and therefore has a larger memory footprint and is
slower. This is deemed acceptable considering these routines are only
used at boot time.

Tested-by: Robert Marko <robimarko@gmail.com>
Signed-off-by: Thibaut VARÈNE <hacks@slashdirt.org>
---
v2: leave a comment in scripts
---
 package/base-files/Makefile                       | 2 +-
 package/base-files/files/lib/functions/caldata.sh | 8 +++++---
 2 files changed, 6 insertions(+), 4 deletions(-)

Hi Thibaut,

Le dim. 17 mai 2020 à 15:46, Thibaut VARÈNE <hacks@slashdirt.org> a écrit :
>
> tl;dr: dd will silently truncate the output if reading from special
> files (e.g. sysfs attributes) with a too large bs parameter.
>
> This problem was exposed on some RouterBOARD ipq40xx devices which use a
> caldata payload which is larger than PAGE_SIZE, contrary to all other
> currently supported RouterBOARD devices: the caldata would fail to
> properly load with the current scripts.
>
> Background: dd doesn't seem to correctly handle read() results that
> return less than requested data. sysfs attributes have a kernel exchange
> buffer which is at most PAGE_SIZE big, so only 1 page can be read() at a
> time. In this case, if bs is larger than PAGE_SIZE, dd will silently
> truncate blocks to PAGE_SIZE. With the current scripts using bs=<size>
> count=1, the data is truncated to PAGE_SIZE as soon as the requested
> <size> exceeds this value.
>
> This commit works around this problem by using `cat` in the caldata
> routines that can read from a file (routines that read from mtd devices
> are untouched). cat correctly handles partial read requests. The output
> is then piped to dd with the same parameters as before, to ensure that
> the resulting file remains exactly the same.
>
> This is a simple workaround, the downside is that it uses a pipe and one
> more executable, and therefore has a larger memory footprint and is
> slower. This is deemed acceptable considering these routines are only
> used at boot time.
>
> Tested-by: Robert Marko <robimarko@gmail.com>
> Signed-off-by: Thibaut VARÈNE <hacks@slashdirt.org>
> ---
> v2: leave a comment in scripts
> ---
>  package/base-files/Makefile                       | 2 +-
>  package/base-files/files/lib/functions/caldata.sh | 8 +++++---
>  2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/package/base-files/Makefile b/package/base-files/Makefile
> index d8e7c31878..5fb275533d 100644
> --- a/package/base-files/Makefile
> +++ b/package/base-files/Makefile
> @@ -12,7 +12,7 @@ include $(INCLUDE_DIR)/version.mk
>  include $(INCLUDE_DIR)/feeds.mk
>
>  PKG_NAME:=base-files
> -PKG_RELEASE:=220
> +PKG_RELEASE:=221
>  PKG_FLAGS:=nonshared
>
>  PKG_FILE_DEPENDS:=$(PLATFORM_DIR)/ $(GENERIC_PLATFORM_DIR)/base-files/
> diff --git a/package/base-files/files/lib/functions/caldata.sh b/package/base-files/files/lib/functions/caldata.sh
> index 6862da7164..e22c7d27e6 100644
> --- a/package/base-files/files/lib/functions/caldata.sh
> +++ b/package/base-files/files/lib/functions/caldata.sh
> @@ -64,7 +64,8 @@ caldata_from_file() {
>
>         [ -n "$target" ] || target=/lib/firmware/$FIRMWARE
>
> -       dd if=$source of=$target iflag=skip_bytes bs=$count skip=$offset count=1 2>/dev/null || \
> +       # dd doesn't handle partial reads from special files: use cat
> +       cat $source | dd of=$target iflag=skip_bytes bs=$count skip=$offset count=1 2>/dev/null || \

Not way more elegant, but you could use something like
tail -c+$start $source | head -c$count > $target
with $start == $offset+1 I think

# head -c10 /dev/zero | wc
        0         0        10
# head -c10 /dev/zero | tail -c+3 | wc
        0         0         8

>                 caldata_die "failed to extract calibration data from $source"
>  }
>
> @@ -73,13 +74,14 @@ caldata_sysfsload_from_file() {
>         local offset=$(($2))
>         local count=$(($3))
>
> +       # dd doesn't handle partial reads from special files: use cat
>         # test extract to /dev/null first
> -       dd if=$source of=/dev/null iflag=skip_bytes bs=$count skip=$offset count=1 2>/dev/null || \
> +       cat $source | dd of=/dev/null iflag=skip_bytes bs=$count skip=$offset count=1 2>/dev/null || \
>                 caldata_die "failed to extract calibration data from $source"
>
>         # can't fail now
>         echo 1 > /sys/$DEVPATH/loading
> -       dd if=$source of=/sys/$DEVPATH/data iflag=skip_bytes bs=$count skip=$offset count=1 2>/dev/null
> +       cat $source | dd of=/sys/$DEVPATH/data iflag=skip_bytes bs=$count skip=$offset count=1 2>/dev/null
>         echo 0 > /sys/$DEVPATH/loading
>  }
>
> --
> 2.11.0
>
>
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel@lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel

Hi Étienne,

> Le 17 mai 2020 à 22:24, Etienne Champetier <champetier.etienne@gmail.com> a écrit :
> 
> Hi Thibaut,
> 
> Le dim. 17 mai 2020 à 15:46, Thibaut VARÈNE <hacks@slashdirt.org <mailto:hacks@slashdirt.org>> a écrit :
>> 
>> -       dd if=$source of=$target iflag=skip_bytes bs=$count skip=$offset count=1 2>/dev/null || \
>> +       # dd doesn't handle partial reads from special files: use cat
>> +       cat $source | dd of=$target iflag=skip_bytes bs=$count skip=$offset count=1 2>/dev/null || \
> 
> Not way more elegant, but you could use something like
> tail -c+$start $source | head -c$count > $target
> with $start == $offset+1 I think
> 
> # head -c10 /dev/zero | wc
>        0         0        10
> # head -c10 /dev/zero | tail -c+3 | wc
>        0         0         8

I’m not sure there’s a good reason to do it that way: my change barely touches the current logic: it is trivial to confirm that it doesn’t break anything that was previously working. On the other hand, doing away with dd entirely will need more proofing, and if some extra dd flag is needed in the future it will have to be reinstated.

Your approach also puts more burden on the shell (it uses redirection to output the final file).
All in all I’m afraid it doesn’t look more elegant to me, and it seems less efficient too.

The really clean fix which isn’t available to us as busybox dd doesn’t support the ‘count_bytes’ iflag would be something along those lines:

dd if=$source of=$target iflag=skip_bytes,count_bytes bs=$PAGE_SIZE skip=$offset count=$count

And then everything would work correctly. This is a different approach which I tested successfully with the « real » dd.

Cheers,
Thibaut

Ping?
What’s the stance on this patch?

Should I rebase before resubmitting following package version bump or is this NACKd as is?

This patch (or a solution fixing this issue) is necessary to support mikrotik ipq40xx devices, see for instance PR #3037

Thanks,
Thibaut