Re: [RFCv2 PATCH 01/36] iommu: Keep track of processes and PASIDs

[Jean-Philippe Brucker <jean-philippe.brucker@arm.com>]

Hi Sinan,

Sorry for the delay, thanks for reviewing this!

On 21/10/17 00:32, Sinan Kaya wrote:
> few nits below.
> 
>> +/*
>> + * Allocate a iommu_process structure for the given task.
>> + *
>> + * Ideally we shouldn't need the domain parameter, since iommu_process is
>> + * system-wide, but we use it to retrieve the driver's allocation ops and a
>> + * PASID range.
>> + */
>> +static struct iommu_process *
>> +iommu_process_alloc(struct iommu_domain *domain, struct task_struct *task)
>> +{
>> +	int err;
>> +	int pasid;
>> +	struct iommu_process *process;
>> +
>> +	if (WARN_ON(!domain->ops->process_alloc || !domain->ops->process_free))
>> +		return ERR_PTR(-ENODEV);
>> +
>> +	process = domain->ops->process_alloc(task);
>> +	if (IS_ERR(process))
>> +		return process;
>> +	if (!process)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	process->pid		= get_task_pid(task, PIDTYPE_PID);
>> +	process->release	= domain->ops->process_free;
>> +	INIT_LIST_HEAD(&process->domains);
>> +	kref_init(&process->kref);
>> +
> nit, I think you should place this check right after the pid assignment.

Sure

>> +	if (!process->pid) {
>> +		err = -EINVAL;
>> +		goto err_free_process;
>> +	}
>> +
>> +	idr_preload(GFP_KERNEL);
>> +	spin_lock(&iommu_process_lock);
>> +	pasid = idr_alloc_cyclic(&iommu_process_idr, process, domain->min_pasid,
>> +				 domain->max_pasid + 1, GFP_ATOMIC);
>> +	process->pasid = pasid;
>> +	spin_unlock(&iommu_process_lock);
>> +	idr_preload_end();
>> +
> 
> nit, You can maybe return here if pasid is not negative.

Ok

>> +	if (pasid < 0) {
>> +		err = pasid;
>> +		goto err_put_pid;
>> +	}
>> +
>> +	return process;
>> +
>> +err_put_pid:
>> +	put_pid(process->pid);
>> +
>> +err_free_process:
>> +	domain->ops->process_free(process);
>> +
>> +	return ERR_PTR(err);
>> +}
>> +
>> +static void iommu_process_release(struct kref *kref)
>> +{
>> +	struct iommu_process *process;
>> +	void (*release)(struct iommu_process *);
>> +
>> +	assert_spin_locked(&iommu_process_lock);
>> +
>> +	process = container_of(kref, struct iommu_process, kref);
> 
> if we are concerned about things going wrong (assert above), we should
> also add some pointer check here (WARN) for process and release pointers as well.

We can probably get rid of this assert, as any external users releasing
the process will have to go through iommu_process_put() which takes the
lock. process_alloc() ensures that release isn't NULL, and process should
always be valid here since we're being called from kref_put, but I should
check the value in process_put.

>> +	release = process->release;
>> +
>> +	WARN_ON(!list_empty(&process->domains));
>> +
>> +	idr_remove(&iommu_process_idr, process->pasid);
>> +	put_pid(process->pid);
>> +	release(process);
>> +}
>> +
>> +/*
>> + * Returns non-zero if a reference to the process was successfully taken.
>> + * Returns zero if the process is being freed and should not be used.
>> + */
>> +static int iommu_process_get_locked(struct iommu_process *process)
>> +{
>> +	assert_spin_locked(&iommu_process_lock);
>> +
>> +	if (process)
>> +		return kref_get_unless_zero(&process->kref);
>> +
>> +	return 0;
>> +}
>> +
>> +static void iommu_process_put_locked(struct iommu_process *process)
>> +{
>> +	assert_spin_locked(&iommu_process_lock);
>> +
>> +	kref_put(&process->kref, iommu_process_release);
>> +}
>> +
>> +static int iommu_process_attach(struct iommu_domain *domain, struct device *dev,
>> +				struct iommu_process *process)
>> +{
>> +	int err;
>> +	int pasid = process->pasid;
>> +	struct iommu_context *context;
>> +
>> +	if (WARN_ON(!domain->ops->process_attach || !domain->ops->process_detach))
>> +		return -ENODEV;
>> +
>> +	if (pasid > domain->max_pasid || pasid < domain->min_pasid)
>> +		return -ENOSPC;
>> +
>> +	context = kzalloc(sizeof(*context), GFP_KERNEL);
>> +	if (!context)
>> +		return -ENOMEM;
>> +
> 
> devm_kzalloc maybe?

I don't think we can ever leak contexts. Before the device is released, it
has to detach() from the domain, which will unbind from any process (call
unbind_dev_all()).

Thanks,
Jean