Page MenuHomeFreeBSD

tun: VIMAGE fix for if_tun cloner
ClosedPublic

Authored by kristof on Feb 19 2019, 6:27 PM.

Details

Summary
tun: VIMAGE fix for if_tun cloner

The if_tun cloner is not virtualised, but if_clone_attach() does use a
virtualised list of cloners.
The result is that we can't find the if_tun cloner when we try to remove
a renamed tun interface. Virtualise the cloner, and move the final
cleanup into a sysuninit so that we're sure this happens after all of
the vnet_sysuninits

Note that we need unit numbers to be system-unique (rather than unique
per vnet, as is done by if_clone_simple()). The unit number is used to
create the corresponding /dev/tunX device node, and this node must match
with the interface.
Switch to if_clone_advanced() so that we have control over the unit
numbers.

Reproduction scenario:
        jail -c -n foo persist vnet
        jexec test ifconfig tun create
        jexec test ifconfig tun0 name wg0
        jexec test ifconfig wg0 destroy

PR:     235704

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

kristof created this revision.Feb 19 2019, 6:27 PM
kristof set the repository for this revision to rS FreeBSD src repository.

Note that the character device subsystem is not virtualized, so tunX must be system uniq.

Thanks. The original reporter of the bug discovered that too.

I'll see what I can do about that.

kristof updated this revision to Diff 54460.Feb 27 2019, 9:36 AM

Ensure that unit numbers are system-unique.

kristof updated this revision to Diff 54462.Feb 27 2019, 9:52 AM

Remove stray debug lines

Harbormaster completed remote builds in B22767: Diff 54462.
hselasky added inline comments.Feb 27 2019, 10:12 AM
sys/net/if_tun.c
182 ↗(On Diff #54462)

Technically you should check there is a digit after tun?

if (name[0] == 't' && name[1] == 'u' && name[2] == 'n' && isdigit(name[3]))

kristof added inline comments.Feb 27 2019, 1:33 PM
sys/net/if_tun.c
182 ↗(On Diff #54462)

That'd break ifconfig tun create.

hselasky added inline comments.Feb 27 2019, 1:45 PM
sys/net/if_tun.c
182 ↗(On Diff #54462)

So check for (name[3] == 0 || isdigit(name[3]))

strncmp() will match anything that starts with tun.

kristof added inline comments.Feb 27 2019, 1:50 PM
sys/net/if_tun.c
182 ↗(On Diff #54462)

That is a good idea. I'll update the patch.

kristof updated this revision to Diff 54481.Feb 27 2019, 2:28 PM

Fix tun_clone_match() to not match on e.g. tunnel.

hrs added a subscriber: hrs.Feb 27 2019, 2:37 PM
hrs added inline comments.
sys/net/if_tun.c
137 ↗(On Diff #54462)

This should be VNET_DEFINE_STATIC() instead of VNET_DEFINE().

206 ↗(On Diff #54462)

I think "%s%d", tunname, unit is better than a string literal "tun" here.

kristof updated this revision to Diff 54483.Feb 27 2019, 2:57 PM

Fix issues pointed out by hrs.

Harbormaster completed remote builds in B22784: Diff 54483.
kristof marked 4 inline comments as done.Feb 27 2019, 9:13 PM

If there are no objections I'm going to commit this soon.

hrs accepted this revision.Mar 4 2019, 6:43 PM

Looks good to me.

This revision is now accepted and ready to land.Mar 4 2019, 6:43 PM
hselasky accepted this revision.Mar 4 2019, 6:58 PM
bz added a comment.Mar 4 2019, 7:04 PM

I hate manually virtualised cloners but people changed if_clone.c logic under the virtualised cloner infrastructure and I gave up on this years ago. Sorry I am not really helpful here currently. Also I think it needs more words describing the problem and part of the solution as I cannot imagine this only being a problem of tun(4) left but probably another few others?

sys/net/if_tun.c
184 ↗(On Diff #54483)

Probably '\0'

This revision now requires review to proceed.Mar 5 2019, 9:23 AM
kristof edited the summary of this revision. (Show Details)Mar 5 2019, 9:24 AM
bz accepted this revision as: bz.Mar 5 2019, 9:34 AM

Thanks for updating the description; The repro scenario really should be a test case. Accept for the general idea, not for having checked the code in detail.

This revision is now accepted and ready to land.Mar 5 2019, 9:34 AM
This revision was automatically updated to reflect the committed changes.