Lock concurrent access to mWpaSupplicant

[tewarid]

Lock concurrent access to mWpaSupplicant.

Problem

state in GDBusWpaSupplicant may be concurrently accessed by multiples threads leading to race condition.

Change overview

Lock concurrent access to mWpaSupplicant.

Testing

Tested using a custom embedded linux system with all-clusters-app example, and macOS chip-tool for provisioning.

[mspang]

Please use a mutex.

[tewarid]

Please use a mutex.

@mspang Please see #9322 (comment) where decision was made with @bzbarsky-apple to use std::atomic. Why is a lock called for in this case?

Code uses std::atomic here under similar circumstance

connectedhomeip/src/include/platform/internal/GenericPlatformManagerImpl_POSIX.h

Line 113 in 181ab66

std::atomic<bool> mShouldRunEventLoop;

Code also uses volatile

connectedhomeip/src/inet/InetLayer.h

Line 167 in 181ab66

volatile enum {

[mspang]

Please use a mutex.

@mspang Please see #9322 (comment) where decision was made with @bzbarsky-apple to use std::atomic. Why is a lock called for in this case?

Code uses std::atomic here under similar circumstance

connectedhomeip/src/include/platform/internal/GenericPlatformManagerImpl_POSIX.h

Line 113 in 181ab66

std::atomic<bool> mShouldRunEventLoop;

It's not quite the same, but this was probably a mistake.

Code also uses volatile

That doesn't avoid the race except in certain compilers (MSVC) using nonstandard assumptions.

connectedhomeip/src/inet/InetLayer.h

Line 167 in 181ab66

volatile enum {

[kghost]

I understand that there can be racing, due to there 2 directions which can go into the class:

1. From CHIP stack.
2. From dbus callback.

The better solution can be dispatching the dbus callback back into the chip main thread, so it will be guarded by the chip global lock

Anyway, if this PR resolves the racing problem, there is no reason to block it. But it would be better if leave notes about it, and maybe we can improve it later.

[tewarid]

The better solution can be dispatching the dbus callback back into the chip main thread, so it will be guarded by the chip global lock

@kghost Could you please point to an example in the code?

[tewarid]

Please use a mutex.

@mspang Please see #9322 (comment) where decision was made with @bzbarsky-apple to use std::atomic. Why is a lock called for in this case?
Code uses std::atomic here under similar circumstance

connectedhomeip/src/include/platform/internal/GenericPlatformManagerImpl_POSIX.h

Line 113 in 181ab66

std::atomic<bool> mShouldRunEventLoop;

It's not quite the same, but this was probably a mistake.

You're right, but don't think it is a mistake - a separate mutex is used to lock critical section.

Code also uses volatile

That doesn't avoid the race except in certain compilers (MSVC) using nonstandard assumptions.

Not a concern of this PR, but does this need to be fixed?

connectedhomeip/src/inet/InetLayer.h

Line 167 in 181ab66

volatile enum {

[kghost]

@kghost Could you please point to an example in the code?

I mean that modify the main loop, use a single loop for both chip sdk and d-bus, such that there is only one thread, to prevent the racing.

It won't be an easy change, I need to invest that.

[mspang]

Please use a mutex.

@mspang Please see #9322 (comment) where decision was made with @bzbarsky-apple to use std::atomic. Why is a lock called for in this case?
Code uses std::atomic here under similar circumstance

connectedhomeip/src/include/platform/internal/GenericPlatformManagerImpl_POSIX.h

Line 113 in 181ab66

std::atomic<bool> mShouldRunEventLoop;

It's not quite the same, but this was probably a mistake.

You're right, but don't think it is a mistake - a separate mutex is used to lock critical section.

The reasons it was a mistake:

• Because people will copy it
• Because atomics are subtle
• Because relaxed atomics are especially subtle to the extent that the folks who standardized them aren't confident that they can be used safely

Not because it doesn't work or encounters a race condition (it does mean that the load that results in event loop termination does not synchronize-with the thread that stored the termination condition, which is where there is room for controversy). Basically, it's not worth the risk and the controversy. Mutex is a more understandable tool and gives you a real critical section.

Code also uses volatile

That doesn't avoid the race except in certain compilers (MSVC) using nonstandard assumptions.

Not a concern of this PR, but does this need to be fixed?

connectedhomeip/src/inet/InetLayer.h

Line 167 in 181ab66

volatile enum {

[mspang]

Please use a mutex.

@mspang Please see #9322 (comment) where decision was made with @bzbarsky-apple to use std::atomic. Why is a lock called for in this case?
Code uses std::atomic here under similar circumstance

connectedhomeip/src/include/platform/internal/GenericPlatformManagerImpl_POSIX.h

Line 113 in 181ab66

std::atomic<bool> mShouldRunEventLoop;

It's not quite the same, but this was probably a mistake.

You're right, but don't think it is a mistake - a separate mutex is used to lock critical section.

The reasons it was a mistake:

• Because people will copy it
• Because atomics are subtle
• Because relaxed atomics are especially subtle to the extent that the folks who standardized them aren't confident that they can be used safely

Not because it doesn't work or encounters a race condition (it does mean that the load that results in event loop termination does not synchronize-with the thread that stored the termination condition, which is where there is room for controversy). Basically, it's not worth the risk and the controversy. Mutex is a more understandable tool and gives you a real critical section.

BTW, I wrote that code, so you can believe me when I say it was a mistake.

Code also uses volatile

That doesn't avoid the race except in certain compilers (MSVC) using nonstandard assumptions.

Not a concern of this PR, but does this need to be fixed?

connectedhomeip/src/inet/InetLayer.h

Line 167 in 181ab66

volatile enum {

[mspang]

Not a concern of this PR, but does this need to be fixed?

connectedhomeip/src/inet/InetLayer.h

Line 167 in 181ab66

volatile enum {

Yes, this code is incorrect and should be fixed, C++11 is the first C++ with a memory model and it did not make volatile a synchronization operation. In fact volatile doesn't have any standard semantics, it's implementation defined what it means.