mirror of
https://github.com/RGBCube/serenity
synced 2025-07-25 23:17:45 +00:00
Kernel+Userland: Add ioctl to set process ownership of DisplayConnector
Now that the infrastructure of the Graphics subsystem is quite stable, it is time to try to fix a long-standing problem, which is the lack of locking on display connector devices. Reading and writing from multiple processes to a framebuffer controlled by the display connector is not a huge problem - it could be solved with POSIX locking. The real problem is some program that will try to do ioctl operations on a display connector without the WindowServer being aware of that which can lead to very bad situations, for example - assuming a framebuffer is encoded at a known resolution and certain display timings, but another process changed the ModeSetting of the display connector, leading to inconsistency on the properties of the current ModeSetting. To solve this, there's a new "master" ioctl to take "ownership" and another one to release that ownership of a display connector device. To ensure we will not hold a Process object forever just because it has an ownership over a display connector, we hold it with a weak reference, and if the process is gone, someone else can take an ownership.
This commit is contained in:
parent
1968aba69b
commit
977aa81310
5 changed files with 101 additions and 8 deletions
|
@ -50,6 +50,16 @@ ALWAYS_INLINE int graphics_connector_get_head_edid(int fd, GraphicsHeadEDID* inf
|
|||
return 0;
|
||||
}
|
||||
|
||||
ALWAYS_INLINE int graphics_connector_set_responsible(int fd)
|
||||
{
|
||||
return ioctl(fd, GRAPHICS_IOCTL_SET_RESPONSIBLE, nullptr);
|
||||
}
|
||||
|
||||
ALWAYS_INLINE int graphics_connector_unset_responsible(int fd)
|
||||
{
|
||||
return ioctl(fd, GRAPHICS_IOCTL_UNSET_RESPONSIBLE, nullptr);
|
||||
}
|
||||
|
||||
ALWAYS_INLINE int fb_get_head_vertical_offset_buffer(int fd, GraphicsHeadVerticalOffset* vertical_offset)
|
||||
{
|
||||
return ioctl(fd, GRAPHICS_IOCTL_GET_HEAD_VERTICAL_OFFSET_BUFFER, vertical_offset);
|
||||
|
|
|
@ -231,11 +231,81 @@ ErrorOr<ByteBuffer> DisplayConnector::get_edid() const
|
|||
return ByteBuffer::copy(m_edid_bytes, sizeof(m_edid_bytes));
|
||||
}
|
||||
|
||||
struct GraphicsIOCtlChecker {
|
||||
unsigned ioctl_number;
|
||||
StringView name;
|
||||
bool requires_ownership { false };
|
||||
};
|
||||
|
||||
static constexpr GraphicsIOCtlChecker s_checkers[] = {
|
||||
{ GRAPHICS_IOCTL_GET_PROPERTIES, "GRAPHICS_IOCTL_GET_PROPERTIES"sv, false },
|
||||
{ GRAPHICS_IOCTL_SET_HEAD_VERTICAL_OFFSET_BUFFER, "GRAPHICS_IOCTL_SET_HEAD_VERTICAL_OFFSET_BUFFER"sv, true },
|
||||
{ GRAPHICS_IOCTL_GET_HEAD_VERTICAL_OFFSET_BUFFER, "GRAPHICS_IOCTL_GET_HEAD_VERTICAL_OFFSET_BUFFER"sv, false },
|
||||
{ GRAPHICS_IOCTL_FLUSH_HEAD_BUFFERS, "GRAPHICS_IOCTL_FLUSH_HEAD_BUFFERS"sv, true },
|
||||
{ GRAPHICS_IOCTL_FLUSH_HEAD, "GRAPHICS_IOCTL_FLUSH_HEAD"sv, true },
|
||||
{ GRAPHICS_IOCTL_SET_HEAD_MODE_SETTING, "GRAPHICS_IOCTL_SET_HEAD_MODE_SETTING"sv, true },
|
||||
{ GRAPHICS_IOCTL_GET_HEAD_MODE_SETTING, "GRAPHICS_IOCTL_GET_HEAD_MODE_SETTING"sv, false },
|
||||
{ GRAPHICS_IOCTL_SET_SAFE_HEAD_MODE_SETTING, "GRAPHICS_IOCTL_SET_SAFE_HEAD_MODE_SETTING"sv, true },
|
||||
{ GRAPHICS_IOCTL_SET_RESPONSIBLE, "GRAPHICS_IOCTL_SET_RESPONSIBLE"sv, false },
|
||||
{ GRAPHICS_IOCTL_UNSET_RESPONSIBLE, "GRAPHICS_IOCTL_UNSET_RESPONSIBLE"sv, true },
|
||||
};
|
||||
|
||||
static StringView ioctl_to_stringview(unsigned request)
|
||||
{
|
||||
for (auto& checker : s_checkers) {
|
||||
if (checker.ioctl_number == request)
|
||||
return checker.name;
|
||||
}
|
||||
VERIFY_NOT_REACHED();
|
||||
}
|
||||
|
||||
bool DisplayConnector::ioctl_requires_ownership(unsigned request) const
|
||||
{
|
||||
for (auto& checker : s_checkers) {
|
||||
if (checker.ioctl_number == request)
|
||||
return checker.requires_ownership;
|
||||
}
|
||||
VERIFY_NOT_REACHED();
|
||||
}
|
||||
|
||||
ErrorOr<void> DisplayConnector::ioctl(OpenFileDescription&, unsigned request, Userspace<void*> arg)
|
||||
{
|
||||
TRY(Process::current().require_promise(Pledge::video));
|
||||
|
||||
// Note: We only allow to set responsibility on a DisplayConnector,
|
||||
// get the current ModeSetting or the hardware framebuffer properties without the
|
||||
// need of having an established responsibility on a DisplayConnector.
|
||||
if (ioctl_requires_ownership(request)) {
|
||||
auto process = m_responsible_process.strong_ref();
|
||||
if (!process || process.ptr() != &Process::current()) {
|
||||
dbgln("DisplayConnector::ioctl: {} requires ownership over the device", ioctl_to_stringview(request));
|
||||
return Error::from_errno(EPERM);
|
||||
}
|
||||
}
|
||||
|
||||
switch (request) {
|
||||
case GRAPHICS_IOCTL_SET_RESPONSIBLE: {
|
||||
SpinlockLocker locker(m_responsible_process_lock);
|
||||
auto process = m_responsible_process.strong_ref();
|
||||
// Note: If there's already a process being responsible, just return an error.
|
||||
// We could technically return 0 if the the requesting process is already
|
||||
// was set to be responsible for this DisplayConnector, but it servicing no
|
||||
// no good purpose and should be considered a bug if this happens anyway.
|
||||
if (process)
|
||||
return Error::from_errno(EPERM);
|
||||
m_responsible_process = Process::current();
|
||||
return {};
|
||||
}
|
||||
case GRAPHICS_IOCTL_UNSET_RESPONSIBLE: {
|
||||
SpinlockLocker locker(m_responsible_process_lock);
|
||||
auto process = m_responsible_process.strong_ref();
|
||||
if (!process)
|
||||
return Error::from_errno(ESRCH);
|
||||
if (process.ptr() != &Process::current())
|
||||
return Error::from_errno(EPERM);
|
||||
m_responsible_process.clear();
|
||||
return {};
|
||||
}
|
||||
case GRAPHICS_IOCTL_GET_PROPERTIES: {
|
||||
auto user_properties = static_ptr_cast<GraphicsConnectorProperties*>(arg);
|
||||
GraphicsConnectorProperties properties {};
|
||||
|
|
|
@ -148,6 +148,8 @@ private:
|
|||
virtual void will_be_destroyed() override;
|
||||
virtual void after_inserting() override;
|
||||
|
||||
bool ioctl_requires_ownership(unsigned request) const;
|
||||
|
||||
OwnPtr<Memory::Region> m_framebuffer_region;
|
||||
OwnPtr<Memory::Region> m_fake_writes_framebuffer_region;
|
||||
u8* m_framebuffer_data {};
|
||||
|
@ -162,6 +164,9 @@ protected:
|
|||
private:
|
||||
RefPtr<Memory::SharedFramebufferVMObject> m_shared_framebuffer_vmobject;
|
||||
|
||||
WeakPtr<Process> m_responsible_process;
|
||||
Spinlock m_responsible_process_lock;
|
||||
|
||||
IntrusiveListNode<DisplayConnector, RefPtr<DisplayConnector>> m_list_node;
|
||||
};
|
||||
}
|
||||
|
|
|
@ -101,6 +101,8 @@ enum IOCtlNumber {
|
|||
GRAPHICS_IOCTL_SET_HEAD_MODE_SETTING,
|
||||
GRAPHICS_IOCTL_GET_HEAD_MODE_SETTING,
|
||||
GRAPHICS_IOCTL_SET_SAFE_HEAD_MODE_SETTING,
|
||||
GRAPHICS_IOCTL_SET_RESPONSIBLE,
|
||||
GRAPHICS_IOCTL_UNSET_RESPONSIBLE,
|
||||
KEYBOARD_IOCTL_GET_NUM_LOCK,
|
||||
KEYBOARD_IOCTL_SET_NUM_LOCK,
|
||||
KEYBOARD_IOCTL_GET_CAPS_LOCK,
|
||||
|
@ -160,6 +162,8 @@ enum IOCtlNumber {
|
|||
#define GRAPHICS_IOCTL_SET_HEAD_MODE_SETTING GRAPHICS_IOCTL_SET_HEAD_MODE_SETTING
|
||||
#define GRAPHICS_IOCTL_GET_HEAD_MODE_SETTING GRAPHICS_IOCTL_GET_HEAD_MODE_SETTING
|
||||
#define GRAPHICS_IOCTL_SET_SAFE_HEAD_MODE_SETTING GRAPHICS_IOCTL_SET_SAFE_HEAD_MODE_SETTING
|
||||
#define GRAPHICS_IOCTL_SET_RESPONSIBLE GRAPHICS_IOCTL_SET_RESPONSIBLE
|
||||
#define GRAPHICS_IOCTL_UNSET_RESPONSIBLE GRAPHICS_IOCTL_UNSET_RESPONSIBLE
|
||||
#define KEYBOARD_IOCTL_GET_NUM_LOCK KEYBOARD_IOCTL_GET_NUM_LOCK
|
||||
#define KEYBOARD_IOCTL_SET_NUM_LOCK KEYBOARD_IOCTL_SET_NUM_LOCK
|
||||
#define KEYBOARD_IOCTL_GET_CAPS_LOCK KEYBOARD_IOCTL_GET_CAPS_LOCK
|
||||
|
|
|
@ -9,6 +9,7 @@
|
|||
#include "EventLoop.h"
|
||||
#include "Screen.h"
|
||||
#include "WindowManager.h"
|
||||
#include <Kernel/API/Graphics.h>
|
||||
#include <LibCore/ConfigFile.h>
|
||||
#include <LibCore/DirIterator.h>
|
||||
#include <LibCore/File.h>
|
||||
|
@ -68,7 +69,7 @@ ErrorOr<int> serenity_main(Main::Arguments)
|
|||
WindowServer::ScreenLayout screen_layout;
|
||||
String error_msg;
|
||||
|
||||
auto add_unconfigured_display_connector_devices = [&]() {
|
||||
auto add_unconfigured_display_connector_devices = [&]() -> ErrorOr<void> {
|
||||
// Enumerate the /dev/gpu/connectorX devices and try to set up any ones we find that we haven't already used
|
||||
Core::DirIterator di("/dev/gpu", Core::DirIterator::SkipParentAndBaseDir);
|
||||
while (di.has_next()) {
|
||||
|
@ -78,18 +79,23 @@ ErrorOr<int> serenity_main(Main::Arguments)
|
|||
auto full_path = String::formatted("/dev/gpu/{}", path);
|
||||
if (!Core::File::is_device(full_path))
|
||||
continue;
|
||||
auto display_connector_fd = TRY(Core::System::open(full_path, O_RDWR | O_CLOEXEC));
|
||||
if (int rc = graphics_connector_set_responsible(display_connector_fd); rc != 0)
|
||||
return Error::from_syscall("graphics_connector_set_responsible"sv, rc);
|
||||
TRY(Core::System::close(display_connector_fd));
|
||||
if (fb_devices_configured.find(full_path) != fb_devices_configured.end())
|
||||
continue;
|
||||
if (!screen_layout.try_auto_add_display_connector(full_path))
|
||||
dbgln("Could not auto-add display connector device {} to screen layout", full_path);
|
||||
}
|
||||
return {};
|
||||
};
|
||||
|
||||
auto apply_and_generate_generic_screen_layout = [&]() {
|
||||
auto apply_and_generate_generic_screen_layout = [&]() -> ErrorOr<bool> {
|
||||
screen_layout = {};
|
||||
fb_devices_configured = {};
|
||||
|
||||
add_unconfigured_display_connector_devices();
|
||||
TRY(add_unconfigured_display_connector_devices());
|
||||
if (!WindowServer::Screen::apply_layout(move(screen_layout), error_msg)) {
|
||||
dbgln("Failed to apply generated fallback screen layout: {}", error_msg);
|
||||
return false;
|
||||
|
@ -104,17 +110,15 @@ ErrorOr<int> serenity_main(Main::Arguments)
|
|||
if (screen_info.mode == WindowServer::ScreenLayout::Screen::Mode::Device)
|
||||
fb_devices_configured.set(screen_info.device.value());
|
||||
|
||||
add_unconfigured_display_connector_devices();
|
||||
TRY(add_unconfigured_display_connector_devices());
|
||||
|
||||
if (!WindowServer::Screen::apply_layout(move(screen_layout), error_msg)) {
|
||||
dbgln("Error applying screen layout: {}", error_msg);
|
||||
if (!apply_and_generate_generic_screen_layout())
|
||||
return 1;
|
||||
TRY(apply_and_generate_generic_screen_layout());
|
||||
}
|
||||
} else {
|
||||
dbgln("Error loading screen configuration: {}", error_msg);
|
||||
if (!apply_and_generate_generic_screen_layout())
|
||||
return 1;
|
||||
TRY(apply_and_generate_generic_screen_layout());
|
||||
}
|
||||
}
|
||||
|
||||
|
|
Loading…
Add table
Add a link
Reference in a new issue