Re: Userspace regression in LTS and stable kernels

[Sasha Levin]

On Fri, Feb 15, 2019 at 10:42:05AM +0100, Michal Hocko wrote:
> On Fri 15-02-19 10:20:13, Greg KH wrote:
> > On Fri, Feb 15, 2019 at 10:10:00AM +0100, Michal Hocko wrote:
> > > On Fri 15-02-19 08:00:22, Greg KH wrote:
> > > > On Thu, Feb 14, 2019 at 12:20:27PM -0800, Andrew Morton wrote:
> > > > > On Thu, 14 Feb 2019 09:56:46 -0800 Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> > > > >
> > > > > > On Wed, Feb 13, 2019 at 3:37 PM Richard Weinberger
> > > > > > <richard.weinberger@xxxxxxxxx> wrote:
> > > > > > >
> > > > > > > Your shebang line exceeds BINPRM_BUF_SIZE.
> > > > > > > Before the said commit the kernel silently truncated the shebang line
> > > > > > > (and corrupted it),
> > > > > > > now it tells the user that the line is too long.
> > > > > >
> > > > > > It doesn't matter if it "corrupted" things by truncating it. All that
> > > > > > matters is "it used to work, now it doesn't"
> > > > > >
> > > > > > Yes, maybe it never *should* have worked. And yes, it's sad that
> > > > > > people apparently had cases that depended on this odd behavior, but
> > > > > > there we are.
> > > > > >
> > > > > > I see that Kees has a patch to fix it up.
> > > > > >
> > > > >
> > > > > Greg, I think we have a problem here.
> > > > >
> > > > > 8099b047ecc431518 ("exec: load_script: don't blindly truncate shebang
> > > > > string") wasn't marked for backporting.  And, presumably as a
> > > > > consequence, Kees's fix "exec: load_script: allow interpreter argument
> > > > > truncation" was not marked for backporting.
> > > > >
> > > > > 8099b047ecc431518 hasn't even appeared in a Linus released kernel, yet
> > > > > it is now present in 4.9.x, 4.14.x, 4.19.x and 4.20.x.
> > > >
> > > > It came in 5.0-rc1, so it fits the "in a Linus released kernel"
> > > > requirement.  If we are to wait until it shows up in a -final, that
> > > > would be months too late for almost all of these types of patches that
> > > > are picked up.
> > >
> > > rc1 is just a too early. Waiting few more rcs or even a final release
> > > for something that people do not see as an issue should be just fine.
> > > Consider this particular patch and tell me why it had to be rushed in
> > > the first place. The original code was broken for _years_ but I do not
> > > remember anybody would be complaining.
> >
> > This patch was in 4.20.10, which was released on Feb 12 while 5.0-rc1
> > came out on Jan 6.  Over a month delay.
>
> Obviously not long enough.

You're assuming that if we wouldn't have taken this patch to stable
somehow someone else would notice this bug and fix it.

What test do we have that would catch this? Which testsuite tests for
long shebang lines? Where is the test added together with this patch
that covers this and similar cases?

The fact is that many patches are not tested until they get to stable,
whether we add them the same week they went upstream or months later.
This is a great case for this: I doubt anyone but NixOS does this crazy
thing with shebang lines, so who else would discover the bug?

If this is indeed a case of us jumping the gun and shipping stuff too
early before all tests are complete, please point me to the test that we
missed and I'll make sure that for any future kernel release it gets run
before we ship a stable kernel.

> > > > > I don't know if Oleg considered backporting that patch.  I certainly
> > > > > did (I always do), and I decided against doing so.  Yet there it is.
> > > >
> > > > This came in through Sasha's tools, which give people a week or so to
> > > > say "hey, this isn't a stable patch!" and it seems everyone ignored that
> > > > :(
> > >
> > > I thought we were through this already. Automagic autoselection of
> > > patches in the core kernel (or mmotm tree patches in particular) is too
> > > dangerous. We try hard to consider each and every patch for stable. Even
> > > if something slips through then it is much more preferred to ask for a
> > > stable backport in the respective email thread and wait for a conclusion
> > > before adding it.
> >
> > We have a list of blacklisted files/subsystems for people that do not
> > want this to happen to their area of the kernel.  The patch seemed to
> > make sense, and it passed all known tests that we currently have.
>
> Yes, the patch makes sense (I wouldn't give my acked-by otherwise). But
> this is one of the area where things that make sense might still break
> because it is hard to assume what userspace depends on.

Great, so the solution is to just not take these things into stable at
all? The solution should be to add tests to the patches that go in there
to verify their correctness and that they don't regress in the future.

If you're really concerned about subsystems being brittle the solution
is to improve their testing rather push stuff in and hope nothing
explodes.

On one hand you Ack it saying it looks great to you and should be
merged, but on the other hand you're saying that you don't really trust
the patch?

Really, if I wouldn't pick this patch now what do you think would have
happened? It would just pop up in a few months as we roll our stable
kernel forward.

> > Sometimes things will slip through like this, it happens.  And really, a
> > 3 day turn-around-time to resolve this is pretty good, don't you think?
>
> Yes, but that doesn't make any difference on the fact that this was not
> marked for stable and I still think this is not a stable material - at
> least not at this moment.

Hindsight is 20/20 :)

If people were good at understanding the impact and implications their
patch has on the kernel we would never introduce new bugs!

I'll happily list a bunch more patches where folks didn't think they're
stable material, but turned out to be important fixes.

--
Thanks,
Sasha