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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACGkMEv71_eUY_2361TNKhTxqxnR4iTyOXq6aKR_6MqaxqM5Dg@mail.gmail.com>
Date: Wed, 11 Sep 2024 11:45:33 +0800
From: Jason Wang <jasowang@...hat.com>
To: "Michael S. Tsirkin" <mst@...hat.com>
Cc: Cindy Lu <lulu@...hat.com>, michael.christie@...cle.com, 
	linux-kernel@...r.kernel.org, virtualization@...ts.linux-foundation.org
Subject: Re: [RESEND PATCH v1 0/7]vhost: Add support of kthread API

On Tue, Sep 10, 2024 at 4:43 PM Michael S. Tsirkin <mst@...hat.com> wrote:
>
> On Tue, Sep 10, 2024 at 04:37:52PM +0800, Jason Wang wrote:
> > On Tue, Sep 10, 2024 at 3:42 PM Michael S. Tsirkin <mst@...hat.com> 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.
> >
>
> which bugs?

https://lore.kernel.org/all/06369c2c-c363-4def-9ce0-f018a9e10e8d@oracle.com/T/

>
> > > 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
>
> this will be up to userspace to resolve.

I actually mean the module parameter. It would be very hard for the
layered product to decide the value for that.

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