Page MenuHomeFreeBSD

Summary: ng_l2tp race fix + ng_uncallout improvements
ClosedPublic

Authored by glebius on Aug 9 2021, 4:48 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 6, 2:45 PM
Unknown Object (File)
Wed, Oct 16, 5:33 PM
Unknown Object (File)
Oct 7 2024, 3:28 PM
Unknown Object (File)
Oct 7 2024, 3:28 PM
Unknown Object (File)
Oct 7 2024, 3:28 PM
Unknown Object (File)
Oct 7 2024, 3:28 PM
Unknown Object (File)
Oct 7 2024, 3:27 PM
Unknown Object (File)
Oct 7 2024, 2:55 PM

Details

Summary

This diff has 4 commits, 2 to the netgraph infrastructure, and 2
to ng_l2tp.

The goal was to fix PR 241133, which according to my experience isn't
fixed by e62e4b85942, just made less probably. The fix could have
been smaller (not touching ng_base), but I decided to add official
ng_uncallout_drain() and while here, decided to change the KPI to
pass rval from callout_stop() as is (something that bugged me for
years).

Commit #1:
netgraph: pass return value from callout_stop() unmodified to callers of ng_uncallout. Most of them do not check it anyway, so very little node changes are required.

Commit #2:
netgraph: add ng_uncallout_drain().

Move shared code into ng_uncallout_internal(). While here add a comment
mentioning a problem with scheduled+executing callout.

Commit #3:
ng_l2tp: improve seq structure locking.

Cover few cases of access to seq without lock missed in 702f98951d5.
There are no known bugs fixed with this change, however. With INVARIANTS
embed ng_l2tp_seq_check() into lock/unlock macros. Slightly reduce number
of locks/unlocks per packet keeping the lock between functions.

Commit #3:
ng_l2tp: improve callout locking.

Apparently e62e4b85942 wasn't enough to close the race between
a queue being flushed by a packet and callout executing, because
the callouts used without a lock aren't 100% bulletproof. To close
the race use callout_init_mtx() for L2TP timers, and make sure that
all calls to ng_callout()/ng_uncallout() are done under the seq lock.

If used properly, a locked callout can be used transparently with
old netgraph KPI of ng_callout/ng_uncallout which predates locked
callouts.

While here, utilize ng_uncallout_drain() instead of ng_uncallout()
on the node shutdown.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

sys/netgraph/ng_base.c
3822

IMO it would be nice to briefly explain what "returned 1" actually means.

3874

Is the return value really useful?

sys/netgraph/ng_l2tp.c
1250 ↗(On Diff #93446)

Doesn't the seq mtx need to be released here and in error paths?

sys/netgraph/ng_l2tp.c
1411 ↗(On Diff #93446)

This should also assert that seq->xwin[0] != NULL

1411 ↗(On Diff #93446)

Erm, I meant to comment on ng_l2tp_seq_rack_timeout

glebius marked 2 inline comments as done.

Address review comments.

glebius added inline comments.
sys/netgraph/ng_base.c
3874

I believe better allow it to be retrieved rather than hide it. Let the ng_callout/ng_uncallout be a fully transparent interface over callout(9). I can imagine a future node that would want rval.

sys/netgraph/ng_l2tp.c
1435 ↗(On Diff #94925)

missing func? I would just MPASS(seq->xwin[0] != NULL)

glebius marked an inline comment as done.

Address mjg's review.

This revision is now accepted and ready to land.Sep 9 2021, 7:28 PM