Messages in this thread | | | Subject | Re: [PATCH] ftrace: Fix use-after-free for dynamic ftrace_ops | From | Li Huafei <> | Date | Thu, 3 Nov 2022 10:06:33 +0800 |
| |
On 2022/11/3 6:53, Steven Rostedt wrote: > On Tue, 1 Nov 2022 14:41:46 +0800 > Li Huafei <lihuafei1@huawei.com> wrote: > > >> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c >> index fbf2543111c0..4219cc2a04a6 100644 >> --- a/kernel/trace/ftrace.c >> +++ b/kernel/trace/ftrace.c >> @@ -3030,13 +3030,16 @@ int ftrace_shutdown(struct ftrace_ops *ops, int command) >> >> if (!command || !ftrace_enabled) { > > ftrace_enabled is seldom not set. I don't think we even need to check > it. It's just the value of /proc/sys/kernel/ftrace_enabled, where most > people don't even know that file exists. I do want to get rid of it one > day too. So let's not optimize for it. >
Agree. The code will look much simpler this way.
>> /* >> - * If these are dynamic or per_cpu ops, they still >> - * need their data freed. Since, function tracing is >> - * not currently active, we can just free them >> - * without synchronizing all CPUs. >> + * If these are dynamic, they still need their data freed. If >> + * function tracing is currently active, we neet to synchronize >> + * all CPUs before we can release them. >> */ >> - if (ops->flags & FTRACE_OPS_FL_DYNAMIC) >> + if (ops->flags & FTRACE_OPS_FL_DYNAMIC) { >> + if (ftrace_enabled) >> + goto sync_rcu; >> + >> goto free_ops; > > Change the above just to "goto out;" > >> + } >> >> return 0; >> } >> @@ -3083,6 +3086,7 @@ int ftrace_shutdown(struct ftrace_ops *ops, int command) >> * ops. >> */ > > Add here: > > out: > >> if (ops->flags & FTRACE_OPS_FL_DYNAMIC) { >> + sync_rcu: >> /* >> * We need to do a hard force of sched synchronization. >> * This is because we use preempt_disable() to do RCU, but > > And get rid of the labels in the if block. >
Will make the above changes in v2, thanks!
> Thanks! > > -- Steve > > . >
| |