RFC - 4.9 unwinder #659

joe-lawrence · 2017-01-20T16:30:52Z

This is based on @flaming-toast 's 4.7-changes branch, so I don't know how well the github review interface will work, but here goes.

patch 1 - I don't know if this is a kernel config or v4.9 thing, but kpatch-build was complaining about initramfs_data.o so just skipped the file so I could continue on.
patch 2 - adds in v4.9+ support for the shiny new unwind_{start,done,next_frame} interface. Much simpler than the deprecated dump_trace and callback mechanism! (Good work, @jpoimboe ) My modifications are pretty straightforward, I tested with a patch that changed n_tty_read, which an agetty process will always be sitting on... and verified the safety check detected and returned -EBUSY.

Hope this is on the right track. If this is easier to review as a branch off master rather than 4.7-changes, let me know and I can respin.

jpoimboe · 2017-01-20T18:01:14Z

Thanks for looking at this. Something just occurred to me that somehow eluded me before. Instead of using the x86-specific interfaces, how about we use the arch-independent save_stack_trace_tsk(). That interface is stable and also easy to use, and can be used for old and new kernels alike.

joe-lawrence · 2017-01-20T20:29:58Z

Whoops... I had only checked that unwind_* were exported but overlooked their home in arch/x86.

v2, I modified for save_stack_trace_tsk and a lot of kernel version specific code dropped out. Same test (n_tty_read patch = -EBUSY and the stock cmdline patch succeeds). I can run the whole test suite on pre/post v4.9 kernels if this is on the right track.

jpoimboe

Looks good, added one minor comment.

jpoimboe · 2017-01-23T16:34:03Z

kmod/core/core.c

+			.skip		= 0,
+		};
+
+                save_stack_trace_tsk(t, &trace);


It would be good to report an error if there were more than 64 functions on the stack and the entries array filled up.

Will do. BTW, I picked 64 since that was the value used in the proc stack interface. It could just as easily be a module parameter (default to 64) if you think it should be more flexible.

I think a hard coded value is fine, as we make a loud warning when it's not big enough.

Er, I meant to say as long as we make a loud warning. Having trouble with English this week!

Got it, how about "kpatch: more than X trace entries!" with a stack dump of the ones we did get?

Sounds good, and then it should also return an error and abort the patch.

ERROR: initramfs_data.o: 2 unsupported section change(s) initramfs_data.o: changed section .init.ramfs not selected for inclusion initramfs_data.o: changed section .init.ramfs.info not selected for inclusion /usr/local/libexec/kpatch/create-diff-object: unreconcilable difference cmdline.o: changed function: cmdline_proc_show ERROR: 1 error(s) encountered. Check /root/.kpatch/build.log for more details.

joe-lawrence · 2017-01-24T21:32:02Z

v3 - rebased against master, added warnings when save_stack_trace_tsk() exceeds the 64 entries we've allocated, whitespace indenting fixups.

Tested the following kernel/configurations:

4.9.0 # CONFIG_LIVEPATCH is not set
4.7.0 # CONFIG_LIVEPATCH is not set
RHEL7 - 3.10.0-543.el7.bz1369704_v2.x86_64

Each for the following scenarios:

0 - kpatch: builds
1 - kpatch_cmdline_string: builds and loads successfully
2 - n_tty_read patch: builds and fails safety check, as expected. dmesg x 5:

  kpatch: activeness safety check failed for n_tty_read
  kpatch: PID: 715 Comm: agetty
  kpatch:   [<ffffffff810d32d5>] wait_woken+0x65/0x80
  kpatch:   [<ffffffff8144e0d3>] n_tty_read+0x3d3/0x8c0
  kpatch:   [<ffffffff81447eb2>] tty_read+0x92/0xf0
  kpatch:   [<ffffffff8122c387>] __vfs_read+0x37/0x130
  kpatch:   [<ffffffff8122d6ac>] vfs_read+0x8c/0x130
  kpatch:   [<ffffffff8122ebc5>] SyS_read+0x55/0xc0
  kpatch:   [<ffffffff81003a47>] do_syscall_64+0x67/0x180
  kpatch:   [<ffffffff8170146b>] entry_SYSCALL64_slow_path+0x25/0x25
  kpatch:   [<ffffffffffffffff>] 0xffffffffffffffff

3 - MAX_STACK_TRACE_DEPTH=2 (for debug): builds and fails to load patch. dmesg x 5:

  kpatch: more than 2 trace entries!
  kpatch: PID: 1 Comm: systemd
  kpatch:   [<ffffffff81278108>] ep_poll+0x2f8/0x3c0
  kpatch:   [<ffffffff8127963c>] SyS_epoll_wait+0xbc/0xe0

I couldn't think of a better errno value for when we've exceeded MAX_STACK_TRACE_DEPTH, so I just used -EBUSY like the other cases. Any suggestions?

jpoimboe · 2017-01-25T02:45:26Z

kmod/core/core.c

-	};
+	entries = kmalloc(MAX_STACK_TRACE_DEPTH * sizeof(*entries), GFP_KERNEL);
+	if (!entries)
+		return -ENOMEM;


I think sleeping isn't allowed in stop_machine(), so this should probably be allocated on the stack.

Ah, good catch! I'll fix that up.

The dump_trace interface was deprecated in v4.9: instead of adding yet another kernel-specific code block to kpatch's stack safety checks, use save_stack_trace_tsk. It's relatively simple (no callbacks like dump_trace), arch-independent, and its interface is stable across kernel releases. Fixes: dynup#623.

joe-lawrence · 2017-01-25T19:50:43Z

v4

as seen in ftrace code, added trace.entries[i] == ULONG_MAX early breaks to avoid dumping out useless lines like: [<ffffffffffffffff>] 0xffffffffffffffff
made struct stack_trace a module global to avoid kmalloc from stop_machine context. This should be safe as kpatch will only run the stop_machine function on a single CPU. It could have gone on the kpatch_verify_activeness_safety() stack, but an .entries array for 64 functions would be 512 bytes. This change required a few code adjustments, most interestingly a reset of trace.nr_entries to 0 on each call to save_stack_trace_tsk().

Tested: same kernel/config + tests as v3, this time without bogus [<ffffffffffffffff>] 0xffffffffffffffff lines.

jpoimboe · 2017-01-25T20:06:19Z

👍

joe-lawrence force-pushed the 4.9-unwinder branch from 148b81e to 479df05 Compare January 20, 2017 20:26

jpoimboe reviewed Jan 23, 2017

View reviewed changes

joe-lawrence force-pushed the 4.9-unwinder branch from 479df05 to c503c66 Compare January 24, 2017 21:31

jpoimboe reviewed Jan 25, 2017

View reviewed changes

joe-lawrence force-pushed the 4.9-unwinder branch from c503c66 to 586feb4 Compare January 25, 2017 19:50

flaming-toast merged commit 8e1aef2 into dynup:master Jan 30, 2017

RFC - 4.9 unwinder #659

RFC - 4.9 unwinder #659

Uh oh!

Conversation

joe-lawrence commented Jan 20, 2017

Uh oh!

jpoimboe commented Jan 20, 2017

Uh oh!

joe-lawrence commented Jan 20, 2017

Uh oh!

jpoimboe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joe-lawrence commented Jan 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joe-lawrence commented Jan 25, 2017

Uh oh!

jpoimboe commented Jan 25, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants