lookup: local_match can be empty #1345

lzw3232 · 2023-05-16T03:39:43Z

symtab may have two STT_FILE symbols with the same name, but neither of them has a local symbol.
we can choose either one,
because the lookup_table_file_sym is used to match the local symbol.

for example:
in x86:
arch/x86/mm/kaslr.c have two local symbols (can modify the code to have no local symbols)
arch/x86/lib/kaslr.c have no local symbols

if we make the patch for arch/x86/lib/kaslr.c or arch/x86/mm/kaslr.c, it will be error.

jpoimboe · 2023-05-16T23:02:32Z

Thanks for the patch. I would rather do a more explicit check, would this patch work instead? jpoimboe@143322e

lzw3232 · 2023-05-17T01:33:37Z

Thanks for the patch. I would rather do a more explicit check, would this patch work instead? jpoimboe@143322e

yes

joe-lawrence · 2023-05-17T13:01:08Z

I tried to reproduce this yesterday w/o any luck.

My kernel has three cmdline.o objects:

$ find . -name 'cmdline.o'
./arch/x86/lib/cmdline.o
./fs/proc/cmdline.o
./lib/cmdline.o

(Edit: I later realized that vmlinux.symtab isn't the correct place to look, we should be observing the reference/patched build object file symbol tables as they may contain intermediate symbols that don't end up in vmlinux)

And none of their associated cmdline.c files have any local object symbols according to ~/.kpatch/tmp/vmlinux.symtab:

 [ ... snip ... ]
 46450: 0000000000000000     0 FILE    LOCAL  DEFAULT  ABS cmdline.c
 46451: ffffffff83869918     0 NOTYPE  LOCAL  DEFAULT   43 __initcall__kmod_proc__206_19_proc_cmdline_init5
 46452: ffffffff8366ade6    38 FUNC    LOCAL  DEFAULT   39 proc_cmdline_init
 46453: ffffffff814688b0    42 FUNC    LOCAL  DEFAULT    1 cmdline_proc_show
 [ ... snip ... ]
 61488: 0000000000000000     0 FILE    LOCAL  DEFAULT  ABS cmdline.c
 61489: ffffffff82578d61     0 NOTYPE  LOCAL  DEFAULT   17 __kstrtab_get_option
 61490: ffffffff8256b9ac     0 NOTYPE  LOCAL  DEFAULT   17 __kstrtabns_get_option
 61491: ffffffff8253e284     0 NOTYPE  LOCAL  DEFAULT    9 __ksymtab_get_option
 61492: ffffffff8257c922     0 NOTYPE  LOCAL  DEFAULT   17 __kstrtab_get_options
 61493: ffffffff8256b9ac     0 NOTYPE  LOCAL  DEFAULT   17 __kstrtabns_get_options
 61494: ffffffff8253e290     0 NOTYPE  LOCAL  DEFAULT    9 __ksymtab_get_options
 61495: ffffffff82578d6c     0 NOTYPE  LOCAL  DEFAULT   17 __kstrtab_memparse
 61496: ffffffff8256b9ac     0 NOTYPE  LOCAL  DEFAULT   17 __kstrtabns_memparse
 61497: ffffffff825408d0     0 NOTYPE  LOCAL  DEFAULT    9 __ksymtab_memparse
 61498: ffffffff82578d75     0 NOTYPE  LOCAL  DEFAULT   17 __kstrtab_next_arg
 61499: ffffffff8256b9ac     0 NOTYPE  LOCAL  DEFAULT   17 __kstrtabns_next_arg
 61500: ffffffff82541770     0 NOTYPE  LOCAL  DEFAULT    9 __ksymtab_next_arg
 [ ... snip ... ]
 62671: 0000000000000000     0 FILE    LOCAL  DEFAULT  ABS cmdline.c

I can build kpatches to either arch/x86/lib/cmdline.o and /fs/proc/cmdline.o without any issues.

I had similar success with net/core/scm.c and net/unix/scm.c which only have local notype symbols... does this scenario only occur if there are no local symbols of any type (no function, object, or notype) ?

joe-lawrence · 2023-05-17T13:03:40Z

does this scenario only occur if there are no local symbols of any type (no function, object, or notype) ?

Oh, I see Josh posted a revised version of the fix -- yes, the explicit check is much more approachable for dolts like myself :)

lzw3232 · 2023-05-17T13:35:59Z

I tried to reproduce this yesterday w/o any luck.

My kernel has three cmdline.o objects:

$ find . -name 'cmdline.o'
./arch/x86/lib/cmdline.o
./fs/proc/cmdline.o
./lib/cmdline.o

And none of their associated cmdline.c files have any local object symbols according to ~/.kpatch/tmp/vmlinux.symtab:

 [ ... snip ... ]
 46450: 0000000000000000     0 FILE    LOCAL  DEFAULT  ABS cmdline.c
 46451: ffffffff83869918     0 NOTYPE  LOCAL  DEFAULT   43 __initcall__kmod_proc__206_19_proc_cmdline_init5
 46452: ffffffff8366ade6    38 FUNC    LOCAL  DEFAULT   39 proc_cmdline_init
 46453: ffffffff814688b0    42 FUNC    LOCAL  DEFAULT    1 cmdline_proc_show
 [ ... snip ... ]
 61488: 0000000000000000     0 FILE    LOCAL  DEFAULT  ABS cmdline.c
 61489: ffffffff82578d61     0 NOTYPE  LOCAL  DEFAULT   17 __kstrtab_get_option
 61490: ffffffff8256b9ac     0 NOTYPE  LOCAL  DEFAULT   17 __kstrtabns_get_option
 61491: ffffffff8253e284     0 NOTYPE  LOCAL  DEFAULT    9 __ksymtab_get_option
 61492: ffffffff8257c922     0 NOTYPE  LOCAL  DEFAULT   17 __kstrtab_get_options
 61493: ffffffff8256b9ac     0 NOTYPE  LOCAL  DEFAULT   17 __kstrtabns_get_options
 61494: ffffffff8253e290     0 NOTYPE  LOCAL  DEFAULT    9 __ksymtab_get_options
 61495: ffffffff82578d6c     0 NOTYPE  LOCAL  DEFAULT   17 __kstrtab_memparse
 61496: ffffffff8256b9ac     0 NOTYPE  LOCAL  DEFAULT   17 __kstrtabns_memparse
 61497: ffffffff825408d0     0 NOTYPE  LOCAL  DEFAULT    9 __ksymtab_memparse
 61498: ffffffff82578d75     0 NOTYPE  LOCAL  DEFAULT   17 __kstrtab_next_arg
 61499: ffffffff8256b9ac     0 NOTYPE  LOCAL  DEFAULT   17 __kstrtabns_next_arg
 61500: ffffffff82541770     0 NOTYPE  LOCAL  DEFAULT    9 __ksymtab_next_arg
 [ ... snip ... ]
 62671: 0000000000000000     0 FILE    LOCAL  DEFAULT  ABS cmdline.c

I can build kpatches to either arch/x86/lib/cmdline.o and /fs/proc/cmdline.o without any issues.

I had similar success with net/core/scm.c and net/unix/scm.c which only have local notype symbols... does this scenario only occur if there are no local symbols of any type (no function, object, or notype) ?

Both files do not have function and object local symbol

joe-lawrence · 2023-05-17T20:28:09Z

@lzw3232 : could you possibly attach the object files in question? I find it difficult to artificially create this scenario my kernel objects will end up with a local section symbol at the very least.

lzw3232 · 2023-05-18T05:14:11Z

@lzw3232 : could you possibly attach the object files in question? I find it difficult to artificially create this scenario my kernel objects will end up with a local section symbol at the very least.

I test in federa, kernel-6.2.15-100.fc36.src.rpm

patch:

diff -uNr src/arch/x86/lib/cmdline.c src_next/arch/x86/lib/cmdline.c
--- src/arch/x86/lib/cmdline.c	2023-05-18 01:01:49.045460240 -0400
+++ src_next/arch/x86/lib/cmdline.c	2023-05-18 01:01:44.288460240 -0400
@@ -203,7 +203,7 @@
 
 int cmdline_find_option_bool(const char *cmdline, const char *option)
 {
-	return __cmdline_find_option_bool(cmdline, COMMAND_LINE_SIZE, option);
+	return __cmdline_find_option_bool(cmdline, COMMAND_LINE_SIZE, option) + 1;
 }
 
 int cmdline_find_option(const char *cmdline, const char *option, char *buffer,

result:

Skipping cleanup
Using cache at /home/test/.kpatch/src
Testing patch file(s)
Reading special section data
Building original source
Building patched source
Extracting new and modified ELF sections
create-diff-object: ERROR: cmdline.o: find_local_syms: 183: found duplicate matches for cmdline.c local symbols in vmlinux symbol table
ERROR: 1 error(s) encountered. Check /home/test/.kpatch/build.log for more details.

lzw3232 · 2023-05-18T10:34:29Z

@lzw3232 : could you possibly attach the object files in question? I find it difficult to artificially create this scenario my kernel objects will end up with a local section symbol at the very least.

I test in federa, kernel-6.2.15-100.fc36.src.rpm

patch:

diff -uNr src/arch/x86/lib/cmdline.c src_next/arch/x86/lib/cmdline.c
--- src/arch/x86/lib/cmdline.c	2023-05-18 01:01:49.045460240 -0400
+++ src_next/arch/x86/lib/cmdline.c	2023-05-18 01:01:44.288460240 -0400
@@ -203,7 +203,7 @@
 
 int cmdline_find_option_bool(const char *cmdline, const char *option)
 {
-	return __cmdline_find_option_bool(cmdline, COMMAND_LINE_SIZE, option);
+	return __cmdline_find_option_bool(cmdline, COMMAND_LINE_SIZE, option) + 1;
 }
 
 int cmdline_find_option(const char *cmdline, const char *option, char *buffer,

result:

Skipping cleanup
Using cache at /home/test/.kpatch/src
Testing patch file(s)
Reading special section data
Building original source
Building patched source
Extracting new and modified ELF sections
create-diff-object: ERROR: cmdline.o: find_local_syms: 183: found duplicate matches for cmdline.c local symbols in vmlinux symbol table
ERROR: 1 error(s) encountered. Check /home/test/.kpatch/build.log for more details.

That patch’s result: (after using my commit)

ERROR: cmdline.o: 1 function(s) can not be patched
create-diff-object: unreconcilable difference
cmdline.o: function cmdline_find_option_bool has no fentry/mcount call, unable to patch

So you can use this patch for testing:

diff -uNr src/net/unix/scm.c src_next/net/unix/scm.c
--- src/net/unix/scm.c	2023-05-18 06:14:00.031460240 -0400
+++ src_next/net/unix/scm.c	2023-05-18 06:13:56.268460240 -0400
@@ -64,6 +64,7 @@
 		WRITE_ONCE(unix_tot_inflight, unix_tot_inflight + 1);
 	}
 	user->unix_inflight++;
+	user->unix_inflight++;
 	spin_unlock(&unix_gc_lock);
 }

result: (before using commit)
create-diff-object: ERROR: scm.o: find_local_syms: 183: found duplicate matches for scm.c local symbols in vmlinux symbol table
result: (after using commit)

Skipping cleanup
Using cache at /home/test/.kpatch/src
Testing patch file(s)
Reading special section data
Building original source
Building patched source
Extracting new and modified ELF sections
scm.o: changed function: unix_inflight
Patched objects: vmlinux
Building patch module: livepatch-scm.ko
SUCCESS

joe-lawrence · 2023-05-18T20:26:33Z

Thanks for the examples, @lzw3232 , that helped me confirm that Josh's patch was not fixing my scm.c examples.

Using your net/unix/scm.c patch, I wrapped it into a repro script (drop it in the cloned kpatch directory):
repro-1345.tar.gz

it fails because "__kstrtab_unix_tot_inflight" and other such exported symbols are NOTYPE and LOCAL. If I modify file_has_locals() to skip STT_NOTYPE, this repro will pass. But I'm not sure how safe that is, Josh?

lzw3232 · 2023-05-19T06:43:31Z

Thanks for the examples, @lzw3232 , that helped me confirm that Josh's patch was not fixing my scm.c examples.

Using your net/unix/scm.c patch, I wrapped it into a repro script (drop it in the cloned kpatch directory): repro-1345.tar.gz

it fails because "__kstrtab_unix_tot_inflight" and other such exported symbols are NOTYPE and LOCAL. If I modify file_has_locals() to skip STT_NOTYPE, this repro will pass. But I'm not sure how safe that is, Josh?

I think that file_has_locals() should skip symbols which are not STT_FUNC and STT_OBJECT, just like locals_match()

jpoimboe · 2023-05-31T21:34:00Z

Agreed, I'll update the patch and open a new PR.

If the file doesn't have local symbols, any empty match will do, and duplicate matching local symbol lists aren't a problem. Fixes dynup#1345. Reported-by: lzwycc <lzw32321226@163.com> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>

If the file doesn't have local object/func symbols, any empty match will do, and duplicate matching local symbol lists aren't a problem. Fixes dynup#1345. Reported-by: lzwycc <lzw32321226@163.com> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>

lzw3232 · 2023-06-05T02:23:02Z

Agreed, I'll update the patch and open a new PR.

your patch was not fixing my scm.c examples. because you forget the maybe_discarded_sym. I modify my code, Could you use my PR, because it is useful for me.

If the file doesn't have local object/func symbols, any empty match will do, and duplicate matching local symbol lists aren't a problem. Signed-off-by: lzw3232 <lzw32321226@163.com>

If the file doesn't have local object/func symbols, any empty match will do, and duplicate matching local symbol lists aren't a problem. Fixes dynup#1345. Reported-by: lzwycc <lzw32321226@163.com> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>

jpoimboe mentioned this pull request May 31, 2023

lookup: skip duplicate local symbol table check for files without locals #1346

Merged

lzw3232 force-pushed the master branch from 53bf5b9 to 0dc06ed Compare June 5, 2023 02:18

lookup: skip duplicate local symbol table check for files without locals

ba014c8

If the file doesn't have local object/func symbols, any empty match will do, and duplicate matching local symbol lists aren't a problem. Signed-off-by: lzw3232 <lzw32321226@163.com>

lzw3232 force-pushed the master branch from 0dc06ed to ba014c8 Compare June 5, 2023 02:25

jpoimboe closed this in #1346 Jul 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lookup: local_match can be empty #1345

lookup: local_match can be empty #1345

lzw3232 commented May 16, 2023

jpoimboe commented May 16, 2023

lzw3232 commented May 17, 2023

joe-lawrence commented May 17, 2023 •

edited

Loading

joe-lawrence commented May 17, 2023

lzw3232 commented May 17, 2023 •

edited

Loading

joe-lawrence commented May 17, 2023

lzw3232 commented May 18, 2023

lzw3232 commented May 18, 2023

joe-lawrence commented May 18, 2023

lzw3232 commented May 19, 2023

jpoimboe commented May 31, 2023

lzw3232 commented Jun 5, 2023

lookup: local_match can be empty #1345

lookup: local_match can be empty #1345

Conversation

lzw3232 commented May 16, 2023

jpoimboe commented May 16, 2023

lzw3232 commented May 17, 2023

joe-lawrence commented May 17, 2023 • edited Loading

joe-lawrence commented May 17, 2023

lzw3232 commented May 17, 2023 • edited Loading

joe-lawrence commented May 17, 2023

lzw3232 commented May 18, 2023

lzw3232 commented May 18, 2023

joe-lawrence commented May 18, 2023

lzw3232 commented May 19, 2023

jpoimboe commented May 31, 2023

lzw3232 commented Jun 5, 2023

joe-lawrence commented May 17, 2023 •

edited

Loading

lzw3232 commented May 17, 2023 •

edited

Loading