Re: [PATCH v3 7/7] clk: tests: Add Kunit testing for nexus nodes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The `of_find_node_by_name_kunit` helper creates a double-free vulnerability if a KUnit-managed node is passed as the `from` argument.
- [Low] The `of_find_node_by_name_kunit` kernel-doc incorrectly states the return value is only a node or negative errno, omitting the NULL case when a node is not found.
--

--- Patch [7]: [PATCH v3 7/7] clk: tests: Add Kunit testing for nexus nodes ---
commit 5c005eb5dfe6aeb07b97b34ac1e15ba0def6eeae
Author: Miquel Raynal (Schneider Electric) <miquel.raynal@bootlin.com>

clk: tests: Add Kunit testing for nexus nodes

This commit adds a fake DT overlay with a nexus node to verify clock parsing
in KUnit tests. It also introduces of_find_node_by_name_kunit() to manage
device node lifetimes during tests.

> diff --git a/drivers/clk/clk_kunit_helpers.c b/drivers/clk/clk_kunit_helpers.c
> index 68a28e70bb61c..151f62ab57c1a 100644
> --- a/drivers/clk/clk_kunit_helpers.c
> +++ b/drivers/clk/clk_kunit_helpers.c
[ ... ]
> +/**
> + * of_find_node_by_name_kunit() - Test managed of_find_node_by_name()
> + * @test: The test context
> + * @from: Parent device node to start searching from, or NULL to search from root
> + * @name: The name string to match against
> + *
> + * Just like of_find_node_by_name(), except the device_noded is managed by
> + * the test case and is automatically put after the test case concludes.
> + *
> + * Return: the device_node on success or a negative errno value on failure.

[Severity: Low]
This isn't a bug, but the kernel-doc states the function returns a device_node
on success or a negative errno on failure.

However, the implementation also returns NULL if the node is not found.

Could the documentation be updated to mention the NULL return case, so
callers don't assume IS_ERR() is sufficient?

> + */
> +struct device_node *of_find_node_by_name_kunit(struct kunit *test, struct device_node *from,
> +					       const char *name)
> +{
> +	struct device_node *np;
> +	int ret;
> +
> +	np = of_find_node_by_name(from, name);

[Severity: Medium]
Could this result in a double-free if a KUnit-managed node is passed as the
'from' argument?

of_find_node_by_name() automatically consumes and drops the reference to the
'from' node. If a developer chains KUnit-managed lookups:

    parent = of_find_node_by_name_kunit(test, NULL, "parent_name");
    child = of_find_node_by_name_kunit(test, parent, "child_name");

The inner of_find_node_by_name() will drop the 'parent' reference.

Since KUnit is unaware of this consumption, won't the deferred test cleanup
still call of_node_put() on 'parent' at test exit, resulting in a guaranteed
double-free?

> +	if (!np)
> +		return NULL;
> +
> +	ret = kunit_add_action_or_reset(test, of_node_put_wrapper, np);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	return np;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716-schneider-v7-2-rc1-eip201-upstream-v3-0-98454e83845c@bootlin.com?part=7