Re: [PATCH RFC 12/20] ctlv: Add device and operation dispatch

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] ctlv: mixing goto-based error handling with scope-based cleanup (__free)
--

commit d40367eef6344eb381abe711a6b81ec26de22d13
Author: Jiri Pirko <jiri@nvidia.com>

    ctlv: Add device and operation dispatch
    
Register CTLV devices, resolve family chains and dispatch operations
through validated messages. Wire CONFIG_CTLV and the generated root family
after all module objects are present.

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

[ ... ]

> +static int ctlv_op_exec(struct ctlv_file *cfile, struct file *filp, struct ctlv_op_exec *exec,
> +			void **reply, u32 *reply_len)
> +{
> +	struct ctlv_device *ctlvdev = cfile->ctlvdev;
> +	const struct ctlv_op_entry *op;
> +	struct ctlv_op_ctx ctx = {};
> +	u32 capacity;
> +	int ret;

[ ... ]

> +	void *snapshot __free(kvfree) = NULL;
> +	void *out __free(kvfree) = NULL;
> +
> +	if (exec->request_len < sizeof(struct ctlv_attr) ||
> +	    exec->request_len > CTLV_MAX_INLINE_MESSAGE_LEN)
> +		return -EINVAL;
> +	/* Everything after this reads the snapshot, never userspace. */
> +	snapshot = kvmalloc(exec->request_len, GFP_KERNEL_ACCOUNT);
> +	if (!snapshot)
> +		return -ENOMEM;
> +	if (copy_from_user(snapshot, u64_to_user_ptr(exec->request), exec->request_len))
> +		return -EFAULT;
> +	/* Before any hook, so that no family code sees an unchecked attr. */
> +	ret = ctlv_request_validate(&ctx, exec, snapshot);
> +	if (ret)
> +		goto out_error_info;

[ ... ]

> +	/* Where the caller carries on from, zero for anything but a query. */
> +	exec->generation = ctx.frag.reported;
> +	exec->cursor = ctx.frag.next_cursor;
> +	*reply_len = ctx.output.root ? ctx.output.root->len : 0;
> +	*reply = no_free_ptr(out);
> +	ret = 0;
> +	goto out_request;
> +
> +out_error_info:
> +	ctlv_error_info(&ctx, exec->reply_len, reply, reply_len);
> +out_request:
> +	ctlv_message_index_free(&ctx.request);
> +	return ret;
> +}

[Severity: Low]
Does this mix scope-based cleanup (__free) with goto-based error handling?

The cleanup guidelines (include/linux/cleanup.h) state that functions
should either convert all resources to scope-based cleanup or convert none
of them, but never mix the two in the same function.

[ ... ]

> +struct ctlv_device *ctlv_create(struct device *parent_dev, const struct ctlv_family *family,
> +				const void *init_info)
> +{
> +	struct ctlv_device *ctlvdev;
> +	int devnum, ret;

[ ... ]

> +	ctlv_event_contexts_start(ctlvdev);
> +	ret = ctlv_family_privs_post_init(ctlvdev);
> +	if (ret)
> +		goto err_fini;
> +	scoped_guard(rwsem_write, &ctlvdev->registration_lock) {
> +		ret = ctlv_device_add(ctlvdev);
> +		if (!ret)
> +			ctlvdev->registered = true;
> +	}
> +	if (ret)
> +		goto err_fini;
> +	return ctlvdev;
> +
> +err_fini:
> +	ctlv_device_fini(ctlvdev);
> +err_put:
> +	put_device(&ctlvdev->dev);
> +	return ERR_PTR(ret);
> +err_free:
> +	bitmap_free(ctlvdev->disabled);
> +	ctlv_family_privs_free(ctlvdev);
> +	kfree(ctlvdev->ops);
> +	kfree(ctlvdev);
> +	return ERR_PTR(ret);
> +}

[Severity: Low]
Does this mix scoped_guard with goto-based error handling?

The kernel guidelines expect functions to avoid mixing goto error labels and
scope-based cleanup macros like scoped_guard.

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