Re: [PATCH RFC v2 10/15] ctlv: Prepare runtime introspection

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] ctlv: premature depth limit in ctlv_introspection_children()
  triggers WARN
--

commit 04c73a263bc43bc51c0a09b62604d548973da60c
Author: Jiri Pirko <jiri@resnulli.us>

ctlv: Prepare runtime introspection

This commit adds runtime introspection handlers for registered families,
effective operations, and message schemas through the CTLV root family, and
introduces struct ctlv.

> diff --git a/lib/ctlv/introspection.c b/lib/ctlv/introspection.c
> new file mode 100644
> index 0000000000000..d030debe69e3c
> --- /dev/null
> +++ b/lib/ctlv/introspection.c

[ ... ]

> +/* The members of a fill inherit the optionality of its extension point. */
> +static bool ctlv_introspection_children(struct ctlv_introspection_walk *walk, u32 depth,
> +					const struct ctlv_schema_tree *tree, u32 index,
> +					bool force_optional)
> +{
> +	const struct ctlv_node_schema *node = &tree->node_schemas[index];
> +	u32 i;
> +
> +	if (WARN_ON_ONCE(depth + 1 >= CTLV_MAX_RECORD_DEPTH)) {
> +		/* Deeper than a specification may declare, so unreachable. */
> +		walk->error = -EINVAL;
> +		return false;
> +	}

[ ... ]

> +/* One node, into the record that stands for it. */
> +static bool ctlv_introspection_node(struct ctlv_introspection_walk *walk, u32 depth,
> +				    const struct ctlv_schema_tree *tree, u32 index)
> +{
> +	const struct ctlv_node_schema *node = &tree->node_schemas[index];
> +	u32 reenters = 0;
> +
> +	if (node->reenters) {
> +		reenters = ctlv_introspection_reenters(walk, depth, tree, node);
> +		if (!reenters)
> +			return false;
> +	}
> +	if (!ctlv_introspection_node_fields(walk, depth, node, reenters))
> +		return false;
> +	/*
> +	 * A node that re-enters a level says so instead of listing that level's
> +	 * members again, which is what makes unbounded depth describable.
> +	 */
> +	if (!node->reenters && !ctlv_introspection_children(walk, depth, tree, index, false))
> +		return false;
> +	if (walk->frames[depth].open)
> +		ctlv_record_end(&walk->frames[depth].record);
> +	return true;
> +}

[ ... ]

> +int ctlv_op_schema_get_query(struct ctlv *ctlv, struct ctlv_op_ctx *ctx,
> +			     const struct ctlv_cursor *request, struct ctlv_cursor *reply)
> +{

[ ... ]

> +	/*
> +	 * One frame per level the reply may reach, too many for the stack: a
> +	 * description is one level deeper than the message it describes.
> +	 */
> +	struct ctlv_introspection_frame *frames __free(kfree) =
> +		kcalloc(CTLV_MAX_RECORD_DEPTH, sizeof(*frames), GFP_KERNEL);

[Severity: Medium]
Can ctlv_op_schema_get_query() trigger WARN_ON_ONCE() in
ctlv_introspection_children() and fail with -EINVAL on valid schemas near
CTLV_MAX_DECLARED_RECORD_DEPTH?

ctlv_introspection_node() calls ctlv_introspection_children() on every
non-reentering node, including scalar leaves with no children or extension
points, and ctlv_introspection_children() checks depth + 1 >=
CTLV_MAX_RECORD_DEPTH (8) before checking whether the node has any children.
A leaf field inside 6 nested records sits at walk depth 7, so describing it
hits the WARN_ON_ONCE() and returns -EINVAL.

In addition, CTLV_MAX_DECLARED_RECORD_DEPTH (7) counts only record levels,
whereas op-schema-get wraps both the message root (walk->frames[0]) and the
leaf fields of the deepest record in CTLV_SCHEMA_RECORD nodes.  A message at
the maximum declared record depth of 7 needs walk depth 8 (9 record levels in
the reply), which exceeds both the kcalloc(CTLV_MAX_RECORD_DEPTH, ...)
allocation and the CTLV_MAX_RECORD_DEPTH limit in ctlv_record_start().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113155515.2510194-1-jiri@resnulli.us?part=10