Messages in this thread | | | Date | Tue, 17 Nov 2020 09:09:36 +0900 | From | Masami Hiramatsu <> | Subject | Re: [PATCH v2 1/2] tools/bootconfig: Align the bootconfig applied initrd image size to 4 |
| |
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>
| |