Re: [RESEND PATCH v1 0/7]vhost: Add support of kthread API

[Jason Wang]

On Tue, Sep 10, 2024 at 3:42 PM Michael S. Tsirkin <mst@xxxxxxxxxx> wrote:
>
> On Mon, Sep 09, 2024 at 10:00:38AM +0800, Cindy Lu wrote:
> > In commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads"),
> > vhost removed the support for the kthread API. However, there are
> > still situations where there is a request to use kthread.
> > In this PATCH, the support of kthread is added back. Additionally,
> > a module_param is added to enforce which mode we are using, and
> > a new UAPI is introduced to allow the userspace app to set the
> > mode they want to use.
> >
> > Tested the user application with QEMU.
>
> Cindy, the APIs do not make sense, security does not make sense,
> and you are not describing the issue and the fix.
>
>
> The name should reflect what this does from userspace POV, not from
> kernel API POV.  kthread and task mode is not something that any users
> have any business even to consider.
>
>
> To help you out, some ideas:
>
> I think the issue is something like "vhost is now a child of the
> owner thread. While this makes sense from containerization
> POV, some old userspace is confused, as previously vhost not
> and so was allowed to steal cpu resources from outside the container."
>
> Now, what can be done? Given we already released a secure kernel,
> I am not inclined to revert it back to be insecure by default.

It depends on how we define "secure". There's plenty of users of
kthread and if I was not wrong, mike may still need to fix some bugs.


> But I'm fine with a modparam to allow userspace to get insecure
> behaviour.
>
>
> Now, a modparam is annoying in that it affects all userspace,
> and people might be running a mix of old and new userspace.
> So if we do that, we also want a flag that will get
> safe behaviour even if unsafe one is allowed.

I am not sure this can help. My understanding is that the flag is
sufficient. Otherwise the layered product needs to know if there's old
user space or new which seems to be a challenge.

Thanks

>
>
> Is this clear enough, or do I need to elaborate more?
>
> Thanks!
>
> > Cindy Lu (7):
> >   vhost: Add a new module_param for enable kthread
> >   vhost: Add kthread support in function vhost_worker_queue()
> >   vhost: Add kthread support in function vhost_workers_free()
> >   vhost: Add the vhost_worker to support kthread
> >   vhost: Add the cgroup related function
> >   vhost: Add kthread support in function vhost_worker_create
> >   vhost: Add new UAPI to support change to task mode
> >
> >  drivers/vhost/vhost.c      | 246 +++++++++++++++++++++++++++++++++++--
> >  drivers/vhost/vhost.h      |   1 +
> >  include/uapi/linux/vhost.h |   2 +
> >  3 files changed, 240 insertions(+), 9 deletions(-)
> >
> > --
> > 2.45.0
>