Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider: - [Critical] core: stack use-after-free on virtqueue timeout - [High] virtio: overlapping IN/OUT scatterlist mappings cause DMA aliasing - [High] virtio: missing virtio_reset_device() before deleting virtqueues - [High] cpufreq: self-deadlock in .target_index due to double transition_begin - [High] virtio: concurrent virtqueue modification due to missing IRQ-safe locking - [Medium] cpufreq: global pointer clobbering breaks active device on probe failure - [Medium] virtio: premature allocation of unassigned device ID in UAPI - [Medium] cpufreq: mutex held across blocking timeout serializes all CPUs --
commit e9a0cf53064fc73cd4797ce5c61c255c2aef1ee7 Author: Baorui.Liu <[email protected]> cpufreq: virtio: add driver to report host frequency to guests Some virtualization stacks pin each vCPU to a pCPU. Guest software still needs the current host frequency for energy accounting, but the guest cannot access host sysfs. Add a virtio frontend that queries the host for the frequency of the mapped pCPU and exposes it through the cpufreq .get() callback. > diff --git a/drivers/cpufreq/virtio-cpufreq.c > b/drivers/cpufreq/virtio-cpufreq.c > new file mode 100644 > index 0000000000000..23fde862a3137 > --- /dev/null > +++ b/drivers/cpufreq/virtio-cpufreq.c [ ... ] > +static int virtio_cpufreq_target_index(struct cpufreq_policy *policy, > + unsigned int index) > +{ > + struct cpufreq_freqs freqs; > + > + freqs.old = policy->cur; > + freqs.new = policy->freq_table[index].frequency; > + > + /* > + * This frontend does not change host frequency. It only keeps > + * the cpufreq core in sync so userspace can observe values. > + */ > + cpufreq_freq_transition_begin(policy, &freqs); > + cpufreq_freq_transition_end(policy, &freqs, 0); [Severity: High] Is it possible this leads to a self-deadlock? For drivers without the CPUFREQ_ASYNC_NOTIFICATION flag, the cpufreq core automatically invokes cpufreq_freq_transition_begin() on their behalf before calling the .target_index() callback. Calling it a second time here might trigger a double-invocation warning in the core and cause the current task to wait on itself forever. > + > + return 0; > +} [ ... ] > +static void virtio_cpufreq_vq_cb(struct virtqueue *vq) > +{ > + struct completion *done; > + unsigned int len; > + > + while ((done = virtqueue_get_buf(vq, &len)) != NULL) > + complete(done); [Severity: High] Does virtqueue_get_buf() need to be protected by an IRQ-safe spinlock? Since virtqueue_add_sgs() modifies the virtqueue from process context in virtio_cpufreq_get(), and this IRQ callback modifies the same state here, could they race and corrupt the virtqueue structures if an interrupt fires on the same or another CPU? > +} > + > +static unsigned int virtio_cpufreq_get(unsigned int cpu) > +{ > + struct virtio_cpufreq *vc = virtio_cpufreq_dev; > + struct virtio_cpufreq_req *req; > + struct scatterlist out_sg, in_sg, *sgs[2]; > + struct completion done; > + unsigned int freq_khz; > + int ret; > + > + if (!vc || !vc->vq) > + return virtio_cpufreq_fallback(cpu); > + > + req = kzalloc_obj(*req, GFP_KERNEL); > + if (!req) > + return virtio_cpufreq_fallback(cpu); > + > + req->cpu_id = cpu_to_le32(cpu); > + > + init_completion(&done); > + sg_init_one(&out_sg, req, sizeof(*req)); > + sg_init_one(&in_sg, req, sizeof(*req)); [Severity: High] Does using the exact same buffer and size for both input and output scatterlists cause DMA aliasing? Since virtqueue_add_sgs() maps these simultaneously as DMA_TO_DEVICE and DMA_FROM_DEVICE, could this violate the DMA API? On systems relying on SWIOTLB or non-coherent caches, cache operations might trample each other and cause data corruption. Should the request fields be split into separate, non-overlapping buffers (for example, mapping &req->cpu_id for output and &req->freq_khz for input)? > + sgs[0] = &out_sg; > + sgs[1] = &in_sg; > + > + mutex_lock(&vc->lock); > + ret = virtqueue_add_sgs(vc->vq, sgs, 1, 1, &done, GFP_KERNEL); > + if (ret) { > + mutex_unlock(&vc->lock); > + kfree(req); > + return virtio_cpufreq_fallback(cpu); > + } > + > + virtqueue_kick(vc->vq); > + ret = wait_for_completion_timeout(&done, msecs_to_jiffies(1000)); > + mutex_unlock(&vc->lock); [Severity: Medium] Does holding the mutex across the blocking wait_for_completion_timeout() call serialize all CPUs? By holding vc->lock while waiting for up to 1000ms, does this block all other CPUs from submitting frequency queries, potentially causing latency spikes globally across the system? [Severity: Critical] Can a timeout lead to a stack use-after-free? When wait_for_completion_timeout() times out, this function returns immediately and abandons the request in the virtqueue. Since the completion token is allocated on the local stack, could the hard IRQ callback virtio_cpufreq_vq_cb() write to deallocated stack memory when the device eventually completes the request? > + > + if (!ret) { > + /* > + * The buffer may still be on the virtqueue. Leak it > + * rather than freeing while the host may still write. > + */ > + return virtio_cpufreq_fallback(cpu); > + } > + > + freq_khz = le32_to_cpu(req->freq_khz); > + kfree(req); [ ... ] > +static int virtio_cpufreq_probe(struct virtio_device *vdev) > +{ > + struct virtio_cpufreq *vc; > + struct virtqueue *vq; > + int ret; > + > + vc = devm_kzalloc(&vdev->dev, sizeof(*vc), GFP_KERNEL); > + if (!vc) > + return -ENOMEM; > + > + mutex_init(&vc->lock); > + vc->vdev = vdev; > + > + vq = virtio_find_single_vq(vdev, virtio_cpufreq_vq_cb, "requests"); > + if (IS_ERR(vq)) > + return PTR_ERR(vq); > + > + vc->vq = vq; > + vdev->priv = vc; > + virtio_cpufreq_dev = vc; > + virtio_device_ready(vdev); > + > + ret = cpufreq_register_driver(&virtio_cpufreq_driver); > + if (ret) { > + vdev->config->del_vqs(vdev); > + virtio_cpufreq_dev = NULL; [Severity: Medium] Could this unconditional assignment break an already active device if a second probe fails? If a second virtio-cpufreq device is probed, it overwrites the global virtio_cpufreq_dev pointer. When cpufreq_register_driver() fails with -EEXIST, the error path sets virtio_cpufreq_dev to NULL, destroying the global pointer for the first successfully probed device. [Severity: High] Does the device need to be stopped with virtio_reset_device(vdev) before deleting its virtqueues? Since virtio_device_ready(vdev) has already enabled the device, could deleting the queues without resetting the device first cause hypervisor crashes or memory corruption if the device is still attempting to access memory or raise IRQs? > + return ret; > + } > + > + return 0; > +} > + > +static void virtio_cpufreq_remove(struct virtio_device *vdev) > +{ > + cpufreq_unregister_driver(&virtio_cpufreq_driver); > + virtio_cpufreq_dev = NULL; > + vdev->config->del_vqs(vdev); [Severity: High] Is a call to virtio_reset_device(vdev) missing here before freeing the queues? The virtio specification and driver model generally require that the device is stopped prior to deleting its virtqueues. > +} [ ... ] > diff --git a/include/uapi/linux/virtio_ids.h b/include/uapi/linux/virtio_ids.h > index f9056af0c6223..f7e357f3bf946 100644 > --- a/include/uapi/linux/virtio_ids.h > +++ b/include/uapi/linux/virtio_ids.h > @@ -68,6 +68,7 @@ > #define VIRTIO_ID_AUDIO_POLICY 39 /* virtio audio policy */ > #define VIRTIO_ID_BT 40 /* virtio bluetooth */ > #define VIRTIO_ID_GPIO 41 /* virtio gpio */ > +#define VIRTIO_ID_CPUFREQ 42 /* virtio cpufreq */ [Severity: Medium] Is it premature to add an unallocated device ID into the UAPI headers? Since the commit message notes this ID is not yet allocated by the virtio specification, including it here might freeze the ABI and risk collisions if the standard assigns 42 to a different device type in the future. > #define VIRTIO_ID_SPI 45 /* virtio spi */ > #define VIRTIO_ID_MEDIA 48 /* virtio media */ -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
