On Wed, Mar 08, 2023 at 08:29:41PM +0000, Richard W.M. Jones wrote: Note - POSIX says that calling pthread_key_delete() can leak memory. If the reason we are calling pthread_key_delete() is because libnbd was dlopen'd and is now being dlclose'd, and an app repeatedly reloads libnbd, causes an error, and then unloads libnbd, this leak could grow to be rather sizeable. But that is a rather unlikely scenario, and safely coordinating cleanup is hard when we consider both normal thread exits (where the key destructor runs if the key is not yet deleted) and module unloads (where pthread_key_delete skips key destructors). This is undefined behavior per POSIX. It is not safe to call pthread_getspecific() on a destroyed key (even though glibc lets us get away with it by giving us EINVAL). Indeed - once we call pthread_key_delete() in step 2, ANY further use of the key is going to cause undefined behavior (hopefully EINVAL failures, but worse behavior is also possible). One possible idea: have a non-thread-local global that starts life false, but which errors_free() atomically sets to true prior to calling pthread_key_delete(). All other functions which reference errors_key check that the global is still false before using pthread_{get,set}specific. Another idea: have a counter of the number of threads that have set a thread-local error but not yet exited. Increment the counter any time allocate_last_error_on_demand() says a new thread is triggering an error, and decrement the counter any time free_errors_key() calls the destructor at the end of a thread. Only when all threads with an allocated error have exited will the counter reach zero, so only the last thread to clean up needs to call pthread_key_delete(). The counter might not reach zero before the process exits, but that's okay (if the reason the module is being unloaded is because the process is going away, cleanup is less important), although I don't know if valgrind would complain. In fact, POSIX suggests: https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_key_de... "For an application to know that it is safe to delete a key, it has to know that all the threads that might potentially ever use the key do not attempt to use it again. For example, it could know this if all the client threads have called a cleanup procedure declaring that they are through with the module that is being shut down, perhaps by setting a reference count to zero." This still doesn't protect us from a parallel thread encountering its first need for error allocation after the point that we previously saw the count of threads with active errors go to zero. That said, we call allocate_last_error_on_demand() fairly reliably (most API calls do so in order to set the context - it's pretty hard to write a multi-threaded app using libnbd APIs but the first call to a libnbd API that would allocate error storage occurs after another thread has already started the shutdown process). I concur that removing the assertion seems reasonable to paper over the problem. But it does not solve the issue that we may still be leaking memory if the pthread_key_delete() happens before all threads that have allocated error storage have had a chance to reach their destructors, nor that even though glibc was nice and gave us EINVAL, we really are invoking undefined behavior by using the key after it is deleted. I might have split this into two lines, but this style for one-liner ifs is present elsewhere in the tree so it is not wrong. If we add some sort of global telling us whether the key is still active, we could still have: assert (nbd_error_key_finalized || nbd_get_error ()) But I'm not sure if it is worth that. Dropping the assertion is certainly simpler, even if it papers over other potential problems. This is where I'm thinking we can still do better by having a reference counter. I'll try a v2 along the lines of my thoughts above... -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org