From: Joe Lawrence <joe.lawrence@redhat.com>
To: Miroslav Benes <mbenes@suse.cz>
Cc: Petr Mladek <pmladek@suse.com>,
linux-kernel@vger.kernel.org, live-patching@vger.kernel.org,
linux-kbuild@vger.kernel.org
Subject: Re: [PATCH v4 00/10] klp-convert livepatch build tooling
Date: Tue, 25 Jun 2019 15:08:36 -0400 [thread overview]
Message-ID: <20190625190836.GL20356@redhat.com> (raw)
In-Reply-To: <alpine.LSU.2.21.1906251324450.12085@pobox.suse.cz>
On Tue, Jun 25, 2019 at 01:36:37PM +0200, Miroslav Benes wrote:
>
> [ ... snip ... ]
>
> If I revert commit d59cadc0a8f8 ("[squash] klp-convert: make
> convert_rela() list-safe") (from Joe's expanded github tree), the problem
> disappears.
>
> I haven't spotted any problem in the code and I cannot explain a
> dependency on GCC version. Any ideas?
>
I can confirm that test_klp_convert1.ko crashes with RHEL-7 and its
older gcc. I added some debugging printf's to klp-convert and see:
% ./scripts/livepatch/klp-convert \
./Symbols.list \
lib/livepatch/test_klp_convert1.klp.o \
lib/livepatch/test_klp_convert1.ko | \
grep saved_command_line
convert_rela: oldsec: .rela.text rela @ 0x1279670 rela->sym @ 0x12791f0 (.klp.sym.vmlinux.saved_command_line,0) offset: 0x3
convert_rela: oldsec: .rela.text rela @ 0x1279cd0 rela->sym @ 0x12791f0 (.klp.sym.vmlinux.saved_command_line,0) offset: 0x9a
move_rela: rela @ 0x1279670 rela->sym @ 0x12791f0 (.klp.sym.vmlinux.saved_command_line,0) offset: 0x3
main: skipping rela @ 0x1279cd0 rela->sym @ 0x12791f0 (.klp.sym.vmlinux.saved_command_line,0) (!must_convert)
I think the problem is:
- Relas at different offsets, but for the same symbol may share symbol
storage. Note the same rela->sym value above.
- Before d59cadc0a8f8 ("[squash] klp-convert: make convert_rela()
list-safe"), convert_rela() iterated through the entire section's
relas, moving any of the same name. This was determined not to be
list safe when moving consecutive relas in the linked list.
- After d59cadc0a8f8 ("[squash] klp-convert: make convert_rela()
list-safe"), convert_rela() still iterates through the section relas,
but only updates r1->sym->klp_rela_sec instead of moving them.
move_rela() was added to be called by the for-each-rela loop in
main().
- Bug 1: klp_rela_sec probably belongs in struct rela and not struct
symbol
- Bug 2: the main loop skips over second, third, etc. matching relas
anyway as the shared symbol name will have already been converted
The following fix might not be elegant, but I can't think of a clever
way to handle the original issue d59cadc0a8f8 ("[squash] klp-convert:
make convert_rela() list-safe") as well as these resulting regressions.
So I broke out the moving of relas to a seperate loop. That is probably
worth a comment and at the same time we might be able to drop some of
these other "safe" loop traversals for ordinary list_for_each_entry.
-- Joe
-->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8--
diff --git a/scripts/livepatch/elf.h b/scripts/livepatch/elf.h
index 7551409..57a8242 100644
--- a/scripts/livepatch/elf.h
+++ b/scripts/livepatch/elf.h
@@ -33,7 +33,6 @@ struct symbol {
struct list_head list;
GElf_Sym sym;
struct section *sec;
- struct section *klp_rela_sec;
char *name;
unsigned int idx;
unsigned char bind, type;
@@ -45,6 +44,7 @@ struct rela {
struct list_head list;
GElf_Rela rela;
struct symbol *sym;
+ struct section *klp_rela_sec;
unsigned int type;
unsigned long offset;
int addend;
diff --git a/scripts/livepatch/klp-convert.c b/scripts/livepatch/klp-convert.c
index b5873ab..50d6471 100644
--- a/scripts/livepatch/klp-convert.c
+++ b/scripts/livepatch/klp-convert.c
@@ -525,7 +525,7 @@ static bool convert_rela(struct section *oldsec, struct rela *r,
list_for_each_entry_safe(r1, r2, &oldsec->relas, list) {
if (r1->sym->name == r->sym->name) {
- r1->sym->klp_rela_sec = sec;
+ r1->klp_rela_sec = sec;
}
}
return true;
@@ -535,7 +535,7 @@ static void move_rela(struct rela *r)
{
/* Move the converted rela to klp rela section */
list_del(&r->list);
- list_add(&r->list, &r->sym->klp_rela_sec->relas);
+ list_add(&r->list, &r->klp_rela_sec->relas);
}
/* Checks if given symbol name matches a symbol in exp_symbols */
@@ -687,8 +687,11 @@ int main(int argc, const char **argv)
return -1;
}
}
+ }
- move_rela(rela);
+ list_for_each_entry_safe(rela, tmprela, &sec->relas, list) {
+ if (is_converted(rela->sym->name))
+ move_rela(rela);
}
}
next prev parent reply other threads:[~2019-06-25 19:08 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-05-09 14:38 [PATCH v4 00/10] klp-convert livepatch build tooling Joe Lawrence
2019-05-09 14:38 ` [PATCH v4 01/10] livepatch: Create and include UAPI headers Joe Lawrence
2019-05-09 14:38 ` [PATCH v4 02/10] kbuild: Support for Symbols.list creation Joe Lawrence
2019-05-21 13:48 ` Masahiro Yamada
2019-05-09 14:38 ` [PATCH v4 03/10] livepatch: Add klp-convert tool Joe Lawrence
2019-07-31 2:50 ` Masahiro Yamada
2019-07-31 3:36 ` Masahiro Yamada
2019-08-09 18:42 ` Joe Lawrence
2019-08-13 1:15 ` Masahiro Yamada
2019-05-09 14:38 ` [PATCH v4 04/10] livepatch: Add klp-convert annotation helpers Joe Lawrence
2019-05-09 14:38 ` [PATCH v4 05/10] modpost: Integrate klp-convert Joe Lawrence
2019-05-09 14:38 ` [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules Joe Lawrence
2019-07-31 5:58 ` Masahiro Yamada
2019-08-12 15:56 ` Joe Lawrence
2019-08-15 15:05 ` Masahiro Yamada
2019-08-16 8:19 ` Miroslav Benes
2019-08-16 12:43 ` Joe Lawrence
2019-08-16 19:01 ` Joe Lawrence
2019-08-19 3:50 ` Masahiro Yamada
2019-08-19 15:55 ` Joe Lawrence
2019-08-20 7:54 ` Miroslav Benes
2019-08-19 3:49 ` Masahiro Yamada
2019-08-19 7:31 ` Miroslav Benes
2019-08-19 16:02 ` Joe Lawrence
2019-08-22 3:35 ` Masahiro Yamada
2019-08-13 10:26 ` Miroslav Benes
2019-05-09 14:38 ` [PATCH v4 07/10] livepatch: Add sample livepatch module Joe Lawrence
2019-08-16 11:35 ` Masahiro Yamada
2019-08-16 12:47 ` Joe Lawrence
2019-05-09 14:38 ` [PATCH v4 08/10] documentation: Update on livepatch elf format Joe Lawrence
2019-05-09 14:38 ` [PATCH v4 09/10] livepatch/selftests: add klp-convert Joe Lawrence
2019-05-09 14:38 ` [PATCH v4 10/10] livepatch/klp-convert: abort on special sections Joe Lawrence
2019-06-13 13:00 ` [PATCH v4 00/10] klp-convert livepatch build tooling Miroslav Benes
2019-06-13 13:15 ` Joe Lawrence
2019-06-13 20:48 ` Joe Lawrence
2019-06-14 8:34 ` Petr Mladek
2019-06-14 14:20 ` Joe Lawrence
2019-06-14 16:36 ` Libor Pechacek
2019-06-25 11:36 ` Miroslav Benes
2019-06-25 13:24 ` Joe Lawrence
2019-06-25 19:08 ` Joe Lawrence [this message]
2019-06-26 10:27 ` Miroslav Benes
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190625190836.GL20356@redhat.com \
--to=joe.lawrence@redhat.com \
--cc=linux-kbuild@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=live-patching@vger.kernel.org \
--cc=mbenes@suse.cz \
--cc=pmladek@suse.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).