Hacker News Reader

Comment by mdaniel | Hacker News Reader

While reading over that kernel code, I kept wondering how it could possibly be code reviewed before landing. The amount of context required to spot mistakes must be overwhelming

If there was ever a case for Literate Programming, I'd vote for using it on kernel code before I would on rando CRUD app

View on HN

People think that because what they are doing is arcane or complicated that it then has the right for the code to be arcane and complicated.

This is not a local effect. Many compilers have more use of goto than you might expect - because goto is acceptable in some parts of a compiler, it becomes less unacceptable in parts where it should remain so.

It doesn't help that C is an awful programming language, which actively encourages repetition and the expansion of the surface area from which bugs can occur.

KVM is stable, small, and well fuzzed. I'm not super surprised this CVE is in the nested support since it's complicated and in flux. Last time I ran CLOC on x86 KVM it was only ~50k lines, but that was years ago.

The amount of context necessary for virt is sort of silly. IMO most of the complexity is not from the code style (though kernel code style doesn't help). It's mostly that reading KVM requires thorough context on the underlying architecture, and its virtualization features.

That said, I'd really like to see parts of KVM replaced with rust code (particularly the x86 instruction emulator). Shoving it into a safe language would give me some extra faith.

It makes you wonder what else is lurking in there, yet to be discovered.

Agree on your concern around code review. It's even worse than that, though, because it seems that the same internalized context that allows you to see mistakes also allows your brain to buy in to the same assumptions that allowed the bug to be written in the first place.

I spent most of my career in infosec. When I write code I try to follow most of the practices I've been preaching, but I certainly don't make fewer mistakes.

Huge difference between knowing better and doing better.

Remember too that the majority of the linux kernel isn't covered by any unit or integration tests.

A unit test isn't going to spot a bug like this, nor would a pre-written integration test. The kernel is extensively fuzzed.

What methods/techniques/processes would have caught this? What's the most cost effective of those? (Are those even the right questions?)

Formal verification assuming you can even specify the problem well enough.

Fuzzing is probably the most likely, but even then..

That was a really good read. And nothing that something like Rust would solve. Just knowing that userspace with access to more than one core could outrace the kernel between the validation checks and the action.

I believe that Rust actually would allow you to protect against this sort of thing if you integrated the low-level API with rust’s ownership/linearity system. Most rust APIs for embedded SOCs (“HALs”) do this correctly.

In the example in the OP, ownership rules were violated as there were two direct mutable handles to an object.

Trying to impose Rust-like “only one mutable reference at a time” semantics on a VM host’s management of guest memory is somewhere between a performance disaster and a complete nonstarter. We’re talking about TOCTOU where the writer is a vCPU. There are two big problems with Rust-like semantics:

1. Revoking access from other vCPUs is very expensive. It would be a huge performance loss to freeze all vCPUs or TLB-shootdown them just to read guest memory for emulation.

2. Multiple vCPUs at once fundamentally mutate the same memory. Shared-memory ISAs aren’t Rust.

What could be done is to have all access to guest memory be unsafe and to strongly encourage reading the whole block at once. This might be tricky to make work cleanly efficiently.

I think the way you’d want to implement it would be to only allow atomic copies (no aliasing) of the configuration blocks. There might be a way to do it efficiently without just pausing all the virtual cores. For example, you could blow out the memory mapping to that block so that it would trigger a page fault if and only if the guest tried to access that block, and you could make the page fault handler block until the supervisor was done copying. Or something along those lines.

Blowing away the memory mapping is a rather expensive operation. It’s also unclear why an atomic copy is useful. To prevent TOCTOU, you also need to prevent two consecutive atomic copies that inadvertently assume they get the same result each time.

I've done something really similar (albeit in modern C++) that created a type safe guard around DMAed structures to provide mutual exclusion between the device and the driver (and to handle the appropriate cache management as the buffer transitions on non coherent systems, but that part isn't really applicable to this article).

How do kernels written in {C, Rust} handle that userspace code can create UB situations in kernel code, and prevent userspace memory unsafety from causing the kernel to malfunction?

Typically by simply not having important data structures like that in untrusted memory. You're either dealing with raw, fairly untyped buffers, or structs that get marshalled into kernel space from user space before even being validated. I did hear about some similar TOCTOU bugs though with seccomp filtering, that then required a fix to move the marshalling to happen before the BPF filter was run.

True, although this kind of reasoning against Rust, or any other memory safe systems programming language for that matter, always misses the point.

Seatbelts, protected jackets, helmets also don't prevent death in all accidents, yet plenty of people appreciate still being around by having weared them whent it came to be.

Wonder no more: hardly anything in Linux has ever been meaningfully reviewed. Code looks like it couldn't possibly pass review because it can't, and didn't.

> There is, instead, a somewhat involved (if somewhat informal) process designed to ensure that each patch is reviewed for quality and that each patch implements a change which is desirable to have in the mainline.

- https://www.kernel.org/doc/html/latest/process/2.Process.htm...

That's basically the process by which gatekeepers tell newcomers to go away. It doesn't really apply to the vast majority of changes from established contributors. You only have to hang out on l-k for a few minutes to get the flavor of things.

The only patches getting thorough code reviews are getting them from organizations with their own internal code review culture.

Do you have evidence for your claims?

> Wonder no more: hardly anything in Linux has ever been meaningfully reviewed.

Incorrect.

> Code looks like it couldn't possibly pass review because it can't, and didn't.

What's this supposed to mean? Are you talking about a specific piece of code, all of Linux, or some strange general statement? No matter what it seems wrong, if nothing else because you seem to be ascribing your own unique definition to the concept of "review" or what code must look like in order to "pass review".

It's clear that you have some dislike for Linux, maybe technical maybe personal, maybe justified maybe not. Unfortunately though even if it is a well founded technical dislike, it has clouded your vision so much that you're lashing out and making trivially disproven nonsense like this on an internet forum. I think it's time for you to take a breath and think what you are hoping to achieve.

Linux has extensive review efforts many of which are public but also many more private ones that you nevertheless can see the evidence of, both pre and post code merge, which I'm almost certain you would know of unless you're just a complete clueless troll.

So given that, do you think it bolsters your credibility or achieves your goals of readjusting peoples' opinion of Linux, by blurting out such garbage? I don't think it does, but I think you sound frustrated and angry, perhaps because nobody will listen to you.

Let's start with a clean slate and let's hear your misgivings about Linux, the people involved with it, the process, etc.

Reply on news.ycombinator.com