Consistently enforce the restriction against calling malloc/free when in a critical section

[jtl]

Summary

uma_zalloc_arg()/uma_zalloc_free() may acquire a sleepable lock on the
zone. The malloc() family of functions may call uma_zalloc_arg() or
uma_zalloc_free().

The malloc(9) man page currently claims that free() will never sleep.
It also implies that the malloc() family of functions will not sleep
when called with M_NOWAIT. However, it is more correct to say that
these functions will not sleep indefinitely. Indeed, they may acquire
a sleepable lock. However, a developer may overlook this restriction
because the WITNESS check that catches attempts to call the malloc()
family of functions within a critical section is inconsistenly
applied.

This change clarifies the language of the malloc(9) man page to clarify
the restriction against calling the malloc() family of functions
while in a critical section or holding a spin lock. It also adds
KASSERTs at appropriate points to make the enforcement of this
restriction more consistent.

PR: 204633
Sponsored by: Juniper Networks
Approved by: gnn (mentor) [assuming he does...]

Test Plan

The kernel compiles and doesn't panic with a WITNESS kernel.

[markj]

share/man/man9/malloc.9

226 ↗

(On Diff #10277)

As a general rule, "sleep" always refers to unbounded sleep except in "sleep mutex," which is terminology that's only used by witness anyway. So qualifying "unbounded sleep" makes the man page inconsistent with everything else - I would suggest sticking with sleep to avoid confusion.

236 ↗

(On Diff #10277)

mutexes aren't "sleepable": a thread is not allowed to sleep while holding one.

It's also better to avoid mentioning the reason for the restriction, since that's just an implementation detail. This is related to the paragraph above - really, free may not be called from fast interrupt handlers either (only spin mutexes may be acquired in fast interrupt handlers), and malloc/realloc/reallocf/free may not be called while holding a spin lock or critical section. So this could be coalesced with the paragraph above.

sys/kern/kern_malloc.c

479 ↗

(On Diff #10277)

The changes to malloc and free are redundant since these functions are built on top of UMA. So the assertions in uma_core.c should be sufficient.

[jtl]

share/man/man9/malloc.9

236 ↗

(On Diff #10277)

ACK. Let me try again.

sys/kern/kern_malloc.c

479 ↗

(On Diff #10277)

I added these here to promote consistency. It appears that large malloc/frees don't go through uma_zalloc_arg()/uma_zalloc_free().

I also added the checks to uma_zalloc_arg/uma_zalloc_free because other code may call them directly.

[jtl]

Updated the man page wording.

[markj]

sys/kern/kern_malloc.c

479 ↗

(On Diff #10279)

Ah, indeed. In that case this seems fine to me.

[gnn]

Looks good to me. Approved for commit.

[mjg]

As a minor remark, is this a serious enough offence to warrant a panic instead of just a warning? We definitely don't panic on detecting such threads entering a function which can induce an unbound sleep, nor for several other violations.

The real question though is: any reason for hardcoding the check? This looke like a nice opportunity to provide simple to use macros for handling bound and unbound sleep. We already have plenty of WITNESS_WARN(WARN_GIANTOK | WARN_SLEEPOK, NULL, thorough the tree.

Instead we can have something like KASSERT(CAN_SLEEP()) and KASSERT(CAN_SLEEP_UNBOUND()) (excuse my English) which could be just plopped in relevant places. Or, if just warning is deemed enough maybe have KASSERT_WARN with these.

[jtl]

In D4197#88345, @mjg wrote:

As a minor remark, is this a serious enough offence to warrant a panic instead of just a warning? We definitely don't panic on detecting such threads entering a function which can induce an unbound sleep, nor for several other violations.

Actually, we do panic in these situations - at least when they produce sleep. WITNESS_WARN will panic in this situation. And, even if WITNESS_WARN doesn't panic, mi_switch will panic if you try to context switch out a thread in a critical section.

Instead we can have something like KASSERT(CAN_SLEEP()) and KASSERT(CAN_SLEEP_UNBOUND()) (excuse my English) which could be just plopped in relevant places. Or, if just warning is deemed enough maybe have KASSERT_WARN with these.

We probably could turn this into a macro similar to CRITICAL_ASSERT. But, I think it would only be used in these four places for now. Let me think about it.

[mjg]

In D4197#88369, @jtl wrote:

In D4197#88345, @mjg wrote:

As a minor remark, is this a serious enough offence to warrant a panic instead of just a warning? We definitely don't panic on detecting such threads entering a function which can induce an unbound sleep, nor for several other violations.

Actually, we do panic in these situations - at least when they produce sleep. WITNESS_WARN will panic in this situation. And, even if WITNESS_WARN doesn't panic, mi_switch will panic if you try to context switch out a thread in a critical section.

The kernel does not panic if you e.g. mtx_lock + malloc(..., M_WAITOK) as long as you don't have to sleep, which is the common case. As I understand the thread is about detecting when a thread is put into a situation where it may be possible it will block, even though it typically does not. Exactly same problem.

Instead we can have something like KASSERT(CAN_SLEEP()) and KASSERT(CAN_SLEEP_UNBOUND()) (excuse my English) which could be just plopped in relevant places. Or, if just warning is deemed enough maybe have KASSERT_WARN with these.

We probably could turn this into a macro similar to CRITICAL_ASSERT. But, I think it would only be used in these four places for now. Let me think about it.

If we had proper macros, we could just plop them into locking primitives.

[jtl]

In D4197#88372, @mjg wrote:

In D4197#88369, @jtl wrote:

In D4197#88345, @mjg wrote:

As a minor remark, is this a serious enough offence to warrant a panic instead of just a warning? We definitely don't panic on detecting such threads entering a function which can induce an unbound sleep, nor for several other violations.

Actually, we do panic in these situations - at least when they produce sleep. WITNESS_WARN will panic in this situation. And, even if WITNESS_WARN doesn't panic, mi_switch will panic if you try to context switch out a thread in a critical section.

The kernel does not panic if you e.g. mtx_lock + malloc(..., M_WAITOK) as long as you don't have to sleep, which is the common case. As I understand the thread is about detecting when a thread is put into a situation where it may be possible it will block, even though it typically does not. Exactly same problem.

Calling malloc while holding a lock is fundamentally different than calling malloc in a critical section. The code can assume that it will not be context switched out while in a critical section. It can make no such assumption when it merely holds a lock (at least, most locks).

For an example of another case where these two situations are treated differently, look at the way witness_checkorder() handles sleepable locks. It will panic if you are in the critical section; however, if you own a non-sleepable lock, it will only warn you.

The point of this change is to catch cases where people call one of the memory allocations functions in a critical section. Because of the assumptions the code may make about the way it will be handled in a critical section, it is worth reliably catching these instances.

Instead we can have something like KASSERT(CAN_SLEEP()) and KASSERT(CAN_SLEEP_UNBOUND()) (excuse my English) which could be just plopped in relevant places. Or, if just warning is deemed enough maybe have KASSERT_WARN with these.

We probably could turn this into a macro similar to CRITICAL_ASSERT. But, I think it would only be used in these four places for now. Let me think about it.

If we had proper macros, we could just plop them into locking primitives.

I think I see your point. However, I would like to commit the change this way. We can come back to the wider discussion about how to handle these checks later.