Adjusting Patch Submission Guidelines

Kevin Cernekee posted some patches to document some changes to the kernel development process. The idea was to reduce the load on maintainers during the merge window. For example, one patch documented the idea that patch submitters should send only urgent patches during the merge window – that is, the time after a new kernel version and before the first -rc release.

There was a very mixed response to Kevin's patches. Christoph Hellwig, for example, pointed out that there was no reason to avoid sending regular patches during a merge window, because maintainers could easily queue up patches to apply after the window had closed. Christoph suggested amending Kevin's documentation to simply say that folks should not expect replies during merge windows and that they should avoid resending patches during that time.

Borislav Petkov, on the other hand, had no issue with Kevin's suggestion and thought it would be fine to encourage developers to send only urgent patches during merge windows. Kalle Valo suggested that http://phb-crystal-ball.org be updated to predict merge windows as well as kernel releases.

Another of Kevin's patches included a diagram to help developers understand when their patches might appear in the kernel:

kernel.org "mainline:"  |  Patch may appear
field when posted       |  in released kernel
------------------------+--------------------
3.18-rc[1-4]            |  3.19
3.18-rc[5-9]            |  3.20
3.18                    |  Merge window open
                        |  -- don't post
3.19-rc[1-4]            |  3.20
3.19-rc[5-9]            |  3.21
3.19                    |  Merge window open
                        |  -- don't post

Alan Cox and Borislav both liked this. Alan remarked, "That's exactly what was needed I think." Linus Torvalds, however, wanted it to be made clear that the diagram represented ideal timings and that, in practice, there could be delays for many reasons. He also wanted it made clear that each maintainer had their own way of doing things that might also affect the timing of patch acceptance.

Linus said, "some maintainers basically have their pull requests for the merge window open *before* the merge window even starts, and for them, the merge window itself isn't actually all that busy, it's often the week before that is the busy one. So the exact timing can vary by maintainership, and while I think the above is a reasonable example, it should perhaps be documented as such. An *example*, not a 'this is how it always works'. If you are preparing a 50-patch monster, I suspect you may want to talk to the maintainer you're planning on spamming before you send it out."

Kevin's final patch addressed the issue of when a developer should consider resubmitting a patch that had not yet been accepted. Specifically, he said, developers should give reviewers enough time to review a patch before sending out a new version. Kevin estimated roughly three to seven calendar days, depending on patch complexity.

Borislav thought that this language should be toughened up a bit, however. He suggested that developers should wait until the reviewers had definitely finished reviewing the code, and that the next version of the patch should incorporate – or at least address – all their suggestions. He said, "You can use the time while waiting to test and hammer more on your code. Any non-trivial submission of a patchset should be resent after a full week (7 days) the earliest in order not to spam people unnecessarily."

Jonathan Corbet responded to the whole group of Kevin's patches, saying, "I worry about trying to codify things too much just because different maintainers have different expectations. As Linus noted, some maintainers have their work done by the time the merge window starts and can take patches just fine – until something catches fire, at least."

Theodore Ts'o agreed that all of Kevin's admonitions would vary on a maintainer-by-maintainer basis. He said that for himself, there were plenty of non-merge-window times when he would be very busy, and that those times would affect his work-flow just as much as the merge window. He also agreed that Kevin's diagram of patch acceptance expectations was at best a very rough guide and could encourage the wrong expectations in developers.

Closing Attack Vectors

Richard Weinberger posted a security patch that occurred to him while studying the offset2lib weakness. The offset2lib weakness is a method of bypassing ASLR (Address-Space Layout Randomization). ASLR is a security measure aimed at attacks that rely on knowing the exact location of a piece of code in memory. If the address space is known to the attackers, they can quickly launch a root shell and do whatever mischief they have in mind. ASLR randomizes addresses so they can't be found and exploited.

The offset2lib weakness exploits a design flaw in ASLR (that has since been fixed). The value of ASLR is in running Position-Independent Executable (PIE) code, which loads its code sections at random locations in memory. By design, however, only the first code section is loaded into a random location – the others are loaded one after the other, in order. So, if any single address is leaked, attackers can gain a map of the full set of code sections, and thus launch their root shell.

The offset2lib weakness may be old news by now, but it still makes interesting study, and it put Richard in mind of a grsecurity feature that addressed a similar problem. He said, "Exploits can brute stack canaries often very easily if the target is a forking server like sshd or Apache httpd. The problem is that after fork( ) the child has by definition exactly the same memory as the parent and therefore also the same stack canaries. The attacker can guess the stack canaries byte by byte. After 256 times 7 fork( ) a good exploit can find the correct canary value."

Richard's patch simply delayed fork() calls by 30 seconds if the child process died due to a fatal error. This would slow attackers down, presumably by so much that it would discourage them from making the attempt.

Kees Cook and Andy Lutomirski both liked the idea but thought it needed to be more configurable; specifically, it needed an on/off option and control over the specific timing delays.

Pavel Machek also at first liked the patch but wanted to make sure it didn't break other tools, such as Trinity and crashme. Richard reported that yes, "If they fork( ) without execve( ) and a child dies very fast the next fork( ) will be throttled." For this reason, he said, he wanted his code to be disabled by default. Richard also mentioned that his code was very stripped down for the initial consideration and could easily incorporate Kees' and Andy's configuration ideas.

After some further discussion, though, Pavel realized he didn't like the whole "adding random delays to the kernel, hoping they slow attacker down" concept that Richard seemed to propose. He suggested that 99 percent of the problem cases could be fixed in a straightforward way within glibc, and that other codebases would follow that lead. He didn't think Richard's patch would be good in the kernel.

There was some further debate that ended inconclusively. Richard, Andy, and Kees felt that delaying fork() calls had to be done in the kernel rather than in glibc; in particular, they didn't agree that Richard's patch simply added random delays – the delays were targeted at specific attacking behaviors to make them less appealing to attackers. Neither side triumphed in the discussion.

Security is a very hairy topic. It's no wonder that debates can get heated. A given attack vector can exploit a bug, or it can exploit the intended design. It can exploit software running on the kernel or the kernel itself. It can exploit a problem that actually exists or a problem that only comes into being when users do something wrong.

In all cases, closing the security hole takes precedence over absolutely any level of user and developer inconvenience. It's conceivable that large, user-visible delays may be required to deal with a particular security issue. The kernel developers themselves can never be sure that a clean and simple fix will be available for a given exploit. They have no choice but to debate the possibilities, look for solutions that seem best, and preserve the user experience as best as they can.

ARM Compatibility

Mark Rutland didn't like that the ARM and ARM64 architectures used different formats for the /proc/cpuinfo file. He said that user applications that were otherwise fully portable would run into problems just on that file. He wanted to fix this in order to re-establish portability.

Specifically, Mark wanted to change the "Features" line of that file in ARM64. Currently, he said, the line "describes the 64-bit hwcaps [Hardware CAPabilities] exclusive of the 32-bit hwcaps, which causes issues for certain applications which attempt to parse /proc/cpuinfo to detect features rather than directly using the hwcaps exposed via auxval [the binary API]."

Russell King confirmed the importance of keeping ARM and ARM64 compatible. He pointed out that "every file in procfs is a userspace API and can be parsed by any program. If we change a procfs file and a userspace program then stops working, that's our fault, and our problem to fix (by restoring the information published there in a manner which userspace can parse.)"

Russell said that because Mark had identified programs that lost compatibility because of the current procfs behavior, the behavior had to be changed to regain compatibility. Of course, reverting that change might break other programs. Russell said:

It's unfortunate that people have decided that parsing the ELF HWCAPs from /proc/cpuinfo is an acceptable way to go, rather than using the binary information passed, but procfs is a much more visible source of information than some binary interface which you need to read man pages to find.

That's the danger of publishing information in either procfs or sysfs. Once published, it becomes part of the userspace API, and it can become hard to remove it. This is why we should /always/ think very carefully about what we expose through these filesystems.

Mark agreed and said that his main concern was how to fix the procfs files without breaking any applications that relied on the incompatible behaviors. He added, "For future architectures, it's probably worth putting stronger guidelines in place to prevent precisely the issues we've hit here."

Russell admonished Mark and the rest of the ARM developers:

You know, I saw something like this happening years ago.

I guess that if people had listened to me when ARM64 was in the process of being released about my concerns about the ARM64 code diverging from the ARM32 code, we wouldn't be having this problem right now, because we would have been aware of the API differences much earlier on.

As ARM64 wants to be compatible with ARM32 (in that it wants to be able to run ARM32 applications), ARM64 *has* to offer a compatible user API for everything that is a user API.

That means you have to generate an ARM32 compatible /proc/cpuinfo, ARM32 compatible hwcap information, ARM32 compatible signal structures, ARM32 compatible everything else. Which means you basically need to have a copy of the ARM32 core code in ARM64, even if you want a different native ARM64 user API.

This is _exactly_ the reason why architectures like X86 decided it was silly having separated 32 and 64-bit, and why they went through a process of merging the two together. A lot of the code was identical, and a lot of the 32-bit specific code was needed for 64-bit to provide the 32-bit API.

Right now, you're finding out this the hard way, and hitting these API problems in the process, and going "oh fsck" when you hit them – quite simply because you've backed yourselves into a corner over this. You have established a different ARM64 API because you didn't want the ARM32 legacy, but then you've found that you /do/ need the ARM32 legacy. Now you're in the position of having to change the ARM64 API, possibly breaking ARM64 applications in the process.

No one responded to Russell's admonition, but Greg Hackmann, Catalin Marinas, and Will Deacon responded to Mark's patch. Overall, they liked it; and, as no one howled about anything breaking, they queued it up for the -next tree.