ng_l2tp: Fix synchronization with node shutdown or reset

[mjg]

Summary

ng_l2tp_seq_reset() clears sequence number state for a node but does so
without holding the associated mutex. It stops callouts first but
without the mutex this is racy. This can be triggered by node shutdown
or a NGM_L2TP_SET_CONFIG message with "enabled" set to 0.

Also modify the shutdown code to block until callouts are finished
executing before freeing memory. ng_uncallout() does not, and cannot,
cancel a running callout.

[donner]

sys/netgraph/ng_l2tp.c

660–661

ng_uncallout calls NG_FREE_ITEM(...->c_arg), too.
How is this handled now? By waiting for the callout to do it itself?

[markj]

sys/netgraph/ng_l2tp.c

660–661

Hmm, good question. This case is handled in the netgraph framework: when you use ng_callout(), a callout is scheduled using ng_callout_trampoline(), which uses a NGQF_FN message to execute the timeout handler in the context of a netgraph node. ng_apply_item() handles freeing the item after calling the function. So ng_uncallout() frees the item if it stopped the callout before it started executing, otherwise it is freed by the callout handler itself.

Note that ng_l2tp_seq_reset() calls ng_uncallout() already to stop the handlers. If that catches the handlers before they check callout_active() then they're ok. However, ng_l2tp_seq_rack_timeout() reschedules itself. So there is a (very narrow) race:

CPU 1:

• in ng_l2tp_seq_rack_timeout()
• reschedules the callout, allocating an item
• drops seq->mtx, but does not return yet

CPU 2:

• calls ng_l2tp_seq_reset(), calls ng_uncallout(&seq->rack_timer)
• callout_stop() returns 0 because rack_timer is still executing
• ng_uncallout() does not free the item allocated by CPU 1 above
• item is leaked

I will think about it some more, but I think this problem is general to ng_callout()/ng_uncallout().

[donner]

In order to prevent this race, the node should be know. that it's in shutdown. This way the callout can skip the reinvocation.
Another solution would be to free the c_arg argument manually.

[mjg]

Take the revision as agreed with markj.

[mjg]

D31476