Re: [PATCH v19 05/40] dept: add proc knobs to show stats and dependency graph

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Lockless traversal of `dept_classes` and `fc->dep_head` in procfs read operations causes kernel panic due to LIST_POISON pointer dereference and Use-After-Free.
--

commit e2e753e7050a1aa0365bc001321fe4e79158b5b4
Author: Byungchul Park <[email protected]>

dept: add proc knobs to show stats and dependency graph

This commit introduces procfs knobs to expose internal stats and the dependency
graph for the dependency tracker at runtime.

> diff --git a/kernel/dependency/dept_proc.c b/kernel/dependency/dept_proc.c
> new file mode 100644
> index 0000000000000..97beaf397715f
> --- /dev/null
> +++ b/kernel/dependency/dept_proc.c

[ ... ]

> +static void *l_next(struct seq_file *m, void *v, loff_t *pos)
> +{
> +	/*
> +	 * XXX: Serialize list traversal if needed. The following might
> +	 * give a wrong information on contention.
> +	 */
> +	return seq_list_next(v, &dept_classes, pos);
> +}
> +
> +static void *l_start(struct seq_file *m, loff_t *pos)
> +{
> +	/*
> +	 * XXX: Serialize list traversal if needed. The following might
> +	 * give a wrong information on contention.
> +	 */
> +	return seq_list_start_head(&dept_classes, *pos);
> +}

[Severity: High]
Is it safe to traverse dept_classes without locks here?

If a concurrent module unload occurs, dept_free_range() calls list_del() on the
class node:

kernel/dependency/dept.c:dept_free_range() {
	...
	list_del(&c->all_node);
	...
}

Because list_del() poisons the removed node's next pointer with LIST_POISON1,
a concurrent lockless reader in seq_list_next() will dereference LIST_POISON1
and cause a kernel panic, rather than just returning wrong information.

> +
> +static void l_stop(struct seq_file *m, void *v)
> +{
> +}
> +
> +static int l_show(struct seq_file *m, void *v)
> +{
> +	struct dept_class *fc = list_entry(v, struct dept_class, all_node);
> +	struct dept_dep *d;
> +	const char *prefix;
> +
> +	if (v == &dept_classes) {
> +		seq_puts(m, "All classes:\n\n");
> +		return 0;
> +	}
> +
> +	prefix = fc->sched_map ? "<sched> " : "";
> +	seq_printf(m, "[%p] %s%s\n", (void *)fc->key, prefix, fc->name);
> +
> +	/*
> +	 * XXX: Serialize list traversal if needed. The following might
> +	 * give a wrong information on contention.
> +	 */
> +	list_for_each_entry(d, &fc->dep_head, dep_node) {

[Severity: High]
Does traversing fc->dep_head without rcu_read_lock() risk a use-after-free?

Since the dependency objects in this list are RCU-managed, traversing them
without holding rcu_read_lock() might allow the objects to be freed to the
pool while they are still being iterated.

> +		struct dept_class *tc = d->wait->class;
> +
> +		prefix = tc->sched_map ? "<sched> " : "";
> +		seq_printf(m, " -> [%p] %s%s\n", (void *)tc->key, prefix, tc->name);
> +	}
> +	seq_puts(m, "\n");
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5