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.
Danilo Krummrich (7):
drm/arm/hdlcd: use drmm_* to allocate driver structures
drm/arm/hdlcd: replace drm->dev_private with drm_to_hdlcd_priv()
drm/arm/hdlcd: crtc: use drmm_crtc_init_with_planes()
drm/arm/hdlcd: plane: use drm managed resources
drm/arm/hdlcd: use drm_dev_unplug()
drm/arm/hdlcd: crtc: protect device resources after removal
drm/arm/hdlcd: debugfs: protect device resources after removal
drivers/gpu/drm/arm/hdlcd_crtc.c | 78 ++++++++++++++++++++++++--------
drivers/gpu/drm/arm/hdlcd_drv.c | 36 ++++++++-------
drivers/gpu/drm/arm/hdlcd_drv.h | 2 +
3 files changed, 79 insertions(+), 37 deletions(-)
base-commit: 8fe444eb326869823f3788a4b4da5dca03339d10
--
2.37.2
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/hdlcd_drv.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
index 120c87934a91..e41def6d47cc 100644
--- a/drivers/gpu/drm/arm/hdlcd_drv.c
+++ b/drivers/gpu/drm/arm/hdlcd_drv.c
@@ -325,7 +325,7 @@ static void hdlcd_drm_unbind(struct device *dev)
struct drm_device *drm = dev_get_drvdata(dev);
struct hdlcd_drm_private *hdlcd = drm_to_hdlcd_priv(drm);
- drm_dev_unregister(drm);
+ drm_dev_unplug(drm);
drm_kms_helper_poll_fini(drm);
component_unbind_all(dev, drm);
of_node_put(hdlcd->crtc.port);
--
2.37.2
Using drm_device->dev_private is deprecated. Since we've switched to
devm_drm_dev_alloc(), struct drm_device is now embedded in struct
hdlcd_drm_private, hence we can use container_of() to get the struct
drm_device instance instead.
Signed-off-by: Danilo Krummrich <[email protected]>
---
drivers/gpu/drm/arm/hdlcd_crtc.c | 4 ++--
drivers/gpu/drm/arm/hdlcd_drv.c | 10 ++++------
drivers/gpu/drm/arm/hdlcd_drv.h | 1 +
3 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c
index 7030339fa232..4a8959d0b2a6 100644
--- a/drivers/gpu/drm/arm/hdlcd_crtc.c
+++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
@@ -275,7 +275,7 @@ static void hdlcd_plane_atomic_update(struct drm_plane *plane,
dest_h = drm_rect_height(&new_plane_state->dst);
scanout_start = drm_fb_dma_get_gem_addr(fb, new_plane_state, 0);
- hdlcd = plane->dev->dev_private;
+ hdlcd = drm_to_hdlcd_priv(plane->dev);
hdlcd_write(hdlcd, HDLCD_REG_FB_LINE_LENGTH, fb->pitches[0]);
hdlcd_write(hdlcd, HDLCD_REG_FB_LINE_PITCH, fb->pitches[0]);
hdlcd_write(hdlcd, HDLCD_REG_FB_LINE_COUNT, dest_h - 1);
@@ -325,7 +325,7 @@ static struct drm_plane *hdlcd_plane_init(struct drm_device *drm)
int hdlcd_setup_crtc(struct drm_device *drm)
{
- struct hdlcd_drm_private *hdlcd = drm->dev_private;
+ struct hdlcd_drm_private *hdlcd = drm_to_hdlcd_priv(drm);
struct drm_plane *primary;
int ret;
diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
index 463381d11cff..120c87934a91 100644
--- a/drivers/gpu/drm/arm/hdlcd_drv.c
+++ b/drivers/gpu/drm/arm/hdlcd_drv.c
@@ -98,7 +98,7 @@ static void hdlcd_irq_uninstall(struct hdlcd_drm_private *hdlcd)
static int hdlcd_load(struct drm_device *drm, unsigned long flags)
{
- struct hdlcd_drm_private *hdlcd = drm->dev_private;
+ struct hdlcd_drm_private *hdlcd = drm_to_hdlcd_priv(drm);
struct platform_device *pdev = to_platform_device(drm->dev);
struct resource *res;
u32 version;
@@ -190,7 +190,7 @@ static int hdlcd_show_underrun_count(struct seq_file *m, void *arg)
{
struct drm_info_node *node = (struct drm_info_node *)m->private;
struct drm_device *drm = node->minor->dev;
- struct hdlcd_drm_private *hdlcd = drm->dev_private;
+ struct hdlcd_drm_private *hdlcd = drm_to_hdlcd_priv(drm);
seq_printf(m, "underrun : %d\n", atomic_read(&hdlcd->buffer_underrun_count));
seq_printf(m, "dma_end : %d\n", atomic_read(&hdlcd->dma_end_count));
@@ -203,7 +203,7 @@ static int hdlcd_show_pxlclock(struct seq_file *m, void *arg)
{
struct drm_info_node *node = (struct drm_info_node *)m->private;
struct drm_device *drm = node->minor->dev;
- struct hdlcd_drm_private *hdlcd = drm->dev_private;
+ struct hdlcd_drm_private *hdlcd = drm_to_hdlcd_priv(drm);
unsigned long clkrate = clk_get_rate(hdlcd->clk);
unsigned long mode_clock = hdlcd->crtc.mode.crtc_clock * 1000;
@@ -253,7 +253,6 @@ static int hdlcd_drm_bind(struct device *dev)
drm = &hdlcd->base;
- drm->dev_private = hdlcd;
dev_set_drvdata(dev, drm);
hdlcd_setup_mode_config(drm);
@@ -324,7 +323,7 @@ static int hdlcd_drm_bind(struct device *dev)
static void hdlcd_drm_unbind(struct device *dev)
{
struct drm_device *drm = dev_get_drvdata(dev);
- struct hdlcd_drm_private *hdlcd = drm->dev_private;
+ struct hdlcd_drm_private *hdlcd = drm_to_hdlcd_priv(drm);
drm_dev_unregister(drm);
drm_kms_helper_poll_fini(drm);
@@ -339,7 +338,6 @@ static void hdlcd_drm_unbind(struct device *dev)
pm_runtime_disable(dev);
of_reserved_mem_device_release(dev);
drm_mode_config_cleanup(drm);
- drm->dev_private = NULL;
dev_set_drvdata(dev, NULL);
}
diff --git a/drivers/gpu/drm/arm/hdlcd_drv.h b/drivers/gpu/drm/arm/hdlcd_drv.h
index 3892b36767ac..f1c1da2ac2db 100644
--- a/drivers/gpu/drm/arm/hdlcd_drv.h
+++ b/drivers/gpu/drm/arm/hdlcd_drv.h
@@ -21,6 +21,7 @@ struct hdlcd_drm_private {
#endif
};
+#define drm_to_hdlcd_priv(x) container_of(x, struct hdlcd_drm_private, base)
#define crtc_to_hdlcd_priv(x) container_of(x, struct hdlcd_drm_private, crtc)
static inline void hdlcd_write(struct hdlcd_drm_private *hdlcd,
--
2.37.2
(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/hdlcd_drv.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
index e41def6d47cc..020c7d0c70a5 100644
--- a/drivers/gpu/drm/arm/hdlcd_drv.c
+++ b/drivers/gpu/drm/arm/hdlcd_drv.c
@@ -204,11 +204,19 @@ static int hdlcd_show_pxlclock(struct seq_file *m, void *arg)
struct drm_info_node *node = (struct drm_info_node *)m->private;
struct drm_device *drm = node->minor->dev;
struct hdlcd_drm_private *hdlcd = drm_to_hdlcd_priv(drm);
- unsigned long clkrate = clk_get_rate(hdlcd->clk);
- unsigned long mode_clock = hdlcd->crtc.mode.crtc_clock * 1000;
+ unsigned long clkrate, mode_clock;
+ int idx;
+
+ if (!drm_dev_enter(drm, &idx))
+ return -ENODEV;
+
+ clkrate = clk_get_rate(hdlcd->clk);
+ mode_clock = hdlcd->crtc.mode.crtc_clock * 1000;
seq_printf(m, "hw : %lu\n", clkrate);
seq_printf(m, "mode: %lu\n", mode_clock);
+
+ drm_dev_exit(idx);
return 0;
}
--
2.37.2
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/hdlcd_drv.c | 12 ++++--------
drivers/gpu/drm/arm/hdlcd_drv.h | 1 +
2 files changed, 5 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
index a032003c340c..463381d11cff 100644
--- a/drivers/gpu/drm/arm/hdlcd_drv.c
+++ b/drivers/gpu/drm/arm/hdlcd_drv.c
@@ -247,13 +247,11 @@ static int hdlcd_drm_bind(struct device *dev)
struct hdlcd_drm_private *hdlcd;
int ret;
- hdlcd = devm_kzalloc(dev, sizeof(*hdlcd), GFP_KERNEL);
- if (!hdlcd)
- return -ENOMEM;
+ hdlcd = devm_drm_dev_alloc(dev, &hdlcd_driver, typeof(*hdlcd), base);
+ if (IS_ERR(hdlcd))
+ return PTR_ERR(hdlcd);
- drm = drm_dev_alloc(&hdlcd_driver, dev);
- if (IS_ERR(drm))
- return PTR_ERR(drm);
+ drm = &hdlcd->base;
drm->dev_private = hdlcd;
dev_set_drvdata(dev, drm);
@@ -319,7 +317,6 @@ static int hdlcd_drm_bind(struct device *dev)
err_free:
drm_mode_config_cleanup(drm);
dev_set_drvdata(dev, NULL);
- drm_dev_put(drm);
return ret;
}
@@ -344,7 +341,6 @@ static void hdlcd_drm_unbind(struct device *dev)
drm_mode_config_cleanup(drm);
drm->dev_private = NULL;
dev_set_drvdata(dev, NULL);
- drm_dev_put(drm);
}
static const struct component_master_ops hdlcd_master_ops = {
diff --git a/drivers/gpu/drm/arm/hdlcd_drv.h b/drivers/gpu/drm/arm/hdlcd_drv.h
index 909c39c28487..3892b36767ac 100644
--- a/drivers/gpu/drm/arm/hdlcd_drv.h
+++ b/drivers/gpu/drm/arm/hdlcd_drv.h
@@ -7,6 +7,7 @@
#define __HDLCD_DRV_H__
struct hdlcd_drm_private {
+ struct drm_device base;
void __iomem *mmio;
struct clk *clk;
struct drm_crtc crtc;
--
2.37.2
Use drmm_crtc_init_with_planes() instead of drm_crtc_init_with_planes()
to get rid of the explicit drm_crtc_cleanup() call.
Signed-off-by: Danilo Krummrich <[email protected]>
---
drivers/gpu/drm/arm/hdlcd_crtc.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c
index 4a8959d0b2a6..c0a5ca7f578a 100644
--- a/drivers/gpu/drm/arm/hdlcd_crtc.c
+++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
@@ -42,7 +42,6 @@ static void hdlcd_crtc_cleanup(struct drm_crtc *crtc)
/* stop the controller on cleanup */
hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 0);
- drm_crtc_cleanup(crtc);
}
static int hdlcd_crtc_enable_vblank(struct drm_crtc *crtc)
@@ -333,8 +332,8 @@ int hdlcd_setup_crtc(struct drm_device *drm)
if (IS_ERR(primary))
return PTR_ERR(primary);
- ret = drm_crtc_init_with_planes(drm, &hdlcd->crtc, primary, NULL,
- &hdlcd_crtc_funcs, NULL);
+ ret = drmm_crtc_init_with_planes(drm, &hdlcd->crtc, primary, NULL,
+ &hdlcd_crtc_funcs, NULL);
if (ret)
return ret;
--
2.37.2
(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/hdlcd_crtc.c | 49 ++++++++++++++++++++++++++++++--
1 file changed, 47 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c
index 17d3ccf12245..bfc42d4a53c2 100644
--- a/drivers/gpu/drm/arm/hdlcd_crtc.c
+++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
@@ -18,6 +18,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_fb_dma_helper.h>
#include <drm/drm_fb_helper.h>
#include <drm/drm_framebuffer.h>
@@ -39,27 +40,47 @@
static void hdlcd_crtc_cleanup(struct drm_crtc *crtc)
{
struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc);
+ int idx;
+
+ if (!drm_dev_enter(crtc->dev, &idx))
+ return;
/* stop the controller on cleanup */
hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 0);
+
+ drm_dev_exit(idx);
}
static int hdlcd_crtc_enable_vblank(struct drm_crtc *crtc)
{
struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc);
- unsigned int mask = hdlcd_read(hdlcd, HDLCD_REG_INT_MASK);
+ unsigned int mask;
+ int idx;
+ if (!drm_dev_enter(crtc->dev, &idx))
+ return -ENODEV;
+
+ mask = hdlcd_read(hdlcd, HDLCD_REG_INT_MASK);
hdlcd_write(hdlcd, HDLCD_REG_INT_MASK, mask | HDLCD_INTERRUPT_VSYNC);
+ drm_dev_exit(idx);
+
return 0;
}
static void hdlcd_crtc_disable_vblank(struct drm_crtc *crtc)
{
struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc);
- unsigned int mask = hdlcd_read(hdlcd, HDLCD_REG_INT_MASK);
+ unsigned int mask;
+ int idx;
+ if (!drm_dev_enter(crtc->dev, &idx))
+ return;
+
+ mask = hdlcd_read(hdlcd, HDLCD_REG_INT_MASK);
hdlcd_write(hdlcd, HDLCD_REG_INT_MASK, mask & ~HDLCD_INTERRUPT_VSYNC);
+
+ drm_dev_exit(idx);
}
static const struct drm_crtc_funcs hdlcd_crtc_funcs = {
@@ -170,21 +191,33 @@ static void hdlcd_crtc_atomic_enable(struct drm_crtc *crtc,
struct drm_atomic_state *state)
{
struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc);
+ int idx;
+
+ if (!drm_dev_enter(crtc->dev, &idx))
+ return;
clk_prepare_enable(hdlcd->clk);
hdlcd_crtc_mode_set_nofb(crtc);
hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 1);
drm_crtc_vblank_on(crtc);
+
+ drm_dev_exit(idx);
}
static void hdlcd_crtc_atomic_disable(struct drm_crtc *crtc,
struct drm_atomic_state *state)
{
struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc);
+ int idx;
+
+ if (!drm_dev_enter(crtc->dev, &idx))
+ return;
drm_crtc_vblank_off(crtc);
hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 0);
clk_disable_unprepare(hdlcd->clk);
+
+ drm_dev_exit(idx);
}
static enum drm_mode_status hdlcd_crtc_mode_valid(struct drm_crtc *crtc,
@@ -192,6 +225,10 @@ static enum drm_mode_status hdlcd_crtc_mode_valid(struct drm_crtc *crtc,
{
struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc);
long rate, clk_rate = mode->clock * 1000;
+ int idx;
+
+ if (!drm_dev_enter(crtc->dev, &idx))
+ return MODE_NOCLOCK;
rate = clk_round_rate(hdlcd->clk, clk_rate);
/* 0.1% seems a close enough tolerance for the TDA19988 on Juno */
@@ -200,6 +237,8 @@ static enum drm_mode_status hdlcd_crtc_mode_valid(struct drm_crtc *crtc,
return MODE_NOCLOCK;
}
+ drm_dev_exit(idx);
+
return MODE_OK;
}
@@ -267,6 +306,10 @@ static void hdlcd_plane_atomic_update(struct drm_plane *plane,
struct hdlcd_drm_private *hdlcd;
u32 dest_h;
dma_addr_t scanout_start;
+ int idx;
+
+ if (!drm_dev_enter(plane->dev, &idx))
+ return;
if (!fb)
return;
@@ -279,6 +322,8 @@ static void hdlcd_plane_atomic_update(struct drm_plane *plane,
hdlcd_write(hdlcd, HDLCD_REG_FB_LINE_PITCH, fb->pitches[0]);
hdlcd_write(hdlcd, HDLCD_REG_FB_LINE_COUNT, dest_h - 1);
hdlcd_write(hdlcd, HDLCD_REG_FB_BASE, scanout_start);
+
+ drm_dev_exit(idx);
}
static const struct drm_plane_helper_funcs hdlcd_plane_helper_funcs = {
--
2.37.2