[PATCH 01/10] Revert "drm/msm: Add missing check and destroy for alloc_ordered_workqueue"

[PATCH 01/10] Revert "drm/msm: Add missing check and destroy for alloc_ordered_workqueue"

[Johan Hovold]

This reverts commit 643b7d0869cc7f1f7a5ac7ca6bd25d88f54e31d0.

A recent patch that tried to fix up the msm_drm_init() paths with
respect to the workqueue but only ended up making things worse:

First, the newly added calls to msm_drm_uninit() on early errors would
trigger NULL-pointer dereferences, for example, as the kms pointer would
not have been initialised. (Note that these paths were also modified by
a second broken error handling patch which in effect cancelled out this
part when merged.)

Second, the newly added allocation sanity check would still leak the
previously allocated drm device.

Instead of trying to salvage what was badly broken (and clearly not
tested), let's revert the bad commit so that clean and backportable
fixes can be added in its place.

Fixes: 643b7d0869cc ("drm/msm: Add missing check and destroy for alloc_ordered_workqueue")
Cc: Jiasheng Jiang <jiasheng@iscas.ac.cn>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/gpu/drm/msm/msm_drv.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index aca48c868c14..b7f5a78eadd4 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -420,8 +420,6 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
 	priv->dev = ddev;
 
 	priv->wq = alloc_ordered_workqueue("msm", 0);
-	if (!priv->wq)
-		return -ENOMEM;
 
 	INIT_LIST_HEAD(&priv->objects);
 	mutex_init(&priv->obj_lock);
-- 
2.39.2

Re: [PATCH 01/10] Revert "drm/msm: Add missing check and destroy for alloc_ordered_workqueue"

[Jiasheng Jiang]

On Mon, 06 Mar 2023 18:07:13 +0800, Johan Hovold wrote:
> This reverts commit 643b7d0869cc7f1f7a5ac7ca6bd25d88f54e31d0.

The commit not only adds the allocation sanity check, but also adds the
destroy_workqueue to release the allocated priv->wq.
Therefore, revert the commit will cause memory leak.

> A recent patch that tried to fix up the msm_drm_init() paths with
> respect to the workqueue but only ended up making things worse:
> 
> First, the newly added calls to msm_drm_uninit() on early errors would
> trigger NULL-pointer dereferences, for example, as the kms pointer would
> not have been initialised. (Note that these paths were also modified by
> a second broken error handling patch which in effect cancelled out this
> part when merged.)

There is a check for the kms pointer to avoid NULL-pointer dereference in
the msm_drm_uninit().

> Second, the newly added allocation sanity check would still leak the
> previously allocated drm device.

The ddev is allocated by drm_dev_alloc which support automatic cleanup.

Thanks,
Jiang

Re: [PATCH 01/10] Revert "drm/msm: Add missing check and destroy for alloc_ordered_workqueue"

[Johan Hovold]

On Wed, Mar 08, 2023 at 10:10:24AM +0800, Jiasheng Jiang wrote:
> On Mon, 06 Mar 2023 18:07:13 +0800, Johan Hovold wrote:
> > This reverts commit 643b7d0869cc7f1f7a5ac7ca6bd25d88f54e31d0.
> 
> The commit not only adds the allocation sanity check, but also adds the
> destroy_workqueue to release the allocated priv->wq.
> Therefore, revert the commit will cause memory leak.

No, reverting this commit does not cause any memory leaks (look at at
diff again).

The original patch called msm_drm_uninit() in some early error paths,
but that was just completely broken as that function must not be called
before the subcomponents have been bound and also triggered a bunch of
other NULL-pointer dereferences.

That bit was however removed when the change was merged with a second
branch that also touched these error paths. In the end, the leaked wq is
still there and this commit only added broken error handling for the wq
allocation failing (as it does not free the drm device).

> > A recent patch that tried to fix up the msm_drm_init() paths with
> > respect to the workqueue but only ended up making things worse:
> > 
> > First, the newly added calls to msm_drm_uninit() on early errors would
> > trigger NULL-pointer dereferences, for example, as the kms pointer would
> > not have been initialised. (Note that these paths were also modified by
> > a second broken error handling patch which in effect cancelled out this
> > part when merged.)
> 
> There is a check for the kms pointer to avoid NULL-pointer dereference in
> the msm_drm_uninit().

No, there were further places in msm_drm_uninit() which did not have any
such checks when you submitted your patch. Some of the missing checks
were added by a separate patch, but that would not in itself have been
sufficient as with your patch you'd still end up trying to unbind the
subcomponents before they are bound, which will lead to further crashes.
 
> > Second, the newly added allocation sanity check would still leak the
> > previously allocated drm device.
> 
> The ddev is allocated by drm_dev_alloc which support automatic cleanup.

We don't have automatic garbage collection in the kernel. You still need
to release the reference to the device for it to be freed.

Johan

Re: [PATCH 01/10] Revert "drm/msm: Add missing check and destroy for alloc_ordered_workqueue"

[Dmitry Baryshkov]

On 06/03/2023 12:07, Johan Hovold wrote:
> This reverts commit 643b7d0869cc7f1f7a5ac7ca6bd25d88f54e31d0.
> 
> A recent patch that tried to fix up the msm_drm_init() paths with
> respect to the workqueue but only ended up making things worse:
> 
> First, the newly added calls to msm_drm_uninit() on early errors would
> trigger NULL-pointer dereferences, for example, as the kms pointer would
> not have been initialised. (Note that these paths were also modified by
> a second broken error handling patch which in effect cancelled out this
> part when merged.)
> 
> Second, the newly added allocation sanity check would still leak the
> previously allocated drm device.
> 
> Instead of trying to salvage what was badly broken (and clearly not
> tested), let's revert the bad commit so that clean and backportable
> fixes can be added in its place.
> 
> Fixes: 643b7d0869cc ("drm/msm: Add missing check and destroy for alloc_ordered_workqueue")
> Cc: Jiasheng Jiang <jiasheng@iscas.ac.cn>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>   drivers/gpu/drm/msm/msm_drv.c | 2 --
>   1 file changed, 2 deletions(-)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry