PPC64le: Replacing reference of array without STT_OBJECT fails

[kamalesh-babulal]

Consider core.c, where function sched_cpu_deactivate() calls another function synchronize_rcu_mult(), which expects two function pointers as its arguments.

int sched_cpu_deactivate(unsigned int cpu)
{
        int ret;

        set_cpu_active(cpu, false);
        /*
         * We've cleared cpu_active_mask, wait for all preempt-disabled and RCU
         * users of this state to go away such that all new such users will
         * observe it.
         *
         * Do sync before park smpboot threads to take care the rcu boost case.
         */
        synchronize_rcu_mult(call_rcu, call_rcu_sched);
... 
}

Relocation section '.rela.toc' at offset 0xae458 contains 161 entries:
    Offset             Info             Type               Symbol's Value  Symbol's Name + Addend
...
0000000000000460  0000011c00000026 R_PPC64_ADDR64         0000000000000000 .data + 0

Relocation section '.rela.data' at offset 0xaf370 contains 2 entries:
    Offset             Info             Type               Symbol's Value  Symbol's Name + Addend
0000000000000000  000003b200000026 R_PPC64_ADDR64         0000000000000000 call_rcu_sched + 0
0000000000000008  000003b200000026 R_PPC64_ADDR64         0000000000000000 call_rcu_sched + 0

0000000000000008 <sched_cpu_deactivate>:
...
  84:   f4 ff c2 40     bne-    78 <sched_cpu_deactivate+0x70>
  88:   00 00 42 3d     addis   r10,r2,0
                        88: R_PPC64_TOC16_HA    .toc+0x460
  8c:   00 00 60 38     li      r3,0
  90:   00 00 2a e9     ld      r9,0(r10)
                        90: R_PPC64_TOC16_LO_DS .toc+0x460
  94:   60 00 c1 38     addi    r6,r1,96
  98:   c0 00 a1 38     addi    r5,r1,192
  9c:   02 00 80 38     li      r4,2
  a0:   00 00 49 e9     ld      r10,0(r9)
  a4:   08 00 69 e9     ld      r11,8(r9)

$ readelf -s ./kernel/sched/core.o|grep -i __crcu_array|wc -l
$ 0

Code snippet for the above array of function pointer is:

#define _wait_rcu_gp(checktiny, ...) \

do {                                                                 \
        call_rcu_func_t __crcu_array[] = { __VA_ARGS__ };            \
        struct rcu_synchronize __rs_array[ARRAY_SIZE(__crcu_array)]; \
        __wait_rcu_gp(checktiny, ARRAY_SIZE(__crcu_array),           \
                        __crcu_array, __rs_array);                   \
} while (0)

#define wait_rcu_gp(...) _wait_rcu_gp(false, __VA_ARGS__)

#define synchronize_rcu_mult(...)                                    \
        _wait_rcu_gp(IS_ENABLED(CONFIG_TINY_RCU), __VA_ARGS__) 

Attempt to replace sym reference .data + 0 in .toc + 460 section fails in kpatch_replace_sections_syms() as the STT_OBJECT or array name __crcu_array is not generated. The question is how to resolve .toc section + 0x458 to the original array in core.c, when STT_OBJECT for __crcu_array is not generated.

[kamalesh-babulal]

Such local objects are simplified by the assembler in .toc reloc to .data + offset. The idea being that this potentially reduces the number of unnecessary local symbols.

One idea is to modify the .toc reloc, from .data+off preferably to an absolute kernel symbol plus the appropriate offset, or omit the symbol and just use the absolute address as the addend. The difficulty is in figuring out how to map patched .o file .data to a kernel address. One way to find where the original .o file .data is placed would be to generate a linker map file when linking the kernel and use the linker map to find the absolute address of .data + 0.

[jpoimboe]

So __crcu_array[] is a local variable. That's why GCC doesn't make a symbol for it. It doesn't make sense to make symbols for local variables, unless they're static.

But local variables are supposed to be allocated on the stack. So why does GCC put it in .data? Is this a weird ppc64le ABI thing? Or is it a weird GCC optimization? Either way it's weird :-)

[jpoimboe]

Anyway, because this array is a local variable, I don't think it makes sense to try to find it in the kernel after all.

Instead, the array belongs to the patched function in the patch module. So leaving it alone, as a normal relocation, sounds like the right thing to do.

Of course the problem with that is, then we need to include the .data section, but we don't support that because .data is normally used for global data, and we don't support global data changes.

So it would be good to understand why GCC is putting local "stack" data in .data, and if there's anything we can do to change that. Maybe by setting some gcc flag to disable the "feature"? Or maybe by using the gcc plugin to put the array in another section? I don't know...

[kamalesh-babulal]

But local variables are supposed to be allocated on the stack. So why does GCC put it in .data? Is this a weird ppc64le ABI thing? Or is it a weird GCC optimization? Either way it's weird :-)

Thanks for looking at the issue. It's GCC optimization, the idea being that this potentially reduces the number of unnecessary local symbols.

So it would be good to understand why GCC is putting local "stack" data in .data, and if there's anything we can do to change that. Maybe by setting some gcc flag to disable the "feature"?

I doubt there is a GCC flag to disable this optimization. I will try to dig more on hints.

[jpoimboe]

But local variables are supposed to be allocated on the stack. So why does GCC put it in .data? Is this a weird ppc64le ABI thing? Or is it a weird GCC optimization? Either way it's weird :-)

Thanks for looking at the issue. It's GCC optimization, the idea being that this potentially reduces the number of unnecessary local symbols.

But stack variables don't have ELF symbols anyway. So that doesn't explain why the array was put in .data instead of the stack.

[kamalesh-babulal]

But stack variables don't have ELF symbols anyway. So that doesn't explain why the array was put in .data instead of the stack.

One assumption is when, compiler runs into code like:

void (*fp)(void) = foo;

To cope with a function pointer in .data or possibly other sections. The compiler might do the brace initialization, gcc is effectively introducing a temporary variable like the above user code, ie. as if the first part of the _wait_rcu_gp macro expansion was

static call_rcu_func_t temp_arr[] = {call_rcu, call_rcu_sched};
call_rcu_func_t __crcu_array[] = temp_arr;

As per toolchains developers, It isn't unreasonable for a compiler to do this. There is no way to force gcc to not to do such initialization.

[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.