if_gre(4): Fix races by changing initialization order and locks
Needs ReviewPublic
Actions

Authored by pouria on Fri, Jun 19, 3:30 PM.

Details

Reviewers

melifaro
glebius
markj
ae

Group Reviewers

network

Summary

Treat if_gre like any other network drivers during module
initialization by using SI_SUB_PROTO_IF.
Also, destroy cloned interfaces via a prison removal callback for
gre over udp.

PR: 275474

Test Plan

# kyua test -k /usr/tests/Kyuafile sys/net/if_gre
sys/net/if_gre:gre_blind_options  ->  passed  [0.660s]
sys/net/if_gre:gre6_udpencap  ->  passed  [0.799s]
sys/net/if_gre:gre6_basic  ->  passed  [0.819s]
sys/net/if_gre:gre4_basic  ->  passed  [0.818s]
sys/net/if_gre:gre6_csum  ->  passed  [0.822s]
sys/net/if_gre:gre6_seq  ->  passed  [0.799s]
sys/net/if_gre:gre6_key  ->  passed  [0.825s]

Results file id is usr_tests.20260619-152458-418771
Results saved to /root/.kyua/store/results.usr_tests.20260619-152458-418771.db

7/7 passed (0 broken, 0 failed, 0 skipped)
# kyua test -k /usr/tests/Kyuafile
sys/netlink/test_rtnl_gre
sys/netlink/test_rtnl_gre:test_rtnl_gre  ->  passed  [0.008s]

Results file id is usr_tests.20260619-152826-808921
Results saved to
/root/.kyua/store/results.usr_tests.20260619-152826-808921.db

1/1 passed (0 broken, 0 failed, 0 skipped)

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 74007
Build 70890: arc lint + arc unit

Event Timeline

pouria created this revision.Fri, Jun 19, 3:30 PM

Herald added a subscriber: imp. · View Herald TranscriptFri, Jun 19, 3:30 PM

pouria requested review of this revision.Fri, Jun 19, 3:30 PM

Harbormaster completed remote builds in B74007: Diff 180068.Fri, Jun 19, 3:30 PM

pouria added a child revision: D55363: if_gre: Add a regression test.Fri, Jun 19, 3:40 PM

I'd recommend to split this to multiple commits. For example the removal of GRE_RLOCK() / GRE_RUNLOCK() in gre_transmit() is an optimization and does not look like a FIX.

In D57669#1322708, @zlei wrote:

I'd recommend to split this to multiple commits. For example the removal of GRE_RLOCK() / GRE_RUNLOCK() in gre_transmit() is an optimization and does not look like a FIX.

Hi, this is actually a fix.
During gre_clone_destroy(), we call gre_delete_tunnel() to delete the tunnel, and that generates an ifnet_link_event.
That event eventually calls gre_clone_dump_nl(), and that function tries to read gre_softc while we are destroying the softc itself.

We are not triggering it now because I didn't commit the D55366 review due to the PR275474 issue.
This patch fixes both issues at once.
I can separate these changes into different reviews, but they can't be tested independently.

glebius added inline comments.Sun, Jun 21, 12:46 PM

sys/net/if_gre.c
409	That's an interesting non-standard trick, but it has a race. This thread may proceed with if_free() before the dump_nl thread is scheduled to obtain the shared lock and read data out from ifp.

pouria added inline comments.Sun, Jun 21, 5:15 PM

sys/net/if_gre.c
409	Right, but this is also true for entire `dump_iface()` in netlink. (`sys/netlink/route/iface.c:292`) We should enter epoch in that function then. Should I do it?

Use more appropriate SI_SUB_PROTO_IF order and destroy cloned
interfaces via a prison removal callback for gre over udp.

Harbormaster completed remote builds in B74111: Diff 180321.Mon, Jun 22, 7:15 PM

pouria retitled this revision from if_gre(4): Change domain and don't use epoch for control-plane to if_gre(4): Fix races by changing initialization order and locks.Mon, Jun 22, 7:16 PM

pouria edited the summary of this revision. (Show Details)