LWN: Comments on "Linux 5.12's very bad, double ungood day"

Linux 5.12's very bad, double ungood day

pizza — Sun, 02 May 2021 02:58:13 +0000

> Not that it will stop folks complaining when "5.32-alpha0-rc4-pre3" fails to boot on their production system, obviously because it should have been tested first, and we need a pre-pre-pre-pre-pre release snapshot to start testing against.

I saw this scroll by when I upgraded this system to Fedora 34:

$ rpm -q icedtea-web
icedtea-web-2.0.0-pre.0.3.alpha16.patched1.fc34.3.x86_64

Linux 5.12's very bad, double ungood day

pabs — Sun, 21 Mar 2021 05:34:33 +0000

While GNU ed is actively developed, it seems unlikely it will ever support LSP :)

Linux 5.12's very bad, double ungood day

Hi-Angel — Sat, 20 Mar 2021 23:50:23 +0000

For C these days part of such refactoring could be done with LSP-servers, like clangd for example. Thankfully, all actively developed editors and IDEs support LSP servers, whether natively or through a plugin.

Automated tests

thumperward — Thu, 11 Mar 2021 19:56:10 +0000

For an integration test that would specifically have caught this issue, sure. But given the assumption that swap_page_sector() could be called on a swap file, a unit test that called swap_page_sector() on a swap file with a given input and verify that the file contained the right bytes in the right order afterwards is something that could well have existed and caught said bug before the refectoring inadvertently exposed it.

Linux 5.12's very bad, double ungood day

mathstuf — Thu, 11 Mar 2021 16:04:54 +0000

Another instance I saw today was that a version of the patch that went in was never on the list (a whitespace fix). While such a tiny change is probably OK in practice, I vastly prefer having a bot attached to the service that stashes away every version of the patchset for posterity.

Linux 5.12's very bad, double ungood day

pbonzini — Thu, 11 Mar 2021 08:43:24 +0000

You weren't supposed to do that in production though. Also, answering "sorry I can't" is perfectly valid. :)

Linux 5.12's very bad, double ungood day

Wol — Wed, 10 Mar 2021 23:20:50 +0000

> I don’t think I’ve built a kernel in 10 years, or maybe that one time 7-8 years ago.

You clearly don't run gentoo :-)

Cheers,
Wol

Linux 5.12's very bad, double ungood day

roc — Wed, 10 Mar 2021 22:22:13 +0000

In the past, when reporting a bug in a release kernel, people have asked me to install some random kernel revision to see if the bug is still present with it. If we should never do that, people should stop asking for it.

Hibernation

corbet — Wed, 10 Mar 2021 22:18:09 +0000

That work, and the use case behind it, were discussed in this OSPM article from last May.

Automated tests

sjj — Wed, 10 Mar 2021 22:12:01 +0000

Interesting, I never thought about hibernation in AWS. I haven’t thought about hibernation in years, since it was the unreliable thing you had to do on laptops of the day.

Curious what the use case for it in AWS is.

Linux 5.12's very bad, double ungood day

sjj — Wed, 10 Mar 2021 21:58:19 +0000

How many people run non-distro kernels these days, especially in production? If you do that with an rc kernel, you certainly deserve whatever pieces are left of your data.

I don’t think I’ve built a kernel in 10 years, or maybe that one time 7-8 years ago.

Linux 5.12's very bad, double ungood day

error27 — Wed, 10 Mar 2021 08:10:24 +0000

Yeah. If you make your patch with Coccinelle then we normally encourage you to post the script (unless it's one of the ones that ship with the kernel).

Linux 5.12's very bad, double ungood day

epa — Wed, 10 Mar 2021 07:23:40 +0000

Another approach is when you use an automated tool to make the change in the first place. I mostly develop in C# and I use the proprietary tool Resharper to rename variables, inline methods, convert for-loops to foreach-loops, and a few more exotic transformations like "extract method" (take a chunk of code and move it to its own method, passing in and out the variables it uses). In this case the commit message could include full details of the transformation you applied, in machine-readable format. Then to verify the patch series you run the same transformation and check the output code is the same. (That would be an additional verification step; it checks you just ran the "convert for to foreach" automated refactoring and didn't accidentally introduce other changes, but it doesn't check that the refactoring tool itself is correct. So some kind of bytecode check would also be useful.)

Such a machine-readable record of the refactoring change would also be handy when rebasing. Instead of hitting a merge conflict on the 'renamed variable' commit, you could reapply that change using the refactoring tool. As long as both the old commit and the new one are pure refactorings (logical bytecode doesn't change), this can be done automatically as part of the rebase process, leaving the human programmer to concentrate on the conflicts that aren't trivial.

Automated tests

marcH — Wed, 10 Mar 2021 06:42:43 +0000

> You'd need to set up a machine with a swap file,

As this is an apparently common configuration, you'd expect anyone modifying swap code to grant that configuration some reasonable amount of test time.

> drive it into memory pressure with a lot of dirty anonymous pages,

Also called "swapping"?

> then somehow verify that none of the swap traffic went astray.

Unless you're re-installing your entire system every few tests, there's a good chance you will soon notice something somewhere has gone terribly wrong even when not verifying every byte on the disk. This is apparently how the bug was found and relatively quickly by people not even testing swap but other things. The perfect that does not get done is the enemy of the good that does and this is especially true with chronically underdeveloped validation.

Automated tests

Cyberax — Wed, 10 Mar 2021 02:52:00 +0000

> One could certainly write an automated test to catch this, but it would not be easy.
I was one of people responsible for getting EC2 instances to hibernate. We used files for hibernation and actually found that the kernel had been broken for YEARS with file hibernation (it required a reboot for the hibernation target setting to take effect).

We also had a test for this very issue. It created an EC2 instance with a small disk (~2Gb) and limited RAM (512Mb). The test program created a swap file and then filled the disk to capacity with pseudo-random numbers (by creating a file and writing to it). It then allocated enough pseudo-random data to swap out at least some of it.

Then hibernate, thaw, and checksum the disk and the data in RAM to check for corruption.

The tests ran in about 2 minutes.

Automated tests

roc — Wed, 10 Mar 2021 02:49:39 +0000

You could make it run pretty fast by having the test generate a virtual machine image that is just big enough, i.e. a minimal amount of memory and a minimal-sized block device. Lots of tests could potentially benefit from this.

You'd have to write block device verification code to check the free space and the contents of all files, but that code could be useful for detecting all kinds of bugs.

One thing about automated testing is that once you bite the bullet and start creating infrastructure for things that look hard to test, you make it easier to test all kinds of things and people are much more willing to write tests for all kinds of things as part of their normal development. So the sooner you create such infrastructure, the better.

Linux 5.12's very bad, double ungood day

roc — Wed, 10 Mar 2021 02:43:35 +0000

Many projects run tests on every PR before merging. It's difficult to get this to scale, but not impossible.

Linux 5.12's very bad, double ungood day

roc — Wed, 10 Mar 2021 02:34:53 +0000

Projects that are serious about testing run all their automated tests as often as they can. The sooner you detect a regression the easier it is to bisect, debug, and fix, with less impact on other developers and users.

In practice, large projects often try to maximise bang-for-the-buck by dividing tests into tiers, e.g. tier 1 tests run on every push, tier 2 every day, maybe a tier 3 that runs less often. Many projects use heuristics or machine learning to choose which tests to run in each run of tier 1.

Yes, I understand that it's difficult to thoroughly test weird hardware and configuration combinations. Ideally organizations that produce hardware with Linux support would contribute testing on that hardware. But even if we ignore all those bugs, there are still lots of core kernel bugs not being caught by kernel CI.

Linux 5.12's very bad, double ungood day

NYKevin — Wed, 10 Mar 2021 01:47:39 +0000

This is (hypothetically) a test. It doesn't need to be a reasonable FS in the first place. Tell the swapfile code that the file begins at such-and-such offset and occupies such-and-such size, and fill the rest of the image with 0xDEADBEEF or whatever sentinel value you like. I'm sure they could make an extremely stupid filesystem that works that way internally (and just returns EROFS or whatever if the user tries to create files or do other things it doesn't like), so you don't even need to properly mock out any of the FS code.

Linux 5.12's very bad, double ungood day

dbnichol — Tue, 09 Mar 2021 22:44:33 +0000

Why not test linus master HEAD prior to tagging? It's not like he pulls all the requests and tags the release in one giant push. Testing a tagged release makes sense as that's what people are going to use, but a CI system could test HEAD constantly. I think that's what basically every other project does - test what's on the branch before you tag it.

Automated tests

corbet — Tue, 09 Mar 2021 21:33:49 +0000

One could certainly write an automated test to catch this, but it would not be easy. You'd need to set up a machine with a swap file, drive it into memory pressure with a lot of dirty anonymous pages, then somehow verify that none of the swap traffic went astray. That means comparing the entire block device (including free space) with what you expect it to be, or mapping out the swap file, picking the swap traffic out of a blktrace stream, and ensuring that each page goes to the right place.

Certainly doable, but this would not be a fast-running test.

Linux 5.12's very bad, double ungood day

marcH — Tue, 09 Mar 2021 21:11:28 +0000

> I was a bit disappointed that the (otherwise excellent as usual) article doesn't discuss the possibility of automated tests catching buggy changes like this before they are merged anywhere. The kernel is behind current best practices there.

I think the gap in the article hides a much more worrying gap. There's a long and interesting discussion in the middle of the comments about test setups and workflows with not a single link to .... test code. Is there any?

Linux 5.12's very bad, double ungood day

error27 — Tue, 09 Mar 2021 21:01:49 +0000

I use my rename_rev.pl script to review renamed the variable patches.

https://github.com/error27/rename_rev

It also has a -r NULL mode to review changes like "if (foo != NULL) {" --> "if (foo) {" because sometimes people get those transitions inverted. A bunch of little tricks like that.

I sometimes think like you do about ways to do something similar at the bytecode level or with static analysis but I can't think how that would work...

Linux 5.12's very bad, double ungood day

marcH — Tue, 09 Mar 2021 20:29:33 +0000

> Unfortunately patchwork is disconnected from the target repository. That's why Gerrit is better, for example.

patchwork tries to find code submissions not addressed to it directly. All other code review solutions require everything to be sent to them directly, they act as gateways. That's the very simple reason why they have all the information and all work better.

The icing on the cake is of course recipient-controlled email notifications instead of sender-control notifications (a.k.a... "spam"). In other words people routinely subscribe and unsubscribe to/from individual code reviews; not possible with a crude mailing list.

None of this is rocket science, it's all very easy to see and understand except when obsessing about the "email versus web" user interface questions; these debates are not "wrong" but they hide the much more important backend and database design issues.

You could totally have a gateway type code review tool entirely driven by email. In fact requesting all submissions, review comments and approvals or rejections to be sent (by email) to patchwork directly and putting patchwork in better control of the git repo would get at least half-way there.

Linux 5.12's very bad, double ungood day

mathstuf — Tue, 09 Mar 2021 19:55:11 +0000

Crazy idea: require kernel.testing.alpha=5.20.0-alpha1 on the cmdline to boot such an alpha kernel. Reject such an option in non-alpha kernels (including development kernels; the tagged release would require this code and not otherwise).

But this kind of one-off code is annoying to test itself and someone will script adding it to their boot command lines anyways.

Linux 5.12's very bad, double ungood day

NYKevin — Tue, 09 Mar 2021 19:09:43 +0000

I tend to imagine you can get 90% of the way there with gcc -S -O0 -finline-functions -findirect-inlining and some combination of the max-inline tuning parameters (whose documentation I'm finding a little hard to follow). But that won't cross translation units because it doesn't link. You'd need some kind of LTO optionally followed by a disassembly step to get the other 10%.

Linux 5.12's very bad, double ungood day

Wol — Tue, 09 Mar 2021 17:47:19 +0000

And how many people will ignore that (or be unaware) and load alphaN on their production server anyway?

Horse to water and all that ...

Cheers,
Wol

Linux 5.12's very bad, double ungood day

daenzer — Tue, 09 Mar 2021 17:32:36 +0000

Yeah, user-mode-linux should have sufficed in this case.

And there are CI pipelines on https://gitlab.freedesktop.org/ running VMs via KVM, which should allow booting kernels built as part of the pipeline as well. (Another possibility is having dedicated test machines which can be powered on/off remotely; Mesa is using a number of those for testing GPU drivers)

Linux 5.12's very bad, double ungood day

mathstuf — Tue, 09 Mar 2021 16:43:07 +0000

Here's an idea:

- initialize a swapfile
- do things with it
- copy it to a new, fresh FS
- restore from it

That at least gets the "everything was written to the swapfile" (rather than willy-nilly across the hosting FS). Doesn't guarantee that *extra* writes didn't go out. Some form of writing a given bitpattern to the entire FS and then ensuring that post-FS init and swap file creation, nothing else changes (modulo mtime/inode updates) might suffice? I have no idea if such a "simple" FS exists though. Or create a single file which takes up the remaining FS space with a given bitpattern reads properly after using the swapfile. Wrecking any data or metadata would presumably be detectable then, no?

Linux 5.12's very bad, double ungood day

andy_shev — Tue, 09 Mar 2021 16:12:33 +0000

Unfortunately patchwork is disconnected from the target repository. That's why Gerrit is better, for example.

Linux 5.12's very bad, double ungood day

pabs — Tue, 09 Mar 2021 16:12:01 +0000

You could run user-mode-linux, or possibly kvm if the platform has nested VMs.

Linux 5.12's very bad, double ungood day

pizza — Tue, 09 Mar 2021 15:35:01 +0000

> That's why we linux-next, which is used for lots of automated integration tests
> Three weeks passed between the buggy commit entering linux-next and upstream.

So the "problem" here isn't that nothing was being tested, it's just that none of the tests run during this interval window caught this particular issue. It's also not clear that there was even a test out there that could have caught this, except by pure happenstance.

But that's the reality of software work; a bug turns up, write a test to catch it (and hopefully others of the same class), add it to the test suite (which runs as often as your available resources allow) .... and repeat endlessly.

Linux 5.12's very bad, double ungood day

Cyberax — Tue, 09 Mar 2021 15:26:52 +0000

Kinda. You'll have alphaN that is released before rcN, and the distinction is that alphaN is meant only for automatic testing on throwaway hardware.

Linux 5.12's very bad, double ungood day

pizza — Tue, 09 Mar 2021 15:25:15 +0000

... okay, so rename "rc1" to "alpha1" , "rc2" to "alpha2" and so forth. Problem solved?

Not that it will stop folks complaining when "5.32-alpha0-rc4-pre3" fails to boot on their production system, obviously because it should have been tested first, and we need a pre-pre-pre-pre-pre release snapshot to start testing against.

Linux 5.12's very bad, double ungood day

geert — Tue, 09 Mar 2021 15:22:49 +0000

That's why we linux-next, which is used for lots of automated integration tests

$ git tag --contains 48d15436fde6
next-20210128
next-20210129
next-20210201
next-20210202
next-20210203
next-20210204
next-20210205
next-20210208
next-20210209
next-20210210
next-20210211
next-20210212
next-20210215
next-20210216
next-20210217
next-20210218
next-20210219
next-20210222
next-20210223
next-20210224
next-20210225
next-20210226
next-20210301
next-20210302
next-20210303
next-20210304
next-20210305
next-20210309
v5.12-rc1
v5.12-rc1-dontuse
v5.12-rc2

Three weeks passed between the buggy commit entering linux-next and upstream.

Linux 5.12's very bad, double ungood day

Cyberax — Tue, 09 Mar 2021 15:12:28 +0000

Linus can issue a pre-RC (alpha1?) to give time to run the tests, a day before the actual RC.

Linux 5.12's very bad, double ungood day

pbonzini — Tue, 09 Mar 2021 14:54:48 +0000

There's already CKI, KernelCI and more. However testing kernels is not that easy. For example for obvious reasons you cannot install a kernel from a bog standard gitlab pipeline.

Linux 5.12's very bad, double ungood day

daenzer — Tue, 09 Mar 2021 14:29:23 +0000

Like other commenters, I was a bit disappointed that the (otherwise excellent as usual) article doesn't discuss the possibility of automated tests catching buggy changes like this before they are merged anywhere. The kernel is behind current best practices there.

E.g. it shouldn't be hard to hook up tests to a GitLab CI pipeline which can catch this bug (and more), and only allow merging changes which pass the tests.

Linux 5.12's very bad, double ungood day

epa — Tue, 09 Mar 2021 14:11:16 +0000

I guess if the test setup uses a checksummed filesystem then you verify the filesystem after the tests have completed and if it's corrupted, that's a failure. If the filesystem doesn't have per-file checksums, you can at least do the usual fsck stuff to check metadata. (For testing it would be handy to have a filesystem mode that always zeroes out unused pages, so that a thorough fsck can later check that all unused pages are zero.)

Linux 5.12's very bad, double ungood day

pizza — Tue, 09 Mar 2021 13:27:06 +0000

> Well then, like I said above: tests need to be run before the RC releases.

Okay, so... when exactly?

There are 10K commits (give or take a couple thousand) that land in every -rc1. Indeed, until -rc1 lands, nobody can really be sure if a given pull request (or even a specific patch) will get accepted. This is why nearly all upstream tooling treats "-rc1" as the "time to start looking for regressions" inflection point [1], and they spend the next *two months* fixing whatever comes up. This has been the established process for over a decade now.

So what if there was a (nasty) bug that takes down a test rig? That's what the test rigs are for! The only thing unusual about this bug is that it leads to silent corruption, to the point where "testing" in of itself wasn't enough; the test would have had to been robust enough to ensure nothing unexpected was written anywhere to the disk. That's a deceptively hairy testing scenario, arguably going well beyond the tests folks developing filesystems run.

Note I'm not making excuses here; it is a nasty bug and clearly the tests that its developers ran was insufficient. But it is ridiculous to expect "release-quality" regression testing to be completed at the start of the designated testing period.

[1] Indeed, many regressions are due to the combinations of unrelated changes in a given -rc1; each of those 10K patches in of themselves is fine, but (eg) patch #3313 could lead to data loss, but only in combination of a specific kernel option being enabled, and run on a system containing an old 3Ware RAID controller and a specific motherboard with a PCI-X bridge that can't pass through MSI interrupts due to how it was physically wired up. [2] [3]

[2] It's sitting about four feet away from me as I type this.

[3] Kernel bugzilla #43074