Page MenuHomeFreeBSD

Catch illegal M_WAITOK immediately.
Needs ReviewPublic

Authored by glebius on Aug 11 2020, 3:49 PM.

Details

Summary

Right now illegal M_WAITOK can easily leak into sources because
INVARIANTS would catch it only when it really would sleep(9). So
most test runs will succeed and only stress testing may catch it.

Fix that with assertion right in malloc.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 32915
Build 30313: arc lint + arc unit

Event Timeline

sys/kern/kern_malloc.c
622

Why not check it in UMA as well?

Why it is not WITNESS_WARN() ? Perhaps witness_warn should take td_no_sleeping as arbitrary non-sleepable lock as well.

In D26027#577390, @kib wrote:

Why it is not WITNESS_WARN() ? Perhaps witness_warn should take td_no_sleeping as arbitrary non-sleepable lock as well.

Better to keep this check under INVARIANTS rather than under WITNESS.

First, WITNESS is slow and expensive, so many developers skip testing with WITNESS unless they introduce new locks or new locking schemes. Second, WITNESS is designed for testing lock orders mostly, but non-sleepability can be induced not only by locks, e.g. epoch.

I remember UMA warning about relevant allocations without even attempting to sleep, but maybe my memory is off here.

I think the key problem is that there are many places which opencode a variant of "must be able to go off cpu". Something like ASSERT_CAN_SLEEP() provided by INVARIANTS and possibly extended when WITNESS is present would be best in my opinion.

Then this patch would be:

if (flags & M_WAITOK)
    ASSERT_CAN_SLEEP();
In D26027#577448, @mjg wrote:

I remember UMA warning about relevant allocations without even attempting to sleep, but maybe my memory is off here.

It has this check in uma_zalloc_domain() with WITNESS and in general path with WITNESS & INVARIANTS.

This revision is now accepted and ready to land.Aug 12 2020, 2:38 PM

So this indeed catches more stuff but panicking is too harsh. This should convert to just a warning for the foreseeable future.