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

Authored by jtl on Nov 17 2015, 8:31 PM.

Details

Reviewers

sjg
markj
gnn

Group Reviewers

manpages

Commits

rS291074: Consistently enforce the restriction against calling malloc/free when in a

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.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

jtl updated this revision to Diff 10277.Nov 17 2015, 8:31 PM

jtl retitled this revision from to Consistently enforce the restriction against calling malloc/free when in a critical section.

jtl updated this object.

jtl edited the test plan for this revision. (Show Details)

jtl added reviewers: gnn, sjg, markj.

Herald added a reviewer: manpages. · View Herald TranscriptNov 17 2015, 8:31 PM

Herald added a subscriber: imp. · View Herald Transcript

markj added inline comments.Nov 17 2015, 8:52 PM

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 added inline comments.Nov 17 2015, 8:57 PM

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.

Updated the man page wording.

Herald edited edge metadata. · View Herald TranscriptNov 17 2015, 9:00 PM

markj accepted this revision.Nov 17 2015, 10:44 PM

markj edited edge metadata.

markj added inline comments.

sys/kern/kern_malloc.c
479 ↗	(On Diff #10279)	Ah, indeed. In that case this seems fine to me.

This revision is now accepted and ready to land.Nov 17 2015, 10:44 PM

Looks good to me. Approved for commit.

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.

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.

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.

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.

Closed by commit rS291074: Consistently enforce the restriction against calling malloc/free when in a (authored by jtl). · Explain WhyNov 19 2015, 2:05 PM

This revision was automatically updated to reflect the committed changes.