linuxkpi: Allow ida_destroy and idr_destroy to be called multiple times

[ashafer_badland.io]

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.

[bz]

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.

[ashafer_badland.io]

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]

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

[manu]

sys/compat/linuxkpi/common/src/linux_idr.c

186

mtx_initialized returns an int so style(9) says to not use ! but == 0

[ashafer_badland.io]

Updated with style, thanks.

[ashafer_badland.io]

Any update on getting this merged?

[bz]

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

[imp]

git arc patch -c might work too

[bz]

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.

[imp]

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.

[ashafer_badland.io]

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.