Paths

Table of Contentst

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

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

Details

Reviewers

jhb
imp
bz
kevans
glebius
zlei

Group Reviewers

network

Commits

rG48c59140bd7d: bus: Set the current VNET in device_attach()
rG2e80ea70b98e: bus: Set the current VNET in device_attach()
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 Skipped

Unit

Tests Skipped

Build Status

Buildable 60066
Build 56950: arc lint + arc unit

Event Timeline

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

Herald added a subscriber: olce. · View Herald TranscriptOct 18 2024, 1:25 PM

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

Harbormaster completed remote builds in B60063: Diff 145105.Oct 18 2024, 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.Oct 18 2024, 1:46 PM

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

kevans added inline comments.

sys/kern/subr_bus.c
2577	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
2577	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.Oct 18 2024, 2:31 PM

sys/kern/subr_bus.c
2577	+1

Use TD_TO_VNET(curthread); add an assertion.

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

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

Thanks!

This revision is now accepted and ready to land.Oct 18 2024, 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
2577	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.Oct 18 2024, 3:14 PM

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

ivy added a subscriber: ivy.Oct 18 2024, 4:14 PM

Add a missing include.

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

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

emaste added a subscriber: emaste.Oct 18 2024, 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.Oct 18 2024, 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.Oct 18 2024, 11:29 PM