Widen ifnet_detach_sxlock coverage

[kp]

Summary

Widen the ifnet_detach_sxlock to cover the entire vnet sysuninit code.
This ensures that we can't end up having the vnet_sysuninit free the UDP
pcb while the detach code is running and trying to purge the UDP pcb.

MFC after: 1 week

[kp]

Fix shared->exclusive lock order

[markj]

sys/net/vnet.c

288

This locking can trigger a deadlock: some vnet sysuninits will deregister eventhandlers, particularly from ifnet_arrival_event and ifnet_departure_event. In order to deregister an eventhandler, we have to remove it and wait for threads executing the event to drain. But, some handlers try to lock ifnet_detach_sxlock, so if they block on it, the thread destroying the VNET won't be able to make progress.

This very occasionally causes hangs when I run the test suite with parallelism enabled.

It feels wrong to acquire this lock here; individual SYSINITs can acquire it if they really need it. Do you remember the details of what goes wrong without this? I will try testing with the locking removed.

[kp]

sys/net/vnet.c

288

I'm afraid I don't remember details.

I assume (based on the commit message) that the worry was that udp_destroy() was destroying the V_udbinfo inpcbinfo while if_detach_internal() runs in_pcbpurgeif0(V_udbinfo) (via in_ifdetach()).

Perhaps the correct fix is to not take the ifnet_detach_sxlock here, but to have udp_destory() take the IN_MULTI_LOCK(). That covers the in_pcbpurgeif0() in in_ifdetach()

[zlei]

sys/net/vnet.c

288

This locking can trigger a deadlock: some vnet sysuninits will deregister eventhandlers, particularly from ifnet_arrival_event and ifnet_departure_event. In order to deregister an eventhandler, we have to remove it and wait for threads executing the event to drain.

I bet vnet_destroy() does not need to acquire ifnet_detach_sxlock.

For a vnet, typically,

1. The cloned interfaces will be destroyed via if_clone_detach() which will invoke if_detach() and acquire ifnet_detach_sxlock.
2. The vmove loaned interfaces will be returned via vnet_if_return() which invokes if_vmove() under the lock ifnet_detach_sxlock.

But, some handlers try to lock ifnet_detach_sxlock, so if they block on it, the thread destroying the VNET won't be able to make progress.

Can you elaborate which handlers try to lock ifnet_detach_sxlock ?

This very occasionally causes hangs when I run the test suite with parallelism enabled.

It feels wrong to acquire this lock here; individual SYSINITs can acquire it if they really need it. Do you remember the details of what goes wrong without this? I will try testing with the locking removed.

[markj]

sys/net/vnet.c

288

Can you elaborate which handlers try to lock ifnet_detach_sxlock ?

Actually, now I can't find one. :( I had a VM deadlocked with a thread holding the ifnet_detach_sxlock:

jail_remove taskq   mi_switch+0x26b sleepq_switch+0x18e _sleep+0x4dd _eventhandler_deregister+0x2c1 ipf_event_dereg+0x93 vnet_ipf_uninit+0xf7 vnet_destroy+0x233 prison_deref+0x1180 taskqueue_run_locked+0x3b9 taskqueue_thread_loop+0x138 fork_exit+0xa3 fork_trampoline+0xe

And I presumed that some thread running event handlers is blocked waiting for the detach lock, but I can't see it in procstat output. Nor can I spot it by code inspection. Maybe it's rtnl_handle_dellink(), but I don't know how that gets invoked. Unfortunately I don't have a vmcore to look at.

I still feel it's wrong to acquire the detach lock here; I was able to run the test suite to completion without it.

[zlei]

sys/net/vnet.c

288

So a thread is in the process of destroying a vnet, holding the lock ifnet_detach_sxlock and calling all vnet sysuninit functions, while

static void
_eventhandler_deregister(struct eventhandler_list *list, eventhandler_tag tag,
    bool wait)
{
...
    while (wait && list->el_runcount > 0)
            mtx_sleep(list, &list->el_lock, 0, "evhrm", 0);
    EHL_UNLOCK(list);
}

waiting for the condition list->el_runcount == 0.

The only consumer that increases el_runcount is the macro _EVENTHANDLER_INVOKE().

/*
 * Macro to invoke the handlers for a given event.
 */
#define _EVENTHANDLER_INVOKE(name, list, ...) do {                      \
        struct eventhandler_entry *_ep;                                 \
        struct eventhandler_entry_ ## name *_t;                         \
                                                                        \
        EHL_LOCK_ASSERT((list), MA_OWNED);                              \
        (list)->el_runcount++;                                          \
        KASSERT((list)->el_runcount > 0,                                \
            ("eventhandler_invoke: runcount overflow"));                \
        CTR0(KTR_EVH, "eventhandler_invoke(\"" __STRING(name) "\")");   \
        TAILQ_FOREACH(_ep, &((list)->el_entries), ee_link) {            \
                if (_ep->ee_priority != EHE_DEAD_PRIORITY) {            \
                        EHL_UNLOCK((list));                             \
                        _t = (struct eventhandler_entry_ ## name *)_ep; \
                        CTR1(KTR_EVH, "eventhandler_invoke: executing %p", \
                            (void *)_t->eh_func);                       \
                        _t->eh_func(_ep->ee_arg , ## __VA_ARGS__);      \
                        EHL_LOCK((list));                               \
                }                                                       \
        }                                                               \
        KASSERT((list)->el_runcount > 0,                                \
            ("eventhandler_invoke: runcount underflow"));               \
        (list)->el_runcount--;                                          \
        if ((list)->el_runcount == 0)                                   \
                eventhandler_prune_list(list);                          \
        EHL_UNLOCK((list));                                             \
} while (0)

So it must be one another thread is invoking the event handlers and wants to lock ifnet_detach_sxlock.

The event handler is either ifnet_arrival_event or probably ifnet_departure_event, as the former does not need ifnet_detach_sxlock.

A suspicious function is if_rename(). It invokes ifnet_departure_event handlers before renaming and ifnet_arrival_event handlers when done.
sys/netlink/route/iface.c registered ifnet_departure_event with the handler rtnl_handle_ifdetach() and it will unconditionally invoke NL_RTM_DELLINK command which appears wrong to me. I'm writing a test to verify that. Also CC @melifaro .

[zlei]

sys/net/vnet.c

288

_sleep+0x4dd _eventhandler_deregister+0x2c1

Possibly fixed by 735b16d490ae ( eventhandler: Fix a race when pruning eventhandlers ) ?

[markj]

sys/net/vnet.c

288

Oh, good memory. Yes that seems likely. It does still feel wrong to lock ifnet_detach_sxlock here, but, one step at a time...