| Message ID | 20220718001405.2236811-2-song@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | ftrace: host klp and bpf trampoline together | expand |
On Sun 2022-07-17 17:14:02, Song Liu wrote: > This is similar to modify_ftrace_direct_multi, but does not acquire > direct_mutex. This is useful when direct_mutex is already locked by the > user. > > --- a/kernel/trace/ftrace.c > +++ b/kernel/trace/ftrace.c > @@ -5691,22 +5691,8 @@ int unregister_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr) > @@ -5717,12 +5703,8 @@ int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr) > int i, size; > int err; > > - if (check_direct_multi(ops)) > + if (WARN_ON_ONCE(!mutex_is_locked(&direct_mutex))) > return -EINVAL; IMHO, it is better to use: lockdep_assert_held_once(&direct_mutex); It will always catch the problem when called without the lock and lockdep is enabled. > - if (!(ops->flags & FTRACE_OPS_FL_ENABLED)) > - return -EINVAL; > - > - mutex_lock(&direct_mutex); > > /* Enable the tmp_ops to have the same functions as the direct ops */ > ftrace_ops_init(&tmp_ops); > @@ -5730,7 +5712,7 @@ int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr) > > err = register_ftrace_function(&tmp_ops); > if (err) > - goto out_direct; > + return err; > > /* > * Now the ftrace_ops_list_func() is called to do the direct callers. > @@ -5754,7 +5736,64 @@ int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr) > /* Removing the tmp_ops will add the updated direct callers to the functions */ > unregister_ftrace_function(&tmp_ops); > > - out_direct: > + return err; > +} > + > +/** > + * modify_ftrace_direct_multi_nolock - Modify an existing direct 'multi' call > + * to call something else > + * @ops: The address of the struct ftrace_ops object > + * @addr: The address of the new trampoline to call at @ops functions > + * > + * This is used to unregister currently registered direct caller and > + * register new one @addr on functions registered in @ops object. > + * > + * Note there's window between ftrace_shutdown and ftrace_startup calls > + * where there will be no callbacks called. > + * > + * Caller should already have direct_mutex locked, so we don't lock > + * direct_mutex here. > + * > + * Returns: zero on success. Non zero on error, which includes: > + * -EINVAL - The @ops object was not properly registered. > + */ > +int modify_ftrace_direct_multi_nolock(struct ftrace_ops *ops, unsigned long addr) > +{ > + if (check_direct_multi(ops)) > + return -EINVAL; > + if (!(ops->flags & FTRACE_OPS_FL_ENABLED)) > + return -EINVAL; > + > + return __modify_ftrace_direct_multi(ops, addr); > +} > +EXPORT_SYMBOL_GPL(modify_ftrace_direct_multi_nolock); > + > +/** > + * modify_ftrace_direct_multi - Modify an existing direct 'multi' call > + * to call something else > + * @ops: The address of the struct ftrace_ops object > + * @addr: The address of the new trampoline to call at @ops functions > + * > + * This is used to unregister currently registered direct caller and > + * register new one @addr on functions registered in @ops object. > + * > + * Note there's window between ftrace_shutdown and ftrace_startup calls > + * where there will be no callbacks called. > + * > + * Returns: zero on success. Non zero on error, which includes: > + * -EINVAL - The @ops object was not properly registered. > + */ > +int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr) > +{ > + int err; > + > + if (check_direct_multi(ops)) > + return -EINVAL; > + if (!(ops->flags & FTRACE_OPS_FL_ENABLED)) > + return -EINVAL; > + > + mutex_lock(&direct_mutex); > + err = __modify_ftrace_direct_multi(ops, addr); > mutex_unlock(&direct_mutex); > return err; > } I would personally do: int __modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr, bool lock) { int err; if (check_direct_multi(ops)) return -EINVAL; if (!(ops->flags & FTRACE_OPS_FL_ENABLED)) return -EINVAL; if (lock) mutex_lock(&direct_mutex); err = __modify_ftrace_direct_multi(ops, addr); if (lock) mutex_unlock(&direct_mutex); return err; } int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr) { __modify_ftrace_direct_multi(ops, addr, true); } int modify_ftrace_direct_multi_nolock(struct ftrace_ops *ops, unsigned long addr) { __modify_ftrace_direct_multi(ops, addr, false); } To avoid duplication of the checks. But it is a matter of taste. Best Regards, Petr
Hi Petr, Thanks for your quick review! > On Jul 18, 2022, at 5:50 AM, Petr Mladek <pmladek@suse.com> wrote: > > On Sun 2022-07-17 17:14:02, Song Liu wrote: >> This is similar to modify_ftrace_direct_multi, but does not acquire >> direct_mutex. This is useful when direct_mutex is already locked by the >> user. >> >> --- a/kernel/trace/ftrace.c >> +++ b/kernel/trace/ftrace.c >> @@ -5691,22 +5691,8 @@ int unregister_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr) >> @@ -5717,12 +5703,8 @@ int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr) >> int i, size; >> int err; >> >> - if (check_direct_multi(ops)) >> + if (WARN_ON_ONCE(!mutex_is_locked(&direct_mutex))) >> return -EINVAL; > > IMHO, it is better to use: > > lockdep_assert_held_once(&direct_mutex); > > It will always catch the problem when called without the lock and > lockdep is enabled. Will fix. > >> - if (!(ops->flags & FTRACE_OPS_FL_ENABLED)) >> - return -EINVAL; >> - >> - mutex_lock(&direct_mutex); >> >> /* Enable the tmp_ops to have the same functions as the direct ops */ >> ftrace_ops_init(&tmp_ops); >> @@ -5730,7 +5712,7 @@ int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr) >> >> err = register_ftrace_function(&tmp_ops); >> if (err) >> - goto out_direct; >> + return err; >> >> /* >> * Now the ftrace_ops_list_func() is called to do the direct callers. >> @@ -5754,7 +5736,64 @@ int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr) >> /* Removing the tmp_ops will add the updated direct callers to the functions */ >> unregister_ftrace_function(&tmp_ops); >> >> - out_direct: >> + return err; >> +} >> + >> +/** >> + * modify_ftrace_direct_multi_nolock - Modify an existing direct 'multi' call >> + * to call something else >> + * @ops: The address of the struct ftrace_ops object >> + * @addr: The address of the new trampoline to call at @ops functions >> + * >> + * This is used to unregister currently registered direct caller and >> + * register new one @addr on functions registered in @ops object. >> + * >> + * Note there's window between ftrace_shutdown and ftrace_startup calls >> + * where there will be no callbacks called. >> + * >> + * Caller should already have direct_mutex locked, so we don't lock >> + * direct_mutex here. >> + * >> + * Returns: zero on success. Non zero on error, which includes: >> + * -EINVAL - The @ops object was not properly registered. >> + */ >> +int modify_ftrace_direct_multi_nolock(struct ftrace_ops *ops, unsigned long addr) >> +{ >> + if (check_direct_multi(ops)) >> + return -EINVAL; >> + if (!(ops->flags & FTRACE_OPS_FL_ENABLED)) >> + return -EINVAL; >> + >> + return __modify_ftrace_direct_multi(ops, addr); >> +} >> +EXPORT_SYMBOL_GPL(modify_ftrace_direct_multi_nolock); >> + >> +/** >> + * modify_ftrace_direct_multi - Modify an existing direct 'multi' call >> + * to call something else >> + * @ops: The address of the struct ftrace_ops object >> + * @addr: The address of the new trampoline to call at @ops functions >> + * >> + * This is used to unregister currently registered direct caller and >> + * register new one @addr on functions registered in @ops object. >> + * >> + * Note there's window between ftrace_shutdown and ftrace_startup calls >> + * where there will be no callbacks called. >> + * >> + * Returns: zero on success. Non zero on error, which includes: >> + * -EINVAL - The @ops object was not properly registered. >> + */ >> +int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr) >> +{ >> + int err; >> + >> + if (check_direct_multi(ops)) >> + return -EINVAL; >> + if (!(ops->flags & FTRACE_OPS_FL_ENABLED)) >> + return -EINVAL; >> + >> + mutex_lock(&direct_mutex); >> + err = __modify_ftrace_direct_multi(ops, addr); >> mutex_unlock(&direct_mutex); >> return err; >> } > > I would personally do: > > int __modify_ftrace_direct_multi(struct ftrace_ops *ops, > unsigned long addr, bool lock) > { > int err; > > if (check_direct_multi(ops)) > return -EINVAL; > if (!(ops->flags & FTRACE_OPS_FL_ENABLED)) > return -EINVAL; > > if (lock) > mutex_lock(&direct_mutex); > > err = __modify_ftrace_direct_multi(ops, addr); > > if (lock) > mutex_unlock(&direct_mutex); The "if (lock) lock" pattern bothers me a little. But I agrees this is a matter of taste. If other folks prefers this way, I will make the change. Thanks, Song > > return err; > } > > int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr) > { > __modify_ftrace_direct_multi(ops, addr, true); > } > > int modify_ftrace_direct_multi_nolock(struct ftrace_ops *ops, unsigned long addr) > { > __modify_ftrace_direct_multi(ops, addr, false); > } > > To avoid duplication of the checks. But it is a matter of taste. > > Best Regards, > Petr
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h index 979f6bfa2c25..acb35243ce5d 100644 --- a/include/linux/ftrace.h +++ b/include/linux/ftrace.h @@ -340,6 +340,7 @@ unsigned long ftrace_find_rec_direct(unsigned long ip); int register_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr); int unregister_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr); int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr); +int modify_ftrace_direct_multi_nolock(struct ftrace_ops *ops, unsigned long addr); #else struct ftrace_ops; @@ -384,6 +385,10 @@ static inline int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned lo { return -ENODEV; } +static inline int modify_ftrace_direct_multi_nolock(struct ftrace_ops *ops, unsigned long addr) +{ + return -ENODEV; +} #endif /* CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS */ #ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c index 601ccf1b2f09..0c15ec997c13 100644 --- a/kernel/trace/ftrace.c +++ b/kernel/trace/ftrace.c @@ -5691,22 +5691,8 @@ int unregister_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr) } EXPORT_SYMBOL_GPL(unregister_ftrace_direct_multi); -/** - * modify_ftrace_direct_multi - Modify an existing direct 'multi' call - * to call something else - * @ops: The address of the struct ftrace_ops object - * @addr: The address of the new trampoline to call at @ops functions - * - * This is used to unregister currently registered direct caller and - * register new one @addr on functions registered in @ops object. - * - * Note there's window between ftrace_shutdown and ftrace_startup calls - * where there will be no callbacks called. - * - * Returns: zero on success. Non zero on error, which includes: - * -EINVAL - The @ops object was not properly registered. - */ -int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr) +static int +__modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr) { struct ftrace_hash *hash; struct ftrace_func_entry *entry, *iter; @@ -5717,12 +5703,8 @@ int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr) int i, size; int err; - if (check_direct_multi(ops)) + if (WARN_ON_ONCE(!mutex_is_locked(&direct_mutex))) return -EINVAL; - if (!(ops->flags & FTRACE_OPS_FL_ENABLED)) - return -EINVAL; - - mutex_lock(&direct_mutex); /* Enable the tmp_ops to have the same functions as the direct ops */ ftrace_ops_init(&tmp_ops); @@ -5730,7 +5712,7 @@ int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr) err = register_ftrace_function(&tmp_ops); if (err) - goto out_direct; + return err; /* * Now the ftrace_ops_list_func() is called to do the direct callers. @@ -5754,7 +5736,64 @@ int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr) /* Removing the tmp_ops will add the updated direct callers to the functions */ unregister_ftrace_function(&tmp_ops); - out_direct: + return err; +} + +/** + * modify_ftrace_direct_multi_nolock - Modify an existing direct 'multi' call + * to call something else + * @ops: The address of the struct ftrace_ops object + * @addr: The address of the new trampoline to call at @ops functions + * + * This is used to unregister currently registered direct caller and + * register new one @addr on functions registered in @ops object. + * + * Note there's window between ftrace_shutdown and ftrace_startup calls + * where there will be no callbacks called. + * + * Caller should already have direct_mutex locked, so we don't lock + * direct_mutex here. + * + * Returns: zero on success. Non zero on error, which includes: + * -EINVAL - The @ops object was not properly registered. + */ +int modify_ftrace_direct_multi_nolock(struct ftrace_ops *ops, unsigned long addr) +{ + if (check_direct_multi(ops)) + return -EINVAL; + if (!(ops->flags & FTRACE_OPS_FL_ENABLED)) + return -EINVAL; + + return __modify_ftrace_direct_multi(ops, addr); +} +EXPORT_SYMBOL_GPL(modify_ftrace_direct_multi_nolock); + +/** + * modify_ftrace_direct_multi - Modify an existing direct 'multi' call + * to call something else + * @ops: The address of the struct ftrace_ops object + * @addr: The address of the new trampoline to call at @ops functions + * + * This is used to unregister currently registered direct caller and + * register new one @addr on functions registered in @ops object. + * + * Note there's window between ftrace_shutdown and ftrace_startup calls + * where there will be no callbacks called. + * + * Returns: zero on success. Non zero on error, which includes: + * -EINVAL - The @ops object was not properly registered. + */ +int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr) +{ + int err; + + if (check_direct_multi(ops)) + return -EINVAL; + if (!(ops->flags & FTRACE_OPS_FL_ENABLED)) + return -EINVAL; + + mutex_lock(&direct_mutex); + err = __modify_ftrace_direct_multi(ops, addr); mutex_unlock(&direct_mutex); return err; }
This is similar to modify_ftrace_direct_multi, but does not acquire direct_mutex. This is useful when direct_mutex is already locked by the user. Signed-off-by: Song Liu <song@kernel.org> --- include/linux/ftrace.h | 5 +++ kernel/trace/ftrace.c | 85 ++++++++++++++++++++++++++++++------------ 2 files changed, 67 insertions(+), 23 deletions(-)