[PATCH v4] livepatch: Cleanup module page permission changes

[PATCH v4] livepatch: Cleanup module page permission changes

[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 <mbenes@suse.cz>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
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(-)

diff --git a/arch/x86/kernel/livepatch.c b/arch/x86/kernel/livepatch.c
index bcc06e8..92fc1a5 100644
--- a/arch/x86/kernel/livepatch.c
+++ b/arch/x86/kernel/livepatch.c
@@ -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);
 }
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 94893e8..bc2c85c 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -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,
-- 
2.4.3

Re: [PATCH v4] livepatch: Cleanup module page permission changes

[Jiri Kosina]

On Thu, 3 Dec 2015, Josh Poimboeuf wrote:

> 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 <mbenes@suse.cz>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
> 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,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v4] livepatch: Cleanup module page permission changes

[Rusty Russell]

Jiri Kosina <jikos@kernel.org> writes:
> On Thu, 3 Dec 2015, Josh Poimboeuf wrote:
>
>> 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 <mbenes@suse.cz>
>> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
>> ---
>> 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

Hmm, that's always a bit icky.

> (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 :)

> (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.

> (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...

> 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.

Re: [PATCH v4] livepatch: Cleanup module page permission changes

[Josh Poimboeuf]

On Fri, Dec 04, 2015 at 01:11:29AM +0100, Jiri Kosina wrote:
> On Thu, 3 Dec 2015, Josh Poimboeuf wrote:
> 
> > 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 <mbenes@suse.cz>
> > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > ---
> > 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).

[ 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?

-- 
Josh

Re: [PATCH v4] livepatch: Cleanup module page permission changes

[Josh Poimboeuf]

On Fri, Dec 04, 2015 at 07:27:24AM -0600, Josh Poimboeuf wrote:
> On Fri, Dec 04, 2015 at 01:11:29AM +0100, Jiri Kosina wrote:
> > On Thu, 3 Dec 2015, Josh Poimboeuf wrote:
> > 
> > > 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 <mbenes@suse.cz>
> > > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > > ---
> > > 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).
> 
> [ off-list ]

Or not :-)

> 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?

-- 
Josh

Re: [PATCH v4] livepatch: Cleanup module page permission changes

[Petr Mladek]

On Thu 2015-12-03 16:33:26, Josh Poimboeuf wrote:
> 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 <mbenes@suse.cz>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>

This patch looks fine to me.

Reviewed-by: Petr Mladek <pmladek@suse.com>

Thanks,
Petr

Re: [PATCH v4] livepatch: Cleanup module page permission changes

[Jiri Kosina]

On Fri, 4 Dec 2015, Rusty Russell wrote:

> > 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,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v4] livepatch: Cleanup module page permission changes

[Jiri Kosina]

On Thu, 3 Dec 2015, Josh Poimboeuf wrote:

> 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 <mbenes@suse.cz>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
> 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,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v4] livepatch: Cleanup module page permission changes

[Jiri Kosina]

On Fri, 4 Dec 2015, Josh Poimboeuf wrote:

> > (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).
> 
> [ 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?

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,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v4] livepatch: Cleanup module page permission changes

[Josh Poimboeuf]

On Fri, Dec 04, 2015 at 10:57:45PM +0100, Jiri Kosina wrote:
> On Fri, 4 Dec 2015, Josh Poimboeuf wrote:
> 
> > > (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).
> > 
> > [ 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?
> 
> 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.

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

-- 
Josh