patrick/zephyr
1
0
Fork 0
Code Issues Pull requests Actions 8 Packages Projects Releases Wiki Activity

net: sockets: socketpair: fix locking

Browse source 
The irq_lock() usage here is incompatible with SMP systems, and one's
first reaction might be to convert it to a spinlock.

But are those irq_lock() instances really necessary?

Commit 6161ea2542 ("net: socket: socketpair: mitigate possible race
condition") doesn't say much:

> There was a possible race condition between sock_is_nonblock()
> and k_sem_take() in spair_read() and spair_write() that was
> mitigated.

A possible race without the irq_lock would be:

thread A                thread B
|                       |
+ spair_write():        |
+   is_nonblock = sock_is_nonblock(spair); [false]
*   [preemption here]   |
|                       + spair_ioctl():
|                       +   res = k_sem_take(&spair->sem, K_FOREVER);
|                       +   [...]
|                       +   spair->flags |= SPAIR_FLAG_NONBLOCK;
|                       *   [preemption here]
+   res = k_sem_take(&spair->sem, K_NO_WAIT); [-1]
+   if (res < 0) {      |
+     if (is_nonblock) { [skipped] }
*     res = k_sem_take(&spair->sem, K_FOREVER); [blocks here]
|                       +   [...]

But the version with irq_lock() isn't much better:

thread A                thread B
|                       |
|                       + spair_ioctl():
|                       +   res = k_sem_take(&spair->sem, K_FOREVER);
|                       +   [...]
|                       *   [preemption here]
+ spair_write():        |
+   irq_lock();         |
+   is_nonblock = sock_is_nonblock(spair); [false]
+   res = k_sem_take(&spair->sem, K_NO_WAIT); [-1]
+   irq_unlock();       |
*   [preemption here]   |
|                       +   spair->flags |= SPAIR_FLAG_NONBLOCK;
|                       +   [...]
|                       +   k_sem_give(&spair->sem);
|                       + spair_read():
|                       +   res = k_sem_take(&spair->sem, K_NO_WAIT);
|                       *   [preemption here]
+   if (res < 0) {      |
+     if (is_nonblock) { [skipped] }
*     res = k_sem_take(&spair->sem, K_FOREVER); [blocks here]

In both cases the last k_sem_take(K_FOREVER) will block despite
SPAIR_FLAG_NONBLOCK being set at that moment. Other race scenarios
exist too, and on SMP they are even more likely.

The only guarantee provided by the irq_lock() is to make sure that
whenever the semaphore is acquired then the is_nonblock value is always
current. A better way to achieve that and be SMP compatible is to simply
move the initial sock_is_nonblock() *after* the k_sem_take() and remove
those irq_locks().

Signed-off-by: Nicolas Pitre <npitre@baylibre.com>
This commit is contained in:
Nicolas Pitre 2021-10-05 13:26:18 -04:00 committed by Christopher Friedt
parent c24d0c8405
commit cf49699b0d
1 changed files with 2 additions and 8 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

10
subsys/net/lib/sockets/socketpair.c
View file

@ -387,7 +387,6 @@ out:
static ssize_t spair_write(void *obj, const void *buffer, size_t count)
{
int res;
int key;
size_t avail;
bool is_nonblock;
size_t bytes_written;
@ -403,10 +402,8 @@ static ssize_t spair_write(void *obj, const void *buffer, size_t count)
goto out;
}
key = irq_lock();
is_nonblock = sock_is_nonblock(spair);
res = k_sem_take(&spair->sem, K_NO_WAIT);
irq_unlock(key);
is_nonblock = sock_is_nonblock(spair);
if (res < 0) {
if (is_nonblock) {
errno = EAGAIN;
@ -595,7 +592,6 @@ out:
static ssize_t spair_read(void *obj, void *buffer, size_t count)
{
int res;
int key;
bool is_connected;
size_t avail;
bool is_nonblock;
@ -610,10 +606,8 @@ static ssize_t spair_read(void *obj, void *buffer, size_t count)
goto out;
}
key = irq_lock();
is_nonblock = sock_is_nonblock(spair);
res = k_sem_take(&spair->sem, K_NO_WAIT);
irq_unlock(key);
is_nonblock = sock_is_nonblock(spair);
if (res < 0) {
if (is_nonblock) {
errno = EAGAIN;