qubes-gui: get rid of shmoverride

[iamahuman]

The problem you're addressing (if any)

• Get rid of the following error that pops up time to time preventing windows from showing up, caused by leftover /var/run/qubes/shm.id.<DISPLAY>:

  ErrorHandler: BadValue (integer parameter out of range for operation)
                 Major opcode: 130 (MIT-SHM)
                 Minor opcode: 3 (X_ShmPutImage)
                 Value:        0x7d0
                 Failed serial number:  243
                 Current serial number: 244

  • Remove partially unreliable kludge of hooking System V shared memory operations in X11.

Describe the solution you'd like Replace shmoverride.so with MIT-SHM X_ShmAttachFd, passing the gntdev fd directly.

Ideally, consider looking into gntdev DMA-BUF, to be used with either glamor or Wayland. DMA-BUF is the "right" way to achieve this, ignoring everything else.

Where is the value to a user, and who might that user be? Not immediately apparent, but may solve the issues above for the user.

Describe alternatives you've considered Not applicable.

Additional context ~This only applies to Qubes OS 4.1 and above. X_ShmAttachFd is introduced in version 1.15 of X.org server, which is not available for Qubes OS 4.0.~ (EDIT: it is, the X server version is already 1.19.3; my apologies).

Relevant documentation you've consulted

• https://www.qubes-os.org/doc/gui/
• https://lists.x.org/archives/xorg-announce/2013-November/002350.html

Related, non-duplicate issues TBD

[marmarek]

This may be a good idea indeed, but it is too late for this change to go into R4.1 (we are basically in a soft feature freeze), lets schedule it for R4.2 or later.

[DemiMarie]

Ideally, consider looking into gntdev DMA-BUF, to be used with either glamor or Wayland. DMA-BUF is the "right" way to achieve this, ignoring everything else.

How does one use DMA-BUF with Glamor? I did not find any documentation for that.

[marmarek]

This turned out to be not that simple - copying @DemiMarie comment from the PR:

I kept getting BadMatch errors no matter what I tried on the client side. I finally checked the X.Org Server source code, and found this:

    if (fstat(fd, &statb) < 0 || statb.st_size == 0) {
        close(fd);
        return BadMatch;
    }

    shmdesc = malloc(sizeof(ShmDescRec));
    if (!shmdesc) {
        close(fd);
        return BadAlloc;
    }
    shmdesc->is_fd = TRUE;
    shmdesc->addr = mmap(NULL, statb.st_size,
                         stuff->readOnly ? PROT_READ : PROT_READ|PROT_WRITE,
                         MAP_SHARED,
                         fd, 0);

    close(fd);
    if (shmdesc->addr == ((char *) -1)) {
        free(shmdesc);
        return BadAccess;
    }

That is, the server rejects file descriptors pointing to files that appear to be empty, and mmap()s non-empty files in their entirety. This is reasonable in the case of regular files, where sending a descriptor to an empty file is almost certainly a client bug, and where the size of the file determines how large the shared memory segment should be.

However, /dev/xen/gntdev is not a regular file! It is a character device and therefore of course has size 0. There is no way for the GUI daemon to set the size to something else, and the request has no other way to convey this information. Theoretically, the X server could keep the file descriptor open and defer mapping it until xcb_shm_put_image is called, but that would require as-yet-unknown changes on the X server side. And if we can change the X server, we can solve the problem properly: with a new extension that allows the client to specify the sizes. Another alternative is to use the DRI3 extension and dma-bufs, but that would be both significantly more complex and involve less-tested kernel code.

After discussing it a bit more, @andyhhp gave us an idea: adjust the kernel side, to let the /dev/xen/gntdev (or new device node?) behave like Xorg (or Wayland) wants. Specifically - always use 0 offset (and separate FD for each map), and let the fstat() return the mapping size. That could work, and for Xorg would have an benefit of not requiring DRI3 being present (specifically - working with dummy driver too, which we need). But on the other hand, we'll likely need DMA-BUF anyway, so that sounds like a more logical path to pursue first.

[DemiMarie]

Wayland allows for a size to be passed along with the file descriptor. Furthermore, a fresh instance of gntdev will always set struct ioctl_gntdev_map_grant_ref::index to 0 in the first call to IOCTL_GNTDEV_MAP_GRANT_REF. This means that shmoverride will not be necessary at all under Wayland, provided that we drop support for the old mfn-based method.

[marmarek]

This means that shmoverride will not be necessary at all under Wayland, provided that we drop support for the old mfn-based method.

Yes, this is ok for Wayland version to not support mfn-based sharing.