if_gif: Add netlink support with tests

[pouria]

Summary

Migrate to new if_clone KPI and implement netlink support
for gif(4). Also break GIFSOPTS ioctl logic out of gif_ioctl.

Test Plan

kyua test -k /usr/tests/Kyuafile sys/net/if_gif 
kyua test -k /usr/tests/Kyuafile sys/netlink/test_rtnl_gif

[pouria]

ping @glebius @markj

[markj]

sys/net/if_gif.c

293

Don't we need to use a lock and check sc != NULL?

335

Same here, we should verify sc != NULL first, or prove it's not possible after all.

[pouria]

Address @markj comments.

[markj]

sys/net/if_gif.c

106

Why introduce a macro? Other operations on git_ioctl_sx just use the sx(9) interface directly.

349

I don't think this really makes sense: the interface is about to go away, so why try to optimize for other threads accessing it?

700

Why do we no longer bring the interface down in this case?

[pouria]

sys/net/if_gif.c

106

Simply because other drivers are implemented similarly.
I can remove it.

349

It's not an optimization.
We need this one because if_link_state_change (and there are more) will try to notify other threads that this interface is going down.
Without this, either dump_nl causes a panic or gets stuck.

700

We will do this in gif_clone_destroy after setting ifp->if_sofc to null and downgrading our lock to ensure dump_nl returns early.

[markj]

sys/net/if_gif.c

349

What exactly is happening? Your explanation doesn't make sense to me, it sounds like there is a deeper bug and downgrading the lock just happens to hide it.

Eh, ok, so further below we release our ifp reference, but meanwhile, the link state change is being handled asynchronously, and there is nothing ensuring that the ifp won't be freed. There is even a comment in if_link_state_change() which points out the bug: /* XXXGL: reference ifp? */.

Downgrading the lock just happens to hide the bug if you're lucky, but that's not the right solution.

What's the purpose of bringing the interface down to begin with if we're about to destroy the interface?

[pouria]

sys/net/if_gif.c

349

Thank you for your review.

What exactly is happening? Your explanation doesn't make sense to me, it sounds like there is a deeper bug and downgrading the lock just happens to hide it.

Yes, there is a deeper problem, and no, I'm not trying to hide it by downgrading.
I've done the same in D57669.

Here is the problem:
If anyone tries to dump/modify the interface using netlink (async) during clone_destroy, there is a gap where the softc is freed but the interface still exists.
Netlink acquires a reference to ifp during modify_link and rtnl_handle_getlink calls in sys/netlink/route/iface.c: 484, 489, 634, and 644 to ensure ifp won't get freed until it's done, and then it calls the [dump|modify]_nl.

Drivers can ensure the softc pointer is consistent using rmlock/rwlock like geneve(4), or they can downgrade their lock (which I'm proposing) like below:

gif_clone_dump_nl()
...
	sx_slock(&gif_ioctl_sx);
	sc = ifp->if_softc;
	if (sc == NULL)
		goto ret;

Eh, ok, so further below we release our ifp reference, but meanwhile, the link state change is being handled asynchronously, and there is nothing ensuring that the ifp won't be freed. There is even a comment in if_link_state_change() which points out the bug: /* XXXGL: reference ifp? */.

netlink does it for us.

Downgrading the lock just happens to hide the bug if you're lucky, but that's not the right solution.

What's the purpose of bringing the interface down to begin with if we're about to destroy the interface?

It's not a problem of if_link_state_change() because anyone can call dump_nl in another thread at the same time.
The purpose of if_link_state_change() in every interface driver is to notify that the interface is going down, which gives time to other software/modules to prepare themselves.