Bug 2182541

Can you isolate the issue to a simple reproducer that I can use to further investigate?

If not can you explain the full steps so I can replicate this manually?

(In reply to Eelco Chaudron from comment #1)
> Can you isolate the issue to a simple reproducer that I can use to further
> investigate?
> 
> If not can you explain the full steps so I can replicate this manually?

I will try to find a simple reproducer and give you an answer next week.

Looking at the setup available I noticed that for some reason the lock is "unlocked" based on the OVS addition, but some threads are waiting for read/write access.

* 1    Thread 0x7f2775b5ec00 (LWP 2710833) 0x00007f27757364ff in pthread_rwlock_wrlock () from /lib64/libpthread.so.0
  3    Thread 0x7f277017b700 (LWP 2710838) 0x00007f2775735ec9 in pthread_rwlock_rdlock () from /lib64/libpthread.so.0
  40   Thread 0x7f27575fa700 (LWP 2710875) 0x00007f2775735ec9 in pthread_rwlock_rdlock () from /lib64/libpthread.so.0
  44   Thread 0x7f2756df6700 (LWP 2710879) 0x00007f2775735ec9 in pthread_rwlock_rdlock () from /lib64/libpthread.so.0
  48   Thread 0x7f27565f2700 (LWP 2710883) 0x00007f2775735ec9 in pthread_rwlock_rdlock () from /lib64/libpthread.so.0
  49   Thread 0x7f27563f1700 (LWP 2710884) 0x00007f2775735ec9 in pthread_rwlock_rdlock () from /lib64/libpthread.so.0

All are waiting for the same netdev_hmap_rwlock, lock. However, the lock looks free / but there are working readers (I do not know the internals of pthread).

(gdb) p netdev_hmap_rwlock
$3 = {lock = {__data = {__readers = 30, __writers = 0, __wrphase_futex = 2, __writers_futex = 1, __pad3 = 0, __pad4 = 0, __cur_writer = 0, __shared = 0, __rwelision = 0 '\000', __pad1 = "\000\000\000\000\000\000", __pad2 = 0, __flags = 2}, 
    __size = "\036\000\000\000\000\000\000\000\002\000\000\000\001", '\000' <repeats 35 times>, "\002\000\000\000\000\000\000", __align = 30}, where = 0x55f8b5f7b1fa "<unlocked>"}

Looked like to me there is some imbalance between lock/unlock. However, I could not find any in the code. I added some logging and I found some read-lock nesting:

     netdev_ports_flow_get()
       It takes the read lock, while it walks all the ports
       in the port_to_netdev hmap and calls:
       - netdev_flow_get() which will call:
         - netdev_tc_flow_get() which will call:
           - netdev_ifindex_to_odp_port()
              This function also takes the same read lock to
              walk the ifindex_to_port hmap.

However in theory this is not a problem, as you are allowed to take the read lock multiple times. I tried some simple code to see if it's a problem on the DUT, and it's not:

```
#include <pthread.h>
#include <stdio.h>
#include <unistd.h>

pthread_rwlock_t  rwlock = PTHREAD_RWLOCK_INITIALIZER;

void *read_thread_func(void *ptr)
{
    while(1) {
        pthread_rwlock_rdlock(&rwlock);
        pthread_rwlock_rdlock(&rwlock);
        pthread_rwlock_wrlock(&rwlock);
        printf("read_thread\n");
        pthread_rwlock_unlock(&rwlock);
        pthread_rwlock_unlock(&rwlock);
        pthread_rwlock_unlock(&rwlock);
        sleep(1);
    }
    return NULL;
}

int main()
{
    pthread_t read_thread;

    pthread_create(&read_thread, NULL, read_thread_func, NULL);

    while(1) {
        pthread_rwlock_wrlock(&rwlock);
        printf("a\n");
        pthread_rwlock_unlock(&rwlock);
        sleep(1);
    }
    return 0;
}
```

Just in case I split up the read lock in multiple locks to avoid the recursion of the read lock. I'll run it for some time to see if it fixes the problem or not.

oops, cut/paste an experiment code the while code should not have the write lock, so it should be:

while(1) {
        pthread_rwlock_rdlock(&rwlock);
        pthread_rwlock_rdlock(&rwlock);
        pthread_rwlock_rdlock(&rwlock);
        printf("read_thread\n");
        pthread_rwlock_unlock(&rwlock);
        pthread_rwlock_unlock(&rwlock);
        pthread_rwlock_unlock(&rwlock);
        sleep(1);
    }

(In reply to Eelco Chaudron from comment #7)

> All are waiting for the same netdev_hmap_rwlock, lock. However, the lock
> looks free / but there are working readers (I do not know the internals of
> thread).

Ignore this part, as this is not true. The 'where' value is reset on each unlock...

So the problem is with the way OVS initialises the rw locks. It's per design, see the ovs-thread.h file.

The problem happens when the recursive read lock happens when a write lock is waiting. The following code will cause the issue:

```
#include <pthread.h>
#include <stdio.h>
#include <unistd.h>

pthread_rwlock_t  rwlock = PTHREAD_RWLOCK_INITIALIZER;

void *read_thread_func(void *ptr)
{
    while(1) {
        printf("Getting first read lock\n");
        pthread_rwlock_rdlock(&rwlock);
        printf("Got first read lock!\n");
        sleep(10);
        printf("Getting second read lock\n");
        pthread_rwlock_rdlock(&rwlock);
        printf("Got second read lock!\n");
        sleep(5);
        printf("Unlock 2nd read lock\n");
        pthread_rwlock_unlock(&rwlock);
        printf("2nd read unlocked\n");
        printf("Unlock 1st read lock\n");
        pthread_rwlock_unlock(&rwlock);
        printf("1st read unlocked\n");
        sleep(1);
        break;
    }
    return NULL;
}

int main()
{
    int error;
    pthread_t read_thread;

    pthread_create(&read_thread, NULL, read_thread_func, NULL);

    pthread_rwlockattr_t attr;
    pthread_rwlockattr_init(&attr);
    pthread_rwlockattr_setkind_np(
        &attr, PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP);
    error = pthread_rwlock_init(&rwlock, &attr);
    pthread_rwlockattr_destroy(&attr);

    sleep(2);
    printf("Getting write lock\n");
    pthread_rwlock_wrlock(&rwlock);
    printf("Got write lock\n");
    sleep(1);
    printf("Unlock write lock\n");
    pthread_rwlock_unlock(&rwlock);
    printf("Write unlocked\n");
    sleep(5);
    return 0;
}
```

I will prepare an OVS patch to fix the problem.

A patch was sent upstream to address this problem:

https://patchwork.ozlabs.org/project/openvswitch/patch/168197633909.1845473.12868417804926632801.stgit@ebuild.local/

A v2 was sent for the #11 mentioned patch:

https://patchwork.ozlabs.org/project/openvswitch/patch/168364258097.4336.10159074711651103545.stgit@ebuild.local/

* Wed May 10 2023 Open vSwitch CI <ovs-ci> - 3.1.0-28
- Merging upstream branch-3.1 [RH git: d7b0d724f2]
    Commit list:
    9e27e8fe81 netdev-offload: Fix deadlock/recursive use of the netdev_hmap_rwlock rwlock. (#2182541)
    087439e416 ofproto-dpif-xlate: Fix use-after-free when xlate_actions().

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory (openvswitch3.1 bug fix and enhancement update), and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHBA-2023:3994