Hi,
On 03.06.2019 23:05, Joe Lawrence wrote:
> On 6/3/19 7:14 AM, Evgenii Shatokhin wrote:
>> Hi,
>>
>> It is possible that keeping each new and patched function in a separate
>> ELF section in a livepatch kernel module could cause problems.
>>
>> During the testing of binary patches for the kernels used in Virtuozzo
>> 7, I found that the kernel may try to allocate a relatively large (for
>> kmalloc) memory area to support sysfs files
>> /sys/module/<module_name>/sections/* for the patch modules. The size was
>> 16 - 35 KB, depending on the patch, i.e. 3rd and 4th order allocations.
>> The numbers do not look very big but still increase the chance that the
>> patch module will fail to load when the memory is fragmented.
>>
>> kernel/module.c, add_sect_attrs():
>> /* Count loaded sections and allocate structures */
>> for (i = 0; i < info->hdr->e_shnum; i++)
>> if (!sect_empty(&info->sechdrs[i]))
>> nloaded++;
>> 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);
>>
>> So, in our case, the size of the requested memory chunk was
>> 48 + 80 * <number_of_loaded_ELF_sections>.
>>
>> Both livepatch and old-style KPatch kernel modules place each new or
>> patched function in a separate section, same for the new global and
>> static data...
>
> Hi Evgenii,
>
> To be specific, I think this is a kpatch-related issue, right? I don't
> recall there being any section specifications in the upstream livepatch
> code base (aside from arch-specific relocations for alt/para inst, etc.)
Yes, I think so. The patch modules built with kpatch-build, be they
livepatch-compatible or the old-style kpatch modules, keep the functions
in separate sections and therefore have the issue.
As far as I can see, Documentation/livepatch/module-elf-format.rst does
not strictly require such separation of functions, etc.
I forgot about .klp.rela.*, however. It is needed to check if merging of
.text.* sections plays well with livepatch relocations. I'll experiment
with that a bit more.
>
> > ... So, if we patch 200+ kernel functions (which not that
>> unusual in cumulative patches), the kernel will try to allocate around
>> 16 KB of memory for these sysfs data only. The largest of our binary
>> patches have around 400 new and patched functions, plus a few dozens of
>> new data items, which results in a wasted kernel memory chunk of 35 KB
>> in size.
>>
>> Here are the questions.
>>
>> 1. The files /sys/module/<module_name>/sections/* currently contain the
>> start addresses of the relevant sections, i.e. the addresses of new and
>> patched functions among other things. Is this info really needed for
>> livepatch kernel modules after they have been loaded?
>
> I think this is generic module debugging info and that livepatch isn't
> interested in these. I suppose there might be some userspace tools that
> look at those sysfs files for quick verification purposes... though
> there appears to be some filtering of sections that are listed here. Not
> sure which meet the criteria.
>
>> 2. Of course, create-diff-object relies heavily upon placing each new or
>> patched function in a separate section. But is it needed to keep the
>> functions there after the diff object files have been prepared?
>>
>> Does the code that loads/unloads the patches require that each function
>> is kept that way? Looks like no, but I am not 100% sure.
>
> Josh will know better than me, but I suspect these sections are just an
> artifact of -ffunction-sections used for the kpatch ELF comparison and
> extraction.
Yes, it seems so.
>
>> As an experiment, I added the following to kmod/patch/kpatch.lds.S to
>> merge all .text.* sections into .text.livepatch in the resulting patch
>> module:
>>
>> SECTIONS
>> {
>> + .text.livepatch : {
>> + *(.text.*)
>> + }
>>
>> My test patch module with around 200 changed functions was built OK with
>> that, the functions were actually placed into .text.livepatch section as
>> requested. The patch was loaded fine but I haven't tested it much yet.
>>
>> It also might be reasonable to merge .rodata.__func__.* the same way.
>>
>
> Nice. If you have success with this approach, please post up a PR on
> the kpatch github for review.
I'd like to experiment with that in upstream livepatch-compatible
patches a bit more. dynrela / .klp.rela* are my primary concern here. If
the approach plays well with these, I'll post the PR, of course.
>
> Also, in the interest of minimizing sections, perhaps the kpatch
> callback and force sections can be omitted if they are unused. Do they
> count towards the kmalloc allocation you listed above or are they
> skipped when empty?
If I am not mistaken, zero-size sections are skipped, so we are fine
here. sect_empty() from kernel/module.c checks both SHF_ALLOC flag and
the size of the section.
>
>> Are there any pitfalls in such merging of sections? Am I missing
>> something obvious?
> Regards,
>
> -- Joe
> .
>
Regards,
Evgenii
