* [RFC PATCH] vfio: type1: fix kthread use case
@ 2020-07-06 10:49 Hillf Danton
2020-07-06 11:32 ` Markus Elfring
2020-07-07 0:31 ` Yan Zhao
0 siblings, 2 replies; 10+ messages in thread
From: Hillf Danton @ 2020-07-06 10:49 UTC (permalink / raw)
To: Alex Williamson
Cc: Kevin Tian, Yan Zhao, linux-kernel, Hillf Danton, iommu,
Markus Elfring, Christoph Hellwig
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [RFC PATCH] vfio: type1: fix kthread use case
2020-07-06 10:49 [RFC PATCH] vfio: type1: fix kthread use case Hillf Danton
@ 2020-07-06 11:32 ` Markus Elfring
2020-07-06 12:42 ` Hillf Danton
2020-07-07 0:31 ` Yan Zhao
1 sibling, 1 reply; 10+ messages in thread
From: Markus Elfring @ 2020-07-06 11:32 UTC (permalink / raw)
To: Hillf Danton, Alex Williamson, iommu
Cc: Kevin Tian, Yan Zhao, kernel-janitors, linux-kernel, Christoph Hellwig
…
> +++ 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] vfio: type1: fix kthread use case
2020-07-06 11:32 ` Markus Elfring
@ 2020-07-06 12:42 ` Hillf Danton
2020-07-06 13:33 ` Markus Elfring
2020-07-06 14:04 ` Markus Elfring
0 siblings, 2 replies; 10+ messages in thread
From: Hillf Danton @ 2020-07-06 12:42 UTC (permalink / raw)
To: Markus Elfring
Cc: Hillf Danton, Yan Zhao, kernel-janitors, iommu, linux-kernel,
Kevin Tian, Alex Williamson, Christoph Hellwig
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] vfio: type1: fix kthread use case
2020-07-06 12:42 ` Hillf Danton
@ 2020-07-06 13:33 ` Markus Elfring
2020-07-06 14:08 ` Hillf Danton
2020-07-06 14:04 ` Markus Elfring
1 sibling, 1 reply; 10+ messages in thread
From: Markus Elfring @ 2020-07-06 13:33 UTC (permalink / raw)
To: Hillf Danton, Alex Williamson, iommu
Cc: Kevin Tian, Yan Zhao, kernel-janitors, linux-kernel, Christoph Hellwig
>> 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] vfio: type1: fix kthread use case
2020-07-06 13:33 ` Markus Elfring
@ 2020-07-06 14:08 ` Hillf Danton
0 siblings, 0 replies; 10+ messages in thread
From: Hillf Danton @ 2020-07-06 14:08 UTC (permalink / raw)
To: Markus Elfring
Cc: Hillf Danton, Yan Zhao, kernel-janitors, iommu, linux-kernel,
Kevin Tian, Alex Williamson, Christoph Hellwig
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] vfio: type1: fix kthread use case
2020-07-06 12:42 ` Hillf Danton
2020-07-06 13:33 ` Markus Elfring
@ 2020-07-06 14:04 ` Markus Elfring
2020-07-07 1:14 ` Hillf Danton
1 sibling, 1 reply; 10+ messages in thread
From: Markus Elfring @ 2020-07-06 14:04 UTC (permalink / raw)
To: Hillf Danton, Alex Williamson, iommu
Cc: Kevin Tian, Yan Zhao, kernel-janitors, linux-kernel, Christoph Hellwig
…
> +++ 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] vfio: type1: fix kthread use case
2020-07-06 14:04 ` Markus Elfring
@ 2020-07-07 1:14 ` Hillf Danton
2020-07-07 6:04 ` Markus Elfring
0 siblings, 1 reply; 10+ messages in thread
From: Hillf Danton @ 2020-07-07 1:14 UTC (permalink / raw)
To: Markus Elfring
Cc: Hillf Danton, Yan Zhao, iommu, linux-kernel, Kevin Tian,
Alex Williamson, Christoph Hellwig
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] vfio: type1: fix kthread use case
2020-07-07 1:14 ` Hillf Danton
@ 2020-07-07 6:04 ` Markus Elfring
0 siblings, 0 replies; 10+ messages in thread
From: Markus Elfring @ 2020-07-07 6:04 UTC (permalink / raw)
To: Hillf Danton, iommu
Cc: Kevin Tian, Yan Zhao, kernel-janitors, linux-kernel,
Alex Williamson, Christoph Hellwig
…
> +++ 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] vfio: type1: fix kthread use case
2020-07-06 10:49 [RFC PATCH] vfio: type1: fix kthread use case Hillf Danton
2020-07-06 11:32 ` Markus Elfring
@ 2020-07-07 0:31 ` Yan Zhao
2020-07-07 2:00 ` Hillf Danton
1 sibling, 1 reply; 10+ messages in thread
From: Yan Zhao @ 2020-07-07 0:31 UTC (permalink / raw)
To: Hillf Danton
Cc: Kevin Tian, linux-kernel, iommu, Alex Williamson, Markus Elfring,
Christoph Hellwig
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [RFC PATCH] vfio: type1: fix kthread use case
2020-07-07 0:31 ` Yan Zhao
@ 2020-07-07 2:00 ` Hillf Danton
0 siblings, 0 replies; 10+ messages in thread
From: Hillf Danton @ 2020-07-07 2:00 UTC (permalink / raw)
To: Yan Zhao
Cc: Hillf Danton, iommu, linux-kernel, Kevin Tian, Alex Williamson,
Markus Elfring, Christoph Hellwig
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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-07-07 6:05 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-06 10:49 [RFC PATCH] vfio: type1: fix kthread use case Hillf Danton
2020-07-06 11:32 ` Markus Elfring
2020-07-06 12:42 ` Hillf Danton
2020-07-06 13:33 ` Markus Elfring
2020-07-06 14:08 ` Hillf Danton
2020-07-06 14:04 ` Markus Elfring
2020-07-07 1:14 ` Hillf Danton
2020-07-07 6:04 ` Markus Elfring
2020-07-07 0:31 ` Yan Zhao
2020-07-07 2:00 ` Hillf Danton
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).