function pointers to global functions don't get converted to dynrelas

[jpoimboe]

As discovered in #834.

[jpoimboe]

@sm00th After looking at this bug, I'm starting to think that our unit test hacks aren't sufficient. Instead of having fake vmlinux and Module.symvers files, I think we will need the real things, to properly test the dynrela code. Can you update the unit test framework accordingly?

I guess we'll need to target our unit tests against 4.16 (or similar kernels) for now. Maybe the tests could be moved to a 4.16 subdir of the x86_64 dir, or something similar, so we can eventually add new tests for later kernels. The 4.16 subdir can have a stripped vmlinux and Module.symvers in it, along with a file containing the values of PARA_STRUCT_SIZE and friends.

git-lfs space may become an issue, so it would also be good to add a check to ensure vmlinux is stripped.

Also I think we can stay on 4.16 for a while. Even test binaries built against later kernels will probably work with the old vmlinux for quite a while.

[sm00th]

I assume config used to build vmlinux/Module.symvers does matter? Should it be f27 config + olddefconfig or something else?

[jpoimboe]

Yeah, I think f27 config + olddefconfig would be fine.

[jpoimboe]

BTW, one possible issue is that some of the tests might be against modules instead of vmlinux. If so, I'm not sure the best way to deal with those.

[sm00th]

BTW, one possible issue is that some of the tests might be against modules instead of vmlinux. If so, I'm not sure the best way to deal with those.

I think it won't be much of a problem to come up with a solution for modules, but I think we need to draw a line somewhere.

Stripped vmlinux I got is 53Mb + 5Mb of objectfiles and we get ~17 downloads a month. And thats just x86_64. Modules, while much smaller, still may add up to a lot. Another thing: I don't think there is a way to purge old binaries from lfs, so updating vmlinux even during pullrequest review would mean using twice the storage and once we reach 1Gb there will be no way of cleaning old stuff up.

I propose that the tests that would need to be built against modules (or even against real vmlinux if you don't expect there to be many of those) be moved to integration tests rather than unit tests.

[jpoimboe]

Hm, I forgot about the download quota. Maybe a 53M vmlinux file isn't such a good idea then. Especially looking forward where the unit tests support multiple kernels and arches.

I think all we really need is the symbol table, so maybe we need to change create-diff-object somehow so that it can take a symbol table as input. Will think about it a little more.

[sm00th]

Ok, I have a working prototype if we'll need it. It also makes me wonder whether travis-runs count towards bandwidth limit. There doesn't seem to be any lfs downloads in travis output. Can you check how much bandwidth we used up so far? https://help.github.com/articles/viewing-storage-and-bandwidth-usage-for-an-organization/

[jpoimboe]

Billing cycle – 29 days left, billed monthly Bandwidth – Using 0.21 of 1 GB/month Storage – Using 0.01 of 1 GB

I bet Travis is using a lot of that bandwidth.

[jpoimboe]

We may need to abandon git-lfs and go back to the submodule idea...

[sm00th]

Yes, the ratio looks like it is mostly travis. We may run out of bandwith quite quickly if we add more tests. I'll look into moving it back to a submodule, there are still some limitations: repo shouldn't be more than 1Gb and a single file can't be >100mb(>50mb triggers a warning but is still commitable) which should be ok for quite some time , especially if we can make create-diff-object work without vmlinux/original module.

[jpoimboe]

Ok, sounds good.

[sm00th]

I've been playing around with the "no kobject for create-diff-object" idea. The symtab for vmlinux file is under 10mb which is a good improvement, however the implementation I have so far is rather ugly. You can take a look at https://github.com/sm00th/kpatch/commit/d61a44cc565dbb68d83c65b9fc73e6edbf674fcb The magic string to work around scanf, the wall of hardcoded ifs to convert strings to proper binds/types and the fact that all of this is just another step in build that does not improve anything apart from unit-testing makes me wonder if it is really worth it.

Any ideas on how to make this less awful are welcome.

[jpoimboe]

In my opinion, it's completely worth it if it makes testing better.

I actually did something very similar a few weeks ago, just got distracted in the meantime. See: https://github.com/jpoimboe/kpatch/commit/8a67f2e0bdc271a8e54cd851680d010dfc894930.

I'm still quite busy with other things, so feel free to combine the best parts of both patches to make a super-patch :-)

[joe-lawrence]

Would a scenario where both a callback and the function assigning a pointer to that callback are kpatched hit this problem? Should we be using dynrela to the original callback function, and then let normal livepatch ftrace hooks bring the replacement callback implementation into and out of service safely?

[github-actions[bot]]

This issue has been open for 30 days with no activity and no assignee. It will be closed in 7 days unless a comment is added.

[github-actions[bot]]

This issue was closed because it was inactive for 7 days after being marked stale.