ifnet: Remove unreachable code

[zlei]

Summary

We're holding a reference to the valid prison, then vnet_alloc() has
finished and all VNET_SYSINIT has done. Meanwhile, for a still valid
prison the vnet_destroy() will not be invoked thus VNET_SYSUNINIT has
not chance to run. So it is not possible to see a VNET under constructing
or destructing.

No functional change intended.

MFC after: 2 weeks

[zlei]

I think I read the code carefully. Please do not hesitate to correct me if I missed anything important.

[glebius]

I'd suggest to add assertions that VNET is not shutting down.

[jamie]

This makes sense in if_vmove_reclaim, where the vnet comes from the held prison. But in if_vmove_loan, you're only holding the prison that will get the interface, not the one that currently has it. That wouldn't affect whether ifp currently belongs to a mid-shutdown vnet.

You get the child prison using td, and the current vnet comes from ifp. Do you know that ifp's vnet is the one tied to td? I suspect it is, but it's not clear just from the function itself. If they are known to be the same vnet, then you're good because having a hold on a child jail implies a hold on td's jail as well.

[zlei]

In D55828#1277269, @jamie wrote:

This makes sense in if_vmove_reclaim, where the vnet comes from the held prison.

Ah yes, for if_vmove_reclaim() it is clear the prison is held.

But in if_vmove_loan, you're only holding the prison that will get the interface, not the one that currently has it. That wouldn't affect whether ifp currently belongs to a mid-shutdown vnet.

The ifp is obtained from current prison / vnet. So VNET_IS_SHUTTING_DOWN(ifp->if_vnet) is indeed checking against the current vnet.

2779 /*
2780  * Interface ioctls.
2781  */
2782 int
2783 ifioctl(struct socket *so, u_long cmd, caddr_t data, struct thread *td)
2784 {
...
2937         ifp = ifunit_ref(ifr->ifr_name);
2938         if (ifp == NULL) {
2939                 error = ENXIO;
2940                 goto out_noref;
2941         }               
2942                             
2943         error = ifhwioctl(cmd, ifp, data, td);

You get the child prison using td, and the current vnet comes from ifp. Do you know that ifp's vnet is the one tied to td? I suspect it is, but it's not clear just from the function itself.

The td is from sys_socket(struct thread *td, struct socket_args *uap). I believe it is the current thread.

The entry of those two function if_vmove_loan() and if_vmove_reclaim() are for ioctl SIOCSIFVNET and SIOCSIFRVNET . They are for userland, typically

### SIOCSIFVNET if_vmove_loan
# ifconfig em0  vnet foo

### SIOCSIFRVNET if_vmove_reclaim
# ifconfig em0 -vnet foo

Both are operating in current thread.

If they are known to be the same vnet, then you're good because having a hold on a child jail implies a hold on td's jail as well.

[zlei]

In D55828#1277259, @glebius wrote:

I'd suggest to add assertions that VNET is not shutting down.

I though that a bit, and I think no need for the assertions.

Both SIOCSIFVNET and SIOCSIFRVNET are for userland only. Drivers want if_detach() and never call into those two functions, so I do not expect they will fail.

There's also a short-circuite in the entry ioctl(),

2779 /*
2780  * Interface ioctls.
2781  */
2782 int
2783 ifioctl(struct socket *so, u_long cmd, caddr_t data, struct thread *td)
2784 {
...
2806         CURVNET_SET(so->so_vnet);
2807 #ifdef VIMAGE
2808         /* Make sure the VNET is stable. */
2809         shutdown = VNET_IS_SHUTTING_DOWN(so->so_vnet);
2810         if (shutdown) { 
2811                 CURVNET_RESTORE();
2812                 return (EBUSY);
2813         }       
2814 #endif  
...
2884 #ifdef VIMAGE
2885         case SIOCSIFRVNET:
2886                 error = priv_check(td, PRIV_NET_SETIFVNET);
2887                 if (error == 0)
2888                         error = if_vmove_reclaim(td, ifr->ifr_name,
2889                             ifr->ifr_jid);
2890                 goto out_noref;
2891 #endif
...
2937         ifp = ifunit_ref(ifr->ifr_name);
2938         if (ifp == NULL) {
2939                 error = ENXIO;
2940                 goto out_noref;
2941         }
2942 
2943         error = ifhwioctl(cmd, ifp, data, td);
2944         if (error != ENOIOCTL)
2945                 goto out_ref;
...
}

2306 /*      
2307  * Hardware specific interface ioctls.
2308  */     
2309 int
2310 ifhwioctl(u_long cmd, struct ifnet *ifp, caddr_t data, struct thread *td)
2311 {
...
2589 #ifdef VIMAGE
2590         case SIOCSIFVNET:
2591                 error = priv_check(td, PRIV_NET_SETIFVNET);
2592                 if (error)
2593                         return (error);
2594                 error = if_vmove_loan(td, ifp, ifr->ifr_name, ifr->ifr_jid);
2595                 break;
2596 #endif
...
}

[glebius]

In D55828#1277277, @zlei wrote:

I though that a bit, and I think no need for the assertions.

So your point is that as we see that ifioctl() already did this check the assertions are not needed, as we already know that they will never fire. But that's the point of assertions - to put them so that they never fire :) Assertions also act as documentation.

I'm not insisting on assertions, just suggesting. My longer term plan is to get rid of if_vmove anyway.

[zlei]

The current logic of checking the src / dst vnet is a bit confusing. Refactor them a little. See D55832 .

[zlei]

In D55828#1277278, @glebius wrote:

In D55828#1277277, @zlei wrote:

I though that a bit, and I think no need for the assertions.

So your point is that as we see that ifioctl() already did this check the assertions are not needed, as we already know that they will never fire. But that's the point of assertions - to put them so that they never fire :) Assertions also act as documentation.

I'm not insisting on assertions, just suggesting. My longer term plan is to get rid of if_vmove anyway.

Ah, I have WIP to fix the race condition between if_detach() and if_vmove(). See PR 292993 .

This is a cleanup and preparing, mostly to make the if_vmove() logic clear.

[zlei]

In D55828#1277278, @glebius wrote:

In D55828#1277277, @zlei wrote:

I though that a bit, and I think no need for the assertions.

So your point is that as we see that ifioctl() already did this check the assertions are not needed, as we already know that they will never fire.

I have not checked throughly there're how many consumers of ifioctl(), so I have no clue why ifioctl() want VNET is stable. So I did not want to mention that ifioctl() in the commit message.

But that's the point of assertions - to put them so that they never fire :) Assertions also act as documentation.

I'm aware of that. My point is, for this case, the context is short, so the condition ( the ifp's vnet is stable ) is obvious, and then extra assertions would make people thinking , "Oh, is it really possible ? "

I'm not insisting on assertions, just suggesting. My longer term plan is to get rid of if_vmove anyway.

I think I have to revise the commit message a bit to be more clear.

[pouria]

In D55828#1277300, @zlei wrote:

Ah, I have WIP to fix the race condition between if_detach() and if_vmove(). See PR 292993 .

Oh, I was working on that problem too.
I thought you had seen D55777 earlier, you're on the reviewers list.
I can release the PR and assign it to you to avoid duplicating effort, if you want.

[glebius]

In D55828#1277603, @pouria wrote:

In D55828#1277300, @zlei wrote:

Ah, I have WIP to fix the race condition between if_detach() and if_vmove(). See PR 292993 .

Oh, I was working on that problem too.

IMHO, that's an endless pursuit to fix all problems around if_vmove. Better to focus on implementing proper full detach/attach of physical NICs to allow them to be properly moved into a VNET and provide extension for creation of clonable interfaces inside a jail.

[zlei]

In D55828#1277603, @pouria wrote:

In D55828#1277300, @zlei wrote:

Ah, I have WIP to fix the race condition between if_detach() and if_vmove(). See PR 292993 .

Oh, I was working on that problem too.
I thought you had seen D55777 earlier, you're on the reviewers list.
I can release the PR and assign it to you to avoid duplicating effort, if you want.

I'm currently focusing on the net stack. For ng_eiface(4) I think there's still room to improve.