Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Missing cleanup of PCI driver on VMBus registration failure in module init. - [High] Flawed synchronous VMBus message handling: missing reinit_completion() and ignored send errors. -- commit a50bc353055fe2753b3f284aeec394019513c888 Author: Michael Kelley <[email protected]> Subject: drm/hyperv: Use "hv_drm_" as symbol name prefix This commit mechanically substitutes the "hyperv_" prefix with "hv_drm_" for all functions and structures in the Hyper-V DRM driver. This avoids namespace conflicts with generic Hyper-V initialization code. > diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_drv.c > b/drivers/gpu/drm/hyperv/hyperv_drm_drv.c > index b6bf6412ae34a..20f35c48c0b82 100644 > --- a/drivers/gpu/drm/hyperv/hyperv_drm_drv.c > +++ b/drivers/gpu/drm/hyperv/hyperv_drm_drv.c [ ... ] > -static int __init hyperv_init(void) > +static int __init hv_drm_init(void) > { > int ret; > > if (drm_firmware_drivers_only()) > return -ENODEV; > > - ret = pci_register_driver(&hyperv_pci_driver); > + ret = pci_register_driver(&hv_drm_pci_driver); > if (ret != 0) > return ret; > > - return vmbus_driver_register(&hyperv_hv_driver); > + return vmbus_driver_register(&hv_drm_hv_driver); > } [Severity: High] This is a pre-existing issue, but if vmbus_driver_register() fails, does this leave the PCI driver registered while the module initialization fails? If the module initialization returns an error and the kernel frees the module's memory, the PCI subsystem might retain a dangling pointer to the freed hv_drm_pci_driver structure. Could this lead to a kernel panic or use-after-free if the PCI driver list is traversed later? > diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_proto.c > b/drivers/gpu/drm/hyperv/hyperv_drm_proto.c > index 6e09b0218df4e..f0ef627b4898a 100644 > --- a/drivers/gpu/drm/hyperv/hyperv_drm_proto.c > +++ b/drivers/gpu/drm/hyperv/hyperv_drm_proto.c [ ... ] > -static int hyperv_negotiate_version(struct hv_device *hdev, u32 ver) > +static int hv_drm_negotiate_version(struct hv_device *hdev, u32 ver) > { > - struct hyperv_drm_device *hv = hv_get_drvdata(hdev); > + struct hv_drm_device *hv = hv_get_drvdata(hdev); > struct synthvid_msg *msg = (struct synthvid_msg *)hv->init_buf; > struct drm_device *dev = &hv->dev; > unsigned long t; > @@ -223,7 +223,7 @@ static int hyperv_negotiate_version(struct hv_device > *hdev, u32 ver) > msg->vid_hdr.size = sizeof(struct synthvid_msg_hdr) + > sizeof(struct synthvid_version_req); > msg->ver_req.version = ver; > - hyperv_sendpacket(hdev, msg); > + hv_drm_sendpacket(hdev, msg); > > t = wait_for_completion_timeout(&hv->wait, VMBUS_VSP_TIMEOUT); [Severity: High] This isn't a bug introduced by this patch, but since the driver only initializes the completion once during probe, is it possible for it to become stale here? If a previous request timed out and the host responds later, the completion could remain signaled. This would cause a subsequent request to immediately return from wait_for_completion_timeout() and process incorrect or stale data. Should there be a call to reinit_completion() before sending the packet? Additionally, since hv_drm_sendpacket() errors are ignored, does the driver unnecessarily hang in wait_for_completion_timeout() for the full timeout duration if the packet completely failed to send? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
