| 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: <CACLfguVUnpxCczJ5NdtWdiDVSsYDNG6XPFGdjqdgwncX4y74UA@mail.gmail.com>
Date: Sun, 15 Jun 2025 22:06:51 +0800
From: Cindy Lu <lulu@...hat.com>
To: "Michael S. Tsirkin" <mst@...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 Thu, Jun 12, 2025 at 2:31 PM Michael S. Tsirkin <mst@...hat.com> wrote:
>
> 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
>
Thanks, Micheal. I'll address all above comments and send a revised version.
Thanks
cindy
> > + 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