Live-Patching Archive on lore.kernel.org
 help / color / Atom feed
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
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);
 		}
 	}
 

  parent reply index

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20190509143859.9050-1-joe.lawrence@redhat.com>
     [not found] ` <alpine.LSU.2.21.1906131451560.22698@pobox.suse.cz>
     [not found]   ` <b1a627a4-3702-9689-6c03-0c2123c06a2d@redhat.com>
     [not found]     ` <c9021573-11c6-b576-0aa6-97754c98a06e@redhat.com>
     [not found]       ` <20190614083435.uq3mk6mprbatysol@pathway.suse.cz>
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
     [not found] ` <20190509143859.9050-4-joe.lawrence@redhat.com>
2019-07-31  2:50   ` [PATCH v4 03/10] livepatch: Add klp-convert tool Masahiro Yamada
2019-07-31  3:36     ` Masahiro Yamada
2019-08-09 18:42       ` Joe Lawrence
2019-08-13  1:15         ` Masahiro Yamada
     [not found] ` <20190509143859.9050-7-joe.lawrence@redhat.com>
2019-07-31  5:58   ` [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules 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
     [not found] ` <20190509143859.9050-8-joe.lawrence@redhat.com>
2019-08-16 11:35   ` [PATCH v4 07/10] livepatch: Add sample livepatch module Masahiro Yamada
2019-08-16 12:47     ` Joe Lawrence

Reply instructions:

You may reply publically 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

Live-Patching Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/live-patching/0 live-patching/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 live-patching live-patching/ https://lore.kernel.org/live-patching \
		live-patching@vger.kernel.org
	public-inbox-index live-patching

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.live-patching


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git