Page MenuHomeFreeBSD

ng_l2tp: Fix synchronization with node shutdown or reset
AbandonedPublic

Authored by mjg on Sep 29 2020, 1:24 PM.
Tags
None
Referenced Files
F99241267: D26586.id.diff
Mon, Oct 7, 3:25 PM
F99241256: D26586.id77629.diff
Mon, Oct 7, 3:25 PM
Unknown Object (File)
Mon, Oct 7, 2:54 PM
Unknown Object (File)
Thu, Sep 26, 9:31 AM
Unknown Object (File)
Thu, Sep 26, 9:31 AM
Unknown Object (File)
Thu, Sep 26, 9:20 AM
Unknown Object (File)
Wed, Sep 11, 2:29 PM
Unknown Object (File)
Sep 5 2024, 9:18 AM

Details

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.

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 33875
Build 31083: arc lint + arc unit

Event Timeline

markj requested review of this revision.Sep 29 2020, 1:24 PM
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?

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().

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 added a reviewer: markj.
mjg added a subscriber: mjg.

Take the revision as agreed with markj.