drivers/gpu/drm/display/drm_bridge_connector.c | 114 +++++++++++++++++-------- 1 file changed, 78 insertions(+), 36 deletions(-)
drm_bridge_connector_init() takes eight pointers to various bridges, some
of which can be identical, and stores them in pointers inside struct
drm_bridge_connector. Get a reference to each of the taken bridges and put
it on cleanup.
This is tricky because the pointers are currently stored directly in the
drm_bridge_connector in the loop, but there is no nice and clean way to put
those pointers on error return paths. To overcome this, store all pointers
in temporary local variables with a cleanup action, and only on success
copy them into struct drm_bridge_connector (getting another ref while
copying).
Additionally four of these pointers (edid, hpd, detect and modes) can be
written in multiple loop iterations, in order to eventually store the last
matching bridge. However, when one of those pointers is overwritten, we
need to put the reference that we got during the previous assignment. Add a
drm_bridge_put() before writing them to handle this.
Finally, there is also a function-local panel_bridge pointer taken inside
the loop and used after the loop. Use a cleanup action as well to ensure it
is put on return.
Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
This series ensures the bridge-connector gets a reference to bridges when
storing a pointer to them, and releases them afterwards.
This is part of the work towards removal of bridges from a still existing
DRM pipeline without use-after-free. The grand plan was discussed in [1].
Here's the work breakdown (➜ marks the current series):
1. ➜ add refcounting to DRM bridges (struct drm_bridge)
(based on devm_drm_bridge_alloc() [0])
A. ✔ add new alloc API and refcounting (v6.16)
B. ✔ convert all bridge drivers to new API (v6.17)
C. ✔ kunit tests (v6.17)
D. ✔ add get/put to drm_bridge_add/remove() + attach/detach()
and warn on old allocation pattern (v6.17)
E. … add get/put on drm_bridge accessors
1. ✔ drm_bridge_chain_get_first_bridge() + add a cleanup action
(drm-misc-next)
2. ✔ drm_bridge_get_prev_bridge() (drm-misc-next)
3. ✔ drm_bridge_get_next_bridge() (drm-misc-next)
4. ✔ drm_for_each_bridge_in_chain() (drm-misc-next)
5. ➜ drm_bridge_connector_init
6. protect encoder bridge chain with a mutex
7. of_drm_find_bridge
8. drm_of_find_panel_or_bridge, *_of_get_bridge
F. ➜ debugfs improvements
1. ✔ add top-level 'bridges' file (v6.16)
2. ✔ show refcount and list removed bridges (drm-misc-next)
2. … handle gracefully atomic updates during bridge removal
3. … DSI host-device driver interaction
4. removing the need for the "always-disconnected" connector
5. finish the hotplug bridge work, moving code to the core and potentially
removing the hotplug-bridge itself (this needs to be clarified as
points 1-3 are developed)
This was tricky both because there is no central place in
drm_bridge_connector.c to put the references on disposal (handled by patch
1) and because of the complex code flow of drm_bridge_connector_init()
(handled by patch 2).
---
Changes in v2:
- Use drmm_add_action() instead of hacking the .destroy connector func
- Removed patch 1 (where the hacking the .destroy connector func was)
- Link to v1: https://lore.kernel.org/r/20250925-drm-bridge-alloc-getput-bridge-connector-v1-0-f0736e1c73ee@bootlin.com
---
drivers/gpu/drm/display/drm_bridge_connector.c | 114 +++++++++++++++++--------
1 file changed, 78 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
index a5bdd6c1064399ece6b19560f145b877c9e0680e..7b18be3ff9a32b362468351835bdab43c3f524f1 100644
--- a/drivers/gpu/drm/display/drm_bridge_connector.c
+++ b/drivers/gpu/drm/display/drm_bridge_connector.c
@@ -618,6 +618,20 @@ static const struct drm_connector_hdmi_cec_funcs drm_bridge_connector_hdmi_cec_f
* Bridge Connector Initialisation
*/
+static void drm_bridge_connector_put_bridges(struct drm_device *dev, void *data)
+{
+ struct drm_bridge_connector *bridge_connector = (struct drm_bridge_connector *)data;
+
+ drm_bridge_put(bridge_connector->bridge_edid);
+ drm_bridge_put(bridge_connector->bridge_hpd);
+ drm_bridge_put(bridge_connector->bridge_detect);
+ drm_bridge_put(bridge_connector->bridge_modes);
+ drm_bridge_put(bridge_connector->bridge_hdmi);
+ drm_bridge_put(bridge_connector->bridge_hdmi_audio);
+ drm_bridge_put(bridge_connector->bridge_dp_audio);
+ drm_bridge_put(bridge_connector->bridge_hdmi_cec);
+}
+
/**
* drm_bridge_connector_init - Initialise a connector for a chain of bridges
* @drm: the DRM device
@@ -638,7 +652,15 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
struct drm_bridge_connector *bridge_connector;
struct drm_connector *connector;
struct i2c_adapter *ddc = NULL;
- struct drm_bridge *panel_bridge = NULL;
+ struct drm_bridge *panel_bridge __free(drm_bridge_put) = NULL;
+ struct drm_bridge *bridge_edid __free(drm_bridge_put) = NULL;
+ struct drm_bridge *bridge_hpd __free(drm_bridge_put) = NULL;
+ struct drm_bridge *bridge_detect __free(drm_bridge_put) = NULL;
+ struct drm_bridge *bridge_modes __free(drm_bridge_put) = NULL;
+ struct drm_bridge *bridge_hdmi __free(drm_bridge_put) = NULL;
+ struct drm_bridge *bridge_hdmi_audio __free(drm_bridge_put) = NULL;
+ struct drm_bridge *bridge_dp_audio __free(drm_bridge_put) = NULL;
+ struct drm_bridge *bridge_hdmi_cec __free(drm_bridge_put) = NULL;
unsigned int supported_formats = BIT(HDMI_COLORSPACE_RGB);
unsigned int max_bpc = 8;
bool support_hdcp = false;
@@ -649,6 +671,10 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
if (!bridge_connector)
return ERR_PTR(-ENOMEM);
+ ret = drmm_add_action(drm, drm_bridge_connector_put_bridges, bridge_connector);
+ if (ret)
+ return ERR_PTR(ret);
+
bridge_connector->encoder = encoder;
/*
@@ -672,22 +698,30 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
if (!bridge->ycbcr_420_allowed)
connector->ycbcr_420_allowed = false;
- if (bridge->ops & DRM_BRIDGE_OP_EDID)
- bridge_connector->bridge_edid = bridge;
- if (bridge->ops & DRM_BRIDGE_OP_HPD)
- bridge_connector->bridge_hpd = bridge;
- if (bridge->ops & DRM_BRIDGE_OP_DETECT)
- bridge_connector->bridge_detect = bridge;
- if (bridge->ops & DRM_BRIDGE_OP_MODES)
- bridge_connector->bridge_modes = bridge;
+ if (bridge->ops & DRM_BRIDGE_OP_EDID) {
+ drm_bridge_put(bridge_edid);
+ bridge_edid = drm_bridge_get(bridge);
+ }
+ if (bridge->ops & DRM_BRIDGE_OP_HPD) {
+ drm_bridge_put(bridge_hpd);
+ bridge_hpd = drm_bridge_get(bridge);
+ }
+ if (bridge->ops & DRM_BRIDGE_OP_DETECT) {
+ drm_bridge_put(bridge_detect);
+ bridge_detect = drm_bridge_get(bridge);
+ }
+ if (bridge->ops & DRM_BRIDGE_OP_MODES) {
+ drm_bridge_put(bridge_modes);
+ bridge_modes = drm_bridge_get(bridge);
+ }
if (bridge->ops & DRM_BRIDGE_OP_HDMI) {
- if (bridge_connector->bridge_hdmi)
+ if (bridge_hdmi)
return ERR_PTR(-EBUSY);
if (!bridge->funcs->hdmi_write_infoframe ||
!bridge->funcs->hdmi_clear_infoframe)
return ERR_PTR(-EINVAL);
- bridge_connector->bridge_hdmi = bridge;
+ bridge_hdmi = drm_bridge_get(bridge);
if (bridge->supported_formats)
supported_formats = bridge->supported_formats;
@@ -696,10 +730,10 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
}
if (bridge->ops & DRM_BRIDGE_OP_HDMI_AUDIO) {
- if (bridge_connector->bridge_hdmi_audio)
+ if (bridge_hdmi_audio)
return ERR_PTR(-EBUSY);
- if (bridge_connector->bridge_dp_audio)
+ if (bridge_dp_audio)
return ERR_PTR(-EBUSY);
if (!bridge->hdmi_audio_max_i2s_playback_channels &&
@@ -710,14 +744,14 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
!bridge->funcs->hdmi_audio_shutdown)
return ERR_PTR(-EINVAL);
- bridge_connector->bridge_hdmi_audio = bridge;
+ bridge_hdmi_audio = drm_bridge_get(bridge);
}
if (bridge->ops & DRM_BRIDGE_OP_DP_AUDIO) {
- if (bridge_connector->bridge_dp_audio)
+ if (bridge_dp_audio)
return ERR_PTR(-EBUSY);
- if (bridge_connector->bridge_hdmi_audio)
+ if (bridge_hdmi_audio)
return ERR_PTR(-EBUSY);
if (!bridge->hdmi_audio_max_i2s_playback_channels &&
@@ -728,7 +762,7 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
!bridge->funcs->dp_audio_shutdown)
return ERR_PTR(-EINVAL);
- bridge_connector->bridge_dp_audio = bridge;
+ bridge_dp_audio = drm_bridge_get(bridge);
}
if (bridge->ops & DRM_BRIDGE_OP_HDMI_CEC_NOTIFIER) {
@@ -739,10 +773,10 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
}
if (bridge->ops & DRM_BRIDGE_OP_HDMI_CEC_ADAPTER) {
- if (bridge_connector->bridge_hdmi_cec)
+ if (bridge_hdmi_cec)
return ERR_PTR(-EBUSY);
- bridge_connector->bridge_hdmi_cec = bridge;
+ bridge_hdmi_cec = drm_bridge_get(bridge);
if (!bridge->funcs->hdmi_cec_enable ||
!bridge->funcs->hdmi_cec_log_addr ||
@@ -762,7 +796,7 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
ddc = bridge->ddc;
if (drm_bridge_is_panel(bridge))
- panel_bridge = bridge;
+ panel_bridge = drm_bridge_get(bridge);
if (bridge->support_hdcp)
support_hdcp = true;
@@ -771,13 +805,13 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
if (connector_type == DRM_MODE_CONNECTOR_Unknown)
return ERR_PTR(-EINVAL);
- if (bridge_connector->bridge_hdmi) {
+ if (bridge_hdmi) {
if (!connector->ycbcr_420_allowed)
supported_formats &= ~BIT(HDMI_COLORSPACE_YUV420);
ret = drmm_connector_hdmi_init(drm, connector,
- bridge_connector->bridge_hdmi->vendor,
- bridge_connector->bridge_hdmi->product,
+ bridge_hdmi->vendor,
+ bridge_hdmi->product,
&drm_bridge_connector_funcs,
&drm_bridge_connector_hdmi_funcs,
connector_type, ddc,
@@ -793,15 +827,14 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
return ERR_PTR(ret);
}
- if (bridge_connector->bridge_hdmi_audio ||
- bridge_connector->bridge_dp_audio) {
+ if (bridge_hdmi_audio || bridge_dp_audio) {
struct device *dev;
struct drm_bridge *bridge;
- if (bridge_connector->bridge_hdmi_audio)
- bridge = bridge_connector->bridge_hdmi_audio;
+ if (bridge_hdmi_audio)
+ bridge = bridge_hdmi_audio;
else
- bridge = bridge_connector->bridge_dp_audio;
+ bridge = bridge_dp_audio;
dev = bridge->hdmi_audio_dev;
@@ -815,9 +848,9 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
return ERR_PTR(ret);
}
- if (bridge_connector->bridge_hdmi_cec &&
- bridge_connector->bridge_hdmi_cec->ops & DRM_BRIDGE_OP_HDMI_CEC_NOTIFIER) {
- struct drm_bridge *bridge = bridge_connector->bridge_hdmi_cec;
+ if (bridge_hdmi_cec &&
+ bridge_hdmi_cec->ops & DRM_BRIDGE_OP_HDMI_CEC_NOTIFIER) {
+ struct drm_bridge *bridge = bridge_hdmi_cec;
ret = drmm_connector_hdmi_cec_notifier_register(connector,
NULL,
@@ -826,9 +859,9 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
return ERR_PTR(ret);
}
- if (bridge_connector->bridge_hdmi_cec &&
- bridge_connector->bridge_hdmi_cec->ops & DRM_BRIDGE_OP_HDMI_CEC_ADAPTER) {
- struct drm_bridge *bridge = bridge_connector->bridge_hdmi_cec;
+ if (bridge_hdmi_cec &&
+ bridge_hdmi_cec->ops & DRM_BRIDGE_OP_HDMI_CEC_ADAPTER) {
+ struct drm_bridge *bridge = bridge_hdmi_cec;
ret = drmm_connector_hdmi_cec_register(connector,
&drm_bridge_connector_hdmi_cec_funcs,
@@ -841,9 +874,9 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
drm_connector_helper_add(connector, &drm_bridge_connector_helper_funcs);
- if (bridge_connector->bridge_hpd)
+ if (bridge_hpd)
connector->polled = DRM_CONNECTOR_POLL_HPD;
- else if (bridge_connector->bridge_detect)
+ else if (bridge_detect)
connector->polled = DRM_CONNECTOR_POLL_CONNECT
| DRM_CONNECTOR_POLL_DISCONNECT;
@@ -854,6 +887,15 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
IS_ENABLED(CONFIG_DRM_DISPLAY_HDCP_HELPER))
drm_connector_attach_content_protection_property(connector, true);
+ bridge_connector->bridge_edid = drm_bridge_get(bridge_edid);
+ bridge_connector->bridge_hpd = drm_bridge_get(bridge_hpd);
+ bridge_connector->bridge_detect = drm_bridge_get(bridge_detect);
+ bridge_connector->bridge_modes = drm_bridge_get(bridge_modes);
+ bridge_connector->bridge_hdmi = drm_bridge_get(bridge_hdmi);
+ bridge_connector->bridge_hdmi_audio = drm_bridge_get(bridge_hdmi_audio);
+ bridge_connector->bridge_dp_audio = drm_bridge_get(bridge_dp_audio);
+ bridge_connector->bridge_hdmi_cec = drm_bridge_get(bridge_hdmi_cec);
+
return connector;
}
EXPORT_SYMBOL_GPL(drm_bridge_connector_init);
---
base-commit: 063db451832b8849faf1b0b8404b3a6a39995b29
change-id: 20250925-drm-bridge-alloc-getput-bridge-connector-556f8dc14af4
Best regards,
--
Luca Ceresoli <luca.ceresoli@bootlin.com>
Hi Luca,
On Sun, 28 Sept 2025 at 16:25, Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
> drm_bridge_connector_init() takes eight pointers to various bridges, some
> of which can be identical, and stores them in pointers inside struct
> drm_bridge_connector. Get a reference to each of the taken bridges and put
> it on cleanup.
>
> This is tricky because the pointers are currently stored directly in the
> drm_bridge_connector in the loop, but there is no nice and clean way to put
> those pointers on error return paths. To overcome this, store all pointers
> in temporary local variables with a cleanup action, and only on success
> copy them into struct drm_bridge_connector (getting another ref while
> copying).
>
> Additionally four of these pointers (edid, hpd, detect and modes) can be
> written in multiple loop iterations, in order to eventually store the last
> matching bridge. However, when one of those pointers is overwritten, we
> need to put the reference that we got during the previous assignment. Add a
> drm_bridge_put() before writing them to handle this.
>
> Finally, there is also a function-local panel_bridge pointer taken inside
> the loop and used after the loop. Use a cleanup action as well to ensure it
> is put on return.
>
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
Thanks for your patch, which is now commit 2be300f9a0b6f6b0
("drm/display: bridge_connector: get/put the stored bridges")
in drm-misc/drm-misc-next.
FTR, this causes the following crash on Koelsch (R-Car M2-W):
Unable to handle kernel NULL pointer dereference at virtual
address 00000050 when read
[00000050] *pgd=00000000
Internal error: Oops: 5 [#1] SMP ARM
CPU: 1 UID: 0 PID: 12 Comm: kworker/u8:0 Not tainted
6.17.0-rc6-shmobile-01124-g2be300f9a0b6 #2283 NONE
Hardware name: Generic R-Car Gen2 (Flattened Device Tree)
Workqueue: events_unbound deferred_probe_work_func
PC is at drm_bridge_connector_hdmi_cec_init+0x8/0x24
LR is at drmm_connector_hdmi_cec_register+0x104/0x1a8
pc : [<c0507240>] lr : [<c051460c>] psr: 60000013
sp : f0849c50 ip : 00000000 fp : 00000008
r10: c1f865c8 r9 : c1f84820 r8 : c1d32ecc
r7 : c1e4a980 r6 : c1f94000 r5 : c1d32840 r4 : c0a97930
r3 : c0507238 r2 : c151e040 r1 : c1d32840 r0 : 00000000
Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none
Control: 10c5387d Table: 4000406a DAC: 00000051
Register r0 information: NULL pointer
Register r1 information: slab kmalloc-2k start c1d32800 pointer
offset 64 size 2048
Register r2 information: slab task_struct start c151e040 pointer
offset 0 size 2240
Register r3 information: non-slab/vmalloc memory
Register r4 information: non-slab/vmalloc memory
Register r5 information: slab kmalloc-2k start c1d32800 pointer
offset 64 size 2048
Register r6 information: slab kmalloc-1k start c1f94000 pointer
offset 0 size 1024
Register r7 information: slab kmalloc-64 start c1e4a980 pointer
offset 0 size 64
Register r8 information: slab kmalloc-2k start c1d32800 pointer
offset 1740 size 2048
Register r9 information: slab kmalloc-1k start c1f84800 pointer
offset 32 size 1024
Register r10 information: slab kmalloc-1k start c1f86400 pointer
offset 456 size 1024
Register r11 information: non-paged memory
Register r12 information: NULL pointer
Process kworker/u8:0 (pid: 12, stack limit = 0x(ptrval))
Stack: (0xf0849c50 to 0xf084a000)
9c40: 00000003 00000011
00000001 00000000
9c60: 00000049 00000000 00000000 00000000 00000000 00000000
00000000 00000000
9c80: 00000000 00000000 00000000 00000000 00000000 00000000
00000000 a8bfa8e4
9ca0: c1d32840 c1d32840 c1f865c8 c1f865c8 00000000 c1d32840
00000000 c0507ba0
9cc0: c1f84820 00000011 00000001 00000002 00000001 00000008
c04fba80 c204800c
9ce0: 00000dc0 00000000 c1f865c8 c1f865c8 c1f865c8 00000000
c1f865c8 c1f865c8
9d00: 00000000 c1f865c8 00000000 a8bfa8e4 c1e42a00 00000000
00000000 c2048000
9d20: c1f865c8 c1e42a00 c204800c c0c66958 ef7f7b44 c0517ab4
00000000 00000000
9d40: c2048000 00000000 00000000 c204b000 ef7f0794 00000000
00000000 c0518ab8
9d60: 00000000 00000000 01ffffff 00000000 c204800c ef7f0214
f0f40000 c204f000
9d80: 00000000 00000000 ef7f0794 c2081f80 c0c7a60d c052d728
c1588c10 a0000013
9da0: c2081fc0 c052d780 f0f40000 c2081fc0 00040000 c1588c10
f0f40000 c03f57f4
9dc0: c0c4fe58 feb00000 c1588c10 c16683c0 00000000 a8bfa8e4
00000000 00000000
9de0: c2048000 c204800c c1588c00 00000000 c1588c10 00000000
c0fe8e30 c0517818
9e00: c1588c10 c0fe8670 c0fe8670 00000000 00000005 c140ed0d
61c88647 c052c148
9e20: 00000000 c1588c10 c0fe8670 c052a118 c1588c10 c0fe8670
f0849ecc c1588c10
9e40: 00000005 c052a3e8 c0fe8670 c1588c10 c10773d0 c10773d8
f0849ecc c1588c10
9e60: 00000005 61c88647 c0fe8e30 c052a490 00000001 c0fe8670
f0849ecc c1588c10
9e80: c102fc00 c052a568 00000000 c14ac400 f0849ecc c052a510
c102fc00 c05287fc
9ea0: c140ed0d c14ac46c c1594db8 a8bfa8e4 c1588c10 c1588c10
c14ac400 00000001
9ec0: c1588c54 c0529f80 c1588c10 c1588c10 00000001 a8bfa8e4
c14ac400 c1588c10
9ee0: c14ac400 c1588c10 00000000 c05290f4 c1588c10 c0fe8e10
c0fe8e68 00000000
9f00: c102fc00 c0529b3c c1496180 c140ed00 c1406600 c0fe8e2c
c102fc00 c01414ac
9f20: 00000002 a8bfa8e4 c151e040 c151e040 c1406620 c1406600
c140665c c1496180
9f40: c1406620 c1406600 c140665c c151e040 c14961ac c1030120
c0f03d00 c014173c
9f60: 00000000 c151e040 c1496400 c1494140 00000001 00000000
c01415cc c1496180
9f80: 00000000 c01498cc c1494140 a8bfa8e4 c1494140 c014976c
00000000 00000000
9fa0: 00000000 00000000 00000000 c010014c 00000000 00000000
00000000 00000000
9fc0: 00000000 00000000 00000000 00000000 00000000 00000000
00000000 00000000
9fe0: 00000000 00000000 00000000 00000000 00000013 00000000
00000000 00000000
Call trace:
drm_bridge_connector_hdmi_cec_init from
drmm_connector_hdmi_cec_register+0x104/0x1a8
drmm_connector_hdmi_cec_register from drm_bridge_connector_init+0x4d8/0x5e8
drm_bridge_connector_init from rcar_du_encoder_init+0x1e4/0x240
rcar_du_encoder_init from rcar_du_modeset_init+0x4f0/0x640
rcar_du_modeset_init from rcar_du_probe+0xe0/0x164
rcar_du_probe from platform_probe+0x58/0x90
platform_probe from really_probe+0x128/0x28c
really_probe from __driver_probe_device+0x16c/0x18c
__driver_probe_device from driver_probe_device+0x3c/0xbc
driver_probe_device from __device_attach_driver+0x58/0xbc
__device_attach_driver from bus_for_each_drv+0xc0/0xd4
bus_for_each_drv from __device_attach+0xec/0x154
__device_attach from bus_probe_device+0x2c/0x84
bus_probe_device from deferred_probe_work_func+0x80/0x98
deferred_probe_work_func from process_scheduled_works+0x1bc/0x2dc
process_scheduled_works from worker_thread+0x170/0x208
worker_thread from kthread+0x160/0x1fc
kthread from ret_from_fork+0x14/0x28
Exception stack(0xf0849fb0 to 0xf0849ff8)
9fa0: 00000000 00000000
00000000 00000000
9fc0: 00000000 00000000 00000000 00000000 00000000 00000000
00000000 00000000
9fe0: 00000000 00000000 00000000 00000000 00000013 00000000
Code: e49de004 e12fff1c e1a01000 e59006c8 (e5903050)
---[ end trace 0000000000000000 ]---
Applying "[PATCH v2 0/3] drm/display: bridge_connector: get/put the
stored bridges: fix NULL pointer regression"[1] fixes the issue.
[1] https://lore.kernel.org/20251017-drm-bridge-alloc-getput-bridge-connector-fix-hdmi_cec-v2-0-667abf6d47c0@bootlin.com/
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Fri, 26 Sep 2025 16:59:40 +0200, Luca Ceresoli wrote:
> drm_bridge_connector_init() takes eight pointers to various bridges, some
> of which can be identical, and stores them in pointers inside struct
> drm_bridge_connector. Get a reference to each of the taken bridges and put
> it on cleanup.
>
> This is tricky because the pointers are currently stored directly in the
> drm_bridge_connector in the loop, but there is no nice and clean way to put
> those pointers on error return paths. To overcome this, store all pointers
> in temporary local variables with a cleanup action, and only on success
> copy them into struct drm_bridge_connector (getting another ref while
> copying).
>
> [...]
Applied, thanks!
[1/1] drm/display: bridge_connector: get/put the stored bridges
commit: 2be300f9a0b6f6b0ae2a90be97e558ec0535be54
Best regards,
--
Luca Ceresoli <luca.ceresoli@bootlin.com>
On Fri, Sep 26, 2025 at 04:59:40PM +0200, Luca Ceresoli wrote:
> drm_bridge_connector_init() takes eight pointers to various bridges, some
> of which can be identical, and stores them in pointers inside struct
> drm_bridge_connector. Get a reference to each of the taken bridges and put
> it on cleanup.
>
> This is tricky because the pointers are currently stored directly in the
> drm_bridge_connector in the loop, but there is no nice and clean way to put
> those pointers on error return paths. To overcome this, store all pointers
> in temporary local variables with a cleanup action, and only on success
> copy them into struct drm_bridge_connector (getting another ref while
> copying).
>
> Additionally four of these pointers (edid, hpd, detect and modes) can be
> written in multiple loop iterations, in order to eventually store the last
> matching bridge. However, when one of those pointers is overwritten, we
> need to put the reference that we got during the previous assignment. Add a
> drm_bridge_put() before writing them to handle this.
>
> Finally, there is also a function-local panel_bridge pointer taken inside
> the loop and used after the loop. Use a cleanup action as well to ensure it
> is put on return.
>
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> ---
> This series ensures the bridge-connector gets a reference to bridges when
> storing a pointer to them, and releases them afterwards.
>
> This is part of the work towards removal of bridges from a still existing
> DRM pipeline without use-after-free. The grand plan was discussed in [1].
> Here's the work breakdown (➜ marks the current series):
>
> 1. ➜ add refcounting to DRM bridges (struct drm_bridge)
> (based on devm_drm_bridge_alloc() [0])
> A. ✔ add new alloc API and refcounting (v6.16)
> B. ✔ convert all bridge drivers to new API (v6.17)
> C. ✔ kunit tests (v6.17)
> D. ✔ add get/put to drm_bridge_add/remove() + attach/detach()
> and warn on old allocation pattern (v6.17)
> E. … add get/put on drm_bridge accessors
> 1. ✔ drm_bridge_chain_get_first_bridge() + add a cleanup action
> (drm-misc-next)
> 2. ✔ drm_bridge_get_prev_bridge() (drm-misc-next)
> 3. ✔ drm_bridge_get_next_bridge() (drm-misc-next)
> 4. ✔ drm_for_each_bridge_in_chain() (drm-misc-next)
> 5. ➜ drm_bridge_connector_init
> 6. protect encoder bridge chain with a mutex
> 7. of_drm_find_bridge
> 8. drm_of_find_panel_or_bridge, *_of_get_bridge
> F. ➜ debugfs improvements
> 1. ✔ add top-level 'bridges' file (v6.16)
> 2. ✔ show refcount and list removed bridges (drm-misc-next)
> 2. … handle gracefully atomic updates during bridge removal
> 3. … DSI host-device driver interaction
> 4. removing the need for the "always-disconnected" connector
> 5. finish the hotplug bridge work, moving code to the core and potentially
> removing the hotplug-bridge itself (this needs to be clarified as
> points 1-3 are developed)
>
> This was tricky both because there is no central place in
> drm_bridge_connector.c to put the references on disposal (handled by patch
> 1) and because of the complex code flow of drm_bridge_connector_init()
> (handled by patch 2).
> ---
> Changes in v2:
> - Use drmm_add_action() instead of hacking the .destroy connector func
> - Removed patch 1 (where the hacking the .destroy connector func was)
> - Link to v1: https://lore.kernel.org/r/20250925-drm-bridge-alloc-getput-bridge-connector-v1-0-f0736e1c73ee@bootlin.com
> ---
> drivers/gpu/drm/display/drm_bridge_connector.c | 114 +++++++++++++++++--------
> 1 file changed, 78 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
> index a5bdd6c1064399ece6b19560f145b877c9e0680e..7b18be3ff9a32b362468351835bdab43c3f524f1 100644
> --- a/drivers/gpu/drm/display/drm_bridge_connector.c
> +++ b/drivers/gpu/drm/display/drm_bridge_connector.c
> @@ -618,6 +618,20 @@ static const struct drm_connector_hdmi_cec_funcs drm_bridge_connector_hdmi_cec_f
> * Bridge Connector Initialisation
> */
>
> +static void drm_bridge_connector_put_bridges(struct drm_device *dev, void *data)
> +{
> + struct drm_bridge_connector *bridge_connector = (struct drm_bridge_connector *)data;
> +
> + drm_bridge_put(bridge_connector->bridge_edid);
> + drm_bridge_put(bridge_connector->bridge_hpd);
> + drm_bridge_put(bridge_connector->bridge_detect);
> + drm_bridge_put(bridge_connector->bridge_modes);
> + drm_bridge_put(bridge_connector->bridge_hdmi);
> + drm_bridge_put(bridge_connector->bridge_hdmi_audio);
> + drm_bridge_put(bridge_connector->bridge_dp_audio);
> + drm_bridge_put(bridge_connector->bridge_hdmi_cec);
> +}
> +
I'd rather have a drmm_bridge_get helper that register the action
itself, but that can be fixed later on.
Maxime
On Fri, Sep 26, 2025 at 04:59:40PM +0200, Luca Ceresoli wrote: > drm_bridge_connector_init() takes eight pointers to various bridges, some > of which can be identical, and stores them in pointers inside struct > drm_bridge_connector. Get a reference to each of the taken bridges and put > it on cleanup. > > This is tricky because the pointers are currently stored directly in the > drm_bridge_connector in the loop, but there is no nice and clean way to put > those pointers on error return paths. To overcome this, store all pointers > in temporary local variables with a cleanup action, and only on success > copy them into struct drm_bridge_connector (getting another ref while > copying). > > Additionally four of these pointers (edid, hpd, detect and modes) can be > written in multiple loop iterations, in order to eventually store the last > matching bridge. However, when one of those pointers is overwritten, we > need to put the reference that we got during the previous assignment. Add a > drm_bridge_put() before writing them to handle this. > > Finally, there is also a function-local panel_bridge pointer taken inside > the loop and used after the loop. Use a cleanup action as well to ensure it > is put on return. > > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> > --- > This series ensures the bridge-connector gets a reference to bridges when > storing a pointer to them, and releases them afterwards. > > This is part of the work towards removal of bridges from a still existing > DRM pipeline without use-after-free. The grand plan was discussed in [1]. > Here's the work breakdown (➜ marks the current series): > > 1. ➜ add refcounting to DRM bridges (struct drm_bridge) > (based on devm_drm_bridge_alloc() [0]) > A. ✔ add new alloc API and refcounting (v6.16) > B. ✔ convert all bridge drivers to new API (v6.17) > C. ✔ kunit tests (v6.17) > D. ✔ add get/put to drm_bridge_add/remove() + attach/detach() > and warn on old allocation pattern (v6.17) > E. … add get/put on drm_bridge accessors > 1. ✔ drm_bridge_chain_get_first_bridge() + add a cleanup action > (drm-misc-next) > 2. ✔ drm_bridge_get_prev_bridge() (drm-misc-next) > 3. ✔ drm_bridge_get_next_bridge() (drm-misc-next) > 4. ✔ drm_for_each_bridge_in_chain() (drm-misc-next) > 5. ➜ drm_bridge_connector_init > 6. protect encoder bridge chain with a mutex > 7. of_drm_find_bridge > 8. drm_of_find_panel_or_bridge, *_of_get_bridge > F. ➜ debugfs improvements > 1. ✔ add top-level 'bridges' file (v6.16) > 2. ✔ show refcount and list removed bridges (drm-misc-next) > 2. … handle gracefully atomic updates during bridge removal > 3. … DSI host-device driver interaction > 4. removing the need for the "always-disconnected" connector > 5. finish the hotplug bridge work, moving code to the core and potentially > removing the hotplug-bridge itself (this needs to be clarified as > points 1-3 are developed) > > This was tricky both because there is no central place in > drm_bridge_connector.c to put the references on disposal (handled by patch > 1) and because of the complex code flow of drm_bridge_connector_init() > (handled by patch 2). > --- > Changes in v2: > - Use drmm_add_action() instead of hacking the .destroy connector func > - Removed patch 1 (where the hacking the .destroy connector func was) > - Link to v1: https://lore.kernel.org/r/20250925-drm-bridge-alloc-getput-bridge-connector-v1-0-f0736e1c73ee@bootlin.com > --- > drivers/gpu/drm/display/drm_bridge_connector.c | 114 +++++++++++++++++-------- > 1 file changed, 78 insertions(+), 36 deletions(-) > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> -- With best wishes Dmitry
© 2016 - 2026 Red Hat, Inc.