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@xxxxxxxxx>
> Cc: Markus Elfring <Markus.Elfring@xxxxxx>
> Cc: Christoph Hellwig <hch@xxxxxx>
> Signed-off-by: Hillf Danton <hdanton@xxxxxxxx>
> ---
> 
> --- 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);
> --
>