[RFC] kmod/patch: merge .text.* and .rodata.* sections for new and patched functions

[euspectre]

This is a request for comments, because the relevant code is quite complex and I may be missing something important.
Reviews and suggestions are welcome.

When a kernel module is loaded, sysfs attributes are created for its ELF
sections (visible as /sys/module/<module_name>/sections/*). and contain
the start addresses of the ELF sections. A single memory chunk is allocated
for all these, see add_sect_attrs(), kernel/module.c:

        size[0] = ALIGN(sizeof(*sect_attrs)
                        + nloaded * sizeof(sect_attrs->attrs[0]),
                        sizeof(sect_attrs->grp.attrs[0]));
        size[1] = (nloaded + 1) * sizeof(sect_attrs->grp.attrs[0]);
        sect_attrs = kzalloc(size[0] + size[1], GFP_KERNEL);

'nloaded' is the number of loaded ELF section in the module.

As of the kernel 4.18.0-193.6.3 from RHEL 8, x86_64, the total allocation
size is (80 * nloaded + 56).

Cumulative livepatches can easily get hundreds of new and changed
functions. Each such function will be placed into a separate .text.*,
.text.unlikely.* or a similar section, R/O data for each function will
also be stored in a separate section, same for .klp.rela.* sections.

Testing had shown that a patch that added 120+ new functions and touched
a handful of existing ones got 450+ loaded sections, so more than 450 *
80 == 36000 bytes of kmalloc'ed memory was requested. This is a 4th
order allocation, which might fail (and fail loading of the patch) or
become slow when the memory is fragmented.

The thing is, the addresses of these ELF sections of a livepatch module
are unlikely to be needed once the module has been loaded, so that kernel
memory is wasted.

If I understand it right, keeping functions and data in separate
sections is no longer needed after the patch module has been built. The
kernel code that handles loading of livepatch modules does not require
that. It requires that .klp.rela* and .klp.sym* sections have proper
names but that is preserved here.

create-klp-module and create-kpatch-module seem to be fine with that as
well.

The linker script for the patch module has been updated to merge:

• all .text.unlikely.* sections into .text.unlikely.livepatch;
• all other .text.* sections into .text.livepatch;
• .rodata.__func__.*, .rodata.*.str1.1, .rodata.*.str1.8 into respective
  sections.

As for .klp.rela sections, create-klp-module already does the right
thing and generates these as follows:

• .klp.rela.vmlinux..text.livepatch
• .klp.rela.vmlinux..text.unlikely.livepatch

This way, the number of loaded sections in the test patch mentioned above
shrinks from 454 to 29 and the amount of requested memory -
from 36376 to 2376 bytes.

To check the amount of requested memory, one can use 'mm_page_alloc'
tracepoint:

(Assuming debugfs is mounted to /sys/kernel/debug.)

  # echo nop > /sys/kernel/debug/tracing/current_tracer
  # echo 'order > 3' > /sys/kernel/debug/tracing/events/kmem/mm_page_alloc/filter
  # echo 1 > /sys/kernel/debug/tracing/events/kmem/mm_page_alloc/enable
  # echo 1 > /sys/kernel/debug/tracing/tracing_on

  <... load the patch module with lots of sections ... >
  # kpatch load test-patch.ko

  <shortened output>
  # cat /sys/kernel/debug/tracing/trace
  TASK-PID   CPU#    ||||    TIMESTAMP  FUNCTION
    |   |       |    ||||       |         |
  <...>-189468 [001] .... 1110159.836398: mm_page_alloc: <...> order=4 <...>
  <...>-189468 [001] .... 1110159.836653: mm_page_alloc: <...> order=4 <...>

  # echo 0 > /sys/kernel/debug/tracing/tracing_on

In this example, 4th order allocations have been detected when loading
the patch.

[euspectre]

Here is the patch I mentioned:
many_sections.patch.txt

I used kernel 4.18.0-193.6.3 from RHEL 8 for testing.

[joe-lawrence]

Hi @euspectre , thanks for posting the PR, I vaguely remember discussing this on the livepatching mailing list some time ago. Livepatching hundreds of functions is definitely impressive! Just curious, how many kpatch modules do you typically load on a given system?

I think these linker script changes to consolidate the .text and .rodata sections should be safe. I keep wondering if it might break some special section construct, but I can't come up with a failing case. I'm guessing you've tested this against x86_64, how about ppc64le?

BTW, have you seen the upstream Function Granular KASLR series? That uses -ffunction-sections to build modules and then randomize the loading of separate module sections. I wonder if Kristen is concerned about similar high-order sysfs allocations since presumable all modules will have many sections when CONFIG_MODULE_FG_KASLR=y. I only mention it in case there may be an opportunity to modify the upstream module loader and these allocations.

[euspectre]

I vaguely remember discussing this on the livepatching mailing list some time ago.

Yes, I wrote about that a while ago, was asked to submit patches and then completely forgot about it ;-) My bad.

how many kpatch modules do you typically load on a given system?

We mostly use cumulative patches, so, usually, no more than 2 kpatch modules are loaded at the same time:

• the official cumulative patch;
• an additional, temporary, non-cumulative patch for debugging or enhancements if the customers request that.

Our kernels are supported with livepatches for about 1.5 years. The latest cumulative patch for the oldest supported kernel touches about 130 kernel functions and has 200+ loaded ELF sections, so these large allocations can be a problem.

I keep wondering if it might break some special section construct

Me too. This one of the reasons, I'd like to collect comments on the matter.

I'm guessing you've tested this against x86_64, how about ppc64le?

Right, I have only tested it against x86_64. If someone could check if it works OK for ppc64le as well, it would be great, because I have no such hardware available at the moment.

have you seen the upstream Function Granular KASLR series?

I saw the series but did not dig into it. Interesting, so we might have a similar issue with all modules with FGKASLR, not only livepatch.

Need to take a closer look.

At the first glance, kmalloc'ed, physically contiguous memory areas for these sysfs attributes is an overkill. Merging sections is not an option for FGKASLR, so, just in case someone does need /sys/module/<...>/sections/*, I think, I'll suggest using vmalloc there instead.

[joe-lawrence]

I'm guessing you've tested this against x86_64, how about ppc64le?

Right, I have only tested it against x86_64. If someone could check if it works OK for ppc64le as well, it would be great, because I have no such hardware available at the moment.

I can confirm that the changes can build and load many-sections.patch on ppc64le if I add __attribute__((optimize("-fno-optimize-sibling-calls"))) before the definition of pde_put().

[joe-lawrence]

FWIW, this pattern seems compatible with scripts/module-common.lds and arch/powerpc/kernel/module.lds (no x86_64 counterpart).

I did notice that arch/x86/kernel/vmlinux.lds.S does use the linker script to setup __indirect_thunk_{start,end} relocations like so:

SECTIONS
{
...
        /* Text and read-only data */
        .text :  AT(ADDR(.text) - LOAD_OFFSET) {
...
                __indirect_thunk_start = .;
                *(.text.__x86.indirect_thunk)
                __indirect_thunk_end = .;

and I wonder if a pattern like this ever made it into one of the module.lds files if we're going to experience some weird failure modes. I don't know much about module linker files, so this only speculation. For another example, arch/arm64/kernel/module.lds includes this:

SECTIONS {
...
	.text.ftrace_trampoline (NOLOAD) : { BYTE(0) }
}

so maybe *(.text.*) is too inclusive and we should try to limit it to the functions that kpatch-build detects have changed?

[euspectre]

so maybe (.text.) is too inclusive and we should try to limit it to the functions that kpatch-build detects have changed?

You mean, generate kpatch.lds on the fly, with the patterns corresponding to changed functions like *(.text.*func1*), *(.text.*func2*), etc. ?

[joe-lawrence]

Yeah that was the suggestion, but maybe I'm being too paranoid, I don't know.

@jpoimboe any thoughts on this patch module linker script?

[jpoimboe]

As @euspectre said, merging sections is not an option for FGKASLR, so wouldn't this make kpatch incompatible with FGKASLR? Maybe the proper fix is indeed to use vmalloc.

[euspectre]

I'm guessing you've tested this against x86_64, how about ppc64le?

Right, I have only tested it against x86_64. If someone could check if it works OK for ppc64le as well, it would be great, because I have no such hardware available at the moment.

I can confirm that the changes can build and load many-sections.patch on ppc64le if I add __attribute__((optimize("-fno-optimize-sibling-calls"))) before the definition of pde_put().

Cool. Thanks!

[euspectre]

I have finally built kernel 5.8-rc7 with FGKASLR and monitored memory allocations with FTrace. Yes, the problem is even worse there. Large modules like xfs.ko or btrfs.ko cause allocations of 6th order for sysfs attrs:

modprobe-1509: mm_page_alloc: <...> order=4 migratetype=0 gfp_flags=GFP_KERNEL|__GFP_COMP
modprobe-1509: mm_page_alloc: <...> order=6 migratetype=0 gfp_flags=GFP_KERNEL|__GFP_COMP|__GFP_ZERO

The first alloc is from randomize_text(), size == 8 * total_number_of_sections. 8 * 4849 == 38792 bytes for xfs.ko.

The second one is what we were talking about, the one from add_sect_attrs(): size == 56 + 72 * num_loaded_sections in this case, 56 + 72 * 2428 == 174872 bytes for xfs.ko.

I'll write to the thread where FGKASLR is discussed and will probably suggest to switch to kvmalloc+kvfree in both places.

[joe-lawrence]

@euspectre : Hi, way back when you opened this bug, I remember we took the discussion over the livepatching kernel list. Did we ever implement anything upstream to fix this?

[euspectre]

@joe-lawrence FGKASLR series v5 now includes the patch that changes add_sect_attrs() and friends to use kvmalloc/kvfree, among other things: "[PATCH v5 09/10] module: Reorder functions", https://www.spinics.net/lists/kernel-hardening/msg04575.html

The series, however, was not merged and the change is not in the mainline kernel yet. They said, v6 of the series was needed but it is yet to appear.

[euspectre]

This PR can be closed, I think. Using kvmalloc/kvfree seems safer than merging sections.

[github-actions]

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.

[euspectre]

Nothing more to do here. Closing.