bus: Set the current VNET in device_attach()
ClosedPublic
Actions

Authored by markj on Fri, Oct 18, 1:25 PM.

Details

Reviewers

jhb
imp
bz
kevans
glebius
zlei

Group Reviewers

network

Commits

rGf4e35c044c89: bus: Set the current VNET in device_attach()

Summary

Some drivers, in particular anything which creates an ifnet during
attach, need to have the current VNET set, as if_attach_internal() and
its callees access VNET-global variables.

device_probe_and_attach() handles this, but this is not the only way to
arrive in DEVICE_ATTACH. In particular, bus drivers may invoke
device_attach() directly, as does devctl2's DEV_ENABLE ioctl handler.
So, set the current VNET in device_attach() instead.

I believe it is always safe to use vnet0, as devctl2 ioctls are not
permitted within a jail.

PR: 282168

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

markj created this revision.Fri, Oct 18, 1:25 PM

Herald added a subscriber: olce. · View Herald TranscriptFri, Oct 18, 1:25 PM

markj requested review of this revision.Fri, Oct 18, 1:25 PM

Harbormaster completed remote builds in B60063: Diff 145105.Fri, Oct 18, 1:25 PM

While the change seems fine (hence accepted) some people always found it questionable that the device framework should care about VNETs and in some ways this seems to be wrong.

The problem, I believe, is that we do not have further abstractions for networking-related drivers which could in that case be used as a handler for setting/unsetting the vnet and pushing this into 40-ish (just guessed) drivers seems not right either... Probably a separate discussion to be had on net@ ?

This revision is now accepted and ready to land.Fri, Oct 18, 1:46 PM

kevans added a subscriber: kevans.Fri, Oct 18, 1:59 PM

kevans added inline comments.

sys/kern/subr_bus.c
2578	Seems like`TD_TO_VNET(curthread)` might be a slightly better way to spell the same thing, just in case

In D47174#1075587, @bz wrote:

While the change seems fine (hence accepted) some people always found it questionable that the device framework should care about VNETs and in some ways this seems to be wrong.

The problem, I believe, is that we do not have further abstractions for networking-related drivers which could in that case be used as a handler for setting/unsetting the vnet and pushing this into 40-ish (just guessed) drivers seems not right either... Probably a separate discussion to be had on net@ ?

I'm not sure what abstractions you are suggesting? iflib happens to solve this problem already. I don't see any solution that doesn't involve churning all drivers though.

sys/kern/subr_bus.c
2578	I somewhat prefer to be explicit about it here, since we shouldn't be attaching devices from within a jail. Probably we should assert `IS_DEFAULT_VNET(TD_TO_VNET(curthread))` is true?

kevans added inline comments.Fri, Oct 18, 2:31 PM

sys/kern/subr_bus.c
2578	+1

Use TD_TO_VNET(curthread); add an assertion.

This revision now requires review to proceed.Fri, Oct 18, 2:47 PM

Harbormaster completed remote builds in B60066: Diff 145109.Fri, Oct 18, 2:47 PM

Thanks!

This revision is now accepted and ready to land.Fri, Oct 18, 2:54 PM

I never liked the vnet stuff in newbus. So finding another way to deal with it would be ideal.
However, that's a somewhat large delta from where we are today, and beyond the scope of this fix.
The delta in scope of vnet shouldn't matter at all: resource_disabled() doesn't use it, nor does the device_sysctl_update() routine that I could see.
So this does make it better, I think.

sys/kern/subr_bus.c
2578	If we are attaching from within a jail, then this is a panic service. None of the drivers attached at boot will be doing that, nor will the interrupt-driven or proc-deferred ones (since we don't put them in a jail today). So that leaves kldload from inside a jail (which is prohibited) and devctl, which I think disallows /dev/devctl2 in jails, so I think this is a good assert (eg, should never happen).

markj added inline comments.Fri, Oct 18, 3:14 PM

sys/kern/subr_bus.c
2578	Yes, devctl2 ioctls are not permitted in jails, they require PRIV_DRIVER.

lexi_le-fay.org added a subscriber: lexi_le-fay.org.Fri, Oct 18, 4:14 PM

Add a missing include.

This revision now requires review to proceed.Fri, Oct 18, 4:34 PM

Harbormaster completed remote builds in B60068: Diff 145113.Fri, Oct 18, 4:34 PM

emaste added a subscriber: emaste.Fri, Oct 18, 5:45 PM

I'm fine with this change, it tells it explicitly that attach is VNET-aware and currently is supposed to happen in the default vnet only.

I have a plan, that we discussed together with @melifaro and @imp, that we should get rid of if_vmove() and moving physical interfaces to vnet jail should happen with a full detach & attach. In case you want certain physical interface to be in a vnet jail right at the boot time, that would be specified by kenv. The work on this project hasn't started yet.

This revision is now accepted and ready to land.Fri, Oct 18, 7:35 PM

In D47174#1075792, @glebius wrote:

I'm fine with this change, it tells it explicitly that attach is VNET-aware and currently is supposed to happen in the default vnet only.

I have a plan, that we discussed together with @melifaro and @imp, that we should get rid of if_vmove() and moving physical interfaces to vnet jail should happen with a full detach & attach. In case you want certain physical interface to be in a vnet jail right at the boot time, that would be specified by kenv. The work on this project hasn't started yet.

This needs a different discussion elsewhere; please Cc: me on it.

bz accepted this revision.Fri, Oct 18, 11:29 PM