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

kamalesh-babulal · 2018-03-12T14:28:54Z

…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 kamalesh@linux.vnet.ibm.com

jpoimboe · 2018-03-15T16:20:07Z

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;

kamalesh-babulal · 2018-03-16T15:13:51Z

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)

jpoimboe · 2018-03-16T20:24:51Z

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.

kamalesh-babulal · 2018-03-19T15:48:19Z

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;

jpoimboe · 2018-03-19T19:42:58Z

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.

kamalesh-babulal · 2018-03-20T03:54:59Z

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.

kamalesh-babulal · 2018-03-26T05:53:58Z

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.

joe-lawrence · 2018-03-28T20:42:43Z

Hi @kamalesh-babulal ,

Do you mind if I let @jpoimboe 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.

kamalesh-babulal · 2018-03-29T09:36:53Z

@joe-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.

jpoimboe · 2018-04-04T21:15:41Z

kpatch-build/create-diff-object.c

 			}

-			patched_sym = kpatch_find_static_twin(sec, sym);
+			patched_sym = kpatch_find_static_twin(toc_sec, sym);


I think this is still not quite right. It's comparing the .rela.toc section relas. Instead it should be comparing the original .rela.data.* section relas. Otherwise it may get confused and correlate the wrong variables. For example, if there are multiple static locals with the same name, but in different .data.* sections, they should not be correlated. But this code might accidentally correlate them, because they're both referenced in .rela.toc.

Instead I think the original .data.* section should be passed to kpatch_find_static_twin(), and kpatch_find_static_twin() should be fixed to compare toc_rela-based symbols.

The static local variable correlation is a bit confusing (and actually needs to be simplified). If you want, I can try to take a pass at fixing this bug.

I think this is still not quite right. It's comparing the .rela.toc section relas. Instead it should be comparing the original .rela.data.* section relas.

This stuff is really hard to communicate precisely, it might be easier if I write a patch to communicate it :-)

On ppc64le, the static local variable correlation doesn't take into account the .toc rela indirection for data references, meaning that it's basically broken in many cases. Fix it by making the code .toc-aware. Fixes dynup#793. Reported-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>

This patch adds a reproducer testcase for correlating static local variables added in .rodata* section, ahead of .toc section. This testcase is for issue seen on PowerPC. For more details on the issue refer pull request: 793 and 817. It add's the testcase for: - Fedora-27 Kernel version 4.15.10-300 - Centos-7 Kernel version 3.10.0-693 and - Ubuntu-16.04 Kernel version 4.13.0-25.29 Signed-off-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>

kamalesh-babulal · 2018-04-05T07:12:48Z

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

kamalesh-babulal mentioned this pull request Mar 24, 2018

[RFC] PC64le - do not use the patched functions as callbacks directly #789

Merged

kamalesh-babulal force-pushed the ppc64le_correlate branch from bcf35ac to 2c123b4 Compare March 26, 2018 05:47

jpoimboe reviewed Apr 4, 2018

View reviewed changes

jpoimboe mentioned this pull request Apr 4, 2018

create-diff-object: fix ppc64le static local variable correlation #817

Merged

kamalesh-babulal force-pushed the ppc64le_correlate branch from 2c123b4 to 63d81e2 Compare April 5, 2018 07:08

jpoimboe closed this in #817 Apr 5, 2018

kamalesh-babulal deleted the ppc64le_correlate branch April 6, 2018 05:21

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

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

Uh oh!

Conversation

kamalesh-babulal commented Mar 12, 2018

Uh oh!

jpoimboe commented Mar 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kamalesh-babulal commented Mar 16, 2018

Uh oh!

jpoimboe commented Mar 16, 2018

Uh oh!

kamalesh-babulal commented Mar 19, 2018

Uh oh!

jpoimboe commented Mar 19, 2018

Uh oh!

kamalesh-babulal commented Mar 20, 2018

Uh oh!

kamalesh-babulal commented Mar 26, 2018

Uh oh!

joe-lawrence commented Mar 28, 2018

Uh oh!

kamalesh-babulal commented Mar 29, 2018

Uh oh!

jpoimboe Apr 4, 2018

Choose a reason for hiding this comment

Uh oh!

jpoimboe Apr 4, 2018

Choose a reason for hiding this comment

Uh oh!

kamalesh-babulal commented Apr 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jpoimboe commented Mar 15, 2018 •

edited

Loading