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

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

[Hillf Danton]

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;
 	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);
--

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Hillf Danton]

On Mon, 6 Jul 2020 13:32:18 +0200 Markus Elfring wrote:
> =E2=80=A6
> > +++ b/drivers/vfio/vfio_iommu_type1.c
> > @@ -2798,7 +2798,8 @@ static int vfio_iommu_type1_dma_rw_chunk
> =E2=80=A6
> > -	bool kthread =3D current->mm =3D=3D NULL;
> > +	bool kthread =3D current->flags & PF_KTHREAD;
> > +	bool use_mm =3D current->mm =3D=3D NULL;
> =E2=80=A6
> 
> Can it be helpful to convert initialisations for these variables
> into later assignments?

Perhaps. Then it looks like the below.

--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -2798,9 +2798,12 @@ 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;
+	bool use_mm;
 	size_t offset;
 
+	kthread = current->flags & PF_KTHREAD;
+	use_mm = current->mm == NULL;
 	*copied = 0;
 
 	dma = vfio_find_dma(iommu, user_iova, 1);
@@ -2812,11 +2815,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 +2845,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);

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Hillf Danton]

On Mon, 6 Jul 2020 15:33:58 +0200 Markus Elfring wrote:
>
> I would prefer the following variable declarations then.
> 
> +	bool kthread, use_mm;
> 
> 
> >  	size_t offset;
> >
> > +	kthread =3D current->flags & PF_KTHREAD;
> > +	use_mm =3D current->mm =3D=3D NULL;
> 
> I propose to move such assignments directly before the corresponding check=
> .

Got it. See below.

> 
> >  	if (!mm)
> >  		return -EPERM;
> 
> 
> +	kthread =3D current->flags & PF_KTHREAD;
> +	use_mm =3D !current->mm;
> 
> > -	if (kthread)
> > +	if (kthread && use_mm)
> >  		kthread_use_mm(mm);


--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -2798,7 +2798,7 @@ 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, use_mm;
 	size_t offset;
 
 	*copied = 0;
@@ -2812,11 +2812,13 @@ static int vfio_iommu_type1_dma_rw_chunk
 		return -EPERM;
 
 	mm = get_task_mm(dma->task);
-
 	if (!mm)
 		return -EPERM;
 
-	if (kthread)
+	kthread = current->flags & PF_KTHREAD;
+	use_mm = current->mm == NULL;
+
+	if (kthread && use_mm)
 		kthread_use_mm(mm);
 	else if (current->mm != mm)
 		goto out;
@@ -2843,7 +2845,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);

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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);
> --
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Hillf Danton]

On Mon, 6 Jul 2020 16:04:13 +0200 Markus Elfring wrote:
> 
> Can another design approach make sense here?
> 
> +	bool thread_use_mm = ((current->flags & PF_KTHREAD) && !current->mm);
> +	if (thread_use_mm)

Good. See below.
> 
> 
> >  		kthread_use_mm(mm);


--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -2798,7 +2798,7 @@ 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_load_mm;
 	size_t offset;
 
 	*copied = 0;
@@ -2812,11 +2812,12 @@ static int vfio_iommu_type1_dma_rw_chunk
 		return -EPERM;
 
 	mm = get_task_mm(dma->task);
-
 	if (!mm)
 		return -EPERM;
 
-	if (kthread)
+	kthread_load_mm = current->flags & PF_KTHREAD &&
+				current->mm == NULL;
+	if (kthread_load_mm)
 		kthread_use_mm(mm);
 	else if (current->mm != mm)
 		goto out;
@@ -2843,7 +2844,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_load_mm)
 		kthread_unuse_mm(mm);
 out:
 	mmput(mm);

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Hillf Danton]

On Tue, 7 Jul 2020 08:31:41 +0800 Yan Zhao wrote:
>
> 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?

Nope, it's the fix target as checking the current mm does not make
sense without current being kthread.

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu