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

[Masami Hiramatsu <mhiramat@kernel.org>]

On Mon, 16 Nov 2020 10:29:45 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Mon, Nov 16, 2020 at 8:22 AM Masami Hiramatsu <mhiramat@kernel.org> 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.

Yes, and I also found the error message used return code, but it must use
errno...  

> 
> > +               /* 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...

Oops, OK, fix it.

> 
> >         /* 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..

Agreed. If I hit the partial write, I need to roll it back. Or bootconfig
command can not find the magic word and initrd image may just increase
the size with the garbage data.

Thank you!

-- 
Masami Hiramatsu <mhiramat@kernel.org>