Re: [RFC PATCH] vfio: type1: fix kthread use case

Re: [RFC PATCH] vfio: type1: fix kthread use case

[Markus Elfring]

…
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -2798,7 +2798,8 @@ static int vfio_iommu_type1_dma_rw_chunk
…
> -	bool kthread = current->mm == NULL;
> +	bool kthread = current->flags & PF_KTHREAD;
> +	bool use_mm = current->mm == NULL;
…

Can it be helpful to convert initialisations for these variables
into later assignments?

Regards,
Markus

Re: [RFC PATCH] vfio: type1: fix kthread use case

[Markus Elfring]

>> Can it be helpful to convert initialisations for these variables
>> into later assignments?
>
> Perhaps. Then it looks like the below.


…
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -2798,9 +2798,12 @@ static int vfio_iommu_type1_dma_rw_chunk
…
> -	bool kthread = current->mm == NULL;
> +	bool kthread;
> +	bool use_mm;

I would prefer the following variable declarations then.

+	bool kthread, use_mm;


>  	size_t offset;
>
> +	kthread = current->flags & PF_KTHREAD;
> +	use_mm = current->mm == NULL;

I propose to move such assignments directly before the corresponding check.


…
>  	if (!mm)
>  		return -EPERM;


+	kthread = current->flags & PF_KTHREAD;
+	use_mm = !current->mm;

> -	if (kthread)
> +	if (kthread && use_mm)
>  		kthread_use_mm(mm);
…

Regards,
Markus

Re: [RFC PATCH] vfio: type1: fix kthread use case

[Markus Elfring]

…
> +++ b/drivers/vfio/vfio_iommu_type1.c
…
> @@ -2812,11 +2815,10 @@ static int vfio_iommu_type1_dma_rw_chunk
…
>  	if (!mm)
>  		return -EPERM;
>
> -	if (kthread)
> +	if (kthread && use_mm)

Can another design approach make sense here?

+	bool thread_use_mm = ((current->flags & PF_KTHREAD) && !current->mm);
+	if (thread_use_mm)


>  		kthread_use_mm(mm);
…

Regards,
Markus

Re: [RFC PATCH] vfio: type1: fix kthread use case

[Yan Zhao]

On Mon, Jul 06, 2020 at 06:49:15PM +0800, Hillf Danton wrote:
> 
> It's incorrect to tell out if a task is kthread without checking
> PF_KTHREAD at all.
> 
> What's also fixed, if no need to be in a seperate patch, is to
> invoke kthread_use_mm() without checking the current mm first as
> the kthread may hold a mm struct atm and it's not the right place
> to switch mm.
> 
> Fixes: 8d46c0cca5f4 ("vfio: introduce vfio_dma_rw to read/write a range of IOVAs")
> Cc: Yan Zhao <yan.y.zhao@intel.com>
> Cc: Markus Elfring <Markus.Elfring@web.de>
> Cc: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Hillf Danton <hdanton@sina.com>
> ---
> 
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -2798,7 +2798,8 @@ static int vfio_iommu_type1_dma_rw_chunk
>  	struct mm_struct *mm;
>  	unsigned long vaddr;
>  	struct vfio_dma *dma;
> -	bool kthread = current->mm == NULL;
> +	bool kthread = current->flags & PF_KTHREAD;
> +	bool use_mm = current->mm == NULL;
is it acceptable to just rename "kthread" to "kthread_no_use_mm"?

I think "current->mm == NULL" in itself implies kthread and not use_mm,
as a user thread is not able to have "current->mm == NULL", right?


Thanks
Yan

>  	size_t offset;
>  
>  	*copied = 0;
> @@ -2812,11 +2813,10 @@ static int vfio_iommu_type1_dma_rw_chunk
>  		return -EPERM;
>  
>  	mm = get_task_mm(dma->task);
> -
>  	if (!mm)
>  		return -EPERM;
>  
> -	if (kthread)
> +	if (kthread && use_mm)
>  		kthread_use_mm(mm);
>  	else if (current->mm != mm)
>  		goto out;
> @@ -2843,7 +2843,7 @@ static int vfio_iommu_type1_dma_rw_chunk
>  	} else
>  		*copied = copy_from_user(data, (void __user *)vaddr,
>  					   count) ? 0 : count;
> -	if (kthread)
> +	if (kthread && use_mm)
>  		kthread_unuse_mm(mm);
>  out:
>  	mmput(mm);
> --
> 

Re: [RFC PATCH] vfio: type1: fix kthread use case

[Markus Elfring]

…
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -2798,7 +2798,7 @@ static int vfio_iommu_type1_dma_rw_chunk
…
> -	bool kthread = current->mm == NULL;
> +	bool kthread_load_mm;
>  	size_t offset;

How do you think about to reduce the scope for such variables?
https://refactoring.com/catalog/reduceScopeOfVariable.html


…
> @@ -2812,11 +2812,12 @@ static int vfio_iommu_type1_dma_rw_chunk
…
>  	if (!mm)
>  		return -EPERM;
…
> +	kthread_load_mm = current->flags & PF_KTHREAD &&
> +				current->mm == NULL;
…

Would you like to apply a more succinct code variant?

+	kthread_load_mm = current->flags & PF_KTHREAD && !current->mm;


Regards,
Markus