2024-11-22, 10:41:26 +0100, Antonio Quartulli wrote:
On 12/11/2024 14:20, Antonio Quartulli wrote: [...]
+static int ovpn_peer_del_nolock(struct ovpn_peer *peer, + enum ovpn_del_peer_reason reason) +{ + switch (peer->ovpn->mode) { + case OVPN_MODE_MP:
I think it would be nice to add
lockdep_assert_held(&peer->ovpn->peers->lock);
Sabrina, in other places I have used the sparse notation __must_hold() instead. Is there any preference in regards to lockdep vs sparse?
I could switch them all to lockdep_assert_held if needed.
__must_hold has the advantage of being checked at compile time (though I'm not sure it's that reliable [1]), so you don't need to run a test that actually hits that particular code path.
In this case I see lockdep_assert_held as mainly documenting that the locking that makes ovpn_peer_del_nolock safe (as safe as ovpn_peer_del) is provided by its caller. The splat for incorrect use on debug kernels is a bonus. Sprinkling lockdep_assert_held all over ovpn might be bloating the code too much, but I'm not opposed to adding them if it helps.
[1] I ran sparse on drivers/net/ovpn/peer.c before/after removing the locking from ovpn_peer_del and didn't get any warnings. sparse is good to detect imbalances (function that locks without unlocking), but maybe don't trust __must_hold for more than documenting expectations.
[note: if you end up merging ovpn->peers->lock with ovpn->lock as we've discussed somewhere else, the locking around keepalive and ovpn_peer_del becomes a bit less hairy]