add support for shadow variables by jpoimboe · Pull Request #380 · dynup/kpatch

jpoimboe · 2014-08-29T17:07:46Z

This adds support for shadow variables, which allow you to add new
"shadow" fields to existing data structures.

NOTICE: To use this feature, you have to tell kpatch-build exactly which
object files are affected by using the -t option. For example, if the
patch modifies fs/proc/array.c, kernel/exit.c, and kernel/fork.c:

kpatch-build -t fs/proc/array.o -t kernel/exit.o -t kernel/fork.o my.patch

This limitation is caused by the fact that the kpatch core module
(kpatch.ko) is a module, so the link step of the patched kernel fails
when it can't link to the kpatch_shadow_* functions. If the core module
gets statically compiled into the kernel, this limitation goes away.

Fixes #314.

It works with the following test patch:

Index: src/kernel/fork.c
===================================================================
--- src.orig/kernel/fork.c
+++ src/kernel/fork.c
@@ -1572,6 +1572,7 @@ struct task_struct *fork_idle(int cpu)
  * It copies the process, and if successful kick-starts
  * it and waits for it to finish using the VM if required.
  */
+#include "kpatch-macros.h"
 long do_fork(unsigned long clone_flags,
          unsigned long stack_start,
          unsigned long stack_size,
@@ -1622,6 +1623,12 @@ long do_fork(unsigned long clone_flags,
    if (!IS_ERR(p)) {
        struct completion vfork;
        struct pid *pid;
+       int *newpid;
+       static int ctr = 0;
+
+       KPATCH_SHADOW_CREATE(p, newpid, GFP_KERNEL);
+       if (newpid)
+           *newpid = ctr++;

        trace_sched_process_fork(current, p);

Index: src/fs/proc/array.c
===================================================================
--- src.orig/fs/proc/array.c
+++ src/fs/proc/array.c
@@ -337,13 +337,20 @@ static inline void task_seccomp(struct s
 #endif
 }

+#include "kpatch-macros.h"
 static inline void task_context_switch_counts(struct seq_file *m,
                        struct task_struct *p)
 {
+   int *newpid;
+
    seq_printf(m,   "voluntary_ctxt_switches:\t%lu\n"
            "nonvoluntary_ctxt_switches:\t%lu\n",
            p->nvcsw,
            p->nivcsw);
+
+   KPATCH_SHADOW_GET(p, newpid);
+   if (newpid)
+       seq_printf(m, "newpid:\t%d\n", *newpid);
 }

 static void task_cpus_allowed(struct seq_file *m, struct task_struct *task)
Index: src/kernel/exit.c
===================================================================
--- src.orig/kernel/exit.c
+++ src/kernel/exit.c
@@ -694,10 +694,12 @@ static void check_stack_usage(void)
 static inline void check_stack_usage(void) {}
 #endif

+#include "kpatch-macros.h"
 void do_exit(long code)
 {
    struct task_struct *tsk = current;
    int group_dead;
+   int *newpid;

    profile_task_exit(tsk);

@@ -790,6 +792,8 @@ void do_exit(long code)
    exit_task_work(tsk);
    exit_thread();

+   KPATCH_SHADOW_DESTROY(tsk, newpid);
+
    /*
     * Flush inherited counters to the parent - before the parent
     * gets woken up by child-exit notifications.

With the following arguments to kpatch-build:

kpatch-build -t fs/proc/array.o -t kernel/exit.o -t kernel/fork.o fork-shadow.patch

With this patch loaded, any new tasks get a new "newpid" field which can be seen in /proc/<pid>/status.

jpoimboe · 2014-09-05T13:47:41Z

Added a new commit to update the comments to hopefully make them clearer.

sjenning · 2014-09-05T21:02:11Z

kmod/core/core.c

this seems like the perfect place for RCU with reads (gets) being frequent and updates being infrequent (create/destroy). can we use RCU instead of the lock?

Hm, maybe so.

cormander · 2014-09-06T04:12:04Z

I'm not able to build with this patch on this branch;

$ kpatch-build -t fs/proc/array.o -t kernel/exit.o -t kernel/fork.o fork.patch 
Using cache at /home/corman/.kpatch/src
Testing patch file
checking file fs/proc/array.c
checking file kernel/exit.c
checking file kernel/fork.c
Building original kernel
Building patched kernel
Extracting new and modified ELF sections
ERROR: invalid ancestor fs/proc/array.o for fs/proc/array.o

Tried this on EL7, as well as a freshly updated FC20 image. Any ideas?

I threw a -t vmlinux in there but then the build died with unresolved symbols. I appended --warn-unresolved-symbols to LDFLAGS_vmlinux in my kernel's Makefile which fixed the problem.

jpoimboe · 2014-09-08T15:23:58Z

Rebased the commit based on review comments:

Added --warn-unresolved-symbols to LDFLAGS
Removed "clever" macros and just made them direct function calls to make usage clearer and more straightforward
Renamed _create/_destroy to _alloc/_free
Added an integration test

jpoimboe · 2014-09-08T16:18:08Z

Added a commit which converts the locking to RCU.

jpoimboe · 2014-09-08T16:43:21Z

Rebased the RCU commit to have proper locking (based on our IRC discussion)

This adds support for shadow variables, which allow you to add new "shadow" fields to existing data structures. To allow patches to call the shadow functions in the core module, I had to add a funky hack to use --warn-unresolved-symbols when linking, which allows the patched vmlinux to link with the missing symbols. I also added greps to the log file to ensure that only unresolved symbols to kpatch_shadow_* are allowed. We can remove this hack once the core module gets moved into the kernel tree. Fixes dynup#314.

jpoimboe · 2014-09-08T18:38:50Z

Rebased and squashed with more RCU fixes as discussed on IRC, as well as moving the shadow code out into its own .c file.

add support for shadow variables

sjenning reviewed Sep 5, 2014
View reviewed changes

cormander mentioned this pull request Sep 6, 2014

Warn on unresolved symbols, rather than exit the build jpoimboe/kpatch#1

Closed

jpoimboe force-pushed the shadow-variables branch from 3fd07fa to bf7456c Compare September 8, 2014 15:20

jpoimboe changed the title ~~RFC: add support for shadow variables~~ add support for shadow variables Sep 8, 2014

jpoimboe force-pushed the shadow-variables branch from 5b93d91 to 3007e94 Compare September 8, 2014 16:43

jpoimboe mentioned this pull request Sep 8, 2014

document KPATCH_LOAD_HOOK and KPATCH_UNLOAD_HOOK #389

Closed

jpoimboe force-pushed the shadow-variables branch from 3007e94 to 4dee892 Compare September 8, 2014 18:37

sjenning added a commit that referenced this pull request Sep 8, 2014

Merge pull request #380 from jpoimboe/shadow-variables

3343eed

add support for shadow variables

sjenning merged commit 3343eed into dynup:master Sep 8, 2014

jpoimboe deleted the shadow-variables branch September 9, 2014 18:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for shadow variables#380

add support for shadow variables#380
sjenning merged 1 commit intodynup:masterfrom
jpoimboe:shadow-variables

jpoimboe commented Aug 29, 2014

Uh oh!

jpoimboe commented Sep 5, 2014

Uh oh!

sjenning Sep 5, 2014

Uh oh!

jpoimboe Sep 8, 2014

Uh oh!

cormander commented Sep 6, 2014

Uh oh!

jpoimboe commented Sep 8, 2014

Uh oh!

jpoimboe commented Sep 8, 2014

Uh oh!

jpoimboe commented Sep 8, 2014

Uh oh!

jpoimboe commented Sep 8, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

jpoimboe commented Aug 29, 2014

Uh oh!

jpoimboe commented Sep 5, 2014

Uh oh!

sjenning Sep 5, 2014

Choose a reason for hiding this comment

Uh oh!

jpoimboe Sep 8, 2014

Choose a reason for hiding this comment

Uh oh!

cormander commented Sep 6, 2014

Uh oh!

jpoimboe commented Sep 8, 2014

Uh oh!

jpoimboe commented Sep 8, 2014

Uh oh!

jpoimboe commented Sep 8, 2014

Uh oh!

jpoimboe commented Sep 8, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants