On Mon, 16 Nov 2020 10:29:45 -0800
Linus Torvalds <torva...@linux-foundation.org> wrote:
> On Mon, Nov 16, 2020 at 8:22 AM Masami Hiramatsu <mhira...@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 <mhira...@kernel.org>
