Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [Low] The patch mistakenly defines a `drm_connector_funcs` structure named `qxl_connector_funcs` instead of a `drm_encoder_funcs` structure for the new encoder, causing a redefinition build error. Pre-existing issues: - [High] Lockless reallocation of `client_monitors_config` leads to Use-After-Free during connector operations. - [High] Missing `free_irq()` and `cancel_work_sync()` during device teardown leads to Use-After-Free on module unload. -- --- Patch [1]: [PATCH] drm/qxl: remove dependency on DRM simple helpers --- commit d182374fb0440bb0cb67a0c069afac8494f4bf85 Author: Diogo Silva <[email protected]> drm/qxl: remove dependency on DRM simple helpers This patch removes the drm_simple_encoder_init() helper usage in the qxl display driver by open coding it and using the encoder atomic helpers directly. This is a step to eventually get rid of the simple KMS helper. > diff --git a/drivers/gpu/drm/qxl/qxl_display.c > b/drivers/gpu/drm/qxl/qxl_display.c > index a026bd35ef485..236d027fbc9fe 100644 > --- a/drivers/gpu/drm/qxl/qxl_display.c > +++ b/drivers/gpu/drm/qxl/qxl_display.c > @@ -1095,6 +1095,10 @@ static const struct drm_connector_helper_funcs > qxl_connector_helper_funcs = { > .best_encoder = qxl_best_encoder, > }; > > +static const struct drm_connector_funcs qxl_connector_funcs = { > + .destroy = drm_encoder_cleanup, > +}; > + [Severity: Low] Does this code inadvertently define struct drm_connector_funcs instead of struct drm_encoder_funcs? The struct name qxl_connector_funcs also conflicts with an existing definition later in the file. [ ... ] > @@ -1169,10 +1173,10 @@ static int qdev_output_init(struct drm_device *dev, > int num_output) > drm_connector_init(dev, &qxl_output->base, > &qxl_connector_funcs, DRM_MODE_CONNECTOR_VIRTUAL); > > - ret = drm_simple_encoder_init(dev, &qxl_output->enc, > - DRM_MODE_ENCODER_VIRTUAL); > + ret = drm_encoder_init(dev, &qxl_output->enc, &qxl_connector_funcs, > + DRM_MODE_ENCODER_VIRTUAL, NULL); [Severity: Low] Is this passing a pointer to the newly added struct drm_connector_funcs where drm_encoder_init() expects struct drm_encoder_funcs *? [ ... ] [Severity: High] This isn't a bug introduced by this patch, but is there a use-after-free risk during connector operations? In qxl_alloc_client_monitors_config() in drivers/gpu/drm/qxl/qxl_display.c, the shared client monitors config can be freed locklessly: if (qdev->client_monitors_config && count > qdev->client_monitors_config->count) { kfree(qdev->client_monitors_config); qdev->client_monitors_config = NULL; } Concurrently, DRM ioctls like qxl_conn_detect() check and dereference this pointer without synchronization: if (!qdev->client_monitors_config) { if (output->index == 0) connected = true; } else connected = qdev->client_monitors_config->count > output->index && qxl_head_enabled(&qdev->client_monitors_config->heads[output->index]); Could a concurrent reallocation free the config while it is being accessed here? [Severity: High] This is a pre-existing issue, but is there a missing teardown of the IRQ handler and workqueue on module unload? In qxl_irq_init() in drivers/gpu/drm/qxl/qxl_irq.c, the IRQ is manually requested and the workqueue is initialized: INIT_WORK(&qdev->client_monitors_config_work, qxl_client_monitors_config_work_func); ... ret = request_irq(pdev->irq, qxl_irq_handler, IRQF_SHARED, ddev->driver->name, ddev); However, the teardown logic in qxl_pci_remove() in drivers/gpu/drm/qxl/qxl_drv.c appears to omit free_irq() and cancel_work_sync(): static void qxl_pci_remove(struct pci_dev *pdev) { struct drm_device *dev = pci_get_drvdata(pdev); drm_kms_helper_poll_fini(dev); drm_dev_unregister(dev); drm_atomic_helper_shutdown(dev); Does this leak the IRQ handler registration, potentially leading to a use-after-free if the shared IRQ fires after the device is unbound? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
