Re: [PATCH] drm/msm/gem: Add some cleanup.h

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/msm: uninitialized return value in msm_ioctl_gem_info_get_metadata()
--

commit 26d3997a404e96c83bc421df7eb395c717c9751f
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm/gem: Add some cleanup.h

This commit converts msm_gem_lock() and related locking helpers to use
scope-based cleanup from cleanup.h, while also fixing a lock leak in
msm_ioctl_gem_info_set_metadata().

> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 5e8f3ce26b32c..42381db0c4686 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c

[ ... ]

> @@ -607,21 +600,14 @@ static int msm_ioctl_gem_info_get_metadata(struct drm_gem_object *obj,
>  		return 0;
>  	}
>  
> -	ret = msm_gem_lock_interruptible(obj);
> -	if (ret)
> -		return ret;
> -
> -	/* Avoid copy_to_user() under gem obj lock: */
> -	len = msm_obj->metadata_size;
> -	buf = kmemdup(msm_obj->metadata, len, GFP_KERNEL);
> -
> -	if (!buf) {
> -		msm_gem_unlock(obj);
> -		return -ENOMEM;
> +	scoped_cond_guard (gem_lock_intr, return -EINTR, obj) {
> +		/* Avoid copy_to_user() under gem obj lock by making a copy: */
> +		len = msm_obj->metadata_size;
> +		buf = kmemdup(msm_obj->metadata, len, GFP_KERNEL);
> +		if (!buf)
> +			return -ENOMEM;
>  	}
>  
> -	msm_gem_unlock(obj);
> -
>  	if (*metadata_size < len) {
>  		ret = -ETOOSMALL;
>  	} else if (copy_to_user(metadata, buf, len)) {

[Severity: High]
Does msm_ioctl_gem_info_get_metadata() return an uninitialized ret on the
success path?

Previously, ret was initialized to 0 by:

    ret = msm_gem_lock_interruptible(obj);

With that replaced by scoped_cond_guard, when *metadata_size >= len and
copy_to_user() succeeds, execution takes the final else branch without
assigning ret:

drivers/gpu/drm/msm/msm_drv.c:msm_ioctl_gem_info_get_metadata() {
    ...
	int ret, len;
    ...
	if (*metadata_size < len) {
		ret = -ETOOSMALL;
	} else if (copy_to_user(metadata, buf, len)) {
		ret = -EFAULT;
	} else {
		*metadata_size = len;
	}

	kfree(buf);

	return ret;
}

Should ret be initialized to 0 (or set to 0 in the else branch)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026213701.22126-1-robin.clark@oss.qualcomm.com?part=1