* [PATCH v3] drm/cma-helper: Fix crash in fbdev error path
@ 2018-10-01 19:45 Noralf Trønnes
2018-10-02 11:14 ` Noralf Trønnes
0 siblings, 1 reply; 5+ messages in thread
From: Noralf Trønnes @ 2018-10-01 19:45 UTC (permalink / raw)
To: dri-devel; +Cc: Stefan Wahren, Daniel Vetter, ssuloev
Sergey Suloev reported a crash happening in drm_client_dev_hotplug()
when fbdev had failed to register.
[ 9.124598] vc4_hdmi 3f902000.hdmi: ASoC: Failed to create component debugfs directory
[ 9.147667] vc4_hdmi 3f902000.hdmi: vc4-hdmi-hifi <-> 3f902000.hdmi mapping ok
[ 9.155184] vc4_hdmi 3f902000.hdmi: ASoC: no DMI vendor name!
[ 9.166544] vc4-drm soc:gpu: bound 3f902000.hdmi (ops vc4_hdmi_ops [vc4])
[ 9.173840] vc4-drm soc:gpu: bound 3f806000.vec (ops vc4_vec_ops [vc4])
[ 9.181029] vc4-drm soc:gpu: bound 3f004000.txp (ops vc4_txp_ops [vc4])
[ 9.188519] vc4-drm soc:gpu: bound 3f400000.hvs (ops vc4_hvs_ops [vc4])
[ 9.195690] vc4-drm soc:gpu: bound 3f206000.pixelvalve (ops vc4_crtc_ops [vc4])
[ 9.203523] vc4-drm soc:gpu: bound 3f207000.pixelvalve (ops vc4_crtc_ops [vc4])
[ 9.215032] vc4-drm soc:gpu: bound 3f807000.pixelvalve (ops vc4_crtc_ops [vc4])
[ 9.274785] vc4-drm soc:gpu: bound 3fc00000.v3d (ops vc4_v3d_ops [vc4])
[ 9.290246] [drm] Initialized vc4 0.0.0 20140616 for soc:gpu on minor 0
[ 9.297464] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
[ 9.304600] [drm] Driver supports precise vblank timestamp query.
[ 9.382856] vc4-drm soc:gpu: [drm:drm_fb_helper_fbdev_setup [drm_kms_helper]] *ERROR* Failed to set fbdev configuration
[ 10.404937] Unable to handle kernel paging request at virtual address 00330a656369768a
[ 10.441620] [00330a656369768a] address between user and kernel address ranges
[ 10.449087] Internal error: Oops: 96000004 [#1] PREEMPT SMP
[ 10.454762] Modules linked in: brcmfmac vc4 drm_kms_helper cfg80211 drm rfkill smsc95xx brcmutil usbnet drm_panel_orientation_quirks raspberrypi_hwmon bcm2835_dma crc32_ce pwm_bcm2835 bcm2835_rng virt_dma rng_core i2c_bcm2835 ip_tables x_tables ipv6
[ 10.477296] CPU: 2 PID: 45 Comm: kworker/2:1 Not tainted 4.19.0-rc5 #3
[ 10.483934] Hardware name: Raspberry Pi 3 Model B Rev 1.2 (DT)
[ 10.489966] Workqueue: events output_poll_execute [drm_kms_helper]
[ 10.596515] Process kworker/2:1 (pid: 45, stack limit = 0x000000007e8924dc)
[ 10.603590] Call trace:
[ 10.606259] drm_client_dev_hotplug+0x5c/0xb0 [drm]
[ 10.611303] drm_kms_helper_hotplug_event+0x30/0x40 [drm_kms_helper]
[ 10.617849] output_poll_execute+0xc4/0x1e0 [drm_kms_helper]
[ 10.623616] process_one_work+0x1c8/0x318
[ 10.627695] worker_thread+0x48/0x428
[ 10.631420] kthread+0xf8/0x128
[ 10.634615] ret_from_fork+0x10/0x18
[ 10.638255] Code: 54000220 f9401261 aa1303e0 b4000141 (f9400c21)
[ 10.644456] ---[ end trace c75b4a4b0e141908 ]---
The reason for this is that drm_fbdev_cma_init() removes the drm_client
when fbdev registration fails, but it doesn't remove the client from the
drm_device client list. So the client list now has a pointer that points
into the unknown and we have a 'use after free' situation.
Split drm_client_new() into drm_client_init() and drm_client_add() to fix
removal in the error path.
Fixes: 894a677f4b3e ("drm/cma-helper: Use the generic fbdev emulation")
Reported-by: Sergey Suloev <ssuloev@orpaltech.com>
Cc: Stefan Wahren <stefan.wahren@i2se.com>
Cc: Eric Anholt <eric@anholt.net>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
Changes since version 2:
- Fix drm_client_new() ref in docs (Daniel)
- Expand drm_client_add() docs (Daniel)
Changes since version 1:
- Split drm_client_new() instead of letting drm_client hang around (Daniel)
drivers/gpu/drm/drm_client.c | 35 ++++++++++++++++++++++++++---------
drivers/gpu/drm/drm_fb_cma_helper.c | 4 +++-
drivers/gpu/drm/drm_fb_helper.c | 4 +++-
include/drm/drm_client.h | 5 +++--
4 files changed, 35 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
index 17d9a64e885e..fc03d26fcacc 100644
--- a/drivers/gpu/drm/drm_client.c
+++ b/drivers/gpu/drm/drm_client.c
@@ -63,20 +63,21 @@ static void drm_client_close(struct drm_client_dev *client)
EXPORT_SYMBOL(drm_client_close);
/**
- * drm_client_new - Create a DRM client
+ * drm_client_init - Initialise a DRM client
* @dev: DRM device
* @client: DRM client
* @name: Client name
* @funcs: DRM client functions (optional)
*
+ * This initialises the client and opens a &drm_file. Use drm_client_add() to complete the process.
* The caller needs to hold a reference on @dev before calling this function.
* The client is freed when the &drm_device is unregistered. See drm_client_release().
*
* Returns:
* Zero on success or negative error code on failure.
*/
-int drm_client_new(struct drm_device *dev, struct drm_client_dev *client,
- const char *name, const struct drm_client_funcs *funcs)
+int drm_client_init(struct drm_device *dev, struct drm_client_dev *client,
+ const char *name, const struct drm_client_funcs *funcs)
{
int ret;
@@ -95,10 +96,6 @@ int drm_client_new(struct drm_device *dev, struct drm_client_dev *client,
if (ret)
goto err_put_module;
- mutex_lock(&dev->clientlist_mutex);
- list_add(&client->list, &dev->clientlist);
- mutex_unlock(&dev->clientlist_mutex);
-
drm_dev_get(dev);
return 0;
@@ -109,13 +106,33 @@ int drm_client_new(struct drm_device *dev, struct drm_client_dev *client,
return ret;
}
-EXPORT_SYMBOL(drm_client_new);
+EXPORT_SYMBOL(drm_client_init);
+
+/**
+ * drm_client_add - Add client to the device list
+ * @client: DRM client
+ *
+ * Add the client to the &drm_device client list to activate its callbacks.
+ * @client must be initialized by a call to drm_client_init(). After
+ * drm_client_add() it is no longer permissible to call drm_client_release()
+ * directly (outside the unregister callback), instead cleanup will happen
+ * automatically on driver unload.
+ */
+void drm_client_add(struct drm_client_dev *client)
+{
+ struct drm_device *dev = client->dev;
+
+ mutex_lock(&dev->clientlist_mutex);
+ list_add(&client->list, &dev->clientlist);
+ mutex_unlock(&dev->clientlist_mutex);
+}
+EXPORT_SYMBOL(drm_client_add);
/**
* drm_client_release - Release DRM client resources
* @client: DRM client
*
- * Releases resources by closing the &drm_file that was opened by drm_client_new().
+ * Releases resources by closing the &drm_file that was opened by drm_client_init().
* It is called automatically if the &drm_client_funcs.unregister callback is _not_ set.
*
* This function should only be called from the unregister callback. An exception
diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
index 47e0e2f6642d..fb0dfc62b1b6 100644
--- a/drivers/gpu/drm/drm_fb_cma_helper.c
+++ b/drivers/gpu/drm/drm_fb_cma_helper.c
@@ -167,7 +167,7 @@ struct drm_fbdev_cma *drm_fbdev_cma_init(struct drm_device *dev,
fb_helper = &fbdev_cma->fb_helper;
- ret = drm_client_new(dev, &fb_helper->client, "fbdev", NULL);
+ ret = drm_client_init(dev, &fb_helper->client, "fbdev", NULL);
if (ret)
goto err_free;
@@ -176,6 +176,8 @@ struct drm_fbdev_cma *drm_fbdev_cma_init(struct drm_device *dev,
if (ret)
goto err_client_put;
+ drm_client_add(&fb_helper->client);
+
return fbdev_cma;
err_client_put:
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index a504a5e05676..e9095537e0c0 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -3229,13 +3229,15 @@ int drm_fbdev_generic_setup(struct drm_device *dev, unsigned int preferred_bpp)
if (!fb_helper)
return -ENOMEM;
- ret = drm_client_new(dev, &fb_helper->client, "fbdev", &drm_fbdev_client_funcs);
+ ret = drm_client_init(dev, &fb_helper->client, "fbdev", &drm_fbdev_client_funcs);
if (ret) {
kfree(fb_helper);
DRM_DEV_ERROR(dev->dev, "Failed to register client: %d\n", ret);
return ret;
}
+ drm_client_add(&fb_helper->client);
+
fb_helper->preferred_bpp = preferred_bpp;
ret = drm_fbdev_client_hotplug(&fb_helper->client);
diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h
index 989f8e52864d..971bb7853776 100644
--- a/include/drm/drm_client.h
+++ b/include/drm/drm_client.h
@@ -87,9 +87,10 @@ struct drm_client_dev {
struct drm_file *file;
};
-int drm_client_new(struct drm_device *dev, struct drm_client_dev *client,
- const char *name, const struct drm_client_funcs *funcs);
+int drm_client_init(struct drm_device *dev, struct drm_client_dev *client,
+ const char *name, const struct drm_client_funcs *funcs);
void drm_client_release(struct drm_client_dev *client);
+void drm_client_add(struct drm_client_dev *client);
void drm_client_dev_unregister(struct drm_device *dev);
void drm_client_dev_hotplug(struct drm_device *dev);
--
2.15.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v3] drm/cma-helper: Fix crash in fbdev error path
2018-10-01 19:45 [PATCH v3] drm/cma-helper: Fix crash in fbdev error path Noralf Trønnes
@ 2018-10-02 11:14 ` Noralf Trønnes
2018-11-04 16:14 ` Sergey Suloev
2018-11-04 17:10 ` Sergey Suloev
0 siblings, 2 replies; 5+ messages in thread
From: Noralf Trønnes @ 2018-10-02 11:14 UTC (permalink / raw)
To: dri-devel; +Cc: Stefan Wahren, Daniel Vetter, ssuloev
Den 01.10.2018 21.45, skrev Noralf Trønnes:
> Sergey Suloev reported a crash happening in drm_client_dev_hotplug()
> when fbdev had failed to register.
>
> [ 9.124598] vc4_hdmi 3f902000.hdmi: ASoC: Failed to create component debugfs directory
> [ 9.147667] vc4_hdmi 3f902000.hdmi: vc4-hdmi-hifi <-> 3f902000.hdmi mapping ok
> [ 9.155184] vc4_hdmi 3f902000.hdmi: ASoC: no DMI vendor name!
> [ 9.166544] vc4-drm soc:gpu: bound 3f902000.hdmi (ops vc4_hdmi_ops [vc4])
> [ 9.173840] vc4-drm soc:gpu: bound 3f806000.vec (ops vc4_vec_ops [vc4])
> [ 9.181029] vc4-drm soc:gpu: bound 3f004000.txp (ops vc4_txp_ops [vc4])
> [ 9.188519] vc4-drm soc:gpu: bound 3f400000.hvs (ops vc4_hvs_ops [vc4])
> [ 9.195690] vc4-drm soc:gpu: bound 3f206000.pixelvalve (ops vc4_crtc_ops [vc4])
> [ 9.203523] vc4-drm soc:gpu: bound 3f207000.pixelvalve (ops vc4_crtc_ops [vc4])
> [ 9.215032] vc4-drm soc:gpu: bound 3f807000.pixelvalve (ops vc4_crtc_ops [vc4])
> [ 9.274785] vc4-drm soc:gpu: bound 3fc00000.v3d (ops vc4_v3d_ops [vc4])
> [ 9.290246] [drm] Initialized vc4 0.0.0 20140616 for soc:gpu on minor 0
> [ 9.297464] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
> [ 9.304600] [drm] Driver supports precise vblank timestamp query.
> [ 9.382856] vc4-drm soc:gpu: [drm:drm_fb_helper_fbdev_setup [drm_kms_helper]] *ERROR* Failed to set fbdev configuration
> [ 10.404937] Unable to handle kernel paging request at virtual address 00330a656369768a
> [ 10.441620] [00330a656369768a] address between user and kernel address ranges
> [ 10.449087] Internal error: Oops: 96000004 [#1] PREEMPT SMP
> [ 10.454762] Modules linked in: brcmfmac vc4 drm_kms_helper cfg80211 drm rfkill smsc95xx brcmutil usbnet drm_panel_orientation_quirks raspberrypi_hwmon bcm2835_dma crc32_ce pwm_bcm2835 bcm2835_rng virt_dma rng_core i2c_bcm2835 ip_tables x_tables ipv6
> [ 10.477296] CPU: 2 PID: 45 Comm: kworker/2:1 Not tainted 4.19.0-rc5 #3
> [ 10.483934] Hardware name: Raspberry Pi 3 Model B Rev 1.2 (DT)
> [ 10.489966] Workqueue: events output_poll_execute [drm_kms_helper]
> [ 10.596515] Process kworker/2:1 (pid: 45, stack limit = 0x000000007e8924dc)
> [ 10.603590] Call trace:
> [ 10.606259] drm_client_dev_hotplug+0x5c/0xb0 [drm]
> [ 10.611303] drm_kms_helper_hotplug_event+0x30/0x40 [drm_kms_helper]
> [ 10.617849] output_poll_execute+0xc4/0x1e0 [drm_kms_helper]
> [ 10.623616] process_one_work+0x1c8/0x318
> [ 10.627695] worker_thread+0x48/0x428
> [ 10.631420] kthread+0xf8/0x128
> [ 10.634615] ret_from_fork+0x10/0x18
> [ 10.638255] Code: 54000220 f9401261 aa1303e0 b4000141 (f9400c21)
> [ 10.644456] ---[ end trace c75b4a4b0e141908 ]---
>
> The reason for this is that drm_fbdev_cma_init() removes the drm_client
> when fbdev registration fails, but it doesn't remove the client from the
> drm_device client list. So the client list now has a pointer that points
> into the unknown and we have a 'use after free' situation.
>
> Split drm_client_new() into drm_client_init() and drm_client_add() to fix
> removal in the error path.
>
> Fixes: 894a677f4b3e ("drm/cma-helper: Use the generic fbdev emulation")
> Reported-by: Sergey Suloev <ssuloev@orpaltech.com>
> Cc: Stefan Wahren <stefan.wahren@i2se.com>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
Applied to drm-misc-fixes, thanks for the review.
Noralf.
> Changes since version 2:
> - Fix drm_client_new() ref in docs (Daniel)
> - Expand drm_client_add() docs (Daniel)
>
> Changes since version 1:
> - Split drm_client_new() instead of letting drm_client hang around (Daniel)
>
> drivers/gpu/drm/drm_client.c | 35 ++++++++++++++++++++++++++---------
> drivers/gpu/drm/drm_fb_cma_helper.c | 4 +++-
> drivers/gpu/drm/drm_fb_helper.c | 4 +++-
> include/drm/drm_client.h | 5 +++--
> 4 files changed, 35 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
> index 17d9a64e885e..fc03d26fcacc 100644
> --- a/drivers/gpu/drm/drm_client.c
> +++ b/drivers/gpu/drm/drm_client.c
> @@ -63,20 +63,21 @@ static void drm_client_close(struct drm_client_dev *client)
> EXPORT_SYMBOL(drm_client_close);
>
> /**
> - * drm_client_new - Create a DRM client
> + * drm_client_init - Initialise a DRM client
> * @dev: DRM device
> * @client: DRM client
> * @name: Client name
> * @funcs: DRM client functions (optional)
> *
> + * This initialises the client and opens a &drm_file. Use drm_client_add() to complete the process.
> * The caller needs to hold a reference on @dev before calling this function.
> * The client is freed when the &drm_device is unregistered. See drm_client_release().
> *
> * Returns:
> * Zero on success or negative error code on failure.
> */
> -int drm_client_new(struct drm_device *dev, struct drm_client_dev *client,
> - const char *name, const struct drm_client_funcs *funcs)
> +int drm_client_init(struct drm_device *dev, struct drm_client_dev *client,
> + const char *name, const struct drm_client_funcs *funcs)
> {
> int ret;
>
> @@ -95,10 +96,6 @@ int drm_client_new(struct drm_device *dev, struct drm_client_dev *client,
> if (ret)
> goto err_put_module;
>
> - mutex_lock(&dev->clientlist_mutex);
> - list_add(&client->list, &dev->clientlist);
> - mutex_unlock(&dev->clientlist_mutex);
> -
> drm_dev_get(dev);
>
> return 0;
> @@ -109,13 +106,33 @@ int drm_client_new(struct drm_device *dev, struct drm_client_dev *client,
>
> return ret;
> }
> -EXPORT_SYMBOL(drm_client_new);
> +EXPORT_SYMBOL(drm_client_init);
> +
> +/**
> + * drm_client_add - Add client to the device list
> + * @client: DRM client
> + *
> + * Add the client to the &drm_device client list to activate its callbacks.
> + * @client must be initialized by a call to drm_client_init(). After
> + * drm_client_add() it is no longer permissible to call drm_client_release()
> + * directly (outside the unregister callback), instead cleanup will happen
> + * automatically on driver unload.
> + */
> +void drm_client_add(struct drm_client_dev *client)
> +{
> + struct drm_device *dev = client->dev;
> +
> + mutex_lock(&dev->clientlist_mutex);
> + list_add(&client->list, &dev->clientlist);
> + mutex_unlock(&dev->clientlist_mutex);
> +}
> +EXPORT_SYMBOL(drm_client_add);
>
> /**
> * drm_client_release - Release DRM client resources
> * @client: DRM client
> *
> - * Releases resources by closing the &drm_file that was opened by drm_client_new().
> + * Releases resources by closing the &drm_file that was opened by drm_client_init().
> * It is called automatically if the &drm_client_funcs.unregister callback is _not_ set.
> *
> * This function should only be called from the unregister callback. An exception
> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
> index 47e0e2f6642d..fb0dfc62b1b6 100644
> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> @@ -167,7 +167,7 @@ struct drm_fbdev_cma *drm_fbdev_cma_init(struct drm_device *dev,
>
> fb_helper = &fbdev_cma->fb_helper;
>
> - ret = drm_client_new(dev, &fb_helper->client, "fbdev", NULL);
> + ret = drm_client_init(dev, &fb_helper->client, "fbdev", NULL);
> if (ret)
> goto err_free;
>
> @@ -176,6 +176,8 @@ struct drm_fbdev_cma *drm_fbdev_cma_init(struct drm_device *dev,
> if (ret)
> goto err_client_put;
>
> + drm_client_add(&fb_helper->client);
> +
> return fbdev_cma;
>
> err_client_put:
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index a504a5e05676..e9095537e0c0 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -3229,13 +3229,15 @@ int drm_fbdev_generic_setup(struct drm_device *dev, unsigned int preferred_bpp)
> if (!fb_helper)
> return -ENOMEM;
>
> - ret = drm_client_new(dev, &fb_helper->client, "fbdev", &drm_fbdev_client_funcs);
> + ret = drm_client_init(dev, &fb_helper->client, "fbdev", &drm_fbdev_client_funcs);
> if (ret) {
> kfree(fb_helper);
> DRM_DEV_ERROR(dev->dev, "Failed to register client: %d\n", ret);
> return ret;
> }
>
> + drm_client_add(&fb_helper->client);
> +
> fb_helper->preferred_bpp = preferred_bpp;
>
> ret = drm_fbdev_client_hotplug(&fb_helper->client);
> diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h
> index 989f8e52864d..971bb7853776 100644
> --- a/include/drm/drm_client.h
> +++ b/include/drm/drm_client.h
> @@ -87,9 +87,10 @@ struct drm_client_dev {
> struct drm_file *file;
> };
>
> -int drm_client_new(struct drm_device *dev, struct drm_client_dev *client,
> - const char *name, const struct drm_client_funcs *funcs);
> +int drm_client_init(struct drm_device *dev, struct drm_client_dev *client,
> + const char *name, const struct drm_client_funcs *funcs);
> void drm_client_release(struct drm_client_dev *client);
> +void drm_client_add(struct drm_client_dev *client);
>
> void drm_client_dev_unregister(struct drm_device *dev);
> void drm_client_dev_hotplug(struct drm_device *dev);
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] drm/cma-helper: Fix crash in fbdev error path
2018-10-02 11:14 ` Noralf Trønnes
@ 2018-11-04 16:14 ` Sergey Suloev
2018-11-04 17:10 ` Sergey Suloev
1 sibling, 0 replies; 5+ messages in thread
From: Sergey Suloev @ 2018-11-04 16:14 UTC (permalink / raw)
To: Noralf Trønnes, dri-devel; +Cc: Stefan Wahren, Daniel Vetter
This doesn't seem to fix the initially reported issue. The problem still
exists with 4.19.
On 10/2/18 2:14 PM, Noralf Trønnes wrote:
>
> Den 01.10.2018 21.45, skrev Noralf Trønnes:
>> Sergey Suloev reported a crash happening in drm_client_dev_hotplug()
>> when fbdev had failed to register.
>>
>> [ 9.124598] vc4_hdmi 3f902000.hdmi: ASoC: Failed to create
>> component debugfs directory
>> [ 9.147667] vc4_hdmi 3f902000.hdmi: vc4-hdmi-hifi <->
>> 3f902000.hdmi mapping ok
>> [ 9.155184] vc4_hdmi 3f902000.hdmi: ASoC: no DMI vendor name!
>> [ 9.166544] vc4-drm soc:gpu: bound 3f902000.hdmi (ops vc4_hdmi_ops
>> [vc4])
>> [ 9.173840] vc4-drm soc:gpu: bound 3f806000.vec (ops vc4_vec_ops
>> [vc4])
>> [ 9.181029] vc4-drm soc:gpu: bound 3f004000.txp (ops vc4_txp_ops
>> [vc4])
>> [ 9.188519] vc4-drm soc:gpu: bound 3f400000.hvs (ops vc4_hvs_ops
>> [vc4])
>> [ 9.195690] vc4-drm soc:gpu: bound 3f206000.pixelvalve (ops
>> vc4_crtc_ops [vc4])
>> [ 9.203523] vc4-drm soc:gpu: bound 3f207000.pixelvalve (ops
>> vc4_crtc_ops [vc4])
>> [ 9.215032] vc4-drm soc:gpu: bound 3f807000.pixelvalve (ops
>> vc4_crtc_ops [vc4])
>> [ 9.274785] vc4-drm soc:gpu: bound 3fc00000.v3d (ops vc4_v3d_ops
>> [vc4])
>> [ 9.290246] [drm] Initialized vc4 0.0.0 20140616 for soc:gpu on
>> minor 0
>> [ 9.297464] [drm] Supports vblank timestamp caching Rev 2
>> (21.10.2013).
>> [ 9.304600] [drm] Driver supports precise vblank timestamp query.
>> [ 9.382856] vc4-drm soc:gpu: [drm:drm_fb_helper_fbdev_setup
>> [drm_kms_helper]] *ERROR* Failed to set fbdev configuration
>> [ 10.404937] Unable to handle kernel paging request at virtual
>> address 00330a656369768a
>> [ 10.441620] [00330a656369768a] address between user and kernel
>> address ranges
>> [ 10.449087] Internal error: Oops: 96000004 [#1] PREEMPT SMP
>> [ 10.454762] Modules linked in: brcmfmac vc4 drm_kms_helper
>> cfg80211 drm rfkill smsc95xx brcmutil usbnet
>> drm_panel_orientation_quirks raspberrypi_hwmon bcm2835_dma crc32_ce
>> pwm_bcm2835 bcm2835_rng virt_dma rng_core i2c_bcm2835 ip_tables
>> x_tables ipv6
>> [ 10.477296] CPU: 2 PID: 45 Comm: kworker/2:1 Not tainted
>> 4.19.0-rc5 #3
>> [ 10.483934] Hardware name: Raspberry Pi 3 Model B Rev 1.2 (DT)
>> [ 10.489966] Workqueue: events output_poll_execute [drm_kms_helper]
>> [ 10.596515] Process kworker/2:1 (pid: 45, stack limit =
>> 0x000000007e8924dc)
>> [ 10.603590] Call trace:
>> [ 10.606259] drm_client_dev_hotplug+0x5c/0xb0 [drm]
>> [ 10.611303] drm_kms_helper_hotplug_event+0x30/0x40 [drm_kms_helper]
>> [ 10.617849] output_poll_execute+0xc4/0x1e0 [drm_kms_helper]
>> [ 10.623616] process_one_work+0x1c8/0x318
>> [ 10.627695] worker_thread+0x48/0x428
>> [ 10.631420] kthread+0xf8/0x128
>> [ 10.634615] ret_from_fork+0x10/0x18
>> [ 10.638255] Code: 54000220 f9401261 aa1303e0 b4000141 (f9400c21)
>> [ 10.644456] ---[ end trace c75b4a4b0e141908 ]---
>>
>> The reason for this is that drm_fbdev_cma_init() removes the drm_client
>> when fbdev registration fails, but it doesn't remove the client from the
>> drm_device client list. So the client list now has a pointer that points
>> into the unknown and we have a 'use after free' situation.
>>
>> Split drm_client_new() into drm_client_init() and drm_client_add() to
>> fix
>> removal in the error path.
>>
>> Fixes: 894a677f4b3e ("drm/cma-helper: Use the generic fbdev emulation")
>> Reported-by: Sergey Suloev <ssuloev@orpaltech.com>
>> Cc: Stefan Wahren <stefan.wahren@i2se.com>
>> Cc: Eric Anholt <eric@anholt.net>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> ---
>
> Applied to drm-misc-fixes, thanks for the review.
>
> Noralf.
>
>> Changes since version 2:
>> - Fix drm_client_new() ref in docs (Daniel)
>> - Expand drm_client_add() docs (Daniel)
>>
>> Changes since version 1:
>> - Split drm_client_new() instead of letting drm_client hang around
>> (Daniel)
>>
>> drivers/gpu/drm/drm_client.c | 35
>> ++++++++++++++++++++++++++---------
>> drivers/gpu/drm/drm_fb_cma_helper.c | 4 +++-
>> drivers/gpu/drm/drm_fb_helper.c | 4 +++-
>> include/drm/drm_client.h | 5 +++--
>> 4 files changed, 35 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
>> index 17d9a64e885e..fc03d26fcacc 100644
>> --- a/drivers/gpu/drm/drm_client.c
>> +++ b/drivers/gpu/drm/drm_client.c
>> @@ -63,20 +63,21 @@ static void drm_client_close(struct
>> drm_client_dev *client)
>> EXPORT_SYMBOL(drm_client_close);
>> /**
>> - * drm_client_new - Create a DRM client
>> + * drm_client_init - Initialise a DRM client
>> * @dev: DRM device
>> * @client: DRM client
>> * @name: Client name
>> * @funcs: DRM client functions (optional)
>> *
>> + * This initialises the client and opens a &drm_file. Use
>> drm_client_add() to complete the process.
>> * The caller needs to hold a reference on @dev before calling this
>> function.
>> * The client is freed when the &drm_device is unregistered. See
>> drm_client_release().
>> *
>> * Returns:
>> * Zero on success or negative error code on failure.
>> */
>> -int drm_client_new(struct drm_device *dev, struct drm_client_dev
>> *client,
>> - const char *name, const struct drm_client_funcs *funcs)
>> +int drm_client_init(struct drm_device *dev, struct drm_client_dev
>> *client,
>> + const char *name, const struct drm_client_funcs *funcs)
>> {
>> int ret;
>> @@ -95,10 +96,6 @@ int drm_client_new(struct drm_device *dev,
>> struct drm_client_dev *client,
>> if (ret)
>> goto err_put_module;
>> - mutex_lock(&dev->clientlist_mutex);
>> - list_add(&client->list, &dev->clientlist);
>> - mutex_unlock(&dev->clientlist_mutex);
>> -
>> drm_dev_get(dev);
>> return 0;
>> @@ -109,13 +106,33 @@ int drm_client_new(struct drm_device *dev,
>> struct drm_client_dev *client,
>> return ret;
>> }
>> -EXPORT_SYMBOL(drm_client_new);
>> +EXPORT_SYMBOL(drm_client_init);
>> +
>> +/**
>> + * drm_client_add - Add client to the device list
>> + * @client: DRM client
>> + *
>> + * Add the client to the &drm_device client list to activate its
>> callbacks.
>> + * @client must be initialized by a call to drm_client_init(). After
>> + * drm_client_add() it is no longer permissible to call
>> drm_client_release()
>> + * directly (outside the unregister callback), instead cleanup will
>> happen
>> + * automatically on driver unload.
>> + */
>> +void drm_client_add(struct drm_client_dev *client)
>> +{
>> + struct drm_device *dev = client->dev;
>> +
>> + mutex_lock(&dev->clientlist_mutex);
>> + list_add(&client->list, &dev->clientlist);
>> + mutex_unlock(&dev->clientlist_mutex);
>> +}
>> +EXPORT_SYMBOL(drm_client_add);
>> /**
>> * drm_client_release - Release DRM client resources
>> * @client: DRM client
>> *
>> - * Releases resources by closing the &drm_file that was opened by
>> drm_client_new().
>> + * Releases resources by closing the &drm_file that was opened by
>> drm_client_init().
>> * It is called automatically if the &drm_client_funcs.unregister
>> callback is _not_ set.
>> *
>> * This function should only be called from the unregister
>> callback. An exception
>> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
>> b/drivers/gpu/drm/drm_fb_cma_helper.c
>> index 47e0e2f6642d..fb0dfc62b1b6 100644
>> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
>> @@ -167,7 +167,7 @@ struct drm_fbdev_cma *drm_fbdev_cma_init(struct
>> drm_device *dev,
>> fb_helper = &fbdev_cma->fb_helper;
>> - ret = drm_client_new(dev, &fb_helper->client, "fbdev", NULL);
>> + ret = drm_client_init(dev, &fb_helper->client, "fbdev", NULL);
>> if (ret)
>> goto err_free;
>> @@ -176,6 +176,8 @@ struct drm_fbdev_cma *drm_fbdev_cma_init(struct
>> drm_device *dev,
>> if (ret)
>> goto err_client_put;
>> + drm_client_add(&fb_helper->client);
>> +
>> return fbdev_cma;
>> err_client_put:
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c
>> b/drivers/gpu/drm/drm_fb_helper.c
>> index a504a5e05676..e9095537e0c0 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -3229,13 +3229,15 @@ int drm_fbdev_generic_setup(struct drm_device
>> *dev, unsigned int preferred_bpp)
>> if (!fb_helper)
>> return -ENOMEM;
>> - ret = drm_client_new(dev, &fb_helper->client, "fbdev",
>> &drm_fbdev_client_funcs);
>> + ret = drm_client_init(dev, &fb_helper->client, "fbdev",
>> &drm_fbdev_client_funcs);
>> if (ret) {
>> kfree(fb_helper);
>> DRM_DEV_ERROR(dev->dev, "Failed to register client: %d\n",
>> ret);
>> return ret;
>> }
>> + drm_client_add(&fb_helper->client);
>> +
>> fb_helper->preferred_bpp = preferred_bpp;
>> ret = drm_fbdev_client_hotplug(&fb_helper->client);
>> diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h
>> index 989f8e52864d..971bb7853776 100644
>> --- a/include/drm/drm_client.h
>> +++ b/include/drm/drm_client.h
>> @@ -87,9 +87,10 @@ struct drm_client_dev {
>> struct drm_file *file;
>> };
>> -int drm_client_new(struct drm_device *dev, struct drm_client_dev
>> *client,
>> - const char *name, const struct drm_client_funcs *funcs);
>> +int drm_client_init(struct drm_device *dev, struct drm_client_dev
>> *client,
>> + const char *name, const struct drm_client_funcs *funcs);
>> void drm_client_release(struct drm_client_dev *client);
>> +void drm_client_add(struct drm_client_dev *client);
>> void drm_client_dev_unregister(struct drm_device *dev);
>> void drm_client_dev_hotplug(struct drm_device *dev);
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] drm/cma-helper: Fix crash in fbdev error path
2018-10-02 11:14 ` Noralf Trønnes
2018-11-04 16:14 ` Sergey Suloev
@ 2018-11-04 17:10 ` Sergey Suloev
2018-11-05 9:16 ` Daniel Vetter
1 sibling, 1 reply; 5+ messages in thread
From: Sergey Suloev @ 2018-11-04 17:10 UTC (permalink / raw)
To: Noralf Trønnes, dri-devel; +Cc: Stefan Wahren, Daniel Vetter
To be exact, the issue does not happen in case I use default kernel
config but only with the custom one.
On 10/2/18 2:14 PM, Noralf Trønnes wrote:
>
> Den 01.10.2018 21.45, skrev Noralf Trønnes:
>> Sergey Suloev reported a crash happening in drm_client_dev_hotplug()
>> when fbdev had failed to register.
>>
>> [ 9.124598] vc4_hdmi 3f902000.hdmi: ASoC: Failed to create
>> component debugfs directory
>> [ 9.147667] vc4_hdmi 3f902000.hdmi: vc4-hdmi-hifi <->
>> 3f902000.hdmi mapping ok
>> [ 9.155184] vc4_hdmi 3f902000.hdmi: ASoC: no DMI vendor name!
>> [ 9.166544] vc4-drm soc:gpu: bound 3f902000.hdmi (ops vc4_hdmi_ops
>> [vc4])
>> [ 9.173840] vc4-drm soc:gpu: bound 3f806000.vec (ops vc4_vec_ops
>> [vc4])
>> [ 9.181029] vc4-drm soc:gpu: bound 3f004000.txp (ops vc4_txp_ops
>> [vc4])
>> [ 9.188519] vc4-drm soc:gpu: bound 3f400000.hvs (ops vc4_hvs_ops
>> [vc4])
>> [ 9.195690] vc4-drm soc:gpu: bound 3f206000.pixelvalve (ops
>> vc4_crtc_ops [vc4])
>> [ 9.203523] vc4-drm soc:gpu: bound 3f207000.pixelvalve (ops
>> vc4_crtc_ops [vc4])
>> [ 9.215032] vc4-drm soc:gpu: bound 3f807000.pixelvalve (ops
>> vc4_crtc_ops [vc4])
>> [ 9.274785] vc4-drm soc:gpu: bound 3fc00000.v3d (ops vc4_v3d_ops
>> [vc4])
>> [ 9.290246] [drm] Initialized vc4 0.0.0 20140616 for soc:gpu on
>> minor 0
>> [ 9.297464] [drm] Supports vblank timestamp caching Rev 2
>> (21.10.2013).
>> [ 9.304600] [drm] Driver supports precise vblank timestamp query.
>> [ 9.382856] vc4-drm soc:gpu: [drm:drm_fb_helper_fbdev_setup
>> [drm_kms_helper]] *ERROR* Failed to set fbdev configuration
>> [ 10.404937] Unable to handle kernel paging request at virtual
>> address 00330a656369768a
>> [ 10.441620] [00330a656369768a] address between user and kernel
>> address ranges
>> [ 10.449087] Internal error: Oops: 96000004 [#1] PREEMPT SMP
>> [ 10.454762] Modules linked in: brcmfmac vc4 drm_kms_helper
>> cfg80211 drm rfkill smsc95xx brcmutil usbnet
>> drm_panel_orientation_quirks raspberrypi_hwmon bcm2835_dma crc32_ce
>> pwm_bcm2835 bcm2835_rng virt_dma rng_core i2c_bcm2835 ip_tables
>> x_tables ipv6
>> [ 10.477296] CPU: 2 PID: 45 Comm: kworker/2:1 Not tainted
>> 4.19.0-rc5 #3
>> [ 10.483934] Hardware name: Raspberry Pi 3 Model B Rev 1.2 (DT)
>> [ 10.489966] Workqueue: events output_poll_execute [drm_kms_helper]
>> [ 10.596515] Process kworker/2:1 (pid: 45, stack limit =
>> 0x000000007e8924dc)
>> [ 10.603590] Call trace:
>> [ 10.606259] drm_client_dev_hotplug+0x5c/0xb0 [drm]
>> [ 10.611303] drm_kms_helper_hotplug_event+0x30/0x40 [drm_kms_helper]
>> [ 10.617849] output_poll_execute+0xc4/0x1e0 [drm_kms_helper]
>> [ 10.623616] process_one_work+0x1c8/0x318
>> [ 10.627695] worker_thread+0x48/0x428
>> [ 10.631420] kthread+0xf8/0x128
>> [ 10.634615] ret_from_fork+0x10/0x18
>> [ 10.638255] Code: 54000220 f9401261 aa1303e0 b4000141 (f9400c21)
>> [ 10.644456] ---[ end trace c75b4a4b0e141908 ]---
>>
>> The reason for this is that drm_fbdev_cma_init() removes the drm_client
>> when fbdev registration fails, but it doesn't remove the client from the
>> drm_device client list. So the client list now has a pointer that points
>> into the unknown and we have a 'use after free' situation.
>>
>> Split drm_client_new() into drm_client_init() and drm_client_add() to
>> fix
>> removal in the error path.
>>
>> Fixes: 894a677f4b3e ("drm/cma-helper: Use the generic fbdev emulation")
>> Reported-by: Sergey Suloev <ssuloev@orpaltech.com>
>> Cc: Stefan Wahren <stefan.wahren@i2se.com>
>> Cc: Eric Anholt <eric@anholt.net>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> ---
>
> Applied to drm-misc-fixes, thanks for the review.
>
> Noralf.
>
>> Changes since version 2:
>> - Fix drm_client_new() ref in docs (Daniel)
>> - Expand drm_client_add() docs (Daniel)
>>
>> Changes since version 1:
>> - Split drm_client_new() instead of letting drm_client hang around
>> (Daniel)
>>
>> drivers/gpu/drm/drm_client.c | 35
>> ++++++++++++++++++++++++++---------
>> drivers/gpu/drm/drm_fb_cma_helper.c | 4 +++-
>> drivers/gpu/drm/drm_fb_helper.c | 4 +++-
>> include/drm/drm_client.h | 5 +++--
>> 4 files changed, 35 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
>> index 17d9a64e885e..fc03d26fcacc 100644
>> --- a/drivers/gpu/drm/drm_client.c
>> +++ b/drivers/gpu/drm/drm_client.c
>> @@ -63,20 +63,21 @@ static void drm_client_close(struct
>> drm_client_dev *client)
>> EXPORT_SYMBOL(drm_client_close);
>> /**
>> - * drm_client_new - Create a DRM client
>> + * drm_client_init - Initialise a DRM client
>> * @dev: DRM device
>> * @client: DRM client
>> * @name: Client name
>> * @funcs: DRM client functions (optional)
>> *
>> + * This initialises the client and opens a &drm_file. Use
>> drm_client_add() to complete the process.
>> * The caller needs to hold a reference on @dev before calling this
>> function.
>> * The client is freed when the &drm_device is unregistered. See
>> drm_client_release().
>> *
>> * Returns:
>> * Zero on success or negative error code on failure.
>> */
>> -int drm_client_new(struct drm_device *dev, struct drm_client_dev
>> *client,
>> - const char *name, const struct drm_client_funcs *funcs)
>> +int drm_client_init(struct drm_device *dev, struct drm_client_dev
>> *client,
>> + const char *name, const struct drm_client_funcs *funcs)
>> {
>> int ret;
>> @@ -95,10 +96,6 @@ int drm_client_new(struct drm_device *dev,
>> struct drm_client_dev *client,
>> if (ret)
>> goto err_put_module;
>> - mutex_lock(&dev->clientlist_mutex);
>> - list_add(&client->list, &dev->clientlist);
>> - mutex_unlock(&dev->clientlist_mutex);
>> -
>> drm_dev_get(dev);
>> return 0;
>> @@ -109,13 +106,33 @@ int drm_client_new(struct drm_device *dev,
>> struct drm_client_dev *client,
>> return ret;
>> }
>> -EXPORT_SYMBOL(drm_client_new);
>> +EXPORT_SYMBOL(drm_client_init);
>> +
>> +/**
>> + * drm_client_add - Add client to the device list
>> + * @client: DRM client
>> + *
>> + * Add the client to the &drm_device client list to activate its
>> callbacks.
>> + * @client must be initialized by a call to drm_client_init(). After
>> + * drm_client_add() it is no longer permissible to call
>> drm_client_release()
>> + * directly (outside the unregister callback), instead cleanup will
>> happen
>> + * automatically on driver unload.
>> + */
>> +void drm_client_add(struct drm_client_dev *client)
>> +{
>> + struct drm_device *dev = client->dev;
>> +
>> + mutex_lock(&dev->clientlist_mutex);
>> + list_add(&client->list, &dev->clientlist);
>> + mutex_unlock(&dev->clientlist_mutex);
>> +}
>> +EXPORT_SYMBOL(drm_client_add);
>> /**
>> * drm_client_release - Release DRM client resources
>> * @client: DRM client
>> *
>> - * Releases resources by closing the &drm_file that was opened by
>> drm_client_new().
>> + * Releases resources by closing the &drm_file that was opened by
>> drm_client_init().
>> * It is called automatically if the &drm_client_funcs.unregister
>> callback is _not_ set.
>> *
>> * This function should only be called from the unregister
>> callback. An exception
>> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
>> b/drivers/gpu/drm/drm_fb_cma_helper.c
>> index 47e0e2f6642d..fb0dfc62b1b6 100644
>> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
>> @@ -167,7 +167,7 @@ struct drm_fbdev_cma *drm_fbdev_cma_init(struct
>> drm_device *dev,
>> fb_helper = &fbdev_cma->fb_helper;
>> - ret = drm_client_new(dev, &fb_helper->client, "fbdev", NULL);
>> + ret = drm_client_init(dev, &fb_helper->client, "fbdev", NULL);
>> if (ret)
>> goto err_free;
>> @@ -176,6 +176,8 @@ struct drm_fbdev_cma *drm_fbdev_cma_init(struct
>> drm_device *dev,
>> if (ret)
>> goto err_client_put;
>> + drm_client_add(&fb_helper->client);
>> +
>> return fbdev_cma;
>> err_client_put:
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c
>> b/drivers/gpu/drm/drm_fb_helper.c
>> index a504a5e05676..e9095537e0c0 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -3229,13 +3229,15 @@ int drm_fbdev_generic_setup(struct drm_device
>> *dev, unsigned int preferred_bpp)
>> if (!fb_helper)
>> return -ENOMEM;
>> - ret = drm_client_new(dev, &fb_helper->client, "fbdev",
>> &drm_fbdev_client_funcs);
>> + ret = drm_client_init(dev, &fb_helper->client, "fbdev",
>> &drm_fbdev_client_funcs);
>> if (ret) {
>> kfree(fb_helper);
>> DRM_DEV_ERROR(dev->dev, "Failed to register client: %d\n",
>> ret);
>> return ret;
>> }
>> + drm_client_add(&fb_helper->client);
>> +
>> fb_helper->preferred_bpp = preferred_bpp;
>> ret = drm_fbdev_client_hotplug(&fb_helper->client);
>> diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h
>> index 989f8e52864d..971bb7853776 100644
>> --- a/include/drm/drm_client.h
>> +++ b/include/drm/drm_client.h
>> @@ -87,9 +87,10 @@ struct drm_client_dev {
>> struct drm_file *file;
>> };
>> -int drm_client_new(struct drm_device *dev, struct drm_client_dev
>> *client,
>> - const char *name, const struct drm_client_funcs *funcs);
>> +int drm_client_init(struct drm_device *dev, struct drm_client_dev
>> *client,
>> + const char *name, const struct drm_client_funcs *funcs);
>> void drm_client_release(struct drm_client_dev *client);
>> +void drm_client_add(struct drm_client_dev *client);
>> void drm_client_dev_unregister(struct drm_device *dev);
>> void drm_client_dev_hotplug(struct drm_device *dev);
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] drm/cma-helper: Fix crash in fbdev error path
2018-11-04 17:10 ` Sergey Suloev
@ 2018-11-05 9:16 ` Daniel Vetter
0 siblings, 0 replies; 5+ messages in thread
From: Daniel Vetter @ 2018-11-05 9:16 UTC (permalink / raw)
To: Sergey Suloev; +Cc: Stefan Wahren, dri-devel, Daniel Vetter
On Sun, Nov 04, 2018 at 08:10:57PM +0300, Sergey Suloev wrote:
> To be exact, the issue does not happen in case I use default kernel config
> but only with the custom one.
Can you pls grab a new backtrace on latest kernels?
Thanks, Daniel
>
>
> On 10/2/18 2:14 PM, Noralf Trønnes wrote:
> >
> > Den 01.10.2018 21.45, skrev Noralf Trønnes:
> > > Sergey Suloev reported a crash happening in drm_client_dev_hotplug()
> > > when fbdev had failed to register.
> > >
> > > [ 9.124598] vc4_hdmi 3f902000.hdmi: ASoC: Failed to create
> > > component debugfs directory
> > > [ 9.147667] vc4_hdmi 3f902000.hdmi: vc4-hdmi-hifi <->
> > > 3f902000.hdmi mapping ok
> > > [ 9.155184] vc4_hdmi 3f902000.hdmi: ASoC: no DMI vendor name!
> > > [ 9.166544] vc4-drm soc:gpu: bound 3f902000.hdmi (ops
> > > vc4_hdmi_ops [vc4])
> > > [ 9.173840] vc4-drm soc:gpu: bound 3f806000.vec (ops vc4_vec_ops
> > > [vc4])
> > > [ 9.181029] vc4-drm soc:gpu: bound 3f004000.txp (ops vc4_txp_ops
> > > [vc4])
> > > [ 9.188519] vc4-drm soc:gpu: bound 3f400000.hvs (ops vc4_hvs_ops
> > > [vc4])
> > > [ 9.195690] vc4-drm soc:gpu: bound 3f206000.pixelvalve (ops
> > > vc4_crtc_ops [vc4])
> > > [ 9.203523] vc4-drm soc:gpu: bound 3f207000.pixelvalve (ops
> > > vc4_crtc_ops [vc4])
> > > [ 9.215032] vc4-drm soc:gpu: bound 3f807000.pixelvalve (ops
> > > vc4_crtc_ops [vc4])
> > > [ 9.274785] vc4-drm soc:gpu: bound 3fc00000.v3d (ops vc4_v3d_ops
> > > [vc4])
> > > [ 9.290246] [drm] Initialized vc4 0.0.0 20140616 for soc:gpu on
> > > minor 0
> > > [ 9.297464] [drm] Supports vblank timestamp caching Rev 2
> > > (21.10.2013).
> > > [ 9.304600] [drm] Driver supports precise vblank timestamp query.
> > > [ 9.382856] vc4-drm soc:gpu: [drm:drm_fb_helper_fbdev_setup
> > > [drm_kms_helper]] *ERROR* Failed to set fbdev configuration
> > > [ 10.404937] Unable to handle kernel paging request at virtual
> > > address 00330a656369768a
> > > [ 10.441620] [00330a656369768a] address between user and kernel
> > > address ranges
> > > [ 10.449087] Internal error: Oops: 96000004 [#1] PREEMPT SMP
> > > [ 10.454762] Modules linked in: brcmfmac vc4 drm_kms_helper
> > > cfg80211 drm rfkill smsc95xx brcmutil usbnet
> > > drm_panel_orientation_quirks raspberrypi_hwmon bcm2835_dma crc32_ce
> > > pwm_bcm2835 bcm2835_rng virt_dma rng_core i2c_bcm2835 ip_tables
> > > x_tables ipv6
> > > [ 10.477296] CPU: 2 PID: 45 Comm: kworker/2:1 Not tainted
> > > 4.19.0-rc5 #3
> > > [ 10.483934] Hardware name: Raspberry Pi 3 Model B Rev 1.2 (DT)
> > > [ 10.489966] Workqueue: events output_poll_execute [drm_kms_helper]
> > > [ 10.596515] Process kworker/2:1 (pid: 45, stack limit =
> > > 0x000000007e8924dc)
> > > [ 10.603590] Call trace:
> > > [ 10.606259] drm_client_dev_hotplug+0x5c/0xb0 [drm]
> > > [ 10.611303] drm_kms_helper_hotplug_event+0x30/0x40 [drm_kms_helper]
> > > [ 10.617849] output_poll_execute+0xc4/0x1e0 [drm_kms_helper]
> > > [ 10.623616] process_one_work+0x1c8/0x318
> > > [ 10.627695] worker_thread+0x48/0x428
> > > [ 10.631420] kthread+0xf8/0x128
> > > [ 10.634615] ret_from_fork+0x10/0x18
> > > [ 10.638255] Code: 54000220 f9401261 aa1303e0 b4000141 (f9400c21)
> > > [ 10.644456] ---[ end trace c75b4a4b0e141908 ]---
> > >
> > > The reason for this is that drm_fbdev_cma_init() removes the drm_client
> > > when fbdev registration fails, but it doesn't remove the client from the
> > > drm_device client list. So the client list now has a pointer that points
> > > into the unknown and we have a 'use after free' situation.
> > >
> > > Split drm_client_new() into drm_client_init() and drm_client_add()
> > > to fix
> > > removal in the error path.
> > >
> > > Fixes: 894a677f4b3e ("drm/cma-helper: Use the generic fbdev emulation")
> > > Reported-by: Sergey Suloev <ssuloev@orpaltech.com>
> > > Cc: Stefan Wahren <stefan.wahren@i2se.com>
> > > Cc: Eric Anholt <eric@anholt.net>
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > ---
> >
> > Applied to drm-misc-fixes, thanks for the review.
> >
> > Noralf.
> >
> > > Changes since version 2:
> > > - Fix drm_client_new() ref in docs (Daniel)
> > > - Expand drm_client_add() docs (Daniel)
> > >
> > > Changes since version 1:
> > > - Split drm_client_new() instead of letting drm_client hang around
> > > (Daniel)
> > >
> > > drivers/gpu/drm/drm_client.c | 35
> > > ++++++++++++++++++++++++++---------
> > > drivers/gpu/drm/drm_fb_cma_helper.c | 4 +++-
> > > drivers/gpu/drm/drm_fb_helper.c | 4 +++-
> > > include/drm/drm_client.h | 5 +++--
> > > 4 files changed, 35 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
> > > index 17d9a64e885e..fc03d26fcacc 100644
> > > --- a/drivers/gpu/drm/drm_client.c
> > > +++ b/drivers/gpu/drm/drm_client.c
> > > @@ -63,20 +63,21 @@ static void drm_client_close(struct
> > > drm_client_dev *client)
> > > EXPORT_SYMBOL(drm_client_close);
> > > /**
> > > - * drm_client_new - Create a DRM client
> > > + * drm_client_init - Initialise a DRM client
> > > * @dev: DRM device
> > > * @client: DRM client
> > > * @name: Client name
> > > * @funcs: DRM client functions (optional)
> > > *
> > > + * This initialises the client and opens a &drm_file. Use
> > > drm_client_add() to complete the process.
> > > * The caller needs to hold a reference on @dev before calling
> > > this function.
> > > * The client is freed when the &drm_device is unregistered. See
> > > drm_client_release().
> > > *
> > > * Returns:
> > > * Zero on success or negative error code on failure.
> > > */
> > > -int drm_client_new(struct drm_device *dev, struct drm_client_dev
> > > *client,
> > > - const char *name, const struct drm_client_funcs *funcs)
> > > +int drm_client_init(struct drm_device *dev, struct drm_client_dev
> > > *client,
> > > + const char *name, const struct drm_client_funcs *funcs)
> > > {
> > > int ret;
> > > @@ -95,10 +96,6 @@ int drm_client_new(struct drm_device *dev,
> > > struct drm_client_dev *client,
> > > if (ret)
> > > goto err_put_module;
> > > - mutex_lock(&dev->clientlist_mutex);
> > > - list_add(&client->list, &dev->clientlist);
> > > - mutex_unlock(&dev->clientlist_mutex);
> > > -
> > > drm_dev_get(dev);
> > > return 0;
> > > @@ -109,13 +106,33 @@ int drm_client_new(struct drm_device *dev,
> > > struct drm_client_dev *client,
> > > return ret;
> > > }
> > > -EXPORT_SYMBOL(drm_client_new);
> > > +EXPORT_SYMBOL(drm_client_init);
> > > +
> > > +/**
> > > + * drm_client_add - Add client to the device list
> > > + * @client: DRM client
> > > + *
> > > + * Add the client to the &drm_device client list to activate its
> > > callbacks.
> > > + * @client must be initialized by a call to drm_client_init(). After
> > > + * drm_client_add() it is no longer permissible to call
> > > drm_client_release()
> > > + * directly (outside the unregister callback), instead cleanup will
> > > happen
> > > + * automatically on driver unload.
> > > + */
> > > +void drm_client_add(struct drm_client_dev *client)
> > > +{
> > > + struct drm_device *dev = client->dev;
> > > +
> > > + mutex_lock(&dev->clientlist_mutex);
> > > + list_add(&client->list, &dev->clientlist);
> > > + mutex_unlock(&dev->clientlist_mutex);
> > > +}
> > > +EXPORT_SYMBOL(drm_client_add);
> > > /**
> > > * drm_client_release - Release DRM client resources
> > > * @client: DRM client
> > > *
> > > - * Releases resources by closing the &drm_file that was opened by
> > > drm_client_new().
> > > + * Releases resources by closing the &drm_file that was opened by
> > > drm_client_init().
> > > * It is called automatically if the &drm_client_funcs.unregister
> > > callback is _not_ set.
> > > *
> > > * This function should only be called from the unregister
> > > callback. An exception
> > > diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
> > > b/drivers/gpu/drm/drm_fb_cma_helper.c
> > > index 47e0e2f6642d..fb0dfc62b1b6 100644
> > > --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> > > +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> > > @@ -167,7 +167,7 @@ struct drm_fbdev_cma *drm_fbdev_cma_init(struct
> > > drm_device *dev,
> > > fb_helper = &fbdev_cma->fb_helper;
> > > - ret = drm_client_new(dev, &fb_helper->client, "fbdev", NULL);
> > > + ret = drm_client_init(dev, &fb_helper->client, "fbdev", NULL);
> > > if (ret)
> > > goto err_free;
> > > @@ -176,6 +176,8 @@ struct drm_fbdev_cma
> > > *drm_fbdev_cma_init(struct drm_device *dev,
> > > if (ret)
> > > goto err_client_put;
> > > + drm_client_add(&fb_helper->client);
> > > +
> > > return fbdev_cma;
> > > err_client_put:
> > > diff --git a/drivers/gpu/drm/drm_fb_helper.c
> > > b/drivers/gpu/drm/drm_fb_helper.c
> > > index a504a5e05676..e9095537e0c0 100644
> > > --- a/drivers/gpu/drm/drm_fb_helper.c
> > > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > > @@ -3229,13 +3229,15 @@ int drm_fbdev_generic_setup(struct
> > > drm_device *dev, unsigned int preferred_bpp)
> > > if (!fb_helper)
> > > return -ENOMEM;
> > > - ret = drm_client_new(dev, &fb_helper->client, "fbdev",
> > > &drm_fbdev_client_funcs);
> > > + ret = drm_client_init(dev, &fb_helper->client, "fbdev",
> > > &drm_fbdev_client_funcs);
> > > if (ret) {
> > > kfree(fb_helper);
> > > DRM_DEV_ERROR(dev->dev, "Failed to register client: %d\n",
> > > ret);
> > > return ret;
> > > }
> > > + drm_client_add(&fb_helper->client);
> > > +
> > > fb_helper->preferred_bpp = preferred_bpp;
> > > ret = drm_fbdev_client_hotplug(&fb_helper->client);
> > > diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h
> > > index 989f8e52864d..971bb7853776 100644
> > > --- a/include/drm/drm_client.h
> > > +++ b/include/drm/drm_client.h
> > > @@ -87,9 +87,10 @@ struct drm_client_dev {
> > > struct drm_file *file;
> > > };
> > > -int drm_client_new(struct drm_device *dev, struct drm_client_dev
> > > *client,
> > > - const char *name, const struct drm_client_funcs *funcs);
> > > +int drm_client_init(struct drm_device *dev, struct drm_client_dev
> > > *client,
> > > + const char *name, const struct drm_client_funcs *funcs);
> > > void drm_client_release(struct drm_client_dev *client);
> > > +void drm_client_add(struct drm_client_dev *client);
> > > void drm_client_dev_unregister(struct drm_device *dev);
> > > void drm_client_dev_hotplug(struct drm_device *dev);
> >
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-11-05 9:16 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-01 19:45 [PATCH v3] drm/cma-helper: Fix crash in fbdev error path Noralf Trønnes
2018-10-02 11:14 ` Noralf Trønnes
2018-11-04 16:14 ` Sergey Suloev
2018-11-04 17:10 ` Sergey Suloev
2018-11-05 9:16 ` Daniel Vetter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.