Re: [PATCH resend v2 0/3] virt: Add vboxguest driver for Virtual Box Guest integration

[Hans de Goede]

Hi,

On 28-11-17 17:15, Larry Finger wrote:
> On 11/28/2017 04:01 AM, Hans de Goede wrote:
> > Hi,
>
> > > I did have two problems when I tried to build these commits and the one that creates vboxsf.
> > >
> > > The more serious one is that it is possible to build vboxguest without vboxvideo. When that happens, a non-privileged user cannot start X. As I say in the review, > I think that combination does not make sense and should not be allowed.
> >
> > vboxguest and vboxvideo are completely independent at least from the kernel pov,
> > I do not believe that making them depend on each other makes sense.
> >
> > AFAIK a non-privileged user cannot start X without vboxvideo at all, independent
> > of vboxguest being build or not. Falling back to vesa modesetting always requires
> > Xorg to be suid root, or the user to be privileged.
> >
> > TL;DR: I can add a dependency between the 2, but I would rather not.
>
> Keep in mind that at some point, the newest kernel will support vboxvideo and vboxguest; however, any distribution package will still need to contain both kernel modules so that older kernels will work. My test showed that loading an in-kernel vboxguest with Oracle's vboxvideo fails *unless* you run as root, which is not acceptable.

Hmm, I guess what is happening here is that the vbox provided vboxguest is not loading
because the one built into the kernel is not loading, and then vboxvideo does not load
because it depends on symbols from vboxguest. But the vboxvideo shipped with the guest
additions has been changed to no longer depend on vboxguest quite a while ago, so with
current guest additions this should not be an issue and functionality wise the 2
vboxguest drivers are equivalent.

"depends on" / "select" really are reserved for when drivers use symbols from another
module and not to express weird interactions like this. There are many ways a .config
can be made which userspace will not like, yet we still allow this. IMHO adding a
depends on between 2 independent modules is simply just wrong.

For the next version I will add a text to the help part of the Kconfig advising to also
enable the vboxvideo driver. That really is the best we can do IMHO.

Talking about the next version, yesterday I ran out of time while working on this,
I hope to post a v3 addressing your review comments today.

> When both are in the kernel, then Xorg starts for a non-privaleged user. That is why I think you need either a "depends on VBOXVIDEO" or a "selects VBOXVIDEO" in the Kconfig for vboxvideo. My preference is for the latter.
>
> > > When the system is booted, vboxsf is not loaded, and the shared folders are not automounted. Of course, that issue is not germane to these patches, but will be important when vboxsf is merged.
> >
> > Hmm, I mount a couple of shares from rc.local (I don't use vbox' automount as I
> > want to specify a uid for the files) and as soon as mount.vboxsf gets executed
> > the vboxsf module gets auto-loaded as the module contains:
> >
> > MODULE_ALIAS_FS("vboxsf");
> >
> > AFAIK the communication of which volumes to automount is done through vboxguest,
> > anyways I will look into this before submitting vboxsf, in the worst case
> > we need to drop a modprobe.conf.d/vboxguest.conf file which has a postinst vboxguest
> > which loads vboxsf.
>
> Thanks. Adding something of this type will make the in-kernel version match the Oracle documentation.

Regards,

Hans