bus: Properly set virtual network stack before trying to attach devices

[zlei]

Summary

This prevents page fault when trying to attach Ethernet devices.

PR: 275381
Reported by: khng
Fixes: 64de80195bba Add a new device control utility for new-bus devices called devctl
MFC after: 2 weeks

Test Plan

Boot with Ethernet interface disabled, then try to enable it.

> set hint.re.0.disabled="1"
> boot
...
# devctl enable re0

Or

# devctl disable re0
# kenv hint.re.0.disabled="1"
# devctl enable re0
# devctl enable re0 (YES, enable it twice)

[zlei]

Ping @jhb .

[jhb]

I suspect we need this more broadly. Several places in here can call attach. That said, I wonder if the better fix is that ether_ifattach needs to set a default vnet if one is not already set. Oddly, both if_alloc and if_attach set if_vnet which seems buggy. if_detach sets the current vnet to if_vnet before doing the meat of the work. I do think this should be managed down in the ifnet layer and not up here in devctl.

I think we should ask @bz, but I suspect what we want is something more like this:

• In if_alloc_domain, use vnet0 for if_vnet is curvnet is not set so that if_vnet is always valid.

• In if_attach, set the vnet to if_vnet similar to if_detach.

[zlei]

In D42678#976149, @jhb wrote:

I suspect we need this more broadly. Several places in here can call attach. That said, I wonder if the better fix is that ether_ifattach needs to set a default vnet if one is not already set.
Oddly, both if_alloc and if_attach set if_vnet which seems buggy. if_detach sets the current vnet to if_vnet before doing the meat of the work. I do think this should be managed down in the ifnet layer and not up here in devctl.

Agreed.
Indeed only network devices need this. I had an idea that checks class of device before set vnet0 but that seems to be layer violation, and also CURVNET_SET_QUIET can not be simply wrapped around by if condition.

This change is identical to that for device_probe_and_attach() by commit 719fb72517b7 and I would classify it as a quick and dirty fix.

Oddly, both if_alloc and if_attach set if_vnet which seems buggy.

I also noticed that when I was reviewing D36651. I think the right approach should be

--- a/sys/net/if.c
+++ b/sys/net/if.c
@@ -547,7 +547,7 @@ if_alloc_domain(u_char type, int numa_domain)
        ifp->if_alloctype = type;
        ifp->if_numa_domain = numa_domain;
 #ifdef VIMAGE
-       ifp->if_vnet = curvnet;
+       ifp->if_home_vnet = curvnet != NULL ? curvnet : vnet0;
 #endif
        if (if_com_alloc[type] != NULL) {
                ifp->if_l2com = if_com_alloc[type](type, ifp);
@@ -834,9 +834,7 @@ if_attach_internal(struct ifnet *ifp, bool vmove)
        MPASS(ifindex_table[ifp->if_index].ife_ifnet == ifp);
 
 #ifdef VIMAGE
-       ifp->if_vnet = curvnet;
-       if (ifp->if_home_vnet == NULL)
-               ifp->if_home_vnet = curvnet;
+       ifp->if_vnet = vmove ? curvnet : ifp->if_home_vnet;
 #endif
 
        if_addgroup(ifp, IFG_ALL);

I think we should ask @bz, but I suspect what we want is something more like this:

• In if_alloc_domain, use vnet0 for if_vnet is curvnet is not set so that if_vnet is always valid.

• In if_attach, set the vnet to if_vnet similar to if_detach.

Since 719fb72517b7 currently set curvnet during DEVICE_ATTACH then it could lead regression to narrow it down to if_attach() . Anyway I'll try this.

[bz]

I agree with @jhb that this shouldn't be in the bus layer.
The referenced changes are beyond me, so if you ask me, there may be follow-up work cleaning that up too.

I believe the 2nd half of the suggested patch in the comment depending on vmove is wrong but I probably have to go and figure out order of events and triggered by which as much as anyone else.
The basic idea in my mind should be: where you create the ifp, is where the home_vnet resides.
Some of this gives us funky options these days in that the base system can disable physical interfaces lent to a vnet-jail.
The home_vnet is always where the ifp should in the end return to. It was mostly added for physical interfaces which if destroyed would be gone and they had to be somewhere and vnet0 is where the hardware resources start and end. All the cloners followed. I guess we never asserted that they shouldn't be destroyed where they weren't create but there's still way too many cans of worms with interface names and uniqueness.

Happy someone will write test cases for all these things hopefully now; could have saved us some trouble during the last decade and a bit.

[jhb]

In D42678#976697, @zlei wrote:

In D42678#976149, @jhb wrote:

I suspect we need this more broadly. Several places in here can call attach. That said, I wonder if the better fix is that ether_ifattach needs to set a default vnet if one is not already set.
Oddly, both if_alloc and if_attach set if_vnet which seems buggy. if_detach sets the current vnet to if_vnet before doing the meat of the work. I do think this should be managed down in the ifnet layer and not up here in devctl.

Agreed.
Indeed only network devices need this. I had an idea that checks class of device before set vnet0 but that seems to be layer violation, and also CURVNET_SET_QUIET can not be simply wrapped around by if condition.

This change is identical to that for device_probe_and_attach() by commit 719fb72517b7 and I would classify it as a quick and dirty fix.

Oddly, both if_alloc and if_attach set if_vnet which seems buggy.

I also noticed that when I was reviewing D36651. I think the right approach should be

--- a/sys/net/if.c
+++ b/sys/net/if.c
@@ -547,7 +547,7 @@ if_alloc_domain(u_char type, int numa_domain)
        ifp->if_alloctype = type;
        ifp->if_numa_domain = numa_domain;
 #ifdef VIMAGE
-       ifp->if_vnet = curvnet;
+       ifp->if_home_vnet = curvnet != NULL ? curvnet : vnet0;
 #endif
        if (if_com_alloc[type] != NULL) {
                ifp->if_l2com = if_com_alloc[type](type, ifp);
@@ -834,9 +834,7 @@ if_attach_internal(struct ifnet *ifp, bool vmove)
        MPASS(ifindex_table[ifp->if_index].ife_ifnet == ifp);
 
 #ifdef VIMAGE
-       ifp->if_vnet = curvnet;
-       if (ifp->if_home_vnet == NULL)
-               ifp->if_home_vnet = curvnet;
+       ifp->if_vnet = vmove ? curvnet : ifp->if_home_vnet;
 #endif
 
        if_addgroup(ifp, IFG_ALL);

I think we should ask @bz, but I suspect what we want is something more like this:

• In if_alloc_domain, use vnet0 for if_vnet is curvnet is not set so that if_vnet is always valid.

• In if_attach, set the vnet to if_vnet similar to if_detach.

Since 719fb72517b7 currently set curvnet during DEVICE_ATTACH then it could lead regression to narrow it down to if_attach() . Anyway I'll try this.

I think we should revert the subr_bus.c part of 719fb72517b7 along with fixing this in the ifnet layer. I think your change to if.c looks like a good start (and it honestly looks fine to me, but I'm less familiar with if_vmove). I'm not sure if you will also need a CURVNET_SET()/RESTORE inside of if_attach_internal after setting if_vnet?