If we don't call drm_connector_cleanup() manually in panel_bridge_detach(), the connector will be cleaned up with the other DRM objects in the call to drm_mode_config_cleanup(). However, since our drm_connector is devm-allocated, by the time drm_mode_config_cleanup() will be called, our connector will be long gone. Therefore, the connector must be cleaned up when the bridge is detached to avoid use-after-free conditions.
v2: Cleanup connector only if it was created
Fixes: 13dfc0540a57 ("drm/bridge: Refactor out the panel wrapper from the lvds-encoder bridge.") Cc: stable@vger.kernel.org # 4.12+ Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@siol.net Signed-off-by: Paul Cercueil paul@crapouillou.net --- drivers/gpu/drm/bridge/panel.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c index 0ddc37551194..df86b0ee0549 100644 --- a/drivers/gpu/drm/bridge/panel.c +++ b/drivers/gpu/drm/bridge/panel.c @@ -87,6 +87,12 @@ static int panel_bridge_attach(struct drm_bridge *bridge,
static void panel_bridge_detach(struct drm_bridge *bridge) { + struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); + struct drm_connector *connector = &panel_bridge->connector; + + /* Cleanup the connector if we know it was initialized */ + if (!!panel_bridge->connector.dev) + drm_connector_cleanup(connector); }
static void panel_bridge_pre_enable(struct drm_bridge *bridge)
On Wed, Jan 20, 2021 at 1:35 PM Paul Cercueil paul@crapouillou.net wrote:
If we don't call drm_connector_cleanup() manually in panel_bridge_detach(), the connector will be cleaned up with the other DRM objects in the call to drm_mode_config_cleanup(). However, since our drm_connector is devm-allocated, by the time drm_mode_config_cleanup() will be called, our connector will be long gone. Therefore, the connector must be cleaned up when the bridge is detached to avoid use-after-free conditions.
For -fixes this sounds ok, but for -next I think switching to drmm_ would be much better. -Daniel
v2: Cleanup connector only if it was created
Fixes: 13dfc0540a57 ("drm/bridge: Refactor out the panel wrapper from the lvds-encoder bridge.") Cc: stable@vger.kernel.org # 4.12+ Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@siol.net Signed-off-by: Paul Cercueil paul@crapouillou.net
drivers/gpu/drm/bridge/panel.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c index 0ddc37551194..df86b0ee0549 100644 --- a/drivers/gpu/drm/bridge/panel.c +++ b/drivers/gpu/drm/bridge/panel.c @@ -87,6 +87,12 @@ static int panel_bridge_attach(struct drm_bridge *bridge,
static void panel_bridge_detach(struct drm_bridge *bridge) {
struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);struct drm_connector *connector = &panel_bridge->connector;/* Cleanup the connector if we know it was initialized */if (!!panel_bridge->connector.dev)drm_connector_cleanup(connector);}
static void panel_bridge_pre_enable(struct drm_bridge *bridge)
2.29.2
Le mer. 20 janv. 2021 à 17:03, Daniel Vetter daniel@ffwll.ch a écrit :
On Wed, Jan 20, 2021 at 1:35 PM Paul Cercueil paul@crapouillou.net wrote:
If we don't call drm_connector_cleanup() manually in panel_bridge_detach(), the connector will be cleaned up with the other DRM objects in the call to drm_mode_config_cleanup(). However, since our drm_connector is devm-allocated, by the time drm_mode_config_cleanup() will be called, our connector will be long gone. Therefore, the connector must be cleaned up when the bridge is detached to avoid use-after-free conditions.
For -fixes this sounds ok, but for -next I think switching to drmm_ would be much better.
The API would need to change to have access to the drm_device struct, though. That would be quite a big patch, there are a few dozens source files that use this API already.
Cheers, -Paul
v2: Cleanup connector only if it was created
Fixes: 13dfc0540a57 ("drm/bridge: Refactor out the panel wrapper from the lvds-encoder bridge.") Cc: stable@vger.kernel.org # 4.12+ Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@siol.net Signed-off-by: Paul Cercueil paul@crapouillou.net
drivers/gpu/drm/bridge/panel.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c index 0ddc37551194..df86b0ee0549 100644 --- a/drivers/gpu/drm/bridge/panel.c +++ b/drivers/gpu/drm/bridge/panel.c @@ -87,6 +87,12 @@ static int panel_bridge_attach(struct drm_bridge *bridge,
static void panel_bridge_detach(struct drm_bridge *bridge) {
struct panel_bridge *panel_bridge =drm_bridge_to_panel_bridge(bridge);
struct drm_connector *connector = &panel_bridge->connector;/* Cleanup the connector if we know it was initialized */if (!!panel_bridge->connector.dev)drm_connector_cleanup(connector);}
static void panel_bridge_pre_enable(struct drm_bridge *bridge)
2.29.2
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
On Wed, Jan 20, 2021 at 6:12 PM Paul Cercueil paul@crapouillou.net wrote:
Le mer. 20 janv. 2021 à 17:03, Daniel Vetter daniel@ffwll.ch a écrit :
On Wed, Jan 20, 2021 at 1:35 PM Paul Cercueil paul@crapouillou.net wrote:
If we don't call drm_connector_cleanup() manually in panel_bridge_detach(), the connector will be cleaned up with the other DRM objects in the call to drm_mode_config_cleanup(). However, since our drm_connector is devm-allocated, by the time drm_mode_config_cleanup() will be called, our connector will be long gone. Therefore, the connector must be cleaned up when the bridge is detached to avoid use-after-free conditions.
For -fixes this sounds ok, but for -next I think switching to drmm_ would be much better.
The API would need to change to have access to the drm_device struct, though. That would be quite a big patch, there are a few dozens source files that use this API already.
Hm right pure drmm_ doesn't work for panel or bridge since it's usually a separate driver. But devm_ also doesn't work. I think what we need here is two-stage: first kmalloc the panel (or bridge, it's really the same) in the panel/bridge driver load. Then when we bind it to the drm_device we can tie it into the managed resources with drmm_add_action_or_reset. Passing the drm_device to the point where we allocate the panel/bridge doesn't work for these.
I think minimally we need a FIXME here and ack from Laurent on how this should be solved at least, since panel bridge is used rather widely. -Daniel
Cheers, -Paul
v2: Cleanup connector only if it was created
Fixes: 13dfc0540a57 ("drm/bridge: Refactor out the panel wrapper from the lvds-encoder bridge.") Cc: stable@vger.kernel.org # 4.12+ Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@siol.net Signed-off-by: Paul Cercueil paul@crapouillou.net
drivers/gpu/drm/bridge/panel.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c index 0ddc37551194..df86b0ee0549 100644 --- a/drivers/gpu/drm/bridge/panel.c +++ b/drivers/gpu/drm/bridge/panel.c @@ -87,6 +87,12 @@ static int panel_bridge_attach(struct drm_bridge *bridge,
static void panel_bridge_detach(struct drm_bridge *bridge) {
struct panel_bridge *panel_bridge =drm_bridge_to_panel_bridge(bridge);
struct drm_connector *connector = &panel_bridge->connector;/* Cleanup the connector if we know it was initialized */if (!!panel_bridge->connector.dev)drm_connector_cleanup(connector);}
static void panel_bridge_pre_enable(struct drm_bridge *bridge)
2.29.2
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
On Wed, Jan 20, 2021 at 06:38:03PM +0100, Daniel Vetter wrote:
On Wed, Jan 20, 2021 at 6:12 PM Paul Cercueil wrote:
Le mer. 20 janv. 2021 à 17:03, Daniel Vetter a écrit :
On Wed, Jan 20, 2021 at 1:35 PM Paul Cercueil wrote:
If we don't call drm_connector_cleanup() manually in panel_bridge_detach(), the connector will be cleaned up with the other DRM objects in the call to drm_mode_config_cleanup(). However, since our drm_connector is devm-allocated, by the time drm_mode_config_cleanup() will be called, our connector will be long gone. Therefore, the connector must be cleaned up when the bridge is detached to avoid use-after-free conditions.
For -fixes this sounds ok, but for -next I think switching to drmm_ would be much better.
The API would need to change to have access to the drm_device struct, though. That would be quite a big patch, there are a few dozens source files that use this API already.
Hm right pure drmm_ doesn't work for panel or bridge since it's usually a separate driver. But devm_ also doesn't work. I think what we need here is two-stage: first kmalloc the panel (or bridge, it's really the same) in the panel/bridge driver load. Then when we bind it to the drm_device we can tie it into the managed resources with drmm_add_action_or_reset. Passing the drm_device to the point where we allocate the panel/bridge doesn't work for these.
I think minimally we need a FIXME here and ack from Laurent on how this should be solved at least, since panel bridge is used rather widely.
Bridge removal is completely broken. If you unbind a bridge driver from the device, the bridge will be unregistered and resources freed, without the display driver knowing about this. The lifetime of the drm_bridge structure itself isn't the only issue to be addressed here, it's broader than that, and needs to consider that the display driver could be calling the bridge operations concurrently to the removal.
We need a volunteer with enough motivation to solve this subsystem-wide :-) In the meantime, whatever shortcut addresses immediate issues is probably fine, as yak-shaving in this area would definitely not be reasonable.
v2: Cleanup connector only if it was created
Fixes: 13dfc0540a57 ("drm/bridge: Refactor out the panel wrapper from the lvds-encoder bridge.") Cc: stable@vger.kernel.org # 4.12+ Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@siol.net Signed-off-by: Paul Cercueil paul@crapouillou.net
drivers/gpu/drm/bridge/panel.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c index 0ddc37551194..df86b0ee0549 100644 --- a/drivers/gpu/drm/bridge/panel.c +++ b/drivers/gpu/drm/bridge/panel.c @@ -87,6 +87,12 @@ static int panel_bridge_attach(struct drm_bridge *bridge,
static void panel_bridge_detach(struct drm_bridge *bridge) {
- struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
- struct drm_connector *connector = &panel_bridge->connector;
- /* Cleanup the connector if we know it was initialized */
- if (!!panel_bridge->connector.dev)
drm_connector_cleanup(connector);}
static void panel_bridge_pre_enable(struct drm_bridge *bridge)
On Wed, Mar 24, 2021 at 04:15:37AM +0200, Laurent Pinchart wrote:
On Wed, Jan 20, 2021 at 06:38:03PM +0100, Daniel Vetter wrote:
On Wed, Jan 20, 2021 at 6:12 PM Paul Cercueil wrote:
Le mer. 20 janv. 2021 à 17:03, Daniel Vetter a écrit :
On Wed, Jan 20, 2021 at 1:35 PM Paul Cercueil wrote:
If we don't call drm_connector_cleanup() manually in panel_bridge_detach(), the connector will be cleaned up with the other DRM objects in the call to drm_mode_config_cleanup(). However, since our drm_connector is devm-allocated, by the time drm_mode_config_cleanup() will be called, our connector will be long gone. Therefore, the connector must be cleaned up when the bridge is detached to avoid use-after-free conditions.
For -fixes this sounds ok, but for -next I think switching to drmm_ would be much better.
The API would need to change to have access to the drm_device struct, though. That would be quite a big patch, there are a few dozens source files that use this API already.
Hm right pure drmm_ doesn't work for panel or bridge since it's usually a separate driver. But devm_ also doesn't work. I think what we need here is two-stage: first kmalloc the panel (or bridge, it's really the same) in the panel/bridge driver load. Then when we bind it to the drm_device we can tie it into the managed resources with drmm_add_action_or_reset. Passing the drm_device to the point where we allocate the panel/bridge doesn't work for these.
I think minimally we need a FIXME here and ack from Laurent on how this should be solved at least, since panel bridge is used rather widely.
Bridge removal is completely broken. If you unbind a bridge driver from the device, the bridge will be unregistered and resources freed, without the display driver knowing about this. The lifetime of the drm_bridge structure itself isn't the only issue to be addressed here, it's broader than that, and needs to consider that the display driver could be calling the bridge operations concurrently to the removal.
So for the "unloading bridge should first unload display" problem that was supposed to get fixed with device links. There was at least a patch for that, and I Rafel from pm side did all the core changes to make it work. But it didn't land I think, so things keep on sucking.
Ofc the lifetime of the bridge structure is then an additional problem on top here.
We need a volunteer with enough motivation to solve this subsystem-wide :-) In the meantime, whatever shortcut addresses immediate issues is probably fine, as yak-shaving in this area would definitely not be reasonable.
I guess drm/bridge keeps on disappointing :-/ -Daniel
v2: Cleanup connector only if it was created
Fixes: 13dfc0540a57 ("drm/bridge: Refactor out the panel wrapper from the lvds-encoder bridge.") Cc: stable@vger.kernel.org # 4.12+ Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@siol.net Signed-off-by: Paul Cercueil paul@crapouillou.net
drivers/gpu/drm/bridge/panel.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c index 0ddc37551194..df86b0ee0549 100644 --- a/drivers/gpu/drm/bridge/panel.c +++ b/drivers/gpu/drm/bridge/panel.c @@ -87,6 +87,12 @@ static int panel_bridge_attach(struct drm_bridge *bridge,
static void panel_bridge_detach(struct drm_bridge *bridge) {
- struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
- struct drm_connector *connector = &panel_bridge->connector;
- /* Cleanup the connector if we know it was initialized */
- if (!!panel_bridge->connector.dev)
drm_connector_cleanup(connector);}
static void panel_bridge_pre_enable(struct drm_bridge *bridge)
-- Regards,
Laurent Pinchart
On Wed, Mar 24, 2021 at 10:39:52AM +0100, Daniel Vetter wrote:
On Wed, Mar 24, 2021 at 04:15:37AM +0200, Laurent Pinchart wrote:
On Wed, Jan 20, 2021 at 06:38:03PM +0100, Daniel Vetter wrote:
On Wed, Jan 20, 2021 at 6:12 PM Paul Cercueil wrote:
Le mer. 20 janv. 2021 à 17:03, Daniel Vetter a écrit :
On Wed, Jan 20, 2021 at 1:35 PM Paul Cercueil wrote:
If we don't call drm_connector_cleanup() manually in panel_bridge_detach(), the connector will be cleaned up with the other DRM objects in the call to drm_mode_config_cleanup(). However, since our drm_connector is devm-allocated, by the time drm_mode_config_cleanup() will be called, our connector will be long gone. Therefore, the connector must be cleaned up when the bridge is detached to avoid use-after-free conditions.
For -fixes this sounds ok, but for -next I think switching to drmm_ would be much better.
The API would need to change to have access to the drm_device struct, though. That would be quite a big patch, there are a few dozens source files that use this API already.
Hm right pure drmm_ doesn't work for panel or bridge since it's usually a separate driver. But devm_ also doesn't work. I think what we need here is two-stage: first kmalloc the panel (or bridge, it's really the same) in the panel/bridge driver load. Then when we bind it to the drm_device we can tie it into the managed resources with drmm_add_action_or_reset. Passing the drm_device to the point where we allocate the panel/bridge doesn't work for these.
I think minimally we need a FIXME here and ack from Laurent on how this should be solved at least, since panel bridge is used rather widely.
Bridge removal is completely broken. If you unbind a bridge driver from the device, the bridge will be unregistered and resources freed, without the display driver knowing about this. The lifetime of the drm_bridge structure itself isn't the only issue to be addressed here, it's broader than that, and needs to consider that the display driver could be calling the bridge operations concurrently to the removal.
So for the "unloading bridge should first unload display" problem that was supposed to get fixed with device links. There was at least a patch for that, and I Rafel from pm side did all the core changes to make it work. But it didn't land I think, so things keep on sucking.
Ofc the lifetime of the bridge structure is then an additional problem on top here.
There's a set of interesting problems. I don't think it's impossible, but it will require someone with a good understanding of the problem (as that person would really need to see the big picture, and take all use cases into account), and a large amount of time and motivation.
We need a volunteer with enough motivation to solve this subsystem-wide :-) In the meantime, whatever shortcut addresses immediate issues is probably fine, as yak-shaving in this area would definitely not be reasonable.
I guess drm/bridge keeps on disappointing :-/
I usually blame the x86 folks for not caring enough about bridges initially, resulting in it being a second class citizen ;-)
v2: Cleanup connector only if it was created
Fixes: 13dfc0540a57 ("drm/bridge: Refactor out the panel wrapper from the lvds-encoder bridge.") Cc: stable@vger.kernel.org # 4.12+ Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@siol.net Signed-off-by: Paul Cercueil paul@crapouillou.net
drivers/gpu/drm/bridge/panel.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c index 0ddc37551194..df86b0ee0549 100644 --- a/drivers/gpu/drm/bridge/panel.c +++ b/drivers/gpu/drm/bridge/panel.c @@ -87,6 +87,12 @@ static int panel_bridge_attach(struct drm_bridge *bridge,
static void panel_bridge_detach(struct drm_bridge *bridge) {
- struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
- struct drm_connector *connector = &panel_bridge->connector;
- /* Cleanup the connector if we know it was initialized */
- if (!!panel_bridge->connector.dev)
drm_connector_cleanup(connector);}
static void panel_bridge_pre_enable(struct drm_bridge *bridge)
linux-stable-mirror@lists.linaro.org
-
Daniel Vetter -
Laurent Pinchart -
Paul Cercueil