Re: [PATCH v3 bpf-next 1/4] ftrace: add modify_ftrace_direct_multi_nolock

[Song Liu]

Hi Petr, 

Thanks for your quick review!

> On Jul 18, 2022, at 5:50 AM, Petr Mladek <pmladek@xxxxxxxx> 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