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.0On 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
> >
> >
>
© 2016 - 2024 Red Hat, Inc.