Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 18 Jul 2022 14:50:36 +0200
From:   Petr Mladek <pmladek@suse.com>
To:     Song Liu <song@kernel.org>
Cc:     bpf@vger.kernel.org, linux-kernel@vger.kernel.org,
        live-patching@vger.kernel.org, daniel@iogearbox.net,
        kernel-team@fb.com, jolsa@kernel.org, rostedt@goodmis.org
Subject: Re: [PATCH v3 bpf-next 1/4] ftrace: add
 modify_ftrace_direct_multi_nolock
Message-ID: <YtVXHDfV8HDwAm6G@alley>
References: <20220718001405.2236811-1-song@kernel.org>
 <20220718001405.2236811-2-song@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220718001405.2236811-2-song@kernel.org>
Precedence: bulk

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