Re: git: 91f44749c6fe - main - ifnet: make if_index global

On Mon, 31 Jan 2022 11:33:08 -0800
Gleb Smirnoff <glebius@freebsd.org> wrote:

>   Marko,
> 
> On Sat, Jan 29, 2022 at 04:20:34PM +0100, Marko Zec wrote:
> M> >     ifnet: make if_index global
> M> >     
> M> >     Now that ifindex is static to if.c we can unvirtualize it.
> M> > For lifetime of an ifnet its index never changes.  To avoid
> M> > leaking foreign interfaces the net.link.generic.system.ifcount
> M> > sysctl and the ifnet_byindex() KPI filter their returned value
> M> > on curvnet.  Since if_vmove() no longer changes the if_index,
> M> > inline ifindex_alloc() and ifindex_free() into if_alloc() and
> M> > if_free() respectively. 
> M> >     API wise the only change is that now minimum interface index
> M> > can be greater than 1.  
> M> 
> M> IMO this is a huge change in an API which has been stable like
> M> forever. Was this tested with a broader spectrum of userland
> M> consumers, such as FRR & alike?  
> 
> What exactly is FRR?

https://frrouting.org/

> If we see there is an expectation from some
> software that index shall start at 1, I will rethink this change. So
> far there is no such evidence.

The implicit API contract for the past 40 years or so was that lo0
is always present exactly at ifindex 1.  Your change broke that contract
for all vnets except for vnet0.  Saying that there's no evidence
software outside our src tree takes that API contract for granted, while
claiming you have no idea what FRR is, doesn't sound reasurring.

> M> > The holes in interface indexes were always allowed.  
> M> 
> M> True, the holes were formally allowed but were never huge, and now
> M> they will be guaranteed with #vnets > 1.  Again, that's not
> M> something 3rd party tools would expect.  
> 
> Well "huge" or "small" doesn't make difference for algorithms. Holes
> are allowed.
> 
> M> What could be the repercussions of ifindex globalization to 16-bit
> M> size of ifru_index in struct ifreq?
> M> What can prevent a vnet to inflate the global ifindex space,
> M> causing the linear scan in sysctl_ifcount() to become a global
> M> problem?  
> 
> We don't think that virtualization of if_index was there to save
> if_index space, was it?  I think the reason was just cause it was
> easier.

That's a non-answer to the question I asked, so let me restate it: any
vnet can now DoS vnet0 (and / or any / all other vnets) via if_index
exhaustion, so what exactly did we gain in exchange for having that new
attack vector wide open?

And no, if_index virtualization was not done because it was "easier"
(than what?), it was done in order to make all vnets equal, and to
ensure no state leaks through if_index space.  Your commit violated
both of those fundamental VNET concepts.

> M> Why do we now need to check wheter curvnet != NULL in
> M> ifnet_byindex(), i.e. are there callers to ifnet_byindex() where
> M> curvnet is not set?  If so, that smells like a bug in the caller?  
> 
> Good point. Maybe an assertion should be put there.

Now I really don't get it, if you don't expect curvnet to be NULL in
ifnet_byindex(), why can't you resolve ifindex in per-VNET ifnet table?
And why do you check for curvnet != NULL then?

> M> Or, is it the other way around, that ifnet_byindex() is now
> M> intended to become a silent workaround for callers which lack
> M> curvnet context?  If that is so, it is indeed becoming a major
> M> VNET KPI feature (though I'd call it a bug), and as such, it
> M> should have been clearly stated in both the commit message, and as
> M> a comment in the ifnet_byindex() code.
> M> 
> M> Finally, the commit message does not reveal any clues what this
> M> change actually tries to accomplish.  The claim "Now that ifindex
> M> is static to if.c we can unvirtualize it." tell us nothing about
> M> the true intent.  
> 
> Well, the intent is revealed in the following commits, that use index
> as the only true stable & unique identifier of an interface except its
> pointer.

Nowhere is it explained _why_ exactly you need this extra true stable &
unique identifier besides ifnet ptr?  If there's a mbuf lingering
somewhere which points to an ifnet long after that ifnet is gone (such
as when a USB dongle suddenly gets unpluged), that's a bug in the queue
/ code which holds such a mbuf / reference, and that has to be resolved
right there where it happens.  How is ifindex globalization going to
help us with disappearing interfaces (USB, vlan, ng_eiface etc.) besides
making if_vmove() of epairs less prone to panics, which seems to be in
your current focus?

> I have had a previous version, that was maintaining two indexes a
> virtual and non-virtual:
> 
> https://reviews.freebsd.org/D33265
> 
> If you ask me of a perfect version of this design, I would say same
> thing I already said in other topics: if_vmove shall not exist.

If if_vmove() did not exist, how would we move real ifnets (such as
em / bge / igb), or vlan cloners attached to those, from vnet0 to
another vnet, and vice versa?

> If if_vmove didn't exist, a virtual index would provide stable and
> unique identifier for an interface. If interfaces were tied to their
> IFNET for their lifetime, so less problems would exist.

If if_vmove() didn't exist, there would be no VNET / VIMAGE, since use
cases for it would almost cease to exist, except for isolated synthetic
topology tests.

> I already lost count of problems created by if_vmove :(

Again, could you pls. clearly state / elaborate on those problems, so
that we can discuss possible avenues of resolving them, instead of
forcing an arbitrary change which violates the VNET foundations of
isolation and functional equality among all vnets.

Please back this out and let's start from scratch with describing the
real problem you want to solve.

> M> Sorry for chiming in late, but my sentiment against this change is
> M> obvious...  
> 
> I'm also sorry for not including you in the review. Please add
> yourself to https://reviews.freebsd.org/project/members/24/
>