| Message ID | 20170322141123.opss3u4gpupqgl2q@treble (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On 03/22/2017, 03:11 PM, Josh Poimboeuf wrote: > Or, here's a much easier way to do it, without involving objtool: > > --- a/include/linux/linkage.h > +++ b/include/linux/linkage.h > @@ -138,9 +138,17 @@ > name: > #endif > > +#ifndef CHECK_DUP_SYM_END > +#define CHECK_DUP_SYM_END(name) \ > + .pushsection .discard.sym_func_end ASM_NL \ > + SYM_END_##name: .byte 0 ASM_NL \ > + .popsection > +#endif > + > /* SYM_END -- use only if you have to */ > #ifndef SYM_END > #define SYM_END(name, sym_type) \ > + CHECK_DUP_SYM_END(name) ASM_NL \ > .type name sym_type ASM_NL \ > .size name, .-name > #endif I tried this approach and it didn't work for me inside .macros. Oh, well, the name cannot be first, so now, we can have a check for both correct pairing _and_ duplicate ends in one: #define SYM_CHECK_START(name) \ .pushsection .rodata.bubak ASM_NL \ .long has_no_SYM_END_##name - . ASM_NL \ .popsection #define SYM_CHECK_END(name) \ has_no_SYM_END_##name: /* SYM_START -- use only if you have to */ #ifndef SYM_START #define SYM_START(name, align, visibility, entry) \ SYM_CHECK_START(name) ASM_NL \ visibility(name) ASM_NL \ align ASM_NL \ name: ASM_NL \ entry #endif /* SYM_END -- use only if you have to */ #ifndef SYM_END #define SYM_END(name, sym_type, exit) \ exit ASM_NL \ SYM_CHECK_END(name) ASM_NL \ .type name sym_type ASM_NL \ .size name, .-name #endif So for the ftrace mistake I did: AS arch/x86/kernel/mcount_64.o /home/latest/linux/arch/x86/kernel/mcount_64.S: Assembler messages: /home/latest/linux/arch/x86/kernel/mcount_64.S:192: Error: symbol `has_no_SYM_END_ftrace_caller' is already defined or if I remove SYM_END_FUNC completely: LD vmlinux.o MODPOST vmlinux.o arch/x86/built-in.o:(.rodata.bubak+0x130): undefined reference to `has_no_SYM_END_ftrace_stub' Sad is that this occurs only during linking, so I cannot put it in the .discard section -- ideas? thanks,
On Wed, Mar 22, 2017 at 04:01:08PM +0100, Jiri Slaby wrote: > On 03/22/2017, 03:11 PM, Josh Poimboeuf wrote: > > Or, here's a much easier way to do it, without involving objtool: > > > > --- a/include/linux/linkage.h > > +++ b/include/linux/linkage.h > > @@ -138,9 +138,17 @@ > > name: > > #endif > > > > +#ifndef CHECK_DUP_SYM_END > > +#define CHECK_DUP_SYM_END(name) \ > > + .pushsection .discard.sym_func_end ASM_NL \ > > + SYM_END_##name: .byte 0 ASM_NL \ > > + .popsection > > +#endif > > + > > /* SYM_END -- use only if you have to */ > > #ifndef SYM_END > > #define SYM_END(name, sym_type) \ > > + CHECK_DUP_SYM_END(name) ASM_NL \ > > .type name sym_type ASM_NL \ > > .size name, .-name > > #endif > > I tried this approach and it didn't work for me inside .macros. Oh, > well, the name cannot be first, so now, we can have a check for both > correct pairing _and_ duplicate ends in one: > > #define SYM_CHECK_START(name) \ > .pushsection .rodata.bubak ASM_NL \ > .long has_no_SYM_END_##name - . ASM_NL \ > .popsection > > #define SYM_CHECK_END(name) \ > has_no_SYM_END_##name: > > /* SYM_START -- use only if you have to */ > #ifndef SYM_START > #define SYM_START(name, align, visibility, entry) \ > SYM_CHECK_START(name) ASM_NL \ > visibility(name) ASM_NL \ > align ASM_NL \ > name: ASM_NL \ > entry > #endif > > /* SYM_END -- use only if you have to */ > #ifndef SYM_END > #define SYM_END(name, sym_type, exit) \ > exit ASM_NL \ > SYM_CHECK_END(name) ASM_NL \ > .type name sym_type ASM_NL \ > .size name, .-name > #endif > > > So for the ftrace mistake I did: > > AS arch/x86/kernel/mcount_64.o > /home/latest/linux/arch/x86/kernel/mcount_64.S: Assembler messages: > /home/latest/linux/arch/x86/kernel/mcount_64.S:192: Error: symbol > `has_no_SYM_END_ftrace_caller' is already defined > > > or if I remove SYM_END_FUNC completely: > LD vmlinux.o > MODPOST vmlinux.o > arch/x86/built-in.o:(.rodata.bubak+0x130): undefined reference to > `has_no_SYM_END_ftrace_stub' > > > Sad is that this occurs only during linking, so I cannot put it in the > .discard section -- ideas? Ah, interesting idea but I can't think of a way to do the missing end check before link time. But it would be easy for objtool to check for a missing end because the symbol would have a zero size.
* Josh Poimboeuf <jpoimboe@redhat.com> wrote: > On Wed, Mar 22, 2017 at 08:46:16AM +0100, Ingo Molnar wrote: > > > > * Jiri Slaby <jslaby@suse.cz> wrote: > > > > > On 03/22/2017, 08:25 AM, Ingo Molnar wrote: > > > > > > > > * Pavel Machek <pavel@ucw.cz> wrote: > > > > > > > >> Hi! > > > >> > > > >>> -ENTRY(saved_rbp) .quad 0 > > > >>> -ENTRY(saved_rsi) .quad 0 > > > >>> -ENTRY(saved_rdi) .quad 0 > > > >>> -ENTRY(saved_rbx) .quad 0 > > > >>> +SYM_DATA_START(saved_rbp) .quad 0 > > > >>> +SYM_DATA_START(saved_rsi) .quad 0 > > > >>> +SYM_DATA_START(saved_rdi) .quad 0 > > > >>> +SYM_DATA_START(saved_rbx) .quad 0 > > > >> > > > >> Does it make sense to call it SYM_DATA_*START* when there's no > > > >> corresponding end? > > > > > > > > That looks like a bug - I think we should strive for them to always be in pairs. > > > > > > > > Jiri, Josh, could objtool help here perhaps, to detect 'non-terminated' > > > > SYM_*_START() uses? This could be done by emitting debug data into a special > > > > section and then analyzing that section for unpaired entries. The section can be > > > > discarded in the final link, it won't show up in the kernel image. > > > > > > It should be easier than that. No introduction of other info needed -- > > > every global symbol without a ".type" or ".size" (i.e. SYM_*_END) should > > > be a bug now. > > > > I'm all for that! > > It would be easy to add this checking to objtool since it already reads > the symbol table. The hard part is figuring out the logistics. :-) > > - Should the warnings be on by default? Yes, if objtool is running. Keep it simple. > - Part of the "objtool check" command or something else? Yes - I think it's still within the 'object file check' functionality. > - Separate config option or just include it with > CONFIG_STACK_VALIDATION? Yeah, but I'd rename CONFIG_STACK_VALIDATION to CONFIG_OBJ_VALIDATION or such. As I predicted early on, objtool will go beyond stack checking! ;-) > - Should all asm files be checked, including those currently skipped by > objtool with OBJECT_FILES_NON_STANDARD? The symbol syntax check should definitely be for all files, yes. Could we perhaps emit 'non-standard stack frames' information into the .o itself (via a flag or a special section?), so that objtool can decide on its own whether to complain about any weirdnesses there? > > Can we detect double ends as well - i.e. do a build check of the full syntax of > > these symbol definition primitives? > > Detecting double ends would be a little trickier. The second SYM_*_END > supersedes the first, so that information isn't in the ELF symbol table. Indeed. > We could use a special section to annotate all the macro uses and have > objtool do the checking, similar to what you suggested earlier. That might be useful for other purposes as well - such as the non-standard stack frame annotations? But it's your call really: I'm principally fine with any of the solutions, as long as the checking is done. Thanks, Ingo
On Thu, Mar 23, 2017 at 08:38:20AM +0100, Ingo Molnar wrote: > > * Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > > On Wed, Mar 22, 2017 at 08:46:16AM +0100, Ingo Molnar wrote: > > > > > > * Jiri Slaby <jslaby@suse.cz> wrote: > > > > > > > On 03/22/2017, 08:25 AM, Ingo Molnar wrote: > > > > > > > > > > * Pavel Machek <pavel@ucw.cz> wrote: > > > > > > > > > >> Hi! > > > > >> > > > > >>> -ENTRY(saved_rbp) .quad 0 > > > > >>> -ENTRY(saved_rsi) .quad 0 > > > > >>> -ENTRY(saved_rdi) .quad 0 > > > > >>> -ENTRY(saved_rbx) .quad 0 > > > > >>> +SYM_DATA_START(saved_rbp) .quad 0 > > > > >>> +SYM_DATA_START(saved_rsi) .quad 0 > > > > >>> +SYM_DATA_START(saved_rdi) .quad 0 > > > > >>> +SYM_DATA_START(saved_rbx) .quad 0 > > > > >> > > > > >> Does it make sense to call it SYM_DATA_*START* when there's no > > > > >> corresponding end? > > > > > > > > > > That looks like a bug - I think we should strive for them to always be in pairs. > > > > > > > > > > Jiri, Josh, could objtool help here perhaps, to detect 'non-terminated' > > > > > SYM_*_START() uses? This could be done by emitting debug data into a special > > > > > section and then analyzing that section for unpaired entries. The section can be > > > > > discarded in the final link, it won't show up in the kernel image. > > > > > > > > It should be easier than that. No introduction of other info needed -- > > > > every global symbol without a ".type" or ".size" (i.e. SYM_*_END) should > > > > be a bug now. > > > > > > I'm all for that! > > > > It would be easy to add this checking to objtool since it already reads > > the symbol table. The hard part is figuring out the logistics. :-) > > > > - Should the warnings be on by default? > > Yes, if objtool is running. Keep it simple. > > > - Part of the "objtool check" command or something else? > > Yes - I think it's still within the 'object file check' functionality. > > > - Separate config option or just include it with > > CONFIG_STACK_VALIDATION? > > Yeah, but I'd rename CONFIG_STACK_VALIDATION to CONFIG_OBJ_VALIDATION or such. As > I predicted early on, objtool will go beyond stack checking! ;-) > > > - Should all asm files be checked, including those currently skipped by > > objtool with OBJECT_FILES_NON_STANDARD? > > The symbol syntax check should definitely be for all files, yes. That all sounds reasonable. I'll work something up. > Could we perhaps emit 'non-standard stack frames' information into the .o itself > (via a flag or a special section?), so that objtool can decide on its own whether > to complain about any weirdnesses there? For the OBJECT_FILES_NON_STANDARD case, where the whole file is "special", we can just provide a flag to "objtool check" to tell it to skip stack checking for that file, but still do the symbol checks. > > > Can we detect double ends as well - i.e. do a build check of the full syntax of > > > these symbol definition primitives? > > > > Detecting double ends would be a little trickier. The second SYM_*_END > > supersedes the first, so that information isn't in the ELF symbol table. > > Indeed. > > > We could use a special section to annotate all the macro uses and have > > objtool do the checking, similar to what you suggested earlier. > > That might be useful for other purposes as well - such as the non-standard stack > frame annotations? To start with we can try going without all the special sections (other than the SYM_END double end check). If we end up finding another case which isn't covered then we can always add the special sections later.
--- a/include/linux/linkage.h +++ b/include/linux/linkage.h @@ -138,9 +138,17 @@ name: #endif +#ifndef CHECK_DUP_SYM_END +#define CHECK_DUP_SYM_END(name) \ + .pushsection .discard.sym_func_end ASM_NL \ + SYM_END_##name: .byte 0 ASM_NL \ + .popsection +#endif + /* SYM_END -- use only if you have to */ #ifndef SYM_END #define SYM_END(name, sym_type) \ + CHECK_DUP_SYM_END(name) ASM_NL \ .type name sym_type ASM_NL \ .size name, .-name #endif