if_ovpn: avoid LOR between ovpn and UDP locks

[kp]

Summary

When we install the tunneling function we had the ovpn lock, and then took the
UDP lock. During normal data flow we are called with the UDP lock held and then
take the ovpn lock.
This naturally produces a lock order reversal warning.

Avoid this by releasing the ovpn lock before installing the tunnel function.
This is safe, in that installing the tunnel function does not fail (other than
with EBUSY, which would mean another thread has already installed the function).

On cleanup the problem is more difficult, in that we cannot reasonably release
the ovpn lock before we can remove the tunneling function callback.

Solve this by delaying the removal of the tunnel callback until the ovpn_softc
is cleaned up. It's still safe for ovpn_udp_input() to be caled when all peers
are removed. That will only increment counters (which are still allocated),
discover there are no peers and then pass the message on to userspace, if
any userspace users of the socket remain.

We ensure that the socket object remains valid by holding an additional
reference, which we release when we remove the ovpn_softc.

Sponsored by: Rubicon Communications, LLC ("Netgate")

[zlei]

sys/net/if_ovpn.c

638–639

This extra ref make me crazy ;)

I'll look at this tomorrow.

[kp]

sys/net/if_ovpn.c

638–639

Yeah, perhaps I should remove the per-peer reference count for the socket entirely. The point of the new ref is to make sure the socket doesn't go away until we're ready to uninstall the tunnel callback, but that makes the other reference counting we do on the socket redundant.

[kp]

Remove per-peer socket reference counting.

[zlei]

This is much clear and looks good to me. ( I have not test this yet. )

sys/net/if_ovpn.c

641

I'm not sure, but it seems that setting up the kernel tunneling once ( when first time soref(so) ) is enough. Any reason to set the tunneling every time connecting to new peer ?

[kp]

sys/net/if_ovpn.c

641

Once would indeed be enough. The main reason to keep it this way is that we want to set up the tunnel function without the ovpn lock held, so we can't do it next to the soref().

We could do this, but I'm not sure if it's better:

diff --git a/sys/net/if_ovpn.c b/sys/net/if_ovpn.c
index e2c1dc7f7fc5..f1c2e1403d4b 100644
--- a/sys/net/if_ovpn.c
+++ b/sys/net/if_ovpn.c
@@ -511,6 +511,7 @@ ovpn_new_peer(struct ifnet *ifp, const nvlist_t *nvl)
        int fd;
        uint32_t peerid;
        int ret = 0;
+       bool setcb = false;

        if (nvl == NULL)
                return (EINVAL);
@@ -631,6 +632,7 @@ ovpn_new_peer(struct ifnet *ifp, const nvlist_t *nvl)
                 * we're destroying the ifp.
                 */
                soref(sc->so);
+               setcb = true;
        }

        /* Insert the peer into the list. */
@@ -638,9 +640,11 @@ ovpn_new_peer(struct ifnet *ifp, const nvlist_t *nvl)
        sc->peercount++;

        OVPN_WUNLOCK(sc);
-       ret = udp_set_kernel_tunneling(sc->so, ovpn_udp_input, NULL, sc);
-       MPASS(ret == 0 || ret == EBUSY);
-       ret = 0;
+
+       if (setcb) {
+               ret = udp_set_kernel_tunneling(sc->so, ovpn_udp_input, NULL, sc);
+               MPASS(ret == 0);
+       }

        goto done;

[zlei]

sys/net/if_ovpn.c

641

The logic setcb looks good to me. I think that can be put into a separate review. This review is mainly focusing on LOR.