Re: [PATCH v7 8/8] vhost: Add a KConfig knob to enable IOCTL VHOST_FORK_FROM_OWNER

2025-04-05 Thread Cindy Lu 
On Tue, Mar 4, 2025 at 1:33 AM Michael S. Tsirkin  wrote:
>
> On Mon, Mar 03, 2025 at 01:52:06PM +0800, Jason Wang wrote:
> > On Sun, Mar 2, 2025 at 10:34 PM Cindy Lu  wrote:
> > >
> > > Introduce a new config knob `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL`,
> > > to control the availability of the `VHOST_FORK_FROM_OWNER` ioctl.
> > > When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is set to n, the ioctl
> > > is disabled, and any attempt to use it will result in failure.
> > >
> > > Signed-off-by: Cindy Lu 
> > > ---
> > >  drivers/vhost/Kconfig | 15 +++
> > >  drivers/vhost/vhost.c | 11 +++
> > >  2 files changed, 26 insertions(+)
> > >
> > > diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> > > index b455d9ab6f3d..e5b9dcbf31b6 100644
> > > --- a/drivers/vhost/Kconfig
> > > +++ b/drivers/vhost/Kconfig
> > > @@ -95,3 +95,18 @@ config VHOST_CROSS_ENDIAN_LEGACY
> > >   If unsure, say "N".
> > >
> > >  endif
> > > +
> > > +config VHOST_ENABLE_FORK_OWNER_IOCTL
> > > +   bool "Enable IOCTL VHOST_FORK_FROM_OWNER"
> > > +   default n
> > > +   help
> > > + This option enables the IOCTL VHOST_FORK_FROM_OWNER, which 
> > > allows
> > > + userspace applications to modify the thread mode for vhost 
> > > devices.
> > > +
> > > +  By default, `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL` is set to 
> > > `n`,
> > > +  meaning the ioctl is disabled and any operation using this 
> > > ioctl
> > > +  will fail.
> > > +  When the configuration is enabled (y), the ioctl becomes
> > > +  available, allowing users to set the mode if needed.
> > > +
> > > + If unsure, say "N".
> > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > > index fb0c7fb43f78..09e5e44dc516 100644
> > > --- a/drivers/vhost/vhost.c
> > > +++ b/drivers/vhost/vhost.c
> > > @@ -2294,6 +2294,8 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned 
> > > int ioctl, void __user *argp)
> > > r = vhost_dev_set_owner(d);
> > > goto done;
> > > }
> > > +
> > > +#ifdef CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL
> > > if (ioctl == VHOST_FORK_FROM_OWNER) {
> > > u8 inherit_owner;
> > > /*inherit_owner can only be modified before owner is set*/
> > > @@ -2313,6 +2315,15 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned 
> > > int ioctl, void __user *argp)
> > > r = 0;
> > > goto done;
> > > }
> > > +
> > > +#else
> > > +   if (ioctl == VHOST_FORK_FROM_OWNER) {
> > > +   /* When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is 'n', 
> > > return error */
> > > +   r = -ENOTTY;
> > > +   goto done;
> > > +   }
>
> why do we need this? won't it fail as any other unsupported ioctl?
>
Sure, will remove  this
thanks
cindy
> > > +#endif
> > > +
> > > /* You must be the owner to do anything else */
> > > r = vhost_dev_check_owner(d);
> > > if (r)
> > > --
> > > 2.45.0
> >
> > Do we need to change the default value of the inhert_owner? For example:
> >
> > #ifdef CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL
> > inherit_owner = false;
> > #else
> > inherit_onwer = true;
> > #endif
> >
> > ?
>
> I feel it is best to keep the default consistent.
> All the kconfig should do, is block the ioctl.
>
>
> > Other patches look good to me.
> >
> > Thanks
> >
> > >
>

Re: [PATCH v7 8/8] vhost: Add a KConfig knob to enable IOCTL VHOST_FORK_FROM_OWNER

2025-03-27 Thread Cindy Lu 
On Mon, Mar 3, 2025 at 5:12 PM Stefano Garzarella  wrote:
>
> On Mon, Mar 03, 2025 at 01:52:06PM +0800, Jason Wang wrote:
> >On Sun, Mar 2, 2025 at 10:34 PM Cindy Lu  wrote:
> >>
> >> Introduce a new config knob `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL`,
> >> to control the availability of the `VHOST_FORK_FROM_OWNER` ioctl.
> >> When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is set to n, the ioctl
> >> is disabled, and any attempt to use it will result in failure.
> >>
> >> Signed-off-by: Cindy Lu 
> >> ---
> >>  drivers/vhost/Kconfig | 15 +++
> >>  drivers/vhost/vhost.c | 11 +++
> >>  2 files changed, 26 insertions(+)
> >>
> >> diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> >> index b455d9ab6f3d..e5b9dcbf31b6 100644
> >> --- a/drivers/vhost/Kconfig
> >> +++ b/drivers/vhost/Kconfig
> >> @@ -95,3 +95,18 @@ config VHOST_CROSS_ENDIAN_LEGACY
> >>   If unsure, say "N".
> >>
> >>  endif
> >> +
> >> +config VHOST_ENABLE_FORK_OWNER_IOCTL
> >> +   bool "Enable IOCTL VHOST_FORK_FROM_OWNER"
> >> +   default n
> >> +   help
> >> + This option enables the IOCTL VHOST_FORK_FROM_OWNER, which allows
> >> + userspace applications to modify the thread mode for vhost 
> >> devices.
> >> +
> >> +  By default, `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL` is set to 
> >> `n`,
> >> +  meaning the ioctl is disabled and any operation using this ioctl
> >> +  will fail.
> >> +  When the configuration is enabled (y), the ioctl becomes
> >> +  available, allowing users to set the mode if needed.
> >> +
> >> + If unsure, say "N".
> >> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> >> index fb0c7fb43f78..09e5e44dc516 100644
> >> --- a/drivers/vhost/vhost.c
> >> +++ b/drivers/vhost/vhost.c
> >> @@ -2294,6 +2294,8 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned 
> >> int ioctl, void __user *argp)
> >> r = vhost_dev_set_owner(d);
> >> goto done;
> >> }
> >> +
> >> +#ifdef CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL
> >> if (ioctl == VHOST_FORK_FROM_OWNER) {
> >> u8 inherit_owner;
> >> /*inherit_owner can only be modified before owner is set*/
> >> @@ -2313,6 +2315,15 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned 
> >> int ioctl, void __user *argp)
> >> r = 0;
> >> goto done;
> >> }
> >> +
>
> nit: this empyt line is not needed
>
sure , will fix this
Thanks
Cindy
> >> +#else
> >> +   if (ioctl == VHOST_FORK_FROM_OWNER) {
> >> +   /* When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is 'n', 
> >> return error */
> >> +   r = -ENOTTY;
> >> +   goto done;
> >> +   }
> >> +#endif
> >> +
> >> /* You must be the owner to do anything else */
> >> r = vhost_dev_check_owner(d);
> >> if (r)
> >> --
> >> 2.45.0
> >
> >Do we need to change the default value of the inhert_owner? For example:
> >
> >#ifdef CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL
> >inherit_owner = false;
> >#else
> >inherit_onwer = true;
> >#endif
> >
> >?
>
> I'm not sure about this honestly, the user space has no way to figure
> out the default value and still has to do the IOCTL.
> So IMHO better to have a default value that is independent of the kernel
> configuration and consistent with the current behavior.
>
> Thanks,
> Stefano
>
> >
> >Other patches look good to me.
> >
> >Thanks
> >
> >>
> >
>

Re: [PATCH v7 8/8] vhost: Add a KConfig knob to enable IOCTL VHOST_FORK_FROM_OWNER

2025-03-21 Thread Michael S. Tsirkin 
On Mon, Mar 03, 2025 at 01:52:06PM +0800, Jason Wang wrote:
> On Sun, Mar 2, 2025 at 10:34 PM Cindy Lu  wrote:
> >
> > Introduce a new config knob `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL`,
> > to control the availability of the `VHOST_FORK_FROM_OWNER` ioctl.
> > When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is set to n, the ioctl
> > is disabled, and any attempt to use it will result in failure.
> >
> > Signed-off-by: Cindy Lu 
> > ---
> >  drivers/vhost/Kconfig | 15 +++
> >  drivers/vhost/vhost.c | 11 +++
> >  2 files changed, 26 insertions(+)
> >
> > diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> > index b455d9ab6f3d..e5b9dcbf31b6 100644
> > --- a/drivers/vhost/Kconfig
> > +++ b/drivers/vhost/Kconfig
> > @@ -95,3 +95,18 @@ config VHOST_CROSS_ENDIAN_LEGACY
> >   If unsure, say "N".
> >
> >  endif
> > +
> > +config VHOST_ENABLE_FORK_OWNER_IOCTL
> > +   bool "Enable IOCTL VHOST_FORK_FROM_OWNER"
> > +   default n
> > +   help
> > + This option enables the IOCTL VHOST_FORK_FROM_OWNER, which allows
> > + userspace applications to modify the thread mode for vhost 
> > devices.
> > +
> > +  By default, `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL` is set to `n`,
> > +  meaning the ioctl is disabled and any operation using this ioctl
> > +  will fail.
> > +  When the configuration is enabled (y), the ioctl becomes
> > +  available, allowing users to set the mode if needed.
> > +
> > + If unsure, say "N".
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index fb0c7fb43f78..09e5e44dc516 100644
> > --- a/drivers/vhost/vhost.c
> > +++ b/drivers/vhost/vhost.c
> > @@ -2294,6 +2294,8 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned 
> > int ioctl, void __user *argp)
> > r = vhost_dev_set_owner(d);
> > goto done;
> > }
> > +
> > +#ifdef CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL
> > if (ioctl == VHOST_FORK_FROM_OWNER) {
> > u8 inherit_owner;
> > /*inherit_owner can only be modified before owner is set*/
> > @@ -2313,6 +2315,15 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned 
> > int ioctl, void __user *argp)
> > r = 0;
> > goto done;
> > }
> > +
> > +#else
> > +   if (ioctl == VHOST_FORK_FROM_OWNER) {
> > +   /* When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is 'n', return 
> > error */
> > +   r = -ENOTTY;
> > +   goto done;
> > +   }
> > +#endif
> > +
> > /* You must be the owner to do anything else */
> > r = vhost_dev_check_owner(d);
> > if (r)
> > --
> > 2.45.0
> 
> Do we need to change the default value of the inhert_owner? For example:
> 
> #ifdef CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL
> inherit_owner = false;
> #else
> inherit_onwer = true;
> #endif
> 
> ?
> 
> Other patches look good to me.
> 
> Thanks


I think I agree with Cindy. kconfig just tells us whether the
ioctl is allowed. should not affect the default, if we want
a kconfig for default, it should be separate, but I doubt it.

> >

Re: [PATCH v7 8/8] vhost: Add a KConfig knob to enable IOCTL VHOST_FORK_FROM_OWNER

2025-03-20 Thread Cindy Lu 
Ping :)

On Mon, Mar 10, 2025 at 12:54 PM Jason Wang  wrote:
>
> On Tue, Mar 4, 2025 at 1:33 AM Michael S. Tsirkin  wrote:
> >
> > On Mon, Mar 03, 2025 at 01:52:06PM +0800, Jason Wang wrote:
> > > On Sun, Mar 2, 2025 at 10:34 PM Cindy Lu  wrote:
> > > >
> > > > Introduce a new config knob `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL`,
> > > > to control the availability of the `VHOST_FORK_FROM_OWNER` ioctl.
> > > > When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is set to n, the ioctl
> > > > is disabled, and any attempt to use it will result in failure.
> > > >
> > > > Signed-off-by: Cindy Lu 
> > > > ---
> > > >  drivers/vhost/Kconfig | 15 +++
> > > >  drivers/vhost/vhost.c | 11 +++
> > > >  2 files changed, 26 insertions(+)
> > > >
> > > > diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> > > > index b455d9ab6f3d..e5b9dcbf31b6 100644
> > > > --- a/drivers/vhost/Kconfig
> > > > +++ b/drivers/vhost/Kconfig
> > > > @@ -95,3 +95,18 @@ config VHOST_CROSS_ENDIAN_LEGACY
> > > >   If unsure, say "N".
> > > >
> > > >  endif
> > > > +
> > > > +config VHOST_ENABLE_FORK_OWNER_IOCTL
> > > > +   bool "Enable IOCTL VHOST_FORK_FROM_OWNER"
> > > > +   default n
> > > > +   help
> > > > + This option enables the IOCTL VHOST_FORK_FROM_OWNER, which 
> > > > allows
> > > > + userspace applications to modify the thread mode for vhost 
> > > > devices.
> > > > +
> > > > +  By default, `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL` is set to 
> > > > `n`,
> > > > +  meaning the ioctl is disabled and any operation using this 
> > > > ioctl
> > > > +  will fail.
> > > > +  When the configuration is enabled (y), the ioctl becomes
> > > > +  available, allowing users to set the mode if needed.
> > > > +
> > > > + If unsure, say "N".
> > > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > > > index fb0c7fb43f78..09e5e44dc516 100644
> > > > --- a/drivers/vhost/vhost.c
> > > > +++ b/drivers/vhost/vhost.c
> > > > @@ -2294,6 +2294,8 @@ long vhost_dev_ioctl(struct vhost_dev *d, 
> > > > unsigned int ioctl, void __user *argp)
> > > > r = vhost_dev_set_owner(d);
> > > > goto done;
> > > > }
> > > > +
> > > > +#ifdef CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL
> > > > if (ioctl == VHOST_FORK_FROM_OWNER) {
> > > > u8 inherit_owner;
> > > > /*inherit_owner can only be modified before owner is 
> > > > set*/
> > > > @@ -2313,6 +2315,15 @@ long vhost_dev_ioctl(struct vhost_dev *d, 
> > > > unsigned int ioctl, void __user *argp)
> > > > r = 0;
> > > > goto done;
> > > > }
> > > > +
> > > > +#else
> > > > +   if (ioctl == VHOST_FORK_FROM_OWNER) {
> > > > +   /* When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is 'n', 
> > > > return error */
> > > > +   r = -ENOTTY;
> > > > +   goto done;
> > > > +   }
> >
> > why do we need this? won't it fail as any other unsupported ioctl?
> >
> > > > +#endif
> > > > +
> > > > /* You must be the owner to do anything else */
> > > > r = vhost_dev_check_owner(d);
> > > > if (r)
> > > > --
> > > > 2.45.0
> > >
> > > Do we need to change the default value of the inhert_owner? For example:
> > >
> > > #ifdef CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL
> > > inherit_owner = false;
> > > #else
> > > inherit_onwer = true;
> > > #endif
> > >
> > > ?
> >
> > I feel it is best to keep the default consistent.
>
> Just want to make sure we are on the same page.
>
> For "default", did you mean inherit_owner = false which is consistent
> with behaviour without the vhost task?
>
> Or inherit_onwer = true, then the new ioctl to make it false is
> useless. And if legacy applications want kthread behaviour it needs to
> be patched which seems self-contradictory.
>
> > All the kconfig should do, is block the ioctl.
> >
>
> Thanks
>
> >
> > > Other patches look good to me.
> > >
> > > Thanks
> > >
> > > >
> >
>

Re: [PATCH v7 8/8] vhost: Add a KConfig knob to enable IOCTL VHOST_FORK_FROM_OWNER

2025-03-09 Thread Jason Wang 
On Tue, Mar 4, 2025 at 1:33 AM Michael S. Tsirkin  wrote:
>
> On Mon, Mar 03, 2025 at 01:52:06PM +0800, Jason Wang wrote:
> > On Sun, Mar 2, 2025 at 10:34 PM Cindy Lu  wrote:
> > >
> > > Introduce a new config knob `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL`,
> > > to control the availability of the `VHOST_FORK_FROM_OWNER` ioctl.
> > > When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is set to n, the ioctl
> > > is disabled, and any attempt to use it will result in failure.
> > >
> > > Signed-off-by: Cindy Lu 
> > > ---
> > >  drivers/vhost/Kconfig | 15 +++
> > >  drivers/vhost/vhost.c | 11 +++
> > >  2 files changed, 26 insertions(+)
> > >
> > > diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> > > index b455d9ab6f3d..e5b9dcbf31b6 100644
> > > --- a/drivers/vhost/Kconfig
> > > +++ b/drivers/vhost/Kconfig
> > > @@ -95,3 +95,18 @@ config VHOST_CROSS_ENDIAN_LEGACY
> > >   If unsure, say "N".
> > >
> > >  endif
> > > +
> > > +config VHOST_ENABLE_FORK_OWNER_IOCTL
> > > +   bool "Enable IOCTL VHOST_FORK_FROM_OWNER"
> > > +   default n
> > > +   help
> > > + This option enables the IOCTL VHOST_FORK_FROM_OWNER, which 
> > > allows
> > > + userspace applications to modify the thread mode for vhost 
> > > devices.
> > > +
> > > +  By default, `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL` is set to 
> > > `n`,
> > > +  meaning the ioctl is disabled and any operation using this 
> > > ioctl
> > > +  will fail.
> > > +  When the configuration is enabled (y), the ioctl becomes
> > > +  available, allowing users to set the mode if needed.
> > > +
> > > + If unsure, say "N".
> > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > > index fb0c7fb43f78..09e5e44dc516 100644
> > > --- a/drivers/vhost/vhost.c
> > > +++ b/drivers/vhost/vhost.c
> > > @@ -2294,6 +2294,8 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned 
> > > int ioctl, void __user *argp)
> > > r = vhost_dev_set_owner(d);
> > > goto done;
> > > }
> > > +
> > > +#ifdef CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL
> > > if (ioctl == VHOST_FORK_FROM_OWNER) {
> > > u8 inherit_owner;
> > > /*inherit_owner can only be modified before owner is set*/
> > > @@ -2313,6 +2315,15 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned 
> > > int ioctl, void __user *argp)
> > > r = 0;
> > > goto done;
> > > }
> > > +
> > > +#else
> > > +   if (ioctl == VHOST_FORK_FROM_OWNER) {
> > > +   /* When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is 'n', 
> > > return error */
> > > +   r = -ENOTTY;
> > > +   goto done;
> > > +   }
>
> why do we need this? won't it fail as any other unsupported ioctl?
>
> > > +#endif
> > > +
> > > /* You must be the owner to do anything else */
> > > r = vhost_dev_check_owner(d);
> > > if (r)
> > > --
> > > 2.45.0
> >
> > Do we need to change the default value of the inhert_owner? For example:
> >
> > #ifdef CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL
> > inherit_owner = false;
> > #else
> > inherit_onwer = true;
> > #endif
> >
> > ?
>
> I feel it is best to keep the default consistent.

Just want to make sure we are on the same page.

For "default", did you mean inherit_owner = false which is consistent
with behaviour without the vhost task?

Or inherit_onwer = true, then the new ioctl to make it false is
useless. And if legacy applications want kthread behaviour it needs to
be patched which seems self-contradictory.

> All the kconfig should do, is block the ioctl.
>

Thanks

>
> > Other patches look good to me.
> >
> > Thanks
> >
> > >
>

Re: [PATCH v7 8/8] vhost: Add a KConfig knob to enable IOCTL VHOST_FORK_FROM_OWNER

2025-03-03 Thread Michael S. Tsirkin 
On Mon, Mar 03, 2025 at 01:52:06PM +0800, Jason Wang wrote:
> On Sun, Mar 2, 2025 at 10:34 PM Cindy Lu  wrote:
> >
> > Introduce a new config knob `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL`,
> > to control the availability of the `VHOST_FORK_FROM_OWNER` ioctl.
> > When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is set to n, the ioctl
> > is disabled, and any attempt to use it will result in failure.
> >
> > Signed-off-by: Cindy Lu 
> > ---
> >  drivers/vhost/Kconfig | 15 +++
> >  drivers/vhost/vhost.c | 11 +++
> >  2 files changed, 26 insertions(+)
> >
> > diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> > index b455d9ab6f3d..e5b9dcbf31b6 100644
> > --- a/drivers/vhost/Kconfig
> > +++ b/drivers/vhost/Kconfig
> > @@ -95,3 +95,18 @@ config VHOST_CROSS_ENDIAN_LEGACY
> >   If unsure, say "N".
> >
> >  endif
> > +
> > +config VHOST_ENABLE_FORK_OWNER_IOCTL
> > +   bool "Enable IOCTL VHOST_FORK_FROM_OWNER"
> > +   default n
> > +   help
> > + This option enables the IOCTL VHOST_FORK_FROM_OWNER, which allows
> > + userspace applications to modify the thread mode for vhost 
> > devices.
> > +
> > +  By default, `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL` is set to `n`,
> > +  meaning the ioctl is disabled and any operation using this ioctl
> > +  will fail.
> > +  When the configuration is enabled (y), the ioctl becomes
> > +  available, allowing users to set the mode if needed.
> > +
> > + If unsure, say "N".
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index fb0c7fb43f78..09e5e44dc516 100644
> > --- a/drivers/vhost/vhost.c
> > +++ b/drivers/vhost/vhost.c
> > @@ -2294,6 +2294,8 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned 
> > int ioctl, void __user *argp)
> > r = vhost_dev_set_owner(d);
> > goto done;
> > }
> > +
> > +#ifdef CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL
> > if (ioctl == VHOST_FORK_FROM_OWNER) {
> > u8 inherit_owner;
> > /*inherit_owner can only be modified before owner is set*/
> > @@ -2313,6 +2315,15 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned 
> > int ioctl, void __user *argp)
> > r = 0;
> > goto done;
> > }
> > +
> > +#else
> > +   if (ioctl == VHOST_FORK_FROM_OWNER) {
> > +   /* When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is 'n', return 
> > error */
> > +   r = -ENOTTY;
> > +   goto done;
> > +   }

why do we need this? won't it fail as any other unsupported ioctl?

> > +#endif
> > +
> > /* You must be the owner to do anything else */
> > r = vhost_dev_check_owner(d);
> > if (r)
> > --
> > 2.45.0
> 
> Do we need to change the default value of the inhert_owner? For example:
> 
> #ifdef CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL
> inherit_owner = false;
> #else
> inherit_onwer = true;
> #endif
> 
> ?

I feel it is best to keep the default consistent.
All the kconfig should do, is block the ioctl.


> Other patches look good to me.
> 
> Thanks
> 
> >

Re: [PATCH v7 8/8] vhost: Add a KConfig knob to enable IOCTL VHOST_FORK_FROM_OWNER

2025-03-03 Thread Stefano Garzarella 

On Mon, Mar 03, 2025 at 01:52:06PM +0800, Jason Wang wrote:

On Sun, Mar 2, 2025 at 10:34 PM Cindy Lu  wrote:


Introduce a new config knob `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL`,
to control the availability of the `VHOST_FORK_FROM_OWNER` ioctl.
When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is set to n, the ioctl
is disabled, and any attempt to use it will result in failure.

Signed-off-by: Cindy Lu 
---
 drivers/vhost/Kconfig | 15 +++
 drivers/vhost/vhost.c | 11 +++
 2 files changed, 26 insertions(+)

diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
index b455d9ab6f3d..e5b9dcbf31b6 100644
--- a/drivers/vhost/Kconfig
+++ b/drivers/vhost/Kconfig
@@ -95,3 +95,18 @@ config VHOST_CROSS_ENDIAN_LEGACY
  If unsure, say "N".

 endif
+
+config VHOST_ENABLE_FORK_OWNER_IOCTL
+   bool "Enable IOCTL VHOST_FORK_FROM_OWNER"
+   default n
+   help
+ This option enables the IOCTL VHOST_FORK_FROM_OWNER, which allows
+ userspace applications to modify the thread mode for vhost devices.
+
+  By default, `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL` is set to `n`,
+  meaning the ioctl is disabled and any operation using this ioctl
+  will fail.
+  When the configuration is enabled (y), the ioctl becomes
+  available, allowing users to set the mode if needed.
+
+ If unsure, say "N".
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index fb0c7fb43f78..09e5e44dc516 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -2294,6 +2294,8 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int 
ioctl, void __user *argp)
r = vhost_dev_set_owner(d);
goto done;
}
+
+#ifdef CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL
if (ioctl == VHOST_FORK_FROM_OWNER) {
u8 inherit_owner;
/*inherit_owner can only be modified before owner is set*/
@@ -2313,6 +2315,15 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int 
ioctl, void __user *argp)
r = 0;
goto done;
}
+


nit: this empyt line is not needed


+#else
+   if (ioctl == VHOST_FORK_FROM_OWNER) {
+   /* When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is 'n', return 
error */
+   r = -ENOTTY;
+   goto done;
+   }
+#endif
+
/* You must be the owner to do anything else */
r = vhost_dev_check_owner(d);
if (r)
--
2.45.0


Do we need to change the default value of the inhert_owner? For example:

#ifdef CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL
inherit_owner = false;
#else
inherit_onwer = true;
#endif

?


I'm not sure about this honestly, the user space has no way to figure 
out the default value and still has to do the IOCTL.
So IMHO better to have a default value that is independent of the kernel 
configuration and consistent with the current behavior.


Thanks,
Stefano



Other patches look good to me.

Thanks

Re: [PATCH v7 8/8] vhost: Add a KConfig knob to enable IOCTL VHOST_FORK_FROM_OWNER

2025-03-02 Thread Jason Wang 
On Sun, Mar 2, 2025 at 10:34 PM Cindy Lu  wrote:
>
> Introduce a new config knob `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL`,
> to control the availability of the `VHOST_FORK_FROM_OWNER` ioctl.
> When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is set to n, the ioctl
> is disabled, and any attempt to use it will result in failure.
>
> Signed-off-by: Cindy Lu 
> ---
>  drivers/vhost/Kconfig | 15 +++
>  drivers/vhost/vhost.c | 11 +++
>  2 files changed, 26 insertions(+)
>
> diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> index b455d9ab6f3d..e5b9dcbf31b6 100644
> --- a/drivers/vhost/Kconfig
> +++ b/drivers/vhost/Kconfig
> @@ -95,3 +95,18 @@ config VHOST_CROSS_ENDIAN_LEGACY
>   If unsure, say "N".
>
>  endif
> +
> +config VHOST_ENABLE_FORK_OWNER_IOCTL
> +   bool "Enable IOCTL VHOST_FORK_FROM_OWNER"
> +   default n
> +   help
> + This option enables the IOCTL VHOST_FORK_FROM_OWNER, which allows
> + userspace applications to modify the thread mode for vhost devices.
> +
> +  By default, `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL` is set to `n`,
> +  meaning the ioctl is disabled and any operation using this ioctl
> +  will fail.
> +  When the configuration is enabled (y), the ioctl becomes
> +  available, allowing users to set the mode if needed.
> +
> + If unsure, say "N".
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index fb0c7fb43f78..09e5e44dc516 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -2294,6 +2294,8 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int 
> ioctl, void __user *argp)
> r = vhost_dev_set_owner(d);
> goto done;
> }
> +
> +#ifdef CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL
> if (ioctl == VHOST_FORK_FROM_OWNER) {
> u8 inherit_owner;
> /*inherit_owner can only be modified before owner is set*/
> @@ -2313,6 +2315,15 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int 
> ioctl, void __user *argp)
> r = 0;
> goto done;
> }
> +
> +#else
> +   if (ioctl == VHOST_FORK_FROM_OWNER) {
> +   /* When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is 'n', return 
> error */
> +   r = -ENOTTY;
> +   goto done;
> +   }
> +#endif
> +
> /* You must be the owner to do anything else */
> r = vhost_dev_check_owner(d);
> if (r)
> --
> 2.45.0

Do we need to change the default value of the inhert_owner? For example:

#ifdef CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL
inherit_owner = false;
#else
inherit_onwer = true;
#endif

?

Other patches look good to me.

Thanks

>