linuxkpi: `GFP_KERNEL` equals `M_NOWAIT` now instead of `M_WAITOK`
ClosedPublic
Actions

Authored by dumbbell on Oct 2 2023, 10:15 PM.

Details

Reviewers

manu
bz

Commits

rG14dcd4098374: linuxkpi: `GFP_KERNEL` equals `M_NOWAIT` now

Summary

Why

The reason is that in some places in the DRM drivers (in particular, the framebuffer management code), kmalloc() is called from a non-sleepable context, such as after a call to mtx_lock(8) with an MTX_DEF mutex.

If GFP_KERNEL is defined as M_WAITOK, we hit an assertion from witness(4).

How

The definition of GFP_KERNEL is changed to M_NOWAIT. This means that callers should verify the return value of kmalloc(). Fortunately, this is always the case in Linux.

This is required by the proposed new DRM/vt(4) integration (https://github.com/freebsd/drm-kmod/pull/243).

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

dumbbell created this revision.Oct 2 2023, 10:15 PM

Herald added subscribers: linuxkpi, emaste, imp. · View Herald TranscriptOct 2 2023, 10:15 PM

dumbbell requested review of this revision.Oct 2 2023, 10:15 PM

dumbbell edited the summary of this revision. (Show Details)Oct 2 2023, 10:21 PM

There are other users which do NOT check results in LinuxKPI.
vmmap_add() is at least one of them; I would make sure there's a full audit and would then explicitly mention that case in the proposed commit message as well.

That said I have no clue if this really is a good idea or if the quoted code really requires this or, if FreeBSD code, could be fixed.

This revision now requires changes to proceed.Oct 2 2023, 10:31 PM

emaste added inline comments.Oct 2 2023, 10:32 PM

sys/compat/linuxkpi/common/src/linux_shmemfs.c
51–54	Should we just delete this instead? Or do we need a note about a potential future need to handle `GFP_NOWAIT` differently from `GFP_KERNEL`?

Update patch to:

Add "ret == NULL" checks in a few places where GFP_KERNEL is used to handle allocation failures.
Use M_WAITOK explicitly instead of GFP_KERNEL in places where we can't really check the return value or we are not sure it is checked by the caller.

This should address @bz's concerns.

dumbbell marked an inline comment as done.Oct 3 2023, 9:23 PM

dumbbell added inline comments.

sys/compat/linuxkpi/common/src/linux_shmemfs.c
51–54	I followed your advice by deleting the previous check and adding a comment to describe a bit the history.

To tell a bit more about the updated patch: I tracked all uses of GFP_* and verified that the return value was verified. In a few places where the code can't return an error, I replaced GFP_KERNEL with M_WAITOK to make it explicit. vmmap_add() is an example. Another one is the vmalloc*() macros: some of their uses don't check for failures on purpose.

manu accepted this revision.Oct 4 2023, 5:29 AM

This revision was not accepted when it landed; it landed in state Needs Review.Nov 24 2023, 5:32 PM

Closed by commit rG14dcd4098374: linuxkpi: `GFP_KERNEL` equals `M_NOWAIT` now (authored by dumbbell). · Explain Why

This revision was automatically updated to reflect the committed changes.

dumbbell added a commit: rG14dcd4098374: linuxkpi: `GFP_KERNEL` equals `M_NOWAIT` now.

dumbbell added a reverting change: D42962: Revert "linuxkpi: `GFP_KERNEL` equals `M_NOWAIT` now".Dec 7 2023, 6:55 PM

dumbbell added a reverting change: rG8a8e86b819dc: Revert "linuxkpi: `GFP_KERNEL` equals `M_NOWAIT` now".Dec 7 2023, 8:55 PM