* [PATCH] drm/atomic: do not branch based on the value of current->comm[0]
@ 2022-11-05 22:20 ` Jason A. Donenfeld
0 siblings, 0 replies; 12+ messages in thread
From: Jason A. Donenfeld @ 2022-11-05 22:20 UTC (permalink / raw)
To: dri-devel, linux-api, linux-kernel
Cc: Jason A. Donenfeld, Daniel Vetter, Peter Zijlstra, Ilia Mirkin,
Maarten Lankhorst, Nicholas Kazlauskas, Christian Brauner,
Michel Dänzer, Alex Deucher, Adam Jackson,
Greg Kroah-Hartman, Sean Paul, David Airlie, Rob Clark,
Sultan Alsawaf
This reverts 26b1d3b527e7 ("drm/atomic: Take the atomic toys away from
X"), a rootkit-like kludge that has no business being inside of a
general purpose kernel. It's the type of debugging hack I'll use
momentarily but never commit, or a sort of babbies-first-process-hider
malware trick.
The backstory is that some userspace code -- xorg-server -- has a
modesetting DDX that isn't really coded right. With nobody wanting to
maintain X11 anymore, rather than fixing the buggy code, the kernel was
adjusted to avoid having to touch X11. A bummer, but fair enough: if the
kernel doesn't want to support some userspace API any more, the right
thing to do is to arrange for a graceful fallback where userspace thinks
it's not available in a manageable way.
However, the *way* it goes about doing that is just to check
`current->comm[0] == 'X'`, and disable it for only that case. So that
means it's *not* simply a matter of the kernel not wanting to support a
particular userspace API anymore, but rather it's the kernel not wanting
to support xorg-server, in theory, but actually, it turns out, that's
all processes that begin with 'X'.
Playing games with current->comm like this is obviously wrong, and it's
pretty shocking that this ever got committed.
Fortunately, since this was committed, somebody did actually disable
the userspace side by default in X11:
https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/180 and
this was three years ago. So userspace is mostly fine now for ordinary
default usage. And people who opt into this -- since it does actually
work fine for many use cases on i915 -- ostensibly know what they're
getting themselves into (my case).
So let's just revert this `comm[0] == 'X'` business entirely, but still
allow for `value == 2`, in case anybody actually started working on that
part elsewhere.
Fixes: 26b1d3b527e7 ("drm/atomic: Take the atomic toys away from X")
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ilia Mirkin <imirkin@alum.mit.edu>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Michel Dänzer <michel@daenzer.net>
Cc: Alex Deucher <alexdeucher@gmail.com>
Cc: Adam Jackson <ajax@redhat.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Sean Paul <sean@poorly.run>
Cc: David Airlie <airlied@linux.ie>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Sultan Alsawaf <sultan@kerneltoast.com>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
drivers/gpu/drm/drm_ioctl.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index ca2a6e6101dc..017f31e67179 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -336,11 +336,6 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
case DRM_CLIENT_CAP_ATOMIC:
if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
return -EOPNOTSUPP;
- /* The modesetting DDX has a totally broken idea of atomic. */
- if (current->comm[0] == 'X' && req->value == 1) {
- pr_info("broken atomic modeset userspace detected, disabling atomic\n");
- return -EOPNOTSUPP;
- }
if (req->value > 2)
return -EINVAL;
file_priv->atomic = req->value;
--
2.38.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH] drm/atomic: do not branch based on the value of current->comm[0]
@ 2022-11-05 22:20 ` Jason A. Donenfeld
0 siblings, 0 replies; 12+ messages in thread
From: Jason A. Donenfeld @ 2022-11-05 22:20 UTC (permalink / raw)
To: dri-devel, linux-api, linux-kernel
Cc: Jason A. Donenfeld, Peter Zijlstra, Greg Kroah-Hartman,
Michel Dänzer, Christian Brauner, David Airlie,
Daniel Vetter, Sultan Alsawaf, Sean Paul, Nicholas Kazlauskas
This reverts 26b1d3b527e7 ("drm/atomic: Take the atomic toys away from
X"), a rootkit-like kludge that has no business being inside of a
general purpose kernel. It's the type of debugging hack I'll use
momentarily but never commit, or a sort of babbies-first-process-hider
malware trick.
The backstory is that some userspace code -- xorg-server -- has a
modesetting DDX that isn't really coded right. With nobody wanting to
maintain X11 anymore, rather than fixing the buggy code, the kernel was
adjusted to avoid having to touch X11. A bummer, but fair enough: if the
kernel doesn't want to support some userspace API any more, the right
thing to do is to arrange for a graceful fallback where userspace thinks
it's not available in a manageable way.
However, the *way* it goes about doing that is just to check
`current->comm[0] == 'X'`, and disable it for only that case. So that
means it's *not* simply a matter of the kernel not wanting to support a
particular userspace API anymore, but rather it's the kernel not wanting
to support xorg-server, in theory, but actually, it turns out, that's
all processes that begin with 'X'.
Playing games with current->comm like this is obviously wrong, and it's
pretty shocking that this ever got committed.
Fortunately, since this was committed, somebody did actually disable
the userspace side by default in X11:
https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/180 and
this was three years ago. So userspace is mostly fine now for ordinary
default usage. And people who opt into this -- since it does actually
work fine for many use cases on i915 -- ostensibly know what they're
getting themselves into (my case).
So let's just revert this `comm[0] == 'X'` business entirely, but still
allow for `value == 2`, in case anybody actually started working on that
part elsewhere.
Fixes: 26b1d3b527e7 ("drm/atomic: Take the atomic toys away from X")
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ilia Mirkin <imirkin@alum.mit.edu>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Michel Dänzer <michel@daenzer.net>
Cc: Alex Deucher <alexdeucher@gmail.com>
Cc: Adam Jackson <ajax@redhat.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Sean Paul <sean@poorly.run>
Cc: David Airlie <airlied@linux.ie>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Sultan Alsawaf <sultan@kerneltoast.com>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
drivers/gpu/drm/drm_ioctl.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index ca2a6e6101dc..017f31e67179 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -336,11 +336,6 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
case DRM_CLIENT_CAP_ATOMIC:
if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
return -EOPNOTSUPP;
- /* The modesetting DDX has a totally broken idea of atomic. */
- if (current->comm[0] == 'X' && req->value == 1) {
- pr_info("broken atomic modeset userspace detected, disabling atomic\n");
- return -EOPNOTSUPP;
- }
if (req->value > 2)
return -EINVAL;
file_priv->atomic = req->value;
--
2.38.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/atomic: do not branch based on the value of current->comm[0]
2022-11-05 22:20 ` Jason A. Donenfeld
@ 2022-11-16 0:36 ` Jason A. Donenfeld
-1 siblings, 0 replies; 12+ messages in thread
From: Jason A. Donenfeld @ 2022-11-16 0:36 UTC (permalink / raw)
To: dri-devel, linux-api, linux-kernel
Cc: Daniel Vetter, Peter Zijlstra, Ilia Mirkin, Maarten Lankhorst,
Nicholas Kazlauskas, Christian Brauner, Michel Dänzer,
Alex Deucher, Adam Jackson, Greg Kroah-Hartman, Sean Paul,
David Airlie, Rob Clark, Sultan Alsawaf, Linus Torvalds,
Andrew Morton, Kees Cook
Hi David,
I'm a bit surprised that this patch was ignored. I had sort of assumed
the response would be, "good god, I can't believe we're doing this.
Applied to drm-next!" rather than crickets, but maybe it just got buried
under a lot of other patches. So I thought I'd poke about this again.
The original message is reproduced in full below.
Regards,
Jason
On Sat, Nov 05, 2022 at 11:20:12PM +0100, Jason A. Donenfeld wrote:
> This reverts 26b1d3b527e7 ("drm/atomic: Take the atomic toys away from
> X"), a rootkit-like kludge that has no business being inside of a
> general purpose kernel. It's the type of debugging hack I'll use
> momentarily but never commit, or a sort of babbies-first-process-hider
> malware trick.
>
> The backstory is that some userspace code -- xorg-server -- has a
> modesetting DDX that isn't really coded right. With nobody wanting to
> maintain X11 anymore, rather than fixing the buggy code, the kernel was
> adjusted to avoid having to touch X11. A bummer, but fair enough: if the
> kernel doesn't want to support some userspace API any more, the right
> thing to do is to arrange for a graceful fallback where userspace thinks
> it's not available in a manageable way.
>
> However, the *way* it goes about doing that is just to check
> `current->comm[0] == 'X'`, and disable it for only that case. So that
> means it's *not* simply a matter of the kernel not wanting to support a
> particular userspace API anymore, but rather it's the kernel not wanting
> to support xorg-server, in theory, but actually, it turns out, that's
> all processes that begin with 'X'.
>
> Playing games with current->comm like this is obviously wrong, and it's
> pretty shocking that this ever got committed.
>
> Fortunately, since this was committed, somebody did actually disable
> the userspace side by default in X11:
> https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/180 and
> this was three years ago. So userspace is mostly fine now for ordinary
> default usage. And people who opt into this -- since it does actually
> work fine for many use cases on i915 -- ostensibly know what they're
> getting themselves into (my case).
>
> So let's just revert this `comm[0] == 'X'` business entirely, but still
> allow for `value == 2`, in case anybody actually started working on that
> part elsewhere.
>
> Fixes: 26b1d3b527e7 ("drm/atomic: Take the atomic toys away from X")
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ilia Mirkin <imirkin@alum.mit.edu>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
> Cc: Christian Brauner <brauner@kernel.org>
> Cc: Michel Dänzer <michel@daenzer.net>
> Cc: Alex Deucher <alexdeucher@gmail.com>
> Cc: Adam Jackson <ajax@redhat.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Sean Paul <sean@poorly.run>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Sultan Alsawaf <sultan@kerneltoast.com>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
> drivers/gpu/drm/drm_ioctl.c | 5 -----
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index ca2a6e6101dc..017f31e67179 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -336,11 +336,6 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
> case DRM_CLIENT_CAP_ATOMIC:
> if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
> return -EOPNOTSUPP;
> - /* The modesetting DDX has a totally broken idea of atomic. */
> - if (current->comm[0] == 'X' && req->value == 1) {
> - pr_info("broken atomic modeset userspace detected, disabling atomic\n");
> - return -EOPNOTSUPP;
> - }
> if (req->value > 2)
> return -EINVAL;
> file_priv->atomic = req->value;
> --
> 2.38.1
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/atomic: do not branch based on the value of current->comm[0]
@ 2022-11-16 0:36 ` Jason A. Donenfeld
0 siblings, 0 replies; 12+ messages in thread
From: Jason A. Donenfeld @ 2022-11-16 0:36 UTC (permalink / raw)
To: dri-devel, linux-api, linux-kernel
Cc: Christian Brauner, Andrew Morton, Peter Zijlstra,
Greg Kroah-Hartman, Michel Dänzer, Linus Torvalds,
David Airlie, Daniel Vetter, Sultan Alsawaf, Sean Paul,
Nicholas Kazlauskas, Kees Cook
Hi David,
I'm a bit surprised that this patch was ignored. I had sort of assumed
the response would be, "good god, I can't believe we're doing this.
Applied to drm-next!" rather than crickets, but maybe it just got buried
under a lot of other patches. So I thought I'd poke about this again.
The original message is reproduced in full below.
Regards,
Jason
On Sat, Nov 05, 2022 at 11:20:12PM +0100, Jason A. Donenfeld wrote:
> This reverts 26b1d3b527e7 ("drm/atomic: Take the atomic toys away from
> X"), a rootkit-like kludge that has no business being inside of a
> general purpose kernel. It's the type of debugging hack I'll use
> momentarily but never commit, or a sort of babbies-first-process-hider
> malware trick.
>
> The backstory is that some userspace code -- xorg-server -- has a
> modesetting DDX that isn't really coded right. With nobody wanting to
> maintain X11 anymore, rather than fixing the buggy code, the kernel was
> adjusted to avoid having to touch X11. A bummer, but fair enough: if the
> kernel doesn't want to support some userspace API any more, the right
> thing to do is to arrange for a graceful fallback where userspace thinks
> it's not available in a manageable way.
>
> However, the *way* it goes about doing that is just to check
> `current->comm[0] == 'X'`, and disable it for only that case. So that
> means it's *not* simply a matter of the kernel not wanting to support a
> particular userspace API anymore, but rather it's the kernel not wanting
> to support xorg-server, in theory, but actually, it turns out, that's
> all processes that begin with 'X'.
>
> Playing games with current->comm like this is obviously wrong, and it's
> pretty shocking that this ever got committed.
>
> Fortunately, since this was committed, somebody did actually disable
> the userspace side by default in X11:
> https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/180 and
> this was three years ago. So userspace is mostly fine now for ordinary
> default usage. And people who opt into this -- since it does actually
> work fine for many use cases on i915 -- ostensibly know what they're
> getting themselves into (my case).
>
> So let's just revert this `comm[0] == 'X'` business entirely, but still
> allow for `value == 2`, in case anybody actually started working on that
> part elsewhere.
>
> Fixes: 26b1d3b527e7 ("drm/atomic: Take the atomic toys away from X")
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ilia Mirkin <imirkin@alum.mit.edu>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
> Cc: Christian Brauner <brauner@kernel.org>
> Cc: Michel Dänzer <michel@daenzer.net>
> Cc: Alex Deucher <alexdeucher@gmail.com>
> Cc: Adam Jackson <ajax@redhat.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Sean Paul <sean@poorly.run>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Sultan Alsawaf <sultan@kerneltoast.com>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
> drivers/gpu/drm/drm_ioctl.c | 5 -----
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index ca2a6e6101dc..017f31e67179 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -336,11 +336,6 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
> case DRM_CLIENT_CAP_ATOMIC:
> if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
> return -EOPNOTSUPP;
> - /* The modesetting DDX has a totally broken idea of atomic. */
> - if (current->comm[0] == 'X' && req->value == 1) {
> - pr_info("broken atomic modeset userspace detected, disabling atomic\n");
> - return -EOPNOTSUPP;
> - }
> if (req->value > 2)
> return -EINVAL;
> file_priv->atomic = req->value;
> --
> 2.38.1
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/atomic: do not branch based on the value of current->comm[0]
2022-11-16 0:36 ` Jason A. Donenfeld
@ 2022-11-16 0:43 ` Jason A. Donenfeld
-1 siblings, 0 replies; 12+ messages in thread
From: Jason A. Donenfeld @ 2022-11-16 0:43 UTC (permalink / raw)
To: dri-devel, linux-api, linux-kernel
Cc: Daniel Vetter, Peter Zijlstra, Ilia Mirkin, Maarten Lankhorst,
Nicholas Kazlauskas, Christian Brauner, Michel Dänzer,
Alex Deucher, Adam Jackson, Greg Kroah-Hartman, Sean Paul,
David Airlie, Rob Clark, Sultan Alsawaf, Linus Torvalds,
Andrew Morton, Kees Cook
Hey again,
On Wed, Nov 16, 2022 at 01:36:00AM +0100, Jason A. Donenfeld wrote:
> I'm a bit surprised that this patch was ignored. I had sort of assumed
Mystery solved: this message to you bounced from this linux.ie address I
somehow wound up with in the recipients list. Fixing now by using the
one in MAINTAINERS. Sorry about that.
Jason
> On Sat, Nov 05, 2022 at 11:20:12PM +0100, Jason A. Donenfeld wrote:
> > This reverts 26b1d3b527e7 ("drm/atomic: Take the atomic toys away from
> > X"), a rootkit-like kludge that has no business being inside of a
> > general purpose kernel. It's the type of debugging hack I'll use
> > momentarily but never commit, or a sort of babbies-first-process-hider
> > malware trick.
> >
> > The backstory is that some userspace code -- xorg-server -- has a
> > modesetting DDX that isn't really coded right. With nobody wanting to
> > maintain X11 anymore, rather than fixing the buggy code, the kernel was
> > adjusted to avoid having to touch X11. A bummer, but fair enough: if the
> > kernel doesn't want to support some userspace API any more, the right
> > thing to do is to arrange for a graceful fallback where userspace thinks
> > it's not available in a manageable way.
> >
> > However, the *way* it goes about doing that is just to check
> > `current->comm[0] == 'X'`, and disable it for only that case. So that
> > means it's *not* simply a matter of the kernel not wanting to support a
> > particular userspace API anymore, but rather it's the kernel not wanting
> > to support xorg-server, in theory, but actually, it turns out, that's
> > all processes that begin with 'X'.
> >
> > Playing games with current->comm like this is obviously wrong, and it's
> > pretty shocking that this ever got committed.
> >
> > Fortunately, since this was committed, somebody did actually disable
> > the userspace side by default in X11:
> > https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/180 and
> > this was three years ago. So userspace is mostly fine now for ordinary
> > default usage. And people who opt into this -- since it does actually
> > work fine for many use cases on i915 -- ostensibly know what they're
> > getting themselves into (my case).
> >
> > So let's just revert this `comm[0] == 'X'` business entirely, but still
> > allow for `value == 2`, in case anybody actually started working on that
> > part elsewhere.
> >
> > Fixes: 26b1d3b527e7 ("drm/atomic: Take the atomic toys away from X")
> > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Ilia Mirkin <imirkin@alum.mit.edu>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
> > Cc: Christian Brauner <brauner@kernel.org>
> > Cc: Michel Dänzer <michel@daenzer.net>
> > Cc: Alex Deucher <alexdeucher@gmail.com>
> > Cc: Adam Jackson <ajax@redhat.com>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Sean Paul <sean@poorly.run>
> > Cc: David Airlie <airlied@linux.ie>
> > Cc: Rob Clark <robdclark@gmail.com>
> > Cc: Sultan Alsawaf <sultan@kerneltoast.com>
> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> > ---
> > drivers/gpu/drm/drm_ioctl.c | 5 -----
> > 1 file changed, 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> > index ca2a6e6101dc..017f31e67179 100644
> > --- a/drivers/gpu/drm/drm_ioctl.c
> > +++ b/drivers/gpu/drm/drm_ioctl.c
> > @@ -336,11 +336,6 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
> > case DRM_CLIENT_CAP_ATOMIC:
> > if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
> > return -EOPNOTSUPP;
> > - /* The modesetting DDX has a totally broken idea of atomic. */
> > - if (current->comm[0] == 'X' && req->value == 1) {
> > - pr_info("broken atomic modeset userspace detected, disabling atomic\n");
> > - return -EOPNOTSUPP;
> > - }
> > if (req->value > 2)
> > return -EINVAL;
> > file_priv->atomic = req->value;
> > --
> > 2.38.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/atomic: do not branch based on the value of current->comm[0]
@ 2022-11-16 0:43 ` Jason A. Donenfeld
0 siblings, 0 replies; 12+ messages in thread
From: Jason A. Donenfeld @ 2022-11-16 0:43 UTC (permalink / raw)
To: dri-devel, linux-api, linux-kernel
Cc: Christian Brauner, Andrew Morton, Peter Zijlstra,
Greg Kroah-Hartman, Michel Dänzer, Linus Torvalds,
David Airlie, Daniel Vetter, Sultan Alsawaf, Sean Paul,
Nicholas Kazlauskas, Kees Cook
Hey again,
On Wed, Nov 16, 2022 at 01:36:00AM +0100, Jason A. Donenfeld wrote:
> I'm a bit surprised that this patch was ignored. I had sort of assumed
Mystery solved: this message to you bounced from this linux.ie address I
somehow wound up with in the recipients list. Fixing now by using the
one in MAINTAINERS. Sorry about that.
Jason
> On Sat, Nov 05, 2022 at 11:20:12PM +0100, Jason A. Donenfeld wrote:
> > This reverts 26b1d3b527e7 ("drm/atomic: Take the atomic toys away from
> > X"), a rootkit-like kludge that has no business being inside of a
> > general purpose kernel. It's the type of debugging hack I'll use
> > momentarily but never commit, or a sort of babbies-first-process-hider
> > malware trick.
> >
> > The backstory is that some userspace code -- xorg-server -- has a
> > modesetting DDX that isn't really coded right. With nobody wanting to
> > maintain X11 anymore, rather than fixing the buggy code, the kernel was
> > adjusted to avoid having to touch X11. A bummer, but fair enough: if the
> > kernel doesn't want to support some userspace API any more, the right
> > thing to do is to arrange for a graceful fallback where userspace thinks
> > it's not available in a manageable way.
> >
> > However, the *way* it goes about doing that is just to check
> > `current->comm[0] == 'X'`, and disable it for only that case. So that
> > means it's *not* simply a matter of the kernel not wanting to support a
> > particular userspace API anymore, but rather it's the kernel not wanting
> > to support xorg-server, in theory, but actually, it turns out, that's
> > all processes that begin with 'X'.
> >
> > Playing games with current->comm like this is obviously wrong, and it's
> > pretty shocking that this ever got committed.
> >
> > Fortunately, since this was committed, somebody did actually disable
> > the userspace side by default in X11:
> > https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/180 and
> > this was three years ago. So userspace is mostly fine now for ordinary
> > default usage. And people who opt into this -- since it does actually
> > work fine for many use cases on i915 -- ostensibly know what they're
> > getting themselves into (my case).
> >
> > So let's just revert this `comm[0] == 'X'` business entirely, but still
> > allow for `value == 2`, in case anybody actually started working on that
> > part elsewhere.
> >
> > Fixes: 26b1d3b527e7 ("drm/atomic: Take the atomic toys away from X")
> > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Ilia Mirkin <imirkin@alum.mit.edu>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
> > Cc: Christian Brauner <brauner@kernel.org>
> > Cc: Michel Dänzer <michel@daenzer.net>
> > Cc: Alex Deucher <alexdeucher@gmail.com>
> > Cc: Adam Jackson <ajax@redhat.com>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Sean Paul <sean@poorly.run>
> > Cc: David Airlie <airlied@linux.ie>
> > Cc: Rob Clark <robdclark@gmail.com>
> > Cc: Sultan Alsawaf <sultan@kerneltoast.com>
> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> > ---
> > drivers/gpu/drm/drm_ioctl.c | 5 -----
> > 1 file changed, 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> > index ca2a6e6101dc..017f31e67179 100644
> > --- a/drivers/gpu/drm/drm_ioctl.c
> > +++ b/drivers/gpu/drm/drm_ioctl.c
> > @@ -336,11 +336,6 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
> > case DRM_CLIENT_CAP_ATOMIC:
> > if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
> > return -EOPNOTSUPP;
> > - /* The modesetting DDX has a totally broken idea of atomic. */
> > - if (current->comm[0] == 'X' && req->value == 1) {
> > - pr_info("broken atomic modeset userspace detected, disabling atomic\n");
> > - return -EOPNOTSUPP;
> > - }
> > if (req->value > 2)
> > return -EINVAL;
> > file_priv->atomic = req->value;
> > --
> > 2.38.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/atomic: do not branch based on the value of current->comm[0]
2022-11-05 22:20 ` Jason A. Donenfeld
@ 2022-11-16 3:49 ` Dave Airlie
-1 siblings, 0 replies; 12+ messages in thread
From: Dave Airlie @ 2022-11-16 3:49 UTC (permalink / raw)
To: Jason A. Donenfeld, Daniel Vetter
Cc: dri-devel, linux-api, linux-kernel, Peter Zijlstra,
Greg Kroah-Hartman, Michel Dänzer, Christian Brauner,
David Airlie, Daniel Vetter, Sultan Alsawaf, Sean Paul,
Nicholas Kazlauskas
On Sun, 6 Nov 2022 at 08:21, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> This reverts 26b1d3b527e7 ("drm/atomic: Take the atomic toys away from
> X"), a rootkit-like kludge that has no business being inside of a
> general purpose kernel. It's the type of debugging hack I'll use
> momentarily but never commit, or a sort of babbies-first-process-hider
> malware trick.
>
> The backstory is that some userspace code -- xorg-server -- has a
> modesetting DDX that isn't really coded right. With nobody wanting to
> maintain X11 anymore, rather than fixing the buggy code, the kernel was
> adjusted to avoid having to touch X11. A bummer, but fair enough: if the
> kernel doesn't want to support some userspace API any more, the right
> thing to do is to arrange for a graceful fallback where userspace thinks
> it's not available in a manageable way.
>
> However, the *way* it goes about doing that is just to check
> `current->comm[0] == 'X'`, and disable it for only that case. So that
> means it's *not* simply a matter of the kernel not wanting to support a
> particular userspace API anymore, but rather it's the kernel not wanting
> to support xorg-server, in theory, but actually, it turns out, that's
> all processes that begin with 'X'.
>
> Playing games with current->comm like this is obviously wrong, and it's
> pretty shocking that this ever got committed.
I've been ignoring this because I don't really want to reintroduce a
regression for deployed X servers. I don't see the value.
You use a lot of what I'd call overly not backed up language. Why is
it obviously wrong though? Is it "playing games" or is it accessing
the comm to see if the process starts with X.
Do we have lots of userspace processes starting with X that access
this specific piece of the drm modesetting API. I suppose we might and
if we have complaints about that I'd say let's try to fix it better.
Sometimes engineering is hard, It was a big enough problem that a big
enough hammer was used.
I'd hope @Daniel Vetter can comment as well since the original patch was his.
Dave.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/atomic: do not branch based on the value of current->comm[0]
@ 2022-11-16 3:49 ` Dave Airlie
0 siblings, 0 replies; 12+ messages in thread
From: Dave Airlie @ 2022-11-16 3:49 UTC (permalink / raw)
To: Jason A. Donenfeld, Daniel Vetter
Cc: Christian Brauner, Peter Zijlstra, linux-api, Michel Dänzer,
linux-kernel, dri-devel, David Airlie, Greg Kroah-Hartman,
Daniel Vetter, Sultan Alsawaf, Sean Paul, Nicholas Kazlauskas
On Sun, 6 Nov 2022 at 08:21, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> This reverts 26b1d3b527e7 ("drm/atomic: Take the atomic toys away from
> X"), a rootkit-like kludge that has no business being inside of a
> general purpose kernel. It's the type of debugging hack I'll use
> momentarily but never commit, or a sort of babbies-first-process-hider
> malware trick.
>
> The backstory is that some userspace code -- xorg-server -- has a
> modesetting DDX that isn't really coded right. With nobody wanting to
> maintain X11 anymore, rather than fixing the buggy code, the kernel was
> adjusted to avoid having to touch X11. A bummer, but fair enough: if the
> kernel doesn't want to support some userspace API any more, the right
> thing to do is to arrange for a graceful fallback where userspace thinks
> it's not available in a manageable way.
>
> However, the *way* it goes about doing that is just to check
> `current->comm[0] == 'X'`, and disable it for only that case. So that
> means it's *not* simply a matter of the kernel not wanting to support a
> particular userspace API anymore, but rather it's the kernel not wanting
> to support xorg-server, in theory, but actually, it turns out, that's
> all processes that begin with 'X'.
>
> Playing games with current->comm like this is obviously wrong, and it's
> pretty shocking that this ever got committed.
I've been ignoring this because I don't really want to reintroduce a
regression for deployed X servers. I don't see the value.
You use a lot of what I'd call overly not backed up language. Why is
it obviously wrong though? Is it "playing games" or is it accessing
the comm to see if the process starts with X.
Do we have lots of userspace processes starting with X that access
this specific piece of the drm modesetting API. I suppose we might and
if we have complaints about that I'd say let's try to fix it better.
Sometimes engineering is hard, It was a big enough problem that a big
enough hammer was used.
I'd hope @Daniel Vetter can comment as well since the original patch was his.
Dave.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/atomic: do not branch based on the value of current->comm[0]
2022-11-05 22:20 ` Jason A. Donenfeld
@ 2022-11-16 8:48 ` Daniel Abrecht
-1 siblings, 0 replies; 12+ messages in thread
From: Daniel Abrecht @ 2022-11-16 8:48 UTC (permalink / raw)
To: Jason A. Donenfeld, Daniel Vetter
Cc: Christian Brauner, Peter Zijlstra, linux-api, Michel Dänzer,
linux-kernel, dri-devel, David Airlie, Greg Kroah-Hartman,
Daniel Vetter, Sultan Alsawaf, Sean Paul, Nicholas Kazlauskas
Am 2022-11-05 23:20, schrieb Jason A. Donenfeld:
> This reverts 26b1d3b527e7 ("drm/atomic: Take the atomic toys away from
> X")
I'm in favor of reverting this commit. I've tried to get allowing to
enable atomic in Xorg again in there in the past:
https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/533
I've no illusions of getting this through though, after all mostly the
same people control what's merged into Xorg, what drm stuff gets into
the kernel and who disabled it in the kernel in the first place. And
there doesn't seem much interest in dealing with anything Xorg either,
in the merge request I linked, someone even called Xorg "abandonware".
This is also why I didn't respond here until now.
I do see value in enabling this. When I looked at this 2 years ago,
there were situations where enabling atomic brought clear improvements,
and I would expect that it can still improve performance on some special
systems. I think the users should have the option to use it if they want
or need to.
There is also the concern that this may cause a regression, but I would
argue, that there never was a regression to be fixed here in the first
place. There may have been that one broken application in the past, but
it was just that, a broken application, not something broken by the
kernel. I do not think the kernel should modify it's behavior just to
work around bugs in a specific program, which have always existed, and
didn't come from a changer in behavior of the kernel APIs. If a program
was written wrongly, the program should be fixed, and in case of Xorg, I
think it is fixed already.
This probably won't mean much coming from me, but:
Acked-by: Daniel Abrecht <public@danielabrecht.ch>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/atomic: do not branch based on the value of current->comm[0]
@ 2022-11-16 8:48 ` Daniel Abrecht
0 siblings, 0 replies; 12+ messages in thread
From: Daniel Abrecht @ 2022-11-16 8:48 UTC (permalink / raw)
To: Jason A. Donenfeld, Daniel Vetter
Cc: dri-devel, linux-api, linux-kernel, Peter Zijlstra,
Greg Kroah-Hartman, Michel Dänzer, Christian Brauner,
David Airlie, Daniel Vetter, Sultan Alsawaf, Sean Paul,
Nicholas Kazlauskas
Am 2022-11-05 23:20, schrieb Jason A. Donenfeld:
> This reverts 26b1d3b527e7 ("drm/atomic: Take the atomic toys away from
> X")
I'm in favor of reverting this commit. I've tried to get allowing to
enable atomic in Xorg again in there in the past:
https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/533
I've no illusions of getting this through though, after all mostly the
same people control what's merged into Xorg, what drm stuff gets into
the kernel and who disabled it in the kernel in the first place. And
there doesn't seem much interest in dealing with anything Xorg either,
in the merge request I linked, someone even called Xorg "abandonware".
This is also why I didn't respond here until now.
I do see value in enabling this. When I looked at this 2 years ago,
there were situations where enabling atomic brought clear improvements,
and I would expect that it can still improve performance on some special
systems. I think the users should have the option to use it if they want
or need to.
There is also the concern that this may cause a regression, but I would
argue, that there never was a regression to be fixed here in the first
place. There may have been that one broken application in the past, but
it was just that, a broken application, not something broken by the
kernel. I do not think the kernel should modify it's behavior just to
work around bugs in a specific program, which have always existed, and
didn't come from a changer in behavior of the kernel APIs. If a program
was written wrongly, the program should be fixed, and in case of Xorg, I
think it is fixed already.
This probably won't mean much coming from me, but:
Acked-by: Daniel Abrecht <public@danielabrecht.ch>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/atomic: do not branch based on the value of current->comm[0]
2022-11-16 3:49 ` Dave Airlie
@ 2022-11-16 9:39 ` Daniel Vetter
-1 siblings, 0 replies; 12+ messages in thread
From: Daniel Vetter @ 2022-11-16 9:39 UTC (permalink / raw)
To: Dave Airlie
Cc: Jason A. Donenfeld, Daniel Vetter, dri-devel, linux-api,
linux-kernel, Peter Zijlstra, Greg Kroah-Hartman,
Michel Dänzer, Christian Brauner, David Airlie,
Daniel Vetter, Sultan Alsawaf, Sean Paul, Nicholas Kazlauskas
On Wed, Nov 16, 2022 at 01:49:43PM +1000, Dave Airlie wrote:
> On Sun, 6 Nov 2022 at 08:21, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >
> > This reverts 26b1d3b527e7 ("drm/atomic: Take the atomic toys away from
> > X"), a rootkit-like kludge that has no business being inside of a
> > general purpose kernel. It's the type of debugging hack I'll use
> > momentarily but never commit, or a sort of babbies-first-process-hider
> > malware trick.
> >
> > The backstory is that some userspace code -- xorg-server -- has a
> > modesetting DDX that isn't really coded right. With nobody wanting to
> > maintain X11 anymore, rather than fixing the buggy code, the kernel was
> > adjusted to avoid having to touch X11. A bummer, but fair enough: if the
> > kernel doesn't want to support some userspace API any more, the right
> > thing to do is to arrange for a graceful fallback where userspace thinks
> > it's not available in a manageable way.
> >
> > However, the *way* it goes about doing that is just to check
> > `current->comm[0] == 'X'`, and disable it for only that case. So that
> > means it's *not* simply a matter of the kernel not wanting to support a
> > particular userspace API anymore, but rather it's the kernel not wanting
> > to support xorg-server, in theory, but actually, it turns out, that's
> > all processes that begin with 'X'.
> >
> > Playing games with current->comm like this is obviously wrong, and it's
> > pretty shocking that this ever got committed.
>
> I've been ignoring this because I don't really want to reintroduce a
> regression for deployed X servers. I don't see the value.
>
> You use a lot of what I'd call overly not backed up language. Why is
> it obviously wrong though? Is it "playing games" or is it accessing
> the comm to see if the process starts with X.
>
> Do we have lots of userspace processes starting with X that access
> this specific piece of the drm modesetting API. I suppose we might and
> if we have complaints about that I'd say let's try to fix it better.
>
> Sometimes engineering is hard, It was a big enough problem that a big
> enough hammer was used.
>
> I'd hope @Daniel Vetter can comment as well since the original patch was his.
Frankly I refrained from replying when I've seen the patch originally
because I didn't manage to come up with a nice&constructive reply like you
did here. The only thing novel here is the amount of backhanded insults
folded into the commit message.
I very much welcome constructive contributions that actually solve the
problem here, or at least move it forward a bit. This patch is neither.
What might be an option is a tainting module option that disables this
check, since the amount of people willing&able to fix up Xorg is still
zero. But that would need to come with a proper commit message and all
that, and ideally a pile of acks from people who insist they really want
this and need it.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/atomic: do not branch based on the value of current->comm[0]
@ 2022-11-16 9:39 ` Daniel Vetter
0 siblings, 0 replies; 12+ messages in thread
From: Daniel Vetter @ 2022-11-16 9:39 UTC (permalink / raw)
To: Dave Airlie
Cc: Jason A. Donenfeld, Greg Kroah-Hartman, Peter Zijlstra,
Daniel Vetter, Michel Dänzer, linux-kernel, dri-devel,
David Airlie, Christian Brauner, linux-api, Daniel Vetter,
Sultan Alsawaf, Sean Paul, Nicholas Kazlauskas
On Wed, Nov 16, 2022 at 01:49:43PM +1000, Dave Airlie wrote:
> On Sun, 6 Nov 2022 at 08:21, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >
> > This reverts 26b1d3b527e7 ("drm/atomic: Take the atomic toys away from
> > X"), a rootkit-like kludge that has no business being inside of a
> > general purpose kernel. It's the type of debugging hack I'll use
> > momentarily but never commit, or a sort of babbies-first-process-hider
> > malware trick.
> >
> > The backstory is that some userspace code -- xorg-server -- has a
> > modesetting DDX that isn't really coded right. With nobody wanting to
> > maintain X11 anymore, rather than fixing the buggy code, the kernel was
> > adjusted to avoid having to touch X11. A bummer, but fair enough: if the
> > kernel doesn't want to support some userspace API any more, the right
> > thing to do is to arrange for a graceful fallback where userspace thinks
> > it's not available in a manageable way.
> >
> > However, the *way* it goes about doing that is just to check
> > `current->comm[0] == 'X'`, and disable it for only that case. So that
> > means it's *not* simply a matter of the kernel not wanting to support a
> > particular userspace API anymore, but rather it's the kernel not wanting
> > to support xorg-server, in theory, but actually, it turns out, that's
> > all processes that begin with 'X'.
> >
> > Playing games with current->comm like this is obviously wrong, and it's
> > pretty shocking that this ever got committed.
>
> I've been ignoring this because I don't really want to reintroduce a
> regression for deployed X servers. I don't see the value.
>
> You use a lot of what I'd call overly not backed up language. Why is
> it obviously wrong though? Is it "playing games" or is it accessing
> the comm to see if the process starts with X.
>
> Do we have lots of userspace processes starting with X that access
> this specific piece of the drm modesetting API. I suppose we might and
> if we have complaints about that I'd say let's try to fix it better.
>
> Sometimes engineering is hard, It was a big enough problem that a big
> enough hammer was used.
>
> I'd hope @Daniel Vetter can comment as well since the original patch was his.
Frankly I refrained from replying when I've seen the patch originally
because I didn't manage to come up with a nice&constructive reply like you
did here. The only thing novel here is the amount of backhanded insults
folded into the commit message.
I very much welcome constructive contributions that actually solve the
problem here, or at least move it forward a bit. This patch is neither.
What might be an option is a tainting module option that disables this
check, since the amount of people willing&able to fix up Xorg is still
zero. But that would need to come with a proper commit message and all
that, and ideally a pile of acks from people who insist they really want
this and need it.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-11-16 9:40 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-05 22:20 [PATCH] drm/atomic: do not branch based on the value of current->comm[0] Jason A. Donenfeld
2022-11-05 22:20 ` Jason A. Donenfeld
2022-11-16 0:36 ` Jason A. Donenfeld
2022-11-16 0:36 ` Jason A. Donenfeld
2022-11-16 0:43 ` Jason A. Donenfeld
2022-11-16 0:43 ` Jason A. Donenfeld
2022-11-16 3:49 ` Dave Airlie
2022-11-16 3:49 ` Dave Airlie
2022-11-16 9:39 ` Daniel Vetter
2022-11-16 9:39 ` Daniel Vetter
2022-11-16 8:48 ` Daniel Abrecht
2022-11-16 8:48 ` Daniel Abrecht
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.