Re: [PATCH v2 1/2] tools/bootconfig: Align the bootconfig applied initrd image size to 4

[Linus Torvalds]

On Mon, Nov 16, 2020 at 8:22 AM Masami Hiramatsu <mhiramat@xxxxxxxxxx> wrote:
>
>         /* TODO: Ensure the @path is initramfs/initrd image */
> -       ret = write(fd, data, size + 8);
> +       ret = write(fd, data, size);
>         if (ret < 0) {
>                 pr_err("Failed to apply a boot config: %d\n", ret);

Side note: that's not the right error check for a write() call.

The problem predates your patch, so it's independent, I just noticed
as I looked at it.

The right error check is

        if (ret != size) {

because partial writes can happen even with regular files (yeah, it's
not POSIX, but NFS is a thing).

And the filesystem filling up is a possibility too, of course.

> +               /* Write padding null characters */
> +               ret = write(fd, padbuf, pad);
> +               if (ret < 0) {
...
> +       ret = write(fd, &size, sizeof(u32));
> +       ret = write(fd, &csum, sizeof(u32));

Those two guys don't check at all...

>         /* Write a magic word of the bootconfig */
>         ret = write(fd, BOOTCONFIG_MAGIC, BOOTCONFIG_MAGIC_LEN);
>         if (ret < 0) {

.. and same comment.

Anyway, I'm not sure this is worth worrying about - even on NFS the
partial IO thing is basically non-existent.

But the filesystem full (or file size limit, or whatever) case is real
even on POSIX filesystems, and testing for write errors by checking
for a negative return is simply incorrect.

Partial writes are an issue (although the _next_ write will then
return an error, so you have to be unlucky to hit that partial write
on the very last write() that you do and that you test the return
value incorrectly).

The fact that it works in all normal circumstances just means that
these kinds of bugs tend to stay around, until somebody is really
unlocky and then you have inexplicably corrupt files..

                 Linus