| Message ID | 20240806-omapdrm-misc-fixes-v1-3-15d31aea0831@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/omap: Minor fixes | expand |
… > +++ b/drivers/gpu/drm/omapdrm/omap_gem.c … > @@ -1418,21 +1416,17 @@ struct drm_gem_object *omap_gem_new_dmabuf(struct drm_device *dev, size_t size, > pages = kcalloc(npages, sizeof(*pages), GFP_KERNEL); > if (!pages) { > omap_gem_free_object(obj); > - obj = ERR_PTR(-ENOMEM); > - goto done; > + return ERR_PTR(-ENOMEM); > } > > omap_obj->pages = pages; > ret = drm_prime_sg_to_page_array(sgt, pages, npages); > if (ret) { > omap_gem_free_object(obj); > - obj = ERR_PTR(-ENOMEM); > - goto done; > + return ERR_PTR(-ENOMEM); > } … I suggest to use another goto chain instead so that a bit of exception handling can be better reused at the end of this function implementation. Regards, Markus
diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c index 9ea0c64c26b5..ebbe80c1c0ef 100644 --- a/drivers/gpu/drm/omapdrm/omap_gem.c +++ b/drivers/gpu/drm/omapdrm/omap_gem.c @@ -1402,8 +1402,6 @@ struct drm_gem_object *omap_gem_new_dmabuf(struct drm_device *dev, size_t size, omap_obj = to_omap_bo(obj); - mutex_lock(&omap_obj->lock); - omap_obj->sgt = sgt; if (omap_gem_sgt_is_contiguous(sgt, size)) { @@ -1418,21 +1416,17 @@ struct drm_gem_object *omap_gem_new_dmabuf(struct drm_device *dev, size_t size, pages = kcalloc(npages, sizeof(*pages), GFP_KERNEL); if (!pages) { omap_gem_free_object(obj); - obj = ERR_PTR(-ENOMEM); - goto done; + return ERR_PTR(-ENOMEM); } omap_obj->pages = pages; ret = drm_prime_sg_to_page_array(sgt, pages, npages); if (ret) { omap_gem_free_object(obj); - obj = ERR_PTR(-ENOMEM); - goto done; + return ERR_PTR(-ENOMEM); } } -done: - mutex_unlock(&omap_obj->lock); return obj; }
omap_gem_new_dmabuf() creates the new gem object, and then takes and holds the omap_obj->lock for the rest of the function. This has two issues: - omap_gem_free_object(), which is called in the error paths, also takes the same lock, leading to deadlock - Even if the above wouldn't happen, in the error cases omap_gem_new_dmabuf() still unlocks omap_obj->lock, even after the omap_obj has already been freed. Furthermore, I don't think there's any reason to take the lock at all, as the object was just created and not yet shared with anyone else. To fix all this, drop taking the lock. Fixes: 3cbd0c587b12 ("drm/omap: gem: Replace struct_mutex usage with omap_obj private lock") Reported-by: Dan Carpenter <dan.carpenter@linaro.org> Closes: https://lore.kernel.org/all/511b99d7-aade-4f92-bd3e-63163a13d617@stanley.mountain/ Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> --- drivers/gpu/drm/omapdrm/omap_gem.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)