[v2,1/11] drm: logicvc: Use devm_of_reserved_mem_device_init()
Commit Message
Switch to devm_of_reserved_mem_device_init() so the reserved memory
region is released automatically on probe failure or device unbind.
Replace all error paths that jumped to error_reserved_mem: with
error_early: since the manual cleanup label is no longer needed, and
remove the explicit of_reserved_mem_device_release() call in the remove
function.
Reviewed-by: Paul Kocialkowski <paulk@sys-base.io>
Acked-by: Paul Kocialkowski <paulk@sys-base.io>
Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
---
drivers/gpu/drm/logicvc/logicvc_drm.c | 21 ++++++++-------------
1 file changed, 8 insertions(+), 13 deletions(-)
Comments
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] Use-after-free of layer structures (containing drm_plane) during driver cleanup due to incorrect devm vs. drmm initialization order.
- [High] Concurrent probe overwrites shared static global regmap_config, causing a data race.
--
commit 3a0c473b709e63281231182c727f9e5d25ce8b6f
Author: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
drm: logicvc: Use devm_of_reserved_mem_device_init()
Switch to devm_of_reserved_mem_device_init() so the reserved memory region
is released automatically on probe failure or device unbind. Replace manual
cleanup labels with error_early.
> diff --git a/drivers/gpu/drm/logicvc/logicvc_drm.c b/drivers/gpu/drm/logicvc/logicvc_drm.c
> index bbebf4fc7f51a..d6dbe52ff0a99 100644
> --- a/drivers/gpu/drm/logicvc/logicvc_drm.c
> +++ b/drivers/gpu/drm/logicvc/logicvc_drm.c
[ ... ]
> @@ -327,14 +327,14 @@ static int logicvc_drm_probe(struct platform_device *pdev)
[ ... ]
> base = devm_ioremap_resource(dev, &res);
> if (IS_ERR(base)) {
> dev_err(dev, "Failed to map I/O base\n");
> ret = PTR_ERR(base);
> - goto error_reserved_mem;
> + goto error_early;
> }
>
> logicvc_drm_regmap_config.max_register = resource_size(&res) -
[Severity: High]
This isn't a bug introduced by this patch, but does this concurrent probe
overwrite the shared static global logicvc_drm_regmap_config?
Modifying a static global structure without locking can cause a data race if
multiple devices probe concurrently (e.g., if multiple IP cores are defined
in the device tree).
Depending on the race window, one device instance might be initialized with
the max_register value of another instance, leading to either restricted MMIO
access or out-of-bounds MMIO access.
> @@ -345,21 +345,21 @@ static int logicvc_drm_probe(struct platform_device *pdev)
[ ... ]
> irq = platform_get_irq(pdev, 0);
> if (irq < 0) {
> ret = -ENODEV;
> - goto error_reserved_mem;
> + goto error_early;
> }
>
> logicvc = devm_drm_dev_alloc(dev, &logicvc_drm_driver,
> struct logicvc_drm, drm_dev);
[Severity: High]
This is a pre-existing issue, but can this devm_drm_dev_alloc() call lead to
a use-after-free of layer structures during driver cleanup?
This call establishes the point in the devres stack where drm_dev_put() will
execute. Because logicvc_layer_init() allocates layer structures with
devm_kzalloc() after devm_drm_dev_alloc() executes:
drivers/gpu/drm/logicvc/logicvc_layer.c:logicvc_layer_init() {
...
layer = devm_kzalloc(dev, sizeof(*layer), GFP_KERNEL);
...
}
devres will execute in LIFO order on unbind. This means devm_kfree() frees
the layer memory before the DRM device's devres action calls drm_dev_put().
When the DRM device is finally destroyed, drmm actions execute (including
drm_mode_config_cleanup()), which iterates over registered planes and calls
their destroy hook (drm_plane_cleanup). Since the plane is embedded in the
already-freed layer struct, this causes a use-after-free.
@@ -306,7 +306,7 @@ static int logicvc_drm_probe(struct platform_device *pdev)
int irq;
int ret;
- ret = of_reserved_mem_device_init(dev);
+ ret = devm_of_reserved_mem_device_init(dev);
if (ret && ret != -ENODEV) {
dev_err(dev, "Failed to init memory region\n");
goto error_early;
@@ -327,14 +327,14 @@ static int logicvc_drm_probe(struct platform_device *pdev)
ret = of_address_to_resource(of_node, 0, &res);
if (ret) {
dev_err(dev, "Failed to get resource from address\n");
- goto error_reserved_mem;
+ goto error_early;
}
base = devm_ioremap_resource(dev, &res);
if (IS_ERR(base)) {
dev_err(dev, "Failed to map I/O base\n");
ret = PTR_ERR(base);
- goto error_reserved_mem;
+ goto error_early;
}
logicvc_drm_regmap_config.max_register = resource_size(&res) -
@@ -345,21 +345,21 @@ static int logicvc_drm_probe(struct platform_device *pdev)
if (IS_ERR(regmap)) {
dev_err(dev, "Failed to create regmap for I/O\n");
ret = PTR_ERR(regmap);
- goto error_reserved_mem;
+ goto error_early;
}
}
irq = platform_get_irq(pdev, 0);
if (irq < 0) {
ret = -ENODEV;
- goto error_reserved_mem;
+ goto error_early;
}
logicvc = devm_drm_dev_alloc(dev, &logicvc_drm_driver,
struct logicvc_drm, drm_dev);
if (IS_ERR(logicvc)) {
ret = PTR_ERR(logicvc);
- goto error_reserved_mem;
+ goto error_early;
}
platform_set_drvdata(pdev, logicvc);
@@ -371,7 +371,7 @@ static int logicvc_drm_probe(struct platform_device *pdev)
caps = logicvc_drm_caps_match(logicvc);
if (!caps) {
ret = -EINVAL;
- goto error_reserved_mem;
+ goto error_early;
}
logicvc->caps = caps;
@@ -382,7 +382,7 @@ static int logicvc_drm_probe(struct platform_device *pdev)
ret = logicvc_clocks_prepare(logicvc);
if (ret) {
drm_err(drm_dev, "Failed to prepare clocks\n");
- goto error_reserved_mem;
+ goto error_early;
}
ret = devm_request_irq(dev, irq, logicvc_drm_irq_handler, 0,
@@ -450,9 +450,6 @@ static int logicvc_drm_probe(struct platform_device *pdev)
error_clocks:
logicvc_clocks_unprepare(logicvc);
-error_reserved_mem:
- of_reserved_mem_device_release(dev);
-
error_early:
return ret;
}
@@ -469,8 +466,6 @@ static void logicvc_drm_remove(struct platform_device *pdev)
logicvc_mode_fini(logicvc);
logicvc_clocks_unprepare(logicvc);
-
- of_reserved_mem_device_release(dev);
}
static void logicvc_drm_shutdown(struct platform_device *pdev)