| Message ID | 20220209185752.1226407-3-alexandr.lobakin@intel.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
| Series | Function Granular KASLR | expand |
On Wed, Feb 09, 2022 at 07:57:39PM +0100, Alexander Lobakin wrote: > Position-based search, which means that if there are several symbols > with the same name, the user needs to additionally provide the > "index" of a desired symbol, is fragile. For example, it breaks > when two symbols with the same name are located in different > sections. > > Since a while, LD has a flag `-z unique-symbol` which appends > numeric suffixes to the functions with the same name (in symtab > and strtab). It can be used to effectively prevent from having > any ambiguity when referring to a symbol by its name. In the patch description can you also give the version of binutils (and possibly other linkers) which have the flag? > Check for its availability and always prefer when the livepatching > is on. It can be used unconditionally later on after broader testing > on a wide variety of machines, but for now let's stick to the actual > CONFIG_LIVEPATCH=y case, which is true for most of distro configs > anyways. Has anybody objected to just enabling it for *all* configs, not just for livepatch? I'd much prefer that: the less "special" livepatch is (and the distros which enable it), the better. And I think having unique symbols would benefit some other components. > +++ b/kernel/livepatch/core.c > @@ -143,11 +143,13 @@ static int klp_find_callback(void *data, const char *name, > args->count++; > > /* > - * Finish the search when the symbol is found for the desired position > - * or the position is not defined for a non-unique symbol. > + * Finish the search when unique symbol names are enabled > + * or the symbol is found for the desired position or the > + * position is not defined for a non-unique symbol. > */ > - if ((args->pos && (args->count == args->pos)) || > - (!args->pos && (args->count > 1))) > + if (IS_ENABLED(CONFIG_LD_HAS_Z_UNIQUE_SYMBOL) || > + (args->pos && args->count == args->pos) || > + (!args->pos && args->count > 1)) > return 1; There's no real need to do this. The code already works as-is, even if there are no unique symbols. Even if there are no duplicates, there's little harm in going through all the symbols anyway, to check for errors just in case something unexpected happened with the linking (unexpected duplicate) or the patch creation (unexpected sympos). It's not a hot path, so performance isn't really a concern. When the old linker versions eventually age out, we can then go strip out all the sympos stuff. > @@ -169,6 +171,13 @@ static int klp_find_object_symbol(const char *objname, const char *name, > else > kallsyms_on_each_symbol(klp_find_callback, &args); > > + /* > + * If the LD's `-z unique-symbol` flag is available and enabled, > + * sympos checks are not relevant. > + */ > + if (IS_ENABLED(CONFIG_LD_HAS_Z_UNIQUE_SYMBOL)) > + sympos = 0; > + Similarly, I don't see a need for this. If the patch is legit then sympos should already be zero. If not, an error gets reported and the patch fails to load.
On Fri, Feb 11, 2022 at 9:41 AM Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > On Wed, Feb 09, 2022 at 07:57:39PM +0100, Alexander Lobakin wrote: > > Position-based search, which means that if there are several symbols > > with the same name, the user needs to additionally provide the > > "index" of a desired symbol, is fragile. For example, it breaks > > when two symbols with the same name are located in different > > sections. > > > > Since a while, LD has a flag `-z unique-symbol` which appends > > numeric suffixes to the functions with the same name (in symtab > > and strtab). It can be used to effectively prevent from having > > any ambiguity when referring to a symbol by its name. > > In the patch description can you also give the version of binutils (and > possibly other linkers) which have the flag? GNU ld>=2.36 supports -z unique-symbol. ld.lld doesn't support -z unique-symbol. I subscribe to llvm@lists.linux.dev and happen to notice this message (can't keep up with the changes...) I am a bit concerned with this option and replied last time on https://lore.kernel.org/r/20220105032456.hs3od326sdl4zjv4@google.com My full reasoning is on https://maskray.me/blog/2020-11-15-explain-gnu-linker-options#z-unique-symbol > > Check for its availability and always prefer when the livepatching > > is on. It can be used unconditionally later on after broader testing > > on a wide variety of machines, but for now let's stick to the actual > > CONFIG_LIVEPATCH=y case, which is true for most of distro configs > > anyways. > > Has anybody objected to just enabling it for *all* configs, not just for > livepatch? > > I'd much prefer that: the less "special" livepatch is (and the distros > which enable it), the better. And I think having unique symbols would > benefit some other components. > > > +++ b/kernel/livepatch/core.c > > @@ -143,11 +143,13 @@ static int klp_find_callback(void *data, const char *name, > > args->count++; > > > > /* > > - * Finish the search when the symbol is found for the desired position > > - * or the position is not defined for a non-unique symbol. > > + * Finish the search when unique symbol names are enabled > > + * or the symbol is found for the desired position or the > > + * position is not defined for a non-unique symbol. > > */ > > - if ((args->pos && (args->count == args->pos)) || > > - (!args->pos && (args->count > 1))) > > + if (IS_ENABLED(CONFIG_LD_HAS_Z_UNIQUE_SYMBOL) || > > + (args->pos && args->count == args->pos) || > > + (!args->pos && args->count > 1)) > > return 1; > > There's no real need to do this. The code already works as-is, even if > there are no unique symbols. > > Even if there are no duplicates, there's little harm in going through > all the symbols anyway, to check for errors just in case something > unexpected happened with the linking (unexpected duplicate) or the patch > creation (unexpected sympos). It's not a hot path, so performance isn't > really a concern. > > When the old linker versions eventually age out, we can then go strip > out all the sympos stuff. > > > @@ -169,6 +171,13 @@ static int klp_find_object_symbol(const char *objname, const char *name, > > else > > kallsyms_on_each_symbol(klp_find_callback, &args); > > > > + /* > > + * If the LD's `-z unique-symbol` flag is available and enabled, > > + * sympos checks are not relevant. > > + */ > > + if (IS_ENABLED(CONFIG_LD_HAS_Z_UNIQUE_SYMBOL)) > > + sympos = 0; > > + > > Similarly, I don't see a need for this. If the patch is legit then > sympos should already be zero. If not, an error gets reported and the > patch fails to load. > > -- > Josh > >
On Fri, Feb 11, 2022 at 10:05:02AM -0800, Fāng-ruì Sòng wrote: > On Fri, Feb 11, 2022 at 9:41 AM Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > > > On Wed, Feb 09, 2022 at 07:57:39PM +0100, Alexander Lobakin wrote: > > > Position-based search, which means that if there are several symbols > > > with the same name, the user needs to additionally provide the > > > "index" of a desired symbol, is fragile. For example, it breaks > > > when two symbols with the same name are located in different > > > sections. > > > > > > Since a while, LD has a flag `-z unique-symbol` which appends > > > numeric suffixes to the functions with the same name (in symtab > > > and strtab). It can be used to effectively prevent from having > > > any ambiguity when referring to a symbol by its name. > > > > In the patch description can you also give the version of binutils (and > > possibly other linkers) which have the flag? > > GNU ld>=2.36 supports -z unique-symbol. ld.lld doesn't support -z unique-symbol. > > I subscribe to llvm@lists.linux.dev and happen to notice this message > (can't keep up with the changes...) > I am a bit concerned with this option and replied last time on > https://lore.kernel.org/r/20220105032456.hs3od326sdl4zjv4@google.com > > My full reasoning is on > https://maskray.me/blog/2020-11-15-explain-gnu-linker-options#z-unique-symbol Ah, right. Also discussed here: https://lore.kernel.org/all/20210123225928.z5hkmaw6qjs2gu5g@google.com/T/#u https://lore.kernel.org/all/20210125172124.awabevkpvq4poqxf@treble/ I'm not qualified to comment on LTO/PGO stability issues, but it doesn't sound good. And we want to support livepatch for LTO kernels. Also I realized that this flag would have a negative effect on kpatch-build, as it currently does its analysis on .o files. So it would have to figure out how to properly detect function renames, to avoid patching the wrong function for example. And if LLD doesn't plan to support the flag then it will be a headache for livepatch (and the kernel in general) to deal with the divergent configs. One idea I mentioned before, it may be worth exploring changing the "F" in FGKASLR to "File" instead of "Function". In other words, only shuffle at an object-file granularity. Then, even with duplicates, the <file+function> symbol pair doesn't change in the symbol table. And as a bonus, it should help FGKASLR i-cache performance, significantly.
From: Josh Poimboeuf <jpoimboe@redhat.com> Date: Fri, 11 Feb 2022 09:41:30 -0800 > On Wed, Feb 09, 2022 at 07:57:39PM +0100, Alexander Lobakin wrote: > > Position-based search, which means that if there are several symbols > > with the same name, the user needs to additionally provide the > > "index" of a desired symbol, is fragile. For example, it breaks > > when two symbols with the same name are located in different > > sections. > > > > Since a while, LD has a flag `-z unique-symbol` which appends > > numeric suffixes to the functions with the same name (in symtab > > and strtab). It can be used to effectively prevent from having > > any ambiguity when referring to a symbol by its name. > > In the patch description can you also give the version of binutils (and > possibly other linkers) which have the flag? Yeah, sure. > > > Check for its availability and always prefer when the livepatching > > is on. It can be used unconditionally later on after broader testing > > on a wide variety of machines, but for now let's stick to the actual > > CONFIG_LIVEPATCH=y case, which is true for most of distro configs > > anyways. > > Has anybody objected to just enabling it for *all* configs, not just for > livepatch? A few folks previously. > > I'd much prefer that: the less "special" livepatch is (and the distros > which enable it), the better. And I think having unique symbols would > benefit some other components. Agree, I just want this series to be as least invasive for non-FG-KASLR builds as possible. And currently this flag make depmod emit a bunch of harmless false-positive warnings, so I'd wait until at least the series is accepted / I post a patch for depmod and it gets accepted. > > > +++ b/kernel/livepatch/core.c > > @@ -143,11 +143,13 @@ static int klp_find_callback(void *data, const char *name, > > args->count++; > > > > /* > > - * Finish the search when the symbol is found for the desired position > > - * or the position is not defined for a non-unique symbol. > > + * Finish the search when unique symbol names are enabled > > + * or the symbol is found for the desired position or the > > + * position is not defined for a non-unique symbol. > > */ > > - if ((args->pos && (args->count == args->pos)) || > > - (!args->pos && (args->count > 1))) > > + if (IS_ENABLED(CONFIG_LD_HAS_Z_UNIQUE_SYMBOL) || > > + (args->pos && args->count == args->pos) || > > + (!args->pos && args->count > 1)) > > return 1; > > There's no real need to do this. The code already works as-is, even if > there are no unique symbols. > > Even if there are no duplicates, there's little harm in going through > all the symbols anyway, to check for errors just in case something > unexpected happened with the linking (unexpected duplicate) or the patch > creation (unexpected sympos). It's not a hot path, so performance isn't > really a concern. > > When the old linker versions eventually age out, we can then go strip > out all the sympos stuff. > > > @@ -169,6 +171,13 @@ static int klp_find_object_symbol(const char *objname, const char *name, > > else > > kallsyms_on_each_symbol(klp_find_callback, &args); > > > > + /* > > + * If the LD's `-z unique-symbol` flag is available and enabled, > > + * sympos checks are not relevant. > > + */ > > + if (IS_ENABLED(CONFIG_LD_HAS_Z_UNIQUE_SYMBOL)) > > + sympos = 0; > > + > > Similarly, I don't see a need for this. If the patch is legit then > sympos should already be zero. If not, an error gets reported and the > patch fails to load. Right, but for both those chunks the main idea is to let the compiler optimize-out the code non-actual for unique-symbol builds: add/remove: 0/0 grow/shrink: 1/2 up/down: 3/-80 (-77) Function old new delta klp_find_callback 139 142 +3 klp_find_object_symbol.cold 85 48 -37 klp_find_object_symbol 168 125 -43 > > -- > Josh Thanks, Al
From: Josh Poimboeuf <jpoimboe@redhat.com> Date: Fri, 11 Feb 2022 10:35:29 -0800 > On Fri, Feb 11, 2022 at 10:05:02AM -0800, Fāng-ruì Sòng wrote: > > On Fri, Feb 11, 2022 at 9:41 AM Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > > > > > On Wed, Feb 09, 2022 at 07:57:39PM +0100, Alexander Lobakin wrote: > > > > Position-based search, which means that if there are several symbols > > > > with the same name, the user needs to additionally provide the > > > > "index" of a desired symbol, is fragile. For example, it breaks > > > > when two symbols with the same name are located in different > > > > sections. > > > > > > > > Since a while, LD has a flag `-z unique-symbol` which appends > > > > numeric suffixes to the functions with the same name (in symtab > > > > and strtab). It can be used to effectively prevent from having > > > > any ambiguity when referring to a symbol by its name. > > > > > > In the patch description can you also give the version of binutils (and > > > possibly other linkers) which have the flag? > > > > GNU ld>=2.36 supports -z unique-symbol. ld.lld doesn't support -z unique-symbol. > > > > I subscribe to llvm@lists.linux.dev and happen to notice this message > > (can't keep up with the changes...) > > I am a bit concerned with this option and replied last time on > > https://lore.kernel.org/r/20220105032456.hs3od326sdl4zjv4@google.com > > > > My full reasoning is on > > https://maskray.me/blog/2020-11-15-explain-gnu-linker-options#z-unique-symbol > > Ah, right. Also discussed here: > > https://lore.kernel.org/all/20210123225928.z5hkmaw6qjs2gu5g@google.com/T/#u > https://lore.kernel.org/all/20210125172124.awabevkpvq4poqxf@treble/ > > I'm not qualified to comment on LTO/PGO stability issues, but it doesn't > sound good. And we want to support livepatch for LTO kernels. > > Also I realized that this flag would have a negative effect on > kpatch-build, as it currently does its analysis on .o files. So it > would have to figure out how to properly detect function renames, to > avoid patching the wrong function for example. > > And if LLD doesn't plan to support the flag then it will be a headache > for livepatch (and the kernel in general) to deal with the divergent > configs. I'm always down with replacing any of the parts, I'm just not familiar with any other ways of approaching this without huge diffs. I've read Fāng-ruì's blogpost previously and there's a possible replacement described there, but I dunno how to approach it. And them Miroslav just told me that unique-symbol should work just fine and I can go with it. So I asked here prevously and ask once again for any hints regarding some other ways :p > > One idea I mentioned before, it may be worth exploring changing the "F" > in FGKASLR to "File" instead of "Function". In other words, only > shuffle at an object-file granularity. Then, even with duplicates, the > <file+function> symbol pair doesn't change in the symbol table. And as > a bonus, it should help FGKASLR i-cache performance, significantly. Yeah, I keep that in mind. However, this wouldn't solve the duplicate static function names problem, right? Let's say you have a static function f() in file1 and f() in file2, then the layout each boot can be .text.file1 or .text.file2 f() f() .text.file2 .text.file1 f() f() and position-based search won't work anyway, right? > > -- > Josh Thanks, Al
On Mon, Feb 14, 2022 at 01:24:33PM +0100, Alexander Lobakin wrote: > > One idea I mentioned before, it may be worth exploring changing the "F" > > in FGKASLR to "File" instead of "Function". In other words, only > > shuffle at an object-file granularity. Then, even with duplicates, the > > <file+function> symbol pair doesn't change in the symbol table. And as > > a bonus, it should help FGKASLR i-cache performance, significantly. > > Yeah, I keep that in mind. However, this wouldn't solve the > duplicate static function names problem, right? > Let's say you have a static function f() in file1 and f() in file2, > then the layout each boot can be > > .text.file1 or .text.file2 > f() f() > .text.file2 .text.file1 > f() f() > > and position-based search won't work anyway, right? Right, so we'd have to abandon position-based search in favor of file+func based search. It's not perfect because there are still a few file+func duplicates. But it might be good enough. We would presumably just refuse to patch a duplicate. Or we could remove them (and enforce their continued removal with tooling-based warnings). Another variant of this which I described here https://lore.kernel.org/all/20210125172124.awabevkpvq4poqxf@treble/ would be to keep it function-granular, but have kallsyms keep track of what file each func belongs to. Then livepatch could still do the file+func based search.
NOTE: Maybe -zunique-symbol won't get used after all, based on maskray's objections. Regardless, I'm replying below, because the rest of the approach in this patch seems all wrong. On Mon, Feb 14, 2022 at 01:14:47PM +0100, Alexander Lobakin wrote: > From: Josh Poimboeuf <jpoimboe@redhat.com> > Date: Fri, 11 Feb 2022 09:41:30 -0800 > > > On Wed, Feb 09, 2022 at 07:57:39PM +0100, Alexander Lobakin wrote: > > > Position-based search, which means that if there are several symbols > > > with the same name, the user needs to additionally provide the > > > "index" of a desired symbol, is fragile. For example, it breaks > > > when two symbols with the same name are located in different > > > sections. > > > > > > Since a while, LD has a flag `-z unique-symbol` which appends > > > numeric suffixes to the functions with the same name (in symtab > > > and strtab). It can be used to effectively prevent from having > > > any ambiguity when referring to a symbol by its name. > > > > In the patch description can you also give the version of binutils (and > > possibly other linkers) which have the flag? > > Yeah, sure. > > > Check for its availability and always prefer when the livepatching > > > is on. It can be used unconditionally later on after broader testing > > > on a wide variety of machines, but for now let's stick to the actual > > > CONFIG_LIVEPATCH=y case, which is true for most of distro configs > > > anyways. > > > > Has anybody objected to just enabling it for *all* configs, not just for > > livepatch? > > A few folks previously. Why? It would be good to document that here. > > I'd much prefer that: the less "special" livepatch is (and the distros > > which enable it), the better. And I think having unique symbols would > > benefit some other components. > > Agree, I just want this series to be as least invasive for > non-FG-KASLR builds as possible. But in a very real sense, this patch is making the series *more* invasive by complexifying the config space. Adding -zunique-symbols could have kernel-wide implications. If there were bugs, we'd want to root them out, not hide them behind obscure config combinations we hope nobody uses. Effectively this is destabilizing CONFIG_LIVEPATCH. Beyond "least invasive", we also need to consider: - What makes fgkaslr most compatible with other features? - What makes fgkaslr most palatable for wide use? - What's best for the kernel as a whole? It's much better to integrate new features properly with the kernel, rather than just grafting them on to the side. Otherwise it just adds technical debt, with no benefit to the rest of the kernel. Then it might as well just remain an out-of-tree patch set. > > > + if (IS_ENABLED(CONFIG_LD_HAS_Z_UNIQUE_SYMBOL)) > > > + sympos = 0; > > > + > > > > Similarly, I don't see a need for this. If the patch is legit then > > sympos should already be zero. If not, an error gets reported and the > > patch fails to load. > > Right, but for both those chunks the main idea is to let the > compiler optimize-out the code non-actual for unique-symbol builds: > > add/remove: 0/0 grow/shrink: 1/2 up/down: 3/-80 (-77) > Function old new delta > klp_find_callback 139 142 +3 > klp_find_object_symbol.cold 85 48 -37 > klp_find_object_symbol 168 125 -43 As I said, it's not a hot path, so there's no need to complicate the code with edge cases, and remove useful error checking in the process.
> > +++ b/kernel/livepatch/core.c > > @@ -143,11 +143,13 @@ static int klp_find_callback(void *data, const char *name, > > args->count++; > > > > /* > > - * Finish the search when the symbol is found for the desired position > > - * or the position is not defined for a non-unique symbol. > > + * Finish the search when unique symbol names are enabled > > + * or the symbol is found for the desired position or the > > + * position is not defined for a non-unique symbol. > > */ > > - if ((args->pos && (args->count == args->pos)) || > > - (!args->pos && (args->count > 1))) > > + if (IS_ENABLED(CONFIG_LD_HAS_Z_UNIQUE_SYMBOL) || > > + (args->pos && args->count == args->pos) || > > + (!args->pos && args->count > 1)) > > return 1; > > There's no real need to do this. The code already works as-is, even if > there are no unique symbols. > > Even if there are no duplicates, there's little harm in going through > all the symbols anyway, to check for errors just in case something > unexpected happened with the linking (unexpected duplicate) or the patch > creation (unexpected sympos). It's not a hot path, so performance isn't > really a concern. Correct. > When the old linker versions eventually age out, we can then go strip > out all the sympos stuff. Yes. > > @@ -169,6 +171,13 @@ static int klp_find_object_symbol(const char *objname, const char *name, > > else > > kallsyms_on_each_symbol(klp_find_callback, &args); > > > > + /* > > + * If the LD's `-z unique-symbol` flag is available and enabled, > > + * sympos checks are not relevant. > > + */ > > + if (IS_ENABLED(CONFIG_LD_HAS_Z_UNIQUE_SYMBOL)) > > + sympos = 0; > > + > > Similarly, I don't see a need for this. If the patch is legit then > sympos should already be zero. If not, an error gets reported and the > patch fails to load. My concern was that if the patch is not legit (that is, sympos is > 0 for some reason), the error would be really cryptic and would not help the user at all. So zeroing sympos seems to be a good idea to me. There is no harm and the change is very small and compact. On the other hand, I do not insist on this. Regards, Miroslav
On Fri, 11 Feb 2022, Josh Poimboeuf wrote: > On Fri, Feb 11, 2022 at 10:05:02AM -0800, Fāng-ruì Sòng wrote: > > On Fri, Feb 11, 2022 at 9:41 AM Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > > > > > On Wed, Feb 09, 2022 at 07:57:39PM +0100, Alexander Lobakin wrote: > > > > Position-based search, which means that if there are several symbols > > > > with the same name, the user needs to additionally provide the > > > > "index" of a desired symbol, is fragile. For example, it breaks > > > > when two symbols with the same name are located in different > > > > sections. > > > > > > > > Since a while, LD has a flag `-z unique-symbol` which appends > > > > numeric suffixes to the functions with the same name (in symtab > > > > and strtab). It can be used to effectively prevent from having > > > > any ambiguity when referring to a symbol by its name. > > > > > > In the patch description can you also give the version of binutils (and > > > possibly other linkers) which have the flag? > > > > GNU ld>=2.36 supports -z unique-symbol. ld.lld doesn't support -z unique-symbol. > > > > I subscribe to llvm@lists.linux.dev and happen to notice this message > > (can't keep up with the changes...) > > I am a bit concerned with this option and replied last time on > > https://lore.kernel.org/r/20220105032456.hs3od326sdl4zjv4@google.com > > > > My full reasoning is on > > https://maskray.me/blog/2020-11-15-explain-gnu-linker-options#z-unique-symbol > > Ah, right. Also discussed here: > > https://lore.kernel.org/all/20210123225928.z5hkmaw6qjs2gu5g@google.com/T/#u > https://lore.kernel.org/all/20210125172124.awabevkpvq4poqxf@treble/ > > I'm not qualified to comment on LTO/PGO stability issues, but it doesn't > sound good. And we want to support livepatch for LTO kernels. Hm, bear with me, because I am very likely missing something which is clear to everyone else... Is the stability really a problem for the live patching (and I am talking about the live patching only here. It may be a problem elsewhere, but I am just trying to understand.)? I understand that two different kernel builds could have a different name mapping between the original symbols and their unique renames. Not nice. But we can prepare two different live patches for these two different kernels. Something one would like to avoid if possible, but it is not impossible. Am I missing something? > Also I realized that this flag would have a negative effect on > kpatch-build, as it currently does its analysis on .o files. So it > would have to figure out how to properly detect function renames, to > avoid patching the wrong function for example. Yes, that is unfortunate. And not only for kpatch-build. > And if LLD doesn't plan to support the flag then it will be a headache > for livepatch (and the kernel in general) to deal with the divergent > configs. True. The position-based approach clearly shows its limits. I like <file+func> approach based on kallsyms tracking, that you proposed elsewhere in the thread, more. Miroslav
On Wed, Feb 16, 2022 at 04:06:24PM +0100, Miroslav Benes wrote: > > > + /* > > > + * If the LD's `-z unique-symbol` flag is available and enabled, > > > + * sympos checks are not relevant. > > > + */ > > > + if (IS_ENABLED(CONFIG_LD_HAS_Z_UNIQUE_SYMBOL)) > > > + sympos = 0; > > > + > > > > Similarly, I don't see a need for this. If the patch is legit then > > sympos should already be zero. If not, an error gets reported and the > > patch fails to load. > > My concern was that if the patch is not legit (that is, sympos is > 0 for > some reason), the error would be really cryptic and would not help the > user at all. So zeroing sympos seems to be a good idea to me. There is no > harm and the change is very small and compact. But wouldn't a cryptic error be better than no error at all? A bad sympos might be indicative of some larger issue, like the wrong symbol getting patched.
On Wed, Feb 16, 2022 at 04:15:20PM +0100, Miroslav Benes wrote: > > > I subscribe to llvm@lists.linux.dev and happen to notice this message > > > (can't keep up with the changes...) > > > I am a bit concerned with this option and replied last time on > > > https://lore.kernel.org/r/20220105032456.hs3od326sdl4zjv4@google.com > > > > > > My full reasoning is on > > > https://maskray.me/blog/2020-11-15-explain-gnu-linker-options#z-unique-symbol > > > > Ah, right. Also discussed here: > > > > https://lore.kernel.org/all/20210123225928.z5hkmaw6qjs2gu5g@google.com/T/#u > > https://lore.kernel.org/all/20210125172124.awabevkpvq4poqxf@treble/ > > > > I'm not qualified to comment on LTO/PGO stability issues, but it doesn't > > sound good. And we want to support livepatch for LTO kernels. > > Hm, bear with me, because I am very likely missing something which is > clear to everyone else... > > Is the stability really a problem for the live patching (and I am talking > about the live patching only here. It may be a problem elsewhere, but I am > just trying to understand.)? I understand that two different kernel builds > could have a different name mapping between the original symbols and their > unique renames. Not nice. But we can prepare two different live patches > for these two different kernels. Something one would like to avoid if > possible, but it is not impossible. Am I missing something? Maybe Fāng-ruì can clarify, but my understanding was that the stability issue affects the kernel in general (particularly if LTO or PGO is enabled) and isn't necessarily specific to livepatch itself.
On Mon, Feb 14, 2022 at 10:10:00AM -0800, Josh Poimboeuf wrote: > On Mon, Feb 14, 2022 at 01:24:33PM +0100, Alexander Lobakin wrote: > > > One idea I mentioned before, it may be worth exploring changing the "F" > > > in FGKASLR to "File" instead of "Function". In other words, only > > > shuffle at an object-file granularity. Then, even with duplicates, the > > > <file+function> symbol pair doesn't change in the symbol table. And as > > > a bonus, it should help FGKASLR i-cache performance, significantly. > > > > Yeah, I keep that in mind. However, this wouldn't solve the > > duplicate static function names problem, right? > > Let's say you have a static function f() in file1 and f() in file2, > > then the layout each boot can be > > > > .text.file1 or .text.file2 > > f() f() > > .text.file2 .text.file1 > > f() f() > > > > and position-based search won't work anyway, right? > > Right, so we'd have to abandon position-based search in favor of > file+func based search. > > It's not perfect because there are still a few file+func duplicates. > But it might be good enough. We would presumably just refuse to patch a > duplicate. Or we could remove them (and enforce their continued removal > with tooling-based warnings). > You're talking about duplicate file+func combinations as stored in the symbol table? From a recent rhel-9 development kernel: $ readelf --wide --symbols vmlinux | \ awk '$4=="FILE" { f=$NF } $4=="OBJECT" || $4=="FUNC" { print $4 " " f "::" $NF }' | \ sort | uniq -c | sort -n | awk '$1 != 1' 2 FUNC bus.c::new_id_store 2 FUNC core.c::native_read_msr 2 FUNC core.c::type_show 2 FUNC diag.c::sk_diag_fill.constprop.0 2 FUNC hid-core.c::hid_exit 2 FUNC hid-core.c::hid_init 2 FUNC inode.c::remove_one 2 FUNC iommu.c::__list_del_entry 2 FUNC msr.c::msr_init 2 FUNC msr.c::msr_read 2 FUNC proc.c::c_next 2 FUNC proc.c::c_start 2 FUNC proc.c::c_stop 2 FUNC raw.c::copy_overflow 2 FUNC raw.c::dst_output 2 FUNC route.c::dst_discard 2 FUNC sysfs.c::name_show 2 FUNC udp.c::copy_overflow 2 FUNC udp.c::udp_lib_close 2 FUNC udp.c::udp_lib_hash 2 FUNC udp.c::udplite_getfrag 2 FUNC udplite.c::udp_lib_close 2 FUNC udplite.c::udp_lib_hash 2 FUNC udplite.c::udplite_sk_init 2 OBJECT acpi.c::__func__.0 2 OBJECT amd.c::__already_done.10 2 OBJECT amd.c::__func__.4 2 OBJECT amd.c::__func__.5 2 OBJECT bus.c::driver_attr_new_id 2 OBJECT bus.c::__func__.1 2 OBJECT bus.c::__func__.2 2 OBJECT bus.c::__func__.3 2 OBJECT bus.c::__func__.4 2 OBJECT bus.c::__func__.5 2 OBJECT bus.c::__func__.6 2 OBJECT bus.c::__func__.7 2 OBJECT bus.c::__func__.8 2 OBJECT bus.c::__func__.9 2 OBJECT cgroup.c::__func__.0 2 OBJECT class.c::__func__.0 2 OBJECT class.c::__func__.1 2 OBJECT class.c::__func__.3 2 OBJECT class.c::__func__.5 2 OBJECT class.c::__key.0 2 OBJECT class.c::__key.1 2 OBJECT class.c::__key.4 2 OBJECT core.c::__already_done.18 2 OBJECT core.c::__already_done.19 2 OBJECT core.c::__already_done.3 2 OBJECT core.c::dev_attr_size 2 OBJECT core.c::dev_attr_start 2 OBJECT core.c::dev_attr_type 2 OBJECT core.c::empty_attrs 2 OBJECT core.c::__func__.10 2 OBJECT core.c::__func__.14 2 OBJECT core.c::__func__.7 2 OBJECT core.c::__func__.9 2 OBJECT core.c::__key.0 2 OBJECT core.c::__key.2 2 OBJECT core.c::__key.3 2 OBJECT dev.c::__func__.0 2 OBJECT dir.c::__func__.3 2 OBJECT driver.c::__func__.0 2 OBJECT fib_rules.c::__msg.0 2 OBJECT file.c::__func__.2 2 OBJECT file.c::__key.1 2 OBJECT file.c::__key.2 2 OBJECT hpet.c::__func__.4 2 OBJECT icmp.c::__func__.1 2 OBJECT inode.c::__func__.1 2 OBJECT inode.c::__func__.3 2 OBJECT intel.c::__already_done.10 2 OBJECT intel.c::__already_done.11 2 OBJECT intel.c::__already_done.13 2 OBJECT ioctl.c::__func__.0 2 OBJECT iommu.c::__already_done.15 2 OBJECT iommu.c::__func__.10 2 OBJECT iommu.c::__func__.2 2 OBJECT iommu.c::_rs.13 2 OBJECT iommu.c::_rs.5 2 OBJECT iommu.c::_rs.9 2 OBJECT irq.c::__func__.0 2 OBJECT irq.c::__func__.2 2 OBJECT irqdomain.c::__func__.0 2 OBJECT irqdomain.c::__func__.1 2 OBJECT irqdomain.c::__func__.3 2 OBJECT main.c::__func__.10 2 OBJECT main.c::__func__.11 2 OBJECT main.c::__func__.3 2 OBJECT main.c::__func__.4 2 OBJECT main.c::__func__.5 2 OBJECT manage.c::__func__.1 2 OBJECT mount.c::__func__.0 2 OBJECT msr.c::__func__.0 2 OBJECT ping.c::__func__.1 2 OBJECT property.c::__func__.3 2 OBJECT qos.c::__func__.0 2 OBJECT qos.c::__func__.2 2 OBJECT resource.c::__func__.1 2 OBJECT route.c::__key.0 2 OBJECT route.c::__msg.1 2 OBJECT route.c::__msg.2 2 OBJECT route.c::__msg.3 2 OBJECT route.c::__msg.4 2 OBJECT route.c::__msg.5 2 OBJECT route.c::__msg.6 2 OBJECT swap.c::__func__.0 2 OBJECT syncookies.c::___done.1 2 OBJECT syncookies.c::msstab 2 OBJECT syncookies.c::___once_key.2 2 OBJECT sysfs.c::dev_attr_name 2 OBJECT sysfs.c::__key.1 2 OBJECT sysfs.c::power_attrs 2 OBJECT udp.c::descriptor.12 2 OBJECT udp.c::descriptor.13 2 OBJECT udp.c::__func__.2 2 OBJECT udp.c::__func__.3 2 OBJECT udp.c::__func__.4 2 OBJECT utils.c::__func__.5 3 FUNC core.c::cmask_show 3 FUNC core.c::edge_show 3 FUNC core.c::event_show 3 FUNC core.c::inv_show 3 FUNC core.c::umask_show 3 FUNC inode.c::init_once 3 OBJECT acpi.c::__func__.1 3 OBJECT core.c::format_attr_cmask 3 OBJECT core.c::format_attr_edge 3 OBJECT core.c::format_attr_event 3 OBJECT core.c::format_attr_inv 3 OBJECT core.c::format_attr_umask 3 OBJECT core.c::__func__.6 3 OBJECT core.c::__func__.8 3 OBJECT file.c::__key.3 3 OBJECT generic.c::__func__.0 3 OBJECT iommu.c::__func__.0 3 OBJECT iommu.c::__func__.1 3 OBJECT iommu.c::__func__.8 3 OBJECT main.c::__func__.0 3 OBJECT main.c::__func__.1 3 OBJECT main.c::__func__.6 3 OBJECT quirks.c::__func__.0 3 OBJECT sysfs.c::__func__.0 4 OBJECT core.c::__func__.4 5 OBJECT inode.c::tokens 6 OBJECT core.c::__func__.3 6 OBJECT core.c::__func__.5 7 OBJECT core.c::__func__.1 8 OBJECT core.c::__func__.0 8 OBJECT core.c::__func__.2 We could probably minimize the FUNC duplicates with unique names, but I'm not as optimistic about the OBJECTs as most are created via macros like __already_done.X. Unless clever macro magic? Next question: what are the odds that these entries, at least the ones we can't easily rename, need disambiguity for livepatching? or kpatch-build for related purposes? -- Joe
On Wed, Feb 16, 2022 at 03:32:41PM -0500, Joe Lawrence wrote: > > Right, so we'd have to abandon position-based search in favor of > > file+func based search. > > > > It's not perfect because there are still a few file+func duplicates. > > But it might be good enough. We would presumably just refuse to patch a > > duplicate. Or we could remove them (and enforce their continued removal > > with tooling-based warnings). > > > > You're talking about duplicate file+func combinations as stored in the > symbol table? Right. > ... > 6 OBJECT core.c::__func__.3 > 6 OBJECT core.c::__func__.5 > 7 OBJECT core.c::__func__.1 > 8 OBJECT core.c::__func__.0 > 8 OBJECT core.c::__func__.2 > > We could probably minimize the FUNC duplicates with unique names, but > I'm not as optimistic about the OBJECTs as most are created via macros > like __already_done.X. Unless clever macro magic? Good point about objects, as we rely on disambiguating them for klp relocations. Luckily, the fact that most of them are created by macros is largely a good thing. We consider most of those to be "special" static locals, which don't actually need to be correlated or referenced with a klp reloc. For example: - '__func__' is just the function name. The patched function shouldn't need to reference the original function's function name string. - '__already_done' is used for printk_once(); no harm in making a new variable initialized to false and printing it again; or converting printk_once() to just printk() to avoid an extra print. - '__key' is used by lockdep to track lock usage and validate locking order. It probably makes sense to use a new key in the patched function, since the new function might have different locking behavior. > Next question: what are the odds that these entries, at least the ones > we can't easily rename, need disambiguity for livepatching? or > kpatch-build for related purposes? I would guess the odds are rather low, given the fact that there are so few functions, and we don't care about most of the objects on the list. If duplicates were to become problematic then we could consider adding tooling which warns on a duplicate file:sym pair with the goal of eliminating duplicates (exculding the "special" objects).
On Wed, 16 Feb 2022, Josh Poimboeuf wrote: > On Wed, Feb 16, 2022 at 04:06:24PM +0100, Miroslav Benes wrote: > > > > + /* > > > > + * If the LD's `-z unique-symbol` flag is available and enabled, > > > > + * sympos checks are not relevant. > > > > + */ > > > > + if (IS_ENABLED(CONFIG_LD_HAS_Z_UNIQUE_SYMBOL)) > > > > + sympos = 0; > > > > + > > > > > > Similarly, I don't see a need for this. If the patch is legit then > > > sympos should already be zero. If not, an error gets reported and the > > > patch fails to load. > > > > My concern was that if the patch is not legit (that is, sympos is > 0 for > > some reason), the error would be really cryptic and would not help the > > user at all. So zeroing sympos seems to be a good idea to me. There is no > > harm and the change is very small and compact. > > But wouldn't a cryptic error be better than no error at all? A bad > sympos might be indicative of some larger issue, like the wrong symbol > getting patched. Maybe you are right. I do not feel confident enough to decide it. So either way would be fine, I guess. Miroslav
From: Miroslav Benes <mbenes@suse.cz> Date: Wed, 16 Feb 2022 16:15:20 +0100 (CET) > On Fri, 11 Feb 2022, Josh Poimboeuf wrote: > > > On Fri, Feb 11, 2022 at 10:05:02AM -0800, Fāng-ruì Sòng wrote: > > > On Fri, Feb 11, 2022 at 9:41 AM Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > > > > > > > On Wed, Feb 09, 2022 at 07:57:39PM +0100, Alexander Lobakin wrote: > > > > > Position-based search, which means that if there are several symbols > > > > > with the same name, the user needs to additionally provide the > > > > > "index" of a desired symbol, is fragile. For example, it breaks > > > > > when two symbols with the same name are located in different > > > > > sections. > > > > > > > > > > Since a while, LD has a flag `-z unique-symbol` which appends > > > > > numeric suffixes to the functions with the same name (in symtab > > > > > and strtab). It can be used to effectively prevent from having > > > > > any ambiguity when referring to a symbol by its name. > > > > > > > > In the patch description can you also give the version of binutils (and > > > > possibly other linkers) which have the flag? > > > > > > GNU ld>=2.36 supports -z unique-symbol. ld.lld doesn't support -z unique-symbol. > > > > > > I subscribe to llvm@lists.linux.dev and happen to notice this message > > > (can't keep up with the changes...) > > > I am a bit concerned with this option and replied last time on > > > https://lore.kernel.org/r/20220105032456.hs3od326sdl4zjv4@google.com > > > > > > My full reasoning is on > > > https://maskray.me/blog/2020-11-15-explain-gnu-linker-options#z-unique-symbol > > > > Ah, right. Also discussed here: > > > > https://lore.kernel.org/all/20210123225928.z5hkmaw6qjs2gu5g@google.com/T/#u > > https://lore.kernel.org/all/20210125172124.awabevkpvq4poqxf@treble/ > > > > I'm not qualified to comment on LTO/PGO stability issues, but it doesn't > > sound good. And we want to support livepatch for LTO kernels. > > Hm, bear with me, because I am very likely missing something which is > clear to everyone else... > > Is the stability really a problem for the live patching (and I am talking > about the live patching only here. It may be a problem elsewhere, but I am > just trying to understand.)? I understand that two different kernel builds > could have a different name mapping between the original symbols and their > unique renames. Not nice. But we can prepare two different live patches > for these two different kernels. Something one would like to avoid if > possible, but it is not impossible. Am I missing something? > > > Also I realized that this flag would have a negative effect on > > kpatch-build, as it currently does its analysis on .o files. So it > > would have to figure out how to properly detect function renames, to > > avoid patching the wrong function for example. > > Yes, that is unfortunate. And not only for kpatch-build. > > > And if LLD doesn't plan to support the flag then it will be a headache > > for livepatch (and the kernel in general) to deal with the divergent > > configs. > > True. > > The position-based approach clearly shows its limits. I like <file+func> > approach based on kallsyms tracking, that you proposed elsewhere in the > thread, more. Hmm, same. For FG-KASLR part, `-ffunction-sections` has no options, it only appends the function name to the name of a function, i.e. it can be only ".text.dup". However, LD scripts allow to specify a particular input file for the section being described, i.e.: .text.dup { .text.file1_dup { (.text.dup) -> file1.o(.text.dup) } } .text.file2_dup { file2.o(.text.dup) } But the problem is that currently vmlinux is being linked from vmlinux.o solely, so there are no input files apart from vmlinux.o. I could probably (not 100% sure, I'm not deep into the details of thin archives) create a temporary linker script for vmlinux.o itself to process duplicates. Then vmlinux.o will always have only unique section names right from the start. It may not worth it: I don't mind that random functions with the same name go into one section, it's not a big deal and/or security risk, and it doesn't help livepatch which operates with symbol names, not sections. Re livepatch, the best option would probably be storing relative paths to the object files in kallsyms. By relative I mean starting from $srctree -- this would keep their versatility (no abspaths), but provide needed uniquity: dup() main.o:dup() init/main.o:dup() /mnt/init/main.o:dup() dup() main.o:dup() foo/bar/main.o:dup() /mnt/foo/bar/main.o:dup() ^^^^^^ here ^^^^^^ The problem is that kallsyms are being generated at the moment of (re)linking vmlinux already and no earlier. If I could catch STT_FILE (can't say for sure now), it would provide only filenames, so wouldn't be enough. ...oh wait, kallsyms rely on `nm` output. I checked nm's `-l` which tries to find a file corresponding to each symbol and got a nice output: ffffffff8109ad00 T switch_mm_irqs_off /home/alobakin/Documents/work/xdp_hints/linux/arch/x86/mm/tlb.c:488 So this could be parsed with no issues nto: name: switch_mm_irqs_off addr: 0x9ad00 (rel) file: arch/x86/mm/tlb.c This solves a lot. One problem is that > time nm -ln vmlinux > ~/Documents/tmp/nml nm -ln vmlinux > ~/Documents/tmp/nml 120.80s user 1.77s system 99% cpu 2:02.94 total it took 2 minutes to generate the whole map (instead of a split second) (on 64-core CPU, but I guess nm runs in one thread). I guess it can be optimized? I'm no a binutils master (will take a look after sending this), is there a way to do it manually skipping this nm lag or maybe make nm emit filenames without such delays? > > Miroslav Thanks, Al
On Fri, Feb 18, 2022 at 05:31:11PM +0100, Alexander Lobakin wrote: > it took 2 minutes to generate the whole map (instead of a split > second) (on 64-core CPU, but I guess nm runs in one thread). > I guess it can be optimized? I'm no a binutils master (will take a > look after sending this), is there a way to do it manually skipping > this nm lag or maybe make nm emit filenames without such delays? Hm, yeah, adding 2 minutes to the link time isn't going to fly ;-) It probably takes a while to parse all the DWARF. Based on ther other discussions I think just using the basename (main.o) in STT_FILE would be good enough. Some duplicates are probably ok.
diff --git a/Makefile b/Makefile index ceb987e5c87b..fa9f947c9839 100644 --- a/Makefile +++ b/Makefile @@ -871,6 +871,12 @@ ifdef CONFIG_DEBUG_SECTION_MISMATCH KBUILD_CFLAGS += -fno-inline-functions-called-once endif +# Prefer linking with the `-z unique-symbol` if available, this eliminates +# position-based search +ifeq ($(CONFIG_LD_HAS_Z_UNIQUE_SYMBOL)$(CONFIG_LIVEPATCH),yy) +KBUILD_LDFLAGS += -z unique-symbol +endif + ifdef CONFIG_LD_DEAD_CODE_DATA_ELIMINATION KBUILD_CFLAGS_KERNEL += -ffunction-sections -fdata-sections LDFLAGS_vmlinux += --gc-sections diff --git a/init/Kconfig b/init/Kconfig index e9119bf54b1f..8e900d17d42b 100644 --- a/init/Kconfig +++ b/init/Kconfig @@ -86,6 +86,9 @@ config CC_HAS_ASM_INLINE config CC_HAS_NO_PROFILE_FN_ATTR def_bool $(success,echo '__attribute__((no_profile_instrument_function)) int x();' | $(CC) -x c - -c -o /dev/null -Werror) +config LD_HAS_Z_UNIQUE_SYMBOL + def_bool $(ld-option,-z unique-symbol) + config CONSTRUCTORS bool diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c index 585494ec464f..7a330465a8c7 100644 --- a/kernel/livepatch/core.c +++ b/kernel/livepatch/core.c @@ -143,11 +143,13 @@ static int klp_find_callback(void *data, const char *name, args->count++; /* - * Finish the search when the symbol is found for the desired position - * or the position is not defined for a non-unique symbol. + * Finish the search when unique symbol names are enabled + * or the symbol is found for the desired position or the + * position is not defined for a non-unique symbol. */ - if ((args->pos && (args->count == args->pos)) || - (!args->pos && (args->count > 1))) + if (IS_ENABLED(CONFIG_LD_HAS_Z_UNIQUE_SYMBOL) || + (args->pos && args->count == args->pos) || + (!args->pos && args->count > 1)) return 1; return 0; @@ -169,6 +171,13 @@ static int klp_find_object_symbol(const char *objname, const char *name, else kallsyms_on_each_symbol(klp_find_callback, &args); + /* + * If the LD's `-z unique-symbol` flag is available and enabled, + * sympos checks are not relevant. + */ + if (IS_ENABLED(CONFIG_LD_HAS_Z_UNIQUE_SYMBOL)) + sympos = 0; + /* * Ensure an address was found. If sympos is 0, ensure symbol is unique; * otherwise ensure the symbol position count matches sympos. diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c index 4648b7afe5cc..ec521ccebea6 100644 --- a/scripts/mod/modpost.c +++ b/scripts/mod/modpost.c @@ -689,11 +689,28 @@ static void handle_modversion(const struct module *mod, sym_set_crc(symname, crc); } +static char *remove_dot(char *s) +{ + size_t n = strcspn(s, "."); + + if (n && s[n]) { + size_t m = strspn(s + n + 1, "0123456789"); + + if (m && (s[n + m + 1] == '.' || s[n + m + 1] == 0)) + s[n] = 0; + + /* strip trailing .lto */ + if (strends(s, ".lto")) + s[strlen(s) - 4] = '\0'; + } + + return s; +} + static void handle_symbol(struct module *mod, struct elf_info *info, const Elf_Sym *sym, const char *symname) { enum export export; - const char *name; if (strstarts(symname, "__ksymtab")) export = export_from_secname(info, get_secindex(info, sym)); @@ -734,8 +751,11 @@ static void handle_symbol(struct module *mod, struct elf_info *info, default: /* All exported symbols */ if (strstarts(symname, "__ksymtab_")) { - name = symname + strlen("__ksymtab_"); - sym_add_exported(name, mod, export); + char *name; + + name = NOFAIL(strdup(symname + strlen("__ksymtab_"))); + sym_add_exported(remove_dot(name), mod, export); + free(name); } if (strcmp(symname, "init_module") == 0) mod->has_init = 1; @@ -1980,22 +2000,6 @@ static void check_sec_ref(struct module *mod, const char *modname, } } -static char *remove_dot(char *s) -{ - size_t n = strcspn(s, "."); - - if (n && s[n]) { - size_t m = strspn(s + n + 1, "0123456789"); - if (m && (s[n + m + 1] == '.' || s[n + m + 1] == 0)) - s[n] = 0; - - /* strip trailing .lto */ - if (strends(s, ".lto")) - s[strlen(s) - 4] = '\0'; - } - return s; -} - static void read_symbols(const char *modname) { const char *symname;
Position-based search, which means that if there are several symbols with the same name, the user needs to additionally provide the "index" of a desired symbol, is fragile. For example, it breaks when two symbols with the same name are located in different sections. Since a while, LD has a flag `-z unique-symbol` which appends numeric suffixes to the functions with the same name (in symtab and strtab). It can be used to effectively prevent from having any ambiguity when referring to a symbol by its name. Check for its availability and always prefer when the livepatching is on. It can be used unconditionally later on after broader testing on a wide variety of machines, but for now let's stick to the actual CONFIG_LIVEPATCH=y case, which is true for most of distro configs anyways. This needs a little adjustment to the modpost to make it strip suffixes before adding exports. depmod needs some treatment as well, tho its false-positive warnings about unknown symbols are harmless and don't alter the return code. There is probably a bunch more livepatch code to optimize-out after introducing this, leave it for later as well. Suggested-by: H.J. Lu <hjl.tools@gmail.com> Suggested-by: Peter Zijlstra <peterz@infradead.org> Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com> Suggested-by: Miroslav Benes <mbenes@suse.cz> Signed-off-by: Alexander Lobakin <alexandr.lobakin@intel.com> --- Makefile | 6 ++++++ init/Kconfig | 3 +++ kernel/livepatch/core.c | 17 +++++++++++++---- scripts/mod/modpost.c | 42 ++++++++++++++++++++++------------------- 4 files changed, 45 insertions(+), 23 deletions(-)