Re: [PATCH v4 09/31] accel: ethosu: Guard operations after unplug

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] accel: ethosu: mixing __free() and scoped_guard() with goto cleanup labels
--

commit cbd771f7e8b7995eaa7aedcd81a7cde35a4b6ada
Author: Rob Herring (Arm) <robh@kernel.org>

accel: ethosu: Guard operations after unplug

This commit guards ethosu device file open and private ioctl operations
with drm_dev_enter() and drm_dev_exit(), and switches device removal from
drm_dev_unregister() to drm_dev_unplug(). It also drops drm_dev_enter()
from the BO wait ioctl so fence waits do not delay scheduler teardown.

> diff --git a/drivers/accel/ethosu/ethosu_drv.c b/drivers/accel/ethosu/ethosu_drv.c
> index 6875ed17a004d..91b2c2ab0a03e 100644
> --- a/drivers/accel/ethosu/ethosu_drv.c
> +++ b/drivers/accel/ethosu/ethosu_drv.c
[ ... ]
> @@ -139,27 +132,34 @@ static int ethosu_ioctl_cmdstream_bo_create(struct drm_device *ddev, void *data,
>  
>  static int ethosu_open(struct drm_device *ddev, struct drm_file *file)
>  {
> -	int ret = 0;
> +	struct ethosu_file_priv __free(kfree) * priv = NULL;
> +	int cookie, ret;
>  
>  	if (!try_module_get(THIS_MODULE))
>  		return -EINVAL;
> +	if (!drm_dev_enter(ddev, &cookie)) {
> +		module_put(THIS_MODULE);
> +		return -ENODEV;
> +	}
>  
> -	struct ethosu_file_priv __free(kfree) *priv = kzalloc_obj(*priv);
> +	priv = kzalloc_obj(*priv);
>  	if (!priv) {
>  		ret = -ENOMEM;
> -		goto err_put_mod;
> +		goto err_dev_exit;
>  	}
>  	priv->edev = to_ethosu_device(ddev);
>  
>  	ret = ethosu_job_open(priv);
>  	if (ret)
> -		goto err_put_mod;
> +		goto err_dev_exit;
>  
>  	ethosu_perfmon_open_file(priv);
>  	file->driver_priv = no_free_ptr(priv);
> +	drm_dev_exit(cookie);
>  	return 0;
>  
> -err_put_mod:
> +err_dev_exit:
> +	drm_dev_exit(cookie);
>  	module_put(THIS_MODULE);
>  	return ret;
>  }

[Severity: Medium]
In drivers/accel/ethosu/ethosu_drv.c, ethosu_open() declares priv with
__free(kfree) = NULL at the top of the function before try_module_get() and
drm_dev_enter(), and then mixes __free(kfree) with goto err_dev_exit when
kzalloc_obj() or ethosu_job_open() fails.

Because scope-based cleanup runs when the function scope exits after the
err_dev_exit label executes, drm_dev_exit(cookie) and
module_put(THIS_MODULE) will run before kfree(priv), reversing the reverse
acquisition (lifo) teardown order and mixing goto error handling with
cleanup.h helpers.

Should ethosu_open() avoid mixing __free(kfree) and goto labels so that
priv is freed before exiting the drm device section and dropping the module
reference?

> diff --git a/drivers/accel/ethosu/ethosu_job.c b/drivers/accel/ethosu/ethosu_job.c
> index f22cacb19e3dc..8ffdc83d06f98 100644
> --- a/drivers/accel/ethosu/ethosu_job.c
> +++ b/drivers/accel/ethosu/ethosu_job.c
[ ... ]
> @@ -557,26 +558,34 @@ static int ethosu_ioctl_submit_job(struct drm_device *dev, struct drm_file *file
>  int ethosu_ioctl_submit(struct drm_device *dev, void *data, struct drm_file *file)
>  {
>  	struct drm_ethosu_submit *args = data;
> -	int ret = 0;
> +	int cookie, ret = 0;
>  	unsigned int i = 0;
>  
> +	if (!drm_dev_enter(dev, &cookie))
> +		return -ENODEV;
> +
>  	struct drm_ethosu_job __free(kvfree) *jobs =
>  		kvmalloc_objs(*jobs, args->job_count);
> -	if (!jobs)
> -		return -ENOMEM;
> +	if (!jobs) {
> +		ret = -ENOMEM;
> +		goto out_dev_exit;
> +	}
>  
>  	if (copy_from_user(jobs,
>  			   (void __user *)(uintptr_t)args->jobs,
>  			   args->job_count * sizeof(*jobs))) {
>  		drm_dbg(dev, "Failed to copy incoming job array\n");
> -		ret = -EFAULT;
> +		ret = -EFAULT;
> +		goto out_dev_exit;
>  	}
>  
>  	for (i = 0; i < args->job_count; i++) {
>  		ret = ethosu_ioctl_submit_job(dev, file, &jobs[i], args->perfmon_id);
>  		if (ret)
> -			return ret;
> +			goto out_dev_exit;
>  	}
>  
> -	return 0;
> +out_dev_exit:
> +	drm_dev_exit(cookie);
> +	return ret;
>  }
> diff --git a/drivers/accel/ethosu/ethosu_perfmon.c b/drivers/accel/ethosu/ethosu_perfmon.c
> index 2a9b3cc1447bd..8d3635ba3347b 100644
> --- a/drivers/accel/ethosu/ethosu_perfmon.c
> +++ b/drivers/accel/ethosu/ethosu_perfmon.c
[ ... ]
> @@ -260,44 +289,54 @@ int ethosu_ioctl_perfmon_set_global(struct drm_device *dev, void *data,
>  	struct ethosu_file_priv *ethosu_priv = file_priv->driver_priv;
>  	struct drm_ethosu_perfmon_set_global *req = data;
>  	struct ethosu_device *ethosu = to_ethosu_device(dev);
> -	struct ethosu_perfmon *perfmon;
> +	struct ethosu_perfmon *old = NULL, *perfmon;
> +	int cookie, ret = 0;
>  
> -	if (req->flags & ~DRM_ETHOSU_PERFMON_CLEAR_GLOBAL)
> -		return -EINVAL;
> +	if (!drm_dev_enter(dev, &cookie))
> +		return -ENODEV;
> +
> +	if (req->flags & ~DRM_ETHOSU_PERFMON_CLEAR_GLOBAL) {
> +		ret = -EINVAL;
> +		goto out_dev_exit;
> +	}
>  
>  	perfmon = ethosu_perfmon_find(ethosu_priv, req->id);
> -	if (!perfmon)
> -		return -EINVAL;
> +	if (!perfmon) {
> +		ret = -EINVAL;
> +		goto out_dev_exit;
> +	}
>  
>  	/* If the request is to clear the global performance monitor */
>  	if (req->flags & DRM_ETHOSU_PERFMON_CLEAR_GLOBAL) {
> -		struct ethosu_perfmon *old;
> -
>  		scoped_guard(mutex, &ethosu->perfmon_state.lock) {
>  			old = ethosu->global_perfmon;
> -			if (!old) {
> -				ethosu_perfmon_put(perfmon);
> -				return -EINVAL;
> +			if (old) {
> +				ethosu->global_perfmon = NULL;
> +				ethosu_perfmon_stop_locked(ethosu, old, true);
>  			}
> +		}
>  
> -			ethosu->global_perfmon = NULL;
> -			ethosu_perfmon_stop_locked(ethosu, old, true);
> +		ethosu_perfmon_put(perfmon);
> +		if (!old) {
> +			ret = -EINVAL;
> +			goto out_dev_exit;
>  		}
>  
>  		ethosu_perfmon_put(old);
> -		ethosu_perfmon_put(perfmon);
>  
> -		return 0;
> +		goto out_dev_exit;
>  	}
>  
>  	scoped_guard(mutex, &ethosu->perfmon_state.lock) {
> -		if (ethosu->perfmon_state.active || ethosu->global_perfmon) {
> -			ethosu_perfmon_put(perfmon);
> -			return -EBUSY;
> -		}
> -
> -		ethosu->global_perfmon = perfmon;
> +		if (ethosu->perfmon_state.active || ethosu->global_perfmon)
> +			ret = -EBUSY;
> +		else
> +			ethosu->global_perfmon = perfmon;
>  	}
> +	if (ret)
> +		ethosu_perfmon_put(perfmon);
>  
> -	return 0;
> +out_dev_exit:
> +	drm_dev_exit(cookie);
> +	return ret;
>  }

[Severity: Medium]
Similarly, in drivers/accel/ethosu/ethosu_job.c, ethosu_ioctl_submit()
mixes a mid-function __free(kvfree) declaration for jobs with goto
out_dev_exit, which causes drm_dev_exit(cookie) to run before kvfree(jobs).
In drivers/accel/ethosu/ethosu_perfmon.c, ethosu_ioctl_perfmon_set_global()
also mixes scoped_guard(mutex, &ethosu->perfmon_state.lock) with goto
out_dev_exit labels.

Per include/linux/cleanup.h, goto-based cleanup and scope-based cleanup
helpers should not be mixed within the same function. Could these functions
either use scope-based cleanup consistently or use explicit cleanup along
the goto paths?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029-ethosu-fixes-v4-0-4aa25e22578e@kernel.org?part=9