Linux 5.12's very bad, double ungood day

Posted Mar 8, 2021 17:56 UTC (Mon) by farnz (subscriber, #17727)
In reply to: Linux 5.12's very bad, double ungood day by pbonzini
Parent article: Linux 5.12's very bad, double ungood day

The bug is trivially awful (because it's obvious when you see it, but not when you're working on it); for most of the swap code, the swap location for any given page is referenced as an offset from the beginning of the swap file (or device). For a swap partition, this is fine - the offset from the beginning of the device is the correct location in the swap partition, too. For swap files, the correct offset on the device has to account for where on the device the filesystem has placed swap extents.

In the old code, map_swap_page calls map_swap_entry, which accounts for this offset. In the new code, it calls swap_page_sector, which was previously only ever used for swap partitions, and thus didn't account for the filesystem offset. The fix is to change swap_page_sector to account for swap files, and thus to look at the filesystem-provided extent map. That way, the foot gun is removed for everyone.

Linux 5.12's very bad, double ungood day

Posted Mar 8, 2021 18:10 UTC (Mon) by pbonzini (subscriber, #60935) [Link] (8 responses)

> In the old code, map_swap_page calls map_swap_entry, which accounts for this offset. In the new code, it calls swap_page_sector,

And doing that without pointing it out in the commit message is super sloppy. If you're saying that you're Just Inlining Code, you'd better just inline code

Linux 5.12's very bad, double ungood day

Posted Mar 9, 2021 8:32 UTC (Tue) by epa (subscriber, #39769) [Link] (7 responses)

I dream about a compiler flag that would output a 'logical build' of the code. It would compile to some kind of bytecode where all symbol names have been stripped and all non-recursive function calls have been inlined. Then if the commit message says "renamed variable" or "inlined function" or even "use ?: ternary operator instead of if-else", you could automatically verify that indeed it hasn't changed the bytecode output and so has no effect on the meaning of the code. In other words you can flag commits as pure refactorings and have a machine assistant to verify that, so that human reviewers can concentrate their attention on the patches in the series that change something.

Linux 5.12's very bad, double ungood day

Posted Mar 9, 2021 8:36 UTC (Tue) by epa (subscriber, #39769) [Link]

P.S. I know that in general, it is impossible for a computer program to spot all possible cases where two programs have identical behaviour. I am not suggesting this assistant could have anything like 100% coverage, even for pure refactoring commits. But if it can tick off 80% of them that would still make a reviewer's life easier, and improve my peace of mind as a programmer. (Did that change from a for-loop to a while-loop affect the behaviour? Hmm, I'm not sure... instead of staring at it or working it through with pencil and paper, let's see if the bytecode has changed. If it has changed, the bytecode diff tool will usually point out what I've accidentally altered.)

Linux 5.12's very bad, double ungood day

Posted Mar 9, 2021 19:09 UTC (Tue) by NYKevin (subscriber, #129325) [Link]

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

Posted Mar 9, 2021 21:01 UTC (Tue) by error27 (subscriber, #8346) [Link] (4 responses)

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

Posted Mar 10, 2021 7:23 UTC (Wed) by epa (subscriber, #39769) [Link] (3 responses)

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.

Linux 5.12's very bad, double ungood day

Posted Mar 10, 2021 8:10 UTC (Wed) by error27 (subscriber, #8346) [Link]

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

Posted Mar 20, 2021 23:50 UTC (Sat) by Hi-Angel (guest, #110915) [Link] (1 responses)

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.

Linux 5.12's very bad, double ungood day

Posted Mar 21, 2021 5:34 UTC (Sun) by pabs (subscriber, #43278) [Link]

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