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

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

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

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?

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

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

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.

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.

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?

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.

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:

What does downgrading the lock accomplish, specifically? If we do not downgrade, then gif_clone_dump_nl() will block on the ioctl lock until gif_clone_destroy() releases the lock. When it finally acquires the lock it'll observe if_softc == NULL and return. Can you provide more details on the crash or hang that you saw?

Basically, I am very skeptical that downgrading a lock can ever be the correct solution to a hang or crash. At best it is making the bug harder to hit.

netlink does it for us.

That is too late. It needs to be done before the task is scheduled. Look at do_link_state_change(): what is keeping the ifnet alive while it is running eventhandlers etc.?

What does downgrading the lock accomplish, specifically? If we do not downgrade, then gif_clone_dump_nl() will block on the ioctl lock until gif_clone_destroy() releases the lock. When it finally acquires the lock it'll observe if_softc == NULL and return. Can you provide more details on the crash or hang that you saw?

After moving if_link_state_change() out of gif_delete_tunnel(), I'm not able to reproduce that, not sure why, maybe that's enough and I was wrong.

Basically, I am very skeptical that downgrading a lock can ever be the correct solution to a hang or crash. At best it is making the bug harder to hit.

There are multiple benefits to downgrading the lock, particularly in the case of clone_destroy:

1. If we maintain the WLOCK until if_detach completes, do_link_state_change become pointless, because by the time software or other modules are notified, the interface is destroyed by if_detach() and simultaneously we have another notification by if_detach_internal to rtnl_handle_ifnet_event which says the interface is gone.
2. If we release the WLOCK only after setting ifp->if_softc to NULL, theoretically another thread might attempt an ifp-only modification (such as changing the address or mtu) simultaneously, and it might result in a panic.

Downgrading a lock prevents another thread to acquire an exclusive lock without pausing other read operations.
That's why I believe it's more safe and helpful.

netlink does it for us.

That is too late. It needs to be done before the task is scheduled. Look at do_link_state_change(): what is keeping the ifnet alive while it is running eventhandlers etc.?

The caller is responsible for this.
Now, due to our current implementation constraints it doesn't make sense to reference ifp during task_enqueue, so all drivers must ensure they call it at the appropriate time.

In contrast to if_gre, we actually can continue without lock downgrade, but I believe it could be unsafe to do so.

I will just re-iterate that downgrading a lock is never going to fix a race. At best it will hide one. The only time to downgrade a lock is in order to improve parallelism, and here that doesn't make sense since we're about to destroy the interface.

It is a bug that if_link_state_change() does not acquire an ifnet reference: it dispatches asynchronous work that runs in parallel with the caller. Either the caller must wait until the work is done before releasing its reference (in which case, why do the work asynchronously at all?), or the task must own its own reference to the ifnet.