wg: don't unlink an unlinked peer

[kevans]

Summary

This can happen in the case of pretty much any failure in wg_peer_add(),
we end up underflowing sc_peers_num and doing bad things with the tailq
because peers are only accounted for and added at the very end of
wg_peer_add().

Just add a parameter to wg_peer_destroy() to indicate whether the peer
was actually linked in or not, so that we can avoid botching the
accounting. The other parts of wg_peer_destroy() are still generally
applicable.

This path is somewhat hard to exercise since wg(8) does validation of
most things that can cause a failure, so no test is added. The easiest
reproducer is to build a kernel with INET removed and configure a peer
with an IPv4 allowed-ip.

[aly_aaronly.me]

Hi. Sorry for my delayed reply.

I actually noticed this issue (inconsistency between sc_peers_num and sc_peers TAILQ) and I have a TODO to work on a patch for FreeBSD.

In DragonFly, I fixed the issue in another way (by referring to OpenBSD's) as you may find in commit: https://github.com/DragonFlyBSD/DragonFlyBSD/commit/902964ab24ba9d2c978017d369c0faa8d2fe0f9e

(I found the issue when I was working on replacing nvlist with ioctl.)

[markj]

In D46670#1064123, @aly_aaronly.me wrote:

Hi. Sorry for my delayed reply.

I actually noticed this issue (inconsistency between sc_peers_num and sc_peers TAILQ) and I have a TODO to work on a patch for FreeBSD.

In DragonFly, I fixed the issue in another way (by referring to OpenBSD's) as you may find in commit: https://github.com/DragonFlyBSD/DragonFlyBSD/commit/902964ab24ba9d2c978017d369c0faa8d2fe0f9e

(I found the issue when I was working on replacing nvlist with ioctl.)

I think your approach is nicer. (In general I kind of dislike using bool flags to modulate functions this way: it's hard to see what's going on without jumping to the function definition, and the control flow becomes more complicated. Here it's relatively straightforward, but refactoring a bit is nicer IMHO.)

[kevans]

In D46670#1064616, @markj wrote:

In D46670#1064123, @aly_aaronly.me wrote:

Hi. Sorry for my delayed reply.

I actually noticed this issue (inconsistency between sc_peers_num and sc_peers TAILQ) and I have a TODO to work on a patch for FreeBSD.

In DragonFly, I fixed the issue in another way (by referring to OpenBSD's) as you may find in commit: https://github.com/DragonFlyBSD/DragonFlyBSD/commit/902964ab24ba9d2c978017d369c0faa8d2fe0f9e

(I found the issue when I was working on replacing nvlist with ioctl.)

I think your approach is nicer. (In general I kind of dislike using bool flags to modulate functions this way: it's hard to see what's going on without jumping to the function definition, and the control flow becomes more complicated. Here it's relatively straightforward, but refactoring a bit is nicer IMHO.)

+1- I'm intending to port that over here because that seems like the much better approach in hindsight, but I haven't found time just yet.