mshv_portid_lookup() previously took rcu_read_lock() internally, ran
idr_find(), released the read lock, and copied the struct contents
into a caller-supplied buffer. This had two problems.
1. The struct copy ran outside the read section, racing with
mshv_portid_free() which does idr_remove + synchronize_rcu + kfree.
A copy that started just before synchronize_rcu() observed the read
section as already drained and was free to read freed memory while
the writer was kfree()'ing the entry.
2. The only consumer, mshv_doorbell_isr(), then dispatched a callback
using fields of the snapshot — entirely outside any RCU read
section. The callback's data argument and any field it touches
were therefore safe only because mshv_isr() runs from
sysvec_hyperv_callback, a non-threaded system vector that
synchronize_rcu() implicitly waits for via the hardirq quiescent-
state coupling. That protection is real today but undocumented and
fragile: a future move of mshv_isr() to a threaded context, or a
future caller that registers a doorbell with a shorter-lived data
pointer, would silently expose a use-after-free.
Make the contract explicit instead of implicit. mshv_portid_lookup()
now returns a pointer to the table entry and requires the caller to
hold rcu_read_lock for the entire lifetime of that pointer. The
contract is annotated with __must_hold(RCU) so sparse flags any
direct caller that forgets it. The sole caller, mshv_doorbell_isr(),
takes rcu_read_lock around the whole drain loop, so the lookup, the
field reads, and the doorbell_cb dispatch all run inside one
read-side critical section. synchronize_rcu() in mshv_portid_free()
now genuinely waits for any in-flight callback before kfree() runs,
without relying on hardirq context for correctness.
This also drops the by-value struct copy: entries are publish-once
(populated before idr_alloc) and free-once (after synchronize_rcu),
so a pointer dereferenced inside the read section gives a stable
view of the contents without copying.
Fixes: 621191d709b14 ("Drivers: hv: Introduce mshv_root module to expose
/dev/mshv to VMMs")
Signed-off-by: Stanislav Kinsburskii <[email protected]>
---
drivers/hv/mshv_portid_table.c | 22 +++++++---------------
drivers/hv/mshv_root.h | 2 +-
drivers/hv/mshv_synic.c | 15 +++++++++------
3 files changed, 17 insertions(+), 22 deletions(-)
diff --git a/drivers/hv/mshv_portid_table.c b/drivers/hv/mshv_portid_table.c
index c349af1f0aaac..4cdf8e9575390 100644
--- a/drivers/hv/mshv_portid_table.c
+++ b/drivers/hv/mshv_portid_table.c
@@ -64,20 +64,12 @@ mshv_portid_free(int port_id)
kfree(info);
}
-int
-mshv_portid_lookup(int port_id, struct port_table_info *info)
+/*
+ * Caller must hold rcu_read_lock for the entire lifetime of the
+ * returned pointer. Returns NULL if @port_id is not in the table.
+ */
+struct port_table_info *mshv_portid_lookup(int port_id)
+ __must_hold(RCU)
{
- struct port_table_info *_info;
- int ret = -ENOENT;
-
- rcu_read_lock();
- _info = idr_find(&port_table_idr, port_id);
- rcu_read_unlock();
-
- if (_info) {
- *info = *_info;
- ret = 0;
- }
-
- return ret;
+ return idr_find(&port_table_idr, port_id);
}
diff --git a/drivers/hv/mshv_root.h b/drivers/hv/mshv_root.h
index 2e6c4414740cc..b6961a6d9a98b 100644
--- a/drivers/hv/mshv_root.h
+++ b/drivers/hv/mshv_root.h
@@ -241,7 +241,7 @@ void mshv_irqfd_routing_update(struct mshv_partition
*partition);
void mshv_port_table_fini(void);
int mshv_portid_alloc(struct port_table_info *info);
-int mshv_portid_lookup(int port_id, struct port_table_info *info);
+struct port_table_info *mshv_portid_lookup(int port_id) __must_hold(RCU);
void mshv_portid_free(int port_id);
int mshv_register_doorbell(u64 partition_id, doorbell_cb_t doorbell_cb,
diff --git a/drivers/hv/mshv_synic.c b/drivers/hv/mshv_synic.c
index 43f1bcbbf2d34..bac890cd2b468 100644
--- a/drivers/hv/mshv_synic.c
+++ b/drivers/hv/mshv_synic.c
@@ -114,24 +114,27 @@ mshv_doorbell_isr(struct hv_message *msg)
if (notification->sint_index != HV_SYNIC_DOORBELL_SINT_INDEX)
return false;
+ rcu_read_lock();
while ((port =
synic_event_ring_get_queued_port(HV_SYNIC_DOORBELL_SINT_INDEX))) {
- struct port_table_info ptinfo = { 0 };
+ struct port_table_info *ptinfo;
- if (mshv_portid_lookup(port, &ptinfo)) {
+ ptinfo = mshv_portid_lookup(port);
+ if (!ptinfo) {
pr_debug("Failed to get port info from port_table!\n");
continue;
}
- if (ptinfo.hv_port_type != HV_PORT_TYPE_DOORBELL) {
+ if (ptinfo->hv_port_type != HV_PORT_TYPE_DOORBELL) {
pr_debug("Not a doorbell port!, port: %d, port_type:
%d\n",
- port, ptinfo.hv_port_type);
+ port, ptinfo->hv_port_type);
continue;
}
/* Invoke the callback */
- ptinfo.hv_port_doorbell.doorbell_cb(port,
- ptinfo.hv_port_doorbell.data);
+ ptinfo->hv_port_doorbell.doorbell_cb(port,
+ ptinfo->hv_port_doorbell.data);
}
+ rcu_read_unlock();
return true;
}
