On Sat, 19 Oct 2024, Mark Johnston wrote:
On Sat, Oct 19, 2024 at 07:10:53PM +0300, Konstantin Belousov wrote:
On Sat, Oct 19, 2024 at 11:36:32AM -0400, John Baldwin wrote:
On 10/19/24 09:04, Mark Johnston wrote:
The branch main has been updated by markj:
URL:
https://cgit.FreeBSD.org/src/commit/?id=f4e35c044c8988b7452cefbdcc417f5fd723e021
commit f4e35c044c8988b7452cefbdcc417f5fd723e021
Author: Mark Johnston <ma...@freebsd.org>
AuthorDate: 2024-10-19 13:03:56 +0000
Commit: Mark Johnston <ma...@freebsd.org>
CommitDate: 2024-10-19 13:03:56 +0000
bus: Set the current VNET in device_attach()
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
Reviewed by: zlei, kevans, bz, imp, glebius
MFC after: 1 week
Differential Revision: https://reviews.freebsd.org/D47174
Hmm, there was some other review I thought that had a completely different
change.
That change removed all the vnet stuff from new-bus and instead handled it in
if.c. Specifically, that if_attach would set a default vnet to vnet0 if there
wasn't an active vnet at the time. See all the discussion in
https://reviews.freebsd.org/D42678 which has a patch that I think is correct
in the comments.
There it was; thanks I didn't misremeber but couldn't find it.
Gleb's proposal, described a bit in D47147, is to require device-based
ifnet drivers to fully detach themselves from the parent bus in order to
change VNETs. The intent is to eliminate the need for if_vmove() and
all the complexity it entails. This would also eliminate the need for a
"home" VNET, referenced in the patch that you reference here.
Will it?
I asked for a discussion elsewhere but it seems we are having it here now...
I am still inclined to ask:
- how do you want a vnet to attach an unknown to itself device? From
the outside?
- How to you pass it to a child-vnet without escalating priviledges to
outside of the host system (vnet0)?
- Is, e.g., a vcc device [CXGBE(4)] a physical interface?
- How do you pass a controlled set of other non-clonable devices in (or
did we get rid of them all)? The inital idea was still that the
"host" has somehow control over what child can create...
{ I recently tried tuntap in a vnet and it blew up badly by not going
away }
- exmaple: I really would love, e.g., a vlan interface to be passed to a
vnet but but not the pyhsical interface. Can we?
- How will we do with wlan interfaces (which are cloned) but may not own
the hardware (kind-of similar to the vcc example)? I know there are
special PRIV checks for those currently.
- how does a detach in a vnet work and where will the physical interface
re-appear for (automatic) attachment? just detached in that jail?
vnet0? the parent jail?
- what happens on vnet destroy? (same as last question)?
- Are we just going to build a vmove on a layer which doesn't have
anything to do with networking per-se as a special case for some
interfaces but not others?
In fact, I think that bus level is better. At least, I know that detach
also might need something by vnet (e.g. mce(4) needs to clear the IPSEC
offload database on detach, although it still does not do).
Shouldn't something like this be handled by
if_detach()/ether_ifdetach()? Posed another way, why does device detach
itself need to care about the VNET?
IPSec should probably be handled in the per-AF specific data of the
interface and be cleared up automatically. If that needs downcalls into
HW that's likely where they belong. It has nothing to do with ether(4);
wrong layer.
I tend to agree that having VNET knowledge in subr_bus.c is a hack. My
aim was just to fix the panic without making the hack worse.
It sounds as if bus_topo_lock() is the right place. May be some other
name for it is better, like bus_topo_changes_enter().
--
Bjoern A. Zeeb r15:7
