Page MenuHomeFreeBSD

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

Authored by ashafer_badland.io on Apr 19 2024, 2:48 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Jun 16, 5:07 PM
Unknown Object (File)
Fri, Jun 7, 2:41 AM
Unknown Object (File)
Mon, Jun 3, 11:43 AM
Unknown Object (File)
Mon, Jun 3, 1:11 AM
Unknown Object (File)
Thu, May 23, 9:29 PM
Unknown Object (File)
May 12 2024, 12:00 PM
Unknown Object (File)
Apr 28 2024, 11:29 PM
Unknown Object (File)
Apr 27 2024, 9:52 AM

Details

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

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).

bz added a reviewer: linuxkpi.

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 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

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

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.Thu, Jun 6, 8:43 PM
This revision was automatically updated to reflect the committed changes.