Message-ID: <Y3Sv3TgclLH6SD0A@phenom.ffwll.local>
Date:   Wed, 16 Nov 2022 10:39:41 +0100
From:   Daniel Vetter <daniel@...ll.ch>
To:     Dave Airlie <airlied@...il.com>
Cc:     "Jason A. Donenfeld" <Jason@...c4.com>,
        Daniel Vetter <daniel.vetter@...ll.ch>,
        dri-devel@...ts.freedesktop.org, linux-api@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Peter Zijlstra <peterz@...radead.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Michel Dänzer <michel@...nzer.net>,
        Christian Brauner <brauner@...nel.org>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel.vetter@...el.com>,
        Sultan Alsawaf <sultan@...neltoast.com>,
        Sean Paul <sean@...rly.run>,
        Nicholas Kazlauskas <nicholas.kazlauskas@....com>
Subject: Re: [PATCH] drm/atomic: do not branch based on the value of
 current->comm[0]

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@...c4.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

Powered by blists - more mailing lists