uma_zalloc: assert M_NOWAIT ^ M_WAITOK
ClosedPublic
Actions

Authored by vangyzen on Mar 5 2022, 12:56 AM.

Details

Reviewers

rstone
markj

Commits

rGa8cbb835bfdf: uma_zalloc: assert M_NOWAIT ^ M_WAITOK

Summary

The uma_zalloc functions expect exactly one of [M_NOWAIT, M_WAITOK].
If neither or both are passed, print an error and a stack dump.
Only do this ten times, to prevent livelock. In the future, after
this exposes enough bad callers, this will be changed to a KASSERT().

MFC after: 1 month
Sponsored by: Dell EMC Isilon

Test Plan

The whole kyua suite completed without a panic or stack-dump.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

vangyzen created this revision.Mar 5 2022, 12:56 AM

Herald added subscribers: dab, badger, imp. · View Herald TranscriptMar 5 2022, 12:56 AM

vangyzen requested review of this revision.Mar 5 2022, 12:56 AM

Harbormaster completed remote builds in B44670: Diff 103518.Mar 5 2022, 12:56 AM

vangyzen mentioned this in D34450: vm_ksubmap_init: pass M_WAITOK to vmem_init -> uma_zalloc_arg.Mar 5 2022, 12:58 AM

vangyzen mentioned this in D34451: ksiginfo_alloc: pass M_WAITOK or M_NOWAIT to uma_zalloc.

vangyzen added inline comments.Mar 5 2022, 1:01 AM

sys/vm/uma_core.c
3689	I love the irony of hard-coding M_WAITOK here.

rstone added inline comments.Mar 5 2022, 9:29 PM

sys/vm/uma_core.c
3689	Do we actually have to allocate one here? I thought that struct stack was small enough to be safe to be allocated directly from the stack.

auto alloc the struct

Harbormaster completed remote builds in B44680: Diff 103551.Mar 7 2022, 1:20 AM

vangyzen marked an inline comment as done.Mar 7 2022, 1:28 AM

vangyzen added inline comments.

sys/vm/uma_core.c
3689	No, we don't. Contrary to the little voice in my head, I tried using this KPI by reading _only the man page_, which clearly says `stack_create` is required. The src tree tells a different story, where only 1 out of 24 instances uses `stack_create`. From now on, I'll listen to the voices in my head. No...wait... D34461 updates the man page.

rstone accepted this revision.Mar 7 2022, 2:39 PM

This revision is now accepted and ready to land.Mar 7 2022, 2:39 PM

Thanks for the review, Ryan.

Mark, if you have time, I'd appreciate one more set of eyes.

This should go in uma_zalloc_debug() instead. Otherwise uma_zalloc_smr() isn't checked.

This should go in uma_zalloc_debug() instead. Otherwise uma_zalloc_smr() isn't checked.

Thank you! I also noticed that uma_zalloc_domain() does not call uma_zalloc_debug() in the multi-domain path.

In D34452#780985, @vangyzen wrote:

This should go in uma_zalloc_debug() instead. Otherwise uma_zalloc_smr() isn't checked.

Thank you! I also noticed that uma_zalloc_domain() does not call uma_zalloc_debug() in the multi-domain path.

Yet another bug. :(

move to uma_zalloc_debug()

This revision now requires review to proceed.Mar 7 2022, 6:55 PM

Harbormaster completed remote builds in B44685: Diff 103597.Mar 7 2022, 6:55 PM

In D34452#780985, @vangyzen wrote:

I also noticed that uma_zalloc_domain() does not call uma_zalloc_debug() in the multi-domain path.

Fixed in D34472.

D34450 and D34451 fix the WAIT flags in two callers.

rstone accepted this revision.Mar 10 2022, 8:38 PM

This revision is now accepted and ready to land.Mar 10 2022, 8:38 PM

I think the _Static_assert is overkill: yes, if one of the flags is defined to be zero then the assertion is always true, but that would be a legitimate implementation choice (similar to how O_RDONLY is 0) and the assertion would be harmless. In any case it's very unlikely to change. I don't really object though.

Closed by commit rGa8cbb835bfdf: uma_zalloc: assert M_NOWAIT ^ M_WAITOK (authored by vangyzen). · Explain WhyMar 26 2022, 1:14 AM

This revision was automatically updated to reflect the committed changes.

vangyzen added a commit: rGa8cbb835bfdf: uma_zalloc: assert M_NOWAIT ^ M_WAITOK.