kpatch-build, x86: do not use the patched functions as callbacks directly

[euspectre]

A kernel crash happened in __do_softirq() in very rare cases when the
binary patch created from mainline commit be82485fbcbb
("netlink: fix an use-after-free issue for nlk groups") was unloaded.

Kernel: 3.10.0-514.16.1.vz7.30.10, Virtuozzo 7, x86_64.

Investigation has shown that the kernel tried to execute an RCU callback,
deferred_put_nlk_sk(), defined in the patch module after the module had
been unloaded.

The callback was set by the patched variant of netlink_release(), that is
why the address of the patched deferred_put_nlk_sk() rather than of the
original one was used.

[euspectre]

Not sure if livepatch-patch-hook.c needs a similar fix. It might but I am not very familiar with that code yet.

[jpoimboe]

Thanks, this is an interesting issue. However I'm not convinced that this RCU synchronize really belongs in kpatch. Isn't the issue specific to the patch itself? I think a more appropriate fix would be to add a patch unload hook which calls rcu_barrier().

I think this is an example of a broader issue, which should be documented in the patch author guide: You have to be very careful if there are any function pointers to a patched function.

In fact, I wonder if we could detect such an issue (a pointer to a patched function exists) and warn about it in kpatch-build.

[euspectre]

Well, if KPatch did not optimize the references to the patched functions when loading the patch module, only the addresses of the original functions were used and this problem would not happen.

Because of how KPatch core loads the patch module, it is perfectly OK to have the pointers to the patched functions. Such optimization of function references seems valuable.

I think, even with unload hooks available, having rcu_barrier() as a "safety net" in the exit function is worth it. It makes sure no RCU callbacks, not only the one from this particular patch, could harm the system when the patch module unloads.

Timer callbacks, workqueue functions and some other callbacks that might be set from the patched code could still be problematic - but unlike RCU, these should be waited for separately. There is no way to wait for all such pending callbacks at once, so one has to use hooks for these anyway.

[jpoimboe]

Well, if KPatch did not optimize the references to the patched functions when loading the patch module, only the addresses of the original functions were used and this problem would not happen.

Hm. So maybe a better fix would be to ensure that function pointers always refer to the original function instead of the patched function?

[euspectre]

Perhaps, it would be more reliable, yes.

However, I cannot see if it is doable for the callbacks only rather than for all functions. In an arch-independent way, I mean.

Perhaps, some performance measurements could show if using the pointers to the patched functions directly is really worth it. It looks so in the code, esp. for the "old" kpatch which does a table lookup in the common Ftrace filter each time the function is called. Livepatch does things differently, IIRC.

[jpoimboe]

However, I cannot see if it is doable for the callbacks only rather than for all functions. In an arch-independent way, I mean.

Here's how I think it can be detected on x86:

    4154:       48 c7 c6 00 00 00 00    mov    $0x0,%rsi
                        4157: R_X86_64_32S      .text+0xb50
    415b:       e8 00 00 00 00          callq  4160 <netlink_release+0x3a0>
                        415c: R_X86_64_PC32     call_rcu-0x4

We can look for a R_X86_64_32S relocation which references a symbol to a replacement function. In such a case we can convert it to a klp rela for the original function.

Perhaps, some performance measurements could show if using the pointers to the patched functions directly is really worth it. It looks so in the code, esp. for the "old" kpatch which does a table lookup in the common Ftrace filter each time the function is called. Livepatch does things differently, IIRC.

Performance is important, but safety is more important. I would much rather be safe and take a small performance hit. And anyway, kpatch.ko does a hash lookup which should be pretty fast.

[euspectre]

So, we can detect the assignment of callbacks by checking the relocation type and without decoding instructions and such, at least, on x86. This way we would not have to disable the pointer optimization everywhere but rather, for the callbacks only. Thanks for the explanation!

I'll experiment with that when I have time.

[euspectre]

BTW, I have reproduced the issue using workqueue functions instead of RCU callbacks. I created a patch for create_user_ns() and free_user_ns() in a VZ7 kernel (based on version 3.10.0-693.1.1 from RHEL). free_user_ns() is a work function set by create_user_ns().

Loading/unloading the patch module in a loop in parallel with 'unshare --map-root-user --user sh -c whoami' triggered the crash in a couple of minutes.

So, yes, a more general solution rather than a fix for just RCU callbacks is needed.

I'll try to prepare a patch based on your suggestion.

[jpoimboe]

Ok, thanks for looking at it! I haven't looked at what's needed, I'm guessing it's a change to kpatch_create_intermediate_sections(). Let me know if you need any help.

[euspectre]

Here it is, please take a look.

The patches created by kpatch-build with create-diff-object changed this way have passed my tests with RCU and workqueue so far.

[euspectre]

Comments?

[jpoimboe]

Sorry, I've been swamped with other work and haven't had a chance to look at it. I'll try to review it soon...

[disaster123]

Just a note. This one gives me a segfault in create-diff-obj[2592]: segfault at 7f3fa3faea81 ip 00007f3fa364edcc sp 00007ffdf4c064a0 error 4 in libc-2.19.so[7f3fa3604000+1a1000]

[euspectre]

@disaster123: Strange. Which source patch kpatch-build was processing when the segfault happened?

And - for which kernel?

[disaster123]

Patch is this one:
https://pastebin.com/zNUK5C2U

Kernel is an "older" OpenSuSE 42.3 Kernel - source / base:
https://github.com/openSUSE/kernel-source/tree/9c032968bf05aaff3eb55048440c07249e99db33

[euspectre]

Thanks for the info. I'll try to reproduce the issue.

[disaster123]

Were you able to reproduce?

[euspectre]

Unfortunately, I have my hands full right now. Perhaps, I'll find time for that later this week.

[euspectre]

I have reproduced the problem, investigating...

[euspectre]

Nested function cmp() in bch_cache_show(). No surprise. They already caused problems in the past.

The problem happens when create-diff-object processes drivers/md/bcache/sysfs.o.

(gdb) bt
<...>
#5  0x0000000000407aef in error (__format=0x410468 "ERROR: %s: %s: %d: lookup_local_symbol %s:%s needed for %s", __errnum=0, __status=1)
    at /usr/include/bits/error.h:42
#6  kpatch_create_intermediate_sections (kelf=0x623cc0, table=0x75f860, 
    hint=0x7ffff7fb4541 , objname=0x7fffffffe752 "bcache", 
    pmod_name=0x7fffffffe774 "test_patch") at create-diff-object.c:2426
#7  0x00000000004093f6 in main (argc=7, argv=0x7fffffffe318) at create-diff-object.c:2943

create-diff-object.c:

        if (rela->sym->bind == STB_LOCAL) {
	/* An unchanged local symbol */
	ret = lookup_local_symbol(table,
		rela->sym->name, &result);
	if (ret)
		ERROR("lookup_local_symbol %s:%s needed for %s",
		hint, rela->sym->name, sec->base->name); // <<< Here

create-diff-object was trying to process the reference to the function cmp.36204 in bch_cache_show(). From the source code of bch_cache_show() in drivers/md/bcache/sysfs.c:

        if (attr == &sysfs_priority_stats) {
		int cmp(const void *l, const void *r)
		{	return *((uint16_t *) r) - *((uint16_t *) l); }
	<...>
	sort(p, n, sizeof(uint16_t), cmp, NULL);

Technically, cmp() is a callback but I'd rather skip references to such nested functions in kpatch_create_intermediate_sections() somehow. I doubt they are used as asynchronous callbacks anywhere in the kernel. Therefore it should be OK to process the references to such callbacks the same way as before, without dynrelas.

The question is how to detect such functions in create-diff-object.

[euspectre]

Assuming GCC creates the names for the nested functions using the same rules as for static locals (xxxx.nnnn), we could probably check if the name of the referred-to functions contains a dot.

EDIT: Might not work because optimized copies of the functions may also contain dots in the name. Need another way to detect nested functions, it seems.

[euspectre]

By the way, the actual crash happens because the source file name ("sysfs.c", referred to by hint variable) has been freed by kpatch_elf_free(kelf_base) by that time.

This is a separate issue. The original issue with the nested function only helped reveal it.
Moving the call kpatch_elf_free(kelf_base) to the similar calls at the end of main() should fix it.

[euspectre]

I have updated the fixes, please review.

It seems, there is no way to recognize nested functions in the binary code except by their names. On the other hand, I checked a few kernels from different distros and all callbacks with '.' in their names were nested functions. So, my patch uses this to distinguish nested functions and the ordinary ones.

As for the crashes due to 'hint' pointing to nowhere, I think it is better to copy the relevant string rather than to defer freeing of kelf_base. This is what the second patch does.

[joe-lawrence]

This is really minor, but if we move these lines up into the search for STT_FILE, there would only be a single assignment to hint, like hint = strdup(sym->name);

[euspectre]

However, we should still wait until they're both ready, and merge them at the same time.

@jpoimboe That is fine with me. We may otherwise forget the fix for PowerPC anyway. There are quite a few other bugs to fix and tasks to do, as usual. Hopefully, there will be no new meltdown/spectre-like bugs in the near future ;-)

I have updated the patch, so a fix for PowerPC could be done on top of it. I cannot be of much help there, unfortunately, becase I know only a little about that architecture. That is why it is better to do that in a separate patch/PR.

[kamalesh-babulal]

For powerpc -mcmodel=large, the formula to detect them should still be relatively simple:

Thanks @jpoimboe . If the PowerPC changes are be tracked in another PR. I can open one, once we have a patch. I will build upon this approach, I have an idea on how to implement other bits on PowerPC. Need to implement the idea and see what more is needed. Thank you @euspectre.

[jpoimboe]

Sounds good @kamalesh-babulal , thanks!

[euspectre]

@kamalesh-babulal

If the PowerPC changes are be tracked in another PR. I can open one, once we have a patch.

Thanks! Looking forward to it.

[joe-lawrence]

If I read the latest commentary correctly, we're going to leave this PR open until all supported arches have equivalent behavior?

If so, can we spin "kpatch-build: 'hint' is not needed in kpatch_create_*_sections()" as an independent PR ... I just hit this bug doing while working on s390x, so it would be nice to get out of the way :)

[euspectre]

If so, can we spin "kpatch-build: 'hint' is not needed in kpatch_create_*_sections()" as an independent > PR ... I just hit this bug doing while working on s390x, so it would be nice to get out of the way :)

@joe-lawrence Makes sense. I have moved that hint-related fix into #788 and updated this PR.

[jpoimboe]

This is superseded by #789 which has both x86 and ppc versions of the change.