drm/tegra: Fix lockup on a use of staging API
PATCH 1/3 drm/tegra: Fix lockup on a use of staging API
Commit bdd2f9cd ("Don't leak kernel pointer to userspace") added a mutex around staging IOCTL's, some of those mutexes are taken twice.
--- drivers/gpu/drm/tegra/drm.c | 20 ++++---------------- 1 file changed, 4 insertions(+), 16 deletions(-)
drivers/gpu/drm/tegra/drm.c +4 -16
@@ -430,18 +430,6 @@ int tegra_drm_submit(struct tegra_drm_context *context,#ifdef CONFIG_DRM_TEGRA_STAGING -static struct tegra_drm_context * -tegra_drm_file_get_context (struct tegra_drm_file *file,u32 id) -{ -struct tegra_drm_context *context; - -mutex_lock (&file->lock); - context =idr_find (&file->contexts, id); -mutex_unlock (&file->lock); - -return context; -} -static int tegra_gem_create (struct drm_device *drm,void *data,struct drm_file *file) {
@@ -585,7 +573,7 @@ static int tegra_close_channel(struct drm_device *drm, void *data,mutex_lock (&fpriv->lock); - context =tegra_drm_file_get_context (fpriv, args->context); + context =idr_find (&fpriv->contexts, args->context);if (!context) { err = -EINVAL ; goto unlock;
@@ -610,7 +598,7 @@ static int tegra_get_syncpt(struct drm_device *drm, void *data,mutex_lock (&fpriv->lock); - context =tegra_drm_file_get_context (fpriv, args->context); + context =idr_find (&fpriv->contexts, args->context);if (!context) { err = -ENODEV ; goto unlock;
@@ -639,7 +627,7 @@ static int tegra_submit(struct drm_device *drm, void *data,mutex_lock (&fpriv->lock); - context =tegra_drm_file_get_context (fpriv, args->context); + context =idr_find (&fpriv->contexts, args->context);if (!context) { err = -ENODEV ; goto unlock;
@@ -664,7 +652,7 @@ static int tegra_get_syncpt_base(struct drm_device *drm, void *data,mutex_lock (&fpriv->lock); - context =tegra_drm_file_get_context (fpriv, args->context); + context =idr_find (&fpriv->contexts, args->context);if (!context) { err = -ENODEV ; goto unlock;
Signature · 1 line
2.12.2
A message that is not in the archive (sent off-list or not yet received): 20170512190044.17541-1-digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
PATCH 2/3 drm/tegra: Correct idr_alloc() minimum id
The start = 0 is invalid and causes weird CDMA channel timeouts, presumably some memory misuse/corruption is going on.
--- drivers/gpu/drm/tegra/drm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/tegra/drm.c +1 -1
@@ -518,7 +518,7 @@ static int tegra_client_open(struct tegra_drm_file *fpriv,if (err <0 )return err; - err =idr_alloc (&fpriv->contexts, context,0 ,0 ,GFP_KERNEL ); + err =idr_alloc (&fpriv->contexts, context,1 ,0 ,GFP_KERNEL );if (err <0 ) { client->ops->close_channel (context);return err;
Signature · 1 line
2.12.2
A message that is not in the archive (sent off-list or not yet received): 20170512190044.17541-2-digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
Re: [PATCH 2/3] drm/tegra: Correct idr_alloc() minimum id
Quoted 2 lines · Dmitry Osipenko
The start = 0 is invalid and causes weird CDMA channel timeouts, presumably some memory misuse/corruption is going on.
What makes you think start = 0 is invalid? I can't see anything pointing to that in the idr code and there are many users in the kernel passing 0 as start.
Quoted 21 lines
Fixes bdd2f9cd10eb ("drm/tegra: Don't leak kernel pointer to userspace")Signed-off-by Dmitry Osipenko <[email protected]>--- drivers/gpu/drm/tegra/drm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/gpu/drm/tegra/drm.c +1
-1@@ -518,7 +518,7 @@ static int tegra_client_open(struct tegra_drm_file *fpriv,if (err <0 )return err; - err =idr_alloc (&fpriv->contexts, context,0 ,0 ,GFP_KERNEL ); + err =idr_alloc (&fpriv->contexts, context,1 ,0 ,GFP_KERNEL );if (err <0 ) { client->ops->close_channel (context);return err;
A message that is not in the archive (sent off-list or not yet received): b30acd8a-db8c-f43c-7c40-0064444466a0-/[email protected]
Re: [PATCH 2/3] drm/tegra: Correct idr_alloc() minimum id
Quoted 7 lines · Mikko Perttunen
Quoted 2 lines · Dmitry Osipenko
The start = 0 is invalid and causes weird CDMA channel timeouts, presumably some memory misuse/corruption is going on.
What makes you think start = 0 is invalid? I can't see anything pointing to that in the idr code and there are many users in the kernel passing 0 as start.
Well, I can't see either. You are right that there are quite many others with 0 as a start, the 1 probably just masks the bug.
Signature · 1 line
Dmitry
A message that is not in the archive (sent off-list or not yet received): b61d6877-cbfc-9b14-93d2-cf6432452fd8-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
Re: [PATCH 2/3] drm/tegra: Correct idr_alloc() minimum id
Quoted 5 lines · Dmitry Osipenko
Quoted 4 lines · Mikko Perttunen
Quoted 2 lines · Dmitry Osipenko
The start = 0 is invalid and causes weird CDMA channel timeouts, presumably some memory misuse/corruption is going on.
Quoted 7 lines
Quoted 3 lines
What makes you think start = 0 is invalid? I can't see anything pointing to that in the idr code and there are many users in the kernel passing 0 as start.
Well, I can't see either. You are right that there are quite many others with 0 as a start, the 1 probably just masks the bug.
Finally, I found the root of the issue. The job->client is set to the context ID in the tegra_drm_submit() and the host1x_cdma sets client ID to 0 to mark CDMA job timeout timer as already armed. I'll send V2 with a new commit description.
Signature · 1 line
Dmitry
Mikko Perttunen
Reviewed-byQuoted 69 lines · Dmitry Osipenko
Commit
bdd2f9cd("Don't leak kernel pointer to userspace") added a mutex around staging IOCTL's, some of those mutexes are taken twice.Fixes bdd2f9cd10eb ("drm/tegra: Don't leak kernel pointer to userspace")Signed-off-by Dmitry Osipenko <[email protected]>--- drivers/gpu/drm/tegra/drm.c | 20 ++++---------------- 1 file changed, 4 insertions(+), 16 deletions(-)drivers/gpu/drm/tegra/drm.c +4
-16@@ -430,18 +430,6 @@ int tegra_drm_submit(struct tegra_drm_context *context,#ifdef CONFIG_DRM_TEGRA_STAGING -static struct tegra_drm_context * -tegra_drm_file_get_context (struct tegra_drm_file *file,u32 id) -{ -struct tegra_drm_context *context; - -mutex_lock (&file->lock); - context =idr_find (&file->contexts, id); -mutex_unlock (&file->lock); - -return context; -} -static int tegra_gem_create (struct drm_device *drm,void *data,struct drm_file *file) {@@ -585,7 +573,7 @@ static int tegra_close_channel(struct drm_device *drm, void *data,mutex_lock (&fpriv->lock); - context =tegra_drm_file_get_context (fpriv, args->context); + context =idr_find (&fpriv->contexts, args->context);if (!context) { err = -EINVAL ; goto unlock;@@ -610,7 +598,7 @@ static int tegra_get_syncpt(struct drm_device *drm, void *data,mutex_lock (&fpriv->lock); - context =tegra_drm_file_get_context (fpriv, args->context); + context =idr_find (&fpriv->contexts, args->context);if (!context) { err = -ENODEV ; goto unlock;@@ -639,7 +627,7 @@ static int tegra_submit(struct drm_device *drm, void *data,mutex_lock (&fpriv->lock); - context =tegra_drm_file_get_context (fpriv, args->context); + context =idr_find (&fpriv->contexts, args->context);if (!context) { err = -ENODEV ; goto unlock;@@ -664,7 +652,7 @@ static int tegra_get_syncpt_base(struct drm_device *drm, void *data,mutex_lock (&fpriv->lock); - context =tegra_drm_file_get_context (fpriv, args->context); + context =idr_find (&fpriv->contexts, args->context);if (!context) { err = -ENODEV ; goto unlock;