weixin_39911916
weixin_39911916
2020-11-23 13:14

create-diff-object: Use .rela.toc as list head in kpatch_correlate_st…

…atic_local_variables

kpatch_correlate_static_local_variables() assumes that the .text sections are parsed ahead of .rodata sections and the references to static local variable are correlated by the .text section rela referring them. This assumption is not true on PowerPC, as all data loads for -mcmodel=large are loaded from .toc section + offset and fails with the error:

ERROR: netfilter.o: reference to static local variable fake_pinfo.63552 in fake_sk.63553 was removed

As per PPC64le ABIv2: "TOC may straddle the boundary between initialized and uninitialized data in the data segment."

$ readelf -s ./net/ipv6/netfilter.o ... [16] .rodata.func.63533 [17] .rodata.fake_sk.63553 [18] .rela.rodata.fake_sk.63553 ... [28] .toc PROGBITS [29] .rela.toc RELA [30] .data PROGBITS [31] .data.rel.ro.ipv6ops PROGBITS

fix this issue by looping .toc as section head if available, creating the correlation by referring to section twin of .rela.toc.

Signed-off-by: Kamalesh Babulal

该提问来源于开源项目:dynup/kpatch

  • 点赞
  • 写回答
  • 关注问题
  • 收藏
  • 复制链接分享
  • 邀请回答

10条回答

  • weixin_39911916 weixin_39911916 5月前

    Is that not a bug in kpatch_mangled_strcmp()? When comparing the relas, shouldn't it also compare the toc_relas?

    Agree, toc_rela() makes .rela.toc symbols reference easier. Does the code looks ok, build upon your idea, with sec->twin issue fixed. Shall I re-do the git pull request for easier review?

    
    @@ -872,8 +872,8 @@ static void kpatch_correlate_static_local_variables(struct kpatch_elf *base,
                                                        struct kpatch_elf *patched)
     {
            struct symbol *sym, *patched_sym;
    -       struct section *sec;
    -       struct rela *rela, *rela2;
    +       struct section *sec, *toc_sec;
    +       struct rela *rela, *rela2, *rela_toc;
            int bundled, patched_bundled, found;
    
            /*
    @@ -918,7 +918,16 @@ static void kpatch_correlate_static_local_variables(struct kpatch_elf *base,
    
                    list_for_each_entry(rela, &sec->relas, list) {
    
    -                       sym = rela->sym;
    +                       rela_toc = toc_rela(rela);
    +                       /* skip toc constants */
    +                       if (!rela_toc)
    +                               continue;
    +
    +                       sym = rela_toc->sym;
    +                       toc_sec = sec;
    +                       if (!strcmp(rela->sym->name, ".toc"))
    +                               toc_sec = rela->sym->sec->rela;
    +                       
                            if (!kpatch_is_normal_static_local(sym))
                                    continue;
    
    @@ -939,10 +948,11 @@ static void kpatch_correlate_static_local_variables(struct kpatch_elf *base,
                                    continue;
                            }
    
    -                       patched_sym = kpatch_find_static_twin(sec, sym);
    +                       patched_sym = kpatch_find_static_twin(toc_sec, sym);
                            if (!patched_sym)
                                    DIFF_FATAL("reference to static local variable %s in %s was removed",
    -                                          sym->name,
    +                                          sym->name, /* Use sec instead of toc_se, to print section
    +                                                        referring the function */
                                               kpatch_section_function_name(sec));
    
                            patched_bundled = patched_sym == patched_sym->sec->sym;
    
    点赞 评论 复制链接分享
  • weixin_39519554 weixin_39519554 5月前

    From a quick glance, I think it looks ok.

    BTW, when I said

    Is that not a bug in kpatch_mangled_strcmp()?

    I meant to say:

    Is that not a bug in rela_equal()?

    So I think rela_equal() needs to do something similar, where it compares both relas and toc_relas.

    点赞 评论 复制链接分享
  • weixin_39911916 weixin_39911916 5月前

    So I think rela_equal() needs to do something similar, where it compares both relas and toc_relas.

    I have a refactored rela_equal() to use toc_rela(), will post the patch for review.

    点赞 评论 复制链接分享
  • weixin_39911916 weixin_39911916 5月前

    v2 Changes: * Changed the approach to use toc_rela() to reference the symbol from .toc section and correlate the static local variable used by the function section, instead of depending upon .toc section to be traversed first. This approached based on the suggestion from Josh. * Add test case to reproduce this issue.

    点赞 评论 复制链接分享
  • weixin_39992760 weixin_39992760 5月前

    Hi -babulal ,

    Do you mind if I let review v2 when he gets back? I don't have the PPC64 .toc stuff fresh in my head and he can probably do a better job continuing his review.

    Thanks for adding the test case, one very small nit: "kpatch-macros.h" shouldn't be necessary if all the patch it doing is a trivial modification to get those functions included.

    点赞 评论 复制链接分享
  • weixin_39911916 weixin_39911916 5月前

    -lawrence no, not at all. Thanks for the review. Agree, will remove "kpatch-macro.h" from the testcase. I guess, we should remove it from gcc-static-local-var-2.patch testcase also. I will re-spin the patch, along with change to gcc-static-local-var-2.patch after Josh's review.

    点赞 评论 复制链接分享
  • weixin_39911916 weixin_39911916 5月前

    Thanks for working on making kpatch_correlate_static_local_variables() .toc aware. I have re-pushed with pull request with testcase alone.

    点赞 评论 复制链接分享
  • weixin_39519554 weixin_39519554 5月前

    The static local variable correlation has been (and continues to be) tricky to get right, and I think that just looking at the .toc section isn't going to be enough information for more complex situations, for example where there are multiple static variables with the same name (but used in different functions).

    How about this instead, would this work?

    
    diff --git a/kpatch-build/create-diff-object.c b/kpatch-build/create-diff-object.c
    index c075a57..20478c8 100644
    --- a/kpatch-build/create-diff-object.c
    +++ b/kpatch-build/create-diff-object.c
    @@ -922,7 +922,7 @@ static void kpatch_correlate_static_local_variables(struct kpatch_elf *base,
    
                    list_for_each_entry(rela, &sec->relas, list) {
    
    -                       sym = rela->sym;
    +                       sym = toc_rela(rela)->sym;
                            if (!kpatch_is_normal_static_local(sym))
                                    continue;
    
    
    点赞 评论 复制链接分享
  • weixin_39911916 weixin_39911916 5月前

    Thanks for the review. It fails with

    
    ERROR: netfilter.o: reference to static local variable fake_sk.63585 in nf_ip6_route was removed
    

    Reason being section is .rela.text.nf_ip6_route, whereas reference symbol rela .rodata.fake_sk.63585 belongs to .rela.toc and fails to find the match in kpatch_mangled_strcmp()

    My understanding of kpatch_correlate_static_local_variables(), might be wrong. I ran testcases, gcc-static-local-var-2.patch gcc-static-local-var-3.patch gcc-static-local-var-4.patch gcc-static-local-var-5.patch meminfo-cmdline-rebuild-SLOW.patch

    And all of the reference in .rela.toc generated by the test cases, work because the static local belongs to .data* sections and they come after .rela.toc section but in this particular case the reference is ahead of .rela.toc in .bss section, which is ahead of .rela.toc . The approach in this patch treats .rela.toc like any other .text.* section, which is handled ahead of .data* sections.

    Do you think, its good idea to add a testcase, which trigger this issue:

    
    diff --git a/net/ipv6/netfilter.c b/net/ipv6/netfilter.c
    index 531d6957af36..d91ae53ccfde 100644
    --- a/net/ipv6/netfilter.c
    +++ b/net/ipv6/netfilter.c
    @@ -84,6 +84,8 @@ static int nf_ip6_reroute(struct sk_buff *skb,
            return 0;
     }
    
    +#include "kpatch-macros.h"
    +
     static int nf_ip6_route(struct net *net, struct dst_entry **dst,
                            struct flowi *fl, bool strict)
     {
    @@ -97,6 +99,8 @@ static int nf_ip6_route(struct net *net, struct dst_entry **dst,
            struct dst_entry *result;
            int err;
    
    +       if (!jiffies)
    +               printk("kpatch nf_ip6_route foo\n");
            result = ip6_route_output(net, sk, &fl->u.ip6);
            err = result->error;
            if (err)
    
    点赞 评论 复制链接分享
  • weixin_39519554 weixin_39519554 5月前

    Reason being section is .rela.text.nf_ip6_route, whereas reference symbol rela .rodata.fake_sk.63585 belongs to .rela.toc and fails to find the match in kpatch_mangled_strcmp()

    Is that not a bug in kpatch_mangled_strcmp()? When comparing the relas, shouldn't it also compare the toc_relas?

    The approach in this patch treats .rela.toc like any other .text. section, which is handled ahead of .data sections.

    But with -mcmodel=large, isn't .rela.toc going to be the only section that will access static local variables?

    If the intent was to iterate through all the sections, list_for_each_entry() isn't the right way to do that. It can't start in the middle of the list unless you use something like list_for_each_entry_continue(), but even then it wouldn't loop around the begining.

    Do you think, its good idea to add a testcase, which trigger this issue:

    Yes, very good idea.

    点赞 评论 复制链接分享

相关推荐