re-enable forced patch modules

[flaming-toast]

Attempts to address #343, after taking the comments there into consideration.

Quick rundown:

• in kpatch-build, a .kpatch.checksum section is added to the final output.o object using objcopy.
• in kpatch-patch-hook, the contents of .kpatch.checksum are exported to sysfs. sysfs_create_group (previously sysfs_create_file) moved to the end of patch_init to avoid the race condition where kpatch_register is called before kpmod.objects is fully initialized.
• in the kpatch core module, move the kpmod->enabled check in kpatch_register to after obtaining the mutex and try_module_get. This was done so that If there are concurrent calls to kpatch_register, only one would succeed, and the others would see that kpmod->enabled is already set true. Same for kpatch_unregister.
• in the kpatch script, load_module now checks if the given module is already loaded (or if another module with the same name is). If so, verify that the checksums read from the .ko file and the corresponding module in sysfs match.

Other comments:

• The gotos had to be changed up a bit after switching the initialization order in patch_init. Help verify that it's still sane :-) Also, not sure if I be calling sysfs_remove_group if sysfs_create_group fails. Doesn't make sense since if it fails the group was never created...Am I missing any other error conditions?
• Note that this won't work with old forced modules since they won't have a checksum section.
• Tested with a forced version of the meminfo patch and loading/unloading/re-enabling works. I might be missing some corner cases, let me know if you catch any.

This is the initial version of this patch so I suspect there may be things I am missing, or perhaps some parts can be done better. :-) As always, feedback is welcome.

[jpoimboe]

@flaming-toast This looks really good. Some minor comments coming...

[jpoimboe]

s/strlen(__kpatch_checksum) + 1/PAGE_SIZE/ here, to avoid overrunning buf.

[jpoimboe]

end comments

[flaming-toast]

Fixed review comments and rebased.

• switching to PAGE_SIZE w/ snprintf gave me an extra newline for some reason, so I removed the newline in the format string.
• sysfs_create_group jumps to err_sysfs and calls kpatch_unregister if it fails

[jpoimboe]

switching to PAGE_SIZE w/ snprintf gave me an extra newline for some reason, so I removed the newline in the format string.

Ah, that's probably because the checksum string is stored in the section with a newline.

[(33164c4...)] ~/git/kpatch $ eu-readelf -S kpatch-meminfo-string.ko |grep kpatch.checksum
[44] .kpatch.checksum     PROGBITS     0000000000000000 000015c8 00000021  0 A      0   0  1
[(33164c4...)] ~/git/kpatch $ hexdump -C -s 0x15c8 -n 0x21 kpatch-meminfo-string.ko
000015c8  37 63 64 33 30 35 35 30  31 66 31 37 31 61 33 63  |7cd305501f171a3c|
000015d8  36 33 62 61 35 33 31 64  37 35 31 63 62 33 65 64  |63ba531d751cb3ed|
000015e8  0a                                                |.|
000015e9

I think it would be better to replace the newline with a trailing NULL.

[flaming-toast]

@jpoimboe Oh that explains it. :-)
In that case, I think the format string "%s" should be fine, since the kernel's snprintf/vsnprintf follow C99 and should stick a null byte at the end of the buffer or string (http://lxr.free-electrons.com/source/lib/vsprintf.c#L1787)

[jpoimboe]

Ok, I think that works because there just happens to be a NULL byte after the 0xa newline byte in the object file. But I'm not so sure we can rely on that NULL byte being there, since it's technically not part of the section.

Also I think it's kind of weird and unexpected to store the newline. So I think the newline should be replaced with a trailing NULL.

Maybe something like:

md5sum ../patch/output.o | awk '{printf "%s", $1}' > checksum.tmp || die
printf '\0' >> checksump.tmp || die

[flaming-toast]

I think the following should work (removes newline, adds NULL byte at the end):

$ md5sum ../patch/output.o | awk '{printf "%s\0", $1}' > checksum.tmp || die

$ hexdump -C checksum.tmp
00000000  37 66 65 37 61 31 38 36  63 31 32 36 66 37 33 38  |7fe7a186c126f738|
00000010  34 34 37 36 63 30 32 61  65 66 30 39 34 30 64 30  |4476c02aef0940d0|
00000020  00                                                |.|
00000021

So I added that in, as well as the newline back in the snprintf format string; pushed and rebased.

[jpoimboe]

Thanks @flaming-toast!

[flaming-toast]

@jpoimboe I am just realizing this, but I'll need to fix up testmod/doit.sh to include the new checksum section as well (which I can do in a separate PR). In addition, for kpatch unload, I think it probably makes sense to verify the checksum there as well...(I overlooked this, apologies! should be quick to add that change)

[jpoimboe]

I am just realizing this, but I'll need to fix up testmod/doit.sh to include the new checksum section as well (which I can do in a separate PR).

I'm not sure what's needed here?

In addition, for kpatch unload, I think it probably makes sense to verify the checksum there as well

Why would we verify the checksum on unload?

Feel free to add more commits to this PR if needed.

[flaming-toast]

Why would we verify the checksum on unload?

I was not sure if it would be correct behavior if for whatever reason, a user supplies a module that happens to have the same name as another module already loaded, but differ in content. The unload would still succeed, even though the user supplied a fundamentally different module as an argument. But perhaps I am thinking too much here. :-)

As for the doit.sh test script, it now fails while building the test module complaining that it cannot find a .kpatch.checksum section that the linker script is referencing, because of course, it does not have one. Should be quick to fix though.

[jpoimboe]

I was not sure if it would be correct behavior if for whatever reason, a user supplies a module that happens to have the same name as another module already loaded, but differ in content. The unload would still succeed, even though the user supplied a fundamentally different module as an argument. But perhaps I am thinking too much here. :-)

Ah, I see. I'm thinking we can not worry about that case :-)

As for the doit.sh test script, it now fails while building the test module complaining that it cannot find a .kpatch.checksum section that the linker script is referencing, because of course, it does not have one. Should be quick to fix though.

Ah, cool. Add the commit to this PR and I'll put my thumb back up :-)

[flaming-toast]

Cool, fixed up doit.sh, should all work now!

[jpoimboe]

👍

[sjenning]

Thanks @flaming-toast , that is my only comment

[jpoimboe]

End of my comments as well, thanks for your patience @flaming-toast

[flaming-toast]

Absolutely no problem, thanks for all the comments.

[jpoimboe]

I think you missed my last comment here? Basically no need to check kpmod.enabled here or at line 75 because it's racy to check it without the mutex, and it's already being checked in kpatch_register/unregister.

[flaming-toast]

Ah yeah, I'll go ahead and remove those lines.

[flaming-toast]

Alright, I think I've addressed all comments so far. Let me know if there are any more issues.

[jpoimboe]

👍