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: <olbm26f3ifb6ypfmfl22xszbpevqsxc3ogfzosrb44ujtzt6pw@uh7irbqfy5jr>
Date: Mon, 7 Oct 2024 15:31:44 +0200
From: Stefano Garzarella <sgarzare@...hat.com>
To: Cindy Lu <lulu@...hat.com>
Cc: jasowang@...hat.com, mst@...hat.com, michael.christie@...cle.com, 
	linux-kernel@...r.kernel.org, virtualization@...ts.linux-foundation.org
Subject: Re: [PATCH v2 1/7] vhost: Add a new modparam to allow userspace
 select vhost_task

On Fri, Oct 04, 2024 at 09:58:15AM GMT, Cindy Lu wrote:
>The vhost is now using vhost_task and working as a child of the owner thread.
>While this makes sense from containerization POV, some old userspace is
>confused, as previously vhost not 

not what?

> and so was allowed to steal cpu resources
>from outside the container. So we add the kthread API support back

Sorry, but it's not clear the reason.

I understand that we want to provide a way to bring back the previous 
behavior when we had kthreads, but why do we want that?
Do you have examples where the new mechanism is causing problems?

>
>Add a new module parameter to allow userspace to select behaviour
>between using kthread and task
>
>Signed-off-by: Cindy Lu <lulu@...hat.com>
>---
> drivers/vhost/vhost.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
>diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>index 9ac25d08f473..a4a0bc34f59b 100644
>--- a/drivers/vhost/vhost.c
>+++ b/drivers/vhost/vhost.c
>@@ -41,6 +41,10 @@ static int max_iotlb_entries = 2048;
> module_param(max_iotlb_entries, int, 0444);
> MODULE_PARM_DESC(max_iotlb_entries,
> 	"Maximum number of iotlb entries. (default: 2048)");
>+bool enforce_inherit_owner = true;
        ^
This should be static:

$ make -j6 O=build C=2 drivers/vhost/
...
   CHECK   ../drivers/vhost/vhost.c
../drivers/vhost/vhost.c:45:6: warning: symbol 'enforce_inherit_owner' 
was not declared. Should it be static?

>+module_param(enforce_inherit_owner, bool, 0444);
>+MODULE_PARM_DESC(enforce_inherit_owner,
>+		 "enforce vhost use vhost_task(default: Y)");

I would follow the style of the other 2 parameters:
                  "Enforce vhost use vhost_task. (default: Y)"

With a view to simplifying bisection, we added this parameter in this 
patch, but it does nothing, so IMHO we should only add it at the end of 
the series when we have all the code ready. Maybe you can just add 
`enforce_inherit_owner` here or in the first patch where you need it, 
but I'd expose it with module_param() only when we have all the pieces 
in place.

About the param name, I'm not sure "enforce" is the right word, since 
IIUC the user can still change it using the ioctl. It would seem that 
set `enforce_inherit_owner` to true, it is always forced, but instead 
ioctl allows you to change it, right?

Is it more of a default behavior?
Something like `inherit_owner_default` ?

Thanks,
Stefano


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ