* [PATCH] tools/virtio: fix build for kmalloc_obj API and missing stubs
From: Michael S. Tsirkin @ 2026-06-03 21:06 UTC (permalink / raw)
To: linux-kernel; +Cc: Jason Wang, Xuan Zhuo, Eugenio Pérez, virtualization
Add stubs for kmalloc_obj() and kmalloc_objs() to the tools/virtio
test harness, matching the new kernel allocator API. Also add the
DMA_ATTR_CPU_CACHE_CLEAN definition and include kernel.h from err.h
for the unlikely() macro.
Assisted-by: Claude:claude-opus-4-6
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
tools/virtio/linux/dma-mapping.h | 2 ++
tools/virtio/linux/err.h | 1 +
tools/virtio/linux/kernel.h | 6 ++++++
3 files changed, 9 insertions(+)
diff --git a/tools/virtio/linux/dma-mapping.h b/tools/virtio/linux/dma-mapping.h
index fddfa2fbb276..8d1a16cb20db 100644
--- a/tools/virtio/linux/dma-mapping.h
+++ b/tools/virtio/linux/dma-mapping.h
@@ -60,4 +60,6 @@ enum dma_data_direction {
*/
#define DMA_MAPPING_ERROR (~(dma_addr_t)0)
+#define DMA_ATTR_CPU_CACHE_CLEAN (1UL << 11)
+
#endif
diff --git a/tools/virtio/linux/err.h b/tools/virtio/linux/err.h
index 0943c644a701..b7b4cb516dc9 100644
--- a/tools/virtio/linux/err.h
+++ b/tools/virtio/linux/err.h
@@ -1,6 +1,7 @@
/* SPDX-License-Identifier: GPL-2.0 */
#ifndef ERR_H
#define ERR_H
+#include <linux/kernel.h>
#define MAX_ERRNO 4095
#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
diff --git a/tools/virtio/linux/kernel.h b/tools/virtio/linux/kernel.h
index 416d02703f61..104abf9d1aee 100644
--- a/tools/virtio/linux/kernel.h
+++ b/tools/virtio/linux/kernel.h
@@ -65,6 +65,12 @@ static inline void *kmalloc_array(unsigned n, size_t s, gfp_t gfp)
return kmalloc(n * s, gfp);
}
+#define kmalloc_obj(VAR_OR_TYPE, ...) \
+ ((typeof(VAR_OR_TYPE) *)kmalloc(sizeof(typeof(VAR_OR_TYPE)), 0))
+
+#define kmalloc_objs(VAR_OR_TYPE, COUNT, ...) \
+ ((typeof(VAR_OR_TYPE) *)kmalloc(sizeof(typeof(VAR_OR_TYPE)) * (COUNT), 0))
+
static inline void *kzalloc(size_t s, gfp_t gfp)
{
void *p = kmalloc(s, gfp);
--
MST
^ permalink raw reply related
* [PATCH v3 4/4] virtio_console: fix race between hvc put_chars and virtqueue teardown on freeze
From: Sungho Bae @ 2026-06-03 18:37 UTC (permalink / raw)
To: amit, arnd, gregkh; +Cc: virtualization, linux-kernel, Sungho Bae
In-Reply-To: <20260603183757.21587-1-baver.bae@gmail.com>
From: Sungho Bae <baver.bae@lge.com>
With no_console_suspend enabled, hvc console output can continue while
virtio_console is freezing. In that window, put_chars can still enqueue
buffers to the output virtqueue while virtcons_freeze is tearing queues
down, triggering a BUG_ON in virtqueue_detach_unused_buf_split:
BUG_ON(vq->vq.num_free != vq->split.vring.num)
Add a pm_freezing flag to ports_device. Set it via smp_store_release()
at the start of virtcons_freeze(); put_chars() and __send_to_port() drop
output while the flag is set, checked via smp_load_acquire().
The check in __send_to_port() is placed under outvq_lock, making it
atomic with remove_port_data() which also acquires outvq_lock. Once
remove_port_data() returns for a given port, no concurrent
__send_to_port() can add buffers before remove_vqs() tears down the vq.
After setting pm_freezing, acquire and release outvq_lock for each port
(protected by ports_lock to prevent list manipulation races) before
calling virtio_reset_device(). A TX thread that already passed the
pm_freezing check may still hold outvq_lock while spinning for host
acknowledgment; the drain loop ensures all such threads have completed
before the device is reset.
Clear pm_freezing in virtcons_restore() only after all port->out_vq
pointers have been reassigned to the newly allocated virtqueues,
preventing TX paths from dereferencing freed vqs during restore.
Link: https://sashiko.dev/#/patchset/20260519162242.7324-1-baver.bae%40gmail.com
Signed-off-by: Sungho Bae <baver.bae@lge.com>
---
drivers/char/virtio_console.c | 70 ++++++++++++++++++++++++++++++++---
1 file changed, 64 insertions(+), 6 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index dd31f7069e19..482d57a311c6 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -157,6 +157,12 @@ struct ports_device {
/* Major number for this device. Ports will be created as minors. */
int chr_major;
+
+ /*
+ * Set to true during PM freeze to block TX paths that may race
+ * with virtqueue teardown (e.g. hvc put_chars with no_console_suspend).
+ */
+ bool pm_freezing;
};
struct port_stats {
@@ -611,6 +617,17 @@ static ssize_t __send_to_port(struct port *port, struct scatterlist *sg,
goto free_and_done;
}
+ /*
+ * Check freeze flag under the lock so that the flag check and
+ * virtqueue_add_outbuf() are atomic with respect to
+ * remove_port_data() which also takes outvq_lock. This
+ * guarantees that once remove_port_data() returns, no new
+ * buffers can be added before remove_vqs() tears down the vq.
+ * Pairs with smp_store_release() in virtcons_freeze/restore.
+ */
+ if (smp_load_acquire(&portdev->pm_freezing)) /* pairs with freeze/restore */
+ goto free_and_done;
+
out_vq = port->out_vq;
reclaim_consumed_buffers(port);
@@ -1125,14 +1142,27 @@ static ssize_t put_chars(u32 vtermno, const u8 *buf, size_t count)
return -EPIPE;
/*
- * Silently drop output if device hot-unplug is in progress.
- * portdev was NULLed by unplug_port() after hvc_remove() was
- * already called, so the hvc layer will stop invoking put_chars()
- * very soon. Returning count avoids a pointless retry loop in the
- * interim.
+ * Silently drop output in two cases, both by returning count so
+ * that the hvc layer does not spin-retry:
+ *
+ * 1. Device hot-unplug (!portdev): portdev was NULLed by
+ * unplug_port() after hvc_remove() was already called, so
+ * the hvc layer will stop invoking put_chars() very soon.
+ * Returning count avoids a pointless retry loop in the
+ * interim.
+ *
+ * 2. PM freeze (pm_freezing): the hvc console stays active
+ * under no_console_suspend but virtqueues are being torn
+ * down. Drop the output silently so the hvc layer does not
+ * stall suspend.
+ *
+ * This early check avoids a pointless GFP_ATOMIC allocation;
+ * __send_to_port() rechecks under outvq_lock for correctness.
+ * Pairs with smp_store_release() in virtcons_freeze/restore.
*/
portdev = READ_ONCE(port->portdev);
- if (!portdev)
+ if (!portdev ||
+ smp_load_acquire(&portdev->pm_freezing)) /* pairs with freeze/restore */
return count;
pbuf = alloc_buf(portdev->vdev, count, 0, GFP_ATOMIC);
@@ -2002,6 +2032,7 @@ static int virtcons_probe(struct virtio_device *vdev)
/* Attach this portdev to this virtio_device, and vice-versa. */
portdev->vdev = vdev;
vdev->priv = portdev;
+ portdev->pm_freezing = false;
portdev->chr_major = register_chrdev(0, "virtio-portsdev",
&portdev_fops);
@@ -2119,9 +2150,30 @@ static int virtcons_freeze(struct virtio_device *vdev)
{
struct ports_device *portdev;
struct port *port;
+ unsigned long flags;
portdev = vdev->priv;
+ /*
+ * Block TX paths (put_chars, __send_to_port) before resetting the
+ * device and tearing down virtqueues. This prevents races with
+ * hvc console writes that remain active under no_console_suspend.
+ */
+ smp_store_release(&portdev->pm_freezing, true);
+
+ /*
+ * Synchronize with any concurrent __send_to_port() that may have
+ * passed the pm_freezing check. By acquiring and releasing the
+ * outvq_lock for each port, we ensure all active TX paths have
+ * completed before we reset the device.
+ */
+ spin_lock_irqsave(&portdev->ports_lock, flags);
+ list_for_each_entry(port, &portdev->ports, list) {
+ spin_lock(&port->outvq_lock);
+ spin_unlock(&port->outvq_lock);
+ }
+ spin_unlock_irqrestore(&portdev->ports_lock, flags);
+
virtio_reset_device(vdev);
if (use_multiport(portdev))
@@ -2192,6 +2244,12 @@ static int virtcons_restore(struct virtio_device *vdev)
if (use_multiport(portdev))
fill_queue(portdev->c_ivq, &portdev->c_ivq_lock);
+ /*
+ * Allow TX paths only after all port->out_vq pointers have
+ * been reassigned to the newly allocated virtqueues.
+ */
+ smp_store_release(&portdev->pm_freezing, false);
+
return 0;
}
#endif
--
2.34.1
^ permalink raw reply related
* [PATCH v3 3/4] virtio_console: fix control queue race during restore
From: Sungho Bae @ 2026-06-03 18:37 UTC (permalink / raw)
To: amit, arnd, gregkh; +Cc: virtualization, linux-kernel, Sungho Bae
In-Reply-To: <20260603183757.21587-1-baver.bae@gmail.com>
From: Sungho Bae <baver.bae@lge.com>
In virtcons_restore(), after virtio_device_ready() sets DRIVER_OK, the
device becomes active. If the control receive queue (c_ivq) is populated
immediately, the host can instantly deliver pending control messages
(e.g., VIRTIO_CONSOLE_PORT_REMOVE).
This triggers the control_work_handler(), which can modify the
portdev->ports list concurrently with the unprotected list_for_each_entry
loop in virtcons_restore(), leading to list corruption or Use-After-Free.
Fix this by deferring the population of the control receive queue
(fill_queue for c_ivq) until after the list iteration is complete. This
ensures the host cannot inject control messages during the vulnerable
window.
Signed-off-by: Sungho Bae <baver.bae@lge.com>
---
drivers/char/virtio_console.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 589a12261e23..dd31f7069e19 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -2164,9 +2164,6 @@ static int virtcons_restore(struct virtio_device *vdev)
virtio_device_ready(portdev->vdev);
- if (use_multiport(portdev))
- fill_queue(portdev->c_ivq, &portdev->c_ivq_lock);
-
list_for_each_entry(port, &portdev->ports, list) {
port->in_vq = portdev->in_vqs[port->id];
port->out_vq = portdev->out_vqs[port->id];
@@ -2183,6 +2180,18 @@ static int virtcons_restore(struct virtio_device *vdev)
if (port->guest_connected)
send_control_msg(port, VIRTIO_CONSOLE_PORT_OPEN, 1);
}
+
+ /*
+ * Populate the control receive queue only after the list iteration
+ * is complete. If we fill this queue before iterating, the host could
+ * immediately deliver a VIRTIO_CONSOLE_PORT_REMOVE message.
+ * This would trigger the control workqueue, which modifies the
+ * portdev->ports list concurrently with the unprotected loop above,
+ * leading to a Use-After-Free and list corruption.
+ */
+ if (use_multiport(portdev))
+ fill_queue(portdev->c_ivq, &portdev->c_ivq_lock);
+
return 0;
}
#endif
--
2.34.1
^ permalink raw reply related
* [PATCH v3 2/4] virtio_console: fix hot-unplug races in TX paths
From: Sungho Bae @ 2026-06-03 18:37 UTC (permalink / raw)
To: amit, arnd, gregkh; +Cc: virtualization, linux-kernel, Sungho Bae
In-Reply-To: <20260603183757.21587-1-baver.bae@gmail.com>
From: Sungho Bae <baver.bae@lge.com>
When a port is hot-unplugged, unplug_port() nullifies port->portdev.
However, concurrent TX paths (__send_to_port, put_chars) could read a
stale pointer or encounter a NULL pointer dereference.
Add READ_ONCE(port->portdev) and NULL checks in the TX paths. In
__send_to_port(), move the out_vq assignment inside the outvq_lock and
check portdev under the lock. Correspondingly, update unplug_port() to
NULL out port->portdev while holding the outvq_lock to serialize with
__send_to_port().
In put_chars(), return count instead of 0 on unplug to prevent the hvc
layer from spinning in an infinite retry loop.
Signed-off-by: Sungho Bae <baver.bae@lge.com>
---
drivers/char/virtio_console.c | 37 +++++++++++++++++++++++++++++------
1 file changed, 31 insertions(+), 6 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index bbf5b3825f12..589a12261e23 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -600,11 +600,19 @@ static ssize_t __send_to_port(struct port *port, struct scatterlist *sg,
int err;
unsigned long flags;
unsigned int len;
-
- out_vq = port->out_vq;
+ struct ports_device *portdev;
spin_lock_irqsave(&port->outvq_lock, flags);
+ portdev = READ_ONCE(port->portdev);
+
+ if (!portdev) {
+ in_count = 0;
+ goto free_and_done;
+ }
+
+ out_vq = port->out_vq;
+
reclaim_consumed_buffers(port);
err = virtqueue_add_outbuf(out_vq, sg, nents, buf, GFP_ATOMIC);
@@ -1110,12 +1118,24 @@ static ssize_t put_chars(u32 vtermno, const u8 *buf, size_t count)
struct port *port;
struct scatterlist sg[1];
struct port_buffer *pbuf;
+ struct ports_device *portdev;
port = find_port_by_vtermno(vtermno);
if (!port)
return -EPIPE;
- pbuf = alloc_buf(port->portdev->vdev, count, 0, GFP_ATOMIC);
+ /*
+ * Silently drop output if device hot-unplug is in progress.
+ * portdev was NULLed by unplug_port() after hvc_remove() was
+ * already called, so the hvc layer will stop invoking put_chars()
+ * very soon. Returning count avoids a pointless retry loop in the
+ * interim.
+ */
+ portdev = READ_ONCE(port->portdev);
+ if (!portdev)
+ return count;
+
+ pbuf = alloc_buf(portdev->vdev, count, 0, GFP_ATOMIC);
if (!pbuf)
return -ENOMEM;
@@ -1503,11 +1523,16 @@ static void unplug_port(struct port *port)
remove_port_data(port);
/*
- * We should just assume the device itself has gone off --
- * else a close on an open port later will try to send out a
- * control message.
+ * Null out portdev under outvq_lock so that __send_to_port()
+ * cannot race: it checks port->portdev inside the same lock
+ * and bails out if NULL, preventing any buffer from being
+ * enqueued to an already torn-down virtqueue. Also prevents
+ * a close on an open port later from sending a stale control
+ * message.
*/
+ spin_lock_irq(&port->outvq_lock);
port->portdev = NULL;
+ spin_unlock_irq(&port->outvq_lock);
sysfs_remove_group(&port->dev->kobj, &port_attribute_group);
device_destroy(&port_class, port->dev->devt);
--
2.34.1
^ permalink raw reply related
* [PATCH v3 1/4] virtio_console: refactor __send_to_port() buffer ownership
From: Sungho Bae @ 2026-06-03 18:37 UTC (permalink / raw)
To: amit, arnd, gregkh; +Cc: virtualization, linux-kernel, Sungho Bae
In-Reply-To: <20260603183757.21587-1-baver.bae@gmail.com>
From: Sungho Bae <baver.bae@lge.com>
Modify __send_to_port() to take ownership of a struct port_buffer *
instead of a void * raw buffer.
Previously, put_chars() would pass a raw kmemdup'd buffer and free it
immediately after __send_to_port() returned. This caused a potential
Use-After-Free and data corruption if the virtqueue was shared with
nonblocking writers, as virtqueue_get_buf() might return an older
completed buffer, causing the newly added buffer to be kfree'd while the
host is still DMAing from it.
By transferring ownership of the allocated port_buffer to __send_to_port(),
we ensure that the exact buffer returned by the host is the one that gets
freed, resolving the memory lifecycle mismatch.
Signed-off-by: Sungho Bae <baver.bae@lge.com>
---
drivers/char/virtio_console.c | 69 +++++++++++++++++++----------------
1 file changed, 37 insertions(+), 32 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 9a33217c68d9..bbf5b3825f12 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -402,7 +402,7 @@ static void reclaim_dma_bufs(void)
}
static struct port_buffer *alloc_buf(struct virtio_device *vdev, size_t buf_size,
- int pages)
+ int pages, gfp_t gfp)
{
struct port_buffer *buf;
@@ -436,11 +436,10 @@ static struct port_buffer *alloc_buf(struct virtio_device *vdev, size_t buf_size
/* Increase device refcnt to avoid freeing it */
get_device(buf->dev);
- buf->buf = dma_alloc_coherent(buf->dev, buf_size, &buf->dma,
- GFP_KERNEL);
+ buf->buf = dma_alloc_coherent(buf->dev, buf_size, &buf->dma, gfp);
} else {
buf->dev = NULL;
- buf->buf = kmalloc(buf_size, GFP_KERNEL);
+ buf->buf = kmalloc(buf_size, gfp);
}
if (!buf->buf)
@@ -595,7 +594,7 @@ static void reclaim_consumed_buffers(struct port *port)
static ssize_t __send_to_port(struct port *port, struct scatterlist *sg,
int nents, size_t in_count,
- void *data, bool nonblock)
+ struct port_buffer *buf, bool nonblock)
{
struct virtqueue *out_vq;
int err;
@@ -608,14 +607,14 @@ static ssize_t __send_to_port(struct port *port, struct scatterlist *sg,
reclaim_consumed_buffers(port);
- err = virtqueue_add_outbuf(out_vq, sg, nents, data, GFP_ATOMIC);
+ err = virtqueue_add_outbuf(out_vq, sg, nents, buf, GFP_ATOMIC);
/* Tell Host to go! */
virtqueue_kick(out_vq);
if (err) {
in_count = 0;
- goto done;
+ goto free_and_done;
}
if (out_vq->num_free == 0)
@@ -632,10 +631,19 @@ static ssize_t __send_to_port(struct port *port, struct scatterlist *sg,
* buffer and relax the spinning requirement. The downside is
* we need to kmalloc a GFP_ATOMIC buffer each time the
* console driver writes something out.
+ *
+ * Spin until host returns the buffer.
+ * Capture the returned buf so we can free it.
+ * If broken, buf == NULL and buf stays in the vq;
+ * remove_vqs() will call virtqueue_detach_unused_buf() -> free_buf().
*/
- while (!virtqueue_get_buf(out_vq, &len)
+ while (!(buf = virtqueue_get_buf(out_vq, &len))
&& !virtqueue_is_broken(out_vq))
cpu_relax();
+
+free_and_done:
+ if (buf)
+ free_buf(buf, false);
done:
spin_unlock_irqrestore(&port->outvq_lock, flags);
@@ -816,14 +824,14 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
count = min((size_t)(32 * 1024), count);
- buf = alloc_buf(port->portdev->vdev, count, 0);
+ buf = alloc_buf(port->portdev->vdev, count, 0, GFP_KERNEL);
if (!buf)
return -ENOMEM;
ret = copy_from_user(buf->buf, ubuf, count);
if (ret) {
- ret = -EFAULT;
- goto free_buf;
+ free_buf(buf, true);
+ return -EFAULT;
}
/*
@@ -835,15 +843,7 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
*/
nonblock = true;
sg_init_one(sg, buf->buf, count);
- ret = __send_to_port(port, sg, 1, count, buf, nonblock);
-
- if (nonblock && ret > 0)
- goto out;
-
-free_buf:
- free_buf(buf, true);
-out:
- return ret;
+ return __send_to_port(port, sg, 1, count, buf, nonblock);
}
struct sg_list {
@@ -932,7 +932,7 @@ static ssize_t port_fops_splice_write(struct pipe_inode_info *pipe,
goto error_out;
occupancy = pipe_buf_usage(pipe);
- buf = alloc_buf(port->portdev->vdev, 0, occupancy);
+ buf = alloc_buf(port->portdev->vdev, 0, occupancy, GFP_KERNEL);
if (!buf) {
ret = -ENOMEM;
@@ -946,11 +946,12 @@ static ssize_t port_fops_splice_write(struct pipe_inode_info *pipe,
sg_init_table(sgl.sg, sgl.size);
ret = __splice_from_pipe(pipe, &sd, pipe_to_sg);
pipe_unlock(pipe);
+
if (likely(ret > 0))
ret = __send_to_port(port, buf->sg, sgl.n, sgl.len, buf, true);
-
- if (unlikely(ret <= 0))
+ else
free_buf(buf, true);
+
return ret;
error_out:
@@ -1108,21 +1109,25 @@ static ssize_t put_chars(u32 vtermno, const u8 *buf, size_t count)
{
struct port *port;
struct scatterlist sg[1];
- void *data;
- int ret;
+ struct port_buffer *pbuf;
port = find_port_by_vtermno(vtermno);
if (!port)
return -EPIPE;
- data = kmemdup(buf, count, GFP_ATOMIC);
- if (!data)
+ pbuf = alloc_buf(port->portdev->vdev, count, 0, GFP_ATOMIC);
+ if (!pbuf)
return -ENOMEM;
- sg_init_one(sg, data, count);
- ret = __send_to_port(port, sg, 1, count, data, false);
- kfree(data);
- return ret;
+ memcpy(pbuf->buf, buf, count);
+ pbuf->len = count;
+ sg_init_one(sg, pbuf->buf, count);
+
+ /*
+ * Ownership of pbuf is transferred to __send_to_port().
+ * Do not touch or free pbuf after this call.
+ */
+ return __send_to_port(port, sg, 1, count, pbuf, false);
}
/*
@@ -1295,7 +1300,7 @@ static int fill_queue(struct virtqueue *vq, spinlock_t *lock)
nr_added_bufs = 0;
do {
- buf = alloc_buf(vq->vdev, PAGE_SIZE, 0);
+ buf = alloc_buf(vq->vdev, PAGE_SIZE, 0, GFP_KERNEL);
if (!buf)
return -ENOMEM;
--
2.34.1
^ permalink raw reply related
* [PATCH v3 0/4] virtio_console: fix suspend/resume and hot-unplug races
From: Sungho Bae @ 2026-06-03 18:37 UTC (permalink / raw)
To: amit, arnd, gregkh; +Cc: virtualization, linux-kernel, Sungho Bae
Hi all,
This patchset addresses several critical race conditions and memory
lifecycle bugs in the virtio_console driver, primarily surrounding
the TX path(`__send_to_port`, `put_chars`), device hot-unplug, and
PM freeze/restore.
The initial motivation for this series was to fix a race where
`hvc_console` writes (which can remain active during suspend
if `no_console_suspend` is enabled) could enqueue buffers while
`virtcons_freeze` was tearing down virtqueues, leading to a `BUG_ON` in
`virtqueue_detach_unused_buf_split()`.
During the investigation and testing of the freeze race, several related
vulnerabilities were uncovered in how TX paths handled buffer ownership and
stale `portdev` pointers during concurrent hot-unplugs. Because these fixes
structurally overlap in `__send_to_port()` and `put_chars()`, they are
submitted together as a single patchset to avoid merge conflicts.
Patch Summary
=============
1. virtio_console: refactor __send_to_port() buffer ownership
Refactors buffer ownership in `__send_to_port()`. Previously,
`put_chars()` would allocate a raw buffer, pass it down, and `kfree()`
it immediately upon return. If `virtqueue_get_buf()` returned an older
completed buffer from a previous non-blocking write, the newly queued
buffer could be freed while the host was still DMA-ing from it.
By transferring ownership of a `struct port_buffer` to
`__send_to_port()`, we guarantee only the exact buffer returned by
the host is freed.
2. virtio_console: fix hot-unplug races in TX paths
Hardens the TX paths against concurrent hot-unplugs. It adds
`READ_ONCE(port->portdev)` checks, bails out cleanly (returning `count`
to prevent `hvc` infinite retries), and synchronizes `portdev = NULL`
in `unplug_port()` using the `outvq_lock`.
3. virtio_console: fix control queue race during restore
Fixes a race during `virtcons_restore()` where filling the control
receive queue (c_ivq) immediately after `DRIVER_OK` could trigger the
control workqueue before the driver finished restoring port states,
leading to list corruption.
4. virtio_console: fix race between hvc put_chars and virtqueue teardown
Addresses the original PM freeze race by introducing a `pm_freezing`
state. TX paths now safely drop output while freezing.
A synchronization loop in `virtcons_freeze()` ensures all active TX
threads have drained before `virtio_reset_device()` is called.
Testing
=======
Runtime-tested on arm64 systems with `no_console_suspend` enabled
during S4 cycles.
Changes
=======
v3:
Split the original monolithic patch into a 4-part patchset for better
logical grouping and bisectability.
Sungho Bae (4):
virtio_console: refactor __send_to_port() buffer ownership
virtio_console: fix hot-unplug races in TX paths
virtio_console: fix control queue race during restore
virtio_console: fix race between hvc put_chars and virtqueue teardown
on freeze
drivers/char/virtio_console.c | 177 ++++++++++++++++++++++++++--------
1 file changed, 137 insertions(+), 40 deletions(-)
--
2.34.1
^ permalink raw reply
* Re: [PATCH v6 5/7] locking: Add contended_release tracepoint to qspinlock
From: Peter Zijlstra @ 2026-06-03 14:26 UTC (permalink / raw)
To: Dmitry Ilvokhin
Cc: Ingo Molnar, Will Deacon, Boqun Feng, Waiman Long,
Thomas Bogendoerfer, Juergen Gross, Ajay Kaher, Alexey Makhalov,
Broadcom internal kernel review list, Thomas Gleixner,
Borislav Petkov, Dave Hansen, x86, H. Peter Anvin, Arnd Bergmann,
Dennis Zhou, Tejun Heo, Christoph Lameter, Steven Rostedt,
Masami Hiramatsu, Mathieu Desnoyers, linux-kernel, linux-mips,
virtualization, linux-arch, linux-mm, linux-trace-kernel,
kernel-team, Paul E. McKenney
In-Reply-To: <aiA3dzjKVq2_cpSY@shell.ilvokhin.com>
On Wed, Jun 03, 2026 at 02:17:27PM +0000, Dmitry Ilvokhin wrote:
> > Something a little like so, which is completely untested, except to
> > build kernel/locking/spinlock.o (with clang-23).
>
> Thanks a lot for taking a look, Peter.
>
> I like the static_call idea. It's truly zero cost on x86 (and, as you
> note, even a byte smaller). The one caveat is that it relies on
> HAVE_STATIC_CALL_INLINE to stay free.
>
> So my plan would be: static_call where HAVE_STATIC_CALL_INLINE is
> available (x86), and a static branch fallback elsewhere, gated behind a
> default-off config so it imposes nothing on arches/kernels that don't
> opt in. I'm mostly interested in x86, but would like arm64 to work too,
> which would use the fallback.
(i386 doesn't have STATIC_CALL_INLINE, but nobody cares about the
performance on that target, so anything goes really ;-)
>
> Concretely:
>
> 1. Split the sleepable-lock patches out and send them separately.
> They're independent of the static call work and look far less
> controversial.
>
> 2. Convert the paravirt spinlock unlock to a static_call, as the
> foundation for the unlock tracepoint. I'm happy to take a stab at it.
> Let me know if you'd rather do it yourself.
Yeah, I think that patch as-is *should* work, but like said, I haven't
even tried it, so it could be terribly broken :-)
> 3. Build the unlock tracepoint on top: static_call where it's cheap,
> config-gated static_branch fallback where it isn't.
Right, so I think we need some sort of custom callback for tracepoint
enable/disable. Its been a minute since I dug through the tracepoint
code, but I don't think it provides that with a convenient wrapper, but
it should be doable.
One thing to note is that when you set the tracepoint unlock function,
it should either tail-call into the original function, or you have to
create two unlock_trace functions, one for native and one for paravirt
and pick the right one.
> Does this plan sound reasonable to you?
Yeah, should work.
> > Also, I think someone should go do some performance runs with
> > ARCH_INLINE_SPIN_* set for x86 just like for s390.
>
> That's a good point, I'll run benchmarks and report back with the
> results.
Thanks!
^ permalink raw reply
* Re: [PATCH v6 5/7] locking: Add contended_release tracepoint to qspinlock
From: Dmitry Ilvokhin @ 2026-06-03 14:17 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Ingo Molnar, Will Deacon, Boqun Feng, Waiman Long,
Thomas Bogendoerfer, Juergen Gross, Ajay Kaher, Alexey Makhalov,
Broadcom internal kernel review list, Thomas Gleixner,
Borislav Petkov, Dave Hansen, x86, H. Peter Anvin, Arnd Bergmann,
Dennis Zhou, Tejun Heo, Christoph Lameter, Steven Rostedt,
Masami Hiramatsu, Mathieu Desnoyers, linux-kernel, linux-mips,
virtualization, linux-arch, linux-mm, linux-trace-kernel,
kernel-team, Paul E. McKenney
In-Reply-To: <20260603120811.GW3493090@noisy.programming.kicks-ass.net>
On Wed, Jun 03, 2026 at 02:08:11PM +0200, Peter Zijlstra wrote:
> On Thu, May 14, 2026 at 12:34:55PM +0000, Dmitry Ilvokhin wrote:
>
> > Baseline, in best case scenario of least number of executed
> > instructions.
> >
> > 3e0: endbr64 ; 4 bytes (always executed)
> > 3e4: movb $0x0,(%rdi) ; 3 bytes (unlock,
> > ; always executed)
> > 3e7: decl %gs:__preempt_count ; 7 bytes (always executed)
> > 3ee: je 3f5 ; 2 bytes (always executed)
> > 3f0: jmp __x86_return_thunk ; 5 bytes (executed if above
> > ; je is not taken)
> > ; rest is not executed
> > 3f5: call __SCT__preempt_schedule ; 5 bytes
> > 3fa: jmp __x86_return_thunk ; 5 bytes
> >
> > Tracepoint (again same case of least number of executed instructions).
> >
> > bc0: endbr64 ; 4 bytes (always executed)
> > bc4: xchg %ax,%ax ; 2 bytes (always executed, this is an
> > ; only addition on the execution path).
> > bc6: movb $0x0,(%rdi) ; 3 bytes (unlock, always executed)
> > bc9: decl %gs:__preempt_count ; 7 bytes (always executed)
> > bd0: je bde ; 2 bytes (always executed)
> > bd2: jmp __x86_return_thunk ; 5 bytes (executed if above
> > ; je is not taken)
> > ; rest is not executed
> > bd7: call queued_spin_release_traced ; 5 bytes
> > bdc: jmp bc9 ; 2 bytes
> > bde: call __SCT__preempt_schedule ; 5 bytes
> > be3: jmp __x86_return_thunk ; 5 bytes
> >
>
> So I've been playing with this a bit, and it is all really sad.
>
> Now, since pretty much everybody+dog will have PARAVIRT_SPINLOCK=y, the
> 'best' solution would be changing that paravirt call with a
> static_call(), that actually shrinks the code by 1 byte.
>
> And then this tracepoint nonsense can simply use a different unlock
> function, just like paravirt.
>
> 0000 00000000000001d0 <_raw_spin_unlock>:
> 0000 1d0: f3 0f 1e fa endbr64
> 0004 1d4: ff 15 00 00 00 00 call *0x0(%rip) # 1da <_raw_spin_unlock+0xa> 1d6: R_X86_64_PC32 pv_ops_lock+0x4
> 000a 1da: 65 ff 0d 00 00 00 00 decl %gs:0x0(%rip) # 1e1 <_raw_spin_unlock+0x11> 1dd: R_X86_64_PC32 __preempt_count-0x4
> 0011 1e1: 74 06 je 1e9 <_raw_spin_unlock+0x19>
> 0013 1e3: 2e e9 00 00 00 00 cs jmp 1e9 <_raw_spin_unlock+0x19> 1e5: R_X86_64_PLT32 __x86_return_thunk-0x4
> 0019 1e9: e8 00 00 00 00 call 1ee <_raw_spin_unlock+0x1e> 1ea: R_X86_64_PLT32 __SCT__preempt_schedule-0x4
> 001e 1ee: 2e e9 00 00 00 00 cs jmp 1f4 <_raw_spin_unlock+0x24> 1f0: R_X86_64_PLT32 __x86_return_thunk-0x4
>
>
> 0000 00000000000001d0 <_raw_spin_unlock>:
> 0000 1d0: f3 0f 1e fa endbr64
> 0004 1d4: e8 00 00 00 00 call 1d9 <_raw_spin_unlock+0x9> 1d5: R_X86_64_PLT32 __SCT__queued_spin_unlock-0x4
> 0009 1d9: 65 ff 0d 00 00 00 00 decl %gs:0x0(%rip) # 1e0 <_raw_spin_unlock+0x10> 1dc: R_X86_64_PC32 __preempt_count-0x4
> 0010 1e0: 74 06 je 1e8 <_raw_spin_unlock+0x18>
> 0012 1e2: 2e e9 00 00 00 00 cs jmp 1e8 <_raw_spin_unlock+0x18> 1e4: R_X86_64_PLT32 __x86_return_thunk-0x4
> 0018 1e8: e8 00 00 00 00 call 1ed <_raw_spin_unlock+0x1d> 1e9: R_X86_64_PLT32 __SCT__preempt_schedule-0x4
> 001d 1ed: 2e e9 00 00 00 00 cs jmp 1f3 <_raw_spin_unlock+0x23> 1ef: R_X86_64_PLT32 __x86_return_thunk-0x4
>
>
> Something a little like so, which is completely untested, except to
> build kernel/locking/spinlock.o (with clang-23).
Thanks a lot for taking a look, Peter.
I like the static_call idea. It's truly zero cost on x86 (and, as you
note, even a byte smaller). The one caveat is that it relies on
HAVE_STATIC_CALL_INLINE to stay free.
So my plan would be: static_call where HAVE_STATIC_CALL_INLINE is
available (x86), and a static branch fallback elsewhere, gated behind a
default-off config so it imposes nothing on arches/kernels that don't
opt in. I'm mostly interested in x86, but would like arm64 to work too,
which would use the fallback.
Concretely:
1. Split the sleepable-lock patches out and send them separately.
They're independent of the static call work and look far less
controversial.
2. Convert the paravirt spinlock unlock to a static_call, as the
foundation for the unlock tracepoint. I'm happy to take a stab at it.
Let me know if you'd rather do it yourself.
3. Build the unlock tracepoint on top: static_call where it's cheap,
config-gated static_branch fallback where it isn't.
Does this plan sound reasonable to you?
>
> Also, I think someone should go do some performance runs with
> ARCH_INLINE_SPIN_* set for x86 just like for s390.
That's a good point, I'll run benchmarks and report back with the
results.
^ permalink raw reply
* Re: [PATCH v6 5/7] locking: Add contended_release tracepoint to qspinlock
From: Peter Zijlstra @ 2026-06-03 12:08 UTC (permalink / raw)
To: Dmitry Ilvokhin
Cc: Ingo Molnar, Will Deacon, Boqun Feng, Waiman Long,
Thomas Bogendoerfer, Juergen Gross, Ajay Kaher, Alexey Makhalov,
Broadcom internal kernel review list, Thomas Gleixner,
Borislav Petkov, Dave Hansen, x86, H. Peter Anvin, Arnd Bergmann,
Dennis Zhou, Tejun Heo, Christoph Lameter, Steven Rostedt,
Masami Hiramatsu, Mathieu Desnoyers, linux-kernel, linux-mips,
virtualization, linux-arch, linux-mm, linux-trace-kernel,
kernel-team, Paul E. McKenney
In-Reply-To: <agXBb0ga_6HJrrnm@shell.ilvokhin.com>
On Thu, May 14, 2026 at 12:34:55PM +0000, Dmitry Ilvokhin wrote:
> Baseline, in best case scenario of least number of executed
> instructions.
>
> 3e0: endbr64 ; 4 bytes (always executed)
> 3e4: movb $0x0,(%rdi) ; 3 bytes (unlock,
> ; always executed)
> 3e7: decl %gs:__preempt_count ; 7 bytes (always executed)
> 3ee: je 3f5 ; 2 bytes (always executed)
> 3f0: jmp __x86_return_thunk ; 5 bytes (executed if above
> ; je is not taken)
> ; rest is not executed
> 3f5: call __SCT__preempt_schedule ; 5 bytes
> 3fa: jmp __x86_return_thunk ; 5 bytes
>
> Tracepoint (again same case of least number of executed instructions).
>
> bc0: endbr64 ; 4 bytes (always executed)
> bc4: xchg %ax,%ax ; 2 bytes (always executed, this is an
> ; only addition on the execution path).
> bc6: movb $0x0,(%rdi) ; 3 bytes (unlock, always executed)
> bc9: decl %gs:__preempt_count ; 7 bytes (always executed)
> bd0: je bde ; 2 bytes (always executed)
> bd2: jmp __x86_return_thunk ; 5 bytes (executed if above
> ; je is not taken)
> ; rest is not executed
> bd7: call queued_spin_release_traced ; 5 bytes
> bdc: jmp bc9 ; 2 bytes
> bde: call __SCT__preempt_schedule ; 5 bytes
> be3: jmp __x86_return_thunk ; 5 bytes
>
So I've been playing with this a bit, and it is all really sad.
Now, since pretty much everybody+dog will have PARAVIRT_SPINLOCK=y, the
'best' solution would be changing that paravirt call with a
static_call(), that actually shrinks the code by 1 byte.
And then this tracepoint nonsense can simply use a different unlock
function, just like paravirt.
0000 00000000000001d0 <_raw_spin_unlock>:
0000 1d0: f3 0f 1e fa endbr64
0004 1d4: ff 15 00 00 00 00 call *0x0(%rip) # 1da <_raw_spin_unlock+0xa> 1d6: R_X86_64_PC32 pv_ops_lock+0x4
000a 1da: 65 ff 0d 00 00 00 00 decl %gs:0x0(%rip) # 1e1 <_raw_spin_unlock+0x11> 1dd: R_X86_64_PC32 __preempt_count-0x4
0011 1e1: 74 06 je 1e9 <_raw_spin_unlock+0x19>
0013 1e3: 2e e9 00 00 00 00 cs jmp 1e9 <_raw_spin_unlock+0x19> 1e5: R_X86_64_PLT32 __x86_return_thunk-0x4
0019 1e9: e8 00 00 00 00 call 1ee <_raw_spin_unlock+0x1e> 1ea: R_X86_64_PLT32 __SCT__preempt_schedule-0x4
001e 1ee: 2e e9 00 00 00 00 cs jmp 1f4 <_raw_spin_unlock+0x24> 1f0: R_X86_64_PLT32 __x86_return_thunk-0x4
0000 00000000000001d0 <_raw_spin_unlock>:
0000 1d0: f3 0f 1e fa endbr64
0004 1d4: e8 00 00 00 00 call 1d9 <_raw_spin_unlock+0x9> 1d5: R_X86_64_PLT32 __SCT__queued_spin_unlock-0x4
0009 1d9: 65 ff 0d 00 00 00 00 decl %gs:0x0(%rip) # 1e0 <_raw_spin_unlock+0x10> 1dc: R_X86_64_PC32 __preempt_count-0x4
0010 1e0: 74 06 je 1e8 <_raw_spin_unlock+0x18>
0012 1e2: 2e e9 00 00 00 00 cs jmp 1e8 <_raw_spin_unlock+0x18> 1e4: R_X86_64_PLT32 __x86_return_thunk-0x4
0018 1e8: e8 00 00 00 00 call 1ed <_raw_spin_unlock+0x1d> 1e9: R_X86_64_PLT32 __SCT__preempt_schedule-0x4
001d 1ed: 2e e9 00 00 00 00 cs jmp 1f3 <_raw_spin_unlock+0x23> 1ef: R_X86_64_PLT32 __x86_return_thunk-0x4
Something a little like so, which is completely untested, except to
build kernel/locking/spinlock.o (with clang-23).
Also, I think someone should go do some performance runs with
ARCH_INLINE_SPIN_* set for x86 just like for s390.
Of course, this is only x86 done, and it doesn't nicely tie in with
tracepoints, but it does give the sanest asm.
---
diff --git a/arch/x86/hyperv/hv_spinlock.c b/arch/x86/hyperv/hv_spinlock.c
index 210b494e4de0..6b4bdea18218 100644
--- a/arch/x86/hyperv/hv_spinlock.c
+++ b/arch/x86/hyperv/hv_spinlock.c
@@ -78,8 +78,8 @@ void __init hv_init_spinlocks(void)
pr_info("PV spinlocks enabled\n");
__pv_init_lock_hash();
- pv_ops_lock.queued_spin_lock_slowpath = __pv_queued_spin_lock_slowpath;
- pv_ops_lock.queued_spin_unlock = PV_CALLEE_SAVE(__pv_queued_spin_unlock);
+ static_call_update(queued_spin_lock_slowpath, __pv_queued_spin_lock_slowpath);
+ static_call_update(queued_spin_unlock, __raw_callee_save___pv_queued_spin_unlock);
pv_ops_lock.wait = hv_qlock_wait;
pv_ops_lock.kick = hv_qlock_kick;
pv_ops_lock.vcpu_is_preempted = PV_CALLEE_SAVE(hv_vcpu_is_preempted);
diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 1d506e5d6f46..52e7ce10df57 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -225,7 +225,6 @@
#define X86_FEATURE_EPT_AD ( 8*32+17) /* "ept_ad" Intel Extended Page Table access-dirty bit */
#define X86_FEATURE_VMCALL ( 8*32+18) /* Hypervisor supports the VMCALL instruction */
#define X86_FEATURE_VMW_VMMCALL ( 8*32+19) /* VMware prefers VMMCALL hypercall instruction */
-#define X86_FEATURE_PVUNLOCK ( 8*32+20) /* PV unlock function */
#define X86_FEATURE_VCPUPREEMPT ( 8*32+21) /* PV vcpu_is_preempted function */
#define X86_FEATURE_TDX_GUEST ( 8*32+22) /* "tdx_guest" Intel Trust Domain Extensions Guest */
diff --git a/arch/x86/include/asm/paravirt-spinlock.h b/arch/x86/include/asm/paravirt-spinlock.h
index 7beffcb08ed6..553910f92906 100644
--- a/arch/x86/include/asm/paravirt-spinlock.h
+++ b/arch/x86/include/asm/paravirt-spinlock.h
@@ -3,6 +3,7 @@
#define _ASM_X86_PARAVIRT_SPINLOCK_H
#include <asm/paravirt_types.h>
+#include <linux/static_call_types.h>
#ifdef CONFIG_SMP
#include <asm/spinlock_types.h>
@@ -11,9 +12,6 @@
struct qspinlock;
struct pv_lock_ops {
- void (*queued_spin_lock_slowpath)(struct qspinlock *lock, u32 val);
- struct paravirt_callee_save queued_spin_unlock;
-
void (*wait)(u8 *ptr, u8 val);
void (*kick)(int cpu);
@@ -26,20 +24,23 @@ extern struct pv_lock_ops pv_ops_lock;
extern void native_queued_spin_lock_slowpath(struct qspinlock *lock, u32 val);
extern void __pv_init_lock_hash(void);
extern void __pv_queued_spin_lock_slowpath(struct qspinlock *lock, u32 val);
+extern void __raw_callee_save___native_queued_spin_unlock(struct qspinlock *lock);
extern void __raw_callee_save___pv_queued_spin_unlock(struct qspinlock *lock);
extern bool nopvspin;
+DECLARE_STATIC_CALL(queued_spin_lock_slowpath, native_queued_spin_lock_slowpath);
+DECLARE_STATIC_CALL(queued_spin_unlock, __raw_callee_save___native_queued_spin_unlock);
+
static __always_inline void pv_queued_spin_lock_slowpath(struct qspinlock *lock,
u32 val)
{
- PVOP_VCALL2(pv_ops_lock, queued_spin_lock_slowpath, lock, val);
+ static_call(queued_spin_lock_slowpath)(lock, val);
}
static __always_inline void pv_queued_spin_unlock(struct qspinlock *lock)
{
- PVOP_ALT_VCALLEE1(pv_ops_lock, queued_spin_unlock, lock,
- "movb $0, (%%" _ASM_ARG1 ")",
- ALT_NOT(X86_FEATURE_PVUNLOCK));
+ __STATIC_CALL_MOD_ADDRESSABLE(queued_spin_unlock);
+ asm volatile ("call " STATIC_CALL_TRAMP_STR(queued_spin_unlock) : ASM_CALL_CONSTRAINT);
}
static __always_inline bool pv_vcpu_is_preempted(long cpu)
diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
index 29226d112029..5908d9fd94bb 100644
--- a/arch/x86/kernel/kvm.c
+++ b/arch/x86/kernel/kvm.c
@@ -1135,9 +1135,8 @@ void __init kvm_spinlock_init(void)
pr_info("PV spinlocks enabled\n");
__pv_init_lock_hash();
- pv_ops_lock.queued_spin_lock_slowpath = __pv_queued_spin_lock_slowpath;
- pv_ops_lock.queued_spin_unlock =
- PV_CALLEE_SAVE(__pv_queued_spin_unlock);
+ static_call_update(queued_spin_lock_slowpath, __pv_queued_spin_lock_slowpath);
+ static_call_update(queued_spin_unlock, __raw_callee_save___pv_queued_spin_unlock);
pv_ops_lock.wait = kvm_wait;
pv_ops_lock.kick = kvm_kick_cpu;
diff --git a/arch/x86/kernel/paravirt-spinlocks.c b/arch/x86/kernel/paravirt-spinlocks.c
index 95452444868f..28407304f123 100644
--- a/arch/x86/kernel/paravirt-spinlocks.c
+++ b/arch/x86/kernel/paravirt-spinlocks.c
@@ -25,9 +25,12 @@ __visible void __native_queued_spin_unlock(struct qspinlock *lock)
}
PV_CALLEE_SAVE_REGS_THUNK(__native_queued_spin_unlock);
+DEFINE_STATIC_CALL(queued_spin_lock_slowpath, native_queued_spin_lock_slowpath);
+DEFINE_STATIC_CALL(queued_spin_unlock, __raw_callee_save___native_queued_spin_unlock);
+
bool pv_is_native_spin_unlock(void)
{
- return pv_ops_lock.queued_spin_unlock.func ==
+ return static_call_query(queued_spin_unlock) ==
__raw_callee_save___native_queued_spin_unlock;
}
@@ -45,16 +48,11 @@ bool pv_is_native_vcpu_is_preempted(void)
void __init paravirt_set_cap(void)
{
- if (!pv_is_native_spin_unlock())
- setup_force_cpu_cap(X86_FEATURE_PVUNLOCK);
-
if (!pv_is_native_vcpu_is_preempted())
setup_force_cpu_cap(X86_FEATURE_VCPUPREEMPT);
}
struct pv_lock_ops pv_ops_lock = {
- .queued_spin_lock_slowpath = native_queued_spin_lock_slowpath,
- .queued_spin_unlock = PV_CALLEE_SAVE(__native_queued_spin_unlock),
.wait = paravirt_nop,
.kick = paravirt_nop,
.vcpu_is_preempted = PV_CALLEE_SAVE(__native_vcpu_is_preempted),
diff --git a/arch/x86/kernel/static_call.c b/arch/x86/kernel/static_call.c
index 61592e41a6b1..1268b7dc93b1 100644
--- a/arch/x86/kernel/static_call.c
+++ b/arch/x86/kernel/static_call.c
@@ -3,6 +3,7 @@
#include <linux/memory.h>
#include <linux/bug.h>
#include <asm/text-patching.h>
+#include <asm/paravirt-spinlock.h>
enum insn_type {
CALL = 0, /* site call */
@@ -31,6 +32,15 @@ static const u8 retinsn[] = { RET_INSN_OPCODE, 0xcc, 0xcc, 0xcc, 0xcc };
*/
static const u8 warninsn[] = { 0x67, 0x48, 0x0f, 0xb9, 0x3a };
+/*
+ * ds ds movb $0, (_ASM_ARG1)
+ */
+#ifdef CONFIG_64BIT
+static const u8 unlockinsn[] = { 0x3e, 0x3e, 0xc6, 0x07, 0x00 };
+#else
+static const u8 unlockinsn[] = { 0x3e, 0x3e, 0xc6, 0x00, 0x00 };
+#endif
+
static u8 __is_Jcc(u8 *insn) /* Jcc.d32 */
{
u8 ret = 0;
@@ -78,6 +88,10 @@ static void __ref __static_call_transform(void *insn, enum insn_type type,
emulate = code;
code = &warninsn;
}
+ if (func == &__raw_callee_save___native_queued_spin_unlock) {
+ emulate = code;
+ code = &unlockinsn;
+ }
break;
case NOP:
diff --git a/arch/x86/xen/spinlock.c b/arch/x86/xen/spinlock.c
index 83ac24ead289..f718e535ea7c 100644
--- a/arch/x86/xen/spinlock.c
+++ b/arch/x86/xen/spinlock.c
@@ -134,9 +134,8 @@ void __init xen_init_spinlocks(void)
printk(KERN_DEBUG "xen: PV spinlocks enabled\n");
__pv_init_lock_hash();
- pv_ops_lock.queued_spin_lock_slowpath = __pv_queued_spin_lock_slowpath;
- pv_ops_lock.queued_spin_unlock =
- PV_CALLEE_SAVE(__pv_queued_spin_unlock);
+ static_call_update(queued_spin_lock_slowpath, __pv_queued_spin_lock_slowpath);
+ static_call_update(queued_spin_unlock, __raw_callee_save___pv_queued_spin_unlock);
pv_ops_lock.wait = xen_qlock_wait;
pv_ops_lock.kick = xen_qlock_kick;
pv_ops_lock.vcpu_is_preempted = PV_CALLEE_SAVE(xen_vcpu_stolen);
diff --git a/tools/arch/x86/include/asm/cpufeatures.h b/tools/arch/x86/include/asm/cpufeatures.h
index 86d17b195e79..61541f042f74 100644
--- a/tools/arch/x86/include/asm/cpufeatures.h
+++ b/tools/arch/x86/include/asm/cpufeatures.h
@@ -225,7 +225,6 @@
#define X86_FEATURE_EPT_AD ( 8*32+17) /* "ept_ad" Intel Extended Page Table access-dirty bit */
#define X86_FEATURE_VMCALL ( 8*32+18) /* Hypervisor supports the VMCALL instruction */
#define X86_FEATURE_VMW_VMMCALL ( 8*32+19) /* VMware prefers VMMCALL hypercall instruction */
-#define X86_FEATURE_PVUNLOCK ( 8*32+20) /* PV unlock function */
#define X86_FEATURE_VCPUPREEMPT ( 8*32+21) /* PV vcpu_is_preempted function */
#define X86_FEATURE_TDX_GUEST ( 8*32+22) /* "tdx_guest" Intel Trust Domain Extensions Guest */
^ permalink raw reply related
* Re: [PATCH v4 07/47] x86/tdx: Force TSC frequency with CPUID-based info provided by the TDX-Module
From: Kiryl Shutsemau @ 2026-06-03 10:02 UTC (permalink / raw)
To: Sean Christopherson
Cc: Paolo Bonzini, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, x86, K. Y. Srinivasan, Haiyang Zhang, Wei Liu,
Dexuan Cui, Long Li, Ajay Kaher, Alexey Makhalov, Jan Kiszka,
Andy Lutomirski, Peter Zijlstra, Juergen Gross, Daniel Lezcano,
John Stultz, H. Peter Anvin, Rick Edgecombe, Vitaly Kuznetsov,
Broadcom internal kernel review list, Boris Ostrovsky,
Stephen Boyd, kvm, linux-kernel, linux-coco, linux-hyperv,
virtualization, xen-devel, David Woodhouse, Tom Lendacky,
Nikunj A Dadhania, David Woodhouse, Michael Kelley,
Thomas Gleixner
In-Reply-To: <20260529144435.704127-8-seanjc@google.com>
On Fri, May 29, 2026 at 07:43:54AM -0700, Sean Christopherson wrote:
> When running as a TDX guest, explicitly set the TSC frequency to a known
> value, using CPUID-based information, instead of potentially relying on a
> hypervisor-controlled PV routine. For TDX guests, CPUID.0x15 is always
> emulated by the TDX-Module, i.e. the information from CPUID is more
> trustworthy than the information provided by the hypervisor.
Right. EBX is configurable by TD_PARAMS.TSC_FREQUENCY at TD build. The
rest is fixed.
> To maintain backwards compatibility with TDX guest kernels that use native
> calibration, and because it's the least awful option, retain
> native_calibrate_tsc()'s stuffing of the local APIC bus period using the
> core crystal frequency. While it's entirely possible for the hypervisor
> to emulate the APIC timer at a different frequency than the core crystal
> frequency, the commonly accepted interpretation of Intel's SDM is that APIC
> timer runs at the core crystal frequency when that latter is enumerated via
> CPUID:
>
> The APIC timer frequency will be the processor’s bus clock or core
> crystal clock frequency (when TSC/core crystal clock ratio is enumerated
> in CPUID leaf 0x15).
>
> If the hypervisor is malicious and deliberately runs the APIC timer at the
> wrong frequency, nothing would stop the hypervisor from modifying the
> frequency at any time, i.e. attempting to manually calibrate the frequency
> out of paranoia would be futile.
Agreed.
> Deliberately leave CPU frequency calibration as is, since the TDX-Module
> doesn't provide any guarantees with respect to CPUID.0x16.
It is fixed to zeros. Sounds like a guarantee to me :P
> Signed-off-by: Sean Christopherson <seanjc@google.com>
Looks sane to me. Including your reasoning about tsc_early_khz= in reply
to Sashiko.
Reviewed-by: Kiryl Shutsemau (Meta) <kas@kernel.org>
--
Kiryl Shutsemau / Kirill A. Shutemov
^ permalink raw reply
* Re: Re: [PATCH] VIRTIO: Update the desc 'flag' fied last in packed ring.
From: Michael S. Tsirkin @ 2026-06-03 5:10 UTC (permalink / raw)
To: yangjiale133
Cc: Eugenio Perez Martin, Jason Wang, Xuan Zhuo, virtualization,
linux-kernel
In-Reply-To: <5a3e06d5.103d.19e8b1863dd.Coremail.yangjiale133@163.com>
On Wed, Jun 03, 2026 at 09:28:11AM +0800, yangjiale133 wrote:
> From the device's perspective, during a single read of the descriptor list
> specifically when that list spans across cache lines.
> the retrieved data will show `desc[head].flags` as valid,
> and `desc[i].flags` as valid as well; however,
> the `desc[i].addr` and `len` fields may be invalid.
>
> I apologize that I currently lack the necessary environment to verify
> whether this modification definitively resolves the issue or
> merely reduces the probability of its occurrence;
> therefore, this patch can be discarded.
>
> yangjiale
it could be that your device does a single read of the descriptor.
the pci spec is explicit that after getting valid flags,
device must read the descriptor again.
>
> At 2026-06-02 14:04:13, "Eugenio Perez Martin" <eperezma@redhat.com> wrote:
> >On Tue, Jun 2, 2026 at 6:34 AM yangjiale <yangjiale133@163.com> wrote:
> >>
> >> When a descriptor list spans across cache lines,
> >> updating the flag first can lead to a scenario where the device side
> >> perceives the flag as valid, yet the corresponding address and length
> >> fields remain unupdated—resulting in invalid values.
> >> Therefore, the flag field must be updated last.
> >>
> >> Signed-off-by: yangjiale <yangjiale133@163.com>
> >> ---
> >> drivers/virtio/virtio_ring.c | 8 ++++----
> >> 1 file changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> >> index fbca7ce1c6bf..036b4f90d30f 100644
> >> --- a/drivers/virtio/virtio_ring.c
> >> +++ b/drivers/virtio/virtio_ring.c
> >> @@ -1688,6 +1688,10 @@ static inline int virtqueue_add_packed(struct vring_virtqueue *vq,
> >> &addr, &len, premapped, attr))
> >> goto unmap_release;
> >>
> >> + desc[i].addr = cpu_to_le64(addr);
> >> + desc[i].len = cpu_to_le32(len);
> >> + desc[i].id = cpu_to_le16(id);
> >> +
> >> flags = cpu_to_le16(vq->packed.avail_used_flags |
> >> (++c == total_sg ? 0 : VRING_DESC_F_NEXT) |
> >> (n < out_sgs ? 0 : VRING_DESC_F_WRITE));
> >> @@ -1696,10 +1700,6 @@ static inline int virtqueue_add_packed(struct vring_virtqueue *vq,
> >> else
> >> desc[i].flags = flags;
> >>
> >> - desc[i].addr = cpu_to_le64(addr);
> >> - desc[i].len = cpu_to_le32(len);
> >> - desc[i].id = cpu_to_le16(id);
> >> -
> >> if (unlikely(vq->use_map_api)) {
> >> vq->packed.desc_extra[curr].addr = premapped ?
> >> DMA_MAPPING_ERROR : addr;
> >
> >These flags are updated before the flags of the head descriptor at the
> >end of the function, at "vq->packed.vring.desc[head].flags =
> >head_flags", so the device should not see these. Because of that, the
> >relative order between the rest of the fields of the same descriptor
> >or other descriptors' fields, except for the head descriptor's flags,
> >should not matter. There is a write memory barrier just before
> >updating the head's flags.
> >
> >Also, I don't get why the cache line matters here. Can you expand? Am
> >I missing something?
>
^ permalink raw reply
* Re:Re: [PATCH] VIRTIO: Update the desc 'flag' fied last in packed ring.
From: Xuan Zhuo @ 2026-06-03 2:08 UTC (permalink / raw)
To: yangjiale133
Cc: Michael S . Tsirkin, Jason Wang, virtualization@lists.linux.dev,
linux-kernel@vger.kernel.org, Eugenio Perez Martin
In-Reply-To: <46a9dc36.1986.19e8b34896c.Coremail.yangjiale133@163.com>
On Wed, 3 Jun 2026 09:58:56 +0800 (CST), "yangjiale133" <yangjiale133@163.com> wrote:
> From the device's perspective, during a single read of the descriptor
> list-and that list spans across cache lines.
> the retrieved data will show `desc[head].flags` as valid,
> and `desc[i].flags` as valid as well; however,
> the `desc[i].addr` and `len` fields may be invalid.
>
> I suspect this occurs because a single DMA operation is internally
> split into multiple parallel read transactions,
> aligned to cache line boundaries.
I am aware of one case. When reading, the device reads multiple descriptors
at once. If the head descriptor's flag is 'avail', it then checks the data of
the subsequent descriptors from that same batch. However, this does not comply
with the specification. The device should only read the subsequent descriptors
after the head descriptor's flag is confirmed to be 'avail'. Although this might
impact performance, it is the correct behavior.
Thanks
>
> I apologize that I currently lack the necessary environment to verify
> whether this modification definitively resolves the issue or
> merely reduces the probability of its occurrence;
> therefore, this patch can be discarded.
>
> yangjiale
>
>
>
> At 2026-06-02 14:04:13, "Eugenio Perez Martin" <eperezma@redhat.com> wrote:
> >On Tue, Jun 2, 2026 at 6:34 AM yangjiale <yangjiale133@163.com> wrote:
> >>
> >> When a descriptor list spans across cache lines,
> >> updating the flag first can lead to a scenario where the device side
> >> perceives the flag as valid, yet the corresponding address and length
> >> fields remain unupdated—resulting in invalid values.
> >> Therefore, the flag field must be updated last.
> >>
> >> Signed-off-by: yangjiale <yangjiale133@163.com>
> >> ---
> >> drivers/virtio/virtio_ring.c | 8 ++++----
> >> 1 file changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> >> index fbca7ce1c6bf..036b4f90d30f 100644
> >> --- a/drivers/virtio/virtio_ring.c
> >> +++ b/drivers/virtio/virtio_ring.c
> >> @@ -1688,6 +1688,10 @@ static inline int virtqueue_add_packed(struct vring_virtqueue *vq,
> >> &addr, &len, premapped, attr))
> >> goto unmap_release;
> >>
> >> + desc[i].addr = cpu_to_le64(addr);
> >> + desc[i].len = cpu_to_le32(len);
> >> + desc[i].id = cpu_to_le16(id);
> >> +
> >> flags = cpu_to_le16(vq->packed.avail_used_flags |
> >> (++c == total_sg ? 0 : VRING_DESC_F_NEXT) |
> >> (n < out_sgs ? 0 : VRING_DESC_F_WRITE));
> >> @@ -1696,10 +1700,6 @@ static inline int virtqueue_add_packed(struct vring_virtqueue *vq,
> >> else
> >> desc[i].flags = flags;
> >>
> >> - desc[i].addr = cpu_to_le64(addr);
> >> - desc[i].len = cpu_to_le32(len);
> >> - desc[i].id = cpu_to_le16(id);
> >> -
> >> if (unlikely(vq->use_map_api)) {
> >> vq->packed.desc_extra[curr].addr = premapped ?
> >> DMA_MAPPING_ERROR : addr;
> >
> >These flags are updated before the flags of the head descriptor at the
> >end of the function, at "vq->packed.vring.desc[head].flags =
> >head_flags", so the device should not see these. Because of that, the
> >relative order between the rest of the fields of the same descriptor
> >or other descriptors' fields, except for the head descriptor's flags,
> >should not matter. There is a write memory barrier just before
> >updating the head's flags.
> >
> >Also, I don't get why the cache line matters here. Can you expand? Am
> >I missing something?
>
^ permalink raw reply
* Re:Re: [PATCH] VIRTIO: Update the desc 'flag' fied last in packed ring.
From: yangjiale133 @ 2026-06-03 1:58 UTC (permalink / raw)
To: Eugenio Perez Martin
Cc: Michael S . Tsirkin, Jason Wang, Xuan Zhuo,
virtualization@lists.linux.dev, linux-kernel@vger.kernel.org
In-Reply-To: <CAJaqyWfugEttrQuR5_LrbQaivAqCaixsprUOjEVtdiz_sexFpA@mail.gmail.com>
From the device's perspective, during a single read of the descriptor
list-and that list spans across cache lines.
the retrieved data will show `desc[head].flags` as valid,
and `desc[i].flags` as valid as well; however,
the `desc[i].addr` and `len` fields may be invalid.
I suspect this occurs because a single DMA operation is internally
split into multiple parallel read transactions,
aligned to cache line boundaries.
I apologize that I currently lack the necessary environment to verify
whether this modification definitively resolves the issue or
merely reduces the probability of its occurrence;
therefore, this patch can be discarded.
yangjiale
At 2026-06-02 14:04:13, "Eugenio Perez Martin" <eperezma@redhat.com> wrote:
>On Tue, Jun 2, 2026 at 6:34 AM yangjiale <yangjiale133@163.com> wrote:
>>
>> When a descriptor list spans across cache lines,
>> updating the flag first can lead to a scenario where the device side
>> perceives the flag as valid, yet the corresponding address and length
>> fields remain unupdated—resulting in invalid values.
>> Therefore, the flag field must be updated last.
>>
>> Signed-off-by: yangjiale <yangjiale133@163.com>
>> ---
>> drivers/virtio/virtio_ring.c | 8 ++++----
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
>> index fbca7ce1c6bf..036b4f90d30f 100644
>> --- a/drivers/virtio/virtio_ring.c
>> +++ b/drivers/virtio/virtio_ring.c
>> @@ -1688,6 +1688,10 @@ static inline int virtqueue_add_packed(struct vring_virtqueue *vq,
>> &addr, &len, premapped, attr))
>> goto unmap_release;
>>
>> + desc[i].addr = cpu_to_le64(addr);
>> + desc[i].len = cpu_to_le32(len);
>> + desc[i].id = cpu_to_le16(id);
>> +
>> flags = cpu_to_le16(vq->packed.avail_used_flags |
>> (++c == total_sg ? 0 : VRING_DESC_F_NEXT) |
>> (n < out_sgs ? 0 : VRING_DESC_F_WRITE));
>> @@ -1696,10 +1700,6 @@ static inline int virtqueue_add_packed(struct vring_virtqueue *vq,
>> else
>> desc[i].flags = flags;
>>
>> - desc[i].addr = cpu_to_le64(addr);
>> - desc[i].len = cpu_to_le32(len);
>> - desc[i].id = cpu_to_le16(id);
>> -
>> if (unlikely(vq->use_map_api)) {
>> vq->packed.desc_extra[curr].addr = premapped ?
>> DMA_MAPPING_ERROR : addr;
>
>These flags are updated before the flags of the head descriptor at the
>end of the function, at "vq->packed.vring.desc[head].flags =
>head_flags", so the device should not see these. Because of that, the
>relative order between the rest of the fields of the same descriptor
>or other descriptors' fields, except for the head descriptor's flags,
>should not matter. There is a write memory barrier just before
>updating the head's flags.
>
>Also, I don't get why the cache line matters here. Can you expand? Am
>I missing something?
^ permalink raw reply
* Re: [PATCH v3] drm/virtio: abort virtqueue wait on device removal to avoid hung task
From: Dmitry Osipenko @ 2026-06-03 1:23 UTC (permalink / raw)
To: Ryosuke Yasuoka, David Airlie, Gerd Hoffmann, Gurchetan Singh,
Chia-I Wu, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
Simona Vetter
Cc: dri-devel, virtualization, linux-kernel
In-Reply-To: <20260601-virtio-gpu_wait_event-v3-1-89530517a98a@redhat.com>
On 6/1/26 10:53, Ryosuke Yasuoka wrote:
> virtio_gpu_queue_ctrl_sgs() and virtio_gpu_queue_cursor() use
> wait_event() without any abort condition when waiting for virtqueue
> space. If the host device stops processing commands, these waits block
> indefinitely inside a drm_dev_enter/exit() critical section. Since
> drm_dev_unplug(), which is called in device removal and system shutdown
> call path, blocks on synchronize_srcu() until all critical sections
> complete, device removal and system shutdown also hang.
>
> Add a vqs_released flag to virtio_gpu_device and include it in the
> wait_event() condition. Set the flag and wake up both queues in a new
> virtio_gpu_release_vqs() helper, called before drm_dev_unplug() in both
> virtio_gpu_remove() and virtio_gpu_shutdown(). When the flag is set, the
> wait returns immediately and the command is aborted, following the same
> cleanup path as drm_dev_enter() failure.
>
> Signed-off-by: Ryosuke Yasuoka <ryasuoka@redhat.com>
> ---
> Changes in v3:
> - Remove Reported-by and Closes tag from commit msg because they are not
> related to this fix.
> - Link to v2: https://lore.kernel.org/r/20260521-virtio-gpu_wait_event-v2-1-5796b3a71d03@redhat.com
>
> Changes in v2:
> - Update the commit message.
> - Replace wait_event_timeout() with wait_event() using a compound
> condition that includes a new vqs_released flag.
> - Add virtio_gpu_release_vqs() helper to set the flag and wake up
> both queues, called before drm_dev_unplug() in remove and shutdown
> paths.
> - Remove the hardcoded 5-second timeout. Recovery is now driven by
> the driver flag instead of an arbitrary timeout value.
> ---
> drivers/gpu/drm/virtio/virtgpu_drv.c | 15 +++++++++++++++
> drivers/gpu/drm/virtio/virtgpu_drv.h | 1 +
> drivers/gpu/drm/virtio/virtgpu_vq.c | 23 +++++++++++++++++++++--
> 3 files changed, 37 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c
> index a5ce96fb8a1d..e4fe5e0780f9 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
> @@ -119,10 +119,24 @@ static int virtio_gpu_probe(struct virtio_device *vdev)
> return ret;
> }
>
> +/*
> + * Release pending virtqueue waits so the drm_dev_enter/exit() critical
> + * sections complete before drm_dev_unplug() blocks on synchronize_srcu().
> + */
> +static void virtio_gpu_release_vqs(struct drm_device *dev)
> +{
> + struct virtio_gpu_device *vgdev = dev->dev_private;
> +
> + vgdev->vqs_released = true;
> + wake_up_all(&vgdev->ctrlq.ack_queue);
> + wake_up_all(&vgdev->cursorq.ack_queue);
> +}
> +
> static void virtio_gpu_remove(struct virtio_device *vdev)
> {
> struct drm_device *dev = vdev->priv;
>
> + virtio_gpu_release_vqs(dev);
> drm_dev_unplug(dev);
> drm_atomic_helper_shutdown(dev);
> virtio_gpu_deinit(dev);
> @@ -133,6 +147,7 @@ static void virtio_gpu_shutdown(struct virtio_device *vdev)
> {
> struct drm_device *dev = vdev->priv;
>
> + virtio_gpu_release_vqs(dev);
> /* stop talking to the device */
> drm_dev_unplug(dev);
> }
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
> index 2f3531950aa4..5f7bb6cc6ba7 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.h
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
> @@ -235,6 +235,7 @@ struct virtio_gpu_device {
>
> struct virtio_gpu_queue ctrlq;
> struct virtio_gpu_queue cursorq;
> + bool vqs_released;
> struct kmem_cache *vbufs;
>
> atomic_t pending_commands;
> diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
> index 67865810a2e7..8057a9b7356d 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_vq.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
> @@ -396,7 +396,19 @@ static int virtio_gpu_queue_ctrl_sgs(struct virtio_gpu_device *vgdev,
> if (vq->num_free < elemcnt) {
> spin_unlock(&vgdev->ctrlq.qlock);
> virtio_gpu_notify(vgdev);
> - wait_event(vgdev->ctrlq.ack_queue, vq->num_free >= elemcnt);
> + wait_event(vgdev->ctrlq.ack_queue,
> + vq->num_free >= elemcnt || vgdev->vqs_released);
> + /*
> + * Set by virtio_gpu_release_vqs() to unblock
> + * synchronize_srcu() wait in drm_dev_unplug().
> + */
> + if (vgdev->vqs_released) {
> + if (fence && vbuf->objs)
> + virtio_gpu_array_unlock_resv(vbuf->objs);
> + free_vbuf(vgdev, vbuf);
> + drm_dev_exit(idx);
> + return -ENODEV;
> + }
> goto again;
> }
>
> @@ -566,7 +578,14 @@ static void virtio_gpu_queue_cursor(struct virtio_gpu_device *vgdev,
> ret = virtqueue_add_sgs(vq, sgs, outcnt, 0, vbuf, GFP_ATOMIC);
> if (ret == -ENOSPC) {
> spin_unlock(&vgdev->cursorq.qlock);
> - wait_event(vgdev->cursorq.ack_queue, vq->num_free >= outcnt);
> + wait_event(vgdev->cursorq.ack_queue,
> + vq->num_free >= outcnt || vgdev->vqs_released);
> + /* See comment in virtio_gpu_queue_ctrl_sgs(). */
> + if (vgdev->vqs_released) {
> + free_vbuf(vgdev, vbuf);
> + drm_dev_exit(idx);
> + return;
> + }
> spin_lock(&vgdev->cursorq.qlock);
> goto retry;
> } else {
>
> ---
> base-commit: e43ffb69e0438cddd72aaa30898b4dc446f664f8
> change-id: 20260518-virtio-gpu_wait_event-5aa060754f12
>
> Best regards,
Applied to misc-next, thanks!
--
Best regards,
Dmitry
^ permalink raw reply
* Re: [PATCH] VIRTIO: Update the desc 'flag' fied last in packed ring.
From: Xuan Zhuo @ 2026-06-03 1:09 UTC (permalink / raw)
To: yangjiale
Cc: Jason Wang, Eugenio Pérez, virtualization, linux-kernel,
yangjiale, Michael S . Tsirkin
In-Reply-To: <20260602043123.10207-1-yangjiale133@163.com>
On Tue, 2 Jun 2026 12:31:23 +0800, yangjiale <yangjiale133@163.com> wrote:
> When a descriptor list spans across cache lines,
> updating the flag first can lead to a scenario where the device side
> perceives the flag as valid, yet the corresponding address and length
> fields remain unupdated—resulting in invalid values.
> Therefore, the flag field must be updated last.
Are you raising this based on your code review, or did you actually encounter an
issue during testing?
I don't think there is a problem here, as all operations are protected by the
head desc flag. Even if cacheline effects cause data to be written prematurely,
it shouldn't be an issue. The device must wait until the head desc flags
are updated before it can start any subsequent work.
Thanks.
>
> Signed-off-by: yangjiale <yangjiale133@163.com>
> ---
> drivers/virtio/virtio_ring.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index fbca7ce1c6bf..036b4f90d30f 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -1688,6 +1688,10 @@ static inline int virtqueue_add_packed(struct vring_virtqueue *vq,
> &addr, &len, premapped, attr))
> goto unmap_release;
>
> + desc[i].addr = cpu_to_le64(addr);
> + desc[i].len = cpu_to_le32(len);
> + desc[i].id = cpu_to_le16(id);
> +
> flags = cpu_to_le16(vq->packed.avail_used_flags |
> (++c == total_sg ? 0 : VRING_DESC_F_NEXT) |
> (n < out_sgs ? 0 : VRING_DESC_F_WRITE));
> @@ -1696,10 +1700,6 @@ static inline int virtqueue_add_packed(struct vring_virtqueue *vq,
> else
> desc[i].flags = flags;
>
> - desc[i].addr = cpu_to_le64(addr);
> - desc[i].len = cpu_to_le32(len);
> - desc[i].id = cpu_to_le16(id);
> -
> if (unlikely(vq->use_map_api)) {
> vq->packed.desc_extra[curr].addr = premapped ?
> DMA_MAPPING_ERROR : addr;
> --
> 2.25.1
>
^ permalink raw reply
* Re: [patch V2 21/25] ALSA: hda/common: Use system_device_crosststamp::sys_systime
From: Takashi Iwai @ 2026-06-02 17:46 UTC (permalink / raw)
To: Thomas Gleixner
Cc: LKML, David Woodhouse, Miroslav Lichvar, John Stultz,
Stephen Boyd, Anna-Maria Behnsen, Frederic Weisbecker,
thomas.weissschuh, Arthur Kiyanovski, Rodolfo Giometti,
Vincent Donnefort, Marc Zyngier, Oliver Upton, kvmarm,
Oliver Upton, Richard Cochran, netdev, Takashi Iwai,
Miri Korenblit, Johannes Berg, Jacob Keller, Tony Nguyen,
Saeed Mahameed, Peter Hilber, Michael S. Tsirkin, virtualization,
linux-wireless, linux-sound, David Woodhouse, Vadim Fedorenko
In-Reply-To: <20260529195557.995298795@kernel.org>
On Fri, 29 May 2026 22:01:13 +0200,
Thomas Gleixner wrote:
>
> From: Thomas Gleixner <tglx@kernel.org>
>
> sys_systime is an alias for sys_realtime. The latter will be removed so
> switch the code over to the new naming scheme.
>
> No functional change.
>
> Signed-off-by: Thomas Gleixner <tglx@kernel.org>
> Tested-by: Arthur Kiyanovski <akiyano@amazon.com>
> Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
Acked-by: Takashi Iwai <tiwai@suse.de>
thanks,
Takashi
> ---
> sound/hda/common/controller.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> --- a/sound/hda/common/controller.c
> +++ b/sound/hda/common/controller.c
> @@ -525,7 +525,7 @@ static int azx_get_time_info(struct snd_
> break;
>
> default:
> - *system_ts = ktime_to_timespec64(xtstamp.sys_realtime);
> + *system_ts = ktime_to_timespec64(xtstamp.sys_systime);
> break;
>
> }
>
>
>
^ permalink raw reply
* Re: [patch V2 16/25] virtio_rtc: Use provided clock ID for history snapshot
From: Peter Hilber @ 2026-06-02 14:44 UTC (permalink / raw)
To: Thomas Gleixner
Cc: LKML, David Woodhouse, Miroslav Lichvar, John Stultz,
Stephen Boyd, Anna-Maria Behnsen, Frederic Weisbecker,
thomas.weissschuh, Arthur Kiyanovski, Rodolfo Giometti,
Vincent Donnefort, Marc Zyngier, Oliver Upton, kvmarm,
Oliver Upton, Richard Cochran, netdev, Takashi Iwai,
Miri Korenblit, Johannes Berg, Jacob Keller, Tony Nguyen,
Saeed Mahameed, Michael S. Tsirkin, virtualization,
linux-wireless, linux-sound, David Woodhouse, Vadim Fedorenko
In-Reply-To: <20260529195557.744271454@kernel.org>
On Fri, May 29, 2026 at 10:00:52PM +0200, Thomas Gleixner wrote:
> From: Thomas Gleixner <tglx@kernel.org>
>
> The PTP core indicates in system_device_crosststamp::clock_id the clock ID
> for which the system time stamp should be taken. That allows to utilize
> hardware timestamps with e.g. AUX clocks.
>
> Use ktime_get_snapshot_id() and hand the provided clock ID in.
>
> No functional change.
>
> Signed-off-by: Thomas Gleixner <tglx@kernel.org>
> Tested-by: Arthur Kiyanovski <akiyano@amazon.com>
> Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
Acked-by: Peter Hilber <peter.hilber@oss.qualcomm.com>
> ---
> drivers/virtio/virtio_rtc_ptp.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> --- a/drivers/virtio/virtio_rtc_ptp.c
> +++ b/drivers/virtio/virtio_rtc_ptp.c
> @@ -139,7 +139,7 @@ static int viortc_ptp_getcrosststamp(str
> if (ret)
> return ret;
>
> - ktime_get_snapshot(&history_begin);
> + ktime_get_snapshot_id(xtstamp->clock_id, &history_begin);
> if (history_begin.cs_id != cs_id)
> return -EOPNOTSUPP;
>
>
^ permalink raw reply
* Re: [PATCH 2/7] drm/vblank: timer: Fix timestamp calculation
From: Michel Dänzer @ 2026-06-02 14:14 UTC (permalink / raw)
To: Thomas Zimmermann, simona, louis.chauvet, ville.syrjala,
jani.nikula, mhklkml, maarten.lankhorst, mripard, airlied
Cc: dri-devel, amd-gfx, virtualization
In-Reply-To: <f7f53f9b-2576-4488-b0c1-35b6998d123c@suse.de>
On 6/1/26 19:30, Thomas Zimmermann wrote:
> Am 01.06.26 um 18:24 schrieb Michel Dänzer:
>> On 6/1/26 16:08, Thomas Zimmermann wrote:
>>> In drm_crtc_vblank_get_vblank_timeout(), return the timestamp of the
>>> first visible scanline after the last vblank timeout. This is what the
>>> caller expects.
>>>
>>> A vblank phase starts with a vblank timeout. At this point the display
>>> is blanked for several scanlines. Afterwards the display is unblanked
>>> until the next vblank timeout occurs. The display content is only visible
>>> during that second part.
>>>
>>> The current implementation of drm_crtc_vblank_get_vblank_timeout()
>>> returns the timestamp of the last vblank timeout that started the current
>>> vblank phase. But the display only unblanks after 20 to 30 percent of
>>> the overall frame duration. The returned timestamp is therefore too early.
>>>
>>> The next vblank timeout is already known when calculating the returned
>>> timestamp. Instead of subtracting the duration of a full frame from the
>>> value, only subtract the duration of the active, visible part. The result
>>> is the timestamp of the first visible scanline, as expected by the caller.
>>>
>>> This bug was not introduced by the generic vblank timer. It appears that
>>> the get_vblank_timeout logic has always been buggy since it was first
>>> added in commit 3a0709928b17 ("drm/vkms: Add vblank events simulated by
>>> hrtimers").
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> ---
>>> drivers/gpu/drm/drm_vblank.c | 32 +++++++++++++++++++++++++-------
>>> 1 file changed, 25 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
>>> index 96d70c3d4522..d52df247d04e 100644
>>> --- a/drivers/gpu/drm/drm_vblank.c
>>> +++ b/drivers/gpu/drm/drm_vblank.c
>>> [...]
>>> @@ -2312,17 +2321,26 @@ bool drm_crtc_vblank_get_vblank_timeout(struct drm_crtc *crtc, ktime_t *vblank_t
>>> *vblank_time = READ_ONCE(vtimer->timer.node.expires);
>>> } while (cur_count != drm_crtc_vblank_count_and_time(crtc, &cur_time));
>>> - if (drm_WARN_ON(crtc->dev, !ktime_compare(*vblank_time, cur_time)))
>>> + if (drm_WARN_ON(dev, !ktime_compare(*vblank_time, cur_time)))
>>> return false; /* Already expired */
>>> + framedur_ns = vblank->framedur_ns;
>>> +
>>> /*
>>> - * To prevent races we roll the hrtimer forward before we do any
>>> - * interrupt processing - this is how real hw works (the interrupt
>>> - * is only generated after all the vblank registers are updated)
>>> - * and what the vblank core expects. Therefore we need to always
>>> - * correct the timestamp by one frame.
>>> + * To prevent races we rolled the hrtimer forward before we did any
>>> + * timeout processing - this is how real hw works (the interrupt is
>>> + * only generated after all the vblank registers are updated) and what
>>> + * the vblank core expects.
>>> + *
>>> + * Therefore we always need to correct the timestamp. The returned
>>> + * time should be the time of the first active scanline after the
>>> + * previous vblank. Hence subtract the active phase's duration from
>>> + * the next expiration time.
>>> */
>>> - *vblank_time = ktime_sub(*vblank_time, vtimer->interval);
>>> + if (drm_WARN_ON(dev, !mode->crtc_vtotal))
>>> + return false;
>>> + activedur_ns = div_s64(framedur_ns * mode->crtc_vdisplay, mode->crtc_vtotal);
>>> + *vblank_time = ktime_sub_ns(*vblank_time, activedur_ns);
>> Normally the timestamp returned by drm_crtc_vblank_count_and_time is supposed to correspond to the end of vertical blank / start of active, in which case the new code here looks wrong.
>>
>> Also, while the current time is inside an active area, it's supposed to return the timestamp corresponding to the start of the current active area, not the next one.
>
> The initial value of *vblank_time is when the vblank timer fires _next_ and the display blanks. Subtracting the length of the active period should give the time of the first active scanline within the current vblank phase.
>
> Isn't that exactly what you describe?
I don't think so.
It means that the timestamp returned by drm_(crtc_)vblank_count_and_time (which also used e.g. in events sent to user space) corresponds to the end of active / start of vblank, not to the end of vblank / start of active as it should (and does when the vblank timer isn't used).
--
Earthling Michel Dänzer \ GNOME / Xwayland / Mesa developer
https://redhat.com \ Libre software enthusiast
^ permalink raw reply
* Re: [PATCH 01/11] params: bound array element output to the caller's page buffer
From: David Laight @ 2026-06-02 13:04 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Kees Cook, Luis Chamberlain, Pengpeng Hou, stable, Petr Pavlu,
Richard Weinberger, Anton Ivanov, Johannes Berg,
Rafael J. Wysocki, Len Brown, Corey Minyard, Gabriel Somlo,
Michael S. Tsirkin, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
Tvrtko Ursulin, David Airlie, Simona Vetter, Bart Van Assche,
Jason Gunthorpe, Leon Romanovsky, Laurent Pinchart, Hans de Goede,
Mauro Carvalho Chehab, Bjorn Helgaas, Hannes Reinecke,
James E.J. Bottomley, Martin K. Petersen, Daniel Lezcano,
Zhang Rui, Lukasz Luba, Greg Kroah-Hartman, Jiri Slaby,
Alan Stern, Jason Wang, Xuan Zhuo, Eugenio Pérez,
Jason Baron, Jim Cromie, Tiwei Bie, Benjamin Berg,
Ilpo Järvinen, David E. Box, Maciej W. Rozycki,
Srinivas Pandruvada, Peter Zijlstra, Heiko Carstens,
Vasily Gorbik, Sean Christopherson, Paolo Bonzini,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Vinod Koul, Frank Li, Daniel Gomez, Sami Tolvanen,
Aaron Tomlin, Alexander Potapenko, Marco Elver, Dmitry Vyukov,
Andrew Morton, John Johansen, Paul Moore, James Morris,
Serge E. Hallyn, Georgia Garcia, kvm, dmaengine, linux-modules,
kasan-dev, linux-mm, apparmor, linux-security-module, linux-um,
linux-acpi, openipmi-developer, qemu-devel, intel-gfx, dri-devel,
linux-rdma, linux-media, linux-pci, linux-scsi, linux-pm,
linuxppc-dev, linux-serial, linux-usb, usb-storage,
virtualization, linux-kernel, linux-arch, netdev, linux-fsdevel,
linux-hardening
In-Reply-To: <ah699hwLxIIOZ0-7@ashevche-desk.local>
On Tue, 2 Jun 2026 14:26:46 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> On Thu, May 21, 2026 at 06:33:14AM -0700, Kees Cook wrote:
> >
> > param_array_get() appends each element's string representation into the
> > shared sysfs page buffer by passing buffer + off to the element getter.
> >
> > That works for getters that only write a small bounded string, but
> > param_get_charp() and similar helpers format against PAGE_SIZE from the
> > pointer they receive. Once off is non-zero, an element getter can
> > therefore write past the end of the original sysfs page buffer.
> >
> > Collect each element into a temporary PAGE_SIZE buffer first and then
> > copy only the remaining space into the caller's page buffer.
>
> ...
>
> > + elem_buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
>
> get_free_page() (or how it is called)?
The kmalloc() should be faster and I think has to be aligned.
There is another patch set to replace get_free_pages() with kmalloc().
Although all these 'show' functions should really head to using a safer
interface.
Although, at the moment, it is really difficult to find the ones that
are guaranteed to be passed a page aligned buffer.
-- David
>
> > + if (!elem_buf)
> > + return -ENOMEM;
> > +
> > for (i = off = 0; i < (arr->num ? *arr->num : arr->max); i++) {
> > - /* Replace \n with comma */
> > - if (i)
> > - buffer[off - 1] = ',';
> > p.arg = arr->elem + arr->elemsize * i;
> > check_kparam_locked(p.mod);
> > - ret = arr->ops->get(buffer + off, &p);
> > + ret = arr->ops->get(elem_buf, &p);
> > if (ret < 0)
> > - return ret;
> > + goto out;
> > + ret = min(ret, (int)(PAGE_SIZE - 1 - off));
>
> It's usually discouraged to use castings in min/max/clamp. Can we make ret long
> or do something different here?
>
> > + if (!ret)
> > + break;
>
> > + /* Replace the previous element's trailing newline with a comma. */
> > + if (i)
> > + buffer[off - 1] = ',';
>
> Can't we do this after with help of strreplace()?
>
> > + memcpy(buffer + off, elem_buf, ret);
> > off += ret;
> > + if (off == PAGE_SIZE - 1)
> > + break;
> > }
> > buffer[off] = '\0';
> > - return off;
> > + ret = off;
> > +out:
> > + kfree(elem_buf);
> > + return ret;
>
^ permalink raw reply
* Re: [PATCH 01/11] params: bound array element output to the caller's page buffer
From: Jason Gunthorpe @ 2026-06-02 12:33 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Kees Cook, Luis Chamberlain, Pengpeng Hou, stable, Petr Pavlu,
Richard Weinberger, Anton Ivanov, Johannes Berg,
Rafael J. Wysocki, Len Brown, Corey Minyard, Gabriel Somlo,
Michael S. Tsirkin, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
Tvrtko Ursulin, David Airlie, Simona Vetter, Bart Van Assche,
Leon Romanovsky, Laurent Pinchart, Hans de Goede,
Mauro Carvalho Chehab, Bjorn Helgaas, Hannes Reinecke,
James E.J. Bottomley, Martin K. Petersen, Daniel Lezcano,
Zhang Rui, Lukasz Luba, Greg Kroah-Hartman, Jiri Slaby,
Alan Stern, Jason Wang, Xuan Zhuo, Eugenio Pérez,
Jason Baron, Jim Cromie, Tiwei Bie, Benjamin Berg,
Ilpo Järvinen, David E. Box, Maciej W. Rozycki,
Srinivas Pandruvada, Peter Zijlstra, Heiko Carstens,
Vasily Gorbik, Sean Christopherson, Paolo Bonzini,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Vinod Koul, Frank Li, Daniel Gomez, Sami Tolvanen,
Aaron Tomlin, Alexander Potapenko, Marco Elver, Dmitry Vyukov,
Andrew Morton, John Johansen, Paul Moore, James Morris,
Serge E. Hallyn, Georgia Garcia, kvm, dmaengine, linux-modules,
kasan-dev, linux-mm, apparmor, linux-security-module, linux-um,
linux-acpi, openipmi-developer, qemu-devel, intel-gfx, dri-devel,
linux-rdma, linux-media, linux-pci, linux-scsi, linux-pm,
linuxppc-dev, linux-serial, linux-usb, usb-storage,
virtualization, linux-kernel, linux-arch, netdev, linux-fsdevel,
linux-hardening
In-Reply-To: <ah699hwLxIIOZ0-7@ashevche-desk.local>
On Tue, Jun 02, 2026 at 02:26:46PM +0300, Andy Shevchenko wrote:
> On Thu, May 21, 2026 at 06:33:14AM -0700, Kees Cook wrote:
> >
> > param_array_get() appends each element's string representation into the
> > shared sysfs page buffer by passing buffer + off to the element getter.
> >
> > That works for getters that only write a small bounded string, but
> > param_get_charp() and similar helpers format against PAGE_SIZE from the
> > pointer they receive. Once off is non-zero, an element getter can
> > therefore write past the end of the original sysfs page buffer.
> >
> > Collect each element into a temporary PAGE_SIZE buffer first and then
> > copy only the remaining space into the caller's page buffer.
>
> ...
>
> > + elem_buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
>
> get_free_page() (or how it is called)?
I thought modern mm guidance was to use kmalloc whenever possible and
not use get_free_page() unless you intend to use the struct page bits?
Jason
^ permalink raw reply
* Re: [PATCH 01/11] params: bound array element output to the caller's page buffer
From: Andy Shevchenko @ 2026-06-02 11:26 UTC (permalink / raw)
To: Kees Cook
Cc: Luis Chamberlain, Pengpeng Hou, stable, Petr Pavlu,
Richard Weinberger, Anton Ivanov, Johannes Berg,
Rafael J. Wysocki, Len Brown, Corey Minyard, Gabriel Somlo,
Michael S. Tsirkin, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
Tvrtko Ursulin, David Airlie, Simona Vetter, Bart Van Assche,
Jason Gunthorpe, Leon Romanovsky, Laurent Pinchart, Hans de Goede,
Mauro Carvalho Chehab, Bjorn Helgaas, Hannes Reinecke,
James E.J. Bottomley, Martin K. Petersen, Daniel Lezcano,
Zhang Rui, Lukasz Luba, Greg Kroah-Hartman, Jiri Slaby,
Alan Stern, Jason Wang, Xuan Zhuo, Eugenio Pérez,
Jason Baron, Jim Cromie, Tiwei Bie, Benjamin Berg,
Ilpo Järvinen, David E. Box, Maciej W. Rozycki,
Srinivas Pandruvada, Peter Zijlstra, Heiko Carstens,
Vasily Gorbik, Sean Christopherson, Paolo Bonzini,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Vinod Koul, Frank Li, Daniel Gomez, Sami Tolvanen,
Aaron Tomlin, Alexander Potapenko, Marco Elver, Dmitry Vyukov,
Andrew Morton, John Johansen, Paul Moore, James Morris,
Serge E. Hallyn, Georgia Garcia, kvm, dmaengine, linux-modules,
kasan-dev, linux-mm, apparmor, linux-security-module, linux-um,
linux-acpi, openipmi-developer, qemu-devel, intel-gfx, dri-devel,
linux-rdma, linux-media, linux-pci, linux-scsi, linux-pm,
linuxppc-dev, linux-serial, linux-usb, usb-storage,
virtualization, linux-kernel, linux-arch, netdev, linux-fsdevel,
linux-hardening
In-Reply-To: <20260521133326.2465264-1-kees@kernel.org>
On Thu, May 21, 2026 at 06:33:14AM -0700, Kees Cook wrote:
>
> param_array_get() appends each element's string representation into the
> shared sysfs page buffer by passing buffer + off to the element getter.
>
> That works for getters that only write a small bounded string, but
> param_get_charp() and similar helpers format against PAGE_SIZE from the
> pointer they receive. Once off is non-zero, an element getter can
> therefore write past the end of the original sysfs page buffer.
>
> Collect each element into a temporary PAGE_SIZE buffer first and then
> copy only the remaining space into the caller's page buffer.
...
> + elem_buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
get_free_page() (or how it is called)?
> + if (!elem_buf)
> + return -ENOMEM;
> +
> for (i = off = 0; i < (arr->num ? *arr->num : arr->max); i++) {
> - /* Replace \n with comma */
> - if (i)
> - buffer[off - 1] = ',';
> p.arg = arr->elem + arr->elemsize * i;
> check_kparam_locked(p.mod);
> - ret = arr->ops->get(buffer + off, &p);
> + ret = arr->ops->get(elem_buf, &p);
> if (ret < 0)
> - return ret;
> + goto out;
> + ret = min(ret, (int)(PAGE_SIZE - 1 - off));
It's usually discouraged to use castings in min/max/clamp. Can we make ret long
or do something different here?
> + if (!ret)
> + break;
> + /* Replace the previous element's trailing newline with a comma. */
> + if (i)
> + buffer[off - 1] = ',';
Can't we do this after with help of strreplace()?
> + memcpy(buffer + off, elem_buf, ret);
> off += ret;
> + if (off == PAGE_SIZE - 1)
> + break;
> }
> buffer[off] = '\0';
> - return off;
> + ret = off;
> +out:
> + kfree(elem_buf);
> + return ret;
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [patch V2 04/25] pps: Convert to ktime_get_snapshot_id()
From: David Woodhouse @ 2026-06-02 10:56 UTC (permalink / raw)
To: Thomas Gleixner, Rodolfo Giometti, LKML
Cc: Miroslav Lichvar, John Stultz, Stephen Boyd, Anna-Maria Behnsen,
Frederic Weisbecker, thomas.weissschuh, Arthur Kiyanovski,
Vincent Donnefort, Marc Zyngier, Oliver Upton, kvmarm,
Oliver Upton, Richard Cochran, netdev, Takashi Iwai,
Miri Korenblit, Johannes Berg, Jacob Keller, Tony Nguyen,
Saeed Mahameed, Peter Hilber, Michael S. Tsirkin, virtualization,
linux-wireless, linux-sound, Vadim Fedorenko
In-Reply-To: <877boh8f0j.ffs@fw13>
[-- Attachment #1: Type: text/plain, Size: 1836 bytes --]
On Tue, 2026-06-02 at 11:31 +0200, Thomas Gleixner wrote:
> On Sat, May 30 2026 at 13:08, Rodolfo Giometti wrote:
> > On 29/05/2026 22:00, Thomas Gleixner wrote:
> > > static inline void pps_get_ts(struct pps_event_time *ts)
> > > {
> > > +#ifdef CONFIG_NTP_PPS
> > > struct system_time_snapshot snap;
> > >
> > > - ktime_get_snapshot(&snap);
> > > - ts->ts_real = ktime_to_timespec64(snap.real);
> > > -#ifdef CONFIG_NTP_PPS
> > > - ts->ts_raw = ktime_to_timespec64(snap.raw);
> > > + ktime_get_snapshot_id(CLOCK_REALTIME, &snap);
> > > + ts->ts_real = ktime_to_timespec64(snap.systime);
> > > + ts->ts_raw = ktime_to_timespec64(snap.monoraw);
> > > +#else
> > > + ktime_get_real_ts64(&ts->ts_real);
> >
> > /*
> > * Prevent kernel stack information disclosure if the
> > * structure is later copied to userspace.
> > */
> > ts->ts_raw = (struct timespec64){0, 0};
>
> Which will fail to build because of:
>
> struct pps_event_time {
> #ifdef CONFIG_NTP_PPS
> struct timespec64 ts_raw;
> #endif /* CONFIG_NTP_PPS */
> struct timespec64 ts_real;
> };
>
> No?
I think we should kill struct pps_event_time and just use a
system_time_snapshot anyway.
And I think we can make CONFIG_NTP_PPS work for NOHZ too. If I
understand correctly, the main problem with NOHZ is that without a
tick, the reported CLOCK_REALTIME sawtooths much more around the time
that the core timekeeping *wants* to report. That is, tk->ntp_error
goes much higher and much lower, than when the precise 'mult' rate can
be re-adjusted every tick to keep it on course.
If we add an ntp_error field to the system_time_snapshot, the PPS code
could use that to correct the CLOCK_REALTIME value and the jitter that
NOHZ introduces wouldn't matter.
[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5069 bytes --]
^ permalink raw reply
* Re: iommu: iterator used after loop end in resv region insertion?
From: Robin Murphy @ 2026-06-02 10:44 UTC (permalink / raw)
To: Will Deacon, Maoyi Xie; +Cc: joro, jpb, iommu, linux-kernel, virtualization
In-Reply-To: <ah6wYJM4g-GTBTCL@willie-the-truck>
On 2026-06-02 11:28 am, Will Deacon wrote:
> On Tue, May 19, 2026 at 02:49:58AM +0800, Maoyi Xie wrote:
>> Hi all,
>>
>> While reading drivers/iommu/ I noticed two places that look
>> like a past the end iterator pattern. I would appreciate it
>> if you could take a look and let me know whether these are
>> real issues and whether they are worth fixing.
>>
>> The first is iommu_insert_resv_region() in drivers/iommu/iommu.c
>> (linux-7.1-rc1, around line 873):
>>
>> list_for_each_entry(iter, regions, list) {
>> if (nr->start < iter->start ||
>> (nr->start == iter->start && nr->type <= iter->type))
>> break;
>> }
>> list_add_tail(&nr->list, &iter->list);
>>
>> The second is viommu_add_resv_mem() in drivers/iommu/virtio-iommu.c
>> (linux-7.1-rc1, around line 523):
>>
>> list_for_each_entry(next, &vdev->resv_regions, list) {
>> if (next->start > region->start)
>> break;
>> }
>> list_add_tail(®ion->list, &next->list);
>>
>> In both cases, when the loop walks all entries without break,
>> the iterator has gone one step past the last entry. &iter->list
>> then aliases the list head via container_of offset cancellation,
>> so the insert lands at the list tail. That is the intended
>> behaviour, but the access is undefined per C11.
>>
>> Jakob Koschel cleaned up many such sites in 2022, for example
>> commits 99d8ae4ec8a (tracing: Remove usage of list iterator
>> variable after the loop), 2966a9918df (clockevents: Use dedicated
>> list iterator variable) and dc1acd5c946 (dlm: replace usage of
>> found with dedicated list iterator variable). These two sites
>> in drivers/iommu/ were not covered.
>>
>> A candidate fix would track an explicit insert_before pointer
>> initialised to the list head and overwritten to &iter->list
>> only when the loop breaks early. The observable behaviour is
>> unchanged.
>>
>> If this is intentional or already known, please disregard.
>> Otherwise, I am happy to send a [PATCH] or to leave the fix to
>> you. Thank you for your time, and sorry for the noise if this
>> is not actually worth fixing or has already been spotted.
>
> Given that there's some precedent for "fixing" these type of things,
> it's probably best if you just send some patches for these new instances
> if you don't mind.
Agreed, given that the intent in both cases is only to find a list_head
to use as an insertion point, rather than operate on the entries per se,
for clarity I'd suggest changing the whole iteration to list_for_each(),
and keeping the list_entry() dereference strictly inside the loop body.
Thanks,
Robin.
^ permalink raw reply
* Re: iommu: iterator used after loop end in resv region insertion?
From: Will Deacon @ 2026-06-02 10:28 UTC (permalink / raw)
To: Maoyi Xie; +Cc: joro, robin.murphy, jpb, iommu, linux-kernel, virtualization
In-Reply-To: <CAHPEe=G-FZvEXjkE+vAXN5MHXCtsOaUoKwg2RQL6m=om+c20hQ@mail.gmail.com>
On Tue, May 19, 2026 at 02:49:58AM +0800, Maoyi Xie wrote:
> Hi all,
>
> While reading drivers/iommu/ I noticed two places that look
> like a past the end iterator pattern. I would appreciate it
> if you could take a look and let me know whether these are
> real issues and whether they are worth fixing.
>
> The first is iommu_insert_resv_region() in drivers/iommu/iommu.c
> (linux-7.1-rc1, around line 873):
>
> list_for_each_entry(iter, regions, list) {
> if (nr->start < iter->start ||
> (nr->start == iter->start && nr->type <= iter->type))
> break;
> }
> list_add_tail(&nr->list, &iter->list);
>
> The second is viommu_add_resv_mem() in drivers/iommu/virtio-iommu.c
> (linux-7.1-rc1, around line 523):
>
> list_for_each_entry(next, &vdev->resv_regions, list) {
> if (next->start > region->start)
> break;
> }
> list_add_tail(®ion->list, &next->list);
>
> In both cases, when the loop walks all entries without break,
> the iterator has gone one step past the last entry. &iter->list
> then aliases the list head via container_of offset cancellation,
> so the insert lands at the list tail. That is the intended
> behaviour, but the access is undefined per C11.
>
> Jakob Koschel cleaned up many such sites in 2022, for example
> commits 99d8ae4ec8a (tracing: Remove usage of list iterator
> variable after the loop), 2966a9918df (clockevents: Use dedicated
> list iterator variable) and dc1acd5c946 (dlm: replace usage of
> found with dedicated list iterator variable). These two sites
> in drivers/iommu/ were not covered.
>
> A candidate fix would track an explicit insert_before pointer
> initialised to the list head and overwritten to &iter->list
> only when the loop breaks early. The observable behaviour is
> unchanged.
>
> If this is intentional or already known, please disregard.
> Otherwise, I am happy to send a [PATCH] or to leave the fix to
> you. Thank you for your time, and sorry for the noise if this
> is not actually worth fixing or has already been spotted.
Given that there's some precedent for "fixing" these type of things,
it's probably best if you just send some patches for these new instances
if you don't mind.
Cheers,
Will
^ permalink raw reply
* Re: [patch V2 04/25] pps: Convert to ktime_get_snapshot_id()
From: Thomas Gleixner @ 2026-06-02 9:31 UTC (permalink / raw)
To: Rodolfo Giometti, LKML
Cc: David Woodhouse, Miroslav Lichvar, John Stultz, Stephen Boyd,
Anna-Maria Behnsen, Frederic Weisbecker, thomas.weissschuh,
Arthur Kiyanovski, Vincent Donnefort, Marc Zyngier, Oliver Upton,
kvmarm, Oliver Upton, Richard Cochran, netdev, Takashi Iwai,
Miri Korenblit, Johannes Berg, Jacob Keller, Tony Nguyen,
Saeed Mahameed, Peter Hilber, Michael S. Tsirkin, virtualization,
linux-wireless, linux-sound, David Woodhouse, Vadim Fedorenko
In-Reply-To: <912a22e8-878f-4763-958b-71052ee69b36@enneenne.com>
On Sat, May 30 2026 at 13:08, Rodolfo Giometti wrote:
> On 29/05/2026 22:00, Thomas Gleixner wrote:
>> static inline void pps_get_ts(struct pps_event_time *ts)
>> {
>> +#ifdef CONFIG_NTP_PPS
>> struct system_time_snapshot snap;
>>
>> - ktime_get_snapshot(&snap);
>> - ts->ts_real = ktime_to_timespec64(snap.real);
>> -#ifdef CONFIG_NTP_PPS
>> - ts->ts_raw = ktime_to_timespec64(snap.raw);
>> + ktime_get_snapshot_id(CLOCK_REALTIME, &snap);
>> + ts->ts_real = ktime_to_timespec64(snap.systime);
>> + ts->ts_raw = ktime_to_timespec64(snap.monoraw);
>> +#else
>> + ktime_get_real_ts64(&ts->ts_real);
>
> /*
> * Prevent kernel stack information disclosure if the
> * structure is later copied to userspace.
> */
> ts->ts_raw = (struct timespec64){0, 0};
Which will fail to build because of:
struct pps_event_time {
#ifdef CONFIG_NTP_PPS
struct timespec64 ts_raw;
#endif /* CONFIG_NTP_PPS */
struct timespec64 ts_real;
};
No?
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox