sysctl-register fixes

[geofft]

One UB fix, one pile of comments, one cleanup

[alex]

So, my perspective was that add() was safe since it'd never overflow, because access_ok would error if ptr + length overflowed. Do you disagree?

[geofft]

That's not the only thing that makes it unsafe. From https://llvm.org/docs/GetElementPtr.html#what-happens-if-an-array-index-is-out-of-bounds :

With the inbounds keyword, the result value of the GEP is undefined if the address is outside the actual underlying allocated object and not the address one-past-the-end.

Without the inbounds keyword, there are no restrictions on computing out-of-bounds addresses. Obviously, performing a load or a store requires an address of allocated and sufficiently aligned memory. But the GEP itself is only concerned with computing addresses.

offset/add are implemented by intrinsics::offset, i.e., getelementptr inbounds, and wrapping_offset/wrapping_add are implemented by intrinsics::arith_offset, i.e., getelementptr.
https://github.com/rust-lang/rust/blob/2612bbcba08dd81730edd8f2139005fb7a409294/src/librustc_codegen_llvm/intrinsic.rs#L201-L210

(The names are misleading and perhaps the docs are misleading too?)

[alex]

Would this ever construct a ptr not within the bounds?

[alex]

What the hell do bounds even mean when we're talking about a ptr to userspace memory?

[geofft]

I believe userspace pointers always count as out of bounds from the perspective of our kernel code. It isn't an object that's been created by our code, we can't dereference it normally, aliasing rules don't apply to it, etc. (Honestly I'd prefer if we just kept it a usize or a newtype on that, but the kernel APIs pass it around as a pointer.)

To be fair, I don't really know what a compiler might want to do with this information, and I'm not sure how this is different from, say, getting a pointer out of malloc. But I like erring on the side of caution with UB...

[geofft]

I had the same question over at nix-rust/nix#568 (comment) when implementing safe wrappers for process_vm_readv and process_vm_writev, which take pointers valid in some other process's memory, and ended up just newtyping (usize, usize). That one I think makes a little more sense to be cautious about, because the alternative was a non-raw &[u8], and also those pointers can actually alias valid pointers in the current process's own address space. I don't have a chapter-and-verse for why we had to avoid a pointer there (and my reading of the GEP docs is that a raw pointer would have been fine).