| 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
| ||
|
Message-ID: <20250612022053-mutt-send-email-mst@kernel.org>
Date: Thu, 12 Jun 2025 02:31:12 -0400
From: "Michael S. Tsirkin" <mst@...hat.com>
To: Cindy Lu <lulu@...hat.com>
Cc: jasowang@...hat.com, michael.christie@...cle.com, sgarzare@...hat.com,
linux-kernel@...r.kernel.org,
virtualization@...ts.linux-foundation.org, netdev@...r.kernel.org
Subject: Re: [PATCH v11 3/3] vhost: Add configuration controls for vhost
worker's mode
On Mon, Jun 09, 2025 at 03:33:09PM +0800, Cindy Lu wrote:
> This patch introduces functionality to control the vhost worker mode:
>
> - Add two new IOCTLs:
> * VHOST_SET_FORK_FROM_OWNER: Allows userspace to select between
> task mode (fork_owner=1) and kthread mode (fork_owner=0)
> * VHOST_GET_FORK_FROM_OWNER: Retrieves the current thread mode
> setting
>
> - Expose module parameter 'fork_from_owner_default' to allow system
> administrators to configure the default mode for vhost workers
>
> - Add KConfig option CONFIG_VHOST_ENABLE_FORK_OWNER_CONTROL to
> control the availability of these IOCTLs and parameter, allowing
> distributions to disable them if not needed
>
> - The VHOST_NEW_WORKER functionality requires fork_owner to be set
> to true, with validation added to ensure proper configuration
>
> Signed-off-by: Cindy Lu <lulu@...hat.com>
getting there. yet something to improve.
> ---
> drivers/vhost/Kconfig | 17 +++++++++++++++
> drivers/vhost/vhost.c | 44 ++++++++++++++++++++++++++++++++++++++
> include/uapi/linux/vhost.h | 25 ++++++++++++++++++++++
> 3 files changed, 86 insertions(+)
>
> diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> index 020d4fbb947c..49e1d9dc92b7 100644
> --- a/drivers/vhost/Kconfig
> +++ b/drivers/vhost/Kconfig
> @@ -96,3 +96,20 @@ config VHOST_CROSS_ENDIAN_LEGACY
> If unsure, say "N".
>
> endif
> +
> +config CONFIG_VHOST_ENABLE_FORK_OWNER_CONTROL
> + bool "Enable CONFIG_VHOST_ENABLE_FORK_OWNER_CONTROL"
> + default n
> + help
> + This option enables two IOCTLs: VHOST_SET_FORK_FROM_OWNER and
> + VHOST_GET_FORK_FROM_OWNER. These allow userspace applications
> + to modify the vhost worker mode for vhost devices.
> +
> + Also expose module parameter 'fork_from_owner_default' to allow users
> + to configure the default mode for vhost workers.
> +
> + By default, `CONFIG_VHOST_ENABLE_FORK_OWNER_CONTROL` is set to `n`,
> + which disables the IOCTLs and parameter.
> + When enabled (y), users can change the worker thread mode as needed.
> +
> + If unsure, say "N".
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 37d3ed8be822..903d9c3f6784 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -43,6 +43,11 @@ module_param(max_iotlb_entries, int, 0444);
> MODULE_PARM_DESC(max_iotlb_entries,
> "Maximum number of iotlb entries. (default: 2048)");
> static bool fork_from_owner_default = true;
Add empty lines around ifdef to make it clear what code do they
delimit.
> +#ifdef CONFIG_VHOST_ENABLE_FORK_OWNER_CONTROL
> +module_param(fork_from_owner_default, bool, 0444);
> +MODULE_PARM_DESC(fork_from_owner_default,
> + "Set task mode as the default(default: Y)");
> +#endif
>
> enum {
> VHOST_MEMORY_F_LOG = 0x1,
> @@ -1019,6 +1024,13 @@ long vhost_worker_ioctl(struct vhost_dev *dev, unsigned int ioctl,
> switch (ioctl) {
> /* dev worker ioctls */
> case VHOST_NEW_WORKER:
> + /*
> + * vhost_tasks will account for worker threads under the parent's
> + * NPROC value but kthreads do not. To avoid userspace overflowing
> + * the system with worker threads fork_owner must be true.
> + */
> + if (!dev->fork_owner)
> + return -EFAULT;
An empty line here would make the code clearer.
> ret = vhost_new_worker(dev, &state);
> if (!ret && copy_to_user(argp, &state, sizeof(state)))
> ret = -EFAULT;
> @@ -1136,6 +1148,7 @@ void vhost_dev_reset_owner(struct vhost_dev *dev, struct vhost_iotlb *umem)
>
> vhost_dev_cleanup(dev);
>
> + dev->fork_owner = fork_from_owner_default;
> dev->umem = umem;
> /* We don't need VQ locks below since vhost_dev_cleanup makes sure
> * VQs aren't running.
> @@ -2289,6 +2302,37 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
> goto done;
> }
>
> +#ifdef CONFIG_VHOST_ENABLE_FORK_OWNER_CONTROL
> + u8 fork_owner;
Do not declare variables in the middle of a scope please.
This one is not needed in this scope, so just move it down
to within if (yes you will repeat the declaration twice then).
> +
> + if (ioctl == VHOST_SET_FORK_FROM_OWNER) {
> + /*fork_owner can only be modified before owner is set*/
bad comment style.
> + if (vhost_dev_has_owner(d)) {
> + r = -EBUSY;
> + goto done;
> + }
> + if (copy_from_user(&fork_owner, argp, sizeof(u8))) {
get_user is a better fit for this. In particular, typesafe.
> + r = -EFAULT;
> + goto done;
> + }
> + if (fork_owner > 1) {
so 0 and 1 are the only legal values?
maybe add an enum or defines in the header then.
> + r = -EINVAL;
> + goto done;
> + }
> + d->fork_owner = (bool)fork_owner;
!!fork_owner is shorter and idiomatic.
> + r = 0;
> + goto done;
> + }
> + if (ioctl == VHOST_GET_FORK_FROM_OWNER) {
> + fork_owner = d->fork_owner;
> + if (copy_to_user(argp, &fork_owner, sizeof(u8))) {
put_user
> + r = -EFAULT;
> + goto done;
> + }
> + r = 0;
> + goto done;
> + }
> +#endif
> /* You must be the owner to do anything else */
> r = vhost_dev_check_owner(d);
> if (r)
> diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> index d4b3e2ae1314..e51d6a347607 100644
> --- a/include/uapi/linux/vhost.h
> +++ b/include/uapi/linux/vhost.h
> @@ -235,4 +235,29 @@
> */
> #define VHOST_VDPA_GET_VRING_SIZE _IOWR(VHOST_VIRTIO, 0x82, \
> struct vhost_vring_state)
> +
> +/**
> + * VHOST_SET_FORK_FROM_OWNER - Set the fork_owner flag for the vhost device,
> + * This ioctl must called before VHOST_SET_OWNER.
> + * Only available when CONFIG_VHOST_ENABLE_FORK_OWNER_CONTROL=y
> + *
> + * @param fork_owner: An 8-bit value that determines the vhost thread mode
> + *
> + * When fork_owner is set to 1(default value):
> + * - Vhost will create vhost worker as tasks forked from the owner,
> + * inheriting all of the owner's attributes.
> + *
> + * When fork_owner is set to 0:
> + * - Vhost will create vhost workers as kernel threads.
> + */
> +#define VHOST_SET_FORK_FROM_OWNER _IOW(VHOST_VIRTIO, 0x83, __u8)
> +
> +/**
> + * VHOST_GET_FORK_OWNER - Get the current fork_owner flag for the vhost device.
> + * Only available when CONFIG_VHOST_ENABLE_FORK_OWNER_CONTROL=y
> + *
> + * @return: An 8-bit value indicating the current thread mode.
> + */
> +#define VHOST_GET_FORK_FROM_OWNER _IOR(VHOST_VIRTIO, 0x84, __u8)
> +
> #endif
> --
> 2.45.0
Powered by blists - more mailing lists