Added back the previously removed function vhost_workers_free() and
renamed it to vhost_workers_free_khtread(). The new vhost_workers_free()
will select the different mode based on the value of the parameter.

The old function vhost_workers_free was change to support task in
commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads")
also changed in
commit a284f09effea ("vhost: Fix crash during early vhost_transport_send_pkt calls")
change to xarray in
commit 1cdaafa1b8b4 ("vhost: replace single worker pointer with xarray")

Signed-off-by: Cindy Lu <lulu@redhat.com>
---
 drivers/vhost/vhost.c | 52 ++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 47 insertions(+), 5 deletions(-)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index ad359d4b725f..fed6671c1ffb 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -648,8 +648,21 @@ static void vhost_detach_mm(struct vhost_dev *dev)
 	dev->mm = NULL;
 }
 
-static void vhost_worker_destroy(struct vhost_dev *dev,
-				 struct vhost_worker *worker)
+static void vhost_worker_destroy_kthread(struct vhost_dev *dev,
+					 struct vhost_worker *worker)
+{
+	if (!worker)
+		return;
+
+	WARN_ON(!llist_empty(&worker->work_list));
+
+	xa_erase(&dev->worker_xa, worker->id);
+	kthread_stop(worker->task);
+	kfree(worker);
+}
+
+static void vhost_worker_destroy_task(struct vhost_dev *dev,
+				      struct vhost_worker *worker)
 {
 	if (!worker)
 		return;
@@ -660,7 +673,7 @@ static void vhost_worker_destroy(struct vhost_dev *dev,
 	kfree(worker);
 }
 
-static void vhost_workers_free(struct vhost_dev *dev)
+static void vhost_workers_free_task(struct vhost_dev *dev)
 {
 	struct vhost_worker *worker;
 	unsigned long i;
@@ -675,10 +688,36 @@ static void vhost_workers_free(struct vhost_dev *dev)
 	 * created but couldn't clean up (it forgot or crashed).
 	 */
 	xa_for_each(&dev->worker_xa, i, worker)
-		vhost_worker_destroy(dev, worker);
+		vhost_worker_destroy_task(dev, worker);
 	xa_destroy(&dev->worker_xa);
 }
 
+static void vhost_workers_free_kthread(struct vhost_dev *dev)
+{
+	struct vhost_worker *worker;
+	unsigned long i;
+
+	if (!dev->use_worker)
+		return;
+
+	for (i = 0; i < dev->nvqs; i++)
+		rcu_assign_pointer(dev->vqs[i]->worker, NULL);
+	/*
+	 * Free the default worker we created and cleanup workers userspace
+	 * created but couldn't clean up (it forgot or crashed).
+	 */
+	xa_for_each(&dev->worker_xa, i, worker)
+		vhost_worker_destroy_kthread(dev, worker);
+	xa_destroy(&dev->worker_xa);
+}
+
+static void vhost_workers_free(struct vhost_dev *dev)
+{
+	if (enforce_inherit_owner)
+		vhost_workers_free_task(dev);
+	else
+		vhost_workers_free_kthread(dev);
+}
 static struct vhost_worker *vhost_worker_create(struct vhost_dev *dev)
 {
 	struct vhost_worker *worker;
@@ -846,7 +885,10 @@ static int vhost_free_worker(struct vhost_dev *dev,
 	__vhost_worker_flush(worker);
 	mutex_unlock(&worker->mutex);
 
-	vhost_worker_destroy(dev, worker);
+	if (enforce_inherit_owner)
+		vhost_worker_destroy_task(dev, worker);
+	else
+		vhost_worker_destroy_kthread(dev, worker);
 	return 0;
 }
 
-- 
2.45.0

On 10/3/24 8:58 PM, Cindy Lu wrote:
> +static void vhost_workers_free(struct vhost_dev *dev)
> +{
> +	if (enforce_inherit_owner)
> +		vhost_workers_free_task(dev);
> +	else
> +		vhost_workers_free_kthread(dev);
> +}

With patch 7, userspace could change enforce_inherit_owner after
we created thread and we would call the wrong function above.

On Tue, 15 Oct 2024 at 05:06, Mike Christie <michael.christie@oracle.com> wrote:
>
> On 10/3/24 8:58 PM, Cindy Lu wrote:
> > +static void vhost_workers_free(struct vhost_dev *dev)
> > +{
> > +     if (enforce_inherit_owner)
> > +             vhost_workers_free_task(dev);
> > +     else
> > +             vhost_workers_free_kthread(dev);
> > +}
>
> With patch 7, userspace could change enforce_inherit_owner after
> we created thread and we would call the wrong function above.
>
enforce_inherit_owner will only change before the owner was set.
the process is like set enforce_inherit_owner---->set owner->
thread/task creating
in in patch 7's code I have add the check for vhost's owner, if the
owner was set, the ioctl
to set enforce_inherit_owner will fail
Thanks
Cindy

On Tue, Oct 15, 2024 at 02:05:47PM +0800, Cindy Lu wrote:
>On Tue, 15 Oct 2024 at 05:06, Mike Christie <michael.christie@oracle.com> wrote:
>>
>> On 10/3/24 8:58 PM, Cindy Lu wrote:
>> > +static void vhost_workers_free(struct vhost_dev *dev)
>> > +{
>> > +     if (enforce_inherit_owner)
>> > +             vhost_workers_free_task(dev);
>> > +     else
>> > +             vhost_workers_free_kthread(dev);
>> > +}
>>
>> With patch 7, userspace could change enforce_inherit_owner after
>> we created thread and we would call the wrong function above.
>>
>enforce_inherit_owner will only change before the owner was set.

As I pointed out in patch 7, enforce_inherit_owner seems to be shared 
among all vhost devices, so what happens if for example a user sets it 
to /dev/vhost-net, while /dev/vhost-vsock is already initialized and 
therefore already has an owner?

Thanks,
Stefano

>the process is like set enforce_inherit_owner---->set owner->
>thread/task creating
>in in patch 7's code I have add the check for vhost's owner, if the
>owner was set, the ioctl
>to set enforce_inherit_owner will fail
>Thanks
>Cindy
>
>

On Tue, 15 Oct 2024 at 14:52, Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Tue, Oct 15, 2024 at 02:05:47PM +0800, Cindy Lu wrote:
> >On Tue, 15 Oct 2024 at 05:06, Mike Christie <michael.christie@oracle.com> wrote:
> >>
> >> On 10/3/24 8:58 PM, Cindy Lu wrote:
> >> > +static void vhost_workers_free(struct vhost_dev *dev)
> >> > +{
> >> > +     if (enforce_inherit_owner)
> >> > +             vhost_workers_free_task(dev);
> >> > +     else
> >> > +             vhost_workers_free_kthread(dev);
> >> > +}
> >>
> >> With patch 7, userspace could change enforce_inherit_owner after
> >> we created thread and we would call the wrong function above.
> >>
> >enforce_inherit_owner will only change before the owner was set.
>
> As I pointed out in patch 7, enforce_inherit_owner seems to be shared
> among all vhost devices, so what happens if for example a user sets it
> to /dev/vhost-net, while /dev/vhost-vsock is already initialized and
> therefore already has an owner?
>
> Thanks,
> Stefano
>
You are correct, I will think about this and provide a new version
Thanks
cindy

> >the process is like set enforce_inherit_owner---->set owner->
> >thread/task creating
> >in in patch 7's code I have add the check for vhost's owner, if the
> >owner was set, the ioctl
> >to set enforce_inherit_owner will fail
> >Thanks
> >Cindy
> >
> >
>

On Fri, Oct 04, 2024 at 09:58:17AM GMT, Cindy Lu wrote:
>Added back the previously removed function vhost_workers_free() and
>renamed it to vhost_workers_free_khtread(). The new vhost_workers_free()
>will select the different mode based on the value of the parameter.
>
>The old function vhost_workers_free was change to support task in
>commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads")
>also changed in
>commit a284f09effea ("vhost: Fix crash during early vhost_transport_send_pkt calls")
>change to xarray in
>commit 1cdaafa1b8b4 ("vhost: replace single worker pointer with xarray")
>
>Signed-off-by: Cindy Lu <lulu@redhat.com>
>---
> drivers/vhost/vhost.c | 52 ++++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 47 insertions(+), 5 deletions(-)
>
>diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>index ad359d4b725f..fed6671c1ffb 100644
>--- a/drivers/vhost/vhost.c
>+++ b/drivers/vhost/vhost.c
>@@ -648,8 +648,21 @@ static void vhost_detach_mm(struct vhost_dev *dev)
> 	dev->mm = NULL;
> }
>
>-static void vhost_worker_destroy(struct vhost_dev *dev,
>-				 struct vhost_worker *worker)
>+static void vhost_worker_destroy_kthread(struct vhost_dev *dev,
>+					 struct vhost_worker *worker)
>+{
>+	if (!worker)
>+		return;
>+
>+	WARN_ON(!llist_empty(&worker->work_list));
>+
>+	xa_erase(&dev->worker_xa, worker->id);
>+	kthread_stop(worker->task);
>+	kfree(worker);
>+}
>+
>+static void vhost_worker_destroy_task(struct vhost_dev *dev,
>+				      struct vhost_worker *worker)
> {
> 	if (!worker)
> 		return;
>@@ -660,7 +673,7 @@ static void vhost_worker_destroy(struct vhost_dev *dev,
> 	kfree(worker);
> }
>
>-static void vhost_workers_free(struct vhost_dev *dev)
>+static void vhost_workers_free_task(struct vhost_dev *dev)
> {
> 	struct vhost_worker *worker;
> 	unsigned long i;
>@@ -675,10 +688,36 @@ static void vhost_workers_free(struct vhost_dev *dev)
> 	 * created but couldn't clean up (it forgot or crashed).
> 	 */
> 	xa_for_each(&dev->worker_xa, i, worker)
>-		vhost_worker_destroy(dev, worker);
>+		vhost_worker_destroy_task(dev, worker);
> 	xa_destroy(&dev->worker_xa);
> }
>
>+static void vhost_workers_free_kthread(struct vhost_dev *dev)
>+{
>+	struct vhost_worker *worker;
>+	unsigned long i;
>+
>+	if (!dev->use_worker)
>+		return;
>+
>+	for (i = 0; i < dev->nvqs; i++)
>+		rcu_assign_pointer(dev->vqs[i]->worker, NULL);
>+	/*
>+	 * Free the default worker we created and cleanup workers userspace
>+	 * created but couldn't clean up (it forgot or crashed).
>+	 */
>+	xa_for_each(&dev->worker_xa, i, worker)
>+		vhost_worker_destroy_kthread(dev, worker);
>+	xa_destroy(&dev->worker_xa);
>+}
>+
>+static void vhost_workers_free(struct vhost_dev *dev)
>+{
>+	if (enforce_inherit_owner)
>+		vhost_workers_free_task(dev);
>+	else
>+		vhost_workers_free_kthread(dev);
>+}

nit: new line here

> static struct vhost_worker *vhost_worker_create(struct vhost_dev *dev)
> {
> 	struct vhost_worker *worker;
>@@ -846,7 +885,10 @@ static int vhost_free_worker(struct vhost_dev *dev,
> 	__vhost_worker_flush(worker);
> 	mutex_unlock(&worker->mutex);
>
>-	vhost_worker_destroy(dev, worker);
>+	if (enforce_inherit_owner)
>+		vhost_worker_destroy_task(dev, worker);
>+	else
>+		vhost_worker_destroy_kthread(dev, worker);
> 	return 0;
> }
>
>-- 
>2.45.0
>
>

On Mon, 7 Oct 2024 at 21:33, Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Fri, Oct 04, 2024 at 09:58:17AM GMT, Cindy Lu wrote:
> >Added back the previously removed function vhost_workers_free() and
> >renamed it to vhost_workers_free_khtread(). The new vhost_workers_free()
> >will select the different mode based on the value of the parameter.
> >
> >The old function vhost_workers_free was change to support task in
> >commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads")
> >also changed in
> >commit a284f09effea ("vhost: Fix crash during early vhost_transport_send_pkt calls")
> >change to xarray in
> >commit 1cdaafa1b8b4 ("vhost: replace single worker pointer with xarray")
> >
> >Signed-off-by: Cindy Lu <lulu@redhat.com>
> >---
> > drivers/vhost/vhost.c | 52 ++++++++++++++++++++++++++++++++++++++-----
> > 1 file changed, 47 insertions(+), 5 deletions(-)
> >
> >diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> >index ad359d4b725f..fed6671c1ffb 100644
> >--- a/drivers/vhost/vhost.c
> >+++ b/drivers/vhost/vhost.c
> >@@ -648,8 +648,21 @@ static void vhost_detach_mm(struct vhost_dev *dev)
> >       dev->mm = NULL;
> > }
> >
> >-static void vhost_worker_destroy(struct vhost_dev *dev,
> >-                               struct vhost_worker *worker)
> >+static void vhost_worker_destroy_kthread(struct vhost_dev *dev,
> >+                                       struct vhost_worker *worker)
> >+{
> >+      if (!worker)
> >+              return;
> >+
> >+      WARN_ON(!llist_empty(&worker->work_list));
> >+
> >+      xa_erase(&dev->worker_xa, worker->id);
> >+      kthread_stop(worker->task);
> >+      kfree(worker);
> >+}
> >+
> >+static void vhost_worker_destroy_task(struct vhost_dev *dev,
> >+                                    struct vhost_worker *worker)
> > {
> >       if (!worker)
> >               return;
> >@@ -660,7 +673,7 @@ static void vhost_worker_destroy(struct vhost_dev *dev,
> >       kfree(worker);
> > }
> >
> >-static void vhost_workers_free(struct vhost_dev *dev)
> >+static void vhost_workers_free_task(struct vhost_dev *dev)
> > {
> >       struct vhost_worker *worker;
> >       unsigned long i;
> >@@ -675,10 +688,36 @@ static void vhost_workers_free(struct vhost_dev *dev)
> >        * created but couldn't clean up (it forgot or crashed).
> >        */
> >       xa_for_each(&dev->worker_xa, i, worker)
> >-              vhost_worker_destroy(dev, worker);
> >+              vhost_worker_destroy_task(dev, worker);
> >       xa_destroy(&dev->worker_xa);
> > }
> >
> >+static void vhost_workers_free_kthread(struct vhost_dev *dev)
> >+{
> >+      struct vhost_worker *worker;
> >+      unsigned long i;
> >+
> >+      if (!dev->use_worker)
> >+              return;
> >+
> >+      for (i = 0; i < dev->nvqs; i++)
> >+              rcu_assign_pointer(dev->vqs[i]->worker, NULL);
> >+      /*
> >+       * Free the default worker we created and cleanup workers userspace
> >+       * created but couldn't clean up (it forgot or crashed).
> >+       */
> >+      xa_for_each(&dev->worker_xa, i, worker)
> >+              vhost_worker_destroy_kthread(dev, worker);
> >+      xa_destroy(&dev->worker_xa);
> >+}
> >+
> >+static void vhost_workers_free(struct vhost_dev *dev)
> >+{
> >+      if (enforce_inherit_owner)
> >+              vhost_workers_free_task(dev);
> >+      else
> >+              vhost_workers_free_kthread(dev);
> >+}
>
> nit: new line here
>
sure, will fix this
> > static struct vhost_worker *vhost_worker_create(struct vhost_dev *dev)
> > {
> >       struct vhost_worker *worker;
> >@@ -846,7 +885,10 @@ static int vhost_free_worker(struct vhost_dev *dev,
> >       __vhost_worker_flush(worker);
> >       mutex_unlock(&worker->mutex);
> >
> >-      vhost_worker_destroy(dev, worker);
> >+      if (enforce_inherit_owner)
> >+              vhost_worker_destroy_task(dev, worker);
> >+      else
> >+              vhost_worker_destroy_kthread(dev, worker);
> >       return 0;
> > }
> >
> >--
> >2.45.0
> >
> >
>