[PATCH 1/3] drm/amdkfd: Do copy_to/from_user in general kfd_ioctl()

Christian König deathsimple at vodafone.de
Mon Dec 29 05:05:25 PST 2014


Am 29.12.2014 um 13:42 schrieb Oded Gabbay:
> This patch moves the copy_to_user() and copy_from_user() calls from the
> different ioctl functions in amdkfd to the general kfd_ioctl() function, as
> this is a common code for all ioctls.
>
> This was done according to example taken from drm_ioctl.c
>
> Signed-off-by: Oded Gabbay <oded.gabbay at amd.com>

In general sounds like a good idea to me and the patch is "Reviewed-by: 
Christian König <christian.koenig at amd.com>" for now.

What really questions me is why we need all this code duplication and 
can't reuse the DRM infrastructure for this. But that's more a problem 
in the long term.

Regards,
Christian.

> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 234 +++++++++++++++----------------
>   1 file changed, 117 insertions(+), 117 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> index 7d4974b..5460ad2 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> @@ -127,17 +127,14 @@ static int kfd_open(struct inode *inode, struct file *filep)
>   	return 0;
>   }
>   
> -static long kfd_ioctl_get_version(struct file *filep, struct kfd_process *p,
> -					void __user *arg)
> +static int kfd_ioctl_get_version(struct file *filep, struct kfd_process *p,
> +					void *data)
>   {
> -	struct kfd_ioctl_get_version_args args;
> +	struct kfd_ioctl_get_version_args *args = data;
>   	int err = 0;
>   
> -	args.major_version = KFD_IOCTL_MAJOR_VERSION;
> -	args.minor_version = KFD_IOCTL_MINOR_VERSION;
> -
> -	if (copy_to_user(arg, &args, sizeof(args)))
> -		err = -EFAULT;
> +	args->major_version = KFD_IOCTL_MAJOR_VERSION;
> +	args->minor_version = KFD_IOCTL_MINOR_VERSION;
>   
>   	return err;
>   }
> @@ -221,10 +218,10 @@ static int set_queue_properties_from_user(struct queue_properties *q_properties,
>   	return 0;
>   }
>   
> -static long kfd_ioctl_create_queue(struct file *filep, struct kfd_process *p,
> -					void __user *arg)
> +static int kfd_ioctl_create_queue(struct file *filep, struct kfd_process *p,
> +					void *data)
>   {
> -	struct kfd_ioctl_create_queue_args args;
> +	struct kfd_ioctl_create_queue_args *args = data;
>   	struct kfd_dev *dev;
>   	int err = 0;
>   	unsigned int queue_id;
> @@ -233,16 +230,13 @@ static long kfd_ioctl_create_queue(struct file *filep, struct kfd_process *p,
>   
>   	memset(&q_properties, 0, sizeof(struct queue_properties));
>   
> -	if (copy_from_user(&args, arg, sizeof(args)))
> -		return -EFAULT;
> -
>   	pr_debug("kfd: creating queue ioctl\n");
>   
> -	err = set_queue_properties_from_user(&q_properties, &args);
> +	err = set_queue_properties_from_user(&q_properties, args);
>   	if (err)
>   		return err;
>   
> -	dev = kfd_device_by_id(args.gpu_id);
> +	dev = kfd_device_by_id(args->gpu_id);
>   	if (dev == NULL)
>   		return -EINVAL;
>   
> @@ -250,7 +244,7 @@ static long kfd_ioctl_create_queue(struct file *filep, struct kfd_process *p,
>   
>   	pdd = kfd_bind_process_to_device(dev, p);
>   	if (IS_ERR(pdd)) {
> -		err = PTR_ERR(pdd);
> +		err = -ESRCH;
>   		goto err_bind_process;
>   	}
>   
> @@ -263,33 +257,26 @@ static long kfd_ioctl_create_queue(struct file *filep, struct kfd_process *p,
>   	if (err != 0)
>   		goto err_create_queue;
>   
> -	args.queue_id = queue_id;
> +	args->queue_id = queue_id;
>   
>   	/* Return gpu_id as doorbell offset for mmap usage */
> -	args.doorbell_offset = args.gpu_id << PAGE_SHIFT;
> -
> -	if (copy_to_user(arg, &args, sizeof(args))) {
> -		err = -EFAULT;
> -		goto err_copy_args_out;
> -	}
> +	args->doorbell_offset = args->gpu_id << PAGE_SHIFT;
>   
>   	mutex_unlock(&p->mutex);
>   
> -	pr_debug("kfd: queue id %d was created successfully\n", args.queue_id);
> +	pr_debug("kfd: queue id %d was created successfully\n", args->queue_id);
>   
>   	pr_debug("ring buffer address == 0x%016llX\n",
> -			args.ring_base_address);
> +			args->ring_base_address);
>   
>   	pr_debug("read ptr address    == 0x%016llX\n",
> -			args.read_pointer_address);
> +			args->read_pointer_address);
>   
>   	pr_debug("write ptr address   == 0x%016llX\n",
> -			args.write_pointer_address);
> +			args->write_pointer_address);
>   
>   	return 0;
>   
> -err_copy_args_out:
> -	pqm_destroy_queue(&p->pqm, queue_id);
>   err_create_queue:
>   err_bind_process:
>   	mutex_unlock(&p->mutex);
> @@ -297,99 +284,90 @@ err_bind_process:
>   }
>   
>   static int kfd_ioctl_destroy_queue(struct file *filp, struct kfd_process *p,
> -					void __user *arg)
> +					void *data)
>   {
>   	int retval;
> -	struct kfd_ioctl_destroy_queue_args args;
> -
> -	if (copy_from_user(&args, arg, sizeof(args)))
> -		return -EFAULT;
> +	struct kfd_ioctl_destroy_queue_args *args = data;
>   
>   	pr_debug("kfd: destroying queue id %d for PASID %d\n",
> -				args.queue_id,
> +				args->queue_id,
>   				p->pasid);
>   
>   	mutex_lock(&p->mutex);
>   
> -	retval = pqm_destroy_queue(&p->pqm, args.queue_id);
> +	retval = pqm_destroy_queue(&p->pqm, args->queue_id);
>   
>   	mutex_unlock(&p->mutex);
>   	return retval;
>   }
>   
>   static int kfd_ioctl_update_queue(struct file *filp, struct kfd_process *p,
> -					void __user *arg)
> +					void *data)
>   {
>   	int retval;
> -	struct kfd_ioctl_update_queue_args args;
> +	struct kfd_ioctl_update_queue_args *args = data;
>   	struct queue_properties properties;
>   
> -	if (copy_from_user(&args, arg, sizeof(args)))
> -		return -EFAULT;
> -
> -	if (args.queue_percentage > KFD_MAX_QUEUE_PERCENTAGE) {
> +	if (args->queue_percentage > KFD_MAX_QUEUE_PERCENTAGE) {
>   		pr_err("kfd: queue percentage must be between 0 to KFD_MAX_QUEUE_PERCENTAGE\n");
>   		return -EINVAL;
>   	}
>   
> -	if (args.queue_priority > KFD_MAX_QUEUE_PRIORITY) {
> +	if (args->queue_priority > KFD_MAX_QUEUE_PRIORITY) {
>   		pr_err("kfd: queue priority must be between 0 to KFD_MAX_QUEUE_PRIORITY\n");
>   		return -EINVAL;
>   	}
>   
> -	if ((args.ring_base_address) &&
> +	if ((args->ring_base_address) &&
>   		(!access_ok(VERIFY_WRITE,
> -			(const void __user *) args.ring_base_address,
> +			(const void __user *) args->ring_base_address,
>   			sizeof(uint64_t)))) {
>   		pr_err("kfd: can't access ring base address\n");
>   		return -EFAULT;
>   	}
>   
> -	if (!is_power_of_2(args.ring_size) && (args.ring_size != 0)) {
> +	if (!is_power_of_2(args->ring_size) && (args->ring_size != 0)) {
>   		pr_err("kfd: ring size must be a power of 2 or 0\n");
>   		return -EINVAL;
>   	}
>   
> -	properties.queue_address = args.ring_base_address;
> -	properties.queue_size = args.ring_size;
> -	properties.queue_percent = args.queue_percentage;
> -	properties.priority = args.queue_priority;
> +	properties.queue_address = args->ring_base_address;
> +	properties.queue_size = args->ring_size;
> +	properties.queue_percent = args->queue_percentage;
> +	properties.priority = args->queue_priority;
>   
>   	pr_debug("kfd: updating queue id %d for PASID %d\n",
> -			args.queue_id, p->pasid);
> +			args->queue_id, p->pasid);
>   
>   	mutex_lock(&p->mutex);
>   
> -	retval = pqm_update_queue(&p->pqm, args.queue_id, &properties);
> +	retval = pqm_update_queue(&p->pqm, args->queue_id, &properties);
>   
>   	mutex_unlock(&p->mutex);
>   
>   	return retval;
>   }
>   
> -static long kfd_ioctl_set_memory_policy(struct file *filep,
> -				struct kfd_process *p, void __user *arg)
> +static int kfd_ioctl_set_memory_policy(struct file *filep,
> +					struct kfd_process *p, void *data)
>   {
> -	struct kfd_ioctl_set_memory_policy_args args;
> +	struct kfd_ioctl_set_memory_policy_args *args = data;
>   	struct kfd_dev *dev;
>   	int err = 0;
>   	struct kfd_process_device *pdd;
>   	enum cache_policy default_policy, alternate_policy;
>   
> -	if (copy_from_user(&args, arg, sizeof(args)))
> -		return -EFAULT;
> -
> -	if (args.default_policy != KFD_IOC_CACHE_POLICY_COHERENT
> -	    && args.default_policy != KFD_IOC_CACHE_POLICY_NONCOHERENT) {
> +	if (args->default_policy != KFD_IOC_CACHE_POLICY_COHERENT
> +	    && args->default_policy != KFD_IOC_CACHE_POLICY_NONCOHERENT) {
>   		return -EINVAL;
>   	}
>   
> -	if (args.alternate_policy != KFD_IOC_CACHE_POLICY_COHERENT
> -	    && args.alternate_policy != KFD_IOC_CACHE_POLICY_NONCOHERENT) {
> +	if (args->alternate_policy != KFD_IOC_CACHE_POLICY_COHERENT
> +	    && args->alternate_policy != KFD_IOC_CACHE_POLICY_NONCOHERENT) {
>   		return -EINVAL;
>   	}
>   
> -	dev = kfd_device_by_id(args.gpu_id);
> +	dev = kfd_device_by_id(args->gpu_id);
>   	if (dev == NULL)
>   		return -EINVAL;
>   
> @@ -397,23 +375,23 @@ static long kfd_ioctl_set_memory_policy(struct file *filep,
>   
>   	pdd = kfd_bind_process_to_device(dev, p);
>   	if (IS_ERR(pdd)) {
> -		err = PTR_ERR(pdd);
> +		err = -ESRCH;
>   		goto out;
>   	}
>   
> -	default_policy = (args.default_policy == KFD_IOC_CACHE_POLICY_COHERENT)
> +	default_policy = (args->default_policy == KFD_IOC_CACHE_POLICY_COHERENT)
>   			 ? cache_policy_coherent : cache_policy_noncoherent;
>   
>   	alternate_policy =
> -		(args.alternate_policy == KFD_IOC_CACHE_POLICY_COHERENT)
> +		(args->alternate_policy == KFD_IOC_CACHE_POLICY_COHERENT)
>   		   ? cache_policy_coherent : cache_policy_noncoherent;
>   
>   	if (!dev->dqm->set_cache_memory_policy(dev->dqm,
>   				&pdd->qpd,
>   				default_policy,
>   				alternate_policy,
> -				(void __user *)args.alternate_aperture_base,
> -				args.alternate_aperture_size))
> +				(void __user *)args->alternate_aperture_base,
> +				args->alternate_aperture_size))
>   		err = -EINVAL;
>   
>   out:
> @@ -422,53 +400,44 @@ out:
>   	return err;
>   }
>   
> -static long kfd_ioctl_get_clock_counters(struct file *filep,
> -				struct kfd_process *p, void __user *arg)
> +static int kfd_ioctl_get_clock_counters(struct file *filep,
> +				struct kfd_process *p, void *data)
>   {
> -	struct kfd_ioctl_get_clock_counters_args args;
> +	struct kfd_ioctl_get_clock_counters_args *args = data;
>   	struct kfd_dev *dev;
>   	struct timespec time;
>   
> -	if (copy_from_user(&args, arg, sizeof(args)))
> -		return -EFAULT;
> -
> -	dev = kfd_device_by_id(args.gpu_id);
> +	dev = kfd_device_by_id(args->gpu_id);
>   	if (dev == NULL)
>   		return -EINVAL;
>   
>   	/* Reading GPU clock counter from KGD */
> -	args.gpu_clock_counter = kfd2kgd->get_gpu_clock_counter(dev->kgd);
> +	args->gpu_clock_counter = kfd2kgd->get_gpu_clock_counter(dev->kgd);
>   
>   	/* No access to rdtsc. Using raw monotonic time */
>   	getrawmonotonic(&time);
> -	args.cpu_clock_counter = (uint64_t)timespec_to_ns(&time);
> +	args->cpu_clock_counter = (uint64_t)timespec_to_ns(&time);
>   
>   	get_monotonic_boottime(&time);
> -	args.system_clock_counter = (uint64_t)timespec_to_ns(&time);
> +	args->system_clock_counter = (uint64_t)timespec_to_ns(&time);
>   
>   	/* Since the counter is in nano-seconds we use 1GHz frequency */
> -	args.system_clock_freq = 1000000000;
> -
> -	if (copy_to_user(arg, &args, sizeof(args)))
> -		return -EFAULT;
> +	args->system_clock_freq = 1000000000;
>   
>   	return 0;
>   }
>   
>   
>   static int kfd_ioctl_get_process_apertures(struct file *filp,
> -				struct kfd_process *p, void __user *arg)
> +				struct kfd_process *p, void *data)
>   {
> -	struct kfd_ioctl_get_process_apertures_args args;
> +	struct kfd_ioctl_get_process_apertures_args *args = data;
>   	struct kfd_process_device_apertures *pAperture;
>   	struct kfd_process_device *pdd;
>   
>   	dev_dbg(kfd_device, "get apertures for PASID %d", p->pasid);
>   
> -	if (copy_from_user(&args, arg, sizeof(args)))
> -		return -EFAULT;
> -
> -	args.num_of_nodes = 0;
> +	args->num_of_nodes = 0;
>   
>   	mutex_lock(&p->mutex);
>   
> @@ -477,7 +446,8 @@ static int kfd_ioctl_get_process_apertures(struct file *filp,
>   		/* Run over all pdd of the process */
>   		pdd = kfd_get_first_process_device_data(p);
>   		do {
> -			pAperture = &args.process_apertures[args.num_of_nodes];
> +			pAperture =
> +				&args->process_apertures[args->num_of_nodes];
>   			pAperture->gpu_id = pdd->dev->id;
>   			pAperture->lds_base = pdd->lds_base;
>   			pAperture->lds_limit = pdd->lds_limit;
> @@ -487,7 +457,7 @@ static int kfd_ioctl_get_process_apertures(struct file *filp,
>   			pAperture->scratch_limit = pdd->scratch_limit;
>   
>   			dev_dbg(kfd_device,
> -				"node id %u\n", args.num_of_nodes);
> +				"node id %u\n", args->num_of_nodes);
>   			dev_dbg(kfd_device,
>   				"gpu id %u\n", pdd->dev->id);
>   			dev_dbg(kfd_device,
> @@ -503,23 +473,23 @@ static int kfd_ioctl_get_process_apertures(struct file *filp,
>   			dev_dbg(kfd_device,
>   				"scratch_limit %llX\n", pdd->scratch_limit);
>   
> -			args.num_of_nodes++;
> +			args->num_of_nodes++;
>   		} while ((pdd = kfd_get_next_process_device_data(p, pdd)) != NULL &&
> -				(args.num_of_nodes < NUM_OF_SUPPORTED_GPUS));
> +				(args->num_of_nodes < NUM_OF_SUPPORTED_GPUS));
>   	}
>   
>   	mutex_unlock(&p->mutex);
>   
> -	if (copy_to_user(arg, &args, sizeof(args)))
> -		return -EFAULT;
> -
>   	return 0;
>   }
>   
>   static long kfd_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
>   {
>   	struct kfd_process *process;
> -	long err = -EINVAL;
> +	char stack_kdata[128];
> +	char *kdata = NULL;
> +	unsigned int usize, asize;
> +	int retcode = -EINVAL;
>   
>   	dev_dbg(kfd_device,
>   		"ioctl cmd 0x%x (#%d), arg 0x%lx\n",
> @@ -529,54 +499,84 @@ static long kfd_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
>   	if (IS_ERR(process))
>   		return PTR_ERR(process);
>   
> +	if (cmd & (IOC_IN | IOC_OUT)) {
> +		if (asize <= sizeof(stack_kdata)) {
> +			kdata = stack_kdata;
> +		} else {
> +			kdata = kmalloc(asize, GFP_KERNEL);
> +			if (!kdata) {
> +				retcode = -ENOMEM;
> +				goto err_i1;
> +			}
> +		}
> +		if (asize > usize)
> +			memset(kdata + usize, 0, asize - usize);
> +	}
> +
> +	if (cmd & IOC_IN) {
> +		if (copy_from_user(kdata, (void __user *)arg, usize) != 0) {
> +			retcode = -EFAULT;
> +			goto err_i1;
> +		}
> +	} else if (cmd & IOC_OUT) {
> +		memset(kdata, 0, usize);
> +	}
> +
> +
>   	switch (cmd) {
>   	case KFD_IOC_GET_VERSION:
> -		err = kfd_ioctl_get_version(filep, process, (void __user *)arg);
> +		retcode = kfd_ioctl_get_version(filep, process, kdata);
>   		break;
>   	case KFD_IOC_CREATE_QUEUE:
> -		err = kfd_ioctl_create_queue(filep, process,
> -						(void __user *)arg);
> +		retcode = kfd_ioctl_create_queue(filep, process,
> +						kdata);
>   		break;
>   
>   	case KFD_IOC_DESTROY_QUEUE:
> -		err = kfd_ioctl_destroy_queue(filep, process,
> -						(void __user *)arg);
> +		retcode = kfd_ioctl_destroy_queue(filep, process,
> +						kdata);
>   		break;
>   
>   	case KFD_IOC_SET_MEMORY_POLICY:
> -		err = kfd_ioctl_set_memory_policy(filep, process,
> -						(void __user *)arg);
> +		retcode = kfd_ioctl_set_memory_policy(filep, process,
> +						kdata);
>   		break;
>   
>   	case KFD_IOC_GET_CLOCK_COUNTERS:
> -		err = kfd_ioctl_get_clock_counters(filep, process,
> -						(void __user *)arg);
> +		retcode = kfd_ioctl_get_clock_counters(filep, process,
> +						kdata);
>   		break;
>   
>   	case KFD_IOC_GET_PROCESS_APERTURES:
> -		err = kfd_ioctl_get_process_apertures(filep, process,
> -						(void __user *)arg);
> +		retcode = kfd_ioctl_get_process_apertures(filep, process,
> +						kdata);
>   		break;
>   
>   	case KFD_IOC_UPDATE_QUEUE:
> -		err = kfd_ioctl_update_queue(filep, process,
> -						(void __user *)arg);
> +		retcode = kfd_ioctl_update_queue(filep, process,
> +						kdata);
>   		break;
>   
>   	default:
> -		dev_err(kfd_device,
> +		dev_dbg(kfd_device,
>   			"unknown ioctl cmd 0x%x, arg 0x%lx)\n",
>   			cmd, arg);
> -		err = -EINVAL;
> +		retcode = -EINVAL;
>   		break;
>   	}
>   
> -	if (err < 0)
> -		dev_err(kfd_device,
> -			"ioctl error %ld for ioctl cmd 0x%x (#%d)\n",
> -			err, cmd, _IOC_NR(cmd));
> +	if (cmd & IOC_OUT)
> +		if (copy_to_user((void __user *)arg, kdata, usize) != 0)
> +			retcode = -EFAULT;
>   
> -	return err;
> +err_i1:
> +	if (kdata != stack_kdata)
> +		kfree(kdata);
> +
> +	if (retcode)
> +		dev_dbg(kfd_device, "ret = %d\n", retcode);
> +
> +	return retcode;
>   }
>   
>   static int kfd_mmap(struct file *filp, struct vm_area_struct *vma)



More information about the dri-devel mailing list