| 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: <20240930100452-mutt-send-email-mst@kernel.org>
Date: Mon, 30 Sep 2024 10:05:08 -0400
From: "Michael S. Tsirkin" <mst@...hat.com>
To: Aleksandr Mikhalitsyn <aleksandr.mikhalitsyn@...onical.com>
Cc: stefanha@...hat.com, Stefano Garzarella <sgarzare@...hat.com>,
Jason Wang <jasowang@...hat.com>,
Eugenio Pérez <eperezma@...hat.com>,
kvm@...r.kernel.org, virtualization@...ts.linux.dev,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] vhost/vsock: specify module version
On Mon, Sep 30, 2024 at 02:28:30PM +0200, Aleksandr Mikhalitsyn wrote:
> On Sun, Sep 29, 2024 at 9:03 PM Michael S. Tsirkin <mst@...hat.com> wrote:
> >
> > On Sun, Sep 29, 2024 at 08:21:03PM +0200, Alexander Mikhalitsyn wrote:
> > > Add an explicit MODULE_VERSION("0.0.1") specification for the vhost_vsock module.
> > >
> > > It is useful because it allows userspace to check if vhost_vsock is there when it is
> > > configured as a built-in.
> > >
> > > This is what we have *without* this change and when vhost_vsock is configured
> > > as a module and loaded:
> > >
> > > $ ls -la /sys/module/vhost_vsock
> > > total 0
> > > drwxr-xr-x 5 root root 0 Sep 29 19:00 .
> > > drwxr-xr-x 337 root root 0 Sep 29 18:59 ..
> > > -r--r--r-- 1 root root 4096 Sep 29 20:05 coresize
> > > drwxr-xr-x 2 root root 0 Sep 29 20:05 holders
> > > -r--r--r-- 1 root root 4096 Sep 29 20:05 initsize
> > > -r--r--r-- 1 root root 4096 Sep 29 20:05 initstate
> > > drwxr-xr-x 2 root root 0 Sep 29 20:05 notes
> > > -r--r--r-- 1 root root 4096 Sep 29 20:05 refcnt
> > > drwxr-xr-x 2 root root 0 Sep 29 20:05 sections
> > > -r--r--r-- 1 root root 4096 Sep 29 20:05 srcversion
> > > -r--r--r-- 1 root root 4096 Sep 29 20:05 taint
> > > --w------- 1 root root 4096 Sep 29 19:00 uevent
> > >
> > > When vhost_vsock is configured as a built-in there is *no* /sys/module/vhost_vsock directory at all.
> > > And this looks like an inconsistency.
> >
> > And that's expected.
> >
> > > With this change, when vhost_vsock is configured as a built-in we get:
> > > $ ls -la /sys/module/vhost_vsock/
> > > total 0
> > > drwxr-xr-x 2 root root 0 Sep 26 15:59 .
> > > drwxr-xr-x 100 root root 0 Sep 26 15:59 ..
> > > --w------- 1 root root 4096 Sep 26 15:59 uevent
> > > -r--r--r-- 1 root root 4096 Sep 26 15:59 version
> >
>
> Hi Michael,
>
> > Sorry, what I'd like to see is an explanation which userspace
> > is broken without this change, and whether this patch fixes is.
>
> Ok, let me try to write a proper commit message in this thread. I'll
> send a v3 once we agree on it (don't want to spam busy
> kvm developers with my one-liner fix in 10 different revisions :-) ).
>
> ============
> Add an explicit MODULE_VERSION("0.0.1") specification for the
> vhost_vsock module.
>
> It is useful because it allows userspace to check if vhost_vsock is
> there when it is
> configured as a built-in. We already have userspace consumers [1], [2]
> who rely on the
> assumption that if <any_linux_kernel_module> is loaded as a module OR configured
> as a built-in then /sys/module/<any_linux_kernel_module> exists. While
> this assumption
> works well in most cases it is wrong in general. For a built-in module
> X you get a /sys/module/<X>
> only if the module declares MODULE_VERSION or if the module has any
> parameter(s) declared.
>
> Let's just declare MODULE_VERSION("0.0.1") for vhost_vsock to make
> /sys/module/vhost_vsock
> to exist in all possible configurations (loadable module or built-in).
> Version 0.0.1 is chosen to align
> with all other modules in drivers/vhost.
>
> This is what we have *without* this change and when vhost_vsock is configured
> as a module and loaded:
>
> $ ls -la /sys/module/vhost_vsock
> total 0
> drwxr-xr-x 5 root root 0 Sep 29 19:00 .
> drwxr-xr-x 337 root root 0 Sep 29 18:59 ..
> -r--r--r-- 1 root root 4096 Sep 29 20:05 coresize
> drwxr-xr-x 2 root root 0 Sep 29 20:05 holders
> -r--r--r-- 1 root root 4096 Sep 29 20:05 initsize
> -r--r--r-- 1 root root 4096 Sep 29 20:05 initstate
> drwxr-xr-x 2 root root 0 Sep 29 20:05 notes
> -r--r--r-- 1 root root 4096 Sep 29 20:05 refcnt
> drwxr-xr-x 2 root root 0 Sep 29 20:05 sections
> -r--r--r-- 1 root root 4096 Sep 29 20:05 srcversion
> -r--r--r-- 1 root root 4096 Sep 29 20:05 taint
> --w------- 1 root root 4096 Sep 29 19:00 uevent
>
> When vhost_vsock is configured as a built-in there is *no*
> /sys/module/vhost_vsock directory at all.
> And this looks like an inconsistency.
>
> With this change, when vhost_vsock is configured as a built-in we get:
> $ ls -la /sys/module/vhost_vsock/
> total 0
> drwxr-xr-x 2 root root 0 Sep 26 15:59 .
> drwxr-xr-x 100 root root 0 Sep 26 15:59 ..
> --w------- 1 root root 4096 Sep 26 15:59 uevent
> -r--r--r-- 1 root root 4096 Sep 26 15:59 version
>
> Link: https://github.com/canonical/lxd/blob/ef33aea98aec9778499e96302f2605882d8249d7/lxd/instance/drivers/driver_qemu.go#L8568
> [1]
> Link: https://github.com/lxc/incus/blob/cbebce1dcd5f15887967058c8f6fec27cf0da2a2/internal/server/instance/drivers/driver_qemu.go#L8723
> [2]
> Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@...onical.com>
> ============
>
> Does this sound fair enough?
>
> Kind regards,
> Alex
Looks good, thanks!
> >
> >
> >
> > > Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@...onical.com>
> > > ---
> > > drivers/vhost/vsock.c | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
> > > index 802153e23073..287ea8e480b5 100644
> > > --- a/drivers/vhost/vsock.c
> > > +++ b/drivers/vhost/vsock.c
> > > @@ -956,6 +956,7 @@ static void __exit vhost_vsock_exit(void)
> > >
> > > module_init(vhost_vsock_init);
> > > module_exit(vhost_vsock_exit);
> > > +MODULE_VERSION("0.0.1");
> > > MODULE_LICENSE("GPL v2");
> > > MODULE_AUTHOR("Asias He");
> > > MODULE_DESCRIPTION("vhost transport for vsock ");
> > > --
> > > 2.34.1
> >
Powered by blists - more mailing lists