Re: git: f4e35c044c89 - main - bus: Set the current VNET in device_attach()

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 <markj@FreeBSD.org>
>>>> AuthorDate: 2024-10-19 13:03:56 +0000
>>>> Commit:     Mark Johnston <markj@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