udev: prevent interruption by thread cancellation - #24
Conversation
Using mutexes only guarantees no concurrency during normal runtime. However, if a thread is cancelled mid-execution, a libudev API call might be interrupted halfway (partially executed), leaving memory values in an inconsistent or abnormal state. Furthermore, this change aligns with the requirement that libudev interfaces are not thread-safe and must not be called from multiple threads. Signed-off-by: Wu Guanghao <wuguanghao3@huawei.com>
|
Thanks! I have applied your patch to my |
|
@wuguanghao3, @bmarzins, may I ask you to review 58519b1 ? |
LGTM But I have a question: can't we just use (call) directly instead of udev_xxx(arg1, arg2, ...)? Do we need to use different macros based on the parameters? What is the advantage of doing it this way? |
In principle we could. We wouldn't need the Let me try. |
|
I remember the rationale now. @wuguanghao3, your suggestion works only with preprocessor macros. For wrapper functions it's either impossible or really clumsy, because we need to insert the argument types in the wrapper function's argument list, while omitting them in the actual libudev call. (If you can conceive of an elegant way to do this, let me know). What we can do is just use preprocessor macros: This works, and we don't even need any However, we have the wrapper functions for a reason. By introducing the wrapper functions, we achieve clean separation from libudev, and we can make sure that we don't ever call libudev directly. We just don't include Footnotes
|
|
I realize what I did previously here was wrong. By applying my commit on top of yours, I called I've pushed a new version to my tip branch (b2d35f7). @wuguanghao3, please review test it in your environment. And again, if you can conceive of a simpler way to write these wrappers, let me know. |
|
I had to rebase and re-push again, current commit ID is 3019b25 |
OK, LGTM |
Have you run your test with this code? |
Yes, currently testing. The issue has not recurred yet. |
The UAF issue could not be reproduced. However, a new reference issue occurred, which appears to be exclusive to version 0.14.3 and was not present in 0.9.5. I am currently running tests on 0.14.3 with the current patch applied. |
|
Sigh. This indicates a refcount underflow on the udev_device. Which is basically the same thing as a UAF. Could you perhaps apply the current commit to 0.9.5 and see if it's really a regression between 0.95 and 0.14.3? Given that the issue has apparently occured after 4h of testing, you proabably need to run the test for a couple of hours to verify that the problem does not occur. |
The UAF and the reference issue are distinct. The UAF involves memory inconsistency resulting from thread cancellation, whereas the reference issue may stem from a redundant execution of an upper-level call. |
It has been confirmed that this issue was introduced in a release between 0.9.5 and 0.14.3. I conducted a 7-hour test running version 0.9.5 with the latest commit, and the issue could not be reproduced. |
A refcount underflow means that an object is being unref'd after having reached refcount zero. IOW, this object was still in use after it should have been freed already. Maybe the How often have you observed this refcount underflow so far?
There has been a major cleanup of the code that frees maps and paths in 0.14.0 (commit edf2bac ff.) So if you want to to investigate further, you may want to start with comparing 0.13.0 and 0.14.0, both with the latest wrapper patch applied. When you apply the patch, make sure that no direct libudev calls remain. Given that you observe the problem in the Can you share your test program, so that others can try to reproduce the issue and find a solution? I have also done some extensive testing, but apparently the way you restart multipathd during the test can trigger error conditions that I was not seeing. |
Below is a simplified test case that includes the main operations. One additional point to note: we identify local disks as multipath by default. |
I attempted to capture the call stack using ASan but was unsuccessful. This indicates that the memory address had not been released yet when the double decrement of the reference count occurred. |
|
So you're running both I suppose you're aware that this is an unrealistic scenario. Of course it's wrong that multipathd gets the refcount wrong and crashes, but I don't see it as a problem with high practical relevance. I'll try to reproduce the issue nonetheless. Regardless, I'd appreciate a journal output from your test showing the sequence of events before the crash. Because multipathd is restarted immediately after startup, It's likely that the issue is related to the initial startup and device configuration, and to being interrupted / killed in this stage. |
|
Your test doesn't work on my system. systemd simply refuses to restart multipathd in such a quick succession (which is reasonable). At least some sleep time is needed between multipathd restarts. |
|
I've made your test work by adding |
27ed4e7 to
6931e84
Compare
|
For what it's worth, the commit for this in your tip branch looks good to me. |
Sorry for the delay, I've been a bit busy recently and haven't had a chance to look into this. I'll try to reproduce it and find the root cause when I have some time, and will keep you updated. |
|
@wuguanghao3 I copied your setup, and have reproduced the same coredump. Unfortunately, it took hours running overnight to hit it. |
|
@wuguanghao3 are you able to quickly and reliably hit this ref count assert? Could you try removing the multipath-tools/libmultipath/discovery.c Line 224 in 5a60a67 pthread_testcancel() will trigger after the udev device is unref'ed and run the cleanup handlers, including cleanup_udev_device_ptr() which will unref the device again. If the thread didn't get cancelled, the next line would set udevice to NULL so the cleanup handler wouldn't do anything, but with that pthread_testcancel(), the next line never gets run.
The general problem with it is that it makes all the |
|
So, I'm pretty sure I was right about multipath-tools/multipathd/main.c Lines 1718 to 1719 in 5a60a67 If the thread is cancelled (because multipathd is shutting down) while udev_device_unref(pp->udev) is being called, pp->udev will still point to the old, unref'ed value when cleanup_paths() is called.
|
Using mutexes only guarantees no concurrency during normal runtime. However, if a thread is cancelled mid-execution, a libudev API call might be interrupted halfway (partially executed), leaving memory values in an inconsistent or abnormal state.
Furthermore, this change aligns with the requirement that libudev interfaces are not thread-safe and must not be called from multiple threads.