Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Misc DRM fixes #1548

Merged
merged 2 commits into from
Nov 17, 2022
Merged

Misc DRM fixes #1548

merged 2 commits into from
Nov 17, 2022

Conversation

jrtc27
Copy link
Member

@jrtc27 jrtc27 commented Nov 16, 2022

The first commit fixes KWin's ability to start Xwayland (though required X processes started by startplasma-wayland still crash and block plasma actually starting; one looks to be an unaligned allocation, and the other has the hallmarks of an adjust-pointers-by-realloc-diff bug, crashing on strlen of a capability to a string that points to valid data but has a cleared tag and is way out of bounds).

The second commit is why hybrid kernels are more broken than purecap kernels when it comes to DRM ioctls.

This function is at the FreeBSD/Linux boundary, with unlocked_ioctl
conforming to Linux's return value conventions, i.e. errors are
negative, and drm_fstub_ioctl conforming to FreeBSD's, i.e. errors are
positive. Thus, negate the return value here to translate between the
two, otherwise userspace will see negated errno values that don't match
any known value; in particular, libdrm's drmIsMaster will erroneously
always return true, which breaks KWin leasing a DRM fd to Xwayland.

This also matches what linuxkpi does, as used by drm-kmod.
@jrtc27
Copy link
Member Author

jrtc27 commented Nov 16, 2022

NB: evadot/drm-subtree#32 for the first commit

@gvnn3 gvnn3 self-requested a review November 16, 2022 18:47
Copy link
Contributor

@gvnn3 gvnn3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

The use of > rather than >= meant that for capability-aligned ends, if
copying backwands, and capability-aligned starts, if copying forwards,
the final capability to be copied was copied byte-wise, losing its tag,
rather than as a capability. This bug has been present since the very
first commit of this generic C version, but does not appear to have
existed in the old CHERI-MIPS assembly version.

Notably this breaks the DRM_IOCTL_VERSION ioctl, which has a capability
as its final struct member, in hybrid kernels, since the memcpy_c used
in drm_ioctl clears the tag on that member and then proceeds to EFAULT
when trying to copy out to it (note that it always performs a forward
copy here since it's copying from a higher stack frame to a lower one).
This gets even worse for libdrm since the mutated struct is then copied
back out itself, and libdrm tries to free the pointer on error, which
results in a tag fault.
@jrtc27 jrtc27 merged commit 3bea3f8 into dev Nov 17, 2022
@jrtc27 jrtc27 deleted the drm-fixes branch November 17, 2022 06:59
@paulmetzger
Copy link
Contributor

paulmetzger commented Nov 28, 2022

If I apply the first commit then Mesa fails at an assert:

pffm2@hazelnut:~ $ LD_LIBRARY_PATH=/usr/local/morello-purecap/lib/ WAFFLE_PLATFORM=gbm /home/pffm2/ext_packages/apitrace/build/eglretrace -b --singlethread /home/pffm2/traces/ufo.1.trace
/usr/lib/ufoai/ufo
renderableType = 8
debug: succeeded to create OpenGL 1.0 context 0x421f7080.
DEBUG: Set currentContextPtr to 0x421f7080
Assertion failed: (errno == ETIMEDOUT || errno == EBUSY), function panfrost_bo_wait, file ../src/panfrost/lib/pan_bo.c, line 152.
apitrace: warning: caught signal 6
495: error: caught an unhandled exception
libbacktrace: libbacktrace could not find executable to open
/lib/libthr.so.3: pthread_sigmask+0x5b3
/lib/libthr.so.3: pthread_setschedparam+0x97b
apitrace: info: taking default action for signal 6
Abort trap (core dumped)

It works fine if "-" is prepended to ETIMEDOUT and EBUSY in "panfrost_ioctl_wait_bo()" in the panfrost kernel module or when the change that was introduced with this commit is removed.

@jrtc27
Copy link
Member Author

jrtc27 commented Nov 28, 2022

Ew, so the panfrost ioctl implementations are conforming to the FreeBSD convention. That needs fixing, and probably an audit done of all the ioctls for drivers written for FreeBSD (i.e. not the bits taken from Linux which we know will be correct).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants