* [PATCH 1/3] drm/vc4: plane: use drm managed resources
2022-07-13 8:54 [PATCH 0/3] drm/vc4: use drm managed resources Danilo Krummrich
@ 2022-07-13 8:54 ` Danilo Krummrich
2022-07-13 8:54 ` [PATCH 2/3] drm/vc4: crtc: " Danilo Krummrich
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Danilo Krummrich @ 2022-07-13 8:54 UTC (permalink / raw)
To: emma, mripard, daniel, airlied; +Cc: dri-devel, linux-kernel, Danilo Krummrich
Allocate the plane object with drmm_universal_plane_alloc() in order to
tie the release action to the underlying struct drm_device, where all
the userspace visible stuff is attached to, rather than to struct device.
This can prevent potential use-after free issues on driver unload or
EPROBE_DEFERRED backoff.
Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
drivers/gpu/drm/vc4/vc4_plane.c | 19 +++++++------------
1 file changed, 7 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
index 1e866dc00ac3..0d88de31aa9c 100644
--- a/drivers/gpu/drm/vc4/vc4_plane.c
+++ b/drivers/gpu/drm/vc4/vc4_plane.c
@@ -1469,7 +1469,6 @@ struct drm_plane *vc4_plane_init(struct drm_device *dev,
struct vc4_plane *vc4_plane;
u32 formats[ARRAY_SIZE(hvs_formats)];
int num_formats = 0;
- int ret = 0;
unsigned i;
static const uint64_t modifiers[] = {
DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED,
@@ -1480,11 +1479,6 @@ struct drm_plane *vc4_plane_init(struct drm_device *dev,
DRM_FORMAT_MOD_INVALID
};
- vc4_plane = devm_kzalloc(dev->dev, sizeof(*vc4_plane),
- GFP_KERNEL);
- if (!vc4_plane)
- return ERR_PTR(-ENOMEM);
-
for (i = 0; i < ARRAY_SIZE(hvs_formats); i++) {
if (!hvs_formats[i].hvs5_only || vc4->is_vc5) {
formats[num_formats] = hvs_formats[i].drm;
@@ -1492,13 +1486,14 @@ struct drm_plane *vc4_plane_init(struct drm_device *dev,
}
}
+ vc4_plane = drmm_universal_plane_alloc(dev, struct vc4_plane, base, 0,
+ &vc4_plane_funcs, formats,
+ num_formats, modifiers, type,
+ NULL);
+ if (!vc4_plane)
+ return ERR_CAST(vc4_plane);
+
plane = &vc4_plane->base;
- ret = drm_universal_plane_init(dev, plane, 0,
- &vc4_plane_funcs,
- formats, num_formats,
- modifiers, type, NULL);
- if (ret)
- return ERR_PTR(ret);
if (vc4->is_vc5)
drm_plane_helper_add(plane, &vc5_plane_helper_funcs);
--
2.36.1
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 2/3] drm/vc4: crtc: use drm managed resources
2022-07-13 8:54 [PATCH 0/3] drm/vc4: use drm managed resources Danilo Krummrich
2022-07-13 8:54 ` [PATCH 1/3] drm/vc4: plane: " Danilo Krummrich
@ 2022-07-13 8:54 ` Danilo Krummrich
2022-07-13 8:55 ` [PATCH 3/3] drm/vc4: encoder: " Danilo Krummrich
2022-07-13 9:10 ` [PATCH 0/3] drm/vc4: " Maxime Ripard
3 siblings, 0 replies; 6+ messages in thread
From: Danilo Krummrich @ 2022-07-13 8:54 UTC (permalink / raw)
To: emma, mripard, daniel, airlied; +Cc: dri-devel, linux-kernel, Danilo Krummrich
Allocate the crtc objects with drmm_kzalloc() in order to tie the release
action to the underlying struct drm_device, where all the userspace
visible stuff is attached to, rather than to struct device.
This can prevent potential use-after free issues on driver unload or
EPROBE_DEFERRED backoff.
Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
drivers/gpu/drm/vc4/vc4_crtc.c | 2 +-
drivers/gpu/drm/vc4/vc4_txp.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
index 9355213dc883..4b0c61cc3265 100644
--- a/drivers/gpu/drm/vc4/vc4_crtc.c
+++ b/drivers/gpu/drm/vc4/vc4_crtc.c
@@ -1273,7 +1273,7 @@ static int vc4_crtc_bind(struct device *dev, struct device *master, void *data)
struct drm_plane *destroy_plane, *temp;
int ret;
- vc4_crtc = devm_kzalloc(dev, sizeof(*vc4_crtc), GFP_KERNEL);
+ vc4_crtc = drmm_kzalloc(drm, sizeof(*vc4_crtc), GFP_KERNEL);
if (!vc4_crtc)
return -ENOMEM;
crtc = &vc4_crtc->base;
diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
index 3579d487402e..ae2193fe3b29 100644
--- a/drivers/gpu/drm/vc4/vc4_txp.c
+++ b/drivers/gpu/drm/vc4/vc4_txp.c
@@ -479,7 +479,7 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
if (irq < 0)
return irq;
- txp = devm_kzalloc(dev, sizeof(*txp), GFP_KERNEL);
+ txp = drmm_kzalloc(drm, sizeof(*txp), GFP_KERNEL);
if (!txp)
return -ENOMEM;
vc4_crtc = &txp->base;
--
2.36.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 3/3] drm/vc4: encoder: use drm managed resources
2022-07-13 8:54 [PATCH 0/3] drm/vc4: use drm managed resources Danilo Krummrich
2022-07-13 8:54 ` [PATCH 1/3] drm/vc4: plane: " Danilo Krummrich
2022-07-13 8:54 ` [PATCH 2/3] drm/vc4: crtc: " Danilo Krummrich
@ 2022-07-13 8:55 ` Danilo Krummrich
2022-07-13 9:10 ` [PATCH 0/3] drm/vc4: " Maxime Ripard
3 siblings, 0 replies; 6+ messages in thread
From: Danilo Krummrich @ 2022-07-13 8:55 UTC (permalink / raw)
To: emma, mripard, daniel, airlied; +Cc: dri-devel, linux-kernel, Danilo Krummrich
Allocate the encoder objects with drmm_simple_encoder_alloc() in order to
tie the release action to the underlying struct drm_device, where all the
userspace visible stuff is attached to, rather than to struct device.
This can prevent potential use-after free issues on driver unload or
EPROBE_DEFERRED backoff.
Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
drivers/gpu/drm/vc4/vc4_dpi.c | 11 ++++++-----
drivers/gpu/drm/vc4/vc4_dsi.c | 10 +++++-----
drivers/gpu/drm/vc4/vc4_hdmi.c | 10 ++++++----
drivers/gpu/drm/vc4/vc4_vec.c | 11 ++++++-----
4 files changed, 23 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_dpi.c b/drivers/gpu/drm/vc4/vc4_dpi.c
index c180eb60bee8..7f1703a42060 100644
--- a/drivers/gpu/drm/vc4/vc4_dpi.c
+++ b/drivers/gpu/drm/vc4/vc4_dpi.c
@@ -258,10 +258,12 @@ static int vc4_dpi_bind(struct device *dev, struct device *master, void *data)
if (!dpi)
return -ENOMEM;
- vc4_dpi_encoder = devm_kzalloc(dev, sizeof(*vc4_dpi_encoder),
- GFP_KERNEL);
- if (!vc4_dpi_encoder)
- return -ENOMEM;
+ vc4_dpi_encoder = drmm_simple_encoder_alloc(drm, struct vc4_dpi_encoder,
+ base.base,
+ DRM_MODE_ENCODER_DPI);
+ if (IS_ERR(vc4_dpi_encoder))
+ return PTR_ERR(vc4_dpi_encoder);
+
vc4_dpi_encoder->base.type = VC4_ENCODER_TYPE_DPI;
vc4_dpi_encoder->dpi = dpi;
dpi->encoder = &vc4_dpi_encoder->base.base;
@@ -299,7 +301,6 @@ static int vc4_dpi_bind(struct device *dev, struct device *master, void *data)
if (ret)
DRM_ERROR("Failed to turn on core clock: %d\n", ret);
- drm_simple_encoder_init(drm, dpi->encoder, DRM_MODE_ENCODER_DPI);
drm_encoder_helper_add(dpi->encoder, &vc4_dpi_encoder_helper_funcs);
ret = vc4_dpi_init_bridge(dpi);
diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
index 98308a17e4ed..8ca3b73e26dc 100644
--- a/drivers/gpu/drm/vc4/vc4_dsi.c
+++ b/drivers/gpu/drm/vc4/vc4_dsi.c
@@ -1498,10 +1498,11 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data)
dsi->variant = of_device_get_match_data(dev);
- vc4_dsi_encoder = devm_kzalloc(dev, sizeof(*vc4_dsi_encoder),
- GFP_KERNEL);
- if (!vc4_dsi_encoder)
- return -ENOMEM;
+ vc4_dsi_encoder = drmm_simple_encoder_alloc(drm, struct vc4_dsi_encoder,
+ base.base,
+ DRM_MODE_ENCODER_DSI);
+ if (IS_ERR(vc4_dsi_encoder))
+ return PTR_ERR(vc4_dsi_encoder);
INIT_LIST_HEAD(&dsi->bridge_chain);
vc4_dsi_encoder->base.type = VC4_ENCODER_TYPE_DSI1;
@@ -1614,7 +1615,6 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data)
if (ret)
return ret;
- drm_simple_encoder_init(drm, dsi->encoder, DRM_MODE_ENCODER_DSI);
drm_encoder_helper_add(dsi->encoder, &vc4_dsi_encoder_helper_funcs);
ret = drm_bridge_attach(dsi->encoder, dsi->bridge, NULL, 0);
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index ce9d16666d91..4657b09649f7 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -2834,9 +2834,12 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
struct device_node *ddc_node;
int ret;
- vc4_hdmi = devm_kzalloc(dev, sizeof(*vc4_hdmi), GFP_KERNEL);
- if (!vc4_hdmi)
- return -ENOMEM;
+ vc4_hdmi = drmm_simple_encoder_alloc(drm, struct vc4_hdmi,
+ encoder.base,
+ DRM_MODE_ENCODER_DSI);
+ if (IS_ERR(vc4_hdmi))
+ return PTR_ERR(vc4_hdmi);
+
mutex_init(&vc4_hdmi->mutex);
spin_lock_init(&vc4_hdmi->hw_lock);
INIT_DELAYED_WORK(&vc4_hdmi->scrambling_work, vc4_hdmi_scrambling_wq);
@@ -2921,7 +2924,6 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
clk_prepare_enable(vc4_hdmi->pixel_bvb_clock);
}
- drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_TMDS);
drm_encoder_helper_add(encoder, &vc4_hdmi_encoder_helper_funcs);
ret = vc4_hdmi_connector_init(drm, vc4_hdmi);
diff --git a/drivers/gpu/drm/vc4/vc4_vec.c b/drivers/gpu/drm/vc4/vc4_vec.c
index 11fc3d6f66b1..d6466f1ef490 100644
--- a/drivers/gpu/drm/vc4/vc4_vec.c
+++ b/drivers/gpu/drm/vc4/vc4_vec.c
@@ -546,10 +546,12 @@ static int vc4_vec_bind(struct device *dev, struct device *master, void *data)
if (!vec)
return -ENOMEM;
- vc4_vec_encoder = devm_kzalloc(dev, sizeof(*vc4_vec_encoder),
- GFP_KERNEL);
- if (!vc4_vec_encoder)
- return -ENOMEM;
+ vc4_vec_encoder = drmm_simple_encoder_alloc(drm, struct vc4_vec_encoder,
+ base.base,
+ DRM_MODE_ENCODER_TVDAC);
+ if (IS_ERR(vc4_vec_encoder))
+ return PTR_ERR(vc4_vec_encoder);
+
vc4_vec_encoder->base.type = VC4_ENCODER_TYPE_VEC;
vc4_vec_encoder->vec = vec;
vec->encoder = &vc4_vec_encoder->base.base;
@@ -574,7 +576,6 @@ static int vc4_vec_bind(struct device *dev, struct device *master, void *data)
pm_runtime_enable(dev);
- drm_simple_encoder_init(drm, vec->encoder, DRM_MODE_ENCODER_TVDAC);
drm_encoder_helper_add(vec->encoder, &vc4_vec_encoder_helper_funcs);
vec->connector = vc4_vec_connector_init(drm, vec);
--
2.36.1
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 0/3] drm/vc4: use drm managed resources
2022-07-13 8:54 [PATCH 0/3] drm/vc4: use drm managed resources Danilo Krummrich
` (2 preceding siblings ...)
2022-07-13 8:55 ` [PATCH 3/3] drm/vc4: encoder: " Danilo Krummrich
@ 2022-07-13 9:10 ` Maxime Ripard
2022-07-14 14:27 ` Danilo Krummrich
3 siblings, 1 reply; 6+ messages in thread
From: Maxime Ripard @ 2022-07-13 9:10 UTC (permalink / raw)
To: Danilo Krummrich; +Cc: emma, daniel, airlied, dri-devel, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 684 bytes --]
Hi Danilo,
On Wed, Jul 13, 2022 at 10:54:57AM +0200, Danilo Krummrich wrote:
> This patch series converts DRM modeset object allocations from devm_*()
> to drmm_*() memory allocators, or their corresponding convenience
> wrappers, respectively, in order to tie the release action to the
> underlaying struct drm_device.
>
> This can prevent potential use-after free issues on driver unload or
> EPROBE_DEFERRED backoff.
Yeah, the driver had a lot of this kind of issues.
As it turns out, at the moment you sent it, I was applying a larger
series (hopefully) addressing all of them:
https://lore.kernel.org/all/20220711173939.1132294-1-maxime@cerno.tech/
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 0/3] drm/vc4: use drm managed resources
2022-07-13 9:10 ` [PATCH 0/3] drm/vc4: " Maxime Ripard
@ 2022-07-14 14:27 ` Danilo Krummrich
0 siblings, 0 replies; 6+ messages in thread
From: Danilo Krummrich @ 2022-07-14 14:27 UTC (permalink / raw)
To: Maxime Ripard; +Cc: airlied, dri-devel, emma, linux-kernel
Hi Maxime,
On 7/13/22 11:10, Maxime Ripard wrote:
> Hi Danilo,
>
> On Wed, Jul 13, 2022 at 10:54:57AM +0200, Danilo Krummrich wrote:
>> This patch series converts DRM modeset object allocations from devm_*()
>> to drmm_*() memory allocators, or their corresponding convenience
>> wrappers, respectively, in order to tie the release action to the
>> underlaying struct drm_device.
>>
>> This can prevent potential use-after free issues on driver unload or
>> EPROBE_DEFERRED backoff.
>
> Yeah, the driver had a lot of this kind of issues.
>
> As it turns out, at the moment you sent it, I was applying a larger
> series (hopefully) addressing all of them:
> https://lore.kernel.org/all/20220711173939.1132294-1-maxime@cerno.tech/
Ah, great! That's covering even more than the series I sent.
>
> Maxime
- Danilo
^ permalink raw reply [flat|nested] 6+ messages in thread