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:   Wed, 17 Aug 2022 10:41:15 -0400
From:   Steven Rostedt <rostedt@goodmis.org>
To:     Yang Jihong <yangjihong1@huawei.com>
Cc:     <mingo@redhat.com>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] ftrace: Fix NULL pointer dereference in
 is_ftrace_trampoline when ftrace is dead
Message-ID: <20220817104115.0ec6b90b@gandalf.local.home>
In-Reply-To: <20220804021610.209791-1-yangjihong1@huawei.com>
References: <20220804021610.209791-1-yangjihong1@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Thu, 4 Aug 2022 10:16:10 +0800
Yang Jihong <yangjihong1@huawei.com> 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)
> --