Re: [PATCH v8 02/21] tracing/remotes: Release tracefs,eventfs on registration failure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] tracing/remotes: NULL deref in trace_remote_init_eventfs()
  under lockdown
- [Low] tracing/remotes: missing Fixes trailers for
  trace_remote_register() leaks
--

commit 02468907e3da178a9b587aedf4931ee736d0fcda
Author: Vincent Donnefort <vdonnefort@google.com>

> tracing/remotes: Release tracefs,eventfs on registration failure
>
> In trace_remote_register(), if registration of events or the init
> callback fails, the created tracefs and eventfs directories are leaked.
>
> Release the entire eventfs and tracefs hierarchy on trace_remote
> registration failure.
>
> Signed-off-by: Vincent Donnefort <vdonnefort@google.com>

[Severity: Low]
This fixes resource leaks on the trace_remote_register() error paths.
Should it include Fixes: tags for the commits that introduced them:

  Fixes: 072529158e60 ("tracing: Add events to trace remotes")
  Fixes: bf2ba0f8ca1a ("tracing: Add init callback to trace remotes")

> diff --git a/kernel/trace/trace_remote.c b/kernel/trace/trace_remote.c
> index e8cbbd01964df..10091ff9d42ab 100644
> --- a/kernel/trace/trace_remote.c
> +++ b/kernel/trace/trace_remote.c

[ ... ]

> @@ -1374,35 +1395,37 @@ static int trace_remote_init_eventfs(const char *remote_name, struct trace_remot
>  			.callback	= remote_event_callback,
>  		}
>  	};
> -	bool eventfs_create = false;
> +	struct eventfs_inode *eventfs_root, *eventfs_subdir, *e;
>  
> -	if (!eventfs) {
> -		eventfs = eventfs_create_events_dir("events", remote->dentry, dir_entries,
> -						    ARRAY_SIZE(dir_entries), remote);
> -		if (IS_ERR(eventfs))
> -			return PTR_ERR(eventfs);
> +	eventfs_root = remote->eventfs_root;
> +	eventfs_subdir = remote->eventfs_subdir;
> +	if (!eventfs_root) {
> +		eventfs_root = eventfs_create_events_dir("events", remote->dentry, dir_entries,
> +							 ARRAY_SIZE(dir_entries), remote);
> +		if (IS_ERR(eventfs_root))
> +			return PTR_ERR(eventfs_root);
>  
>  		/*
>  		 * Create similar hierarchy as local events even if a single system is supported at
>  		 * the moment
>  		 */
> -		eventfs = eventfs_create_dir(remote_name, eventfs, NULL, 0, NULL);
> -		if (IS_ERR(eventfs))
> -			return PTR_ERR(eventfs);
> -
> -		remote->eventfs = eventfs;
> -		eventfs_create = true;
> +		eventfs_subdir = eventfs_create_dir(remote_name, eventfs_root, NULL, 0, NULL);
> +		if (IS_ERR(eventfs_subdir)) {
> +			eventfs_remove_events_dir(eventfs_root);
> +			return PTR_ERR(eventfs_subdir);
> +		}
>  	}

[Severity: Medium]
Can this dereference a NULL eventfs_root in eventfs_remove_events_dir() if
kernel lockdown becomes active between trace_remote_init_tracefs() and
trace_remote_init_eventfs()?

When security_locked_down(LOCKDOWN_TRACEFS) is true,
eventfs_create_events_dir() returns NULL rather than an ERR_PTR.
IS_ERR(eventfs_root) does not catch NULL, so eventfs_create_dir() is called
with a NULL parent and returns ERR_PTR(-EINVAL).  The new cleanup path then
passes eventfs_root == NULL to eventfs_remove_events_dir(), which
unconditionally dereferences it in get_root_inode().

Checking IS_ERR_OR_NULL(eventfs_root) or !eventfs_root after
eventfs_create_events_dir() would avoid this regression.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106180657.2460376-1-vdonnefort@google.com?part=2