| Message ID | 20211223002209.1092165-2-alexandr.lobakin@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Function Granular KASLR | expand |
On Thu, Dec 23, 2021 at 01:21:55AM +0100, Alexander Lobakin wrote: > For now, that condition from remove_dot(): > > if (m && (s[n + m] == '.' || s[n + m] == 0)) > > which was designed to test if it's a dot or a \0 after the suffix > is never satisfied. > This is due to that s[n + m] always points to the last digit of a > numeric suffix, not on the symbol next to it: > > param_set_uint.0, s[n + m] is '0', s[n + m + 1] is '\0' > > So it's off by one and was like that since 2014. What's the relevance of this? Looking at 7d02b490e93c ("Kbuild, lto: Drop .number postfixes in modpost") what you're fixing here is something LTO-related. How do you trigger this? For a Cc:stable patch, I'm missing a lot of context. > `-z uniq-symbol` linker flag which we are planning to use to ^^ Who's "we"? > simplify livepatching brings numeric suffixes back, fix this. > Otherwise: > > ERROR: modpost: "param_set_uint.0" [vmlinux] is a static EXPORT_SYMBOL > > Fixes: fcd38ed0ff26 ("scripts: modpost: fix compilation warning") > Cc: stable@vger.kernel.org # 3.17+ > Signed-off-by: Alexander Lobakin <alexandr.lobakin@intel.com> ...
From: Borislav Petkov <bp@alien8.de> Date: Thu, 23 Dec 2021 17:19:06 +0100 > On Thu, Dec 23, 2021 at 01:21:55AM +0100, Alexander Lobakin wrote: > > For now, that condition from remove_dot(): > > > > if (m && (s[n + m] == '.' || s[n + m] == 0)) > > > > which was designed to test if it's a dot or a \0 after the suffix > > is never satisfied. > > This is due to that s[n + m] always points to the last digit of a > > numeric suffix, not on the symbol next to it: > > > > param_set_uint.0, s[n + m] is '0', s[n + m + 1] is '\0' > > > > So it's off by one and was like that since 2014. > > What's the relevance of this? Looking at > > 7d02b490e93c ("Kbuild, lto: Drop .number postfixes in modpost") > > what you're fixing here is something LTO-related. How do you trigger > this? It's just a couple lines below. I trigger this using `-z uniq-symbol` which uses numeric suffixes for globals as well. > > For a Cc:stable patch, I'm missing a lot of context. It fixes a commit dated 2014, thus Cc:stable. Although the remove_dot() might've been introduced for neverlanded GCC LTO, but in fact numeric suffixes are used a lot by the toolchains in regular builds as well. Just not for globals, that's why it's "well hidden". > > > `-z uniq-symbol` linker flag which we are planning to use to > ^^ > > Who's "we"? I thought it's a common saying in commit messages, isn't it? > > > simplify livepatching brings numeric suffixes back, fix this. > > Otherwise: > > > > ERROR: modpost: "param_set_uint.0" [vmlinux] is a static EXPORT_SYMBOL > > > > Fixes: fcd38ed0ff26 ("scripts: modpost: fix compilation warning") > > Cc: stable@vger.kernel.org # 3.17+ > > Signed-off-by: Alexander Lobakin <alexandr.lobakin@intel.com> > > ... > > -- > Regards/Gruss, > Boris. > > https://people.kernel.org/tglx/notes-about-netiquette Thanks, Al
On Mon, Dec 27, 2021 at 07:22:46PM +0100, Alexander Lobakin wrote: > It's just a couple lines below. I trigger this using `-z uniq-symbol` > which uses numeric suffixes for globals as well. Aha, so that's for the fgkaslr purposes now. > It fixes a commit dated 2014, thus Cc:stable. Although the > remove_dot() might've been introduced for neverlanded GCC LTO, but > in fact numeric suffixes are used a lot by the toolchains in regular > builds as well. Just not for globals, that's why it's "well hidden". Does "well hidden" warrant a stable backport then? Because if no toolchain is using numeric suffixes for globals, then no need for the stable tag, I'd say. > I thought it's a common saying in commit messages, isn't it? Lemme give you my canned and a lot more eloquent explanation for that: "Please use passive voice in your commit message: no "we" or "I", etc, and describe your changes in imperative mood. Also, pls read section "2) Describe your changes" in Documentation/process/submitting-patches.rst for more details. Also, see section "Changelog" in Documentation/process/maintainer-tip.rst Bottom line is: personal pronouns are ambiguous in text, especially with so many parties/companies/etc developing the kernel so let's avoid them please." Thx.
From: Borislav Petkov <bp@alien8.de> Date: Mon, 27 Dec 2021 22:26:02 +0100 > On Mon, Dec 27, 2021 at 07:22:46PM +0100, Alexander Lobakin wrote: > > It's just a couple lines below. I trigger this using `-z uniq-symbol` > > which uses numeric suffixes for globals as well. > > Aha, so that's for the fgkaslr purposes now. Well, linking using unique names is meant to be used always when available and livepatching is enabled, at least I hope so. > > > It fixes a commit dated 2014, thus Cc:stable. Although the > > remove_dot() might've been introduced for neverlanded GCC LTO, but > > in fact numeric suffixes are used a lot by the toolchains in regular > > builds as well. Just not for globals, that's why it's "well hidden". > > Does "well hidden" warrant a stable backport then? Because if no > toolchain is using numeric suffixes for globals, then no need for the > stable tag, I'd say. Hmm, makes sense. The fact that I haven't seen any similar reports or issues (even on LTO builds) sorta says there are no benefits from backporting this. Ok, I'll drop the tag. It's never too late anyway to port this in case someone will face it. > > > I thought it's a common saying in commit messages, isn't it? > > Lemme give you my canned and a lot more eloquent explanation for that: > > "Please use passive voice in your commit message: no "we" or "I", etc, > and describe your changes in imperative mood. > > Also, pls read section "2) Describe your changes" in > Documentation/process/submitting-patches.rst for more details. > > Also, see section "Changelog" in > Documentation/process/maintainer-tip.rst > > Bottom line is: personal pronouns are ambiguous in text, especially with > so many parties/companies/etc developing the kernel so let's avoid them > please." > > Thx. Ah, you're right. "Common used" doesn't mean "correct". I'll fix it in the next spin being published after accumulating a bunch more comments. Thanks! > > -- > Regards/Gruss, > Boris. > > https://people.kernel.org/tglx/notes-about-netiquette Al
On Thu, 23 Dec 2021, Alexander Lobakin wrote: > For now, that condition from remove_dot(): > > if (m && (s[n + m] == '.' || s[n + m] == 0)) > > which was designed to test if it's a dot or a \0 after the suffix > is never satisfied. > This is due to that s[n + m] always points to the last digit of a > numeric suffix, not on the symbol next to it: > > param_set_uint.0, s[n + m] is '0', s[n + m + 1] is '\0' > > So it's off by one and was like that since 2014. > > `-z uniq-symbol` linker flag which we are planning to use to `-z unique-symbol` Miroslav
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c index cb8ab7d91d30..ccc6d35580f2 100644 --- a/scripts/mod/modpost.c +++ b/scripts/mod/modpost.c @@ -1971,7 +1971,7 @@ static char *remove_dot(char *s) if (n && s[n]) { size_t m = strspn(s + n + 1, "0123456789"); - if (m && (s[n + m] == '.' || s[n + m] == 0)) + if (m && (s[n + m + 1] == '.' || s[n + m + 1] == 0)) s[n] = 0; /* strip trailing .lto */
For now, that condition from remove_dot(): if (m && (s[n + m] == '.' || s[n + m] == 0)) which was designed to test if it's a dot or a \0 after the suffix is never satisfied. This is due to that s[n + m] always points to the last digit of a numeric suffix, not on the symbol next to it: param_set_uint.0, s[n + m] is '0', s[n + m + 1] is '\0' So it's off by one and was like that since 2014. `-z uniq-symbol` linker flag which we are planning to use to simplify livepatching brings numeric suffixes back, fix this. Otherwise: ERROR: modpost: "param_set_uint.0" [vmlinux] is a static EXPORT_SYMBOL Fixes: fcd38ed0ff26 ("scripts: modpost: fix compilation warning") Cc: stable@vger.kernel.org # 3.17+ Signed-off-by: Alexander Lobakin <alexandr.lobakin@intel.com> --- scripts/mod/modpost.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)