Re: [PATCH] PCI: hv: Probe vPCI buses asynchronously

[Naman Jain <namjain@linux.microsoft.com>]

On 9/7/2026 11:32 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [Critical] VMBus transaction mappings are not removed when wait_for_response() aborts early, leading to stack or heap Use-After-Free (UAF) upon delayed host completion.
> - [High] Memory leak of child PCI devices (struct hv_pci_dev / hpdev) on the error paths of hv_pci_probe().
> - [High] Workqueue leaked on hv_pci_remove() if hv_pci_resume() previously failed.
> --
> 

All of the issues reported by Sashiko are pre-existing and unrelated to 
my patch. No action required.

Regards,
Naman

> commit acbe68fb0b4a7fd692c44acd0bc27c2e48b030db
> Author: Naman Jain <namjain@linux.microsoft.com>
> 
> PCI: hv: Probe vPCI buses asynchronously
>      
> On Hyper-V guests each virtual PCI bus is enumerated by its own
> hv_pci_probe() call. The probe performs several synchronous host
> request/response exchanges while negotiating the protocol, querying bus
> relations, entering D0, and reporting allocated resources.
> 
>> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
>> index 89816a2bd7cd3..056d379b3cee4 100644
>> --- a/drivers/pci/controller/pci-hyperv.c
>> +++ b/drivers/pci/controller/pci-hyperv.c
>> @@ -4155,6 +4155,9 @@ static struct hv_driver hv_pci_drv = {
>>   	.remove		= hv_pci_remove,
>>   	.suspend	= hv_pci_suspend,
>>   	.resume		= hv_pci_resume,
>> +	.driver = {
>> +		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
>> +	},
>>   };
> 
> [Severity: High]
> This is a pre-existing issue, but I noticed a potential memory leak of child
> PCI device structures (struct hv_pci_dev) in the error paths of
> hv_pci_probe() in drivers/pci/controller/pci-hyperv.c.
> 
> If hv_pci_enter_d0() fails, the error path jumps directly to
> release_state_lock:
> 
>      ret = hv_pci_enter_d0(hdev);
>      if (ret)
>          goto release_state_lock;
> 
> This completely skips hv_pci_bus_exit(), which is responsible for unlinking
> and freeing the children.
> 
> Additionally, if an error occurs earlier and it jumps to exit_d0, it calls
> hv_pci_bus_exit() but skips freeing the devices:
> 
> exit_d0:
>      (void) hv_pci_bus_exit(hdev, true);
> release_state_lock:
>      mutex_unlock(&hbus->state_lock);
> 
> The keep_devs=true argument explicitly skips freeing the children.
> 
> In both situations, the hbus structure is subsequently freed, leaving the
> previously populated hbus->children list permanently leaked. Could these
> error paths be adjusted to ensure child devices are properly freed?
> 
> [Severity: High]
> This is a pre-existing issue, but could a workqueue be leaked in
> hv_pci_remove() if hv_pci_resume() had previously failed?
> 
> In drivers/pci/controller/pci-hyperv.c:hv_pci_resume(), hbus->state is
> initially set to hv_pcibus_init. If a subsequent operation fails, the
> function returns an error without restoring the state.
> 
> Later, when the device is removed, hv_pci_remove() checks the state before
> destroying the workqueue:
> 
>      if (hbus->state == hv_pcibus_installed) {
>          tasklet_disable(&hdev->channel->callback_event);
>          hbus->state = hv_pcibus_removing;
>          tasklet_enable(&hdev->channel->callback_event);
>          destroy_workqueue(hbus->wq);
>          hbus->wq = NULL;
>          ...
> 
> Since the state was left as hv_pcibus_init, this block is bypassed, and
> the workqueue is leaked when hbus is later freed. Does this state check need
> to be modified to ensure the workqueue is always destroyed on removal?
> 
> [Severity: Critical]
> This is a pre-existing issue, but can VMBus transaction mappings be left
> dangling when wait_for_response() aborts early, leading to a Use-After-Free
> upon delayed host completion?
> 
> In drivers/pci/controller/pci-hyperv.c, several functions allocate a
> struct pci_packet on the stack or heap, pass it to the VMBus requestor,
> and then wait for a response. For example, in new_pcichild_device():
> 
>      if (wait_for_response(hbus->hdev, &comp_pkt.host_event))
>          goto error;
> 
> Or in hv_pci_enter_d0():
> 
>      ret = wait_for_response(hdev, &comp_pkt.host_event);
>      if (ret)
>          goto exit;
> 
> Similar early returns occur in hv_read_config_block() and
> hv_write_config_block().
> 
> If wait_for_response() aborts immediately (e.g. returning -ENODEV due to a
> channel rescind), the caller returns and destroys its stack frame or frees
> the heap allocation.
> 
> However, the transaction ID mapping remains in the VMBus requestor. If the
> host sends a completion packet just after this, the hv_pci_onchannelcallback()
> tasklet will retrieve the dangling pointer and execute its completion_func,
> corrupting the stack or heap.
> 
> Should these functions use vmbus_request_addr_match() to remove the mapping
> on failure, similar to how hv_pci_bus_exit() handles it?
>