Hi,
This patchset attempts to improve error handling when arming or disarming
ftrace-based kprobes. The current behavior is to simply WARN when ftrace
(un-)registration fails, without propagating the error code. This can lead
to confusing situations where, for example, register_kprobe()/enable_kprobe()
would return 0 indicating success even if arming via ftrace had failed. In
this scenario we'd end up with a non-functioning kprobe even though kprobe
registration (or enablement) returned success. In this patchset, we take
errors from ftrace into account and propagate the error when we cannot arm
or disarm a kprobe.
Below is an example that illustrates the problem using livepatch and
systemtap (which uses kprobes underneath). Both livepatch and kprobes use
ftrace ops with the IPMODIFY flag set, so registration at the same
function entry is limited to only one ftrace user.
Before
------
# modprobe livepatch-sample # patches cmdline_proc_show, ftrace ops has IPMODIFY set
# stap -e 'probe kernel.function("cmdline_proc_show").call { printf ("cmdline_proc_show\n"); }'
.. (nothing prints after reading /proc/cmdline) ..
The systemtap handler doesn't execute due to a kprobe arming failure caused
by a ftrace IPMODIFY conflict with livepatch, and there isn't an obvious
indication of error from systemtap (because register_kprobe() returned
success) unless the user inspects dmesg.
After
-----
# modprobe livepatch-sample
# stap -e 'probe kernel.function("cmdline_proc_show").call { printf ("cmdline_proc_show\n"); }'
WARNING: probe kernel.function("cmdline_proc_show@/home/jeyu/work/linux-next/fs/proc/cmdline.c:6").call (address 0xffffffffa82fe910) registration error (rc -16)
Although the systemtap handler doesn't execute (as it shouldn't), the
ftrace error is propagated and now systemtap prints a visible error message
stating that (kprobe) registration had failed (because register_kprobe()
returned an error), along with the propagated error code.
This patchset was based on Petr Mladek's original patchset (patches 2 and 3)
back in 2015, which improved kprobes error handling, found here:
https://lkml.org/lkml/2015/2/26/452
However, further work on this had been paused since then and the patches
were not upstreamed.
This patchset has been lightly sanity-tested (on linux-next) with kprobes,
kretprobes, and optimized kprobes. It passes the kprobes smoke test, but
more testing is greatly appreciated.
Changes from v4:
- Switch from WARN() to pr_debug() in arm_kprobe_ftrace() so the stack
dumps don't pollute dmesg, as IPMODIFY conflicts can occur in normal usage
- Added Masami's ack to the first patch
Changes from v3:
- Have (dis)arm_kprobe_ftrace() return -ENODEV instead of 0 in case of
!CONFIG_KPROBES_ON_FTRACE
- Add total count of all probes tried in (dis)arm_all_kprobes()
Changes from v2:
- Add missing synchronize rcu in register_aggr_kprobe()
- s/kprobes/probes/ on error message in (dis)arm_all_kprobes()
Changes from v1:
- Don't arm the kprobe before adding it to the kprobe table, otherwise
we'll temporarily see a stray breakpoint.
- Remove kprobe from the kprobe_table and call synchronize_sched() if
arming during register_kprobe() fails.
- add Masami's ack on the 2nd patch (unchanged from v1)
---
Jessica Yu (2):
kprobes: propagate error from arm_kprobe_ftrace()
kprobes: propagate error from disarm_kprobe_ftrace()
kernel/kprobes.c | 178 +++++++++++++++++++++++++++++++++++++++----------------
1 file changed, 128 insertions(+), 50 deletions(-)
--
2.13.6
Improve error handling when arming ftrace-based kprobes. Specifically, if
we fail to arm a ftrace-based kprobe, register_kprobe()/enable_kprobe()
should report an error instead of success. Previously, this has lead to
confusing situations where register_kprobe() would return 0 indicating
success, but the kprobe would not be functional if ftrace registration
during the kprobe arming process had failed. We should therefore take any
errors returned by ftrace into account and propagate this error so that we
do not register/enable kprobes that cannot be armed. This can happen if,
for example, register_ftrace_function() finds an IPMODIFY conflict (since
kprobe_ftrace_ops has this flag set) and returns an error. Such a conflict
is possible since livepatches also set the IPMODIFY flag for their ftrace_ops.
arm_all_kprobes() keeps its current behavior and attempts to arm all
kprobes. It returns the last encountered error and gives a warning if
not all probes could be armed.
This patch is based on Petr Mladek's original patchset (patches 2 and 3)
back in 2015, which improved kprobes error handling, found here:
https://lkml.org/lkml/2015/2/26/452
However, further work on this had been paused since then and the patches
were not upstreamed.
Based-on-patches-by: Petr Mladek <[email protected]>
Acked-by: Masami Hiramatsu <[email protected]>
Signed-off-by: Jessica Yu <[email protected]>
---
kernel/kprobes.c | 100 +++++++++++++++++++++++++++++++++++++++++--------------
1 file changed, 75 insertions(+), 25 deletions(-)
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index b4aab48ad258..beda29641ed5 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -988,18 +988,36 @@ static int prepare_kprobe(struct kprobe *p)
}
/* Caller must lock kprobe_mutex */
-static void arm_kprobe_ftrace(struct kprobe *p)
+static int arm_kprobe_ftrace(struct kprobe *p)
{
- int ret;
+ int ret = 0;
ret = ftrace_set_filter_ip(&kprobe_ftrace_ops,
(unsigned long)p->addr, 0, 0);
- WARN(ret < 0, "Failed to arm kprobe-ftrace at %p (%d)\n", p->addr, ret);
- kprobe_ftrace_enabled++;
- if (kprobe_ftrace_enabled == 1) {
+ if (ret) {
+ pr_debug("Failed to arm kprobe-ftrace at %p (%d)\n", p->addr, ret);
+ return ret;
+ }
+
+ if (kprobe_ftrace_enabled == 0) {
ret = register_ftrace_function(&kprobe_ftrace_ops);
- WARN(ret < 0, "Failed to init kprobe-ftrace (%d)\n", ret);
+ if (ret) {
+ pr_debug("Failed to init kprobe-ftrace (%d)\n", ret);
+ goto err_ftrace;
+ }
}
+
+ kprobe_ftrace_enabled++;
+ return ret;
+
+err_ftrace:
+ /*
+ * Note: Since kprobe_ftrace_ops has IPMODIFY set, and ftrace requires a
+ * non-empty filter_hash for IPMODIFY ops, we're safe from an accidental
+ * empty filter_hash which would undesirably trace all functions.
+ */
+ ftrace_set_filter_ip(&kprobe_ftrace_ops, (unsigned long)p->addr, 1, 0);
+ return ret;
}
/* Caller must lock kprobe_mutex */
@@ -1018,22 +1036,23 @@ static void disarm_kprobe_ftrace(struct kprobe *p)
}
#else /* !CONFIG_KPROBES_ON_FTRACE */
#define prepare_kprobe(p) arch_prepare_kprobe(p)
-#define arm_kprobe_ftrace(p) do {} while (0)
+#define arm_kprobe_ftrace(p) (-ENODEV)
#define disarm_kprobe_ftrace(p) do {} while (0)
#endif
/* Arm a kprobe with text_mutex */
-static void arm_kprobe(struct kprobe *kp)
+static int arm_kprobe(struct kprobe *kp)
{
- if (unlikely(kprobe_ftrace(kp))) {
- arm_kprobe_ftrace(kp);
- return;
- }
+ if (unlikely(kprobe_ftrace(kp)))
+ return arm_kprobe_ftrace(kp);
+
cpus_read_lock();
mutex_lock(&text_mutex);
__arm_kprobe(kp);
mutex_unlock(&text_mutex);
cpus_read_unlock();
+
+ return 0;
}
/* Disarm a kprobe with text_mutex */
@@ -1372,9 +1391,15 @@ static int register_aggr_kprobe(struct kprobe *orig_p, struct kprobe *p)
if (ret == 0 && kprobe_disabled(ap) && !kprobe_disabled(p)) {
ap->flags &= ~KPROBE_FLAG_DISABLED;
- if (!kprobes_all_disarmed)
+ if (!kprobes_all_disarmed) {
/* Arm the breakpoint again. */
- arm_kprobe(ap);
+ ret = arm_kprobe(ap);
+ if (ret) {
+ ap->flags |= KPROBE_FLAG_DISABLED;
+ list_del_rcu(&p->list);
+ synchronize_sched();
+ }
+ }
}
return ret;
}
@@ -1594,8 +1619,14 @@ int register_kprobe(struct kprobe *p)
hlist_add_head_rcu(&p->hlist,
&kprobe_table[hash_ptr(p->addr, KPROBE_HASH_BITS)]);
- if (!kprobes_all_disarmed && !kprobe_disabled(p))
- arm_kprobe(p);
+ if (!kprobes_all_disarmed && !kprobe_disabled(p)) {
+ ret = arm_kprobe(p);
+ if (ret) {
+ hlist_del_rcu(&p->hlist);
+ synchronize_sched();
+ goto out;
+ }
+ }
/* Try to optimize kprobe */
try_to_optimize_kprobe(p);
@@ -2137,7 +2168,9 @@ int enable_kprobe(struct kprobe *kp)
if (!kprobes_all_disarmed && kprobe_disabled(p)) {
p->flags &= ~KPROBE_FLAG_DISABLED;
- arm_kprobe(p);
+ ret = arm_kprobe(p);
+ if (ret)
+ p->flags |= KPROBE_FLAG_DISABLED;
}
out:
mutex_unlock(&kprobe_mutex);
@@ -2565,11 +2598,12 @@ static const struct file_operations debugfs_kprobe_ei_ops = {
.release = seq_release,
};
-static void arm_all_kprobes(void)
+static int arm_all_kprobes(void)
{
struct hlist_head *head;
struct kprobe *p;
- unsigned int i;
+ unsigned int i, total = 0, errors = 0;
+ int err, ret = 0;
mutex_lock(&kprobe_mutex);
@@ -2586,16 +2620,28 @@ static void arm_all_kprobes(void)
/* Arming kprobes doesn't optimize kprobe itself */
for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
head = &kprobe_table[i];
- hlist_for_each_entry_rcu(p, head, hlist)
- if (!kprobe_disabled(p))
- arm_kprobe(p);
+ /* Arm all kprobes on a best-effort basis */
+ hlist_for_each_entry_rcu(p, head, hlist) {
+ if (!kprobe_disabled(p)) {
+ err = arm_kprobe(p);
+ if (err) {
+ errors++;
+ ret = err;
+ }
+ total++;
+ }
+ }
}
- printk(KERN_INFO "Kprobes globally enabled\n");
+ if (errors)
+ pr_warn("Kprobes globally enabled, but failed to arm %d out of %d probes\n",
+ errors, total);
+ else
+ pr_info("Kprobes globally enabled\n");
already_enabled:
mutex_unlock(&kprobe_mutex);
- return;
+ return ret;
}
static void disarm_all_kprobes(void)
@@ -2652,6 +2698,7 @@ static ssize_t write_enabled_file_bool(struct file *file,
{
char buf[32];
size_t buf_size;
+ int ret = 0;
buf_size = min(count, (sizeof(buf)-1));
if (copy_from_user(buf, user_buf, buf_size))
@@ -2662,7 +2709,7 @@ static ssize_t write_enabled_file_bool(struct file *file,
case 'y':
case 'Y':
case '1':
- arm_all_kprobes();
+ ret = arm_all_kprobes();
break;
case 'n':
case 'N':
@@ -2673,6 +2720,9 @@ static ssize_t write_enabled_file_bool(struct file *file,
return -EINVAL;
}
+ if (ret)
+ return ret;
+
return count;
}
--
2.13.6
Improve error handling when disarming ftrace-based kprobes. Like with
arm_kprobe_ftrace(), propagate any errors from disarm_kprobe_ftrace() so
that we do not disable/unregister kprobes that are still armed. In other
words, unregister_kprobe() and disable_kprobe() should not report success
if the kprobe could not be disarmed.
disarm_all_kprobes() keeps its current behavior and attempts to
disarm all kprobes. It returns the last encountered error and gives a
warning if not all probes could be disarmed.
This patch is based on Petr Mladek's original patchset (patches 2 and 3)
back in 2015, which improved kprobes error handling, found here:
https://lkml.org/lkml/2015/2/26/452
However, further work on this had been paused since then and the patches
were not upstreamed.
Based-on-patches-by: Petr Mladek <[email protected]>
Acked-by: Masami Hiramatsu <[email protected]>
Signed-off-by: Jessica Yu <[email protected]>
---
kernel/kprobes.c | 78 ++++++++++++++++++++++++++++++++++++++------------------
1 file changed, 53 insertions(+), 25 deletions(-)
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index beda29641ed5..0951d6f0d59d 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1021,23 +1021,27 @@ static int arm_kprobe_ftrace(struct kprobe *p)
}
/* Caller must lock kprobe_mutex */
-static void disarm_kprobe_ftrace(struct kprobe *p)
+static int disarm_kprobe_ftrace(struct kprobe *p)
{
- int ret;
+ int ret = 0;
- kprobe_ftrace_enabled--;
- if (kprobe_ftrace_enabled == 0) {
+ if (kprobe_ftrace_enabled == 1) {
ret = unregister_ftrace_function(&kprobe_ftrace_ops);
- WARN(ret < 0, "Failed to init kprobe-ftrace (%d)\n", ret);
+ if (WARN(ret < 0, "Failed to unregister kprobe-ftrace (%d)\n", ret))
+ return ret;
}
+
+ kprobe_ftrace_enabled--;
+
ret = ftrace_set_filter_ip(&kprobe_ftrace_ops,
(unsigned long)p->addr, 1, 0);
WARN(ret < 0, "Failed to disarm kprobe-ftrace at %p (%d)\n", p->addr, ret);
+ return ret;
}
#else /* !CONFIG_KPROBES_ON_FTRACE */
#define prepare_kprobe(p) arch_prepare_kprobe(p)
#define arm_kprobe_ftrace(p) (-ENODEV)
-#define disarm_kprobe_ftrace(p) do {} while (0)
+#define disarm_kprobe_ftrace(p) (-ENODEV)
#endif
/* Arm a kprobe with text_mutex */
@@ -1056,18 +1060,18 @@ static int arm_kprobe(struct kprobe *kp)
}
/* Disarm a kprobe with text_mutex */
-static void disarm_kprobe(struct kprobe *kp, bool reopt)
+static int disarm_kprobe(struct kprobe *kp, bool reopt)
{
- if (unlikely(kprobe_ftrace(kp))) {
- disarm_kprobe_ftrace(kp);
- return;
- }
+ if (unlikely(kprobe_ftrace(kp)))
+ return disarm_kprobe_ftrace(kp);
cpus_read_lock();
mutex_lock(&text_mutex);
__disarm_kprobe(kp, reopt);
mutex_unlock(&text_mutex);
cpus_read_unlock();
+
+ return 0;
}
/*
@@ -1660,11 +1664,12 @@ static int aggr_kprobe_disabled(struct kprobe *ap)
static struct kprobe *__disable_kprobe(struct kprobe *p)
{
struct kprobe *orig_p;
+ int ret;
/* Get an original kprobe for return */
orig_p = __get_valid_kprobe(p);
if (unlikely(orig_p == NULL))
- return NULL;
+ return ERR_PTR(-EINVAL);
if (!kprobe_disabled(p)) {
/* Disable probe if it is a child probe */
@@ -1678,8 +1683,13 @@ static struct kprobe *__disable_kprobe(struct kprobe *p)
* should have already been disarmed, so
* skip unneed disarming process.
*/
- if (!kprobes_all_disarmed)
- disarm_kprobe(orig_p, true);
+ if (!kprobes_all_disarmed) {
+ ret = disarm_kprobe(orig_p, true);
+ if (ret) {
+ p->flags &= ~KPROBE_FLAG_DISABLED;
+ return ERR_PTR(ret);
+ }
+ }
orig_p->flags |= KPROBE_FLAG_DISABLED;
}
}
@@ -1696,8 +1706,8 @@ static int __unregister_kprobe_top(struct kprobe *p)
/* Disable kprobe. This will disarm it if needed. */
ap = __disable_kprobe(p);
- if (ap == NULL)
- return -EINVAL;
+ if (IS_ERR(ap))
+ return PTR_ERR(ap);
if (ap == p)
/*
@@ -2130,12 +2140,14 @@ static void kill_kprobe(struct kprobe *p)
int disable_kprobe(struct kprobe *kp)
{
int ret = 0;
+ struct kprobe *p;
mutex_lock(&kprobe_mutex);
/* Disable this kprobe */
- if (__disable_kprobe(kp) == NULL)
- ret = -EINVAL;
+ p = __disable_kprobe(kp);
+ if (IS_ERR(p))
+ ret = PTR_ERR(p);
mutex_unlock(&kprobe_mutex);
return ret;
@@ -2644,34 +2656,50 @@ static int arm_all_kprobes(void)
return ret;
}
-static void disarm_all_kprobes(void)
+static int disarm_all_kprobes(void)
{
struct hlist_head *head;
struct kprobe *p;
- unsigned int i;
+ unsigned int i, total = 0, errors = 0;
+ int err, ret = 0;
mutex_lock(&kprobe_mutex);
/* If kprobes are already disarmed, just return */
if (kprobes_all_disarmed) {
mutex_unlock(&kprobe_mutex);
- return;
+ return 0;
}
kprobes_all_disarmed = true;
- printk(KERN_INFO "Kprobes globally disabled\n");
for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
head = &kprobe_table[i];
+ /* Disarm all kprobes on a best-effort basis */
hlist_for_each_entry_rcu(p, head, hlist) {
- if (!arch_trampoline_kprobe(p) && !kprobe_disabled(p))
- disarm_kprobe(p, false);
+ if (!arch_trampoline_kprobe(p) && !kprobe_disabled(p)) {
+ err = disarm_kprobe(p, false);
+ if (err) {
+ errors++;
+ ret = err;
+ }
+ total++;
+ }
}
}
+
+ if (errors)
+ pr_warn("Kprobes globally disabled, but failed to disarm %d out of %d probes\n",
+ errors, total);
+ else
+ pr_info("Kprobes globally disabled\n");
+
mutex_unlock(&kprobe_mutex);
/* Wait for disarming all kprobes by optimizer */
wait_for_kprobe_optimizer();
+
+ return ret;
}
/*
@@ -2714,7 +2742,7 @@ static ssize_t write_enabled_file_bool(struct file *file,
case 'n':
case 'N':
case '0':
- disarm_all_kprobes();
+ ret = disarm_all_kprobes();
break;
default:
return -EINVAL;
--
2.13.6
On Wed, 10 Jan 2018 00:51:22 +0100
Jessica Yu <[email protected]> wrote:
> Hi,
>
> This patchset attempts to improve error handling when arming or disarming
> ftrace-based kprobes. The current behavior is to simply WARN when ftrace
> (un-)registration fails, without propagating the error code. This can lead
> to confusing situations where, for example, register_kprobe()/enable_kprobe()
> would return 0 indicating success even if arming via ftrace had failed. In
> this scenario we'd end up with a non-functioning kprobe even though kprobe
> registration (or enablement) returned success. In this patchset, we take
> errors from ftrace into account and propagate the error when we cannot arm
> or disarm a kprobe.
>
> Below is an example that illustrates the problem using livepatch and
> systemtap (which uses kprobes underneath). Both livepatch and kprobes use
> ftrace ops with the IPMODIFY flag set, so registration at the same
> function entry is limited to only one ftrace user.
This is not only systemtap, but also both bpf and ftrace (and perf-probe)
will face the same problem.
This series is good to me. Thanks!
Thank you,
>
> Before
> ------
> # modprobe livepatch-sample # patches cmdline_proc_show, ftrace ops has IPMODIFY set
> # stap -e 'probe kernel.function("cmdline_proc_show").call { printf ("cmdline_proc_show\n"); }'
>
> .. (nothing prints after reading /proc/cmdline) ..
>
> The systemtap handler doesn't execute due to a kprobe arming failure caused
> by a ftrace IPMODIFY conflict with livepatch, and there isn't an obvious
> indication of error from systemtap (because register_kprobe() returned
> success) unless the user inspects dmesg.
>
> After
> -----
> # modprobe livepatch-sample
> # stap -e 'probe kernel.function("cmdline_proc_show").call { printf ("cmdline_proc_show\n"); }'
> WARNING: probe kernel.function("cmdline_proc_show@/home/jeyu/work/linux-next/fs/proc/cmdline.c:6").call (address 0xffffffffa82fe910) registration error (rc -16)
>
> Although the systemtap handler doesn't execute (as it shouldn't), the
> ftrace error is propagated and now systemtap prints a visible error message
> stating that (kprobe) registration had failed (because register_kprobe()
> returned an error), along with the propagated error code.
>
> This patchset was based on Petr Mladek's original patchset (patches 2 and 3)
> back in 2015, which improved kprobes error handling, found here:
>
> https://lkml.org/lkml/2015/2/26/452
>
> However, further work on this had been paused since then and the patches
> were not upstreamed.
>
> This patchset has been lightly sanity-tested (on linux-next) with kprobes,
> kretprobes, and optimized kprobes. It passes the kprobes smoke test, but
> more testing is greatly appreciated.
>
> Changes from v4:
> - Switch from WARN() to pr_debug() in arm_kprobe_ftrace() so the stack
> dumps don't pollute dmesg, as IPMODIFY conflicts can occur in normal usage
> - Added Masami's ack to the first patch
>
> Changes from v3:
> - Have (dis)arm_kprobe_ftrace() return -ENODEV instead of 0 in case of
> !CONFIG_KPROBES_ON_FTRACE
> - Add total count of all probes tried in (dis)arm_all_kprobes()
>
> Changes from v2:
> - Add missing synchronize rcu in register_aggr_kprobe()
> - s/kprobes/probes/ on error message in (dis)arm_all_kprobes()
>
> Changes from v1:
> - Don't arm the kprobe before adding it to the kprobe table, otherwise
> we'll temporarily see a stray breakpoint.
> - Remove kprobe from the kprobe_table and call synchronize_sched() if
> arming during register_kprobe() fails.
> - add Masami's ack on the 2nd patch (unchanged from v1)
>
> ---
> Jessica Yu (2):
> kprobes: propagate error from arm_kprobe_ftrace()
> kprobes: propagate error from disarm_kprobe_ftrace()
>
> kernel/kprobes.c | 178 +++++++++++++++++++++++++++++++++++++++----------------
> 1 file changed, 128 insertions(+), 50 deletions(-)
>
> --
> 2.13.6
>
--
Masami Hiramatsu <[email protected]>
Hi Ingo,
Could you pick this to tip tree?
Thank you,
On Wed, 10 Jan 2018 00:51:22 +0100
Jessica Yu <[email protected]> wrote:
> Hi,
>
> This patchset attempts to improve error handling when arming or disarming
> ftrace-based kprobes. The current behavior is to simply WARN when ftrace
> (un-)registration fails, without propagating the error code. This can lead
> to confusing situations where, for example, register_kprobe()/enable_kprobe()
> would return 0 indicating success even if arming via ftrace had failed. In
> this scenario we'd end up with a non-functioning kprobe even though kprobe
> registration (or enablement) returned success. In this patchset, we take
> errors from ftrace into account and propagate the error when we cannot arm
> or disarm a kprobe.
>
> Below is an example that illustrates the problem using livepatch and
> systemtap (which uses kprobes underneath). Both livepatch and kprobes use
> ftrace ops with the IPMODIFY flag set, so registration at the same
> function entry is limited to only one ftrace user.
>
> Before
> ------
> # modprobe livepatch-sample # patches cmdline_proc_show, ftrace ops has IPMODIFY set
> # stap -e 'probe kernel.function("cmdline_proc_show").call { printf ("cmdline_proc_show\n"); }'
>
> .. (nothing prints after reading /proc/cmdline) ..
>
> The systemtap handler doesn't execute due to a kprobe arming failure caused
> by a ftrace IPMODIFY conflict with livepatch, and there isn't an obvious
> indication of error from systemtap (because register_kprobe() returned
> success) unless the user inspects dmesg.
>
> After
> -----
> # modprobe livepatch-sample
> # stap -e 'probe kernel.function("cmdline_proc_show").call { printf ("cmdline_proc_show\n"); }'
> WARNING: probe kernel.function("cmdline_proc_show@/home/jeyu/work/linux-next/fs/proc/cmdline.c:6").call (address 0xffffffffa82fe910) registration error (rc -16)
>
> Although the systemtap handler doesn't execute (as it shouldn't), the
> ftrace error is propagated and now systemtap prints a visible error message
> stating that (kprobe) registration had failed (because register_kprobe()
> returned an error), along with the propagated error code.
>
> This patchset was based on Petr Mladek's original patchset (patches 2 and 3)
> back in 2015, which improved kprobes error handling, found here:
>
> https://lkml.org/lkml/2015/2/26/452
>
> However, further work on this had been paused since then and the patches
> were not upstreamed.
>
> This patchset has been lightly sanity-tested (on linux-next) with kprobes,
> kretprobes, and optimized kprobes. It passes the kprobes smoke test, but
> more testing is greatly appreciated.
>
> Changes from v4:
> - Switch from WARN() to pr_debug() in arm_kprobe_ftrace() so the stack
> dumps don't pollute dmesg, as IPMODIFY conflicts can occur in normal usage
> - Added Masami's ack to the first patch
>
> Changes from v3:
> - Have (dis)arm_kprobe_ftrace() return -ENODEV instead of 0 in case of
> !CONFIG_KPROBES_ON_FTRACE
> - Add total count of all probes tried in (dis)arm_all_kprobes()
>
> Changes from v2:
> - Add missing synchronize rcu in register_aggr_kprobe()
> - s/kprobes/probes/ on error message in (dis)arm_all_kprobes()
>
> Changes from v1:
> - Don't arm the kprobe before adding it to the kprobe table, otherwise
> we'll temporarily see a stray breakpoint.
> - Remove kprobe from the kprobe_table and call synchronize_sched() if
> arming during register_kprobe() fails.
> - add Masami's ack on the 2nd patch (unchanged from v1)
>
> ---
> Jessica Yu (2):
> kprobes: propagate error from arm_kprobe_ftrace()
> kprobes: propagate error from disarm_kprobe_ftrace()
>
> kernel/kprobes.c | 178 +++++++++++++++++++++++++++++++++++++++----------------
> 1 file changed, 128 insertions(+), 50 deletions(-)
>
> --
> 2.13.6
>
--
Masami Hiramatsu <[email protected]>
Was this patch ever picked up in the tip tree? (Maybe I'm not looking
in the right branch?)
Thanks,
-- Joe
On 01/19/2018 02:06 AM, Masami Hiramatsu wrote:
> Hi Ingo,
>
> Could you pick this to tip tree?
>
> Thank you,
>
> On Wed, 10 Jan 2018 00:51:22 +0100
> Jessica Yu <[email protected]> wrote:
>
>> Hi,
>>
>> This patchset attempts to improve error handling when arming or disarming
>> ftrace-based kprobes. The current behavior is to simply WARN when ftrace
>> (un-)registration fails, without propagating the error code. This can lead
>> to confusing situations where, for example, register_kprobe()/enable_kprobe()
>> would return 0 indicating success even if arming via ftrace had failed. In
>> this scenario we'd end up with a non-functioning kprobe even though kprobe
>> registration (or enablement) returned success. In this patchset, we take
>> errors from ftrace into account and propagate the error when we cannot arm
>> or disarm a kprobe.
>>
>> Below is an example that illustrates the problem using livepatch and
>> systemtap (which uses kprobes underneath). Both livepatch and kprobes use
>> ftrace ops with the IPMODIFY flag set, so registration at the same
>> function entry is limited to only one ftrace user.
>>
>> Before
>> ------
>> # modprobe livepatch-sample # patches cmdline_proc_show, ftrace ops has IPMODIFY set
>> # stap -e 'probe kernel.function("cmdline_proc_show").call { printf ("cmdline_proc_show\n"); }'
>>
>> .. (nothing prints after reading /proc/cmdline) ..
>>
>> The systemtap handler doesn't execute due to a kprobe arming failure caused
>> by a ftrace IPMODIFY conflict with livepatch, and there isn't an obvious
>> indication of error from systemtap (because register_kprobe() returned
>> success) unless the user inspects dmesg.
>>
>> After
>> -----
>> # modprobe livepatch-sample
>> # stap -e 'probe kernel.function("cmdline_proc_show").call { printf ("cmdline_proc_show\n"); }'
>> WARNING: probe kernel.function("cmdline_proc_show@/home/jeyu/work/linux-next/fs/proc/cmdline.c:6").call (address 0xffffffffa82fe910) registration error (rc -16)
>>
>> Although the systemtap handler doesn't execute (as it shouldn't), the
>> ftrace error is propagated and now systemtap prints a visible error message
>> stating that (kprobe) registration had failed (because register_kprobe()
>> returned an error), along with the propagated error code.
>>
>> This patchset was based on Petr Mladek's original patchset (patches 2 and 3)
>> back in 2015, which improved kprobes error handling, found here:
>>
>> https://lkml.org/lkml/2015/2/26/452
>>
>> However, further work on this had been paused since then and the patches
>> were not upstreamed.
>>
>> This patchset has been lightly sanity-tested (on linux-next) with kprobes,
>> kretprobes, and optimized kprobes. It passes the kprobes smoke test, but
>> more testing is greatly appreciated.
>>
>> Changes from v4:
>> - Switch from WARN() to pr_debug() in arm_kprobe_ftrace() so the stack
>> dumps don't pollute dmesg, as IPMODIFY conflicts can occur in normal usage
>> - Added Masami's ack to the first patch
>>
>> Changes from v3:
>> - Have (dis)arm_kprobe_ftrace() return -ENODEV instead of 0 in case of
>> !CONFIG_KPROBES_ON_FTRACE
>> - Add total count of all probes tried in (dis)arm_all_kprobes()
>>
>> Changes from v2:
>> - Add missing synchronize rcu in register_aggr_kprobe()
>> - s/kprobes/probes/ on error message in (dis)arm_all_kprobes()
>>
>> Changes from v1:
>> - Don't arm the kprobe before adding it to the kprobe table, otherwise
>> we'll temporarily see a stray breakpoint.
>> - Remove kprobe from the kprobe_table and call synchronize_sched() if
>> arming during register_kprobe() fails.
>> - add Masami's ack on the 2nd patch (unchanged from v1)
>>
>> ---
>> Jessica Yu (2):
>> kprobes: propagate error from arm_kprobe_ftrace()
>> kprobes: propagate error from disarm_kprobe_ftrace()
>>
>> kernel/kprobes.c | 178 +++++++++++++++++++++++++++++++++++++++----------------
>> 1 file changed, 128 insertions(+), 50 deletions(-)
>>
>> --
>> 2.13.6
>>
>
>
* Joe Lawrence <[email protected]> wrote:
> Was this patch ever picked up in the tip tree? (Maybe I'm not looking
> in the right branch?)
Sorry, had a bit of a backlog: I picked up the two fixes/improvements now, and
they will show up in tip:perf/urgent once they go through some local testing.
Thanks!
Ingo
Commit-ID: 384f1d86a8890fcf8118add0b6a0785ee35f5a65
Gitweb: https://git.kernel.org/tip/384f1d86a8890fcf8118add0b6a0785ee35f5a65
Author: Jessica Yu <[email protected]>
AuthorDate: Wed, 10 Jan 2018 00:51:23 +0100
Committer: Ingo Molnar <[email protected]>
CommitDate: Tue, 13 Feb 2018 18:20:00 +0100
kprobes: Propagate error from arm_kprobe_ftrace()
Improve error handling when arming ftrace-based kprobes. Specifically, if
we fail to arm a ftrace-based kprobe, register_kprobe()/enable_kprobe()
should report an error instead of success. Previously, this has lead to
confusing situations where register_kprobe() would return 0 indicating
success, but the kprobe would not be functional if ftrace registration
during the kprobe arming process had failed. We should therefore take any
errors returned by ftrace into account and propagate this error so that we
do not register/enable kprobes that cannot be armed. This can happen if,
for example, register_ftrace_function() finds an IPMODIFY conflict (since
kprobe_ftrace_ops has this flag set) and returns an error. Such a conflict
is possible since livepatches also set the IPMODIFY flag for their ftrace_ops.
arm_all_kprobes() keeps its current behavior and attempts to arm all
kprobes. It returns the last encountered error and gives a warning if
not all probes could be armed.
This patch is based on Petr Mladek's original patchset (patches 2 and 3)
back in 2015, which improved kprobes error handling, found here:
https://lkml.org/lkml/2015/2/26/452
However, further work on this had been paused since then and the patches
were not upstreamed.
Based-on-patches-by: Petr Mladek <[email protected]>
Signed-off-by: Jessica Yu <[email protected]>
Acked-by: Masami Hiramatsu <[email protected]>
Cc: Ananth N Mavinakayanahalli <[email protected]>
Cc: Anil S Keshavamurthy <[email protected]>
Cc: David S . Miller <[email protected]>
Cc: Jiri Kosina <[email protected]>
Cc: Joe Lawrence <[email protected]>
Cc: Josh Poimboeuf <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Miroslav Benes <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Petr Mladek <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: [email protected]
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
---
kernel/kprobes.c | 100 +++++++++++++++++++++++++++++++++++++++++--------------
1 file changed, 75 insertions(+), 25 deletions(-)
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index da2ccf1..2d98814 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -978,18 +978,36 @@ static int prepare_kprobe(struct kprobe *p)
}
/* Caller must lock kprobe_mutex */
-static void arm_kprobe_ftrace(struct kprobe *p)
+static int arm_kprobe_ftrace(struct kprobe *p)
{
- int ret;
+ int ret = 0;
ret = ftrace_set_filter_ip(&kprobe_ftrace_ops,
(unsigned long)p->addr, 0, 0);
- WARN(ret < 0, "Failed to arm kprobe-ftrace at %p (%d)\n", p->addr, ret);
- kprobe_ftrace_enabled++;
- if (kprobe_ftrace_enabled == 1) {
+ if (ret) {
+ pr_debug("Failed to arm kprobe-ftrace at %p (%d)\n", p->addr, ret);
+ return ret;
+ }
+
+ if (kprobe_ftrace_enabled == 0) {
ret = register_ftrace_function(&kprobe_ftrace_ops);
- WARN(ret < 0, "Failed to init kprobe-ftrace (%d)\n", ret);
+ if (ret) {
+ pr_debug("Failed to init kprobe-ftrace (%d)\n", ret);
+ goto err_ftrace;
+ }
}
+
+ kprobe_ftrace_enabled++;
+ return ret;
+
+err_ftrace:
+ /*
+ * Note: Since kprobe_ftrace_ops has IPMODIFY set, and ftrace requires a
+ * non-empty filter_hash for IPMODIFY ops, we're safe from an accidental
+ * empty filter_hash which would undesirably trace all functions.
+ */
+ ftrace_set_filter_ip(&kprobe_ftrace_ops, (unsigned long)p->addr, 1, 0);
+ return ret;
}
/* Caller must lock kprobe_mutex */
@@ -1008,22 +1026,23 @@ static void disarm_kprobe_ftrace(struct kprobe *p)
}
#else /* !CONFIG_KPROBES_ON_FTRACE */
#define prepare_kprobe(p) arch_prepare_kprobe(p)
-#define arm_kprobe_ftrace(p) do {} while (0)
+#define arm_kprobe_ftrace(p) (-ENODEV)
#define disarm_kprobe_ftrace(p) do {} while (0)
#endif
/* Arm a kprobe with text_mutex */
-static void arm_kprobe(struct kprobe *kp)
+static int arm_kprobe(struct kprobe *kp)
{
- if (unlikely(kprobe_ftrace(kp))) {
- arm_kprobe_ftrace(kp);
- return;
- }
+ if (unlikely(kprobe_ftrace(kp)))
+ return arm_kprobe_ftrace(kp);
+
cpus_read_lock();
mutex_lock(&text_mutex);
__arm_kprobe(kp);
mutex_unlock(&text_mutex);
cpus_read_unlock();
+
+ return 0;
}
/* Disarm a kprobe with text_mutex */
@@ -1362,9 +1381,15 @@ out:
if (ret == 0 && kprobe_disabled(ap) && !kprobe_disabled(p)) {
ap->flags &= ~KPROBE_FLAG_DISABLED;
- if (!kprobes_all_disarmed)
+ if (!kprobes_all_disarmed) {
/* Arm the breakpoint again. */
- arm_kprobe(ap);
+ ret = arm_kprobe(ap);
+ if (ret) {
+ ap->flags |= KPROBE_FLAG_DISABLED;
+ list_del_rcu(&p->list);
+ synchronize_sched();
+ }
+ }
}
return ret;
}
@@ -1573,8 +1598,14 @@ int register_kprobe(struct kprobe *p)
hlist_add_head_rcu(&p->hlist,
&kprobe_table[hash_ptr(p->addr, KPROBE_HASH_BITS)]);
- if (!kprobes_all_disarmed && !kprobe_disabled(p))
- arm_kprobe(p);
+ if (!kprobes_all_disarmed && !kprobe_disabled(p)) {
+ ret = arm_kprobe(p);
+ if (ret) {
+ hlist_del_rcu(&p->hlist);
+ synchronize_sched();
+ goto out;
+ }
+ }
/* Try to optimize kprobe */
try_to_optimize_kprobe(p);
@@ -2116,7 +2147,9 @@ int enable_kprobe(struct kprobe *kp)
if (!kprobes_all_disarmed && kprobe_disabled(p)) {
p->flags &= ~KPROBE_FLAG_DISABLED;
- arm_kprobe(p);
+ ret = arm_kprobe(p);
+ if (ret)
+ p->flags |= KPROBE_FLAG_DISABLED;
}
out:
mutex_unlock(&kprobe_mutex);
@@ -2407,11 +2440,12 @@ static const struct file_operations debugfs_kprobe_blacklist_ops = {
.release = seq_release,
};
-static void arm_all_kprobes(void)
+static int arm_all_kprobes(void)
{
struct hlist_head *head;
struct kprobe *p;
- unsigned int i;
+ unsigned int i, total = 0, errors = 0;
+ int err, ret = 0;
mutex_lock(&kprobe_mutex);
@@ -2428,16 +2462,28 @@ static void arm_all_kprobes(void)
/* Arming kprobes doesn't optimize kprobe itself */
for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
head = &kprobe_table[i];
- hlist_for_each_entry_rcu(p, head, hlist)
- if (!kprobe_disabled(p))
- arm_kprobe(p);
+ /* Arm all kprobes on a best-effort basis */
+ hlist_for_each_entry_rcu(p, head, hlist) {
+ if (!kprobe_disabled(p)) {
+ err = arm_kprobe(p);
+ if (err) {
+ errors++;
+ ret = err;
+ }
+ total++;
+ }
+ }
}
- printk(KERN_INFO "Kprobes globally enabled\n");
+ if (errors)
+ pr_warn("Kprobes globally enabled, but failed to arm %d out of %d probes\n",
+ errors, total);
+ else
+ pr_info("Kprobes globally enabled\n");
already_enabled:
mutex_unlock(&kprobe_mutex);
- return;
+ return ret;
}
static void disarm_all_kprobes(void)
@@ -2494,6 +2540,7 @@ static ssize_t write_enabled_file_bool(struct file *file,
{
char buf[32];
size_t buf_size;
+ int ret = 0;
buf_size = min(count, (sizeof(buf)-1));
if (copy_from_user(buf, user_buf, buf_size))
@@ -2504,7 +2551,7 @@ static ssize_t write_enabled_file_bool(struct file *file,
case 'y':
case 'Y':
case '1':
- arm_all_kprobes();
+ ret = arm_all_kprobes();
break;
case 'n':
case 'N':
@@ -2515,6 +2562,9 @@ static ssize_t write_enabled_file_bool(struct file *file,
return -EINVAL;
}
+ if (ret)
+ return ret;
+
return count;
}
Commit-ID: cea1e51c8357feb0e98464a82e1ad3ca2d0382a6
Gitweb: https://git.kernel.org/tip/cea1e51c8357feb0e98464a82e1ad3ca2d0382a6
Author: Jessica Yu <[email protected]>
AuthorDate: Wed, 10 Jan 2018 00:51:24 +0100
Committer: Ingo Molnar <[email protected]>
CommitDate: Tue, 13 Feb 2018 18:20:00 +0100
kprobes: Propagate error from disarm_kprobe_ftrace()
Improve error handling when disarming ftrace-based kprobes. Like with
arm_kprobe_ftrace(), propagate any errors from disarm_kprobe_ftrace() so
that we do not disable/unregister kprobes that are still armed. In other
words, unregister_kprobe() and disable_kprobe() should not report success
if the kprobe could not be disarmed.
disarm_all_kprobes() keeps its current behavior and attempts to
disarm all kprobes. It returns the last encountered error and gives a
warning if not all probes could be disarmed.
This patch is based on Petr Mladek's original patchset (patches 2 and 3)
back in 2015, which improved kprobes error handling, found here:
https://lkml.org/lkml/2015/2/26/452
However, further work on this had been paused since then and the patches
were not upstreamed.
Based-on-patches-by: Petr Mladek <[email protected]>
Signed-off-by: Jessica Yu <[email protected]>
Acked-by: Masami Hiramatsu <[email protected]>
Cc: Ananth N Mavinakayanahalli <[email protected]>
Cc: Anil S Keshavamurthy <[email protected]>
Cc: David S . Miller <[email protected]>
Cc: Jiri Kosina <[email protected]>
Cc: Joe Lawrence <[email protected]>
Cc: Josh Poimboeuf <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Miroslav Benes <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Petr Mladek <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: [email protected]
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
---
kernel/kprobes.c | 78 ++++++++++++++++++++++++++++++++++++++------------------
1 file changed, 53 insertions(+), 25 deletions(-)
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 2d98814..102160f 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1011,23 +1011,27 @@ err_ftrace:
}
/* Caller must lock kprobe_mutex */
-static void disarm_kprobe_ftrace(struct kprobe *p)
+static int disarm_kprobe_ftrace(struct kprobe *p)
{
- int ret;
+ int ret = 0;
- kprobe_ftrace_enabled--;
- if (kprobe_ftrace_enabled == 0) {
+ if (kprobe_ftrace_enabled == 1) {
ret = unregister_ftrace_function(&kprobe_ftrace_ops);
- WARN(ret < 0, "Failed to init kprobe-ftrace (%d)\n", ret);
+ if (WARN(ret < 0, "Failed to unregister kprobe-ftrace (%d)\n", ret))
+ return ret;
}
+
+ kprobe_ftrace_enabled--;
+
ret = ftrace_set_filter_ip(&kprobe_ftrace_ops,
(unsigned long)p->addr, 1, 0);
WARN(ret < 0, "Failed to disarm kprobe-ftrace at %p (%d)\n", p->addr, ret);
+ return ret;
}
#else /* !CONFIG_KPROBES_ON_FTRACE */
#define prepare_kprobe(p) arch_prepare_kprobe(p)
#define arm_kprobe_ftrace(p) (-ENODEV)
-#define disarm_kprobe_ftrace(p) do {} while (0)
+#define disarm_kprobe_ftrace(p) (-ENODEV)
#endif
/* Arm a kprobe with text_mutex */
@@ -1046,18 +1050,18 @@ static int arm_kprobe(struct kprobe *kp)
}
/* Disarm a kprobe with text_mutex */
-static void disarm_kprobe(struct kprobe *kp, bool reopt)
+static int disarm_kprobe(struct kprobe *kp, bool reopt)
{
- if (unlikely(kprobe_ftrace(kp))) {
- disarm_kprobe_ftrace(kp);
- return;
- }
+ if (unlikely(kprobe_ftrace(kp)))
+ return disarm_kprobe_ftrace(kp);
cpus_read_lock();
mutex_lock(&text_mutex);
__disarm_kprobe(kp, reopt);
mutex_unlock(&text_mutex);
cpus_read_unlock();
+
+ return 0;
}
/*
@@ -1639,11 +1643,12 @@ static int aggr_kprobe_disabled(struct kprobe *ap)
static struct kprobe *__disable_kprobe(struct kprobe *p)
{
struct kprobe *orig_p;
+ int ret;
/* Get an original kprobe for return */
orig_p = __get_valid_kprobe(p);
if (unlikely(orig_p == NULL))
- return NULL;
+ return ERR_PTR(-EINVAL);
if (!kprobe_disabled(p)) {
/* Disable probe if it is a child probe */
@@ -1657,8 +1662,13 @@ static struct kprobe *__disable_kprobe(struct kprobe *p)
* should have already been disarmed, so
* skip unneed disarming process.
*/
- if (!kprobes_all_disarmed)
- disarm_kprobe(orig_p, true);
+ if (!kprobes_all_disarmed) {
+ ret = disarm_kprobe(orig_p, true);
+ if (ret) {
+ p->flags &= ~KPROBE_FLAG_DISABLED;
+ return ERR_PTR(ret);
+ }
+ }
orig_p->flags |= KPROBE_FLAG_DISABLED;
}
}
@@ -1675,8 +1685,8 @@ static int __unregister_kprobe_top(struct kprobe *p)
/* Disable kprobe. This will disarm it if needed. */
ap = __disable_kprobe(p);
- if (ap == NULL)
- return -EINVAL;
+ if (IS_ERR(ap))
+ return PTR_ERR(ap);
if (ap == p)
/*
@@ -2109,12 +2119,14 @@ static void kill_kprobe(struct kprobe *p)
int disable_kprobe(struct kprobe *kp)
{
int ret = 0;
+ struct kprobe *p;
mutex_lock(&kprobe_mutex);
/* Disable this kprobe */
- if (__disable_kprobe(kp) == NULL)
- ret = -EINVAL;
+ p = __disable_kprobe(kp);
+ if (IS_ERR(p))
+ ret = PTR_ERR(p);
mutex_unlock(&kprobe_mutex);
return ret;
@@ -2486,34 +2498,50 @@ already_enabled:
return ret;
}
-static void disarm_all_kprobes(void)
+static int disarm_all_kprobes(void)
{
struct hlist_head *head;
struct kprobe *p;
- unsigned int i;
+ unsigned int i, total = 0, errors = 0;
+ int err, ret = 0;
mutex_lock(&kprobe_mutex);
/* If kprobes are already disarmed, just return */
if (kprobes_all_disarmed) {
mutex_unlock(&kprobe_mutex);
- return;
+ return 0;
}
kprobes_all_disarmed = true;
- printk(KERN_INFO "Kprobes globally disabled\n");
for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
head = &kprobe_table[i];
+ /* Disarm all kprobes on a best-effort basis */
hlist_for_each_entry_rcu(p, head, hlist) {
- if (!arch_trampoline_kprobe(p) && !kprobe_disabled(p))
- disarm_kprobe(p, false);
+ if (!arch_trampoline_kprobe(p) && !kprobe_disabled(p)) {
+ err = disarm_kprobe(p, false);
+ if (err) {
+ errors++;
+ ret = err;
+ }
+ total++;
+ }
}
}
+
+ if (errors)
+ pr_warn("Kprobes globally disabled, but failed to disarm %d out of %d probes\n",
+ errors, total);
+ else
+ pr_info("Kprobes globally disabled\n");
+
mutex_unlock(&kprobe_mutex);
/* Wait for disarming all kprobes by optimizer */
wait_for_kprobe_optimizer();
+
+ return ret;
}
/*
@@ -2556,7 +2584,7 @@ static ssize_t write_enabled_file_bool(struct file *file,
case 'n':
case 'N':
case '0':
- disarm_all_kprobes();
+ ret = disarm_all_kprobes();
break;
default:
return -EINVAL;
On Tue, 13 Feb 2018 18:21:19 +0100
Ingo Molnar <[email protected]> wrote:
>
> * Joe Lawrence <[email protected]> wrote:
>
> > Was this patch ever picked up in the tip tree? (Maybe I'm not looking
> > in the right branch?)
>
> Sorry, had a bit of a backlog: I picked up the two fixes/improvements now, and
> they will show up in tip:perf/urgent once they go through some local testing.
>
> Thanks!
Thanks Ingo!
--
Masami Hiramatsu <[email protected]>
Commit-ID: 12310e3437554328bcd75186cf331bc712cb30b2
Gitweb: https://git.kernel.org/tip/12310e3437554328bcd75186cf331bc712cb30b2
Author: Jessica Yu <[email protected]>
AuthorDate: Wed, 10 Jan 2018 00:51:23 +0100
Committer: Ingo Molnar <[email protected]>
CommitDate: Fri, 16 Feb 2018 09:12:52 +0100
kprobes: Propagate error from arm_kprobe_ftrace()
Improve error handling when arming ftrace-based kprobes. Specifically, if
we fail to arm a ftrace-based kprobe, register_kprobe()/enable_kprobe()
should report an error instead of success. Previously, this has lead to
confusing situations where register_kprobe() would return 0 indicating
success, but the kprobe would not be functional if ftrace registration
during the kprobe arming process had failed. We should therefore take any
errors returned by ftrace into account and propagate this error so that we
do not register/enable kprobes that cannot be armed. This can happen if,
for example, register_ftrace_function() finds an IPMODIFY conflict (since
kprobe_ftrace_ops has this flag set) and returns an error. Such a conflict
is possible since livepatches also set the IPMODIFY flag for their ftrace_ops.
arm_all_kprobes() keeps its current behavior and attempts to arm all
kprobes. It returns the last encountered error and gives a warning if
not all probes could be armed.
This patch is based on Petr Mladek's original patchset (patches 2 and 3)
back in 2015, which improved kprobes error handling, found here:
https://lkml.org/lkml/2015/2/26/452
However, further work on this had been paused since then and the patches
were not upstreamed.
Based-on-patches-by: Petr Mladek <[email protected]>
Signed-off-by: Jessica Yu <[email protected]>
Acked-by: Masami Hiramatsu <[email protected]>
Cc: Ananth N Mavinakayanahalli <[email protected]>
Cc: Anil S Keshavamurthy <[email protected]>
Cc: David S . Miller <[email protected]>
Cc: Jiri Kosina <[email protected]>
Cc: Joe Lawrence <[email protected]>
Cc: Josh Poimboeuf <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Miroslav Benes <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Petr Mladek <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: [email protected]
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
---
kernel/kprobes.c | 100 +++++++++++++++++++++++++++++++++++++++++--------------
1 file changed, 75 insertions(+), 25 deletions(-)
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index da2ccf1..2d98814 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -978,18 +978,36 @@ static int prepare_kprobe(struct kprobe *p)
}
/* Caller must lock kprobe_mutex */
-static void arm_kprobe_ftrace(struct kprobe *p)
+static int arm_kprobe_ftrace(struct kprobe *p)
{
- int ret;
+ int ret = 0;
ret = ftrace_set_filter_ip(&kprobe_ftrace_ops,
(unsigned long)p->addr, 0, 0);
- WARN(ret < 0, "Failed to arm kprobe-ftrace at %p (%d)\n", p->addr, ret);
- kprobe_ftrace_enabled++;
- if (kprobe_ftrace_enabled == 1) {
+ if (ret) {
+ pr_debug("Failed to arm kprobe-ftrace at %p (%d)\n", p->addr, ret);
+ return ret;
+ }
+
+ if (kprobe_ftrace_enabled == 0) {
ret = register_ftrace_function(&kprobe_ftrace_ops);
- WARN(ret < 0, "Failed to init kprobe-ftrace (%d)\n", ret);
+ if (ret) {
+ pr_debug("Failed to init kprobe-ftrace (%d)\n", ret);
+ goto err_ftrace;
+ }
}
+
+ kprobe_ftrace_enabled++;
+ return ret;
+
+err_ftrace:
+ /*
+ * Note: Since kprobe_ftrace_ops has IPMODIFY set, and ftrace requires a
+ * non-empty filter_hash for IPMODIFY ops, we're safe from an accidental
+ * empty filter_hash which would undesirably trace all functions.
+ */
+ ftrace_set_filter_ip(&kprobe_ftrace_ops, (unsigned long)p->addr, 1, 0);
+ return ret;
}
/* Caller must lock kprobe_mutex */
@@ -1008,22 +1026,23 @@ static void disarm_kprobe_ftrace(struct kprobe *p)
}
#else /* !CONFIG_KPROBES_ON_FTRACE */
#define prepare_kprobe(p) arch_prepare_kprobe(p)
-#define arm_kprobe_ftrace(p) do {} while (0)
+#define arm_kprobe_ftrace(p) (-ENODEV)
#define disarm_kprobe_ftrace(p) do {} while (0)
#endif
/* Arm a kprobe with text_mutex */
-static void arm_kprobe(struct kprobe *kp)
+static int arm_kprobe(struct kprobe *kp)
{
- if (unlikely(kprobe_ftrace(kp))) {
- arm_kprobe_ftrace(kp);
- return;
- }
+ if (unlikely(kprobe_ftrace(kp)))
+ return arm_kprobe_ftrace(kp);
+
cpus_read_lock();
mutex_lock(&text_mutex);
__arm_kprobe(kp);
mutex_unlock(&text_mutex);
cpus_read_unlock();
+
+ return 0;
}
/* Disarm a kprobe with text_mutex */
@@ -1362,9 +1381,15 @@ out:
if (ret == 0 && kprobe_disabled(ap) && !kprobe_disabled(p)) {
ap->flags &= ~KPROBE_FLAG_DISABLED;
- if (!kprobes_all_disarmed)
+ if (!kprobes_all_disarmed) {
/* Arm the breakpoint again. */
- arm_kprobe(ap);
+ ret = arm_kprobe(ap);
+ if (ret) {
+ ap->flags |= KPROBE_FLAG_DISABLED;
+ list_del_rcu(&p->list);
+ synchronize_sched();
+ }
+ }
}
return ret;
}
@@ -1573,8 +1598,14 @@ int register_kprobe(struct kprobe *p)
hlist_add_head_rcu(&p->hlist,
&kprobe_table[hash_ptr(p->addr, KPROBE_HASH_BITS)]);
- if (!kprobes_all_disarmed && !kprobe_disabled(p))
- arm_kprobe(p);
+ if (!kprobes_all_disarmed && !kprobe_disabled(p)) {
+ ret = arm_kprobe(p);
+ if (ret) {
+ hlist_del_rcu(&p->hlist);
+ synchronize_sched();
+ goto out;
+ }
+ }
/* Try to optimize kprobe */
try_to_optimize_kprobe(p);
@@ -2116,7 +2147,9 @@ int enable_kprobe(struct kprobe *kp)
if (!kprobes_all_disarmed && kprobe_disabled(p)) {
p->flags &= ~KPROBE_FLAG_DISABLED;
- arm_kprobe(p);
+ ret = arm_kprobe(p);
+ if (ret)
+ p->flags |= KPROBE_FLAG_DISABLED;
}
out:
mutex_unlock(&kprobe_mutex);
@@ -2407,11 +2440,12 @@ static const struct file_operations debugfs_kprobe_blacklist_ops = {
.release = seq_release,
};
-static void arm_all_kprobes(void)
+static int arm_all_kprobes(void)
{
struct hlist_head *head;
struct kprobe *p;
- unsigned int i;
+ unsigned int i, total = 0, errors = 0;
+ int err, ret = 0;
mutex_lock(&kprobe_mutex);
@@ -2428,16 +2462,28 @@ static void arm_all_kprobes(void)
/* Arming kprobes doesn't optimize kprobe itself */
for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
head = &kprobe_table[i];
- hlist_for_each_entry_rcu(p, head, hlist)
- if (!kprobe_disabled(p))
- arm_kprobe(p);
+ /* Arm all kprobes on a best-effort basis */
+ hlist_for_each_entry_rcu(p, head, hlist) {
+ if (!kprobe_disabled(p)) {
+ err = arm_kprobe(p);
+ if (err) {
+ errors++;
+ ret = err;
+ }
+ total++;
+ }
+ }
}
- printk(KERN_INFO "Kprobes globally enabled\n");
+ if (errors)
+ pr_warn("Kprobes globally enabled, but failed to arm %d out of %d probes\n",
+ errors, total);
+ else
+ pr_info("Kprobes globally enabled\n");
already_enabled:
mutex_unlock(&kprobe_mutex);
- return;
+ return ret;
}
static void disarm_all_kprobes(void)
@@ -2494,6 +2540,7 @@ static ssize_t write_enabled_file_bool(struct file *file,
{
char buf[32];
size_t buf_size;
+ int ret = 0;
buf_size = min(count, (sizeof(buf)-1));
if (copy_from_user(buf, user_buf, buf_size))
@@ -2504,7 +2551,7 @@ static ssize_t write_enabled_file_bool(struct file *file,
case 'y':
case 'Y':
case '1':
- arm_all_kprobes();
+ ret = arm_all_kprobes();
break;
case 'n':
case 'N':
@@ -2515,6 +2562,9 @@ static ssize_t write_enabled_file_bool(struct file *file,
return -EINVAL;
}
+ if (ret)
+ return ret;
+
return count;
}
Commit-ID: 297f9233b53a08fd457815e19f1d6f2c3389857b
Gitweb: https://git.kernel.org/tip/297f9233b53a08fd457815e19f1d6f2c3389857b
Author: Jessica Yu <[email protected]>
AuthorDate: Wed, 10 Jan 2018 00:51:24 +0100
Committer: Ingo Molnar <[email protected]>
CommitDate: Fri, 16 Feb 2018 09:12:58 +0100
kprobes: Propagate error from disarm_kprobe_ftrace()
Improve error handling when disarming ftrace-based kprobes. Like with
arm_kprobe_ftrace(), propagate any errors from disarm_kprobe_ftrace() so
that we do not disable/unregister kprobes that are still armed. In other
words, unregister_kprobe() and disable_kprobe() should not report success
if the kprobe could not be disarmed.
disarm_all_kprobes() keeps its current behavior and attempts to
disarm all kprobes. It returns the last encountered error and gives a
warning if not all probes could be disarmed.
This patch is based on Petr Mladek's original patchset (patches 2 and 3)
back in 2015, which improved kprobes error handling, found here:
https://lkml.org/lkml/2015/2/26/452
However, further work on this had been paused since then and the patches
were not upstreamed.
Based-on-patches-by: Petr Mladek <[email protected]>
Signed-off-by: Jessica Yu <[email protected]>
Acked-by: Masami Hiramatsu <[email protected]>
Cc: Ananth N Mavinakayanahalli <[email protected]>
Cc: Anil S Keshavamurthy <[email protected]>
Cc: David S . Miller <[email protected]>
Cc: Jiri Kosina <[email protected]>
Cc: Joe Lawrence <[email protected]>
Cc: Josh Poimboeuf <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Miroslav Benes <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Petr Mladek <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: [email protected]
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
---
kernel/kprobes.c | 78 ++++++++++++++++++++++++++++++++++++++------------------
1 file changed, 53 insertions(+), 25 deletions(-)
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 2d98814..102160f 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1011,23 +1011,27 @@ err_ftrace:
}
/* Caller must lock kprobe_mutex */
-static void disarm_kprobe_ftrace(struct kprobe *p)
+static int disarm_kprobe_ftrace(struct kprobe *p)
{
- int ret;
+ int ret = 0;
- kprobe_ftrace_enabled--;
- if (kprobe_ftrace_enabled == 0) {
+ if (kprobe_ftrace_enabled == 1) {
ret = unregister_ftrace_function(&kprobe_ftrace_ops);
- WARN(ret < 0, "Failed to init kprobe-ftrace (%d)\n", ret);
+ if (WARN(ret < 0, "Failed to unregister kprobe-ftrace (%d)\n", ret))
+ return ret;
}
+
+ kprobe_ftrace_enabled--;
+
ret = ftrace_set_filter_ip(&kprobe_ftrace_ops,
(unsigned long)p->addr, 1, 0);
WARN(ret < 0, "Failed to disarm kprobe-ftrace at %p (%d)\n", p->addr, ret);
+ return ret;
}
#else /* !CONFIG_KPROBES_ON_FTRACE */
#define prepare_kprobe(p) arch_prepare_kprobe(p)
#define arm_kprobe_ftrace(p) (-ENODEV)
-#define disarm_kprobe_ftrace(p) do {} while (0)
+#define disarm_kprobe_ftrace(p) (-ENODEV)
#endif
/* Arm a kprobe with text_mutex */
@@ -1046,18 +1050,18 @@ static int arm_kprobe(struct kprobe *kp)
}
/* Disarm a kprobe with text_mutex */
-static void disarm_kprobe(struct kprobe *kp, bool reopt)
+static int disarm_kprobe(struct kprobe *kp, bool reopt)
{
- if (unlikely(kprobe_ftrace(kp))) {
- disarm_kprobe_ftrace(kp);
- return;
- }
+ if (unlikely(kprobe_ftrace(kp)))
+ return disarm_kprobe_ftrace(kp);
cpus_read_lock();
mutex_lock(&text_mutex);
__disarm_kprobe(kp, reopt);
mutex_unlock(&text_mutex);
cpus_read_unlock();
+
+ return 0;
}
/*
@@ -1639,11 +1643,12 @@ static int aggr_kprobe_disabled(struct kprobe *ap)
static struct kprobe *__disable_kprobe(struct kprobe *p)
{
struct kprobe *orig_p;
+ int ret;
/* Get an original kprobe for return */
orig_p = __get_valid_kprobe(p);
if (unlikely(orig_p == NULL))
- return NULL;
+ return ERR_PTR(-EINVAL);
if (!kprobe_disabled(p)) {
/* Disable probe if it is a child probe */
@@ -1657,8 +1662,13 @@ static struct kprobe *__disable_kprobe(struct kprobe *p)
* should have already been disarmed, so
* skip unneed disarming process.
*/
- if (!kprobes_all_disarmed)
- disarm_kprobe(orig_p, true);
+ if (!kprobes_all_disarmed) {
+ ret = disarm_kprobe(orig_p, true);
+ if (ret) {
+ p->flags &= ~KPROBE_FLAG_DISABLED;
+ return ERR_PTR(ret);
+ }
+ }
orig_p->flags |= KPROBE_FLAG_DISABLED;
}
}
@@ -1675,8 +1685,8 @@ static int __unregister_kprobe_top(struct kprobe *p)
/* Disable kprobe. This will disarm it if needed. */
ap = __disable_kprobe(p);
- if (ap == NULL)
- return -EINVAL;
+ if (IS_ERR(ap))
+ return PTR_ERR(ap);
if (ap == p)
/*
@@ -2109,12 +2119,14 @@ static void kill_kprobe(struct kprobe *p)
int disable_kprobe(struct kprobe *kp)
{
int ret = 0;
+ struct kprobe *p;
mutex_lock(&kprobe_mutex);
/* Disable this kprobe */
- if (__disable_kprobe(kp) == NULL)
- ret = -EINVAL;
+ p = __disable_kprobe(kp);
+ if (IS_ERR(p))
+ ret = PTR_ERR(p);
mutex_unlock(&kprobe_mutex);
return ret;
@@ -2486,34 +2498,50 @@ already_enabled:
return ret;
}
-static void disarm_all_kprobes(void)
+static int disarm_all_kprobes(void)
{
struct hlist_head *head;
struct kprobe *p;
- unsigned int i;
+ unsigned int i, total = 0, errors = 0;
+ int err, ret = 0;
mutex_lock(&kprobe_mutex);
/* If kprobes are already disarmed, just return */
if (kprobes_all_disarmed) {
mutex_unlock(&kprobe_mutex);
- return;
+ return 0;
}
kprobes_all_disarmed = true;
- printk(KERN_INFO "Kprobes globally disabled\n");
for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
head = &kprobe_table[i];
+ /* Disarm all kprobes on a best-effort basis */
hlist_for_each_entry_rcu(p, head, hlist) {
- if (!arch_trampoline_kprobe(p) && !kprobe_disabled(p))
- disarm_kprobe(p, false);
+ if (!arch_trampoline_kprobe(p) && !kprobe_disabled(p)) {
+ err = disarm_kprobe(p, false);
+ if (err) {
+ errors++;
+ ret = err;
+ }
+ total++;
+ }
}
}
+
+ if (errors)
+ pr_warn("Kprobes globally disabled, but failed to disarm %d out of %d probes\n",
+ errors, total);
+ else
+ pr_info("Kprobes globally disabled\n");
+
mutex_unlock(&kprobe_mutex);
/* Wait for disarming all kprobes by optimizer */
wait_for_kprobe_optimizer();
+
+ return ret;
}
/*
@@ -2556,7 +2584,7 @@ static ssize_t write_enabled_file_bool(struct file *file,
case 'n':
case 'N':
case '0':
- disarm_all_kprobes();
+ ret = disarm_all_kprobes();
break;
default:
return -EINVAL;