Re: [PATCH] objtool: Don't fail the kernel build on fatal errors
On 1/15/21 4:15 AM, Josh Poimboeuf wrote:
> This is basically a revert of commit 644592d32837 ("objtool: Fail the
> kernel build on fatal errors").
>
> That change turned out to be more trouble than it's worth. Failing the
> build is an extreme measure which sometimes gets too much attention and
> blocks CI build testing.
>
> These fatal-type warnings aren't yet as rare as we'd hope, due to the
> ever-increasing matrix of supported toolchains/plugins and their
> fast-changing nature as of late.
>
> Also, there are more people (and bots) looking for objtool warnings than
> ever before, so such warnings not likely to be ignored for long.
>
> Suggested-by: Nick Desaulniers
> Signed-off-by: Josh Poimboeuf
Reviewed-by: Kamalesh Babulal
--
Kamalesh
Re: [PATCH v4 23/52] docs: trace-uses.rst: remove bogus c-domain tags
On 01/10/20 12:11 pm, Mauro Carvalho Chehab wrote: > Em Thu, 1 Oct 2020 11:36:53 +0530 > Kamalesh Babulal escreveu: > >> On 30/09/20 6:54 pm, Mauro Carvalho Chehab wrote: >>> There are some c-domain tags that are wrong. While this won't >>> cause problems with Sphinx < 3.0, this cause troubles with >>> newer versions, as the C parser won't recognize the contents >>> of the tag, and will drop it from the output. >>> >>> Let's just place them at literal blocks. >>> >> >> tired with Sphinx v3.2.1, invalid C declaration warnings are not >> seen with the patch. > > Well, it would be possible to use :c:expr: with Sphinx 3.2.1, > in order for it to check for invalid C declarations. > > Btw, this is one of the improvements over the last versions: the > rewritten C parser there is a lot more pedantic with regards to the > C syntax. > > - > > That's said, the backward-compatibility code I added at > Documentation/sphinx/cdomain.py will convert this into a > literal markup though, as there's no equivalent tag before > Sphinx 3.x. > > As there are still one upstream issue on Sphinx 3.x that requires a fix[1], > and we don't know yet the issues with :c:expr[2], at least for now, I would > avoid adding :c:expr: markups. > > [1] Right now, the C domain is not able to have two names > for different types. So, it is not possible to have > a struct "foo" and a function "foo". > > Due to that, while I was able to fix all warnings with > Sphinx 2.x build, Sphinx 3.x will still have bogus > warnings. > > [2] One of the limitations of :c:expr: is with regards to function > prototypes. You can't use it like: :c:expr:`int foo(void);`, > as it will complain with the function return type. > Thank you for explaining in detail on the :c:expr: tag. I intended to say this patch fixes the warnings seen while using the c-domain tags and are fixed by converting them into literals. -- Kamalesh
Re: [PATCH v4 23/52] docs: trace-uses.rst: remove bogus c-domain tags
On 30/09/20 6:54 pm, Mauro Carvalho Chehab wrote: > There are some c-domain tags that are wrong. While this won't > cause problems with Sphinx < 3.0, this cause troubles with > newer versions, as the C parser won't recognize the contents > of the tag, and will drop it from the output. > > Let's just place them at literal blocks. > tired with Sphinx v3.2.1, invalid C declaration warnings are not seen with the patch. > Signed-off-by: Mauro Carvalho Chehab Reviewed-by: Kamalesh Babulal -- Kamalesh
Re: [PATCH] [v2] tools/objtool: Fix unnecessary jumps
On 11/08/20 9:48 am, Youling Tang wrote: > There is no need to jump to the "out" tag when "ret < 0", just return > directly to "ret". > > Signed-off-by: Youling Tang the patch looks good to me, the commit message doesn't explain the reason for the change. It can be written like: "Previously cleanup() function was called under the out label for both fatal errors (ret < 0) and warnings. Now that cleanup() function is removed, the out label is no longer required. Remove it and return immediately for the fatal errors with ret as return code and 0 for warnings." Reviewed-by: Kamalesh Babulal -- Kamalesh
Re: [PATCH] tools/objtool: Fix unnecessary jumps
On 10/08/20 9:36 am, Youling Tang wrote:
> There is no need to jump to the "out" tag when "ret < 0", just return
> directly to "ret".
>
> Signed-off-by: Youling Tang
> ---
> tools/objtool/check.c | 16
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index e034a8f..94b166d 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
[snip]
>
> if (file.elf->changed) {
> ret = elf_write(file.elf);
> if (ret < 0)
> - goto out;
> + return ret;
> }
>
> out:
the out label code is no more required with this change, so remove
it and return 0 for warnings for now. Previously cleanup() function
was called under the out label for both fatal errors (ret < 0) and
warnings. Now that cleanup() function is removed, the out label is
no longer required.
--
Kamalesh
Re: [PATCH] Revert "kbuild: use -flive-patching when CONFIG_LIVEPATCH is enabled"
On 20/07/20 9:05 am, Joe Lawrence wrote:
> On 7/17/20 2:29 PM, Josh Poimboeuf wrote:
>> Use of the new -flive-patching flag was introduced with the following
>> commit:
>>
>> 43bd3a95c98e ("kbuild: use -flive-patching when CONFIG_LIVEPATCH is
>> enabled")
>>
>> This flag has several drawbacks:
>>
>> [ ... snip ... ]
>>
>> - While there *is* a distro which relies on this flag for their distro
>> livepatch module builds, there's not a publicly documented way to
>> create safe livepatch modules with it. Its use seems to be based on
>> tribal knowledge. It serves no benefit to those who don't know how to
>> use it.
>>
>> (In fact, I believe the current livepatch documentation and samples
>> are misleading and dangerous, and should be corrected. Or at least
>> amended with a disclaimer. But I don't feel qualified to make such
>> changes.)
>
> FWIW, I'm not exactly qualified to document source-based creation either,
> however I have written a few of the samples and obviously the kselftest
> modules.
>
> The samples should certainly include a disclaimer (ie, they are only for API
> demonstration purposes!) and eventually it would be great if the kselftest
> modules could guarantee their safety as well. I don't know quite yet how we
> can automate that, but perhaps some kind of post-build sanity check could
> verify that they are in fact patching what they intend to patch.
>
> As for a more general, long-form warning about optimizations, I grabbed
> Miroslav's LPC slides from a few years back and poked around at some
> IPA-optimized disassembly... Here are my notes that attempt to capture some
> common cases:
>
> http://file.bos.redhat.com/~jolawren/klp-compiler-notes/livepatch/compiler-considerations.html
Hi Joe,
The notes link you shared is not accessible.
Regards,
--
Kamalesh
Re: [PATCH v2] selftests/livepatch: adopt to newer sysctl error format
On 14/07/20 2:40 pm, Petr Mladek wrote: > With procfs v3.3.16, the sysctl command doesn't print the set key and > value on error. This change breaks livepatch selftest test-ftrace.sh, > that tests the interaction of sysctl ftrace_enabled: > > Make it work with all sysctl versions using '-q' option. > > Explicitly print the final status on success so that it can be verified > in the log. The error message is enough on failure. > > Reported-by: Kamalesh Babulal > Signed-off-by: Petr Mladek Reviewed-by: Kamalesh Babulal -- Kamalesh
Re: [PATCH] selftests/livepatch: adopt to newer sysctl error format
On 11/07/20 12:01 am, Joe Lawrence wrote:
> On Fri, Jul 10, 2020 at 05:27:35PM +0200, Petr Mladek wrote:
>> On Fri 2020-07-10 10:40:43, Kamalesh Babulal wrote:
>>> With procfs v3.3.16, the sysctl command doesn't prints the set key and
>>> value on error. This change breaks livepatch selftest test-ftrace.sh,
>>> that tests the interaction of sysctl ftrace_enabled:
>>>
>
> Good catch, it looks like it was this procps commit that modified that
> behavior:
>
> commit da82fe49b1476d227874905068adb69577e11d96
> Author: Patrick Steinhardt
> Date: Tue May 29 13:29:03 2018 +0200
>
> sysctl: do not report set key in case `close_stream` fails
>
> As we're using buffered I/O when writing kernel parameters, write errors
> may get delayed until we close the `FILE` stream. As we are currently
> outputting the key that is to be set disregarding the return value of
> `close_stream`, we may end up in a situation where we report error and
> success:
>
> $ sysctl kernel.printk_ratelimit=100
> sysctl: setting key "kernel.printk_ratelimit": error code 22
> kernel.printk_ratelimit = 100
>
> Fix the issue by only outputting the updated value in case
> `close_stream` does not report an error.
>
> Signed-off-by: Patrick Steinhardt
>
> And I'd agree that echoing the failed new value was confusing to see
> from a user's perspective.
>
>>> # selftests: livepatch: test-ftrace.sh
>>> # TEST: livepatch interaction with ftrace_enabled sysctl ... not ok
>>> #
>>> # --- expected
>>> # +++ result
>>> # @@ -16,7 +16,7 @@ livepatch: 'test_klp_livepatch': initial
>>> # livepatch: 'test_klp_livepatch': starting patching transition
>>> # livepatch: 'test_klp_livepatch': completing patching transition
>>> # livepatch: 'test_klp_livepatch': patching complete
>>> # -livepatch: sysctl: setting key "kernel.ftrace_enabled": Device or
>>> resource busy kernel.ftrace_enabled = 0
>>> # +livepatch: sysctl: setting key "kernel.ftrace_enabled": Device or
>>> resource busy
>>> # % echo 0 > /sys/kernel/livepatch/test_klp_livepatch/enabled
>>> # livepatch: 'test_klp_livepatch': initializing unpatching transition
>>> # livepatch: 'test_klp_livepatch': starting unpatching transition
>>> #
>>> # ERROR: livepatch kselftest(s) failed
>>>
>>> on setting sysctl kernel.ftrace_enabled={0,1} value successfully, the
>>> set key and value is displayed.
>>>
>>> This patch fixes it by limiting the output from both the cases to eight
>>> words, that includes the error message or set key and value on failure
>>> and success. The upper bound of eight words is enough to display the
>>> only tracked error message. Also, adjust the check_result string in
>>> test-ftrace.sh to match the expected output.
>>
>> This looks really tricky.
>>
>> I wonder if we could use "sysctl -q" to refuse printing the value
>> even with older versions. The following patch works here with
>> sysctl 3.3.15:
Agree, "sysctl -q" is more robust, I tested it with sysctl v3.3.16 and the test
passes. Can you please send the patch?
>>
>
> FWIW, --quiet was added to procps way back in 2004, so it should be safe
> to use... and there's already a bunch of net selftests using it.
a couple of tests is using the "-q" options to set the sysctl values.
>
>> diff --git a/tools/testing/selftests/livepatch/functions.sh
>> b/tools/testing/selftests/livepatch/functions.sh
>> index 2aab9791791d..47aa4c762bb4 100644
>> --- a/tools/testing/selftests/livepatch/functions.sh
>> +++ b/tools/testing/selftests/livepatch/functions.sh
>> @@ -64,7 +64,8 @@ function set_dynamic_debug() {
>> }
>>
>> function set_ftrace_enabled() {
>> -result=$(sysctl kernel.ftrace_enabled="$1" 2>&1 | paste --serial
>> --delimiters=' ')
>> +result=$(sysctl -q kernel.ftrace_enabled="$1" 2>&1 && \
>> + sysctl kernel.ftrace_enabled 2>&1)
>> echo "livepatch: $result" > /dev/kmsg
>> }
>>
>> diff --git a/tools/testing/selftests/livepatch/test-ftrace.sh
>> b/tools/testing/selftests/livepatch/test-ftrace.sh
>> index e2a76887f40a..aa967c5d0558 100755
>> --- a/tools/testing/selftests/livepatch/test-ftrace.sh
>> +++ b/tools/testing/selftests/livepatch/test-ftrace.sh
>
[PATCH] selftests/livepatch: adopt to newer sysctl error format
With procfs v3.3.16, the sysctl command doesn't prints the set key and
value on error. This change breaks livepatch selftest test-ftrace.sh,
that tests the interaction of sysctl ftrace_enabled:
# selftests: livepatch: test-ftrace.sh
# TEST: livepatch interaction with ftrace_enabled sysctl ... not ok
#
# --- expected
# +++ result
# @@ -16,7 +16,7 @@ livepatch: 'test_klp_livepatch': initial
# livepatch: 'test_klp_livepatch': starting patching transition
# livepatch: 'test_klp_livepatch': completing patching transition
# livepatch: 'test_klp_livepatch': patching complete
# -livepatch: sysctl: setting key "kernel.ftrace_enabled": Device or
resource busy kernel.ftrace_enabled = 0
# +livepatch: sysctl: setting key "kernel.ftrace_enabled": Device or
resource busy
# % echo 0 > /sys/kernel/livepatch/test_klp_livepatch/enabled
# livepatch: 'test_klp_livepatch': initializing unpatching transition
# livepatch: 'test_klp_livepatch': starting unpatching transition
#
# ERROR: livepatch kselftest(s) failed
on setting sysctl kernel.ftrace_enabled={0,1} value successfully, the
set key and value is displayed.
This patch fixes it by limiting the output from both the cases to eight
words, that includes the error message or set key and value on failure
and success. The upper bound of eight words is enough to display the
only tracked error message. Also, adjust the check_result string in
test-ftrace.sh to match the expected output.
With the patch, the test-ftrace.sh passes on v3.3.15, v3.3.16 versions
of sysctl:
...
# selftests: livepatch: test-ftrace.sh
# TEST: livepatch interaction with ftrace_enabled sysctl ... ok
ok 5 selftests: livepatch: test-ftrace.sh
Signed-off-by: Kamalesh Babulal
---
Based on livepatching/for-5.9/selftests-cleanup, to be merged
through livepatching.git
tools/testing/selftests/livepatch/functions.sh | 3 ++-
tools/testing/selftests/livepatch/test-ftrace.sh | 2 +-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/livepatch/functions.sh
b/tools/testing/selftests/livepatch/functions.sh
index 36648ca367c2..e3c0490d5a45 100644
--- a/tools/testing/selftests/livepatch/functions.sh
+++ b/tools/testing/selftests/livepatch/functions.sh
@@ -75,7 +75,8 @@ function set_dynamic_debug() {
}
function set_ftrace_enabled() {
- result=$(sysctl kernel.ftrace_enabled="$1" 2>&1 | paste --serial
--delimiters=' ')
+ result=$(sysctl kernel.ftrace_enabled="$1" 2>&1 | paste --serial
--delimiters=' ' | \
+cut -d" " -f1-8)
echo "livepatch: $result" > /dev/kmsg
}
diff --git a/tools/testing/selftests/livepatch/test-ftrace.sh
b/tools/testing/selftests/livepatch/test-ftrace.sh
index 9160c9ec3b6f..552e165512f4 100755
--- a/tools/testing/selftests/livepatch/test-ftrace.sh
+++ b/tools/testing/selftests/livepatch/test-ftrace.sh
@@ -51,7 +51,7 @@ livepatch: '$MOD_LIVEPATCH': initializing patching transition
livepatch: '$MOD_LIVEPATCH': starting patching transition
livepatch: '$MOD_LIVEPATCH': completing patching transition
livepatch: '$MOD_LIVEPATCH': patching complete
-livepatch: sysctl: setting key \"kernel.ftrace_enabled\": Device or resource
busy kernel.ftrace_enabled = 0
+livepatch: sysctl: setting key \"kernel.ftrace_enabled\": Device or resource
busy
% echo 0 > /sys/kernel/livepatch/$MOD_LIVEPATCH/enabled
livepatch: '$MOD_LIVEPATCH': initializing unpatching transition
livepatch: '$MOD_LIVEPATCH': starting unpatching transition
base-commit: 3fd9bd8b7e41a1908bf8bc0cd06606f2b787cd39
--
2.26.2
Re: [RFC][PATCH v4 27/32] objtool: mcount: Generic location and relocation table types
On 6/9/20 11:42 PM, Matt Helsley wrote:
[...]
>> Hi Matt,
>>
>> I was trying out the patch series on ppc64le and found that __mcount_loc
>> and .rela__mcount_loc section pairs do not get generated.
>>
>> # readelf -S fs/proc/cmdline.o|grep mcount
>> #
>>
>> Debugged the cause to get_mcountsym()'s return type. It returns reloc
>> type from GELF_R_INFO() and expects Elf64_Xword a.k.a unsigned long
>> to be the return type but get_mcountsym() returns unsigned int on 64-bit.
>>
>> On power the _mcount is of relocation type R_PPC64_REL24 (info 0x17000a),
>> using unsigned int truncates the value to 0xa and fails the above check.
>> Using below fix, that converts mcount_sym_info to use unsigned long,
>> generates
>> the __mcount_loc section pairs.
>>
>> --- a/tools/objtool/mcount.c
>> +++ b/tools/objtool/mcount.c
>> @@ -163,7 +163,7 @@ static int is_mcounted_section_name(char const *const
>> txtname)
>> strcmp(".cpuidle.text", txtname) == 0;
>> }
>>
>> -static unsigned int get_mcount_sym_info(struct reloc *reloc)
>> +static unsigned long get_mcount_sym_info(struct reloc *reloc)
>> {
>> struct symbol *sym = reloc->sym;
>> char const *symname = sym->name;
>> @@ -274,7 +274,7 @@ static int nop_mcount(struct section * const rels,
>> {
>> struct reloc *reloc;
>> struct section *txts = find_section_by_index(lf, rels->sh.sh_info);
>> - unsigned int mcount_sym_info = 0;
>> + unsigned long mcount_sym_info = 0;
>> int once = 0;
>>
>> list_for_each_entry(reloc, >reloc_list, list) {
>> @@ -363,7 +363,7 @@ static void sift_rel_mcount(GElf_Addr **mlocpp,
>> {
>> GElf_Rel *mrelp = *mrelpp;
>> GElf_Rela *mrelap = *mrelpp;
>> - unsigned int mcount_sym_info = 0;
>> + unsigned long mcount_sym_info = 0;
>> struct reloc *reloc;
>>
>> list_for_each_entry(reloc, >reloc_list, list) {
>>
>> # readelf -S fs/proc/cmdline.o|grep mcount
>> [31] __mcount_loc PROGBITS 00022f10
>> [32] .rela__mcount_loc RELA 00022f20
>
> Fixed for next posting.
>
> I've essentially added this as another patch before it moves into
> recordmcount.c, gets renamed to get_mcount_sym_info(), etc. I did it
> this way because it only becomes necessary to change the type before
> moving the function (and eventually its callers) out of the wrapper.
>
> I feel I should credit you as author or at least co-author of the added
> patch since it's basically a "backported" version of the changes you
> suggested. I reviewed the process in submitting-patches.rst and propose
> the commit message:
>
> objtool: mcount: Extend mcountsym size
>
> Before we can move this function out of the wrapper and into
> wordsize-independent code we need to explicitly size the
> type returned from get_mcountsym() to preserve the symbol info.
>
> Reported-by: Kamalesh Babulal
> Signed-off-by: Kamalesh Babulal
> Signed-off-by: Matt Helsley
>
> Is that OK with you or do you have another preference?
Thanks, it works for me.
--
Kamalesh
Re: [RFC][PATCH v4 27/32] objtool: mcount: Generic location and relocation table types
On 6/3/20 1:20 AM, Matt Helsley wrote:
> Rather than building the exact ELF section data we need and
> avoiding libelf's conversion step, use more GElf types
> and then libelf's elfxx_xlatetof() functions to convert
> the mcount locations (GElf_Addr) and associated relocations.
>
> This converts sift_rel_mcount() so that it doesn't use the
> recordmcount wrapper. The next patch will move it out of the
> wrapper.
>
> Signed-off-by: Matt Helsley
> ---
> tools/objtool/recordmcount.c | 44 +++--
> tools/objtool/recordmcount.h | 120 ++-
> 2 files changed, 59 insertions(+), 105 deletions(-)
>
> diff --git a/tools/objtool/recordmcount.c b/tools/objtool/recordmcount.c
> index 06a8f8ddefa7..ef3c360a3db9 100644
> --- a/tools/objtool/recordmcount.c
> +++ b/tools/objtool/recordmcount.c
[...]
> -static uint_t *sift_rel_mcount(uint_t *mlocp,
> -unsigned const offbase,
> -Elf_Rel **const mrelpp,
> +static void sift_rel_mcount(GElf_Addr **mlocpp,
> +GElf_Sxword *r_offsetp,
> +void **const mrelpp,
> const struct section * const rels,
> unsigned const recsym_index,
> unsigned long const recval,
> -unsigned const reltype)
> +unsigned const reltype,
> +bool is_rela)
> {
> - uint_t *const mloc0 = mlocp;
> - Elf_Rel *mrelp = *mrelpp;
> - unsigned int rel_entsize = rels->sh.sh_entsize;
> - unsigned mcountsym = 0;
> + GElf_Rel *mrelp = *mrelpp;
> + GElf_Rela *mrelap = *mrelpp;
> + unsigned int mcount_sym_info = 0;
> struct reloc *reloc;
>
> list_for_each_entry(reloc, >reloc_list, list) {
> - if (!mcountsym)
> - mcountsym = get_mcountsym(reloc);
> -
> - if (mcountsym == GELF_R_INFO(reloc->sym->idx, reloc->type) &&
> !is_fake_mcount(reloc)) {
> - uint_t const addend =
> - _w(reloc->offset - recval + mcount_adjust);
> - mrelp->r_offset = _w(offbase
> - + ((void *)mlocp - (void *)mloc0));
> - Elf_r_info(mrelp, recsym_index, reltype);
> - if (rel_entsize == sizeof(Elf_Rela)) {
> - ((Elf_Rela *)mrelp)->r_addend = addend;
> - *mlocp++ = 0;
> - } else
> - *mlocp++ = addend;
> -
> - mrelp = (Elf_Rel *)(rel_entsize + (void *)mrelp);
> + unsigned long addend;
> +
> + if (!mcount_sym_info)
> + mcount_sym_info = get_mcount_sym_info(reloc);
> +
> + if (mcount_sym_info != GELF_R_INFO(reloc->sym->idx,
> reloc->type) || is_fake_mcount(reloc))
> + continue;
Hi Matt,
I was trying out the patch series on ppc64le and found that __mcount_loc
and .rela__mcount_loc section pairs do not get generated.
# readelf -S fs/proc/cmdline.o|grep mcount
#
Debugged the cause to get_mcountsym()'s return type. It returns reloc
type from GELF_R_INFO() and expects Elf64_Xword a.k.a unsigned long
to be the return type but get_mcountsym() returns unsigned int on 64-bit.
On power the _mcount is of relocation type R_PPC64_REL24 (info 0x17000a),
using unsigned int truncates the value to 0xa and fails the above check.
Using below fix, that converts mcount_sym_info to use unsigned long, generates
the __mcount_loc section pairs.
--- a/tools/objtool/mcount.c
+++ b/tools/objtool/mcount.c
@@ -163,7 +163,7 @@ static int is_mcounted_section_name(char const *const
txtname)
strcmp(".cpuidle.text", txtname) == 0;
}
-static unsigned int get_mcount_sym_info(struct reloc *reloc)
+static unsigned long get_mcount_sym_info(struct reloc *reloc)
{
struct symbol *sym = reloc->sym;
char const *symname = sym->name;
@@ -274,7 +274,7 @@ static int nop_mcount(struct section * const rels,
{
struct reloc *reloc;
struct section *txts = find_section_by_index(lf, rels->sh.sh_info);
- unsigned int mcount_sym_info = 0;
+ unsigned long mcount_sym_info = 0;
int once = 0;
list_for_each_entry(reloc, >reloc_list, list) {
@@ -363,7 +363,7 @@ static void sift_rel_mcount(GElf_Addr **mlocpp,
{
GElf_Rel *mrelp = *mrelpp;
GElf_Rela *mrelap = *mrelpp;
- unsigned int mcount_sym_info = 0;
+ unsigned long mcount_sym_info = 0;
struct reloc *reloc;
list_for_each_entry(reloc, >reloc_list, list) {
# readelf -S fs/proc/cmdline.o|grep mcount
[31] __mcount_loc PROGBITS 00022f10
[32] .rela__mcount_loc RELA 00022f20
> +
> + addend = reloc->offset - recval +
Re: [PATCH 2/2] objtool: Add support for relocations without addends
On 5/30/20 2:31 AM, Matt Helsley wrote:
> Currently objtool only collects information about relocations with
> addends. In recordmcount, which we are about to merge into objtool,
> some supported architectures do not use rela relocations.
>
> Signed-off-by: Matt Helsley
> ---
LGTM, minor nit-pick, checkpatch reports of missing space before '(' in
the switch cases. With space issues resolved:
Reviewed-by: Kamalesh Babulal
--
Kamalesh
Re: [PATCH 2/3] objtool: Move struct objtool_file into arch-independent header
On 5/20/20 2:25 AM, Matt Helsley wrote: > The objtool_file structure describes the files objtool works on, > is used by the check subcommand, and the check.h header is included > by the orc subcommands so it's presently used by all subcommands. > > Since the structure will be useful in all subcommands besides check, > and some subcommands may not want to include check.h to get the > definition, split the structure out into a new header meant for use > by all objtool subcommands. > > Signed-off-by: Matt Helsley Reviewed-by: Kamalesh Babulal -- Kamalesh
Re: [PATCH 3/3] objtool: Enable compilation of objtool for all architectures
[...]
> From: Matt Helsley
> Subject: [PATCH] objtool: Enable compilation of objtool for all architectures
>
> Objtool currently only compiles for x86 architectures. This is
> fine as it presently does not support tooling for other
> architectures. However, we would like to be able to convert other
> kernel tools to run as objtool sub commands because they too
> process ELF object files. This will allow us to convert tools
> such as recordmcount to use objtool's ELF code.
>
> Since much of recordmcount's ELF code is copy-paste code to/from
> a variety of other kernel tools (look at modpost for example) this
> means that if we can convert recordmcount we can convert more.
>
> We define weak definitions for subcommand entry functions and other weak
> definitions for shared functions critical to building existing
> subcommands. These return 127 when the command is missing which signify
> tools that do not exist on all architectures. In this case the "check"
> and "orc" tools do not exist on all architectures so we only add them
> for x86. Future changes adding support for "check", to arm64 for
> example, can then modify the SUBCMD_CHECK variable when building for
> arm64.
>
> Objtool is not currently wired in to KConfig to be built for other
> architectures because it's not needed for those architectures and
> there are no commands it supports other than those for x86. As more
> command support is enabled on various architectures the necessary
> KConfig changes can be made (e.g. adding "STACK_VALIDATION") to
> trigger building objtool.
>
> [ jpoimboe: remove aliases, add __weak macro, add error messages ]
>
> Cc: Julien Thierry
> Signed-off-by: Matt Helsley
> Signed-off-by: Josh Poimboeuf
A minor nit-pick in objtool.h
Reviewed-by: Kamalesh Babulal
> ---
[...]
> diff --git a/tools/objtool/objtool.h b/tools/objtool/objtool.h
> index d89616b2ca39..528028a66816 100644
> --- a/tools/objtool/objtool.h
> +++ b/tools/objtool/objtool.h
> @@ -19,4 +19,9 @@ struct objtool_file {
> bool ignore_unreachables, c_file, hints, rodata;
> };
>
> +int check(const char *objname, bool orc);
> +int orc_dump(const char *objname);
> +int create_orc(struct objtool_file *file);
> +int create_orc_sections(struct objtool_file *file);
> +
> #endif /* _OBJTOOL_H */
above hunk will not apply cleanly on patch 2 of the series, it expects
a new line after struct objtool which is missing in patch 2.
--
Kamalesh
Re: [PATCH 1/3] objtool: Exit successfully when requesting help
On 5/20/20 2:25 AM, Matt Helsley wrote: > When the user requests help it's not an error so do not exit with > a non-zero exit code. This is not especially useful for a user but > any script that might wish to check that objtool --help is at least > available can't rely on the exit code to crudely check that, for > example, building an objtool executable succeeds. > > Signed-off-by: Matt Helsley Reviewed-by: Kamalesh Babulal -- Kamalesh
[PATCH] MAINTAINERS: add lib/livepatch to LIVE PATCHING
Add lib/livepatch to list of livepatching F: patterns in MAINTAINERS. Suggested-by: Jiri Kosina Signed-off-by: Kamalesh Babulal --- MAINTAINERS | 1 + 1 file changed, 1 insertion(+) diff --git a/MAINTAINERS b/MAINTAINERS index 8e1eb98d..de4f6af03198 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -9859,6 +9859,7 @@ F:arch/s390/include/asm/livepatch.h F: arch/x86/include/asm/livepatch.h F: include/linux/livepatch.h F: kernel/livepatch/ +F: lib/livepatch/ F: samples/livepatch/ F: tools/testing/selftests/livepatch/ base-commit: 1a0601ade9e132a03fd3e957f6f27e1ed89c1b2e -- 2.26.2
Re: [PATCH] MAINTAINERS: adjust to livepatch .klp.arch removal
On 5/15/20 3:15 AM, Jiri Kosina wrote:
> On Sat, 9 May 2020, Kamalesh Babulal wrote:
>
>>> Commit 1d05334d2899 ("livepatch: Remove .klp.arch") removed
>>> arch/x86/kernel/livepatch.c, but missed to adjust the LIVE PATCHING entry
>>> in MAINTAINERS.
>>>
>>> Since then, ./scripts/get_maintainer.pl --self-test=patterns complains:
>>>
>>> warning: no file matches F: arch/x86/kernel/livepatch.c
>>>
>>> So, drop that obsolete file entry in MAINTAINERS.
>>
>> Patch looks good to me, you probably want to add following architecture
>> specific livepatching header files to the list:
>>
>> arch/s390/include/asm/livepatch.h
>> arch/powerpc/include/asm/livepatch.h
>
> Good point, thanks for spotting it Kamalesh. I've queued the patch below
> on top.
>
>
Thanks, Jiri. I realized later, that the lib/livepatch directory also needs
to be included in the list of files maintained under livepatch. Earlier, this
week I had sent a patch to the mailing list that includes both arch
headers and lib/livepatch to the list of files, the link to the patch is:
https://lore.kernel.org/live-patching/20200511061014.308675-1-kamal...@linux.vnet.ibm.com/
--
Kamalesh
[PATCH] MAINTAINERS: Update LIVE PATCHING file list
The current list of livepatching files is incomplete, update the list with the missing files. Included files are ordered by the command: ./scripts/parse-maintainers.pl --input=MAINTAINERS --output=MAINTAINERS --order Signed-off-by: Kamalesh Babulal --- The patch applies on top of livepatching/for-next MAINTAINERS | 3 +++ 1 file changed, 3 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 7e0827670425..de4f6af03198 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -9854,9 +9854,12 @@ S: Maintained T: git git://git.kernel.org/pub/scm/linux/kernel/git/livepatching/livepatching.git F: Documentation/ABI/testing/sysfs-kernel-livepatch F: Documentation/livepatch/ +F: arch/powerpc/include/asm/livepatch.h +F: arch/s390/include/asm/livepatch.h F: arch/x86/include/asm/livepatch.h F: include/linux/livepatch.h F: kernel/livepatch/ +F: lib/livepatch/ F: samples/livepatch/ F: tools/testing/selftests/livepatch/ base-commit: f644e7bbd7c124cf08875200d447ce91441cd866 -- 2.26.2
Re: [PATCH] MAINTAINERS: adjust to livepatch .klp.arch removal
On 5/9/20 1:02 PM, Lukas Bulwahn wrote:
> Commit 1d05334d2899 ("livepatch: Remove .klp.arch") removed
> arch/x86/kernel/livepatch.c, but missed to adjust the LIVE PATCHING entry
> in MAINTAINERS.
>
> Since then, ./scripts/get_maintainer.pl --self-test=patterns complains:
>
> warning: no file matches F: arch/x86/kernel/livepatch.c
>
> So, drop that obsolete file entry in MAINTAINERS.
Patch looks good to me, you probably want to add following architecture
specific livepatching header files to the list:
arch/s390/include/asm/livepatch.h
arch/powerpc/include/asm/livepatch.h
>
> Signed-off-by: Lukas Bulwahn
> ---
> Jiri, please take this minor non-urgent patch for livepatching/for-next.
> Peter, please ack.
>
> applies cleanly on next-20200508
>
> MAINTAINERS | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 92657a132417..642f55c4b556 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9909,7 +9909,6 @@ T: git
> git://git.kernel.org/pub/scm/linux/kernel/git/livepatching/livepatching.g
> F: Documentation/ABI/testing/sysfs-kernel-livepatch
> F: Documentation/livepatch/
> F: arch/x86/include/asm/livepatch.h
> -F: arch/x86/kernel/livepatch.c
> F: include/linux/livepatch.h
> F: kernel/livepatch/
> F: samples/livepatch/
>
--
Kamalesh
Re: [PATCH -next] livepatch: Make klp_apply_object_relocs static
On 5/8/20 5:36 PM, Samuel Zou wrote:
> Fix the following sparse warning:
>
> kernel/livepatch/core.c:748:5: warning: symbol 'klp_apply_object_relocs'
> was not declared. Should it be static?
>
> Reported-by: Hulk Robot
> Signed-off-by: Samuel Zou
LGTM, klp_apply_object_relocs() has only one call site within core.c
Reviewed-by: Kamalesh Babulal
> ---
> kernel/livepatch/core.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 96d2da1..f76fdb9 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -745,7 +745,8 @@ static int klp_init_func(struct klp_object *obj, struct
> klp_func *func)
> func->old_sympos ? func->old_sympos : 1);
> }
>
> -int klp_apply_object_relocs(struct klp_patch *patch, struct klp_object *obj)
> +static int klp_apply_object_relocs(struct klp_patch *patch,
> +struct klp_object *obj)
> {
> int i, ret;
> struct klp_modinfo *info = patch->mod->klp_info;
>
Re: [PATCH v3 3/3] selftests/livepatch: Test interaction with ftrace_enabled
On 10/17/19 3:17 AM, Joe Lawrence wrote: > On 10/16/19 1:06 PM, Kamalesh Babulal wrote: >> On 10/16/19 5:03 PM, Miroslav Benes wrote: >>> From: Joe Lawrence >>> >>> Since livepatching depends upon ftrace handlers to implement "patched" >>> code functionality, verify that the ftrace_enabled sysctl value >>> interacts with livepatch registration as expected. At the same time, >>> ensure that ftrace_enabled is set and part of the test environment >>> configuration that is saved and restored when running the selftests. >>> >>> Signed-off-by: Joe Lawrence >>> Signed-off-by: Miroslav Benes >> >> [...] >>> diff --git a/tools/testing/selftests/livepatch/test-ftrace.sh >>> b/tools/testing/selftests/livepatch/test-ftrace.sh >>> new file mode 100755 >>> index ..e2a76887f40a >>> --- /dev/null >>> +++ b/tools/testing/selftests/livepatch/test-ftrace.sh >> >> This test fails due to wrong file permissions, with the warning: >> >> # Warning: file test-ftrace.sh is not executable, correct this. >> not ok 4 selftests: livepatch: test-ftrace.sh >> > > Hi Kamalesh, > > Thanks for testing. This error is weird as the git log says new file mode: > 100755. 'git am' of Miroslav's patchset gives me a new test-ftrace.sh with > "Access: (0775/-rwxrwxr-x)" Did you apply the mbox directly or.. ? > Hi Joe, Thanks, I was using patch command to apply the patches and using git am helped. Sorry for the noise. The test cases passes now, without the issue I previously reported: [...] # TEST: livepatch interaction with ftrace_enabled sysctl ... ok ok 4 selftests: livepatch: test-ftrace.sh Long story is that the patch command version installed on the test machine doesn't understand new file mode permission from the git diff, updating the patch version creates the patch with 755 mode. -- Kamalesh
Re: [PATCH v3 2/3] selftests/livepatch: Make dynamic debug setup and restore generic
On 10/16/19 5:03 PM, Miroslav Benes wrote: > From: Joe Lawrence > > Livepatch selftests currently save the current dynamic debug config and > tweak it for the selftests. The config is restored at the end. Make the > infrastructure generic, so that more variables can be saved and > restored. > > Signed-off-by: Joe Lawrence > Signed-off-by: Miroslav Benes > --- > .../testing/selftests/livepatch/functions.sh | 22 +++ > .../selftests/livepatch/test-callbacks.sh | 2 +- > .../selftests/livepatch/test-livepatch.sh | 2 +- > .../selftests/livepatch/test-shadow-vars.sh | 2 +- A minor nit pick, should the README also updated with the setup_config()? -- Kamalesh
Re: [PATCH v3 3/3] selftests/livepatch: Test interaction with ftrace_enabled
On 10/16/19 5:03 PM, Miroslav Benes wrote: > From: Joe Lawrence > > Since livepatching depends upon ftrace handlers to implement "patched" > code functionality, verify that the ftrace_enabled sysctl value > interacts with livepatch registration as expected. At the same time, > ensure that ftrace_enabled is set and part of the test environment > configuration that is saved and restored when running the selftests. > > Signed-off-by: Joe Lawrence > Signed-off-by: Miroslav Benes [...] > diff --git a/tools/testing/selftests/livepatch/test-ftrace.sh > b/tools/testing/selftests/livepatch/test-ftrace.sh > new file mode 100755 > index ..e2a76887f40a > --- /dev/null > +++ b/tools/testing/selftests/livepatch/test-ftrace.sh This test fails due to wrong file permissions, with the warning: # Warning: file test-ftrace.sh is not executable, correct this. not ok 4 selftests: livepatch: test-ftrace.sh -- Kamalesh
Re: [PATCH v2] ftrace: Introduce PERMANENT ftrace_ops flag
On 10/14/19 4:29 PM, Miroslav Benes wrote:
> Livepatch uses ftrace for redirection to new patched functions. It means
> that if ftrace is disabled, all live patched functions are disabled as
> well. Toggling global 'ftrace_enabled' sysctl thus affect it directly.
> It is not a problem per se, because only administrator can set sysctl
> values, but it still may be surprising.
>
> Introduce PERMANENT ftrace_ops flag to amend this. If the
> FTRACE_OPS_FL_PERMANENT is set on any ftrace ops, the tracing cannot be
> disabled by disabling ftrace_enabled. Equally, a callback with the flag
> set cannot be registered if ftrace_enabled is disabled.
>
> Signed-off-by: Miroslav Benes
The patch looks good to me. A minor typo in flag description below.
Reviewed-by: Kamalesh Babulal
[...]
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index 8a8cb3c401b2..c2cad29dc557 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -142,6 +142,8 @@ ftrace_func_t ftrace_ops_get_func(struct ftrace_ops *ops);
> * PID - Is affected by set_ftrace_pid (allows filtering on those pids)
> * RCU - Set when the ops can only be called when RCU is watching.
> * TRACE_ARRAY - The ops->private points to a trace_array descriptor.
> + * PERMAMENT - Set when the ops is permanent and should not be affected by
> + * ftrace_enabled.
> */
s/PERMAMENT/PERMANENT
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 62a50bf399d6..d2992ea29fe1 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -6752,12 +6764,19 @@ ftrace_enable_sysctl(struct ctl_table *table, int
> write,
> ftrace_startup_sysctl();
>
> } else {
> + if (is_permanent_ops_registered()) {
> + ftrace_enabled = last_ftrace_enabled;
> + ret = -EBUSY;
> + goto out;
> + }
> +
> /* stopping ftrace calls (just send to ftrace_stub) */
> ftrace_trace_function = ftrace_stub;
>
> ftrace_shutdown_sysctl();
> }
>
> + last_ftrace_enabled = !!ftrace_enabled;
No strong feelings on last_ftrace_enabled placement, leaving it to
your preference.
> out:
> mutex_unlock(_lock);
> return ret;
>
--
Kamalesh
Re: [PATCH v4 3/3] livepatch: Remove duplicate warning about missing reliable stacktrace support
On Tue, Jun 11, 2019 at 04:13:20PM +0200, Miroslav Benes wrote: > From: Petr Mladek > > WARN_ON_ONCE() could not be called safely under rq lock because > of console deadlock issues. Moreover WARN_ON_ONCE() is superfluous in > klp_check_stack(), because stack_trace_save_tsk_reliable() cannot return > -ENOSYS thanks to klp_have_reliable_stack() check in > klp_try_switch_task(). > > Signed-off-by: Petr Mladek > [ mbenes: changelog edited ] > Signed-off-by: Miroslav Benes Reviewed-by: Kamalesh Babulal
Re: [PATCH v4 2/3] Revert "livepatch: Remove reliable stacktrace check in klp_try_switch_task()"
On Tue, Jun 11, 2019 at 04:13:19PM +0200, Miroslav Benes wrote:
> This reverts commit 1d98a69e5cef3aeb68bcefab0e67e342d6bb4dad. Commit
> 31adf2308f33 ("livepatch: Convert error about unsupported reliable
> stacktrace into a warning") weakened the enforcement for architectures
> to have reliable stack traces support. The system only warns now about
> it.
>
> It only makes sense to reintroduce the compile time checking in
> klp_try_switch_task() again and bail out early.
>
> Signed-off-by: Miroslav Benes
Reviewed-by: Kamalesh Babulal
Re: [PATCH v4 1/3] stacktrace: Remove weak version of save_stack_trace_tsk_reliable()
On Tue, Jun 11, 2019 at 04:13:18PM +0200, Miroslav Benes wrote:
> Recent rework of stack trace infrastructure introduced a new set of
> helpers for common stack trace operations (commit e9b98e162aa5
> ("stacktrace: Provide helpers for common stack trace operations") and
> related). As a result, save_stack_trace_tsk_reliable() is not directly
> called anywhere. Livepatch, currently the only user of the reliable
> stack trace feature, now calls stack_trace_save_tsk_reliable().
>
> When CONFIG_HAVE_RELIABLE_STACKTRACE is set and depending on
> CONFIG_ARCH_STACKWALK, stack_trace_save_tsk_reliable() calls either
> arch_stack_walk_reliable() or mentioned save_stack_trace_tsk_reliable().
> x86_64 defines the former, ppc64le the latter. All other architectures
> do not have HAVE_RELIABLE_STACKTRACE and include/linux/stacktrace.h
> defines -ENOSYS returning version for them.
>
> In short, stack_trace_save_tsk_reliable() returning -ENOSYS defined in
> include/linux/stacktrace.h serves the same purpose as the old weak
> version of save_stack_trace_tsk_reliable() which is therefore no longer
> needed.
>
> Cc: Thomas Gleixner
> Signed-off-by: Miroslav Benes
Reviewed-by: Kamalesh Babulal
Re: [PATCH] stacktrace: fix CONFIG_ARCH_STACKWALK stack_trace_save_tsk_reliable return
On Fri, May 17, 2019 at 02:51:17PM -0400, Joe Lawrence wrote:
> Miroslav reported that the livepatch self-tests were failing,
> specifically a case in which the consistency model ensures that we do
> not patch a current executing function, "TEST: busy target module".
>
> Recent renovations to stack_trace_save_tsk_reliable() left it returning
> only an -ERRNO success indication in some configuration combinations:
>
> klp_check_stack()
> ret = stack_trace_save_tsk_reliable()
> #ifdef CONFIG_ARCH_STACKWALK && CONFIG_HAVE_RELIABLE_STACKTRACE
> stack_trace_save_tsk_reliable()
> ret = arch_stack_walk_reliable()
> return 0
> return -EINVAL
> ...
> return ret;
> ...
> if (ret < 0)
> /* stack_trace_save_tsk_reliable error */
> nr_entries = ret; << 0
>
> Previously (and currently for !CONFIG_ARCH_STACKWALK &&
> CONFIG_HAVE_RELIABLE_STACKTRACE) stack_trace_save_tsk_reliable()
> returned the number of entries that it consumed in the passed storage
> array.
>
> In the case of the above config and trace, be sure to return the
> stacktrace_cookie.len on stack_trace_save_tsk_reliable() success.
>
> Fixes: 25e39e32b0a3f ("livepatch: Simplify stack trace retrieval")
> Reported-by: Miroslav Benes
> Signed-off-by: Joe Lawrence
Reviewed-by: Kamalesh Babulal
Re: [PATCH] livepatch: Remove stale kobj_added entries from kernel-doc descriptions
On Tue, May 07, 2019 at 03:08:14PM +0200, Miroslav Benes wrote:
> Commit 4d141ab3416d ("livepatch: Remove custom kobject state handling")
> removed kobj_added members of klp_func, klp_object and klp_patch
> structures. kernel-doc descriptions were omitted by accident. Remove
> them.
>
> Reported-by: Kamalesh Babulal
> Signed-off-by: Miroslav Benes
Reviewed-by: Kamalesh Babulal
--
Kamalesh
Re: [PATCH 2/2] livepatch: Remove duplicated code for early initialization
On Fri, May 03, 2019 at 03:26:25PM +0200, Petr Mladek wrote: > kobject_init() call added one more operation that has to be > done when doing the early initialization of both static and > dynamic livepatch structures. > > It would have been easier when the early initialization code > was not duplicated. Let's deduplicate it for future generations > of livepatching hackers. > > The patch does not change the existing behavior. > > Signed-off-by: Petr Mladek Reviewed-by: Kamalesh Babulal -- Kamalesh
Re: [PATCH 1/2] livepatch: Remove custom kobject state handling
On Fri, May 03, 2019 at 03:26:24PM +0200, Petr Mladek wrote:
> kobject_init() always succeeds and sets the reference count to 1.
> It allows to always free the structures via kobject_put() and
> the related release callback.
>
> Note that the custom kobject state handling was used only
> because we did not know that kobject_put() can and actually
> should get called even when kobject_init_and_add() fails.
>
> The patch should not change the existing behavior.
>
> Suggested-by: "Tobin C. Harding"
> Signed-off-by: Petr Mladek
> ---
> include/linux/livepatch.h | 3 ---
> kernel/livepatch/core.c | 56
> ++-
> 2 files changed, 17 insertions(+), 42 deletions(-)
>
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 53551f470722..a14bab1a0a3e 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -86,7 +86,6 @@ struct klp_func {
> struct list_head node;
> struct list_head stack_node;
> unsigned long old_size, new_size;
> - bool kobj_added;
> bool nop;
> bool patched;
> bool transition;
Minor nitpick, the description of kobj_added needs to be removed from
structure descriptions.
Reviewed-by: Kamalesh Babulal
--
Kamalesh
Re: [PATCH v2 2/2] livepatch: Use static buffer for debugging messages under rq lock
On Tue, Apr 30, 2019 at 11:10:49AM +0200, Petr Mladek wrote: > klp_try_switch_task() is called under klp_mutex. The buffer for > debugging messages might be static. > > Signed-off-by: Petr Mladek Reviewed-by: Kamalesh Babulal
Re: [PATCH v2 1/2] livepatch: Remove duplicate warning about missing reliable stacktrace support
On Tue, Apr 30, 2019 at 11:10:48AM +0200, Petr Mladek wrote: > WARN_ON_ONCE() could not be called safely under rq lock because > of console deadlock issues. Fortunately, there is another check > for the reliable stacktrace support in klp_enable_patch(). > > Signed-off-by: Petr Mladek Reviewed-by: Kamalesh Babulal
Re: [PATCH 3/3] livepatch: Cleanup message handling in klp_try_switch_task()
On Wed, Apr 24, 2019 at 08:48:58PM +0200, Miroslav Benes wrote:
> On Wed, 24 Apr 2019, Josh Poimboeuf wrote:
>
[...]
> > > ret = save_stack_trace_tsk_reliable(task, );
> > > - WARN_ON_ONCE(ret == -ENOSYS);
> > > + if (ret == -ENOSYS) {
> > > + if (!enosys_warned) {
> > > + printk_deferred(KERN_WARNING "%s:
> > > save_stack_trace_tsk_reliable() not supported on this architecture.\n",
> > > + __func__);
> > > + enosys_warned = 1;
> > > + }
> > > + return ret;
> > > + }
> >
> > We already have a similar printk in patch 1, so is this warning really
> > needed?
>
> I don't think so. pr_warn() in klp_enable_patch() should be enough in my
> opinion.
>
> However,
>
> if (ret == -ENOSYS)
> return ret;
>
> would be justified, wouldn't it?
>
Probably an one line comment on why we return, will be helpful.
--
Kamalesh
Re: [PATCH 2/3] stacktrace: Remove superfluous WARN_ONCE() from save_stack_trace_tsk_reliable()
On Wed, Apr 24, 2019 at 10:55:49AM +0200, Petr Mladek wrote: > WARN_ONCE() in the generic save_stack_trace_tsk_reliable() is superfluous. > The only current user klp_check_stack() writes its own warning when > -ENOSYS is returned. > > Signed-off-by: Petr Mladek Reviewed-by: Kamalesh Babulal -- Kamalesh
Re: [PATCH 1/3] livepatch: Convert error about unsupported reliable stacktrace into a warning
On Wed, Apr 24, 2019 at 10:55:48AM +0200, Petr Mladek wrote:
> The commit d0807da78e11d46f ("livepatch: Remove immediate feature") caused
> that any livepatch was refused when reliable stacktraces were not supported
> on the given architecture.
>
> The limitation is too strong. User space processes are safely migrated
> even when entering or leaving the kernel. Kthreads transition would
> need to get forced. But it is safe when:
>
>+ The livepatch does not change the semantic of the code.
>+ Callbacks do not depend on a safely finished transition.
>
> Suggested-by: Josh Poimboeuf
> Signed-off-by: Petr Mladek
Reviewed-by: Kamalesh Babulal
--
Kamalesh
Re: [PATCH] livepatch: Enforce reliable stack trace as config dependency
On Mon, Feb 11, 2019 at 08:08:13AM -0600, Josh Poimboeuf wrote:
> On Sat, Feb 09, 2019 at 02:47:28PM +0530, Kamalesh Babulal wrote:
> > After removal of the immediate flag by commit d0807da78e11
> > ("livepatch: Remove immediate feature"), reliable stack trace became
> > enforcing dependency for livepatch support on any architecture. In
> > the current code, we ensure that the dependency is met when
> > enabling the patch during the module load.
> >
> > This dependency check can be improved by moving it to the Kconfig,
> > which disables the support for livepatching in the kernel for unmet
> > dependencies. This patch moves both HAVE_RELIABLE_STACKTRACE and
> > STACKTRACE under config LIVEPATCH, it also removes the
> > klp_have_reliable_stack() function.
> >
> > Loading a livepatching module on an architecture where reliable
> > stack trace is yet to be implemented, the user should see:
> >
> > insmod: ERROR: could not insert module ./livepatch-sample.ko: Invalid
> > module format
> >
> > ...
> > [ 286.453463] livepatch_sample: module is marked as livepatch module, but
> > livepatch support is disabled
>
> Wouldn't the module fail to build in the first place, since
> CONFIG_LIVEPATCH is disabled?
Yes, with this patch applied, the livepatch modules will fail to build.
The intention was to paste the output of a module load, marked as the
livepatch but without the reliable stack trace support. I used the
previously compiled livepatch sample module for the illustration but
yeah we would not have a functioning module build in the first place.
>
> Anyway, I'm not sure about this approach. This patch makes the s390
> livepatch code no longer compilable, turning it into completely dead
> code. So if something changes in the s390 code which causes it to stop
> compiling, nobody will notice.
Fair point, we can drop this patch in favor of getting compile time
testing for s390.
>
> I think I'd rather go in the opposite direction: allow the patches to be
> loaded. Then they can be forced, if needed. That enables both compile
> and runtime testing. That way we don't make any backward progress,
> until such arches get reliable stacktraces.
klp_have_reliable_stack() enforces implementation of reliable
stack trace and run time testing will not be complete with we returning
after the check. We can re-think about enforcing the dependency after
we have the reliable stack trace for s390.
--
Kamalesh
[PATCH] livepatch: Enforce reliable stack trace as config dependency
While the consistency model was introduced, architectures without the
reliable stack trace implementation could use the immediate flag for
livepatching but with its own limitations.
After removal of the immediate flag by commit d0807da78e11
("livepatch: Remove immediate feature"), reliable stack trace became
enforcing dependency for livepatch support on any architecture. In
the current code, we ensure that the dependency is met when
enabling the patch during the module load.
This dependency check can be improved by moving it to the Kconfig,
which disables the support for livepatching in the kernel for unmet
dependencies. This patch moves both HAVE_RELIABLE_STACKTRACE and
STACKTRACE under config LIVEPATCH, it also removes the
klp_have_reliable_stack() function.
Loading a livepatching module on an architecture where reliable
stack trace is yet to be implemented, the user should see:
insmod: ERROR: could not insert module ./livepatch-sample.ko: Invalid module
format
...
[ 286.453463] livepatch_sample: module is marked as livepatch module, but
livepatch support is disabled
[pmla...@suse.com: Suggested to explicitly add CONFIG_STACKTRACE under
config LIVEPATCH]
Signed-off-by: Kamalesh Babulal
Cc: Miroslav Benes
Cc: Petr Mladek
Cc: Josh Poimboeuf
Cc: Jiri Kosina
---
Patch is based on a087cdd4073b (origin/for-5.1/atomic-replace) branch
include/linux/livepatch.h | 6 --
kernel/livepatch/Kconfig | 2 ++
kernel/livepatch/core.c | 6 --
3 files changed, 2 insertions(+), 12 deletions(-)
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 53551f470722..7848c7bbffbb 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -214,12 +214,6 @@ static inline bool klp_patch_pending(struct task_struct
*task)
return test_tsk_thread_flag(task, TIF_PATCH_PENDING);
}
-static inline bool klp_have_reliable_stack(void)
-{
- return IS_ENABLED(CONFIG_STACKTRACE) &&
- IS_ENABLED(CONFIG_HAVE_RELIABLE_STACKTRACE);
-}
-
typedef int (*klp_shadow_ctor_t)(void *obj,
void *shadow_data,
void *ctor_data);
diff --git a/kernel/livepatch/Kconfig b/kernel/livepatch/Kconfig
index ec4565122e65..4e4e4fe040f5 100644
--- a/kernel/livepatch/Kconfig
+++ b/kernel/livepatch/Kconfig
@@ -10,6 +10,8 @@ config LIVEPATCH
depends on SYSFS
depends on KALLSYMS_ALL
depends on HAVE_LIVEPATCH
+ depends on STACKTRACE
+ depends on HAVE_RELIABLE_STACKTRACE
depends on !TRIM_UNUSED_KSYMS
help
Say Y here if you want to support kernel live patching.
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index d1af69e9f0e3..a7a00478f6c3 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -1034,12 +1034,6 @@ int klp_enable_patch(struct klp_patch *patch)
if (!klp_initialized())
return -ENODEV;
- if (!klp_have_reliable_stack()) {
- pr_err("This architecture doesn't have support for the
livepatch consistency model.\n");
- return -EOPNOTSUPP;
- }
-
-
mutex_lock(_mutex);
ret = klp_init_patch_early(patch);
--
2.20.1
Re: [PATCH v2] livepatch: core: Return EOPNOTSUPP instead of ENOSYS
On Fri, Feb 08, 2019 at 10:34:45AM +0100, Petr Mladek wrote:
> On Fri 2019-02-08 11:50:05, Kamalesh Babulal wrote:
> > On Wed, Feb 06, 2019 at 11:28:32AM +0100, Petr Mladek wrote:
> > > On Tue 2019-02-05 09:59:33, Josh Poimboeuf wrote:
> > > > On Tue, Feb 05, 2019 at 03:33:28AM +0900, Alice Ferrazzi wrote:
> > > > > From: Alice Ferrazzi
> > > > >
> > > > > As a result of an unsupported operation is better to use EOPNOTSUPP
> > > > > as error code.
> > > > > ENOSYS is only used for 'invalid syscall nr' and nothing else.
> > > > >
> > > > > Signed-off-by: Alice Ferrazzi
> > > >
> > > > Acked-by: Josh Poimboeuf
> > >
> > > I have applied the patch into for-5.1/atomic-replace branch.
[...]
> > After removal of the immediate flag by
> > commit d0807da78e11 ("livepatch: Remove immediate feature"), every
> > architecture enabling livepatching is required to have implemented
> > reliable stack trace. Is it a better idea to make
> > HAVE_RELIABLE_STACKTRACE a config dependency, which will disable
> > livepatching support for architectures without reliable stack trace
> > function during kernel build?
> >
> > The idea is to remove klp_have_reliable_stack() by moving
> > CONFIG_HAVE_RELIABLE_STACKTRACE as a config dependency to Kconfig
> > file
>
> Looks like a nice cleanup.
Thanks
>
> > and adding the other CONFIG_STACKTRACE as a config dependency is not
> > required, as it's selected via CONFIG_DYNAMIC_FTRACE_WITH_REGS
> > dependency chain. With the patch on architecture without
> > HAVE_RELIABLE_STACKTRACE, the user should see:
>
> Hmm, I see the following in kernel/trace/Kconfig:
>
> config TRACING
> [...]
> select STACKTRACE if STACKTRACE_SUPPORT
>
> It seems that the depency is not guaranted. Or do I miss anything?
I should have tried drawing the config dependency of CONFIG_STACKTRACE
in the mail. It would saved me from nagivating wrong indirections in
tricky Kconfigs. You are right, CONFIG_STACKTRACE is not selected via
CONFIG_DYNAMIC_FTRACE_WITH_REGS config dependency chain.
>
> Anyway, it is pretty indirect. I would prefer to add dependency
> on STACKTRACE explicitly into config LIVEPATCH.
Agree, explicitly listing CONFIG_STACKTRACE as one of the dependencies
under config LIVEPATCH will guarantee the dependency is met. On the
curious note, digging through the git logs, CONFIG_STACKTRACE_SUPPORT
is enabled by default on x86/PowerPC for quite long now.
Thanks,
Kamalesh
Re: [PATCH v2] livepatch: core: Return EOPNOTSUPP instead of ENOSYS
Hi Miroslav,
On Fri, Feb 08, 2019 at 10:24:21AM +0100, Miroslav Benes wrote:
> Hi Kamalesh,
>
> On Fri, 8 Feb 2019, Kamalesh Babulal wrote:
>
> > On Wed, Feb 06, 2019 at 11:28:32AM +0100, Petr Mladek wrote:
> > > On Tue 2019-02-05 09:59:33, Josh Poimboeuf wrote:
> > > > On Tue, Feb 05, 2019 at 03:33:28AM +0900, Alice Ferrazzi wrote:
> > > > > From: Alice Ferrazzi
> > > > >
> > > > > As a result of an unsupported operation is better to use EOPNOTSUPP
> > > > > as error code.
> > > > > ENOSYS is only used for 'invalid syscall nr' and nothing else.
[...]
> > After removal of the immediate flag by
> > commit d0807da78e11 ("livepatch: Remove immediate feature"), every
> > architecture enabling livepatching is required to have implemented
> > reliable stack trace. Is it a better idea to make
> > HAVE_RELIABLE_STACKTRACE a config dependency, which will disable
> > livepatching support for architectures without reliable stack trace
> > function during kernel build?
>
> if I am not mistaken, s390x is currently the only one which is supported
> (the redirection works) but has no reliable stacktraces (so far, it is my
> plan to take a look soon).
>
> Theoretically, it could still work. We have the fake signal and we can
> force the remaining tasks (kthreads). It is not something to be used in
> production but it could make sense for a limited testing.
That was my understanding too, s390 doesn't set HAVE_RELIABLE_STACKTRACE.
(below output is right trimmed for readability)
arch $ find . -name 'Kconfig'|xargs egrep -an "HAVE_LIVEPATCH"
./powerpc/Kconfig:209: select HAVE_LIVEPATCH ...
./x86/Kconfig:171: select HAVE_LIVEPATCH ...
./s390/Kconfig:161: select HAVE_LIVEPATCH
arch $ find . -name 'Kconfig'|xargs egrep -an "HAVE_RELIABLE_STACKTRACE"
./powerpc/Kconfig:223: select HAVE_RELIABLE_STACKTRACE ...
./x86/Kconfig:189: select HAVE_RELIABLE_STACKTRACE ...
./Kconfig:690:config HAVE_RELIABLE_STACKTRACE
klp_have_reliable_stack() will guard against loading of livepatching
module on s390, for the same reason being that HAVE_RELIABLE_STACKTRACE
is not set. My explanation is purely based on the above grep output
on Kconfig files, which might be partial. Am I missing something here?
> > The idea is to remove klp_have_reliable_stack() by moving
> > CONFIG_HAVE_RELIABLE_STACKTRACE as a config dependency to Kconfig file
> > and adding the other CONFIG_STACKTRACE as a config dependency is not
> > required, as it's selected via CONFIG_DYNAMIC_FTRACE_WITH_REGS
> > dependency chain. With the patch on architecture without
> > HAVE_RELIABLE_STACKTRACE, the user should see:
[...]
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index fe1993399823..9a80f7574d75 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -1002,12 +1002,6 @@ int klp_enable_patch(struct klp_patch *patch)
> > if (!klp_initialized())
> > return -ENODEV;
> >
> > - if (!klp_have_reliable_stack()) {
> > - pr_err("This architecture doesn't have support for the
> > livepatch consistency model.\n");
> > - return -ENOSYS;
> > - }
> > -
> > -
> > mutex_lock(_mutex);
> >
> > ret = klp_init_patch_early(patch);
>
> On the other hand, I like this change. So we have two options, I think.
> We can apply this and wait if someone complains (because of s390x
> testing), or we can wait for the full support of s390x and then enforce
> it.
Thanks, I am ok with either of the options. We could enforce the config
dependency, in case the above assumption in regard to s390 is correct.
Thanks,
Kamalesh
Re: [PATCH v2] livepatch: core: Return EOPNOTSUPP instead of ENOSYS
On Wed, Feb 06, 2019 at 11:28:32AM +0100, Petr Mladek wrote:
> On Tue 2019-02-05 09:59:33, Josh Poimboeuf wrote:
> > On Tue, Feb 05, 2019 at 03:33:28AM +0900, Alice Ferrazzi wrote:
> > > From: Alice Ferrazzi
> > >
> > > As a result of an unsupported operation is better to use EOPNOTSUPP
> > > as error code.
> > > ENOSYS is only used for 'invalid syscall nr' and nothing else.
> > >
> > > Signed-off-by: Alice Ferrazzi
> >
> > Acked-by: Josh Poimboeuf
>
> I have applied the patch into for-5.1/atomic-replace branch.
Sorry to jump into the discussion so late. Thinking a little more about
the check itself, previously with immediate flag an architecture can do
livepatching with limitations and without the reliable stack trace
implemented yet.
After removal of the immediate flag by
commit d0807da78e11 ("livepatch: Remove immediate feature"), every
architecture enabling livepatching is required to have implemented
reliable stack trace. Is it a better idea to make
HAVE_RELIABLE_STACKTRACE a config dependency, which will disable
livepatching support for architectures without reliable stack trace
function during kernel build?
The idea is to remove klp_have_reliable_stack() by moving
CONFIG_HAVE_RELIABLE_STACKTRACE as a config dependency to Kconfig file
and adding the other CONFIG_STACKTRACE as a config dependency is not
required, as it's selected via CONFIG_DYNAMIC_FTRACE_WITH_REGS
dependency chain. With the patch on architecture without
HAVE_RELIABLE_STACKTRACE, the user should see:
# insmod ./livepatch-sample.ko
insmod: ERROR: could not insert module ./livepatch-sample.ko: Invalid module
format
# dmesg
...
[ 286.453463] livepatch_sample: module is marked as livepatch module, but
livepatch support is disabled
I have done limited testing on PowerPC and to test the unsupported case,
the config dependency HAVE_RELIABLE_STACKTRACE was misspelled in Kconfig
file. If the idea sounds ok I will send a formal patch.
---8<
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 53551f470722..7848c7bbffbb 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -214,12 +214,6 @@ static inline bool klp_patch_pending(struct task_struct
*task)
return test_tsk_thread_flag(task, TIF_PATCH_PENDING);
}
-static inline bool klp_have_reliable_stack(void)
-{
- return IS_ENABLED(CONFIG_STACKTRACE) &&
- IS_ENABLED(CONFIG_HAVE_RELIABLE_STACKTRACE);
-}
-
typedef int (*klp_shadow_ctor_t)(void *obj,
void *shadow_data,
void *ctor_data);
diff --git a/kernel/livepatch/Kconfig b/kernel/livepatch/Kconfig
index ec4565122e65..16b3692ddf9f 100644
--- a/kernel/livepatch/Kconfig
+++ b/kernel/livepatch/Kconfig
@@ -9,6 +9,7 @@ config LIVEPATCH
depends on MODULES
depends on SYSFS
depends on KALLSYMS_ALL
+ depends on HAVE_RELIABLE_STACKTRACE
depends on HAVE_LIVEPATCH
depends on !TRIM_UNUSED_KSYMS
help
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index fe1993399823..9a80f7574d75 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -1002,12 +1002,6 @@ int klp_enable_patch(struct klp_patch *patch)
if (!klp_initialized())
return -ENODEV;
- if (!klp_have_reliable_stack()) {
- pr_err("This architecture doesn't have support for the
livepatch consistency model.\n");
- return -ENOSYS;
- }
-
-
mutex_lock(_mutex);
ret = klp_init_patch_early(patch);
[PATCH] static_keys.txt: Fix trivial spelling mistake
Fix the spelling of 'functionnality' -> 'functionality'. Signed-off-by: Kamalesh Babulal --- Documentation/static-keys.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/static-keys.txt b/Documentation/static-keys.txt index d68135560895..9803e14639bf 100644 --- a/Documentation/static-keys.txt +++ b/Documentation/static-keys.txt @@ -159,7 +159,7 @@ particularly the CPU hotplug lock (in order to avoid races against CPUs being brought in the kernel while the kernel is getting patched). Calling the static key API from within a hotplug notifier is thus a sure deadlock recipe. In order to still allow use of the -functionnality, the following functions are provided: +functionality, the following functions are provided: static_key_enable_cpuslocked() static_key_disable_cpuslocked() -- 2.20.1
[PATCH] livepatch: Validate module/old func name length
livepatch module author can pass module name/old function name with more than the defined character limit. With obj->name length greater than MODULE_NAME_LEN, the livepatch module gets loaded but waits forever on the module specified by obj->name to be loaded. It also populates a /sys directory with an untruncated object name. In the case of funcs->old_name length greater then KSYM_NAME_LEN, it would not match against any of the symbol table entries. Instead loop through the symbol table comparing them against a nonexisting function, which can be avoided. The same issues apply, to misspelled/incorrect names. At least gatekeep the modules with over the limit string length, by checking for their length during livepatch module registration. Signed-off-by: Kamalesh Babulal --- kernel/livepatch/core.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c index 3a4656fb7047..5b77a7314e01 100644 --- a/kernel/livepatch/core.c +++ b/kernel/livepatch/core.c @@ -678,6 +678,9 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func) if (!func->old_name || !func->new_func) return -EINVAL; + if (strlen(func->old_name) >= KSYM_NAME_LEN) + return -EINVAL; + INIT_LIST_HEAD(>stack_node); func->patched = false; func->transition = false; @@ -751,6 +754,9 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj) if (!obj->funcs) return -EINVAL; + if (klp_is_module(obj) && strlen(obj->name) >= MODULE_NAME_LEN) + return -EINVAL; + obj->patched = false; obj->mod = NULL; -- 2.7.4
[PATCH] livepatch: Validate module/old func name length
livepatch module author can pass module name/old function name with more than the defined character limit. With obj->name length greater than MODULE_NAME_LEN, the livepatch module gets loaded but waits forever on the module specified by obj->name to be loaded. It also populates a /sys directory with an untruncated object name. In the case of funcs->old_name length greater then KSYM_NAME_LEN, it would not match against any of the symbol table entries. Instead loop through the symbol table comparing them against a nonexisting function, which can be avoided. The same issues apply, to misspelled/incorrect names. At least gatekeep the modules with over the limit string length, by checking for their length during livepatch module registration. Signed-off-by: Kamalesh Babulal --- kernel/livepatch/core.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c index 3a4656fb7047..5b77a7314e01 100644 --- a/kernel/livepatch/core.c +++ b/kernel/livepatch/core.c @@ -678,6 +678,9 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func) if (!func->old_name || !func->new_func) return -EINVAL; + if (strlen(func->old_name) >= KSYM_NAME_LEN) + return -EINVAL; + INIT_LIST_HEAD(>stack_node); func->patched = false; func->transition = false; @@ -751,6 +754,9 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj) if (!obj->funcs) return -EINVAL; + if (klp_is_module(obj) && strlen(obj->name) >= MODULE_NAME_LEN) + return -EINVAL; + obj->patched = false; obj->mod = NULL; -- 2.7.4
[PATCH v2] doc: livepatch: fix minor typo/grammar in shadow-vars.txt
Fix the spelling of 'varibles' -> 'variables' and grammar in shadows->vars.txt file. Signed-off-by: Kamalesh Babulal Cc: Josh Poimboeuf --- v2 changes: - Fixed the grammar of the sentence as suggested by Josh Documentation/livepatch/shadow-vars.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/livepatch/shadow-vars.txt b/Documentation/livepatch/shadow-vars.txt index ecc09a7be5dd..ffbc117e46f0 100644 --- a/Documentation/livepatch/shadow-vars.txt +++ b/Documentation/livepatch/shadow-vars.txt @@ -150,7 +150,7 @@ In-flight parent objects Sometimes it may not be convenient or possible to allocate shadow variables alongside their parent objects. Or a livepatch fix may -require shadow varibles to only a subset of parent object instances. In +require shadow variables for only a subset of parent object instances. In these cases, the klp_shadow_get_or_alloc() call can be used to attach shadow variables to parents already in-flight. -- 2.7.4
[PATCH v2] doc: livepatch: fix minor typo/grammar in shadow-vars.txt
Fix the spelling of 'varibles' -> 'variables' and grammar in shadows->vars.txt file. Signed-off-by: Kamalesh Babulal Cc: Josh Poimboeuf --- v2 changes: - Fixed the grammar of the sentence as suggested by Josh Documentation/livepatch/shadow-vars.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/livepatch/shadow-vars.txt b/Documentation/livepatch/shadow-vars.txt index ecc09a7be5dd..ffbc117e46f0 100644 --- a/Documentation/livepatch/shadow-vars.txt +++ b/Documentation/livepatch/shadow-vars.txt @@ -150,7 +150,7 @@ In-flight parent objects Sometimes it may not be convenient or possible to allocate shadow variables alongside their parent objects. Or a livepatch fix may -require shadow varibles to only a subset of parent object instances. In +require shadow variables for only a subset of parent object instances. In these cases, the klp_shadow_get_or_alloc() call can be used to attach shadow variables to parents already in-flight. -- 2.7.4
[PATCH] doc: livepatch: fix minor typo in shadow-vars.txt
Fix the spelling of 'varibles' -> 'variables' in shadows->vars.txt file. Signed-off-by: Kamalesh Babulal --- Documentation/livepatch/shadow-vars.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/livepatch/shadow-vars.txt b/Documentation/livepatch/shadow-vars.txt index ecc09a7be5dd..f69c6261e951 100644 --- a/Documentation/livepatch/shadow-vars.txt +++ b/Documentation/livepatch/shadow-vars.txt @@ -150,7 +150,7 @@ In-flight parent objects Sometimes it may not be convenient or possible to allocate shadow variables alongside their parent objects. Or a livepatch fix may -require shadow varibles to only a subset of parent object instances. In +require shadow variables to only a subset of parent object instances. In these cases, the klp_shadow_get_or_alloc() call can be used to attach shadow variables to parents already in-flight. -- 2.7.4
[PATCH] doc: livepatch: fix minor typo in shadow-vars.txt
Fix the spelling of 'varibles' -> 'variables' in shadows->vars.txt file. Signed-off-by: Kamalesh Babulal --- Documentation/livepatch/shadow-vars.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/livepatch/shadow-vars.txt b/Documentation/livepatch/shadow-vars.txt index ecc09a7be5dd..f69c6261e951 100644 --- a/Documentation/livepatch/shadow-vars.txt +++ b/Documentation/livepatch/shadow-vars.txt @@ -150,7 +150,7 @@ In-flight parent objects Sometimes it may not be convenient or possible to allocate shadow variables alongside their parent objects. Or a livepatch fix may -require shadow varibles to only a subset of parent object instances. In +require shadow variables to only a subset of parent object instances. In these cases, the klp_shadow_get_or_alloc() call can be used to attach shadow variables to parents already in-flight. -- 2.7.4
Re: [PATCH] livepatch: Remove reliable stacktrace check in klp_try_switch_task()
On Saturday 14 July 2018 12:07 AM, Josh Poimboeuf wrote: We bail out during patch registration for architectures, those don't support reliable stack trace. Does anybody know if that change was intentional? I thought the plan was to allow non-consistency-model arches to still use livepatch, and that they'd just have to 'force' patches to completion instead. That seems a little more forgiving. The initial proposal was to allow 'force' feature on architectures without HAVE_RELIABLE_STACKTRACE support and use pr_notice() to warn user about the non-availability of consistency model. It was argued against, as it will encourage people to use it as an alternative instead of adding HAVE_RELIABLE_STACKTRACE support to the kernel. -- cheers, Kamalesh.
Re: [PATCH] livepatch: Remove reliable stacktrace check in klp_try_switch_task()
On Saturday 14 July 2018 12:07 AM, Josh Poimboeuf wrote: We bail out during patch registration for architectures, those don't support reliable stack trace. Does anybody know if that change was intentional? I thought the plan was to allow non-consistency-model arches to still use livepatch, and that they'd just have to 'force' patches to completion instead. That seems a little more forgiving. The initial proposal was to allow 'force' feature on architectures without HAVE_RELIABLE_STACKTRACE support and use pr_notice() to warn user about the non-availability of consistency model. It was argued against, as it will encourage people to use it as an alternative instead of adding HAVE_RELIABLE_STACKTRACE support to the kernel. -- cheers, Kamalesh.
[PATCH] livepatch: Remove reliable stacktrace check in klp_try_switch_task()
Support for immediate flag was removed by commit d0807da78e11
("livepatch: Remove immediate feature"). We bail out during
patch registration for architectures, those don't support
reliable stack trace. Remove the check in klp_try_switch_task(),
as its not required.
Signed-off-by: Kamalesh Babulal
---
kernel/livepatch/transition.c | 7 ---
1 file changed, 7 deletions(-)
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index 7c6631e693bc..5bc349805e03 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -310,13 +310,6 @@ static bool klp_try_switch_task(struct task_struct *task)
return true;
/*
-* For arches which don't have reliable stack traces, we have to rely
-* on other methods (e.g., switching tasks at kernel exit).
-*/
- if (!klp_have_reliable_stack())
- return false;
-
- /*
* Now try to check the stack for any to-be-patched or to-be-unpatched
* functions. If all goes well, switch the task to the target patch
* state.
--
2.7.4
[PATCH] livepatch: Remove reliable stacktrace check in klp_try_switch_task()
Support for immediate flag was removed by commit d0807da78e11
("livepatch: Remove immediate feature"). We bail out during
patch registration for architectures, those don't support
reliable stack trace. Remove the check in klp_try_switch_task(),
as its not required.
Signed-off-by: Kamalesh Babulal
---
kernel/livepatch/transition.c | 7 ---
1 file changed, 7 deletions(-)
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index 7c6631e693bc..5bc349805e03 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -310,13 +310,6 @@ static bool klp_try_switch_task(struct task_struct *task)
return true;
/*
-* For arches which don't have reliable stack traces, we have to rely
-* on other methods (e.g., switching tasks at kernel exit).
-*/
- if (!klp_have_reliable_stack())
- return false;
-
- /*
* Now try to check the stack for any to-be-patched or to-be-unpatched
* functions. If all goes well, switch the task to the target patch
* state.
--
2.7.4
Re: [PATCH] powerpc/modules: remove unused mod_arch_specific.toc field
On Friday 25 May 2018 09:18 AM, Josh Poimboeuf wrote: The toc field in the mod_arch_specific struct isn't actually used anywhere, so remove it. Also the ftrace-specific fields are now common between 32-bit and 64-bit, so simplify the struct definition a bit by moving them out of the __powerpc64__ #ifdef. Signed-off-by: Josh Poimboeuf <jpoim...@redhat.com> Reviewed-by: Kamalesh Babulal <kamal...@linux.vnet.ibm.com>
Re: [PATCH] powerpc/modules: remove unused mod_arch_specific.toc field
On Friday 25 May 2018 09:18 AM, Josh Poimboeuf wrote: The toc field in the mod_arch_specific struct isn't actually used anywhere, so remove it. Also the ftrace-specific fields are now common between 32-bit and 64-bit, so simplify the struct definition a bit by moving them out of the __powerpc64__ #ifdef. Signed-off-by: Josh Poimboeuf Reviewed-by: Kamalesh Babulal
Re: [PATCH v4.2] powerpc/modules: Don't try to restore r2 after a sibling call
On Thursday 16 November 2017 11:15 PM, Josh Poimboeuf wrote:
On Thu, Nov 16, 2017 at 06:39:03PM +0530, Naveen N. Rao wrote:
Josh Poimboeuf wrote:
On Wed, Nov 15, 2017 at 02:58:33PM +0530, Naveen N. Rao wrote:
+int instr_is_link_branch(unsigned int instr)
+{
+ return (instr_is_branch_iform(instr) || instr_is_branch_bform(instr)) &&
+ (instr & BRANCH_SET_LINK);
+}
+
Nitpicking here, but since we're not considering the other branch forms,
perhaps this can be renamed to instr_is_link_relative_branch() (or maybe
instr_is_relative_branch_link()), just so we're clear :)
My understanding is that the absolute/relative bit isn't a "form", but
rather a bit that can be set for either the b-form (conditional) or the
i-form (unconditional). And the above function isn't checking the
absolute bit, so it isn't necessarily a relative branch. Or did I miss
something?
Ah, good point. I was coming from the fact that we are only considering the
i-form and b-form branches and not the lr/ctr/tar based branches, which are
always absolute branches, but can also set the link register.
Hm, RISC is more complicated than I realized ;-)
Thinking about this more, aren't we only interested in relative branches
here (for relocations), so can we actually filter out the absolute branches?
Something like this?
int instr_is_relative_branch_link(unsigned int instr)
{
return ((instr_is_branch_iform(instr) || instr_is_branch_bform(instr))
&&
!(instr & BRANCH_ABSOLUTE) && (instr & BRANCH_SET_LINK));
Yeah, makes sense to me. Here's another try (also untested). If this
looks ok, Kamalesh would you mind testing again?
8<
From: Josh Poimboeuf <jpoim...@redhat.com>
Subject: [PATCH v4.2] powerpc/modules: Don't try to restore r2 after a sibling
call
When attempting to load a livepatch module, I got the following error:
module_64: patch_module: Expect noop after relocate, got 3c82
The error was triggered by the following code in
unregister_netdevice_queue():
14c: 00 00 00 48 b 14c <unregister_netdevice_queue+0x14c>
14c: R_PPC64_REL24 net_set_todo
150: 00 00 82 3c addis r4,r2,0
GCC didn't insert a nop after the branch to net_set_todo() because it's
a sibling call, so it never returns. The nop isn't needed after the
branch in that case.
Signed-off-by: Josh Poimboeuf <jpoim...@redhat.com>
Reviewed-and-tested-by: Kamalesh Babulal <kamal...@linux.vnet.ibm.com>
---
arch/powerpc/include/asm/code-patching.h | 1 +
arch/powerpc/kernel/module_64.c | 12 +++-
arch/powerpc/lib/code-patching.c | 5 +
3 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/include/asm/code-patching.h
b/arch/powerpc/include/asm/code-patching.h
index abef812de7f8..2c895e8d07f7 100644
--- a/arch/powerpc/include/asm/code-patching.h
+++ b/arch/powerpc/include/asm/code-patching.h
@@ -33,6 +33,7 @@ int patch_branch(unsigned int *addr, unsigned long target,
int flags);
int patch_instruction(unsigned int *addr, unsigned int instr);
int instr_is_relative_branch(unsigned int instr);
+int instr_is_relative_link_branch(unsigned int instr);
int instr_is_branch_to_addr(const unsigned int *instr, unsigned long addr);
unsigned long branch_target(const unsigned int *instr);
unsigned int translate_branch(const unsigned int *dest,
diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index 759104b99f9f..180c16f04063 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -487,7 +487,17 @@ static bool is_early_mcount_callsite(u32 *instruction)
restore r2. */
static int restore_r2(u32 *instruction, struct module *me)
{
- if (is_early_mcount_callsite(instruction - 1))
+ u32 *prev_insn = instruction - 1;
+
+ if (is_early_mcount_callsite(prev_insn))
+ return 1;
+
+ /*
+* Make sure the branch isn't a sibling call. Sibling calls aren't
+* "link" branches and they don't return, so they don't need the r2
+* restore afterwards.
+*/
+ if (!instr_is_relative_link_branch(*prev_insn))
return 1;
if (*instruction != PPC_INST_NOP) {
diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
index c9de03e0c1f1..d81aab7441f7 100644
--- a/arch/powerpc/lib/code-patching.c
+++ b/arch/powerpc/lib/code-patching.c
@@ -304,6 +304,11 @@ int instr_is_relative_branch(unsigned int instr)
return instr_is_branch_iform(instr) || instr_is_branch_bform(instr);
}
+int instr_is_relative_link_branch(unsigned int instr)
+{
+ return instr_is_relative_branch(instr) && (instr & BRANCH_SET_LINK);
+}
+
static unsigned long branch_iform_target(const unsigned int *instr)
{
signed long imm;
Re: [PATCH v4.2] powerpc/modules: Don't try to restore r2 after a sibling call
On Thursday 16 November 2017 11:15 PM, Josh Poimboeuf wrote:
On Thu, Nov 16, 2017 at 06:39:03PM +0530, Naveen N. Rao wrote:
Josh Poimboeuf wrote:
On Wed, Nov 15, 2017 at 02:58:33PM +0530, Naveen N. Rao wrote:
+int instr_is_link_branch(unsigned int instr)
+{
+ return (instr_is_branch_iform(instr) || instr_is_branch_bform(instr)) &&
+ (instr & BRANCH_SET_LINK);
+}
+
Nitpicking here, but since we're not considering the other branch forms,
perhaps this can be renamed to instr_is_link_relative_branch() (or maybe
instr_is_relative_branch_link()), just so we're clear :)
My understanding is that the absolute/relative bit isn't a "form", but
rather a bit that can be set for either the b-form (conditional) or the
i-form (unconditional). And the above function isn't checking the
absolute bit, so it isn't necessarily a relative branch. Or did I miss
something?
Ah, good point. I was coming from the fact that we are only considering the
i-form and b-form branches and not the lr/ctr/tar based branches, which are
always absolute branches, but can also set the link register.
Hm, RISC is more complicated than I realized ;-)
Thinking about this more, aren't we only interested in relative branches
here (for relocations), so can we actually filter out the absolute branches?
Something like this?
int instr_is_relative_branch_link(unsigned int instr)
{
return ((instr_is_branch_iform(instr) || instr_is_branch_bform(instr))
&&
!(instr & BRANCH_ABSOLUTE) && (instr & BRANCH_SET_LINK));
Yeah, makes sense to me. Here's another try (also untested). If this
looks ok, Kamalesh would you mind testing again?
8<
From: Josh Poimboeuf
Subject: [PATCH v4.2] powerpc/modules: Don't try to restore r2 after a sibling
call
When attempting to load a livepatch module, I got the following error:
module_64: patch_module: Expect noop after relocate, got 3c82
The error was triggered by the following code in
unregister_netdevice_queue():
14c: 00 00 00 48 b 14c
14c: R_PPC64_REL24 net_set_todo
150: 00 00 82 3c addis r4,r2,0
GCC didn't insert a nop after the branch to net_set_todo() because it's
a sibling call, so it never returns. The nop isn't needed after the
branch in that case.
Signed-off-by: Josh Poimboeuf
Reviewed-and-tested-by: Kamalesh Babulal
---
arch/powerpc/include/asm/code-patching.h | 1 +
arch/powerpc/kernel/module_64.c | 12 +++-
arch/powerpc/lib/code-patching.c | 5 +
3 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/include/asm/code-patching.h
b/arch/powerpc/include/asm/code-patching.h
index abef812de7f8..2c895e8d07f7 100644
--- a/arch/powerpc/include/asm/code-patching.h
+++ b/arch/powerpc/include/asm/code-patching.h
@@ -33,6 +33,7 @@ int patch_branch(unsigned int *addr, unsigned long target,
int flags);
int patch_instruction(unsigned int *addr, unsigned int instr);
int instr_is_relative_branch(unsigned int instr);
+int instr_is_relative_link_branch(unsigned int instr);
int instr_is_branch_to_addr(const unsigned int *instr, unsigned long addr);
unsigned long branch_target(const unsigned int *instr);
unsigned int translate_branch(const unsigned int *dest,
diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index 759104b99f9f..180c16f04063 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -487,7 +487,17 @@ static bool is_early_mcount_callsite(u32 *instruction)
restore r2. */
static int restore_r2(u32 *instruction, struct module *me)
{
- if (is_early_mcount_callsite(instruction - 1))
+ u32 *prev_insn = instruction - 1;
+
+ if (is_early_mcount_callsite(prev_insn))
+ return 1;
+
+ /*
+* Make sure the branch isn't a sibling call. Sibling calls aren't
+* "link" branches and they don't return, so they don't need the r2
+* restore afterwards.
+*/
+ if (!instr_is_relative_link_branch(*prev_insn))
return 1;
if (*instruction != PPC_INST_NOP) {
diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
index c9de03e0c1f1..d81aab7441f7 100644
--- a/arch/powerpc/lib/code-patching.c
+++ b/arch/powerpc/lib/code-patching.c
@@ -304,6 +304,11 @@ int instr_is_relative_branch(unsigned int instr)
return instr_is_branch_iform(instr) || instr_is_branch_bform(instr);
}
+int instr_is_relative_link_branch(unsigned int instr)
+{
+ return instr_is_relative_branch(instr) && (instr & BRANCH_SET_LINK);
+}
+
static unsigned long branch_iform_target(const unsigned int *instr)
{
signed long imm;
Re: [PATCH v4 2/3] powerpc/modules: Don't try to restore r2 after a sibling call
On Tuesday 14 November 2017 09:23 PM, Josh Poimboeuf wrote:
On Tue, Nov 14, 2017 at 03:59:21PM +0530, Naveen N. Rao wrote:
Kamalesh Babulal wrote:
From: Josh Poimboeuf <jpoim...@redhat.com>
When attempting to load a livepatch module, I got the following error:
module_64: patch_module: Expect noop after relocate, got 3c82
The error was triggered by the following code in
unregister_netdevice_queue():
14c: 00 00 00 48 b 14c <unregister_netdevice_queue+0x14c>
14c: R_PPC64_REL24 net_set_todo
150: 00 00 82 3c addis r4,r2,0
GCC didn't insert a nop after the branch to net_set_todo() because it's
a sibling call, so it never returns. The nop isn't needed after the
branch in that case.
Signed-off-by: Josh Poimboeuf <jpoim...@redhat.com>
Signed-off-by: Kamalesh Babulal <kamal...@linux.vnet.ibm.com>
---
arch/powerpc/kernel/module_64.c | 4
1 file changed, 4 insertions(+)
diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index 39b01fd..9e5391f 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -489,6 +489,10 @@ static int restore_r2(u32 *instruction, struct module *me)
if (is_early_mcount_callsite(instruction - 1))
return 1;
+ /* Sibling calls don't return, so they don't need to restore r2 */
+ if (instruction[-1] == PPC_INST_BRANCH)
+ return 1;
+
This looks quite fragile, unless we know for sure that gcc will _always_
emit this instruction form for sibling calls with relocations.
As an alternative, does it make sense to do the following check instead?
if ((instr_is_branch_iform(insn) || instr_is_branch_bform(insn))
&& !(insn & 0x1))
Yes, good point. How about something like this?
(completely untested because I don't have access to a box at the moment)
Reviewed-and-tested-by: Kamalesh Babulal <kamal...@linux.vnet.ibm.com>
diff --git a/arch/powerpc/include/asm/code-patching.h
b/arch/powerpc/include/asm/code-patching.h
index abef812de7f8..302e4368debc 100644
--- a/arch/powerpc/include/asm/code-patching.h
+++ b/arch/powerpc/include/asm/code-patching.h
@@ -33,6 +33,7 @@ int patch_branch(unsigned int *addr, unsigned long target,
int flags);
int patch_instruction(unsigned int *addr, unsigned int instr);
int instr_is_relative_branch(unsigned int instr);
+int instr_is_link_branch(unsigned int instr);
int instr_is_branch_to_addr(const unsigned int *instr, unsigned long addr);
unsigned long branch_target(const unsigned int *instr);
unsigned int translate_branch(const unsigned int *dest,
diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index 9cb007bc7075..b5148a206b4d 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -487,11 +487,13 @@ static bool is_early_mcount_callsite(u32 *instruction)
restore r2. */
static int restore_r2(u32 *instruction, struct module *me)
{
- if (is_early_mcount_callsite(instruction - 1))
+ u32 *prev_insn = instruction - 1;
+
+ if (is_early_mcount_callsite(prev_insn))
return 1;
/* Sibling calls don't return, so they don't need to restore r2 */
- if (instruction[-1] == PPC_INST_BRANCH)
+ if (!instr_is_link_branch(*prev_insn))
return 1;
if (*instruction != PPC_INST_NOP) {
diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
index c9de03e0c1f1..4727fafd37e4 100644
--- a/arch/powerpc/lib/code-patching.c
+++ b/arch/powerpc/lib/code-patching.c
@@ -304,6 +304,12 @@ int instr_is_relative_branch(unsigned int instr)
return instr_is_branch_iform(instr) || instr_is_branch_bform(instr);
}
+int instr_is_link_branch(unsigned int instr)
+{
+ return (instr_is_branch_iform(instr) || instr_is_branch_bform(instr)) &&
+ (instr & BRANCH_SET_LINK);
+}
+
static unsigned long branch_iform_target(const unsigned int *instr)
{
signed long imm;
--
cheers,
Kamalesh.
Re: [PATCH v4 2/3] powerpc/modules: Don't try to restore r2 after a sibling call
On Tuesday 14 November 2017 09:23 PM, Josh Poimboeuf wrote:
On Tue, Nov 14, 2017 at 03:59:21PM +0530, Naveen N. Rao wrote:
Kamalesh Babulal wrote:
From: Josh Poimboeuf
When attempting to load a livepatch module, I got the following error:
module_64: patch_module: Expect noop after relocate, got 3c82
The error was triggered by the following code in
unregister_netdevice_queue():
14c: 00 00 00 48 b 14c
14c: R_PPC64_REL24 net_set_todo
150: 00 00 82 3c addis r4,r2,0
GCC didn't insert a nop after the branch to net_set_todo() because it's
a sibling call, so it never returns. The nop isn't needed after the
branch in that case.
Signed-off-by: Josh Poimboeuf
Signed-off-by: Kamalesh Babulal
---
arch/powerpc/kernel/module_64.c | 4
1 file changed, 4 insertions(+)
diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index 39b01fd..9e5391f 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -489,6 +489,10 @@ static int restore_r2(u32 *instruction, struct module *me)
if (is_early_mcount_callsite(instruction - 1))
return 1;
+ /* Sibling calls don't return, so they don't need to restore r2 */
+ if (instruction[-1] == PPC_INST_BRANCH)
+ return 1;
+
This looks quite fragile, unless we know for sure that gcc will _always_
emit this instruction form for sibling calls with relocations.
As an alternative, does it make sense to do the following check instead?
if ((instr_is_branch_iform(insn) || instr_is_branch_bform(insn))
&& !(insn & 0x1))
Yes, good point. How about something like this?
(completely untested because I don't have access to a box at the moment)
Reviewed-and-tested-by: Kamalesh Babulal
diff --git a/arch/powerpc/include/asm/code-patching.h
b/arch/powerpc/include/asm/code-patching.h
index abef812de7f8..302e4368debc 100644
--- a/arch/powerpc/include/asm/code-patching.h
+++ b/arch/powerpc/include/asm/code-patching.h
@@ -33,6 +33,7 @@ int patch_branch(unsigned int *addr, unsigned long target,
int flags);
int patch_instruction(unsigned int *addr, unsigned int instr);
int instr_is_relative_branch(unsigned int instr);
+int instr_is_link_branch(unsigned int instr);
int instr_is_branch_to_addr(const unsigned int *instr, unsigned long addr);
unsigned long branch_target(const unsigned int *instr);
unsigned int translate_branch(const unsigned int *dest,
diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index 9cb007bc7075..b5148a206b4d 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -487,11 +487,13 @@ static bool is_early_mcount_callsite(u32 *instruction)
restore r2. */
static int restore_r2(u32 *instruction, struct module *me)
{
- if (is_early_mcount_callsite(instruction - 1))
+ u32 *prev_insn = instruction - 1;
+
+ if (is_early_mcount_callsite(prev_insn))
return 1;
/* Sibling calls don't return, so they don't need to restore r2 */
- if (instruction[-1] == PPC_INST_BRANCH)
+ if (!instr_is_link_branch(*prev_insn))
return 1;
if (*instruction != PPC_INST_NOP) {
diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
index c9de03e0c1f1..4727fafd37e4 100644
--- a/arch/powerpc/lib/code-patching.c
+++ b/arch/powerpc/lib/code-patching.c
@@ -304,6 +304,12 @@ int instr_is_relative_branch(unsigned int instr)
return instr_is_branch_iform(instr) || instr_is_branch_bform(instr);
}
+int instr_is_link_branch(unsigned int instr)
+{
+ return (instr_is_branch_iform(instr) || instr_is_branch_bform(instr)) &&
+ (instr & BRANCH_SET_LINK);
+}
+
static unsigned long branch_iform_target(const unsigned int *instr)
{
signed long imm;
--
cheers,
Kamalesh.
[PATCH v4 0/3] ppc64le: Add REL24 relocation support of livepatch symbols
This patchset drop the approach of creating new stub type for livepatch symbols and offloads the issue of handling local function becoming global to kpatch tool via gcc-plugin. In function restore_r2(), a check for sibling call is added and also improves the error message on unexpected op-code. v4: - Drop creation of stubs for livepatch symbols and offload solution to kpatch tool. - Introduce check for sibling call, when restoring r2 after branch. (Josh) - Improve error message in restore_r2(). (Josh) v3: - Defined FUNC_DESC_OFFSET to calculate func_desc offset from struct ppc64le_klp_stub_entry. - Replaced BUG_ON() with WARN_ON() in klp_stub_for_addr(). - Major commit message re-write. v2: - Changed klp_stub construction to re-use livepatch_handler and additional patch code required for klp_stub, instead of duplicating it. - Minor comments and commit body edit. Josh Poimboeuf (2): powerpc/modules: Don't try to restore r2 after a sibling call powerpc/modules: Improve restore_r2() error message Kamalesh Babulal (1): kernel/modules: Add REL24 relocation support of livepatch symbols arch/powerpc/kernel/module_64.c | 11 --- 1 file changed, 8 insertions(+), 3 deletions(-) -- 2.9.3
[PATCH v4 0/3] ppc64le: Add REL24 relocation support of livepatch symbols
This patchset drop the approach of creating new stub type for livepatch symbols and offloads the issue of handling local function becoming global to kpatch tool via gcc-plugin. In function restore_r2(), a check for sibling call is added and also improves the error message on unexpected op-code. v4: - Drop creation of stubs for livepatch symbols and offload solution to kpatch tool. - Introduce check for sibling call, when restoring r2 after branch. (Josh) - Improve error message in restore_r2(). (Josh) v3: - Defined FUNC_DESC_OFFSET to calculate func_desc offset from struct ppc64le_klp_stub_entry. - Replaced BUG_ON() with WARN_ON() in klp_stub_for_addr(). - Major commit message re-write. v2: - Changed klp_stub construction to re-use livepatch_handler and additional patch code required for klp_stub, instead of duplicating it. - Minor comments and commit body edit. Josh Poimboeuf (2): powerpc/modules: Don't try to restore r2 after a sibling call powerpc/modules: Improve restore_r2() error message Kamalesh Babulal (1): kernel/modules: Add REL24 relocation support of livepatch symbols arch/powerpc/kernel/module_64.c | 11 --- 1 file changed, 8 insertions(+), 3 deletions(-) -- 2.9.3
[PATCH v4 3/3] powerpc/modules: Improve restore_r2() error message
From: Josh Poimboeuf <jpoim...@redhat.com>
Print the function address associated with the restore_r2() error to
make it easier to debug the problem.
Also clarify the wording a bit.
Before:
module_64: patch_foo: Expect noop after relocate, got 3c82
After:
module_64: patch_foo: Expected noop after call, got 7c630034 at
netdev_has_upper_dev+0x54/0xb0 [patch_foo]
Signed-off-by: Josh Poimboeuf <jpoim...@redhat.com>
Signed-off-by: Kamalesh Babulal <kamal...@linux.vnet.ibm.com>
---
arch/powerpc/kernel/module_64.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index 9e5391f..dad3ea5 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -494,8 +494,8 @@ static int restore_r2(u32 *instruction, struct module *me)
return 1;
if (*instruction != PPC_INST_NOP) {
- pr_err("%s: Expect noop after relocate, got %08x\n",
- me->name, *instruction);
+ pr_err("%s: Expected noop after call, got %08x at %pS\n",
+ me->name, *instruction, instruction);
return 0;
}
/* ld r2,R2_STACK_OFFSET(r1) */
--
2.9.3
[PATCH v4 3/3] powerpc/modules: Improve restore_r2() error message
From: Josh Poimboeuf
Print the function address associated with the restore_r2() error to
make it easier to debug the problem.
Also clarify the wording a bit.
Before:
module_64: patch_foo: Expect noop after relocate, got 3c82
After:
module_64: patch_foo: Expected noop after call, got 7c630034 at
netdev_has_upper_dev+0x54/0xb0 [patch_foo]
Signed-off-by: Josh Poimboeuf
Signed-off-by: Kamalesh Babulal
---
arch/powerpc/kernel/module_64.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index 9e5391f..dad3ea5 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -494,8 +494,8 @@ static int restore_r2(u32 *instruction, struct module *me)
return 1;
if (*instruction != PPC_INST_NOP) {
- pr_err("%s: Expect noop after relocate, got %08x\n",
- me->name, *instruction);
+ pr_err("%s: Expected noop after call, got %08x at %pS\n",
+ me->name, *instruction, instruction);
return 0;
}
/* ld r2,R2_STACK_OFFSET(r1) */
--
2.9.3
[PATCH v4 2/3] powerpc/modules: Don't try to restore r2 after a sibling call
From: Josh Poimboeuf <jpoim...@redhat.com>
When attempting to load a livepatch module, I got the following error:
module_64: patch_module: Expect noop after relocate, got 3c82
The error was triggered by the following code in
unregister_netdevice_queue():
14c: 00 00 00 48 b 14c <unregister_netdevice_queue+0x14c>
14c: R_PPC64_REL24 net_set_todo
150: 00 00 82 3c addis r4,r2,0
GCC didn't insert a nop after the branch to net_set_todo() because it's
a sibling call, so it never returns. The nop isn't needed after the
branch in that case.
Signed-off-by: Josh Poimboeuf <jpoim...@redhat.com>
Signed-off-by: Kamalesh Babulal <kamal...@linux.vnet.ibm.com>
---
arch/powerpc/kernel/module_64.c | 4
1 file changed, 4 insertions(+)
diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index 39b01fd..9e5391f 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -489,6 +489,10 @@ static int restore_r2(u32 *instruction, struct module *me)
if (is_early_mcount_callsite(instruction - 1))
return 1;
+ /* Sibling calls don't return, so they don't need to restore r2 */
+ if (instruction[-1] == PPC_INST_BRANCH)
+ return 1;
+
if (*instruction != PPC_INST_NOP) {
pr_err("%s: Expect noop after relocate, got %08x\n",
me->name, *instruction);
--
2.9.3
[PATCH v4 2/3] powerpc/modules: Don't try to restore r2 after a sibling call
From: Josh Poimboeuf
When attempting to load a livepatch module, I got the following error:
module_64: patch_module: Expect noop after relocate, got 3c82
The error was triggered by the following code in
unregister_netdevice_queue():
14c: 00 00 00 48 b 14c
14c: R_PPC64_REL24 net_set_todo
150: 00 00 82 3c addis r4,r2,0
GCC didn't insert a nop after the branch to net_set_todo() because it's
a sibling call, so it never returns. The nop isn't needed after the
branch in that case.
Signed-off-by: Josh Poimboeuf
Signed-off-by: Kamalesh Babulal
---
arch/powerpc/kernel/module_64.c | 4
1 file changed, 4 insertions(+)
diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index 39b01fd..9e5391f 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -489,6 +489,10 @@ static int restore_r2(u32 *instruction, struct module *me)
if (is_early_mcount_callsite(instruction - 1))
return 1;
+ /* Sibling calls don't return, so they don't need to restore r2 */
+ if (instruction[-1] == PPC_INST_BRANCH)
+ return 1;
+
if (*instruction != PPC_INST_NOP) {
pr_err("%s: Expect noop after relocate, got %08x\n",
me->name, *instruction);
--
2.9.3
[PATCH v4 1/3] kernel/modules: Add REL24 relocation support of livepatch symbols
Livepatch re-uses module loader function apply_relocate_add() to write
relocations, instead of managing them by arch-dependent
klp_write_module_reloc() function.
apply_relocate_add() doesn't understand livepatch symbols (marked with
SHN_LIVEPATCH symbol section index) and assumes them to be local symbols
by default for R_PPC64_REL24 relocation type. It fails with an error,
when trying to calculate offset with local_entry_offset():
module_64: kpatch_meminfo: REL24 -1152921504897399800 out of range!
Whereas livepatch symbols are essentially SHN_UNDEF, should be
called via stub used for global calls. This issue can be fixed by
teaching apply_relocate_add() to handle both SHN_UNDEF/SHN_LIVEPATCH
symbols via the same stub. This patch extends SHN_UNDEF code to handle
livepatch symbols too.
Signed-off-by: Kamalesh Babulal <kamal...@linux.vnet.ibm.com>
CC: Balbir Singh <bsinghar...@gmail.com>
Cc: Naveen N. Rao <naveen.n@linux.vnet.ibm.com>
Cc: Josh Poimboeuf <jpoim...@redhat.com>
Cc: Jessica Yu <j...@kernel.org>
Cc: Ananth N Mavinakayanahalli <ana...@linux.vnet.ibm.com>
Cc: Aravinda Prasad <aravi...@linux.vnet.ibm.com>
Cc: Torsten Duwe <d...@lst.de>
---
arch/powerpc/kernel/module_64.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index 0b0f896..39b01fd 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -613,7 +613,8 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
case R_PPC_REL24:
/* FIXME: Handle weak symbols here --RR */
- if (sym->st_shndx == SHN_UNDEF) {
+ if (sym->st_shndx == SHN_UNDEF ||
+ sym->st_shndx == SHN_LIVEPATCH) {
/* External: go via stub */
value = stub_for_addr(sechdrs, value, me);
if (!value)
--
2.9.3
[PATCH v4 1/3] kernel/modules: Add REL24 relocation support of livepatch symbols
Livepatch re-uses module loader function apply_relocate_add() to write
relocations, instead of managing them by arch-dependent
klp_write_module_reloc() function.
apply_relocate_add() doesn't understand livepatch symbols (marked with
SHN_LIVEPATCH symbol section index) and assumes them to be local symbols
by default for R_PPC64_REL24 relocation type. It fails with an error,
when trying to calculate offset with local_entry_offset():
module_64: kpatch_meminfo: REL24 -1152921504897399800 out of range!
Whereas livepatch symbols are essentially SHN_UNDEF, should be
called via stub used for global calls. This issue can be fixed by
teaching apply_relocate_add() to handle both SHN_UNDEF/SHN_LIVEPATCH
symbols via the same stub. This patch extends SHN_UNDEF code to handle
livepatch symbols too.
Signed-off-by: Kamalesh Babulal
CC: Balbir Singh
Cc: Naveen N. Rao
Cc: Josh Poimboeuf
Cc: Jessica Yu
Cc: Ananth N Mavinakayanahalli
Cc: Aravinda Prasad
Cc: Torsten Duwe
---
arch/powerpc/kernel/module_64.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index 0b0f896..39b01fd 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -613,7 +613,8 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
case R_PPC_REL24:
/* FIXME: Handle weak symbols here --RR */
- if (sym->st_shndx == SHN_UNDEF) {
+ if (sym->st_shndx == SHN_UNDEF ||
+ sym->st_shndx == SHN_LIVEPATCH) {
/* External: go via stub */
value = stub_for_addr(sechdrs, value, me);
if (!value)
--
2.9.3
[PATCH] objtool: Fix ignore_func() description
ignore_func() is limited to check if a symbol has been manually
whitelisted using STACK_FRAME_NON_STANDARD macro. Change the function
description accordingly.
Cc: Ingo Molnar <mi...@kernel.org>
Signed-off-by: Kamalesh Babulal <kamal...@linux.vnet.ibm.com>
---
tools/objtool/check.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index c0e26ad1fa7e..2c67ca2645f5 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -81,8 +81,7 @@ static struct instruction *next_insn_same_sec(struct
objtool_file *file,
/*
* Check if the function has been manually whitelisted with the
- * STACK_FRAME_NON_STANDARD macro, or if it should be automatically whitelisted
- * due to its use of a context switching instruction.
+ * STACK_FRAME_NON_STANDARD macro.
*/
static bool ignore_func(struct objtool_file *file, struct symbol *func)
{
--
2.7.4
[PATCH] objtool: Fix ignore_func() description
ignore_func() is limited to check if a symbol has been manually
whitelisted using STACK_FRAME_NON_STANDARD macro. Change the function
description accordingly.
Cc: Ingo Molnar
Signed-off-by: Kamalesh Babulal
---
tools/objtool/check.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index c0e26ad1fa7e..2c67ca2645f5 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -81,8 +81,7 @@ static struct instruction *next_insn_same_sec(struct
objtool_file *file,
/*
* Check if the function has been manually whitelisted with the
- * STACK_FRAME_NON_STANDARD macro, or if it should be automatically whitelisted
- * due to its use of a context switching instruction.
+ * STACK_FRAME_NON_STANDARD macro.
*/
static bool ignore_func(struct objtool_file *file, struct symbol *func)
{
--
2.7.4
[PATCH] objtool: Refactor insn search in add_dead_ends()
Move the search for instruction dead_end into helper function
__add_dead_ends(), instead of duplicating the search for unreachable
and reachable sections in add_dead_ends().
Cc: Ingo Molnar <mi...@kernel.org>
Signed-off-by: Kamalesh Babulal <kamal...@linux.vnet.ibm.com>
---
tools/objtool/check.c | 108 +++---
1 file changed, 50 insertions(+), 58 deletions(-)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index c0e26ad1fa7e..c82966b3ad96 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -303,36 +303,23 @@ static int decode_instructions(struct objtool_file *file)
return ret;
}
-/*
- * Mark "ud2" instructions and manually annotated dead ends.
- */
-static int add_dead_ends(struct objtool_file *file)
+static int __add_dead_ends(struct objtool_file *file, struct section *sec,
+ bool unreachable_sec)
{
- struct section *sec;
- struct rela *rela;
+ char warn_str[12] = "unreachable";
struct instruction *insn;
+ struct rela *rela;
bool found;
- /*
-* By default, "ud2" is a dead end unless otherwise annotated, because
-* GCC 7 inserts it for certain divide-by-zero cases.
-*/
- for_each_insn(file, insn)
- if (insn->type == INSN_BUG)
- insn->dead_end = true;
-
- /*
-* Check for manually annotated dead ends.
-*/
- sec = find_section_by_name(file->elf, ".rela.discard.unreachable");
- if (!sec)
- goto reachable;
+ if (!unreachable_sec)
+ strcpy(warn_str, "reachable");
list_for_each_entry(rela, >rela_list, list) {
if (rela->sym->type != STT_SECTION) {
- WARN("unexpected relocation symbol type in %s",
sec->name);
+ WARN("Unexpected relocation symbol type in %s",
sec->name);
return -1;
}
+
insn = find_insn(file, rela->sym->sec, rela->addend);
if (insn)
insn = list_prev_entry(insn, list);
@@ -346,19 +333,54 @@ static int add_dead_ends(struct objtool_file *file)
}
if (!found) {
- WARN("can't find unreachable insn at %s+0x%x",
-rela->sym->sec->name, rela->addend);
+ WARN("can't find %s insn at %s+0x%x",
+ warn_str, rela->sym->sec->name,
+ rela->addend);
return -1;
}
} else {
- WARN("can't find unreachable insn at %s+0x%x",
-rela->sym->sec->name, rela->addend);
+ WARN("can't find %s insn at %s+0x%x",
+ warn_str, rela->sym->sec->name, rela->addend);
return -1;
}
- insn->dead_end = true;
+ /*
+* reachable section's insn->dead_end are false.
+*/
+ insn->dead_end = unreachable_sec;
}
+ return 0;
+}
+
+/*
+ * Mark "ud2" instructions and manually annotated dead ends.
+ */
+static int add_dead_ends(struct objtool_file *file)
+{
+ struct section *sec;
+ struct instruction *insn;
+ int ret;
+
+ /*
+* By default, "ud2" is a dead end unless otherwise annotated, because
+* GCC 7 inserts it for certain divide-by-zero cases.
+*/
+ for_each_insn(file, insn)
+ if (insn->type == INSN_BUG)
+ insn->dead_end = true;
+
+ /*
+* Check for manually annotated dead ends.
+*/
+ sec = find_section_by_name(file->elf, ".rela.discard.unreachable");
+ if (!sec)
+ goto reachable;
+
+ ret = __add_dead_ends(file, sec, true);
+ if (ret == -1)
+ return ret;
+
reachable:
/*
* These manually annotated reachable checks are needed for GCC 4.4,
@@ -370,38 +392,8 @@ static int add_dead_ends(struct objtool_file *file)
if (!sec)
return 0;
- list_for_each_entry(rela, >rela_list, list) {
- if (rela->sym->type != STT_SECTION) {
- WARN("unexpected relocation symbol type in %s",
sec->name);
- return -1;
- }
- insn = find_insn(file, rela->sym->sec, rela->addend);
- if (insn)
-
[PATCH] objtool: Refactor insn search in add_dead_ends()
Move the search for instruction dead_end into helper function
__add_dead_ends(), instead of duplicating the search for unreachable
and reachable sections in add_dead_ends().
Cc: Ingo Molnar
Signed-off-by: Kamalesh Babulal
---
tools/objtool/check.c | 108 +++---
1 file changed, 50 insertions(+), 58 deletions(-)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index c0e26ad1fa7e..c82966b3ad96 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -303,36 +303,23 @@ static int decode_instructions(struct objtool_file *file)
return ret;
}
-/*
- * Mark "ud2" instructions and manually annotated dead ends.
- */
-static int add_dead_ends(struct objtool_file *file)
+static int __add_dead_ends(struct objtool_file *file, struct section *sec,
+ bool unreachable_sec)
{
- struct section *sec;
- struct rela *rela;
+ char warn_str[12] = "unreachable";
struct instruction *insn;
+ struct rela *rela;
bool found;
- /*
-* By default, "ud2" is a dead end unless otherwise annotated, because
-* GCC 7 inserts it for certain divide-by-zero cases.
-*/
- for_each_insn(file, insn)
- if (insn->type == INSN_BUG)
- insn->dead_end = true;
-
- /*
-* Check for manually annotated dead ends.
-*/
- sec = find_section_by_name(file->elf, ".rela.discard.unreachable");
- if (!sec)
- goto reachable;
+ if (!unreachable_sec)
+ strcpy(warn_str, "reachable");
list_for_each_entry(rela, >rela_list, list) {
if (rela->sym->type != STT_SECTION) {
- WARN("unexpected relocation symbol type in %s",
sec->name);
+ WARN("Unexpected relocation symbol type in %s",
sec->name);
return -1;
}
+
insn = find_insn(file, rela->sym->sec, rela->addend);
if (insn)
insn = list_prev_entry(insn, list);
@@ -346,19 +333,54 @@ static int add_dead_ends(struct objtool_file *file)
}
if (!found) {
- WARN("can't find unreachable insn at %s+0x%x",
-rela->sym->sec->name, rela->addend);
+ WARN("can't find %s insn at %s+0x%x",
+ warn_str, rela->sym->sec->name,
+ rela->addend);
return -1;
}
} else {
- WARN("can't find unreachable insn at %s+0x%x",
-rela->sym->sec->name, rela->addend);
+ WARN("can't find %s insn at %s+0x%x",
+ warn_str, rela->sym->sec->name, rela->addend);
return -1;
}
- insn->dead_end = true;
+ /*
+* reachable section's insn->dead_end are false.
+*/
+ insn->dead_end = unreachable_sec;
}
+ return 0;
+}
+
+/*
+ * Mark "ud2" instructions and manually annotated dead ends.
+ */
+static int add_dead_ends(struct objtool_file *file)
+{
+ struct section *sec;
+ struct instruction *insn;
+ int ret;
+
+ /*
+* By default, "ud2" is a dead end unless otherwise annotated, because
+* GCC 7 inserts it for certain divide-by-zero cases.
+*/
+ for_each_insn(file, insn)
+ if (insn->type == INSN_BUG)
+ insn->dead_end = true;
+
+ /*
+* Check for manually annotated dead ends.
+*/
+ sec = find_section_by_name(file->elf, ".rela.discard.unreachable");
+ if (!sec)
+ goto reachable;
+
+ ret = __add_dead_ends(file, sec, true);
+ if (ret == -1)
+ return ret;
+
reachable:
/*
* These manually annotated reachable checks are needed for GCC 4.4,
@@ -370,38 +392,8 @@ static int add_dead_ends(struct objtool_file *file)
if (!sec)
return 0;
- list_for_each_entry(rela, >rela_list, list) {
- if (rela->sym->type != STT_SECTION) {
- WARN("unexpected relocation symbol type in %s",
sec->name);
- return -1;
- }
- insn = find_insn(file, rela->sym->sec, rela->addend);
- if (insn)
- insn = list_prev_entry(insn, list);
- else if (rela-&
Re: [PATCH] objtool: Fix memory leak in decode_instructions()
On Thursday 19 October 2017 08:02 PM, Josh Poimboeuf wrote: On Thu, Oct 19, 2017 at 09:31:37AM -0500, Josh Poimboeuf wrote: On Fri, Oct 13, 2017 at 11:20:58AM +0530, Kamalesh Babulal wrote: On Friday 13 October 2017 10:36 AM, Josh Poimboeuf wrote: On Fri, Oct 13, 2017 at 10:14:36AM +0530, Kamalesh Babulal wrote: On Thursday 12 October 2017 09:40 PM, Josh Poimboeuf wrote: On Thu, Oct 12, 2017 at 02:32:14PM +0530, Kamalesh Babulal wrote: free the allocated insn before returning, when an error occurs before adding insn to file->insn_list. Signed-off-by: Kamalesh Babulal <kamal...@linux.vnet.ibm.com> Any chance you're working on porting objtool to ppc64le? :-) Acked-by: Josh Poimboeuf <jpoim...@redhat.com> Thanks for the review. I have started working on it :) Good! Let me know if you have any questions. Thank you, I am sure I will have lots of them. I originally wrote objtool with arch-independence in mind, though with the new "objtool 2.0" rewrite, it unfortunately became more x86-specific. I was hoping to work on making it more arch-independent, and then start porting it to other arches, but it may be a few months before I have the time to do so. So any work you want to there would be great. Sure, will keep that in mind to abstract arch-independent code in to common files and push arch-dependent code into arch/ directory for both ppc64le/x86. Kamalesh, since you might be working on this soon, here's a bit of a brain dump in case it helps. I have a vague idea for eventually improving objtool, which might make arch-independence easier. I wonder if we could use a GCC plugin to add annotations in special sections, which are then read by objtool to help it understand what's going on in the code. I suspect the plugins would need to be arch-specific. But then maybe that would allow objtool itself to be completely arch-independent. Eventually, if we had similar clang plugins, objtool could become compiler-independent as well. Unfortunately I haven't had a chance to think about it any more deeply than that. Just something to think about. Or maybe it makes sense to port objtool to ppc first, before trying to abstract the arch-specific parts into plugins. I dunno. Thanks for sharing the thoughts on making objtool arch-independent. I have not followed the discussions closely on GCC plugin to add annotations/DWARF data. I will read through the ideas, before commenting on them. Also, another idea that has been suggested, is to use DWARF data as input to objtool. I have my doubts about that idea, but if it worked, that might be another way to help objtool achieve arch-independence. For a previous discussion of these ideas, see: https://lkml.kernel.org/r/20170831044209.4hodx2dasad66yab@treble Also, any chance you'll be in Prague for the conferences next week? If so, we could discuss it more in person. I will not be attending the conference. -- cheers, Kamalesh.
Re: [PATCH] objtool: Fix memory leak in decode_instructions()
On Thursday 19 October 2017 08:02 PM, Josh Poimboeuf wrote: On Thu, Oct 19, 2017 at 09:31:37AM -0500, Josh Poimboeuf wrote: On Fri, Oct 13, 2017 at 11:20:58AM +0530, Kamalesh Babulal wrote: On Friday 13 October 2017 10:36 AM, Josh Poimboeuf wrote: On Fri, Oct 13, 2017 at 10:14:36AM +0530, Kamalesh Babulal wrote: On Thursday 12 October 2017 09:40 PM, Josh Poimboeuf wrote: On Thu, Oct 12, 2017 at 02:32:14PM +0530, Kamalesh Babulal wrote: free the allocated insn before returning, when an error occurs before adding insn to file->insn_list. Signed-off-by: Kamalesh Babulal Any chance you're working on porting objtool to ppc64le? :-) Acked-by: Josh Poimboeuf Thanks for the review. I have started working on it :) Good! Let me know if you have any questions. Thank you, I am sure I will have lots of them. I originally wrote objtool with arch-independence in mind, though with the new "objtool 2.0" rewrite, it unfortunately became more x86-specific. I was hoping to work on making it more arch-independent, and then start porting it to other arches, but it may be a few months before I have the time to do so. So any work you want to there would be great. Sure, will keep that in mind to abstract arch-independent code in to common files and push arch-dependent code into arch/ directory for both ppc64le/x86. Kamalesh, since you might be working on this soon, here's a bit of a brain dump in case it helps. I have a vague idea for eventually improving objtool, which might make arch-independence easier. I wonder if we could use a GCC plugin to add annotations in special sections, which are then read by objtool to help it understand what's going on in the code. I suspect the plugins would need to be arch-specific. But then maybe that would allow objtool itself to be completely arch-independent. Eventually, if we had similar clang plugins, objtool could become compiler-independent as well. Unfortunately I haven't had a chance to think about it any more deeply than that. Just something to think about. Or maybe it makes sense to port objtool to ppc first, before trying to abstract the arch-specific parts into plugins. I dunno. Thanks for sharing the thoughts on making objtool arch-independent. I have not followed the discussions closely on GCC plugin to add annotations/DWARF data. I will read through the ideas, before commenting on them. Also, another idea that has been suggested, is to use DWARF data as input to objtool. I have my doubts about that idea, but if it worked, that might be another way to help objtool achieve arch-independence. For a previous discussion of these ideas, see: https://lkml.kernel.org/r/20170831044209.4hodx2dasad66yab@treble Also, any chance you'll be in Prague for the conferences next week? If so, we could discuss it more in person. I will not be attending the conference. -- cheers, Kamalesh.
[tip:core/urgent] objtool: Fix memory leak in decode_instructions()
Commit-ID: b703798386fb7288d5a995bd2284a984a5e24f3c
Gitweb: https://git.kernel.org/tip/b703798386fb7288d5a995bd2284a984a5e24f3c
Author: Kamalesh Babulal <kamal...@linux.vnet.ibm.com>
AuthorDate: Thu, 19 Oct 2017 11:27:24 -0500
Committer: Ingo Molnar <mi...@kernel.org>
CommitDate: Fri, 20 Oct 2017 09:43:21 +0200
objtool: Fix memory leak in decode_instructions()
When an error occurs before adding an allocated insn to the list, free
it before returning.
Signed-off-by: Kamalesh Babulal <kamal...@linux.vnet.ibm.com>
Signed-off-by: Josh Poimboeuf <jpoim...@redhat.com>
Cc: Linus Torvalds <torva...@linux-foundation.org>
Cc: Peter Zijlstra <pet...@infradead.org>
Cc: Thomas Gleixner <t...@linutronix.de>
Link:
http://lkml.kernel.org/r/336da800bf6070eae11f4e0a3b9ca64c27658114.1508430423.git.jpoim...@redhat.com
Signed-off-by: Ingo Molnar <mi...@kernel.org>
---
tools/objtool/check.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index a0c518e..c0e26ad 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -267,12 +267,13 @@ static int decode_instructions(struct objtool_file *file)
>immediate,
>stack_op);
if (ret)
- return ret;
+ goto err;
if (!insn->type || insn->type > INSN_LAST) {
WARN_FUNC("invalid instruction type %d",
insn->sec, insn->offset, insn->type);
- return -1;
+ ret = -1;
+ goto err;
}
hash_add(file->insn_hash, >hash, insn->offset);
@@ -296,6 +297,10 @@ static int decode_instructions(struct objtool_file *file)
}
return 0;
+
+err:
+ free(insn);
+ return ret;
}
/*
[tip:core/urgent] objtool: Fix memory leak in decode_instructions()
Commit-ID: b703798386fb7288d5a995bd2284a984a5e24f3c
Gitweb: https://git.kernel.org/tip/b703798386fb7288d5a995bd2284a984a5e24f3c
Author: Kamalesh Babulal
AuthorDate: Thu, 19 Oct 2017 11:27:24 -0500
Committer: Ingo Molnar
CommitDate: Fri, 20 Oct 2017 09:43:21 +0200
objtool: Fix memory leak in decode_instructions()
When an error occurs before adding an allocated insn to the list, free
it before returning.
Signed-off-by: Kamalesh Babulal
Signed-off-by: Josh Poimboeuf
Cc: Linus Torvalds
Cc: Peter Zijlstra
Cc: Thomas Gleixner
Link:
http://lkml.kernel.org/r/336da800bf6070eae11f4e0a3b9ca64c27658114.1508430423.git.jpoim...@redhat.com
Signed-off-by: Ingo Molnar
---
tools/objtool/check.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index a0c518e..c0e26ad 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -267,12 +267,13 @@ static int decode_instructions(struct objtool_file *file)
>immediate,
>stack_op);
if (ret)
- return ret;
+ goto err;
if (!insn->type || insn->type > INSN_LAST) {
WARN_FUNC("invalid instruction type %d",
insn->sec, insn->offset, insn->type);
- return -1;
+ ret = -1;
+ goto err;
}
hash_add(file->insn_hash, >hash, insn->offset);
@@ -296,6 +297,10 @@ static int decode_instructions(struct objtool_file *file)
}
return 0;
+
+err:
+ free(insn);
+ return ret;
}
/*
[tip:core/objtool] objtool: Print top level commands on incorrect usage
Commit-ID: 6a93bb7e4a7d6670677d5b0eb980936eb9cc5d2e
Gitweb: https://git.kernel.org/tip/6a93bb7e4a7d6670677d5b0eb980936eb9cc5d2e
Author: Kamalesh Babulal <kamal...@linux.vnet.ibm.com>
AuthorDate: Sat, 14 Oct 2017 20:17:54 +0530
Committer: Ingo Molnar <mi...@kernel.org>
CommitDate: Wed, 18 Oct 2017 15:22:26 +0200
objtool: Print top level commands on incorrect usage
Print top-level objtool commands, along with the error on incorrect
command line usage. Objtool command line parser exit's with code 129,
for incorrect usage. Convert the cmd_usage() exit code also, to maintain
consistency across objtool.
After the patch:
$ ./objtool -j
Unknown option: -j
usage: objtool COMMAND [ARGS]
Commands:
check Perform stack metadata validation on an object file
orc Generate in-place ORC unwind tables for an object file
$ echo $?
129
Signed-off-by: Kamalesh Babulal <kamal...@linux.vnet.ibm.com>
Acked-by: Josh Poimboeuf <jpoim...@redhat.com>
Cc: Linus Torvalds <torva...@linux-foundation.org>
Cc: Peter Zijlstra <pet...@infradead.org>
Cc: Thomas Gleixner <t...@linutronix.de>
Link:
http://lkml.kernel.org/r/1507992474-16142-1-git-send-email-kamal...@linux.vnet.ibm.com
Signed-off-by: Ingo Molnar <mi...@kernel.org>
---
tools/objtool/objtool.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/tools/objtool/objtool.c b/tools/objtool/objtool.c
index 31e0f91..07f3299 100644
--- a/tools/objtool/objtool.c
+++ b/tools/objtool/objtool.c
@@ -70,7 +70,7 @@ static void cmd_usage(void)
printf("\n");
- exit(1);
+ exit(129);
}
static void handle_options(int *argc, const char ***argv)
@@ -86,9 +86,7 @@ static void handle_options(int *argc, const char ***argv)
break;
} else {
fprintf(stderr, "Unknown option: %s\n", cmd);
- fprintf(stderr, "\n Usage: %s\n",
- objtool_usage_string);
- exit(1);
+ cmd_usage();
}
(*argv)++;
[tip:core/objtool] objtool: Print top level commands on incorrect usage
Commit-ID: 6a93bb7e4a7d6670677d5b0eb980936eb9cc5d2e
Gitweb: https://git.kernel.org/tip/6a93bb7e4a7d6670677d5b0eb980936eb9cc5d2e
Author: Kamalesh Babulal
AuthorDate: Sat, 14 Oct 2017 20:17:54 +0530
Committer: Ingo Molnar
CommitDate: Wed, 18 Oct 2017 15:22:26 +0200
objtool: Print top level commands on incorrect usage
Print top-level objtool commands, along with the error on incorrect
command line usage. Objtool command line parser exit's with code 129,
for incorrect usage. Convert the cmd_usage() exit code also, to maintain
consistency across objtool.
After the patch:
$ ./objtool -j
Unknown option: -j
usage: objtool COMMAND [ARGS]
Commands:
check Perform stack metadata validation on an object file
orc Generate in-place ORC unwind tables for an object file
$ echo $?
129
Signed-off-by: Kamalesh Babulal
Acked-by: Josh Poimboeuf
Cc: Linus Torvalds
Cc: Peter Zijlstra
Cc: Thomas Gleixner
Link:
http://lkml.kernel.org/r/1507992474-16142-1-git-send-email-kamal...@linux.vnet.ibm.com
Signed-off-by: Ingo Molnar
---
tools/objtool/objtool.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/tools/objtool/objtool.c b/tools/objtool/objtool.c
index 31e0f91..07f3299 100644
--- a/tools/objtool/objtool.c
+++ b/tools/objtool/objtool.c
@@ -70,7 +70,7 @@ static void cmd_usage(void)
printf("\n");
- exit(1);
+ exit(129);
}
static void handle_options(int *argc, const char ***argv)
@@ -86,9 +86,7 @@ static void handle_options(int *argc, const char ***argv)
break;
} else {
fprintf(stderr, "Unknown option: %s\n", cmd);
- fprintf(stderr, "\n Usage: %s\n",
- objtool_usage_string);
- exit(1);
+ cmd_usage();
}
(*argv)++;
[PATCH v2] objtool: Print top level commands on incorrect usage
Print top-level objtool commands, along with the error on incorrect
command line usage. Objtool command line parser exit's with code 129,
for incorrect usage. Convert the cmd_usage() exit code also, to maintain
consistency across objtool.
After the patch:
$ ./objtool -j
Unknown option: -j
usage: objtool COMMAND [ARGS]
Commands:
check Perform stack metadata validation on an object file
orc Generate in-place ORC unwind tables for an object file
$ echo $?
129
Cc: Ingo Molnar <mi...@kernel.org>
Signed-off-by: Kamalesh Babulal <kamal...@linux.vnet.ibm.com>
---
v2:
- Changes to commit message only.
tools/objtool/objtool.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/tools/objtool/objtool.c b/tools/objtool/objtool.c
index 31e0f91..07f3299 100644
--- a/tools/objtool/objtool.c
+++ b/tools/objtool/objtool.c
@@ -70,7 +70,7 @@ static void cmd_usage(void)
printf("\n");
- exit(1);
+ exit(129);
}
static void handle_options(int *argc, const char ***argv)
@@ -86,9 +86,7 @@ static void handle_options(int *argc, const char ***argv)
break;
} else {
fprintf(stderr, "Unknown option: %s\n", cmd);
- fprintf(stderr, "\n Usage: %s\n",
- objtool_usage_string);
- exit(1);
+ cmd_usage();
}
(*argv)++;
--
2.7.4
[PATCH v2] objtool: Print top level commands on incorrect usage
Print top-level objtool commands, along with the error on incorrect
command line usage. Objtool command line parser exit's with code 129,
for incorrect usage. Convert the cmd_usage() exit code also, to maintain
consistency across objtool.
After the patch:
$ ./objtool -j
Unknown option: -j
usage: objtool COMMAND [ARGS]
Commands:
check Perform stack metadata validation on an object file
orc Generate in-place ORC unwind tables for an object file
$ echo $?
129
Cc: Ingo Molnar
Signed-off-by: Kamalesh Babulal
---
v2:
- Changes to commit message only.
tools/objtool/objtool.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/tools/objtool/objtool.c b/tools/objtool/objtool.c
index 31e0f91..07f3299 100644
--- a/tools/objtool/objtool.c
+++ b/tools/objtool/objtool.c
@@ -70,7 +70,7 @@ static void cmd_usage(void)
printf("\n");
- exit(1);
+ exit(129);
}
static void handle_options(int *argc, const char ***argv)
@@ -86,9 +86,7 @@ static void handle_options(int *argc, const char ***argv)
break;
} else {
fprintf(stderr, "Unknown option: %s\n", cmd);
- fprintf(stderr, "\n Usage: %s\n",
- objtool_usage_string);
- exit(1);
+ cmd_usage();
}
(*argv)++;
--
2.7.4
Re: [PATCH] objtool: Print top level commands on incorrect usage
On Friday 13 October 2017 10:08 PM, Josh Poimboeuf wrote:
[...]
Signed-off-by: Kamalesh Babulal <kamal...@linux.vnet.ibm.com>
---
tools/objtool/objtool.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/tools/objtool/objtool.c b/tools/objtool/objtool.c
index 31e0f91..07f3299 100644
--- a/tools/objtool/objtool.c
+++ b/tools/objtool/objtool.c
@@ -70,7 +70,7 @@ static void cmd_usage(void)
printf("\n");
- exit(1);
+ exit(129);
For consistency with the rest of the code, this should be -1 instead of
129 (though the end result is the same).
Thanks for the review. exit(129) is returned by command line parser
when the usage is incorrect, except for cmd_usage(), where the exit
code 1. I will re-send the patch with the better commit message.
--
cheers,
Kamalesh.
Re: [PATCH] objtool: Print top level commands on incorrect usage
On Friday 13 October 2017 10:08 PM, Josh Poimboeuf wrote:
[...]
Signed-off-by: Kamalesh Babulal
---
tools/objtool/objtool.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/tools/objtool/objtool.c b/tools/objtool/objtool.c
index 31e0f91..07f3299 100644
--- a/tools/objtool/objtool.c
+++ b/tools/objtool/objtool.c
@@ -70,7 +70,7 @@ static void cmd_usage(void)
printf("\n");
- exit(1);
+ exit(129);
For consistency with the rest of the code, this should be -1 instead of
129 (though the end result is the same).
Thanks for the review. exit(129) is returned by command line parser
when the usage is incorrect, except for cmd_usage(), where the exit
code 1. I will re-send the patch with the better commit message.
--
cheers,
Kamalesh.
[PATCH] objtool: Print top level commands on incorrect usage
Maintain the consistency with objtool subcommands, by
printing cmd_usage() of top level commands, along with
the error. When incorrect option/command is passed to
objtool. Also change the exit code to 129, like other
objtool subcommands.
After the patch:
$ ./objtool -j
Unknown option: -j
usage: objtool COMMAND [ARGS]
Commands:
check Perform stack metadata validation on an object file
orc Generate in-place ORC unwind tables for an object file
$ echo $?
129
Cc: Ingo Molnar <mi...@kernel.org>
Signed-off-by: Kamalesh Babulal <kamal...@linux.vnet.ibm.com>
---
tools/objtool/objtool.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/tools/objtool/objtool.c b/tools/objtool/objtool.c
index 31e0f91..07f3299 100644
--- a/tools/objtool/objtool.c
+++ b/tools/objtool/objtool.c
@@ -70,7 +70,7 @@ static void cmd_usage(void)
printf("\n");
- exit(1);
+ exit(129);
}
static void handle_options(int *argc, const char ***argv)
@@ -86,9 +86,7 @@ static void handle_options(int *argc, const char ***argv)
break;
} else {
fprintf(stderr, "Unknown option: %s\n", cmd);
- fprintf(stderr, "\n Usage: %s\n",
- objtool_usage_string);
- exit(1);
+ cmd_usage();
}
(*argv)++;
--
2.7.4
[PATCH] objtool: Print top level commands on incorrect usage
Maintain the consistency with objtool subcommands, by
printing cmd_usage() of top level commands, along with
the error. When incorrect option/command is passed to
objtool. Also change the exit code to 129, like other
objtool subcommands.
After the patch:
$ ./objtool -j
Unknown option: -j
usage: objtool COMMAND [ARGS]
Commands:
check Perform stack metadata validation on an object file
orc Generate in-place ORC unwind tables for an object file
$ echo $?
129
Cc: Ingo Molnar
Signed-off-by: Kamalesh Babulal
---
tools/objtool/objtool.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/tools/objtool/objtool.c b/tools/objtool/objtool.c
index 31e0f91..07f3299 100644
--- a/tools/objtool/objtool.c
+++ b/tools/objtool/objtool.c
@@ -70,7 +70,7 @@ static void cmd_usage(void)
printf("\n");
- exit(1);
+ exit(129);
}
static void handle_options(int *argc, const char ***argv)
@@ -86,9 +86,7 @@ static void handle_options(int *argc, const char ***argv)
break;
} else {
fprintf(stderr, "Unknown option: %s\n", cmd);
- fprintf(stderr, "\n Usage: %s\n",
- objtool_usage_string);
- exit(1);
+ cmd_usage();
}
(*argv)++;
--
2.7.4
Re: [PATCH] objtool: Fix memory leak in decode_instructions()
On Friday 13 October 2017 10:36 AM, Josh Poimboeuf wrote: On Fri, Oct 13, 2017 at 10:14:36AM +0530, Kamalesh Babulal wrote: On Thursday 12 October 2017 09:40 PM, Josh Poimboeuf wrote: On Thu, Oct 12, 2017 at 02:32:14PM +0530, Kamalesh Babulal wrote: free the allocated insn before returning, when an error occurs before adding insn to file->insn_list. Signed-off-by: Kamalesh Babulal <kamal...@linux.vnet.ibm.com> Any chance you're working on porting objtool to ppc64le? :-) Acked-by: Josh Poimboeuf <jpoim...@redhat.com> Thanks for the review. I have started working on it :) Good! Let me know if you have any questions. Thank you, I am sure I will have lots of them. I originally wrote objtool with arch-independence in mind, though with the new "objtool 2.0" rewrite, it unfortunately became more x86-specific. I was hoping to work on making it more arch-independent, and then start porting it to other arches, but it may be a few months before I have the time to do so. So any work you want to there would be great. Sure, will keep that in mind to abstract arch-independent code in to common files and push arch-dependent code into arch/ directory for both ppc64le/x86. -- cheers, Kamalesh.
Re: [PATCH] objtool: Fix memory leak in decode_instructions()
On Friday 13 October 2017 10:36 AM, Josh Poimboeuf wrote: On Fri, Oct 13, 2017 at 10:14:36AM +0530, Kamalesh Babulal wrote: On Thursday 12 October 2017 09:40 PM, Josh Poimboeuf wrote: On Thu, Oct 12, 2017 at 02:32:14PM +0530, Kamalesh Babulal wrote: free the allocated insn before returning, when an error occurs before adding insn to file->insn_list. Signed-off-by: Kamalesh Babulal Any chance you're working on porting objtool to ppc64le? :-) Acked-by: Josh Poimboeuf Thanks for the review. I have started working on it :) Good! Let me know if you have any questions. Thank you, I am sure I will have lots of them. I originally wrote objtool with arch-independence in mind, though with the new "objtool 2.0" rewrite, it unfortunately became more x86-specific. I was hoping to work on making it more arch-independent, and then start porting it to other arches, but it may be a few months before I have the time to do so. So any work you want to there would be great. Sure, will keep that in mind to abstract arch-independent code in to common files and push arch-dependent code into arch/ directory for both ppc64le/x86. -- cheers, Kamalesh.
Re: [PATCH] objtool: Fix memory leak in decode_instructions()
On Thursday 12 October 2017 09:40 PM, Josh Poimboeuf wrote: On Thu, Oct 12, 2017 at 02:32:14PM +0530, Kamalesh Babulal wrote: free the allocated insn before returning, when an error occurs before adding insn to file->insn_list. Signed-off-by: Kamalesh Babulal <kamal...@linux.vnet.ibm.com> Any chance you're working on porting objtool to ppc64le? :-) Acked-by: Josh Poimboeuf <jpoim...@redhat.com> Thanks for the review. I have started working on it :) -- cheers, Kamalesh.
Re: [PATCH] objtool: Fix memory leak in decode_instructions()
On Thursday 12 October 2017 09:40 PM, Josh Poimboeuf wrote: On Thu, Oct 12, 2017 at 02:32:14PM +0530, Kamalesh Babulal wrote: free the allocated insn before returning, when an error occurs before adding insn to file->insn_list. Signed-off-by: Kamalesh Babulal Any chance you're working on porting objtool to ppc64le? :-) Acked-by: Josh Poimboeuf Thanks for the review. I have started working on it :) -- cheers, Kamalesh.
[PATCH] objtool: Fix memory leak in decode_instructions()
free the allocated insn before returning, when an error occurs
before adding insn to file->insn_list.
Signed-off-by: Kamalesh Babulal <kamal...@linux.vnet.ibm.com>
---
tools/objtool/check.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index a0c518ecf085..c0e26ad1fa7e 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -267,12 +267,13 @@ static int decode_instructions(struct objtool_file *file)
>immediate,
>stack_op);
if (ret)
- return ret;
+ goto err;
if (!insn->type || insn->type > INSN_LAST) {
WARN_FUNC("invalid instruction type %d",
insn->sec, insn->offset, insn->type);
- return -1;
+ ret = -1;
+ goto err;
}
hash_add(file->insn_hash, >hash, insn->offset);
@@ -296,6 +297,10 @@ static int decode_instructions(struct objtool_file *file)
}
return 0;
+
+err:
+ free(insn);
+ return ret;
}
/*
--
2.11.0
[PATCH] objtool: Fix memory leak in decode_instructions()
free the allocated insn before returning, when an error occurs
before adding insn to file->insn_list.
Signed-off-by: Kamalesh Babulal
---
tools/objtool/check.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index a0c518ecf085..c0e26ad1fa7e 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -267,12 +267,13 @@ static int decode_instructions(struct objtool_file *file)
>immediate,
>stack_op);
if (ret)
- return ret;
+ goto err;
if (!insn->type || insn->type > INSN_LAST) {
WARN_FUNC("invalid instruction type %d",
insn->sec, insn->offset, insn->type);
- return -1;
+ ret = -1;
+ goto err;
}
hash_add(file->insn_hash, >hash, insn->offset);
@@ -296,6 +297,10 @@ static int decode_instructions(struct objtool_file *file)
}
return 0;
+
+err:
+ free(insn);
+ return ret;
}
/*
--
2.11.0
Re: [PATCH v3 12/15] livepatch: store function sizes
On Thursday 08 December 2016 11:38 PM, Josh Poimboeuf wrote: For the consistency model we'll need to know the sizes of the old and new functions to determine if they're on the stacks of any tasks. Signed-off-by: Josh Poimboeuf <jpoim...@redhat.com> Reviewed-by: Kamalesh Babulal <kamal...@linux.vnet.ibm.com> -- cheers, Kamalesh.
Re: [PATCH v3 12/15] livepatch: store function sizes
On Thursday 08 December 2016 11:38 PM, Josh Poimboeuf wrote: For the consistency model we'll need to know the sizes of the old and new functions to determine if they're on the stacks of any tasks. Signed-off-by: Josh Poimboeuf Reviewed-by: Kamalesh Babulal -- cheers, Kamalesh.
Re: [PATCH v3 04/15] livepatch/x86: add TIF_PATCH_PENDING thread flag
On Thursday 08 December 2016 11:38 PM, Josh Poimboeuf wrote: Add the TIF_PATCH_PENDING thread flag to enable the new livepatch per-task consistency model for x86_64. The bit getting set indicates the thread has a pending patch which needs to be applied when the thread exits the kernel. The bit is placed in the _TIF_ALLWORK_MASK macro, which results in exit_to_usermode_loop() calling klp_update_patch_state() when it's set. Signed-off-by: Josh Poimboeuf <jpoim...@redhat.com> Reviewed-by: Kamalesh Babulal <kamal...@linux.vnet.ibm.com> -- cheers, Kamalesh.
Re: [PATCH v3 04/15] livepatch/x86: add TIF_PATCH_PENDING thread flag
On Thursday 08 December 2016 11:38 PM, Josh Poimboeuf wrote: Add the TIF_PATCH_PENDING thread flag to enable the new livepatch per-task consistency model for x86_64. The bit getting set indicates the thread has a pending patch which needs to be applied when the thread exits the kernel. The bit is placed in the _TIF_ALLWORK_MASK macro, which results in exit_to_usermode_loop() calling klp_update_patch_state() when it's set. Signed-off-by: Josh Poimboeuf Reviewed-by: Kamalesh Babulal -- cheers, Kamalesh.
Re: [PATCH v3 10/15] livepatch: move patching functions into patch.c
On Thursday 08 December 2016 11:38 PM, Josh Poimboeuf wrote: Move functions related to the actual patching of functions and objects into a new patch.c file. Signed-off-by: Josh Poimboeuf <jpoim...@redhat.com> Reviewed-by: Kamalesh Babulal <kamal...@linux.vnet.ibm.com> -- cheers, Kamalesh.
Re: [PATCH v3 10/15] livepatch: move patching functions into patch.c
On Thursday 08 December 2016 11:38 PM, Josh Poimboeuf wrote: Move functions related to the actual patching of functions and objects into a new patch.c file. Signed-off-by: Josh Poimboeuf Reviewed-by: Kamalesh Babulal -- cheers, Kamalesh.
Re: [PATCH v3 09/15] livepatch: remove unnecessary object loaded check
On Thursday 08 December 2016 11:38 PM, Josh Poimboeuf wrote: klp_patch_object()'s callers already ensure that the object is loaded, so its call to klp_is_object_loaded() is unnecessary. This will also make it possible to move the patching code into a separate file. Signed-off-by: Josh Poimboeuf <jpoim...@redhat.com> Reviewed-by: Kamalesh Babulal <kamal...@linux.vnet.ibm.com> -- cheers, Kamalesh.
Re: [PATCH v3 09/15] livepatch: remove unnecessary object loaded check
On Thursday 08 December 2016 11:38 PM, Josh Poimboeuf wrote: klp_patch_object()'s callers already ensure that the object is loaded, so its call to klp_is_object_loaded() is unnecessary. This will also make it possible to move the patching code into a separate file. Signed-off-by: Josh Poimboeuf Reviewed-by: Kamalesh Babulal -- cheers, Kamalesh.
Re: [PATCH v3 08/15] livepatch: separate enabled and patched states
On Thursday 08 December 2016 11:38 PM, Josh Poimboeuf wrote: Once we have a consistency model, patches and their objects will be enabled and disabled at different times. For example, when a patch is disabled, its loaded objects' funcs can remain registered with ftrace indefinitely until the unpatching operation is complete and they're no longer in use. It's less confusing if we give them different names: patches can be enabled or disabled; objects (and their funcs) can be patched or unpatched: - Enabled means that a patch is logically enabled (but not necessarily fully applied). - Patched means that an object's funcs are registered with ftrace and added to the klp_ops func stack. Also, since these states are binary, represent them with booleans instead of ints. Signed-off-by: Josh Poimboeuf <jpoim...@redhat.com> Reviewed-by: Kamalesh Babulal <kamal...@linux.vnet.ibm.com> -- cheers, Kamalesh.
Re: [PATCH v3 08/15] livepatch: separate enabled and patched states
On Thursday 08 December 2016 11:38 PM, Josh Poimboeuf wrote: Once we have a consistency model, patches and their objects will be enabled and disabled at different times. For example, when a patch is disabled, its loaded objects' funcs can remain registered with ftrace indefinitely until the unpatching operation is complete and they're no longer in use. It's less confusing if we give them different names: patches can be enabled or disabled; objects (and their funcs) can be patched or unpatched: - Enabled means that a patch is logically enabled (but not necessarily fully applied). - Patched means that an object's funcs are registered with ftrace and added to the klp_ops func stack. Also, since these states are binary, represent them with booleans instead of ints. Signed-off-by: Josh Poimboeuf Reviewed-by: Kamalesh Babulal -- cheers, Kamalesh.
Re: [PATCH v3 02/15] x86/entry: define _TIF_ALLWORK_MASK flags explicitly
On Thursday 08 December 2016 11:38 PM, Josh Poimboeuf wrote: The _TIF_ALLWORK_MASK macro automatically includes the least-significant 16 bits of the thread_info flags, which is less than obvious and tends to create confusion and surprises when reading or modifying the code. Define the flags explicitly. Signed-off-by: Josh Poimboeuf <jpoim...@redhat.com> For the version with swapped _TIF_SINGLESTEP and _TIF_NEED_RESCHED flags. Reviewed-by: Kamalesh Babulal <kamal...@linux.vnet.ibm.com> -- cheers, Kamalesh.
Re: [PATCH v3 02/15] x86/entry: define _TIF_ALLWORK_MASK flags explicitly
On Thursday 08 December 2016 11:38 PM, Josh Poimboeuf wrote: The _TIF_ALLWORK_MASK macro automatically includes the least-significant 16 bits of the thread_info flags, which is less than obvious and tends to create confusion and surprises when reading or modifying the code. Define the flags explicitly. Signed-off-by: Josh Poimboeuf For the version with swapped _TIF_SINGLESTEP and _TIF_NEED_RESCHED flags. Reviewed-by: Kamalesh Babulal -- cheers, Kamalesh.
