test: Add data relocation test #1244

keiya-nobuta · 2021-12-14T07:27:17Z

This patch adds a test case that the klp-module has a
.klp.rela.<module name>..rodata.<...> section. This pattern can be
an error if the kernel architecture-specific relocation does not fully
support Livepatch.

Currently, implementation of the Arm64-specific relocation for Livepatch
is in progress [1], but it is pointed out that data relocation is insufficient.
I created this test to confirm that data relocation was implemented correctly.

[1] https://lore.kernel.org/linux-arm-kernel/20211103210709.31790-1-surajjs@amazon.com/

The prerequisite for this test is that CONFIG_USB_SERIAL_IR=m and
CONFIG_USB_SERIAL_WWAN=m are set. In the patched-modules,
ir-usb.ko's static local variables refers to usb_wwan.ko's symbols,
so .klp.rela.ir-usb..rodata.<...> section are generated into
test-data-reloc.ko.

Modifications to drivers/usb/serial/bus.c are not directly related to
the purpose of this test, but are necessary for testing with the
data-reloc-LOADED.test.

keiya-nobuta · 2021-12-14T07:28:07Z

The purpose of this test is to check the implementation of the linux kernel rather than the integration of kpatch, so is it inappropriate to add this test here?

sm00th · 2021-12-14T09:12:48Z

The prerequisite for this test is that CONFIG_USB_SERIAL_IR=m and CONFIG_USB_SERIAL_WWAN=m are set.

Ok, these are both set in the test-kernel we automatically run 5.10.11 integration tests on.

The purpose of this test is to check the implementation of the linux kernel rather than the integration of kpatch, so is it inappropriate to add this test here?

You are right, ideally this needs to be a part of livepatch selftests upstream. The problem is - currently kpatch-build is the only tool that generates klp-relocations. There is also a patchset @joe-lawrence is working on that adds klp-relocations creation upstream, but that is still WIP. So I am guessing for the time being we'll have to have this test here.

jpoimboe · 2021-12-15T22:28:51Z

Modifications to drivers/usb/serial/bus.c are not directly related to
the purpose of this test, but are necessary for testing with the
data-reloc-LOADED.test.

-LOADED.test tests are optional, so I wonder if the change to drivers/usb/serial/bus.c is really needed?

For older kernels we had an integration test for testing a variety of klp relocation types: meminfo-string.patch and meminfo-string-LOADED.test. I wonder if that would have found your issue?

When we ported meminfo-string to a unit test, we removed it from the integration tests for newer kernels. Maybe we shouldn't have done that, runtime testing of relocations is important (as you discovered).

keiya-nobuta · 2021-12-16T06:26:58Z

Thanks comments.

-LOADED.test tests are optional, so I wonder if the change to drivers/usb/serial/bus.c is really needed?

I don't really need it, but I'm adding -LOADED.test to load the modules because the issue occurs when relocating references to other modules instead of vmlinux.
kpatch-test framework seems to be OK when all of 1.~3. step the below are true,

-LOADED.test fails
test-klp-module load succeeds
-LOADED.test succeeds

so drivers/usb/serial/bus.c changing is a solution to pass this (To fail at 1. and succeed at 3.).
I wish I could find a smarter way...

For older kernels we had an integration test for testing a variety of klp relocation types: meminfo-string.patch and meminfo-string-LOADED.test. I wonder if that would have found your issue?

I have found the issue when testing gcc-static-local-var-6.patch with CONFIG_IPV6=m. However when CONFIG_IPV6=y it is not occurred. (This was using the Arm64 kernel without [1] I commented above.)
I think this is because vmlinux's symbol relocation is doing at load_module(), but other module's symbol relocation is doing at klp_enable_patch() or after the other module is loaded. At the other module's symbol relocation case, klp-module is already loading completed and text are read-only.

Now the Arm64 kernel with [1], .klp.rela.<module name>..text relocation is okay but .klp.rela.<module name>..rodata relocation will fail. I found .klp.rela.vmlinux..text with meminfo-string.patch and found .klp.rela.ipv6..text with gcc-static-local-var-6.patch, but I don't found .klp.rela.<module name>..rodata case.

jpoimboe · 2021-12-17T03:50:19Z

I don't really need it, but I'm adding -LOADED.test to load the modules because the issue occurs when relocating references to other modules instead of vmlinux. kpatch-test framework seems to be OK when all of 1.~3. step the below are true,
1. -LOADED.test fails

2. test-klp-module load succeeds

3. -LOADED.test succeeds
so drivers/usb/serial/bus.c changing is a solution to pass this (To fail at 1. and succeed at 3.). I wish I could find a smarter way...

Sorry, I still don't really understand. What problem are you seeing in kpatch-test without the -LOADED.test?

keiya-nobuta · 2021-12-17T04:13:34Z

Sorry, I still don't really understand. What problem are you seeing in kpatch-test without the -LOADED.test?

This relocation issue didn't occur without load usb-wwan.ko and usb_ir.ko. So -LOADED.test using to load these modules.

jpoimboe · 2021-12-17T04:51:52Z

Sorry, I still don't really understand. What problem are you seeing in kpatch-test without the -LOADED.test?

This relocation issue didn't occur without load usb-wwan.ko and usb_ir.ko. So -LOADED.test using to load these modules.

Ah, got it.

We also have the concept of a custom test, which is any .test without -LOADED in the name. For example test/integration/linux-5.10.11/multiple.test. That would be another option which might be a better fit. But either way works for me.

keiya-nobuta · 2021-12-17T06:05:35Z

We also have the concept of a custom test, which is any .test without -LOADED in the name. For example test/integration/linux-5.10.11/multiple.test. That would be another option which might be a better fit. But either way works for me.

Thanks, I didn't know the custom test.
All I want to do is load the prerequisite modules, so it seems simpler without -LOADED. I'll check if it works and then update this pull-request.

x86 dmesg displays "Call Trace", but other architectures (for example arm64) may display "Call trace". So change grep with ignore case sensitive. Signed-off-by: Keiya Nobuta <nobuta.keiya@fujitsu.com>

keiya-nobuta · 2021-12-20T03:12:43Z

I updated pathces:

Remove changes to drivers/usb/serial/bus.c
Change test without -LOADED
Export $KPATCH for use in test programs
Change grep Call Trace with ignore case sensitive, because arm64 dmesg shows Call trace.

joe-lawrence · 2022-01-06T21:57:33Z

Hi @keiya-nobuta, apologies for the late request, but could you attach an example arm64 .ko file generated by this test? (You may need to gzip it for github to recognize the file extension.) Thanks!

joe-lawrence · 2022-01-06T23:06:05Z

It's also possible to create similar relocations with:

extern int x;
int * const y = &x;

Relocation section '.rela.rodata' at offset 0xe2a8 contains 5 entries:
    Offset             Info             Type               Symbol's Value  Symbol's Name + Addend
...
0000000000000040  0000004f00000101 R_AARCH64_ABS64        0000000000000000 const_global_small + 0

which I understand why the compiler throws it into .rodata. But the example in this test is weird.. shrink the array to a single element, the relocation moves out of rela.rodata. Same if directly indexing the array with a constant. Ditto if the index variable is non volatile. Etc.

@keiya-nobuta : Does the pattern in this test case pop up a lot in real examples? I'm curious how you found it and perhaps what other common code leads to .rela.rodata.

@ everyone : In the bigger picture, does it make sense to allow late relocations to sections that have been specifically named ".rela.ro/read-only/ro_after_init"? If the patch author could slightly modify the input patch to shift these relocations out of these sections (remains to be seen), would it be worth not allowing such relocations?

keiya-nobuta · 2022-01-07T04:35:15Z

Hi @joe-lawrence,

Hi @keiya-nobuta, apologies for the late request, but could you attach an example arm64 .ko file generated by this test? (You may need to gzip it for github to recognize the file extension.) Thanks!

test-data-reloc.ko.gz
(this was generated using #1236 )

@keiya-nobuta : Does the pattern in this test case pop up a lot in real examples? I'm curious how you found it and perhaps what other common code leads to .rela.rodata.

I didn't find it from a real example, I tried and found out in what cases data relocation would be generated.
When I put variables inside a function, the address was often placed immediate-field of the instruction, probably for optimization. So I looked for a way to suppress compiler optimization.

joe-lawrence · 2022-02-08T19:57:04Z

test/integration/kpatch-test


+# export envs for test progs
+export KPATCH
+


I think this should supercede the assignment here:

kpatch/test/integration/common/multiple.template

Line 3 in aaaebaf

KPATCH="sudo $ROOTDIR/kpatch/kpatch"

so the multiple.test cases would honor the flags added by 78f2c063ae91f to allow the user control over which kpatch tools is used.

In that case, the assignment in multiple.template can be removed and I think it should percolate down to that test just as it did for the new one in this PR.

I removed KPATCH assignment in multiple.template.

joe-lawrence · 2022-02-08T20:21:32Z

test/integration/linux-5.10.11/data-reloc.test

+sudo modprobe usb_wwan
+sudo modprobe ir-usb
+
+$KPATCH load test-data-reloc.ko


Our integration test scripts could stand to be more robust, but a similar one:
https://github.com/dynup/kpatch/blob/master/test/integration/rhel-8.4/module-LOADED.test sets errexit as well as adds a sleep 5 to give time for the incoming modules to settle. This is hardly bulletproof, but easy to avoid (probably) most false negatives from errors/races:

set -o errexit ... sudo modprobe nfsd sleep 5

I added sleep 5 after modprobes

joe-lawrence · 2022-02-10T21:29:30Z

test/integration/linux-5.10.11/data-reloc.patch

+	printk("kpatch: usb_wwan_open=%p\n", funcs[i]);
+
+ 	irda_desc = irda_usb_find_class_desc(serial, 0);
+ 	if (!irda_desc) {


I can't seem to build this patch, I get this error,

ERROR: ir-usb.o: 1 function(s) can not be patched ir-usb.o: WARNING: unable to correlate static local variable funcs.20 used by ir_startup, assuming variable is new ir-usb.o: function irda_usb_dump_class_desc.isra.0 doesn't have patchable function entry, unable to patch /root/github-kpatch/kpatch-build/create-diff-object: unreconcilable difference

Here are my versions, probably something is compiling slightly different for me:
gcc: gcc version 11.2.1 20220127 (Red Hat 11.2.1-9) (GCC)
ld: GNU ld version 2.35.2-17.el9
kernel tree: amazon-arm-livepatch-v5.10
kpatch-build: surajjs95-arm64

OK, I'm not ready for gcc11 environment yet. I will check it as soon as it is ready.

I could not get this error with gcc11 of ubuntu21.04.
I can prepare rhel-9 at my workplace, so I will try it this week.

my versions:
gcc (Ubuntu 11.2.0-7ubuntu2) 11.2.0
ld (GNU Binutils for Ubuntu) 2.37
kernel tree: https://github.com/sm00th/linux/tree/kpatch/aarch64
kpatch-build: https://github.com/joe-lawrence/kpatch/tree/arm64-rebase

Signed-off-by: Keiya Nobuta <nobuta.keiya@fujitsu.com>

This patch adds a test case that the klp-module has a `.klp.rela.<module name>..rodata.<...>` section. This pattern can be an error if architecture-specific relocation in the kernel does not fully support Livepatch. The prerequisite for this test is that CONFIG_USB_SERIAL_IR=m and CONFIG_USB_SERIAL_WWAN=m are set. In the patched-modules, ir-usb.ko's static local variables refers to usb_wwan.ko's symbols, so `.klp.rela.ir-usb..rodata.<...>` section are generated into test-data-reloc.ko. Signed-off-by: Keiya Nobuta <nobuta.keiya@fujitsu.com>

github-actions · 2023-08-12T10:13:39Z

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

github-actions · 2023-08-19T10:13:40Z

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

sm00th approved these changes Dec 14, 2021

View reviewed changes

test: Change grep "Call Trace" with -i

7b04a3f

x86 dmesg displays "Call Trace", but other architectures (for example arm64) may display "Call trace". So change grep with ignore case sensitive. Signed-off-by: Keiya Nobuta <nobuta.keiya@fujitsu.com>

keiya-nobuta force-pushed the test_data_relocation branch from 01cc87e to 07fbab1 Compare December 20, 2021 03:00

jpoimboe approved these changes Dec 20, 2021

View reviewed changes

joe-lawrence reviewed Feb 10, 2022

View reviewed changes

joe-lawrence mentioned this pull request Feb 15, 2022

Add Arm64 Support #1236

Closed

test: Export $KPATCH for test programs

7d76167

Signed-off-by: Keiya Nobuta <nobuta.keiya@fujitsu.com>

keiya-nobuta force-pushed the test_data_relocation branch from 07fbab1 to 77d0d68 Compare February 20, 2022 07:42

keiya-nobuta force-pushed the test_data_relocation branch from 77d0d68 to 1558157 Compare February 20, 2022 07:47

github-actions bot added the stale label Aug 12, 2023

github-actions bot closed this Aug 19, 2023

test: Add data relocation test #1244

test: Add data relocation test #1244

Uh oh!

Conversation

keiya-nobuta commented Dec 14, 2021

Uh oh!

keiya-nobuta commented Dec 14, 2021

Uh oh!

sm00th commented Dec 14, 2021

Uh oh!

jpoimboe commented Dec 15, 2021

Uh oh!

keiya-nobuta commented Dec 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jpoimboe commented Dec 17, 2021

Uh oh!

keiya-nobuta commented Dec 17, 2021

Uh oh!

jpoimboe commented Dec 17, 2021

Uh oh!

keiya-nobuta commented Dec 17, 2021

Uh oh!

keiya-nobuta commented Dec 20, 2021

Uh oh!

joe-lawrence commented Jan 6, 2022

Uh oh!

joe-lawrence commented Jan 6, 2022

Uh oh!

keiya-nobuta commented Jan 7, 2022

Uh oh!

joe-lawrence Feb 8, 2022

Choose a reason for hiding this comment

Uh oh!

keiya-nobuta Feb 20, 2022

Choose a reason for hiding this comment

Uh oh!

joe-lawrence Feb 8, 2022

Choose a reason for hiding this comment

Uh oh!

keiya-nobuta Feb 20, 2022

Choose a reason for hiding this comment

Uh oh!

joe-lawrence Feb 10, 2022

Choose a reason for hiding this comment

Uh oh!

keiya-nobuta Feb 14, 2022

Choose a reason for hiding this comment

Uh oh!

keiya-nobuta Feb 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Aug 12, 2023

Uh oh!

github-actions bot commented Aug 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

keiya-nobuta commented Dec 16, 2021 •

edited

Loading

keiya-nobuta Feb 20, 2022 •

edited

Loading