Possible Weakness with locking in the GNRC network stack

[melshuber]

In the IPv6/6loWPAN stack there are sometimes statements like this: (e.g.: sys/net/gnrc/network_layer/ipv6/netif/gnrc_ipv6_netif.c:95ff)

        mutex_unlock(&entry->mutex);    /* function below relocks mutex */
        gnrc_ndp_internal_send_nbr_sol(entry->pid, &tmp_addr->addr, router, router);
        mutex_lock(&entry->mutex);      /* relock mutex */

Releasing the lock and relocking it again later however breaks the atomicity of those functions, because the memory which is protected by the lock might change while the lock is not held.

I propose two possible solutions:

1. implement local function for _send_nbr_sol which do not try to lock the mutex
2. add a reentrant mutex implementation

[miri64]

We talked already at the summit about this and I agree, that we need to fix this ASAP. I think the re-entrant mutexes would be the cleaner and in the end code-saving. @kaspar030 how hard would that be to realize?

[melshuber]

I also think reentrant mutex are the way to go. However i do not think that you should change the semantics of your current mutex implementation as this may break many other things. Further non-reentrant mutexes are more lightweight. May be it is better to add an rmutex module.

[kaspar030]

@melshuber With re-entrant you mean recursive, e.g., a thread locking a mutex twice doesn't block itself?

[melshuber]

yes - recursive is the correct terminus

[miri64]

I think they are both correct ;-) at least I've seen both in literature.

[miri64]

So the issue is two-fold now: (a) implementing re-entrant mutexes and (b) apply them everywhere it makes sense. @kaspar030 do you have capacities to do (a)? Then I would do (b) at least for the most parts in GNRC with the upcoming re-work of the network layer anyway.

[kaspar030]

Well, they're (fairly) easy to implement, but they would basically add (mutex-codesize + X).

Do I understand correctly that gnrc has a lot of shared memory (between threads) that the mutexes try to protect, and even one single thread might try to lock some of that memory twice, because the accesses are nested? Sounds scary.

[miri64]

Do I understand correctly that gnrc has a lot of shared memory (between threads) that the mutexes try to protect, and even one single thread might try to lock some of that memory twice, because the accesses are nested? Sounds scary.

Not a lot, just the data structures of the network layer that are also accessible to users (interfaces, neighbor cache, ...). The problem comes from the fact, that packets need to be send out if some things are changed (e.g. Router Advertisements when a new prefix is added). Since this happens has to happen while the mutex is locked (otherwise the address could be removed again making the sendout invalid), but sending a packet also involves operations on these data structures. It's not beautiful, I know and there are better ways to do it (which I intend with the promised overhaul), but I think re-entrant mutexes might be an interesting feature regardless.

How about instead of providing a whole new module with a lot of code duplication we just provide a new function mutex_lock_reent() to specificly lock recursively (might also be implementable externally using mutex_trylock())?

[kaspar030]

How about instead of providing a whole new module with a lot of code duplication we just provide a new function |mutex_lock_reent()| to specificly lock recursively (might also be implementable externally using |mutex_trylock()|)?

That could be a way. I'll try to come up with sth.

[melshuber]

@miri64: Just a thought - As RIOT is a Message passing OS why not try to put this paradigm onto the network stack.
Those management functions - read/modify neighbour cache, add/remove ip addressed, read/modify 6loctx, ... are called rarely from the outside and mostly not performance critical.
Why not make them accessible by the synchronous and or asynchronous API and get rid of the mutexes completely.

[miri64]

Might also be an idea. Indeed, I'm thinking about integrating more layers than just L2 into the usage GET/SET commands (which was the original idea). This part of netapi is currently dangerously underutilized ;-).

[jnohlgard]

There's an implementation proposal for a recursive mutex as part of the PR for newlib reentrancy #4529

https://github.com/RIOT-OS/RIOT/pull/4529/files#diff-8f48e1b9ed7a0a48d0c686a87cc5084eR35

[melshuber]

I have checked the implementation and it should work.
However is the atomic_inc/dec necessary? The lock is already/still held at this point.
A normal / non-atomic should do the job just as well.

[jnohlgard]

I agree regarding the atomic inc, but I think there is already another race between the check and update of the owner variable. Probably should be using atomic_cas in some form.

[jnohlgard]

@melshuber Do you have the time to adapt the code from #4529 into a new core/recursive_mutex component?

[melshuber]

Yes I will take a look, i hope can propose a pull request, by the end of next week.

[melshuber]

I have added a pull request for the port. See #5731

[kYc0o]

#5731 was merged.

[miri64]

#5731 did not fix this issue it just provided a new data type to fix it.

[kYc0o]

Ok, just changed the milestone since the fix seems to be hard.

[miri64]

It's not hard, its just a very intensive search-and-replace of mutexes to rmutexes with regards to gnrc_ipv6_netif ;-).

[miri64]

Fixed by #7925

[miri64]

This was fixed with the introduction of the new network interface API in #7925