Re: [PATCH RFC v2 11/15] ctlv: Add operation dispatch

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] ctlv: broken request rebinding and WARN_ON_ONCE in
  ctlv_ext_fill()
- [Low] ctlv: unclaimed ancestor families and bad log in ctlv_init()
--

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

ctlv: Add operation dispatch

This commit adds CTLV family resolution, instance registration, file
operations, and ioctl operation dispatch.

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

[ ... ]

> +/**
> + * ctlv_ext_fill - Delegate an extension-point output record.
> + * @ctx: operation context
> + * @request: validated request root
> + * @record: output record selected by the generated helper
> + *
> + * Return: filling callback result, or 0 for an unfilled point.
> + */
> +int ctlv_ext_fill(struct ctlv_op_ctx *ctx, const struct ctlv_cursor *request,
> +		  struct ctlv_cursor *record)
> +{
> +	const struct ctlv_schema_tree *tree;
> +	u32 index;
> +
> +	if (WARN_ON_ONCE(!record->tree))
> +		return -EINVAL;
> +	tree = record->tree;
> +
> +	for (index = 0; index < tree->ext_schemas_count; index++) {
> +		const struct ctlv_ext_schema *ext_schema = &tree->ext_schemas[index];
> +		const struct ctlv_cursor *filled_request = request;
> +		const struct ctlv_ext_fill_schema *fill_schema;
> +		const struct ctlv_ext_ops *ext_ops;
> +		struct ctlv_cursor filled;
> +		int ret;
> +
> +		if (ext_schema->node != record->node)
> +			continue;
> +		ext_ops = ctlv_ext_ops_find(ctx->ctlv, ext_schema);
> +		if (!ext_ops || !ext_ops->fill)
> +			continue;
> +
> +		if (request) {
> +			fill_schema = ctlv_schema_fill(ctx->family, ext_schema->family_id,
> +						       ext_schema->ext_id);
> +			if (WARN_ON_ONCE(!fill_schema))
> +				return -EINVAL;
> +			filled = *request;
> +			if (ctlv_message_input_rebind(&filled, &fill_schema->members, 0))
> +				filled_request = &filled;
> +			else
> +				filled_request = NULL;
> +		}

[Severity: Medium]
Can ctlv_ext_fill() trigger WARN_ON_ONCE(!record->hdr) in
ctlv_message_input_rebind() when userspace omits an optional request
extension record?

When a parent operation looks up the request extension-point record with
its generated helper and the optional record was omitted in the request
message, ctlv_input_field() sets request->hdr = NULL.  Passing that cursor
to ctlv_ext_fill() enters the if (request) branch and passes filled.hdr ==
NULL to ctlv_message_input_rebind():

lib/ctlv/message.c:ctlv_message_input_rebind() {
	if (WARN_ON_ONCE(!record->hdr || !record->msg || !record->msg->index))
		return false;
    ...
}

Also, the kernel-doc above says @request is the validated request root,
whereas extension fill scopes are attached to the child extension record
rather than index->records[0], so passing the request root cursor always
fails to rebind and sets filled_request = NULL.  Should ctlv_ext_fill()
check ctlv_cursor_ok(request) (setting filled_request = NULL when absent)
and document @request as the matching request extension record?

[ ... ]

> +int ctlv_init(struct ctlv *ctlv, struct device *dev,
> +	      const struct ctlv_family *family, const struct ctlv_hooks *hooks)
> +{
> +	int ret;
> +
> +	/* Nothing in common means no pair, not half of one. */
> +	if (!dev || !family || (hooks && (!hooks->pre || !hooks->post)))
> +		return -EINVAL;
> +
> +	ctlv->dev = dev;
> +	ctlv->hooks = hooks;
> +	init_rwsem(&ctlv->registration_lock);
> +	spin_lock_init(&ctlv->transition_lock);
> +
> +	/* Before anything reads the schema through this instance. */
> +	ret = ctlv_op_resolution_init(ctlv, family);
> +	if (ret)
> +		return ret;
> +
> +	/* Before a subsystem announces a device that could not be. */
> +	ret = ctlv_uevent_check(family);
> +	if (ret)
> +		return ret;
> +
> +	if (ctlv->dynamic_ops_count) {
> +		ctlv->disabled = bitmap_zalloc(ctlv->dynamic_ops_count, GFP_KERNEL);
> +		if (!ctlv->disabled)
> +			return -ENOMEM;
> +	}
> +
> +	ret = ctlv_family_claim_add(family->schema);
> +	if (ret)
> +		return ret;
> +	ctlv->family = family;
> +	return 0;
> +}

[Severity: Low]
Does ctlv_init() leave intermediate ancestor families unclaimed in
ctlv_family_claims?

ctlv_init() only calls ctlv_family_claim_add(family->schema) on the leaf
family, so base families that are only inherited and never instantiated on
their own are not recorded in ctlv_family_claims, allowing conflicting
ancestor names or family_ids across chains.

In addition, when ctlv_family_claim_add() hits a name collision with a
different family_id (same_name && !same_id), its pr_err() reports that
schema->family_id belongs to claim->name instead of claim->family_id.
Finally, ctlv_family_chain_check() and ctlv_family_descriptor_check() do
not check that child->schema->name[len + 1] is non-NUL after '-' or that
strlen(schema->name) fits the 63-byte limit of
CTLV_T_FAMILY_GET_FAMILIES_NAME before CTLV_OP_FAMILY_GET hits a runtime
WARN_ON_ONCE in ctlv_put_string().

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