Re: [PATCH] hv/vmbus: copy dynamic IDs before dropping the lock

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Mon, Oct 19, 2026 at 04:55:44PM +0800, Jiale Yao wrote:
> remove_id_store() can free a matching dynamic ID while VMBus is
> preparing to invoke a driver probe.

And how can that be triggered?

> Copy the ID while holding the dynamic ID lock so probe never receives
> a pointer into a freed list entry.

Why is this driver allowed to handle dynamic ids at all?  Is it really
required?  That will taint the kernel so no real workflow requires it,
right?

> Fixes: fc76936d3ea5 ("vmbus: add support for dynamic device id's")

No cc: stable?

> Signed-off-by: Jiale Yao <yaojiale02@163.com>
> ---
>  drivers/hv/vmbus_drv.c | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 4692687cdbf3..96ad0dfaa50b 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -661,21 +661,22 @@ hv_vmbus_dev_match(const struct hv_vmbus_device_id *id, const guid_t *guid)
>  }
>  
>  static const struct hv_vmbus_device_id *
> -hv_vmbus_dynid_match(struct hv_driver *drv, const guid_t *guid)
> +hv_vmbus_dynid_match(struct hv_driver *drv, const guid_t *guid,
> +		     struct hv_vmbus_device_id *id_copy)
>  {
> -	const struct hv_vmbus_device_id *id = NULL;
>  	struct vmbus_dynid *dynid;
>  
>  	spin_lock(&drv->dynids.lock);
>  	list_for_each_entry(dynid, &drv->dynids.list, node) {
>  		if (guid_equal(&dynid->id.guid, guid)) {
> -			id = &dynid->id;
> -			break;
> +			*id_copy = dynid->id;

Shouldn't this be a memcpy() as that's what you are really doing?

> +			spin_unlock(&drv->dynids.lock);

guard() usage instead?

> +			return id_copy;
>  		}
>  	}
>  	spin_unlock(&drv->dynids.lock);
>  
> -	return id;
> +	return NULL;
>  }
>  
>  static const struct hv_vmbus_device_id vmbus_device_null;
> @@ -685,7 +686,8 @@ static const struct hv_vmbus_device_id vmbus_device_null;
>   * If there is no match, return NULL.
>   */
>  static const struct hv_vmbus_device_id *hv_vmbus_get_id(const struct hv_driver *drv,
> -							struct hv_device *dev)
> +							struct hv_device *dev,
> +							struct hv_vmbus_device_id *id_copy)
>  {
>  	const guid_t *guid = &dev->dev_type;
>  	const struct hv_vmbus_device_id *id;
> @@ -697,7 +699,7 @@ static const struct hv_vmbus_device_id *hv_vmbus_get_id(const struct hv_driver *
>  		return NULL;
>  
>  	/* Look at the dynamic ids first, before the static ones */
> -	id = hv_vmbus_dynid_match((struct hv_driver *)drv, guid);
> +	id = hv_vmbus_dynid_match((struct hv_driver *)drv, guid, id_copy);
>  	if (!id)
>  		id = hv_vmbus_dev_match(drv->id_table, guid);
>  
> @@ -773,6 +775,7 @@ static ssize_t new_id_store(struct device_driver *driver, const char *buf,
>  			    size_t count)
>  {
>  	struct hv_driver *drv = drv_to_hv_drv(driver);
> +	struct hv_vmbus_device_id id_copy;

On the stack?

Ok, I guess this would work, but it feels clunky, why doesn't all other
busses have to do this too?

thanks,

greg k-h