On Thu, May 07, 2026 at 03:43:54PM +0000, Stanislav Kinsburskii wrote:
> 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);
> }
This is now a one line function. We can drop this function completely.
Wei
