Hi,
This patch series converts the driver to use drm managed resources to prevent
potential use-after-free issues on driver unbind/rebind and to get rid of the
usage of deprecated APIs.
Changes in v2:
- While protecting critical sections with drm_dev_{enter,exit} I forgot to
handle alternate return paths within the read-side critical sections, hence
fix them.
- Add a patch to remove explicit calls to drm_mode_config_cleanup() and switch
to drmm_mode_config_init() explicitly.
Danilo Krummrich (9):
drm/arm/malidp: use drmm_* to allocate driver structures
drm/arm/malidp: replace drm->dev_private with drm_to_malidp()
drm/arm/malidp: crtc: use drmm_crtc_init_with_planes()
drm/arm/malidp: plane: use drm managed resources
drm/arm/malidp: use drm_dev_unplug()
drm/arm/malidp: plane: protect device resources after removal
drm/arm/malidp: crtc: protect device resources after removal
drm/arm/malidp: drv: protect device resources after removal
drm/arm/malidp: remove calls to drm_mode_config_cleanup()
drivers/gpu/drm/arm/malidp_crtc.c | 68 +++++++++++++++++++------
drivers/gpu/drm/arm/malidp_drv.c | 78 ++++++++++++-----------------
drivers/gpu/drm/arm/malidp_drv.h | 2 +
drivers/gpu/drm/arm/malidp_hw.c | 10 ++--
drivers/gpu/drm/arm/malidp_mw.c | 6 +--
drivers/gpu/drm/arm/malidp_planes.c | 45 ++++++++---------
6 files changed, 117 insertions(+), 92 deletions(-)
base-commit: 08fb97de03aa2205c6791301bd83a095abc1949c
--
2.37.3
drm_mode_config_init() simply calls drmm_mode_config_init(), hence
cleanup is automatically handled through registering
drm_mode_config_cleanup() with drmm_add_action_or_reset().
While at it, get rid of the deprecated drm_mode_config_init() and
replace it with drmm_mode_config_init() directly.
Signed-off-by: Danilo Krummrich <[email protected]>
---
drivers/gpu/drm/arm/malidp_drv.c | 20 ++++++--------------
1 file changed, 6 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
index 8bb8e8d14461..ef6a9fc1c864 100644
--- a/drivers/gpu/drm/arm/malidp_drv.c
+++ b/drivers/gpu/drm/arm/malidp_drv.c
@@ -401,7 +401,9 @@ static int malidp_init(struct drm_device *drm)
struct malidp_drm *malidp = drm_to_malidp(drm);
struct malidp_hw_device *hwdev = malidp->dev;
- drm_mode_config_init(drm);
+ ret = drmm_mode_config_init(drm);
+ if (ret)
+ goto out;
drm->mode_config.min_width = hwdev->min_line_size;
drm->mode_config.min_height = hwdev->min_line_size;
@@ -412,24 +414,16 @@ static int malidp_init(struct drm_device *drm)
ret = malidp_crtc_init(drm);
if (ret)
- goto crtc_fail;
+ goto out;
ret = malidp_mw_connector_init(drm);
if (ret)
- goto crtc_fail;
-
- return 0;
+ goto out;
-crtc_fail:
- drm_mode_config_cleanup(drm);
+out:
return ret;
}
-static void malidp_fini(struct drm_device *drm)
-{
- drm_mode_config_cleanup(drm);
-}
-
static int malidp_irq_init(struct platform_device *pdev)
{
int irq_de, irq_se, ret = 0;
@@ -879,7 +873,6 @@ static int malidp_bind(struct device *dev)
bind_fail:
of_node_put(malidp->crtc.port);
malidp->crtc.port = NULL;
- malidp_fini(drm);
query_hw_fail:
pm_runtime_put(dev);
if (pm_runtime_enabled(dev))
@@ -907,7 +900,6 @@ static void malidp_unbind(struct device *dev)
component_unbind_all(dev, drm);
of_node_put(malidp->crtc.port);
malidp->crtc.port = NULL;
- malidp_fini(drm);
pm_runtime_put(dev);
if (pm_runtime_enabled(dev))
pm_runtime_disable(dev);
--
2.37.3
(Hardware) resources which are bound to the driver and device lifecycle
must not be accessed after the device and driver are unbound.
However, the DRM device isn't freed as long as the last user didn't
close it, hence userspace can still call into the driver.
Therefore protect the critical sections which are accessing those
resources with drm_dev_enter() and drm_dev_exit().
Signed-off-by: Danilo Krummrich <[email protected]>
---
drivers/gpu/drm/arm/malidp_crtc.c | 61 +++++++++++++++++++++++++------
1 file changed, 50 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/arm/malidp_crtc.c b/drivers/gpu/drm/arm/malidp_crtc.c
index dc01c43f6193..e11cda5fdeb7 100644
--- a/drivers/gpu/drm/arm/malidp_crtc.c
+++ b/drivers/gpu/drm/arm/malidp_crtc.c
@@ -14,6 +14,7 @@
#include <drm/drm_atomic.h>
#include <drm/drm_atomic_helper.h>
#include <drm/drm_crtc.h>
+#include <drm/drm_drv.h>
#include <drm/drm_framebuffer.h>
#include <drm/drm_print.h>
#include <drm/drm_probe_helper.h>
@@ -27,6 +28,8 @@ static enum drm_mode_status malidp_crtc_mode_valid(struct drm_crtc *crtc,
{
struct malidp_drm *malidp = crtc_to_malidp_device(crtc);
struct malidp_hw_device *hwdev = malidp->dev;
+ enum drm_mode_status status = MODE_OK;
+ int idx;
/*
* check that the hardware can drive the required clock rate,
@@ -34,15 +37,21 @@ static enum drm_mode_status malidp_crtc_mode_valid(struct drm_crtc *crtc,
*/
long rate, req_rate = mode->crtc_clock * 1000;
+ if (!drm_dev_enter(&malidp->base, &idx))
+ return MODE_NOCLOCK;
+
if (req_rate) {
rate = clk_round_rate(hwdev->pxlclk, req_rate);
if (rate != req_rate) {
DRM_DEBUG_DRIVER("pxlclk doesn't support %ld Hz\n",
req_rate);
- return MODE_NOCLOCK;
+ status = MODE_NOCLOCK;
+ goto out;
}
}
+out:
+ drm_dev_exit(idx);
return MODE_OK;
}
@@ -52,11 +61,15 @@ static void malidp_crtc_atomic_enable(struct drm_crtc *crtc,
struct malidp_drm *malidp = crtc_to_malidp_device(crtc);
struct malidp_hw_device *hwdev = malidp->dev;
struct videomode vm;
- int err = pm_runtime_get_sync(crtc->dev->dev);
+ int err, idx;
+
+ if (!drm_dev_enter(&malidp->base, &idx))
+ return;
+ err = pm_runtime_get_sync(crtc->dev->dev);
if (err < 0) {
DRM_DEBUG_DRIVER("Failed to enable runtime power management: %d\n", err);
- return;
+ goto out;
}
drm_display_mode_to_videomode(&crtc->state->adjusted_mode, &vm);
@@ -68,6 +81,9 @@ static void malidp_crtc_atomic_enable(struct drm_crtc *crtc,
hwdev->hw->modeset(hwdev, &vm);
hwdev->hw->leave_config_mode(hwdev);
drm_crtc_vblank_on(crtc);
+
+out:
+ drm_dev_exit(idx);
}
static void malidp_crtc_atomic_disable(struct drm_crtc *crtc,
@@ -77,7 +93,10 @@ static void malidp_crtc_atomic_disable(struct drm_crtc *crtc,
crtc);
struct malidp_drm *malidp = crtc_to_malidp_device(crtc);
struct malidp_hw_device *hwdev = malidp->dev;
- int err;
+ int err, idx;
+
+ if (!drm_dev_enter(&malidp->base, &idx))
+ return;
/* always disable planes on the CRTC that is being turned off */
drm_atomic_helper_disable_planes_on_crtc(old_state, false);
@@ -91,6 +110,8 @@ static void malidp_crtc_atomic_disable(struct drm_crtc *crtc,
if (err < 0) {
DRM_DEBUG_DRIVER("Failed to disable runtime power management: %d\n", err);
}
+
+ drm_dev_exit(idx);
}
static const struct gamma_curve_segment {
@@ -260,7 +281,10 @@ static int malidp_crtc_atomic_check_scaling(struct drm_crtc *crtc,
u32 h_upscale_factor = 0; /* U16.16 */
u32 v_upscale_factor = 0; /* U16.16 */
u8 scaling = cs->scaled_planes_mask;
- int ret;
+ int idx, ret;
+
+ if (!drm_dev_enter(&malidp->base, &idx))
+ return -ENODEV;
if (!scaling) {
s->scale_enable = false;
@@ -268,8 +292,10 @@ static int malidp_crtc_atomic_check_scaling(struct drm_crtc *crtc,
}
/* The scaling engine can only handle one plane at a time. */
- if (scaling & (scaling - 1))
- return -EINVAL;
+ if (scaling & (scaling - 1)) {
+ ret = -EINVAL;
+ goto out;
+ }
drm_atomic_crtc_state_for_each_plane_state(plane, pstate, state) {
struct malidp_plane *mp = to_malidp_plane(plane);
@@ -331,10 +357,10 @@ static int malidp_crtc_atomic_check_scaling(struct drm_crtc *crtc,
mclk_calc:
drm_display_mode_to_videomode(&state->adjusted_mode, &vm);
- ret = hwdev->hw->se_calc_mclk(hwdev, s, &vm);
- if (ret < 0)
- return -EINVAL;
- return 0;
+ ret = hwdev->hw->se_calc_mclk(hwdev, s, &vm) < 0 ? -EINVAL : 0;
+out:
+ drm_dev_exit(idx);
+ return ret;
}
static int malidp_crtc_atomic_check(struct drm_crtc *crtc,
@@ -498,9 +524,16 @@ static int malidp_crtc_enable_vblank(struct drm_crtc *crtc)
{
struct malidp_drm *malidp = crtc_to_malidp_device(crtc);
struct malidp_hw_device *hwdev = malidp->dev;
+ int idx;
+
+ if (!drm_dev_enter(&malidp->base, &idx))
+ return -ENODEV;
malidp_hw_enable_irq(hwdev, MALIDP_DE_BLOCK,
hwdev->hw->map.de_irq_map.vsync_irq);
+
+ drm_dev_exit(idx);
+
return 0;
}
@@ -508,9 +541,15 @@ static void malidp_crtc_disable_vblank(struct drm_crtc *crtc)
{
struct malidp_drm *malidp = crtc_to_malidp_device(crtc);
struct malidp_hw_device *hwdev = malidp->dev;
+ int idx;
+
+ if (!drm_dev_enter(&malidp->base, &idx))
+ return;
malidp_hw_disable_irq(hwdev, MALIDP_DE_BLOCK,
hwdev->hw->map.de_irq_map.vsync_irq);
+
+ drm_dev_exit(idx);
}
static const struct drm_crtc_funcs malidp_crtc_funcs = {
--
2.37.3
Use drm managed resources to allocate driver structures and get rid of
the deprecated drm_dev_alloc() call and replace it with
devm_drm_dev_alloc().
This also serves as preparation to get rid of drm_device->dev_private
and to fix use-after-free issues on driver unload.
Signed-off-by: Danilo Krummrich <[email protected]>
---
drivers/gpu/drm/arm/malidp_drv.c | 20 +++++++-------------
drivers/gpu/drm/arm/malidp_drv.h | 1 +
2 files changed, 8 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
index 1d0b0c54ccc7..41c80e905991 100644
--- a/drivers/gpu/drm/arm/malidp_drv.c
+++ b/drivers/gpu/drm/arm/malidp_drv.c
@@ -23,6 +23,7 @@
#include <drm/drm_fourcc.h>
#include <drm/drm_gem_dma_helper.h>
#include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_managed.h>
#include <drm/drm_modeset_helper.h>
#include <drm/drm_module.h>
#include <drm/drm_of.h>
@@ -716,11 +717,13 @@ static int malidp_bind(struct device *dev)
int ret = 0, i;
u32 version, out_depth = 0;
- malidp = devm_kzalloc(dev, sizeof(*malidp), GFP_KERNEL);
- if (!malidp)
- return -ENOMEM;
+ malidp = devm_drm_dev_alloc(dev, &malidp_driver, typeof(*malidp), base);
+ if (IS_ERR(malidp))
+ return PTR_ERR(malidp);
+
+ drm = &malidp->base;
- hwdev = devm_kzalloc(dev, sizeof(*hwdev), GFP_KERNEL);
+ hwdev = drmm_kzalloc(drm, sizeof(*hwdev), GFP_KERNEL);
if (!hwdev)
return -ENOMEM;
@@ -753,12 +756,6 @@ static int malidp_bind(struct device *dev)
if (ret && ret != -ENODEV)
return ret;
- drm = drm_dev_alloc(&malidp_driver, dev);
- if (IS_ERR(drm)) {
- ret = PTR_ERR(drm);
- goto alloc_fail;
- }
-
drm->dev_private = malidp;
dev_set_drvdata(dev, drm);
@@ -887,8 +884,6 @@ static int malidp_bind(struct device *dev)
malidp_runtime_pm_suspend(dev);
drm->dev_private = NULL;
dev_set_drvdata(dev, NULL);
- drm_dev_put(drm);
-alloc_fail:
of_reserved_mem_device_release(dev);
return ret;
@@ -917,7 +912,6 @@ static void malidp_unbind(struct device *dev)
malidp_runtime_pm_suspend(dev);
drm->dev_private = NULL;
dev_set_drvdata(dev, NULL);
- drm_dev_put(drm);
of_reserved_mem_device_release(dev);
}
diff --git a/drivers/gpu/drm/arm/malidp_drv.h b/drivers/gpu/drm/arm/malidp_drv.h
index cdfddfabf2d1..00be369b28f1 100644
--- a/drivers/gpu/drm/arm/malidp_drv.h
+++ b/drivers/gpu/drm/arm/malidp_drv.h
@@ -29,6 +29,7 @@ struct malidp_error_stats {
};
struct malidp_drm {
+ struct drm_device base;
struct malidp_hw_device *dev;
struct drm_crtc crtc;
struct drm_writeback_connector mw_connector;
--
2.37.3
When the driver is unbound, there might still be users in userspace
having an open fd and are calling into the driver.
While this is fine for drm managed resources, it is not for resources
bound to the device/driver lifecycle, e.g. clocks or MMIO mappings.
To prevent use-after-free issues we need to protect those resources with
drm_dev_enter() and drm_dev_exit(). This does only work if we indicate
that the drm device was unplugged, hence use drm_dev_unplug() instead of
drm_dev_unregister().
Protecting the particular resources with drm_dev_enter()/drm_dev_exit()
is handled by subsequent patches.
Signed-off-by: Danilo Krummrich <[email protected]>
---
drivers/gpu/drm/arm/malidp_drv.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
index 678c5b0d8014..aedd30f5f451 100644
--- a/drivers/gpu/drm/arm/malidp_drv.c
+++ b/drivers/gpu/drm/arm/malidp_drv.c
@@ -893,7 +893,7 @@ static void malidp_unbind(struct device *dev)
struct malidp_drm *malidp = drm_to_malidp(drm);
struct malidp_hw_device *hwdev = malidp->dev;
- drm_dev_unregister(drm);
+ drm_dev_unplug(drm);
drm_kms_helper_poll_fini(drm);
pm_runtime_get_sync(dev);
drm_atomic_helper_shutdown(drm);
--
2.37.3
Use drmm_crtc_init_with_planes() instead of drm_crtc_init_with_planes()
to get rid of the explicit destroy hook in struct drm_plane_funcs.
Signed-off-by: Danilo Krummrich <[email protected]>
---
drivers/gpu/drm/arm/malidp_crtc.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/arm/malidp_crtc.c b/drivers/gpu/drm/arm/malidp_crtc.c
index 34ad7e1cd2b8..dc01c43f6193 100644
--- a/drivers/gpu/drm/arm/malidp_crtc.c
+++ b/drivers/gpu/drm/arm/malidp_crtc.c
@@ -514,7 +514,6 @@ static void malidp_crtc_disable_vblank(struct drm_crtc *crtc)
}
static const struct drm_crtc_funcs malidp_crtc_funcs = {
- .destroy = drm_crtc_cleanup,
.set_config = drm_atomic_helper_set_config,
.page_flip = drm_atomic_helper_page_flip,
.reset = malidp_crtc_reset,
@@ -548,8 +547,8 @@ int malidp_crtc_init(struct drm_device *drm)
return -EINVAL;
}
- ret = drm_crtc_init_with_planes(drm, &malidp->crtc, primary, NULL,
- &malidp_crtc_funcs, NULL);
+ ret = drmm_crtc_init_with_planes(drm, &malidp->crtc, primary, NULL,
+ &malidp_crtc_funcs, NULL);
if (ret)
return ret;
--
2.37.3
Hi Danilo,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on 08fb97de03aa2205c6791301bd83a095abc1949c]
url: https://github.com/intel-lab-lkp/linux/commits/Danilo-Krummrich/drm-arm-malidp-use-drm-managed-resources/20221002-004256
base: 08fb97de03aa2205c6791301bd83a095abc1949c
config: s390-allyesconfig
compiler: s390-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/379208cbeb040ca0643b28ff14ca3f6ab99128a4
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Danilo-Krummrich/drm-arm-malidp-use-drm-managed-resources/20221002-004256
git checkout 379208cbeb040ca0643b28ff14ca3f6ab99128a4
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash drivers/gpu/drm/arm/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
drivers/gpu/drm/arm/malidp_crtc.c: In function 'malidp_crtc_mode_valid':
>> drivers/gpu/drm/arm/malidp_crtc.c:31:30: warning: variable 'status' set but not used [-Wunused-but-set-variable]
31 | enum drm_mode_status status = MODE_OK;
| ^~~~~~
vim +/status +31 drivers/gpu/drm/arm/malidp_crtc.c
25
26 static enum drm_mode_status malidp_crtc_mode_valid(struct drm_crtc *crtc,
27 const struct drm_display_mode *mode)
28 {
29 struct malidp_drm *malidp = crtc_to_malidp_device(crtc);
30 struct malidp_hw_device *hwdev = malidp->dev;
> 31 enum drm_mode_status status = MODE_OK;
32 int idx;
33
34 /*
35 * check that the hardware can drive the required clock rate,
36 * but skip the check if the clock is meant to be disabled (req_rate = 0)
37 */
38 long rate, req_rate = mode->crtc_clock * 1000;
39
40 if (!drm_dev_enter(&malidp->base, &idx))
41 return MODE_NOCLOCK;
42
43 if (req_rate) {
44 rate = clk_round_rate(hwdev->pxlclk, req_rate);
45 if (rate != req_rate) {
46 DRM_DEBUG_DRIVER("pxlclk doesn't support %ld Hz\n",
47 req_rate);
48 status = MODE_NOCLOCK;
49 goto out;
50 }
51 }
52
53 out:
54 drm_dev_exit(idx);
55 return MODE_OK;
56 }
57
--
0-DAY CI Kernel Test Service
https://01.org/lkp