if_gre(4): Fix races by changing initialization order and locks
AcceptedPublic
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 74111
Build 70994: 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
412	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
412	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)

ping

markj added inline comments.Mon, Jul 6, 2:30 PM

sys/net/if_gre.c
398	What if `sc == NULL`?
953	So now we're accessing several fields from the softc without any mutual exclusion, e.g., `gre_hlen`, `gre_options`. Are you sure they will not change over the course of this function? IMO it would be better to explicitly load them into local variables so that you have consistent values, e.g., `hlen = atomic_load_int(&sc->gre_hlen);`, etc.. I see now for instance that `gre_hlen` is probably constant after tunnel initialization, but it's still better to be explicit here IMO.
1009	Does this increment need to be atomic?
1175	Shouldn't vnet_gre_uninit() be responsible for setting `V_gre_cloner = NULL`?

Address @markj comments.

Harbormaster completed remote builds in B74637: Diff 181569.Wed, Jul 8, 7:48 PM

pouria added inline comments.Wed, Jul 8, 7:48 PM

sys/net/if_gre.c
953	So now we're accessing several fields from the softc without any mutual exclusion, e.g., `gre_hlen`, `gre_options`. Are you sure they will not change over the course of this function? IMO it would be better to explicitly load them into local variables so that you have consistent values, e.g., `hlen = atomic_load_int(&sc->gre_hlen);`, etc.. I see now for instance that `gre_hlen` is probably constant after tunnel initialization, but it's still better to be explicit here IMO. You're right, we probably should use rmlock like if_geneve/if_vxlan. However, IMHO, this change is out of scope this revision. Unless you argue that GRE_RLOCK_TRACKER macro should be replace with rmlock. :D I can revert this part if you want.
1009	Not sure about this one, it's 32-bit. `atomic(9)`: On all architectures supported by FreeBSD, ordinary loads and stores of integers in cache-coherent memory are inherently atomic if the integer is naturally aligned and its size does not exceed the processor's word size.
1175	Shouldn't vnet_gre_uninit() be responsible for setting `V_gre_cloner = NULL`? Looks like we missed it in D54175 too.

pouria mentioned this in D57375: routing: Subscribe nhops to ifnet link events.Wed, Jul 8, 8:10 PM

markj accepted this revision.Thu, Jul 9, 2:21 PM

markj added inline comments.

sys/net/if_gre.c
953	Oh, when I wrote that comment I didn't realize that GRE_RLOCK() expanded to epoch_enter(). :( Okay then..
1009	That paragraph does not apply to compound load-modify-store operations. I believe it is indeed wrong, though maybe the data path here is single-threaded for some reason. But anyway the code is buggy both before and after the change. :(

This revision is now accepted and ready to land.Thu, Jul 9, 2:21 PM

pouria added inline comments.Thu, Jul 9, 2:24 PM

sys/net/if_gre.c
412	@glebius ping on this comment :D
1009	Unfortunately... I'll try to fix those to in another revision.

pouria added inline comments.Tue, Jul 14, 12:39 PM

sys/net/if_gre.c
412	We add a reference to ifp during `modify_link` and `rtnl_handle_getlink` call in `sys/netlink/route/iface.c`: 484, 489, 634 and 644. It should be enough to delay `if_free` during `[gif\|gre]_clone_[modify\|dump]_nl`. Am I right?