sys: build core headers under bounds-safety

[abhijeetsharma2002_gmail.com]

Summary

Spell out existing idioms the checker rejects: allocator and
register-read locals as __single, per-CPU and td_sched accessors kept
as pointer arithmetic rather than integer round-trips, real array
bounds on the libkern tables. No behaviour or layout change.

Sponsored by: The FreeBSD Foundation

[nickbanks_netflix.com]

sys/sys/fnv_hash.h

50

Sorry to bother, but may I ask for the reasoning behind adding this new local variable n instead of using len directly? Just curious. Also, I didn't realize FreeBSD had these types of annotations. Reminds me of SAL on Windows.

[abhijeetsharma2002_gmail.com]

sys/sys/fnv_hash.h

50

No bother at all.

buf is __sized_by(len), so len is not an independent parameter anymore. It is the recorded size of buf, and the compiler requires the pointer and its count to be assigned together, so decrementing len by itself is rejected. The local keeps len, meaning "size of buf", for the whole function and leaves the counter as an ordinary variable.

You can look up the docs for fbounds-safety here.

I am not familiar with SAL so cannot compare them properly. This is a FreeBSD Foundation project to adopt -fbounds-safety in the kernel network stack, proposal can be found here.

[imp]

So you are using the raw compiler commands, unconditionally. This will likely break gcc badly.
Normally we define 'standard' macros in cdefs.h and use those everywhere we can (eg, we'd use
them in clang, but maybe not gcc if there's no support for the concept). I don't have a good
design, but I can't see this not breaking things.

So that needs to be addressed before these can be committed.

sys/sys/fnv_hash.h

50

We use SAL markings in the syscall.master files. We don't use it elsewhere.

[imp]

I actually missed the other review that did exactly what I requested.

[abhijeetsharma2002_gmail.com]

In D58985#1353091, @imp wrote:

So you are using the raw compiler commands, unconditionally. This will likely break gcc badly.
Normally we define 'standard' macros in cdefs.h and use those everywhere we can (eg, we'd use
them in clang, but maybe not gcc if there's no support for the concept). I don't have a good
design, but I can't see this not breaking things.

So that needs to be addressed before these can be committed.

This part of a stack starting at D58983 which contains definitions for annotations in cdefs.h. I had 5 commits; thus, there are 5 differentials. Please let me know if it is preferable to squash the commits to have only one large differential to review.

[rpaulo]

sys/compat/linuxkpi/common/include/linux/compiler.h

49

I think this would not be needed anymore? Meaning we can remove lines 50 to 55?

sys/sys/pcpu.h

262

Maybe we should do this under #if ptrcheck ?

sys/sys/proc.h

1349

Do you need the unsafe cast?

[abhijeetsharma2002_gmail.com]

sys/compat/linuxkpi/common/include/linux/compiler.h

49

Hey, while this block is not needed as written, we do still want __counted_by defined here. Linux only uses it on FAM, which gcc and mainline clang both support, whereas cdefs.h only gives the real attribute under -fbounds-safety. So, as written the driver annotations become no-ops on a normal build.

sys/sys/pcpu.h

262

Sure

sys/sys/proc.h

1349

Yes. td is _single, so any plain arithmetic on it is rejected. _unsafe_indexable is what makes the arithmetic legal, and the forge converts the result back to a td_sched *.

[abhijeetsharma2002_gmail.com]

Address code review.

[jrtc27]

Prevailing style is to put a space between the * and qualifier that follows, which all these new annotations should follow.

All these new uses of uintptr_t concern me. They don't just go away when you make the macros no-ops.

[jrtc27]

A higher level comment: I don't think we should be landing tree-wide changes to support -fbounds-safety unless there's community buy-in for actually adopting it as an in-tree option in parts of the kernel. Otherwise we're picking up a lot of churn and annotations that will bit-rot and pollute the code for no gain.

[jrtc27]

sys/sys/pcpu.h

249

Why do you need this if? Doesn't __unsafe_forge_single compile to the identity (with a cast) otherwise making the two versions the same?

sys/sys/proc.h

1356–1357

What's the point of the one-past-the-end comment? And of course __single is inappropriate, getting the byte one-past-the-end of the stack is clearly useless.

1359

Why the uintptr_t cast? Why is td_kstack itself not __unsafe_indexable?

sys/sys/sdt.h

234

This doesn't seem right

[jrtc27]

sys/netinet/tcp_var.h

1438

Why? They're equivalent?

sys/sys/ck.h

7

Uh, this is a big hack

sys/sys/libkern.h

51

Surely there's a way to handle incomplete array types?