ifnet_byindex() actually requires network epoch
ClosedPublic
Actions

Authored by glebius on Dec 4 2021, 9:16 PM.

Details

Reviewers

bz
kp
melifaro

Group Reviewers

network
transport

Commits

rGd74b7baeb0d4: ifnet_byindex() actually requires network epoch
rG7e0bba4d8083: ifnet: make V_if_index static to if.c

Summary

Sweep over potentially unsafe calls to ifnet_byindex() and wrap them
in epoch. Most of the code touched remains unsafe, as the returned
pointer is being used after epoch exit. Mark that with a comment.

Validate the index argument inside the function, reducing argument
validation requirement from the callers and making V_if_index
private to if.c.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 43142
Build 40030: arc lint + arc unit

Event Timeline

glebius created this revision.Dec 4 2021, 9:16 PM

Herald added a reviewer: transport. · View Herald TranscriptDec 4 2021, 9:16 PM

Herald added subscribers: melifaro, ae, imp. · View Herald Transcript

glebius requested review of this revision.Dec 4 2021, 9:16 PM

Harbormaster completed remote builds in B43142: Diff 99451.Dec 4 2021, 9:16 PM

glebius added a parent revision: D33262: ifnet: merge ifindex_alloc(), ifnet_setbyindex(), if_grow() and call magic.Dec 4 2021, 9:16 PM

glebius added a child revision: D33264: ifnet: make V_if_index static to if.c.

melifaro accepted this revision as: melifaro.Dec 4 2021, 11:40 PM

melifaro added inline comments.

sys/net/if.c
346	Worth considering using `uint` (or even `uint32_t`) as an index identifier. It'll allow to avoid unnecessary (idx < 0 checks).
sys/netinet/igmp.c
512	Maybe worth moving it to out_locked: , so the future readers won't have to check whether ifp is indeed not used below?
sys/netinet6/mld6.c
359	Worth removing?
sys/netinet6/scope6.c
377	What do you think of having an explicit bool ifnet_checkindex(unit idx) { struct epoch_tracket et; NET_EPOCH_ENTER(et); bool result = ifnet_byindex(idx) != NULL; NET_EPOCH_EXIT(et); } ?

kp added inline comments.Dec 6 2021, 9:32 AM

sys/net/if.c
346	ifp->if_index is an unsigned int, so it makes sense to also use that here. (As an aside, struct in6_defrouter and struct in6_prefix have it as a u_short, and probably need to be fixed. That might be painful, as they're shared with user space.)

glebius marked 2 inline comments as done.Dec 6 2021, 5:11 PM

glebius added inline comments.

sys/netinet/igmp.c
512	Not sure. SYSCTL_OUT() potentially may sleep. It won't sleep in this function, since we wire the buffer. But that may change. Anyway this function has its own problems, like unlocked access to the igi_head. I'd better not include improvements to particular protocols into this sweeping ifnet_byindex() KPI change.
sys/netinet6/scope6.c
377	Don't know much about IPv6. Would it be correct to set sin6_scope_id to non-existent index, freed a moment ago? Cause this can happen now. If this needs to be fixed, then once it is fixed, such function ifnet_checkindex() would be not used.