livepatch: Cleanup module page permission changes

PATCH In review Josh Poimboeuf 4 participants 10 messages Dec 3, 2015 – Dec 4, 2015

Subsystems (2)

x86 Live patching

lore

Share & tools

lore

Josh Poimboeuf

author

2015-12-03 22:33 UTC #0 lore raw

PATCH v4 1/1 livepatch: Cleanup module page permission changes

Calling set_memory_rw() and set_memory_ro() for every iteration of the loop in klp_write_object_relocations() is messy, inefficient, and error-prone.

Change all the read-only pages to read-write before the loop and convert them back to read-only again afterwards.

Signed-off-by Josh Poimboeuf <[email protected]>

---
Based on the following branches:
- git://git.kernel.org/pub/scm/linux/kernel/git/jikos/livepatching.git for-4.5/core
- git://git.kernel.org/pub/scm/linux/kernel/git/rusty/linux.git modules-next

- v4: rebase onto Chris's sympos changes
- v3: use new module_{disable,enable}_ro() functions (in linux-next)

 arch/x86/kernel/livepatch.c | 25 ++-----------------------
 kernel/livepatch/core.c     | 16 +++++++++++-----
 2 files changed, 13 insertions(+), 28 deletions(-)

arch/x86/kernel/livepatch.c +2 ~~-23~~

index bcc06e8..92fc1a5 100644
@@ -20,8 +20,6 @@
 
 #include <linux/module.h>
 #include <linux/uaccess.h>
-#include <asm/cacheflush.h>
-#include <asm/page_types.h>
 #include <asm/elf.h>
 #include <asm/livepatch.h>

@@ -38,8 +36,7 @@
 int klp_write_module_reloc(struct module *mod, unsigned long type,
 			   unsigned long loc, unsigned long value)
 {
-	int ret, numpages, size = 4;
-	bool readonly;
+	size_t size = 4;
 	unsigned long val;
 	unsigned long core = (unsigned long)mod->core_layout.base;
 	unsigned long core_size = mod->core_layout.size;

@@ -69,23 +66,5 @@ int klp_write_module_reloc(struct module *mod, unsigned long type,
 		/* loc does not point to any symbol inside the module */
 		return -EINVAL;
 
-	readonly = false;
-
-#ifdef CONFIG_DEBUG_SET_MODULE_RONX
-	if (loc < core + mod->core_layout.ro_size)
-		readonly = true;
-#endif
-
-	/* determine if the relocation spans a page boundary */
-	numpages = ((loc & PAGE_MASK) == ((loc + size) & PAGE_MASK)) ? 1 : 2;
-
-	if (readonly)
-		set_memory_rw(loc & PAGE_MASK, numpages);
-
-	ret = probe_kernel_write((void *)loc, &val, size);
-
-	if (readonly)
-		set_memory_ro(loc & PAGE_MASK, numpages);
-
-	return ret;
+	return probe_kernel_write((void *)loc, &val, size);
 }

kernel/livepatch/core.c +11 -5

index 94893e8..bc2c85c 100644
@@ -28,6 +28,7 @@
 #include <linux/list.h>
 #include <linux/kallsyms.h>
 #include <linux/livepatch.h>
+#include <asm/cacheflush.h>
 
 /**
  * struct klp_ops - structure for tracking registered ftrace ops structs

@@ -232,7 +233,7 @@ static int klp_find_external_symbol(struct module *pmod, const char *name,
 static int klp_write_object_relocations(struct module *pmod,
 					struct klp_object *obj)
 {
-	int ret;
+	int ret = 0;
 	unsigned long val;
 	struct klp_reloc *reloc;

@@ -242,13 +243,16 @@ static int klp_write_object_relocations(struct module *pmod,
 	if (WARN_ON(!obj->relocs))
 		return -EINVAL;
 
+	module_disable_ro(pmod);
+
 	for (reloc = obj->relocs; reloc->name; reloc++) {
 		/* discover the address of the referenced symbol */
 		if (reloc->external) {
 			if (reloc->sympos > 0) {
 				pr_err("non-zero sympos for external reloc symbol '%s' is not supported\n",
 				       reloc->name);
-				return -EINVAL;
+				ret = -EINVAL;
+				goto out;
 			}
 			ret = klp_find_external_symbol(pmod, reloc->name, &val);
 		} else

@@ -257,18 +261,20 @@ static int klp_write_object_relocations(struct module *pmod,
 						     reloc->sympos,
 						     &val);
 		if (ret)
-			return ret;
+			goto out;
 
 		ret = klp_write_module_reloc(pmod, reloc->type, reloc->loc,
 					     val + reloc->addend);
 		if (ret) {
 			pr_err("relocation failed for symbol '%s' at 0x%016lx (%d)\n",
 			       reloc->name, val, ret);
-			return ret;
+			goto out;
 		}
 	}
 
-	return 0;
+out:
+	module_enable_ro(pmod);
+	return ret;
 }
 
 static void notrace klp_ftrace_handler(unsigned long ip,

Signature · 1 line

2.4.3

Jiri Kosina

2015-12-04 00:11 UTC #1 lore raw

Quoted 9 lines · Josh Poimboeuf

Calling set_memory_rw() and set_memory_ro() for every iteration of the loop in klp_write_object_relocations() is messy, inefficient, and error-prone.
Change all the read-only pages to read-write before the loop and convert them back to read-only again afterwards.
Suggested-by Miroslav Benes <[email protected]>
Signed-off-by Josh Poimboeuf <[email protected]>

Quoted 7 lines

---
Based on the following branches:
- git://git.kernel.org/pub/scm/linux/kernel/git/jikos/livepatching.git for-4.5/core
- git://git.kernel.org/pub/scm/linux/kernel/git/rusty/linux.git modules-next
- v4: rebase onto Chris's sympos changes
- v3: use new module_{disable,enable}_ro() functions (in linux-next)

Rusty,

how would you like to handle this cross-tree dependency?

My proposals:

(1) I pull your 'modules-next' branch, apply this patch on top, and wait for your merge with Linus and send merge request afterwards
(2) If you are okay with rebasing your tree (seems like this is ocassionally happening), how about you prepare a branch that'd have just b3212ec77 ("module: keep percpu symbols in module's symtab") on top of some common base, I merge it, and the cross-dependency is gone
(3) I cherry-pick b3212ec77 ("module: keep percpu symbols in module's symtab") from your tree, and apply this on top. git will handle duplicate commits when Linus merges both just fine
(4) I sign this patch off and you merge it

(4) seems really outside the regular process. (1) is really tricky wrt. coordination of timing during the merge window. I'd prefer (2) (more git-ish way of doing things, but would require you rebasing your tree) or eventually (3) (git will handle this with grace).

What do you think?

Thanks a lot,

Signature · 2 lines

Jiri Kosina
SUSE Labs

Rusty Russell

2015-12-04 01:59 UTC #2 lore raw

Quoted 19 lines · Jiri Kosina

Quoted 9 lines · Josh Poimboeuf
Calling set_memory_rw() and set_memory_ro() for every iteration of the loop in klp_write_object_relocations() is messy, inefficient, and error-prone.
Change all the read-only pages to read-write before the loop and convert them back to read-only again afterwards.
Suggested-by Miroslav Benes <[email protected]>
Signed-off-by Josh Poimboeuf <[email protected]>
Quoted 7 lines
---
Based on the following branches:
- git://git.kernel.org/pub/scm/linux/kernel/git/jikos/livepatching.git for-4.5/core
- git://git.kernel.org/pub/scm/linux/kernel/git/rusty/linux.git modules-next
- v4: rebase onto Chris's sympos changes
- v3: use new module_{disable,enable}_ro() functions (in linux-next)

Quoted 8 lines

Rusty,
how would you like to handle this cross-tree dependency?
My proposals:
(1) I pull your 'modules-next' branch, apply this patch on top, and wait for your merge with Linus and send merge request afterwards

Hmm, that's always a bit icky.

Quoted 4 lines

(2) If you are okay with rebasing your tree (seems like this is ocassionally happening), how about you prepare a branch that'd have just b3212ec77 ("module: keep percpu symbols in module's symtab") on top of some common base, I merge it, and the cross-dependency is gone

I don't like to rebase, but I am *always* happy to give patches away :)

Quoted 3 lines

(3) I cherry-pick b3212ec77 ("module: keep percpu symbols in module's symtab") from your tree, and apply this on top. git will handle duplicate commits when Linus merges both just fine

That's pretty ugly.

Quoted 6 lines

(4) I sign this patch off and you merge it
(4) seems really outside the regular process. (1) is really tricky wrt. coordination of timing during the merge window. I'd prefer (2) (more git-ish way of doing things, but would require you rebasing your tree) or eventually (3) (git will handle this with grace).

The last won't work anyway, since I'd need to grab stuff from your tree...

Quoted 1 line

What do you think?

Please cherry-pick my whole module-next tree. They have my SOB already. You can push them to Linus along with your livepatch stuff at your convenience for the merge window.

Once you've done that, I'll rebase modules-next down to nothing. If something else comes in, I'll either add it there or toss it to you, depending on whether it conflicts or not.

Thanks,
Rusty.

Jiri Kosina

2015-12-04 21:54 UTC #3 lore raw

Quoted 9 lines · Rusty Russell

Quoted 1 line
What do you think?
Please cherry-pick my whole module-next tree. They have my SOB already. You can push them to Linus along with your livepatch stuff at your convenience for the merge window.
Once you've done that, I'll rebase modules-next down to nothing. If something else comes in, I'll either add it there or toss it to you, depending on whether it conflicts or not.

Sounds good, thanks.

I've just done that -- your patches went to livepatching.git#from-rusty/modules-next and I'll be merging it to branch that goes to linux-next; so you can now rewind your tree.

Thanks again,

Signature · 2 lines

Jiri Kosina
SUSE Labs

Josh Poimboeuf

author

2015-12-04 13:27 UTC #4 lore raw

In reply to Jiri Kosina

Quoted 36 lines · Jiri Kosina

Quoted 9 lines · Josh Poimboeuf
Calling set_memory_rw() and set_memory_ro() for every iteration of the loop in klp_write_object_relocations() is messy, inefficient, and error-prone.
Change all the read-only pages to read-write before the loop and convert them back to read-only again afterwards.
Suggested-by Miroslav Benes <[email protected]>
Signed-off-by Josh Poimboeuf <[email protected]>
Quoted 7 lines
---
Based on the following branches:
- git://git.kernel.org/pub/scm/linux/kernel/git/jikos/livepatching.git for-4.5/core
- git://git.kernel.org/pub/scm/linux/kernel/git/rusty/linux.git modules-next
- v4: rebase onto Chris's sympos changes
- v3: use new module_{disable,enable}_ro() functions (in linux-next)
Rusty,
how would you like to handle this cross-tree dependency?
My proposals:
(1) I pull your 'modules-next' branch, apply this patch on top, and wait for your merge with Linus and send merge request afterwards
(2) If you are okay with rebasing your tree (seems like this is ocassionally happening), how about you prepare a branch that'd have just b3212ec77 ("module: keep percpu symbols in module's symtab") on top of some common base, I merge it, and the cross-dependency is gone
(3) I cherry-pick b3212ec77 ("module: keep percpu symbols in module's symtab") from your tree, and apply this on top. git will handle duplicate commits when Linus merges both just fine
(4) I sign this patch off and you merge it

Quoted 4 lines

(4) seems really outside the regular process. (1) is really tricky wrt. coordination of timing during the merge window. I'd prefer (2) (more git-ish way of doing things, but would require you rebasing your tree) or eventually (3) (git will handle this with grace).

[ off-list ]

Quick question. Just curious, because I'm new at this...

My impression was that #1 was standard operating procedure. Merge a (non-rebasable) modules branch into livepatch, and then make sure to submit the livepatch pull request after Rusty sends his, with a note in the mail to Linus stating the dependency. That seems pretty straightforward to me. Or am I missing something?

Signature · 1 line

Josh

Josh Poimboeuf

author

2015-12-04 13:30 UTC #5 lore raw

Quoted 37 lines · Josh Poimboeuf

Quoted 36 lines · Jiri Kosina
Quoted 9 lines · Josh Poimboeuf
Calling set_memory_rw() and set_memory_ro() for every iteration of the loop in klp_write_object_relocations() is messy, inefficient, and error-prone.
Change all the read-only pages to read-write before the loop and convert them back to read-only again afterwards.
Suggested-by Miroslav Benes <[email protected]>
Signed-off-by Josh Poimboeuf <[email protected]>
Quoted 7 lines
---
Based on the following branches:
- git://git.kernel.org/pub/scm/linux/kernel/git/jikos/livepatching.git for-4.5/core
- git://git.kernel.org/pub/scm/linux/kernel/git/rusty/linux.git modules-next
- v4: rebase onto Chris's sympos changes
- v3: use new module_{disable,enable}_ro() functions (in linux-next)
Rusty,
how would you like to handle this cross-tree dependency?
My proposals:
(1) I pull your 'modules-next' branch, apply this patch on top, and wait for your merge with Linus and send merge request afterwards
(2) If you are okay with rebasing your tree (seems like this is ocassionally happening), how about you prepare a branch that'd have just b3212ec77 ("module: keep percpu symbols in module's symtab") on top of some common base, I merge it, and the cross-dependency is gone
(3) I cherry-pick b3212ec77 ("module: keep percpu symbols in module's symtab") from your tree, and apply this on top. git will handle duplicate commits when Linus merges both just fine
(4) I sign this patch off and you merge it

Quoted 6 lines

Quoted 4 lines
(4) seems really outside the regular process. (1) is really tricky wrt. coordination of timing during the merge window. I'd prefer (2) (more git-ish way of doing things, but would require you rebasing your tree) or eventually (3) (git will handle this with grace).
[ off-list ]

Or not :-)

Quoted 7 lines

Quick question. Just curious, because I'm new at this...
My impression was that #1 was standard operating procedure. Merge a (non-rebasable) modules branch into livepatch, and then make sure to submit the livepatch pull request after Rusty sends his, with a note in the mail to Linus stating the dependency. That seems pretty straightforward to me. Or am I missing something?

Signature · 1 line

Josh

Jiri Kosina

2015-12-04 21:57 UTC #6 lore raw

In reply to Josh Poimboeuf

Quoted 11 lines · Josh Poimboeuf

Quoted 11 lines
(1) I pull your 'modules-next' branch, apply this patch on top, and wait for your merge with Linus and send merge request afterwards
(2) If you are okay with rebasing your tree (seems like this is ocassionally happening), how about you prepare a branch that'd have just b3212ec77 ("module: keep percpu symbols in module's symtab") on top of some common base, I merge it, and the cross-dependency is gone
(3) I cherry-pick b3212ec77 ("module: keep percpu symbols in module's symtab") from your tree, and apply this on top. git will handle duplicate commits when Linus merges both just fine
(4) I sign this patch off and you merge it

Quoted 6 lines

Quoted 4 lines
(4) seems really outside the regular process. (1) is really tricky wrt. coordination of timing during the merge window. I'd prefer (2) (more git-ish way of doing things, but would require you rebasing your tree) or eventually (3) (git will handle this with grace).
[ off-list ]

:-)

Quoted 7 lines

Quick question. Just curious, because I'm new at this...
My impression was that #1 was standard operating procedure. Merge a (non-rebasable) modules branch into livepatch, and then make sure to submit the livepatch pull request after Rusty sends his, with a note in the mail to Linus stating the dependency. That seems pretty straightforward to me. Or am I missing something?

It's one of the options, yes. The only drawback is that it introduces, in addition to the actual code cross-dependency, also maintainer timing cross-dependency, and it might easily go wrong during merge window. But I've done this quite a few times already, and it was rather smooth.

What I actually prefer doing in this case is have a common merge base as a separate branch that gets merged to both trees, and then it's not really important who merges first. But that'd require in-advance planning and structuring Rusty's tree for that, and that's probably not worth the hassle for these few patches.

Thanks,

Signature · 2 lines

Jiri Kosina
SUSE Labs

Josh Poimboeuf

author

2015-12-04 22:07 UTC #7 lore raw

Quoted 35 lines · Jiri Kosina

Quoted 11 lines · Josh Poimboeuf
Quoted 11 lines
(1) I pull your 'modules-next' branch, apply this patch on top, and wait for your merge with Linus and send merge request afterwards
(2) If you are okay with rebasing your tree (seems like this is ocassionally happening), how about you prepare a branch that'd have just b3212ec77 ("module: keep percpu symbols in module's symtab") on top of some common base, I merge it, and the cross-dependency is gone
(3) I cherry-pick b3212ec77 ("module: keep percpu symbols in module's symtab") from your tree, and apply this on top. git will handle duplicate commits when Linus merges both just fine
(4) I sign this patch off and you merge it
Quoted 6 lines
Quoted 4 lines
(4) seems really outside the regular process. (1) is really tricky wrt. coordination of timing during the merge window. I'd prefer (2) (more git-ish way of doing things, but would require you rebasing your tree) or eventually (3) (git will handle this with grace).
[ off-list ]
:-)
Quoted 7 lines
Quick question. Just curious, because I'm new at this...
My impression was that #1 was standard operating procedure. Merge a (non-rebasable) modules branch into livepatch, and then make sure to submit the livepatch pull request after Rusty sends his, with a note in the mail to Linus stating the dependency. That seems pretty straightforward to me. Or am I missing something?
It's one of the options, yes. The only drawback is that it introduces, in addition to the actual code cross-dependency, also maintainer timing cross-dependency, and it might easily go wrong during merge window. But I've done this quite a few times already, and it was rather smooth.

Quoted 5 lines

What I actually prefer doing in this case is have a common merge base as a separate branch that gets merged to both trees, and then it's not really important who merges first. But that'd require in-advance planning and structuring Rusty's tree for that, and that's probably not worth the hassle for these few patches.

Ah, got it. That does sound better, assuming there's some advance planning. Thanks for educating me :-)

Signature · 1 line

Josh

Petr Mladek

maintainer Reviewed-by on v4 1/1

2015-12-04 14:17 UTC #8 lore raw

In reply to Josh Poimboeuf

Quoted 9 lines · Josh Poimboeuf

Calling set_memory_rw() and set_memory_ro() for every iteration of the loop in klp_write_object_relocations() is messy, inefficient, and error-prone.
Change all the read-only pages to read-write before the loop and convert them back to read-only again afterwards.
Suggested-by Miroslav Benes <[email protected]>
Signed-off-by Josh Poimboeuf <[email protected]>

This patch looks fine to me.

Reviewed-by Petr Mladek <[email protected]>

Thanks,
Petr

Jiri Kosina

2015-12-04 21:54 UTC #9 lore raw

In reply to Josh Poimboeuf

Quoted 9 lines · Josh Poimboeuf

Calling set_memory_rw() and set_memory_ro() for every iteration of the loop in klp_write_object_relocations() is messy, inefficient, and error-prone.
Change all the read-only pages to read-write before the loop and convert them back to read-only again afterwards.
Suggested-by Miroslav Benes <[email protected]>
Signed-off-by Josh Poimboeuf <[email protected]>

Quoted 7 lines

---
Based on the following branches:
- git://git.kernel.org/pub/scm/linux/kernel/git/jikos/livepatching.git for-4.5/core
- git://git.kernel.org/pub/scm/linux/kernel/git/rusty/linux.git modules-next
- v4: rebase onto Chris's sympos changes
- v3: use new module_{disable,enable}_ro() functions (in linux-next)

Merged together with patches from Rusty's modules-next and applied to for-4.5/core. Thanks,

Signature · 2 lines

Jiri Kosina
SUSE Labs