linuxkpi: Allow ida_destroy and idr_destroy to be called multiple times
ClosedPublic
Actions

Authored by ashafer_badland.io on Apr 19 2024, 2:48 PM.

Details

Reviewers

bz
manu

Group Reviewers

linuxkpi

Commits

rG89e485e04d54: linuxkpi: Allow ida_destroy and idr_destroy to be called multiple times
rG613723bac219: linuxkpi: Allow ida_destroy and idr_destroy to be called multiple times

Summary

This fixes some weird behavior triggered by nvidia-drm.ko: some DRM
cleanup functions will be called multiple times, leading to a double
free. drm_mode_config_cleanup will be called twice, causing ida_destroy
to be called twice. Although calling the cleanup twice doesn't seem
very clean, on Linux this seems to be permissable as it handles it
just fine.

In order to preserve this behavior this change checks if the objects
have already been destroyed and bails if so. This fixes the panic seen
when unloading the nvidia-drm driver.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

ashafer_badland.io created this revision.Apr 19 2024, 2:48 PM

Herald added subscribers: linuxkpi, emaste, imp. · View Herald TranscriptApr 19 2024, 2:48 PM

ashafer_badland.io requested review of this revision.Apr 19 2024, 2:48 PM

Harbormaster completed remote builds in B57217: Diff 137387.Apr 19 2024, 2:48 PM

Nice tracking down.
Is there a PR for this somewhere we need to reference?

Are you actually seeing a double-free as indicated in the description or is the panic on acquiring a non-initilized mutex?

sys/compat/linuxkpi/common/src/linux_idr.c
182	"If" upper case and I'd remove the "empty/" as we don't check for that.
184	I think this should be part of the commit message. I'd rather state that the idr can be re-used--only freeing the internal memory--and we could be called multiple times without an idr_init() in between so guarding for that.
188	You wouldn't need the { } and a space after if. I do wonder if there is any other pointer or simple data type we could check here instead of whether the mutex is initialized? Likely not easily ... given the mtx_destroy(). Hmm. Guess not.

There isn't a PR, this has been a longstanding issue with nvidia-drm that I'm
finally getting around to solving.

If you unload nvidia-drm.ko right now you'll see a panic from the mutex
initialization assert. If you fix that (idr_destroy part of this change) then
you'll run into a double free (ida_destroy part of this change).

Harbormaster completed remote builds in B57228: Diff 137405.Apr 19 2024, 6:33 PM

Thanks. If no one complains the next days I'll put this in.

This revision is now accepted and ready to land.Apr 19 2024, 6:53 PM

manu accepted this revision.Apr 20 2024, 7:22 AM

manu added a subscriber: manu.

manu added inline comments.

sys/compat/linuxkpi/common/src/linux_idr.c
186	mtx_initialized returns an int so style(9) says to not use ! but == 0

Updated with style, thanks.

This revision now requires review to proceed.Apr 20 2024, 4:04 PM

Harbormaster completed remote builds in B57246: Diff 137438.Apr 20 2024, 4:04 PM

Any update on getting this merged?

Do you have this staged somewhere in git I can pull it from?

git arc patch -c might work too

In D44865#1034261, @imp wrote:

git arc patch -c might work too

Yes, it would; I could also do most of that by had but there's a few things arc doesn't give, e.g. AuthorDate; even jhb recently asked in D44306 (another one of Austin's) if we could pull it from somewhere, so I just followed example.

In D44865#1034278, @bz wrote:

In D44865#1034261, @imp wrote:

git arc patch -c might work too

Yes, it would; I could also do most of that by had but there's a few things arc doesn't give, e.g. AuthorDate; even jhb recently asked in D44306 (another one of Austin's) if we could pull it from somewhere, so I just followed example.

Yea. I don't value author date very highly, so I didn't bother. phab likely has it somewhere, but only in some paths to upload is that data present. It has no real significance. It's easily forged so can't be used for legal things and make the git log confusingly non-time-lineaer.

Though what I wrote was, at best, a stop-gap. It often won't work. Ideally there's a branch we can just pull it from. Honestly, the experience writing it is why I think our primary ingest for patches should be github and not phabricator. It's just too hard for 3rd parties.

Pushed here: https://github.com/amshafer/freebsd/commit/214a96bb38bf5534b59eb149367369a233fa6826

Although for the record I don't care much if the author date and such details gets stripped on my patches, so feel free to grab things from phabricator in the future without asking. I'll try to remember to push things, but really as long as the code gets in I'm happy.

This revision was not accepted when it landed; it landed in state Needs Review.Jun 6 2024, 8:43 PM

Closed by commit rG613723bac219: linuxkpi: Allow ida_destroy and idr_destroy to be called multiple times (authored by ashafer_badland.io, committed by wulf). · Explain Why

This revision was automatically updated to reflect the committed changes.

wulf added a commit: rG613723bac219: linuxkpi: Allow ida_destroy and idr_destroy to be called multiple times.

wulf added a commit: rG89e485e04d54: linuxkpi: Allow ida_destroy and idr_destroy to be called multiple times.Aug 1 2024, 10:34 PM