Re: [PATCH] drm/atomic: do not branch based on the value of current->comm[0]

["Jason A. Donenfeld" <Jason@zx2c4.com>]

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
>