ftrace_startup does not remove ops from ftrace_ops_list when
ftrace_startup_enable fails:
register_ftrace_function
ftrace_startup
__register_ftrace_function
...
add_ftrace_ops(&ftrace_ops_list, ops)
...
...
ftrace_startup_enable // if ftrace failed to modify, ftrace_disabled is set to 1
...
return 0 // ops is in the ftrace_ops_list.
When ftrace_disabled = 1, unregister_ftrace_function simply returns without doing anything:
unregister_ftrace_function
ftrace_shutdown
if (unlikely(ftrace_disabled))
return -ENODEV; // return here, __unregister_ftrace_function is not executed,
// as a result, ops is still in the ftrace_ops_list
__unregister_ftrace_function
...
If ops is dynamically allocated, it will be free later, in this case,
is_ftrace_trampoline accesses NULL pointer:
is_ftrace_trampoline
ftrace_ops_trampoline
do_for_each_ftrace_op(op, ftrace_ops_list) // OOPS! op may be NULL!
Syzkaller reports as follows:
[ 1203.506103] BUG: kernel NULL pointer dereference, address: 000000000000010b
[ 1203.508039] #PF: supervisor read access in kernel mode
[ 1203.508798] #PF: error_code(0x0000) - not-present page
[ 1203.509558] PGD 800000011660b067 P4D 800000011660b067 PUD 130fb8067 PMD 0
[ 1203.510560] Oops: 0000 [#1] SMP KASAN PTI
[ 1203.511189] CPU: 6 PID: 29532 Comm: syz-executor.2 Tainted: G B W 5.10.0 #8
[ 1203.512324] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.14.0-0-g155821a1990b-prebuilt.qemu.org 04/01/2014
[ 1203.513895] RIP: 0010:is_ftrace_trampoline+0x26/0xb0
[ 1203.514644] Code: ff eb d3 90 41 55 41 54 49 89 fc 55 53 e8 f2 00 fd ff 48 8b 1d 3b 35 5d 03 e8 e6 00 fd ff 48 8d bb 90 00 00 00 e8 2a 81 26 00 <48> 8b ab 90 00 00 00 48 85 ed 74 1d e8 c9 00 fd ff 48 8d bb 98 00
[ 1203.518838] RSP: 0018:ffffc900012cf960 EFLAGS: 00010246
[ 1203.520092] RAX: 0000000000000000 RBX: 000000000000007b RCX: ffffffff8a331866
[ 1203.521469] RDX: 0000000000000000 RSI: 0000000000000008 RDI: 000000000000010b
[ 1203.522583] RBP: 0000000000000000 R08: 0000000000000000 R09: ffffffff8df18b07
[ 1203.523550] R10: fffffbfff1be3160 R11: 0000000000000001 R12: 0000000000478399
[ 1203.524596] R13: 0000000000000000 R14: ffff888145088000 R15: 0000000000000008
[ 1203.525634] FS: 00007f429f5f4700(0000) GS:ffff8881daf00000(0000) knlGS:0000000000000000
[ 1203.526801] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 1203.527626] CR2: 000000000000010b CR3: 0000000170e1e001 CR4: 00000000003706e0
[ 1203.528611] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 1203.529605] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Therefore, when ftrace_startup_enable fails, we need to rollback registration
process and remove ops from ftrace_ops_list.
Signed-off-by: Yang Jihong <[email protected]>
---
kernel/trace/ftrace.c | 30 +++++++++++++++++++++---------
1 file changed, 21 insertions(+), 9 deletions(-)
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 601ccf1b2f09..63bf7b67ab2e 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -2922,24 +2922,36 @@ int ftrace_startup(struct ftrace_ops *ops, int command)
ops->flags |= FTRACE_OPS_FL_ENABLED | FTRACE_OPS_FL_ADDING;
ret = ftrace_hash_ipmodify_enable(ops);
- if (ret < 0) {
- /* Rollback registration process */
- __unregister_ftrace_function(ops);
- ftrace_start_up--;
- ops->flags &= ~FTRACE_OPS_FL_ENABLED;
- if (ops->flags & FTRACE_OPS_FL_DYNAMIC)
- ftrace_trampoline_free(ops);
- return ret;
- }
+ if (ret < 0)
+ goto out_rollback_registration;
if (ftrace_hash_rec_enable(ops, 1))
command |= FTRACE_UPDATE_CALLS;
ftrace_startup_enable(command);
+ /*
+ * If ftrace_startup_enable fails,
+ * we need to rollback registration process.
+ */
+ if (unlikely(ftrace_disabled)) {
+ ret = -ENODEV;
+ goto out_rollback_registration;
+ }
+
ops->flags &= ~FTRACE_OPS_FL_ADDING;
return 0;
+
+out_rollback_registration:
+ /* Rollback registration process */
+ __unregister_ftrace_function(ops);
+ ftrace_start_up--;
+ ops->flags &= ~FTRACE_OPS_FL_ENABLED;
+ if (ops->flags & FTRACE_OPS_FL_DYNAMIC)
+ ftrace_trampoline_free(ops);
+
+ return ret;
}
int ftrace_shutdown(struct ftrace_ops *ops, int command)
--
2.30.GIT
Hello,
PING.
Regards,
Yang
.
On 2022/8/4 10:16, Yang Jihong wrote:
> ftrace_startup does not remove ops from ftrace_ops_list when
> ftrace_startup_enable fails:
>
> register_ftrace_function
> ftrace_startup
> __register_ftrace_function
> ...
> add_ftrace_ops(&ftrace_ops_list, ops)
> ...
> ...
> ftrace_startup_enable // if ftrace failed to modify, ftrace_disabled is set to 1
> ...
> return 0 // ops is in the ftrace_ops_list.
>
> When ftrace_disabled = 1, unregister_ftrace_function simply returns without doing anything:
> unregister_ftrace_function
> ftrace_shutdown
> if (unlikely(ftrace_disabled))
> return -ENODEV; // return here, __unregister_ftrace_function is not executed,
> // as a result, ops is still in the ftrace_ops_list
> __unregister_ftrace_function
> ...
>
> If ops is dynamically allocated, it will be free later, in this case,
> is_ftrace_trampoline accesses NULL pointer:
>
> is_ftrace_trampoline
> ftrace_ops_trampoline
> do_for_each_ftrace_op(op, ftrace_ops_list) // OOPS! op may be NULL!
>
> Syzkaller reports as follows:
> [ 1203.506103] BUG: kernel NULL pointer dereference, address: 000000000000010b
> [ 1203.508039] #PF: supervisor read access in kernel mode
> [ 1203.508798] #PF: error_code(0x0000) - not-present page
> [ 1203.509558] PGD 800000011660b067 P4D 800000011660b067 PUD 130fb8067 PMD 0
> [ 1203.510560] Oops: 0000 [#1] SMP KASAN PTI
> [ 1203.511189] CPU: 6 PID: 29532 Comm: syz-executor.2 Tainted: G B W 5.10.0 #8
> [ 1203.512324] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.14.0-0-g155821a1990b-prebuilt.qemu.org 04/01/2014
> [ 1203.513895] RIP: 0010:is_ftrace_trampoline+0x26/0xb0
> [ 1203.514644] Code: ff eb d3 90 41 55 41 54 49 89 fc 55 53 e8 f2 00 fd ff 48 8b 1d 3b 35 5d 03 e8 e6 00 fd ff 48 8d bb 90 00 00 00 e8 2a 81 26 00 <48> 8b ab 90 00 00 00 48 85 ed 74 1d e8 c9 00 fd ff 48 8d bb 98 00
> [ 1203.518838] RSP: 0018:ffffc900012cf960 EFLAGS: 00010246
> [ 1203.520092] RAX: 0000000000000000 RBX: 000000000000007b RCX: ffffffff8a331866
> [ 1203.521469] RDX: 0000000000000000 RSI: 0000000000000008 RDI: 000000000000010b
> [ 1203.522583] RBP: 0000000000000000 R08: 0000000000000000 R09: ffffffff8df18b07
> [ 1203.523550] R10: fffffbfff1be3160 R11: 0000000000000001 R12: 0000000000478399
> [ 1203.524596] R13: 0000000000000000 R14: ffff888145088000 R15: 0000000000000008
> [ 1203.525634] FS: 00007f429f5f4700(0000) GS:ffff8881daf00000(0000) knlGS:0000000000000000
> [ 1203.526801] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 1203.527626] CR2: 000000000000010b CR3: 0000000170e1e001 CR4: 00000000003706e0
> [ 1203.528611] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [ 1203.529605] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>
> Therefore, when ftrace_startup_enable fails, we need to rollback registration
> process and remove ops from ftrace_ops_list.
>
> Signed-off-by: Yang Jihong <[email protected]>
> ---
> kernel/trace/ftrace.c | 30 +++++++++++++++++++++---------
> 1 file changed, 21 insertions(+), 9 deletions(-)
>
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 601ccf1b2f09..63bf7b67ab2e 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -2922,24 +2922,36 @@ int ftrace_startup(struct ftrace_ops *ops, int command)
> ops->flags |= FTRACE_OPS_FL_ENABLED | FTRACE_OPS_FL_ADDING;
>
> ret = ftrace_hash_ipmodify_enable(ops);
> - if (ret < 0) {
> - /* Rollback registration process */
> - __unregister_ftrace_function(ops);
> - ftrace_start_up--;
> - ops->flags &= ~FTRACE_OPS_FL_ENABLED;
> - if (ops->flags & FTRACE_OPS_FL_DYNAMIC)
> - ftrace_trampoline_free(ops);
> - return ret;
> - }
> + if (ret < 0)
> + goto out_rollback_registration;
>
> if (ftrace_hash_rec_enable(ops, 1))
> command |= FTRACE_UPDATE_CALLS;
>
> ftrace_startup_enable(command);
>
> + /*
> + * If ftrace_startup_enable fails,
> + * we need to rollback registration process.
> + */
> + if (unlikely(ftrace_disabled)) {
> + ret = -ENODEV;
> + goto out_rollback_registration;
> + }
> +
> ops->flags &= ~FTRACE_OPS_FL_ADDING;
>
> return 0;
> +
> +out_rollback_registration:
> + /* Rollback registration process */
> + __unregister_ftrace_function(ops);
> + ftrace_start_up--;
> + ops->flags &= ~FTRACE_OPS_FL_ENABLED;
> + if (ops->flags & FTRACE_OPS_FL_DYNAMIC)
> + ftrace_trampoline_free(ops);
> +
> + return ret;
> }
>
> int ftrace_shutdown(struct ftrace_ops *ops, int command)
>
On Wed, 17 Aug 2022 16:19:53 +0800
Yang Jihong <[email protected]> wrote:
> Hello,
>
> PING.
Hmm, this patch got set to "Superseded" in my local Patchwork :-/
Thanks for the ping, I'll take a look at it now.
-- Steve
On Thu, 4 Aug 2022 10:16:10 +0800
Yang Jihong <[email protected]> wrote:
> @@ -2922,24 +2922,36 @@ int ftrace_startup(struct ftrace_ops *ops, int command)
> ops->flags |= FTRACE_OPS_FL_ENABLED | FTRACE_OPS_FL_ADDING;
>
> ret = ftrace_hash_ipmodify_enable(ops);
> - if (ret < 0) {
> - /* Rollback registration process */
> - __unregister_ftrace_function(ops);
> - ftrace_start_up--;
> - ops->flags &= ~FTRACE_OPS_FL_ENABLED;
> - if (ops->flags & FTRACE_OPS_FL_DYNAMIC)
> - ftrace_trampoline_free(ops);
> - return ret;
This should stay as is.
> - }
> + if (ret < 0)
> + goto out_rollback_registration;
>
> if (ftrace_hash_rec_enable(ops, 1))
> command |= FTRACE_UPDATE_CALLS;
>
> ftrace_startup_enable(command);
>
> + /*
> + * If ftrace_startup_enable fails,
> + * we need to rollback registration process.
> + */
> + if (unlikely(ftrace_disabled)) {
> + ret = -ENODEV;
> + goto out_rollback_registration;
The only thing to do here is the _unregister_ftrace_function(ops);
And that may not even be safe.
> + }
> +
> ops->flags &= ~FTRACE_OPS_FL_ADDING;
>
> return 0;
> +
> +out_rollback_registration:
> + /* Rollback registration process */
> + __unregister_ftrace_function(ops);
> + ftrace_start_up--;
> + ops->flags &= ~FTRACE_OPS_FL_ENABLED;
> + if (ops->flags & FTRACE_OPS_FL_DYNAMIC)
> + ftrace_trampoline_free(ops);
> +
When ftrace_disabled is set, ftrace is in an undefined state, and a reboot
should be done ASAP. Because we have no idea what went wrong. It means
something happened that ftrace was not designed for.
That means, we do not know if the trampoline can still be called or not.
Maybe it enabled some of the functions, but not all. And maybe those
functions call the dynamic trampoline directly.
Thus, on ftrace_disable being set, only do the bare minimum, as ftrace has
now "shutdown" and will not do any more work.
Basically, this patch is trying to mitigate a kernel that broke and needs
a reboot immediately.
-- Steve
> + return ret;
> }
>
> int ftrace_shutdown(struct ftrace_ops *ops, int command)
> --
Hello,
On 2022/8/17 22:41, Steven Rostedt wrote:
> On Thu, 4 Aug 2022 10:16:10 +0800
> Yang Jihong <[email protected]> wrote:
>
>> @@ -2922,24 +2922,36 @@ int ftrace_startup(struct ftrace_ops *ops, int command)
>> ops->flags |= FTRACE_OPS_FL_ENABLED | FTRACE_OPS_FL_ADDING;
>>
>> ret = ftrace_hash_ipmodify_enable(ops);
>> - if (ret < 0) {
>> - /* Rollback registration process */
>> - __unregister_ftrace_function(ops);
>> - ftrace_start_up--;
>> - ops->flags &= ~FTRACE_OPS_FL_ENABLED;
>> - if (ops->flags & FTRACE_OPS_FL_DYNAMIC)
>> - ftrace_trampoline_free(ops);
>> - return ret;
>
> This should stay as is.
>
>> - }
>> + if (ret < 0)
>> + goto out_rollback_registration;
>>
>> if (ftrace_hash_rec_enable(ops, 1))
>> command |= FTRACE_UPDATE_CALLS;
>>
>> ftrace_startup_enable(command);
>>
>> + /*
>> + * If ftrace_startup_enable fails,
>> + * we need to rollback registration process.
>> + */
>> + if (unlikely(ftrace_disabled)) {
>> + ret = -ENODEV;
>> + goto out_rollback_registration;
>
> The only thing to do here is the _unregister_ftrace_function(ops);
> And that may not even be safe.
>
>
>> + }
>> +
>> ops->flags &= ~FTRACE_OPS_FL_ADDING;
>>
>> return 0;
>> +
>> +out_rollback_registration:
>> + /* Rollback registration process */
>> + __unregister_ftrace_function(ops);
>> + ftrace_start_up--;
>> + ops->flags &= ~FTRACE_OPS_FL_ENABLED;
>> + if (ops->flags & FTRACE_OPS_FL_DYNAMIC)
>> + ftrace_trampoline_free(ops);
>> +
>
> When ftrace_disabled is set, ftrace is in an undefined state, and a reboot
> should be done ASAP. Because we have no idea what went wrong. It means
> something happened that ftrace was not designed for.
>
> That means, we do not know if the trampoline can still be called or not.
> Maybe it enabled some of the functions, but not all. And maybe those
> functions call the dynamic trampoline directly.
>
> Thus, on ftrace_disable being set, only do the bare minimum, as ftrace has
> now "shutdown" and will not do any more work.
>
> Basically, this patch is trying to mitigate a kernel that broke and needs
> a reboot immediately.
>
> -- Steve
Thanks for the detailed explanation.
If panic_on_warn is not set, FTRACE_WARN_ON{_ONCE} only sets
ftrace_disabled, but will not reboot.
I think this is to limit the problem to ftrace itself and not spread to
other subsystems(I don't know if that's right. If it's not right, please
correct it).
Because is_ftrace_trampoline is a common and public interface (This
interface is called in many places in the kernel).
If is_ftrace_trampoline interface is not restricted (for example, just
return true if ftrace_disabled is set), the preceding Syzkaller scenario
may be triggered when this interface is called.
Therefore, my idea is to restrict the is_ftrace_trampoline or roll back
_unregister_ftrace_function when ftrace_disabled is set, so that the
interface can be invoked normally. Or keep the current code and do not
modify.
Please give me some suggestions that you think are better.
Thanks,
Yang
>
>
>> + return ret;
>> }
>>
>> int ftrace_shutdown(struct ftrace_ops *ops, int command)
>> --
> .
>
Hello,
On 2022/8/18 10:14, Steven Rostedt wrote:
> On Thu, 18 Aug 2022 09:50:40 +0800
> Yang Jihong <[email protected]> wrote:
>
>> Thanks for the detailed explanation.
>> If panic_on_warn is not set, FTRACE_WARN_ON{_ONCE} only sets
>> ftrace_disabled, but will not reboot.
>
> Correct. But whenever there's a WARN_ON() the administrator of the machine
> should think about rebooting it ASAP. That's because all WARN_ON()s are
> suppose to only happen when the system does something that was not
> expected, putting it into an inconsistent state. And could be a dangerous
> one. This is why all WARN_ON()s that are triggered are considered bugs and
> must be fixed.
>
>
>> I think this is to limit the problem to ftrace itself and not spread to
>> other subsystems(I don't know if that's right. If it's not right, please
>> correct it).
>
> Yes, the ftrace_disable means that ftrace just found itself in a situation
> that it does not understand, and nothing can be trusted. As ftrace modifies
> kernel code, it basically stops everything and WARNs about it. Because
> anything else it does can make things worse.
>
>> Because is_ftrace_trampoline is a common and public interface (This
>> interface is called in many places in the kernel).
>> If is_ftrace_trampoline interface is not restricted (for example, just
>> return true if ftrace_disabled is set), the preceding Syzkaller scenario
>> may be triggered when this interface is called.
>
> If ftrace_disabled is set, then any operations should fail, and any tests
> should fail with it.
>
>>
>> Therefore, my idea is to restrict the is_ftrace_trampoline or roll back
>> _unregister_ftrace_function when ftrace_disabled is set, so that the
>> interface can be invoked normally. Or keep the current code and do not
>> modify.
>
> Once ftrace_disabled is set, none of its interfaces should perform
> normally.
>
> But you reported that you could hit a NULL pointer from the
> is_ftrace_trampoline() which was caused by the failure adding the dynamic
> trampoline, and then the ops is on the list but later freed.
>
> My suggestion above is to just call _unregister_ftrace_function(ops) to
> take it off the list and prevent the NULL pointer.
>
> Doesn't that fix the bug?
>
> I don't want to totally roll it back and free the trampoline, because those
> actions could cause further damage, depending on the failed state ftrace is
> in.
OK, I understand, and will be modified in this way in next version.
Thanks,
Yang
On Thu, 18 Aug 2022 09:50:40 +0800
Yang Jihong <[email protected]> wrote:
> Thanks for the detailed explanation.
> If panic_on_warn is not set, FTRACE_WARN_ON{_ONCE} only sets
> ftrace_disabled, but will not reboot.
Correct. But whenever there's a WARN_ON() the administrator of the machine
should think about rebooting it ASAP. That's because all WARN_ON()s are
suppose to only happen when the system does something that was not
expected, putting it into an inconsistent state. And could be a dangerous
one. This is why all WARN_ON()s that are triggered are considered bugs and
must be fixed.
> I think this is to limit the problem to ftrace itself and not spread to
> other subsystems(I don't know if that's right. If it's not right, please
> correct it).
Yes, the ftrace_disable means that ftrace just found itself in a situation
that it does not understand, and nothing can be trusted. As ftrace modifies
kernel code, it basically stops everything and WARNs about it. Because
anything else it does can make things worse.
> Because is_ftrace_trampoline is a common and public interface (This
> interface is called in many places in the kernel).
> If is_ftrace_trampoline interface is not restricted (for example, just
> return true if ftrace_disabled is set), the preceding Syzkaller scenario
> may be triggered when this interface is called.
If ftrace_disabled is set, then any operations should fail, and any tests
should fail with it.
>
> Therefore, my idea is to restrict the is_ftrace_trampoline or roll back
> _unregister_ftrace_function when ftrace_disabled is set, so that the
> interface can be invoked normally. Or keep the current code and do not
> modify.
Once ftrace_disabled is set, none of its interfaces should perform
normally.
But you reported that you could hit a NULL pointer from the
is_ftrace_trampoline() which was caused by the failure adding the dynamic
trampoline, and then the ops is on the list but later freed.
My suggestion above is to just call _unregister_ftrace_function(ops) to
take it off the list and prevent the NULL pointer.
Doesn't that fix the bug?
I don't want to totally roll it back and free the trampoline, because those
actions could cause further damage, depending on the failed state ftrace is
in.
-- Steve