uma: New check_align_mask(): Validate alignments (INVARIANTS)
ClosedPublic
Actions

Authored by olce on Oct 17 2023, 2:59 PM.

Details

Reviewers

markj
kib

Commits

rG27199e55a4d5: uma: New check_align_mask(): Validate alignments (INVARIANTS)
rG7deedba4e586: uma: New check_align_mask(): Validate alignments (INVARIANTS)
rG87090f5e5a7b: uma: New check_align_mask(): Validate alignments (INVARIANTS)

Summary

New function check_align_mask() asserts (under INVARIANTS) that the mask
fits in a (signed) integer (see the comment) and that the corresponding
alignment is a power of two.

Use check_align_mask() in uma_set_align_mask() and also in uma_zcreate()
to replace the KASSERT() there (that was checking only for a power of
2).

MFC after: 2 weeks
Sponsored by: The FreeBSD Foundation

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 54094
Build 50984: arc lint + arc unit

Event Timeline

olce created this revision.Oct 17 2023, 2:59 PM

Herald added a subscriber: imp. · View Herald TranscriptOct 17 2023, 2:59 PM

olce requested review of this revision.Oct 17 2023, 2:59 PM

Harbormaster completed remote builds in B54045: Diff 128928.Oct 17 2023, 2:59 PM

olce added a parent revision: D42262: uma: Make the cache alignment mask unsigned.Oct 17 2023, 3:00 PM

olce added a child revision: D42264: linuxkpi: dma_get_cache_alignment(): Fix off-by-one result.Oct 17 2023, 3:02 PM

markj added inline comments.Oct 17 2023, 3:13 PM

sys/vm/uma_core.c
334	I think the prototype isn't needed?
3257	Why not write these as KASSERT()s? __predict_false isn't needed here, the compiler knows that panic() doesn't return.

olce added inline comments.Oct 17 2023, 3:55 PM

sys/vm/uma_core.c
334	Indeed. Happy to remove it.
3257	I would prefer these checks to always be performed (even not under `INVARIANTS`) because I assume that an invalid value will really mess with UMA's internals, but I can change if you think it's not worth it. `__predict_false()` just indicates that the branch is unlikely to be taken, which is in principle independent from whether the branch basic block never exits (think, e.g., `exit1()`). That said, not finishing a basic block usually happens in error branches. Are you saying that compilers automatically "optimize" based on that? It would be a bit surprising.

markj added inline comments.Oct 18 2023, 3:18 PM

sys/vm/uma_core.c
3257	I assume that an invalid value will really mess with UMA's internals That's true of many of the KASSERTs in UMA, and in the kernel at large. Most of the explicit panic() calls in UMA are from debug code, so my inclination is to follow the precedent.

olce mentioned this in D42259: uma: UMA_ALIGN_CACHE: Resolve the proper value at use point.Oct 19 2023, 8:03 AM

olce mentioned this in D42262: uma: Make the cache alignment mask unsigned.Oct 19 2023, 8:09 AM

Cater to comments.

Harbormaster completed remote builds in B54073: Diff 129063.Oct 19 2023, 8:13 AM

olce added inline comments.Oct 19 2023, 8:14 AM

sys/vm/uma_core.c
3257	Done.

kib added inline comments.Oct 19 2023, 11:42 AM

sys/vm/uma_core.c
3265–3276	or even `mask < UINT_MAX / 2` (whatever right spelling for UINT_MAX is)

Change the mask too big check.

sys/vm/uma_core.c
3265–3276	Or even better given the comment: `mask <= INT_MAX`.

Harbormaster completed remote builds in B54094: Diff 129117.Oct 19 2023, 5:01 PM

kib accepted this revision.Oct 19 2023, 9:43 PM

kib added inline comments.

sys/vm/uma_core.c
3295	Did you checked that e.g. gcc does not warn about implicit signedness cast?

This revision is now accepted and ready to land.Oct 19 2023, 9:43 PM

olce added inline comments.Oct 20 2023, 1:13 PM

sys/vm/uma_core.c
3295	No, not yet. For base clang, I just assumed that `-Werror` is set. I've just looked up how to build the kernel with GCC. Compiling the required port right now.