Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

link-vmlinux-syms doesn't handle symbol collisions properly #54

Closed
sjenning opened this issue Mar 12, 2014 · 10 comments
Closed

link-vmlinux-syms doesn't handle symbol collisions properly #54

sjenning opened this issue Mar 12, 2014 · 10 comments

Comments

@sjenning
Copy link
Contributor

Same problem as #53 only with link-vmlinux-syms when pinning addresses into the symbol table of the kernel module.

@sjenning
Copy link
Contributor Author

This issue is actually hard to resolve. create-diff-object changes unchanged LOCAL symbols to GLOBAL symbols with section index UNDEF so that they can be linked to the version in vmlinux. However, this introduces the chance that the local symbol name collides with an existing global symbol in the kernel and we can't determine, from link-vmlinux-syms point of view, which symbol was being referenced since it has no FILE context as LOCAL symbols do.

In the meantime, commit 2be6178 checks for this ambiguity and errors out of the situation above is ever encountered.

@jpoimboe
Copy link
Member

I attempted to recreate this issue, but strangely got another problem. I used the following patch:

Index: src/mm/ksm.c
===================================================================
--- src.orig/mm/ksm.c
+++ src/mm/ksm.c
@@ -2216,6 +2216,10 @@ static ssize_t pages_to_scan_store(struc
 {
    int err;
    unsigned long nr_pages;
+   char buf2[64];
+
+   pages_to_scan_show(kobj, attr, buf2);
+   printk("pages_to_scan_show says %s\n", buf2);

    err = kstrtoul(buf, 10, &nr_pages);
    if (err || nr_pages > UINT_MAX)

I expected an error, but instead it succeeded, and strangely pages_to_scan_show doesn't show up in the symbol table.

@sjenning
Copy link
Contributor Author

Looking at the rela table for pages_to_scan_store with the patch applied:

Relocation section [30] '.rela.text.pages_to_scan_store' for section [29] '.text.pages_to_scan_store' at offset 0x8868 contains 9 entries:
  Offset              Type            Value               Addend Name
  0x0000000000000001  X86_64_NONE     000000000000000000      -4 __fentry__
  0x0000000000000009  X86_64_32S      000000000000000000      +0 .rodata.str1.1
  0x0000000000000023  X86_64_PC32     000000000000000000      -4 .data.ksm_thread_pages_to_scan
  0x0000000000000037  X86_64_PC32     000000000000000000      -4 sprintf
  0x0000000000000042  X86_64_32S      000000000000000000     +14 .rodata.str1.1
  0x0000000000000049  X86_64_PC32     000000000000000000      -4 printk
  0x000000000000005a  X86_64_PC32     000000000000000000      -4 kstrtoull
  0x0000000000000072  X86_64_PC32     000000000000000000      -4 .data.ksm_thread_pages_to_scan
  0x00000000000000a2  X86_64_PC32     000000000000000000      -4 __stack_chk_fail

Looks like the compiler is inlining pages_to_scan_show.

@sjenning
Copy link
Contributor Author

And the patch does work :)

# echo 100 > pages_to_scan 
# dmesg
[17771.153314] pages_to_scan_show says 100

@jpoimboe
Copy link
Member

Blasted compiler.

@jpoimboe
Copy link
Member

/me wonders how to get gcc to not inline an otherwise not inlined function.

@sjenning
Copy link
Contributor Author

I'm playing around with using -fno-inline, in addition to -f[function|data]-sections in the KCFLAGS for building the diff objects.

@sjenning
Copy link
Contributor Author

sjenning commented Apr 7, 2014

This issue is as fixed as it can be right now, see commit 2be6178

@sjenning sjenning closed this as completed Apr 7, 2014
@jpoimboe
Copy link
Member

This may be lower priority, but let's leave it open since it helps us keep track of a real issue that will need to be fixed at some point. I'm not sure that changing unchanged LOCAL symbols to GLOBAL undef symbols in create-diff-object is the best approach.

@jpoimboe jpoimboe reopened this Apr 14, 2014
@sjenning
Copy link
Contributor Author

With the addition of dynrela support, link-vmlinux-syms doesn't exist anymore and the issue has disappeared because we no longer do the change-symbol-binding trick for pinning addresses into the symbol table.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants