Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 16 Nov 2022 10:39:41 +0100
From:   Daniel Vetter <daniel@ffwll.ch>
To:     Dave Airlie <airlied@gmail.com>
Cc:     "Jason A. Donenfeld" <Jason@zx2c4.com>,
        Daniel Vetter <daniel.vetter@ffwll.ch>,
        dri-devel@lists.freedesktop.org, linux-api@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Peter Zijlstra <peterz@infradead.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Michel =?iso-8859-1?Q?D=E4nzer?= <michel@daenzer.net>,
        Christian Brauner <brauner@kernel.org>,
        David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel.vetter@intel.com>,
        Sultan Alsawaf <sultan@kerneltoast.com>,
        Sean Paul <sean@poorly.run>,
        Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
Subject: Re: [PATCH] drm/atomic: do not branch based on the value of
 current->comm[0]
Message-ID: <Y3Sv3TgclLH6SD0A@phenom.ffwll.local>
Mail-Followup-To: Dave Airlie <airlied@gmail.com>,
        "Jason A. Donenfeld" <Jason@zx2c4.com>,
        dri-devel@lists.freedesktop.org, linux-api@vger.kernel.org,
        linux-kernel@vger.kernel.org, Peter Zijlstra <peterz@infradead.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Michel =?iso-8859-1?Q?D=E4nzer?= <michel@daenzer.net>,
        Christian Brauner <brauner@kernel.org>,
        David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel.vetter@intel.com>,
        Sultan Alsawaf <sultan@kerneltoast.com>,
        Sean Paul <sean@poorly.run>,
        Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
References: <20221105222012.4226-1-Jason@zx2c4.com>
 <CAPM=9twc_TBtG_654Hm4SV_G1Ar+PiCuZGg1fV-Zooga+4S0GQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAPM=9twc_TBtG_654Hm4SV_G1Ar+PiCuZGg1fV-Zooga+4S0GQ@mail.gmail.com>
Precedence: bulk

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