Date:   Wed, 23 Sep 2020 20:57:38 +0200
From:   Daniel Vetter <daniel.vetter@...ll.ch>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Pavel Machek <pavel@...x.de>, Willy Tarreau <w@....eu>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>, jikos@...e.cz,
        vojtech@...e.cz,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        stable <stable@...r.kernel.org>,
        Yuan Ming <yuanmingbuaa@...il.com>,
        Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>
Subject: Re: [PATCH 4.19 66/78] fbcon: remove soft scrollback code

On Wed, Sep 23, 2020 at 8:19 PM Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
>
> On Wed, Sep 23, 2020 at 1:44 AM Pavel Machek <pavel@...x.de> wrote:
> >
> > >
> > >   https://www.openwall.com/lists/oss-security/2020/09/15/2
> >
> > Thank you for the pointer!
>
> Note that for me to be willing to take the softscollback code back, it
> really would have to be more than just a revert and a trivial fix.
>
> It would have to be more along the lines of "this is simplified and
> verified". For example, at a minimum all the VT_RESIZE etc stuff would
> have to clearly and obviously reset the soft-scrollback, so that we
> simply can't have those kinds of issues.
>
> If you look at that commit 50145474f6ef ("fbcon: remove soft
> scrollback code") that removes the code, most of that code really
> doesn't make much sense.
>
> I dare anybody looking at that removed fbcon_redraw_softback()
> function (or the different cases in fbcon_scrolldelta(), for that
> matter) to claim they understand what it is doing. It's very odd code,
> and it handles a lot of odd special cases.
>
> So I wouldn't mind re-introducing the scrollback code, but the
> reintroduced version needs to be simple and fairly _obvious_.  It
> would need to not have all those odd special cases, and it would need
> to clearly not have any subtle issues with things like font or screen
> resizing.
>
> I'd also suggest that it not even try to handle the multiple virtual
> console case.
>
> And yes, some of it may involve also clarifying the code in the VT
> code itself, so that the interactions with the cursor and VT switching
> is more obvious. Maybe instead of trying to deal with a SW cursor when
> scrollback is active, just hide the cursor entirely before doing
> scrollback. And make change_console (and all the resizing) explicitly
> reset scrollback etc.
>
> So the reason the code got removed was that it was just very grotty
> and hasn't really been maintained for over a decade.
>
> In order to resurrect it we'd not just have to have a maintainer, the
> whole "it's grotty and incomprehensible and has these nasty
> interactions with random other things" would need to be addressed too.

fbcon has the additional issue that the locking is completely busted,
because fbcon is both accessed through vt, protected by console_lock,
and through the fbdev chardev, protected by the fb_info lock. Ofc both
sides call into the other side, so atm a few operations are just not
protected by locks to avoid deadlock issues. I ripped out the notifier
in the middle that was angering lockdep completely, so not it's at
least fixable. But this still needs someone to sit down and come up
with something that works.

Locking is one, but in general there's all kinds of resizing fun when
you switch modes through the fbdev chardev. Not looking forward to
when syzbot figures out how to race fbcon/vt against fbdev. It's all a
rather big mess.

Oh and add in hotunplug for additional fun.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Powered by blists - more mailing lists