Re: [PATCH RFC 10/20] ctlv: Prepare event queue support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] race: TOCTOU in ctlv_event_file_read causes lost terminal events
- [High] mm: Out-of-bounds write when finalizing reserved event records with unaligned sizes
- [Medium] race: Queue misordering appends overflow event after terminal removed event
- [Low] cleanup: Mixing goto-based error cleanup with scope-based guards
--

commit 77cd7b72d04e886f1416198fdf738e02c8632520
Author: Jiri Pirko <jiri@resnulli.us>
Subject: ctlv: Prepare event queue support

Add per-open subscriptions, bounded event queues and event creation for
CTLV families. Add the generated root family kernel bindings now that their
message and event helpers are available.

> diff --git a/drivers/ctlv/event.c b/drivers/ctlv/event.c
> new file mode 100644
> index 0000000000000..0f68d33e031c8
> --- /dev/null
> +++ b/drivers/ctlv/event.c
> @@ -0,0 +1,713 @@
[ ... ]
> +static void ctlv_event_record_done(struct ctlv_op_ctx *ctx, struct ctlv_event_record *rec)
> +{
> +	u32 root_len = ctx->output.root->len;
> +
> +	rec->len = sizeof(rec->hdr) + CTLV_ALIGN(root_len);
> +	rec->hdr.length = rec->len;
> +	memset(rec->root + root_len, 0, rec->len - sizeof(rec->hdr) - root_len);
> +}
> +
> +static struct ctlv_event_record *ctlv_event_record_reserve(struct ctlv_device *ctlvdev,
> +							   const struct ctlv_op_entry *op)
> +{
> +	struct ctlv_event_record *rec;
> +
> +	rec = ctlv_event_record_alloc(ctlv_event_len(op), GFP_KERNEL_ACCOUNT);

[Severity: High]
Does this sequence cause an out-of-bounds write when finalizing reserved
event records with unaligned sizes?

In ctlv_event_record_reserve(), the record is allocated with a capacity of
ctlv_event_len(op). If the maximum record length from the schema is not a
multiple of 8, this length will be unaligned.

When root_len approaches this unaligned maximum in ctlv_event_record_done(),
CTLV_ALIGN(root_len) rounds up past the allocated size. The subsequent
memset() then writes padding zeros up to the aligned boundary, which can
cause an out-of-bounds write on the kernel heap.

[ ... ]
> +struct ctlv_op_ctx *ctlv_event_create(struct ctlv_device *ctlvdev, u64 op_id,
> +				      struct ctlv_cursor **reply, gfp_t gfp)
> +{
[ ... ]
> +	scoped_guard(spinlock, &ctlvdev->events_lock) {
> +		const struct ctlv_family *family;
> +
> +		if (!ctlvdev->events_live) {
> +			ret = -ENODEV;
> +			goto err_free_ctx;

[Severity: Low]
Is it safe to mix goto-based error cleanup with scope-based guards here?

The cleanup subsystem guide mandates that functions must never mix
goto-based error handling and guard()/scoped_guard() declarations.
Using goto to jump out of a scoped_guard creates confusing ownership
semantics.

[ ... ]
> +int ctlv_event_emit(struct ctlv_op_ctx *ctx)
> +{
[ ... ]
> +	scoped_guard(spinlock, &ctlvdev->events_lock) {
> +		if (ctlvdev->dead_announced)
> +			goto out_free;

[Severity: Low]
Does this goto also violate the cleanup guidelines?

Similar to ctlv_event_create(), jumping out of a scoped_guard to a cleanup
label is discouraged and violates the cleanup rules.

[ ... ]
> +static void ctlv_event_queue_consumed(struct ctlv_file *cfile, struct ctlv_event_record *rec)
> +{
> +	if (rec == cfile->overflow) {
> +		guard(spinlock)
> +			(&cfile->ctlvdev->events_lock);
> +		cfile->reading_overflow = false;
> +		if (cfile->overflowing)
> +			ctlv_event_queue_add(cfile, rec);

[Severity: Medium]
Can this queue misordering append the overflow event after the removed event?

If a reader is consuming the overflow marker while the queue receives another
dropped event, cfile->overflowing becomes true again. If
ctlv_event_device_dead() is then called, it enqueues the removed terminal
event. 

When the reader finishes and calls ctlv_event_queue_consumed(), it sees
overflowing == true and re-enqueues the overflow marker. Because
ctlv_event_queue_add() appends to the tail, the overflow marker is
incorrectly queued after the removed event, breaking monotonicity and the
terminal property of the removal event.

[ ... ]
> +ssize_t ctlv_event_file_read(struct ctlv_file *cfile, char __user *buf, size_t len, bool blocking)
> +{
[ ... ]
> +	while (!(rec = ctlv_event_queue_take(cfile))) {
> +		/* Drained, and the device is gone. */
> +		if (READ_ONCE(ctlvdev->dead_announced)) {
> +			ret = -ENODEV;
> +			goto out_unlock;
> +		}

[Severity: High]
Is there a race condition between the queue check and checking dead_announced?

The queue is checked locklessly (via ctlv_event_queue_take() returning NULL
and dropping the lock) before checking READ_ONCE(ctlvdev->dead_announced).

If ctlv_event_device_dead() runs exactly between these two checks, it will
queue the removed event and set dead_announced = true. The reader will then
see dead_announced as true and return -ENODEV immediately, completely
missing the freshly queued terminal removed event. This race prevents
userspace from observing the final state of the device.

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