Re: git: d461deeaa4a4 - main - VNET: Revert "ifnet: make if_index global"
- In reply to: Marko Zec : "git: d461deeaa4a4 - main - VNET: Revert "ifnet: make if_index global""
- Go to: [ bottom of page ] [ top of archives ] [ this month ]
Date: Thu, 05 May 2022 15:34:10 UTC
Marko,
There were ongoing discussions as to what to do about the issues around
VNETs when you made this unilateral commit. The discussions stopped shortly
after. The discussions had been productively headed towards identification
of the problems with the tree, what steps needed to be taken and how best
to proceed. The original commits had followed the project's processes and
gave everybody a chance to comment before they were committed. Only after
the fact did you raise an objection, and now many months have passed,
making this backout untimely.
In addition, after this series of commits, the tree was so functionally
broken that all VNET tests had to be disabled. This represents, in our
view, an unacceptably large blast radius to our testing infrastructure. We
believe that the changes needed to make Gleb's work acceptable will be much
smaller than redoing that work as well. We don't think that Gleb's work is
fundamentally flawed.
Given these circumstances, core views this series of commits as premature
and will be replaying Gleb's commits and re-enabling tests. We expect
Marko, Gleb and everybody else to work together towards the goal of
identifying actionable issues that remain in the tree after our restoration
and to work in good faith to resolve them. In addition, should people
believe that Gleb's direction is wrong and they want to start with the
state prior to the commit, they are welcome to develop their own solution
to present to the community with arguments about why it is better, and then
follow the normal collaberative process to win people over to that method.
Warner and Kyle on behalf of the core team
On Tue, May 3, 2022 at 11:29 AM Marko Zec <zec@freebsd.org> wrote:
> The branch main has been updated by zec:
>
> URL:
> https://cgit.FreeBSD.org/src/commit/?id=d461deeaa4a47ae71e1d8fda8b35c6faa8dabe85
>
> commit d461deeaa4a47ae71e1d8fda8b35c6faa8dabe85
> Author: Marko Zec <zec@FreeBSD.org>
> AuthorDate: 2022-05-03 14:57:55 +0000
> Commit: Marko Zec <zec@FreeBSD.org>
> CommitDate: 2022-05-03 17:27:57 +0000
>
> VNET: Revert "ifnet: make if_index global"
>
> This reverts commit 91f44749c6feb50f39af8805dd803e860f0418f1.
>
> Devirtualization of V_if_index and V_ifindex_table was rushed into
> the tree lacking proper context, discussion, and declaration of intent,
> so I'm backing it out as harmful to VNET on the following grounds:
>
> 1) The change repurposed the decades-old and stable if_index KBI for
> new, unclear goals which were omitted from the commit note.
>
> 2) The change opened up a new resource exhaustion vector where any vnet
> could starve the system of ifnet indices, including vnet0.
>
> 3) To circumvent the newly introduced problem of separating ifnets
> belonging to different vnets from the globalized ifindex_table, the
> author introduced sysctl_ifcount() which does a linear traversal over
> the (potentially huge) global ifnet list just to return a simple upper
> bound on existing ifnet indices.
>
> 4) The change effectively led to nonuniform ifnet index allocation
> among vnets.
>
> 5) The commit note clearly stated that the patch changed the implicit
> if_index ABI contract where ifnet indices were assumed to be starting
> from one. The commit note also included a correct observation that
> holes in interface indices were always allowed, but failed to declare
> that the userland-observable ifindex tables could now include huge
> empty spans even under modest operating conditions.
>
> 6) The author had an earlier proposal in the works which did not
> affect per-vnet ifnet lists (D33265) but which he abandoned without
> providing the rationale behind his decision to do so, at the expense
> of sacrificing the vnet isolation contract and if_index ABI / KBI.
>
> Furthermore, the author agreed to back out his changes himself and
> to follow up with a proposal for a less intrusive alternative, but
> later silently declined to act. Therefore, I decided to resolve the
> status-quo by backing this out myself. This in no way precludes a
> future proposal aiming to mitigate ifnet-removal related system
> crashes or panics to be accepted, provided it would not unnecessarily
> compromise the goal of as strict as possible isolation between vnets.
>
> Obtained from: github.com/glebius/FreeBSD/commits/backout-ifindex
> ---
> sys/net/if.c | 214
> +++++++++++++++++++++++++++++++++++------------------------
> 1 file changed, 129 insertions(+), 85 deletions(-)
>
> diff --git a/sys/net/if.c b/sys/net/if.c
> index 3b303fe42e99..de63b9366843 100644
> --- a/sys/net/if.c
> +++ b/sys/net/if.c
> @@ -311,30 +311,19 @@ VNET_DEFINE(struct ifnethead, ifnet); /* depend
> on static init XXX */
> VNET_DEFINE(struct ifgrouphead, ifg_head);
>
> /* Table of ifnet by index. */
> -static int if_index;
> -static int if_indexlim = 8;
> -static struct ifnet **ifindex_table;
> +VNET_DEFINE_STATIC(int, if_index);
> +#define V_if_index VNET(if_index)
> +VNET_DEFINE_STATIC(int, if_indexlim) = 8;
> +#define V_if_indexlim VNET(if_indexlim)
> +VNET_DEFINE_STATIC(struct ifnet **, ifindex_table);
> +#define V_ifindex_table VNET(ifindex_table)
>
> SYSCTL_NODE(_net_link_generic, IFMIB_SYSTEM, system,
> CTLFLAG_RW | CTLFLAG_MPSAFE, 0,
> "Variables global to all interfaces");
> -static int
> -sysctl_ifcount(SYSCTL_HANDLER_ARGS)
> -{
> - int rv = 0;
> -
> - IFNET_RLOCK();
> - for (int i = 1; i <= if_index; i++)
> - if (ifindex_table[i] != NULL &&
> - ifindex_table[i]->if_vnet == curvnet)
> - rv = i;
> - IFNET_RUNLOCK();
> -
> - return (sysctl_handle_int(oidp, &rv, 0, req));
> -}
> -SYSCTL_PROC(_net_link_generic_system, IFMIB_IFCOUNT, ifcount,
> - CTLTYPE_INT | CTLFLAG_VNET | CTLFLAG_RD, NULL, 0, sysctl_ifcount, "I",
> - "Maximum known interface index");
> +SYSCTL_INT(_net_link_generic_system, IFMIB_IFCOUNT, ifcount,
> + CTLFLAG_VNET | CTLFLAG_RD, &VNET_NAME(if_index), 0,
> + "Number of configured interfaces");
>
> /*
> * The global network interface list (V_ifnet) and related state (such as
> @@ -363,19 +352,13 @@ MALLOC_DEFINE(M_IFMADDR, "ether_multi", "link-level
> multicast address");
> struct ifnet *
> ifnet_byindex(u_int idx)
> {
> - struct ifnet *ifp;
>
> NET_EPOCH_ASSERT();
>
> - if (__predict_false(idx > if_index))
> + if (__predict_false(idx > V_if_index))
> return (NULL);
>
> - ifp = ck_pr_load_ptr(&ifindex_table[idx]);
> -
> - if (curvnet != NULL && ifp != NULL && ifp->if_vnet != curvnet)
> - ifp = NULL;
> -
> - return (ifp);
> + return (ck_pr_load_ptr(&V_ifindex_table[idx]));
> }
>
> struct ifnet *
> @@ -392,20 +375,63 @@ ifnet_byindex_ref(u_int idx)
> }
>
> /*
> - * Network interface utility routines.
> - *
> - * Routines with ifa_ifwith* names take sockaddr *'s as
> - * parameters.
> + * Allocate an ifindex array entry.
> */
> +static void
> +ifindex_alloc(struct ifnet *ifp)
> +{
> + u_short idx;
> +
> + IFNET_WLOCK();
> + /*
> + * Try to find an empty slot below V_if_index. If we fail, take
> the
> + * next slot.
> + */
> + for (idx = 1; idx <= V_if_index; idx++) {
> + if (V_ifindex_table[idx] == NULL)
> + break;
> + }
> +
> + /* Catch if_index overflow. */
> + if (idx >= V_if_indexlim) {
> + struct ifnet **new, **old;
> + int newlim;
> +
> + newlim = V_if_indexlim * 2;
> + new = malloc(newlim * sizeof(*new), M_IFNET, M_WAITOK |
> M_ZERO);
> + memcpy(new, V_ifindex_table, V_if_indexlim * sizeof(*new));
> + old = V_ifindex_table;
> + ck_pr_store_ptr(&V_ifindex_table, new);
> + V_if_indexlim = newlim;
> + epoch_wait_preempt(net_epoch_preempt);
> + free(old, M_IFNET);
> + }
> + if (idx > V_if_index)
> + V_if_index = idx;
> +
> + ifp->if_index = idx;
> + ck_pr_store_ptr(&V_ifindex_table[idx], ifp);
> + IFNET_WUNLOCK();
> +}
>
> static void
> -if_init(void *arg __unused)
> +ifindex_free(u_short idx)
> {
>
> - ifindex_table = malloc(if_indexlim * sizeof(*ifindex_table),
> - M_IFNET, M_WAITOK | M_ZERO);
> + IFNET_WLOCK_ASSERT();
> +
> + ck_pr_store_ptr(&V_ifindex_table[idx], NULL);
> + while (V_if_index > 0 &&
> + V_ifindex_table[V_if_index] == NULL)
> + V_if_index--;
> }
> -SYSINIT(if_init, SI_SUB_INIT_IF, SI_ORDER_SECOND, if_init, NULL);
> +
> +/*
> + * Network interface utility routines.
> + *
> + * Routines with ifa_ifwith* names take sockaddr *'s as
> + * parameters.
> + */
>
> static void
> vnet_if_init(const void *unused __unused)
> @@ -413,11 +439,29 @@ vnet_if_init(const void *unused __unused)
>
> CK_STAILQ_INIT(&V_ifnet);
> CK_STAILQ_INIT(&V_ifg_head);
> + V_ifindex_table = malloc(V_if_indexlim * sizeof(*V_ifindex_table),
> + M_IFNET, M_WAITOK | M_ZERO);
> vnet_if_clone_init();
> }
> VNET_SYSINIT(vnet_if_init, SI_SUB_INIT_IF, SI_ORDER_SECOND, vnet_if_init,
> NULL);
>
> +#ifdef VIMAGE
> +static void
> +vnet_if_uninit(const void *unused __unused)
> +{
> +
> + VNET_ASSERT(CK_STAILQ_EMPTY(&V_ifnet), ("%s:%d tailq &V_ifnet=%p "
> + "not empty", __func__, __LINE__, &V_ifnet));
> + VNET_ASSERT(CK_STAILQ_EMPTY(&V_ifg_head), ("%s:%d tailq
> &V_ifg_head=%p "
> + "not empty", __func__, __LINE__, &V_ifg_head));
> +
> + free((caddr_t)V_ifindex_table, M_IFNET);
> +}
> +VNET_SYSUNINIT(vnet_if_uninit, SI_SUB_INIT_IF, SI_ORDER_FIRST,
> + vnet_if_uninit, NULL);
> +#endif
> +
> static void
> if_link_ifnet(struct ifnet *ifp)
> {
> @@ -510,7 +554,6 @@ static struct ifnet *
> if_alloc_domain(u_char type, int numa_domain)
> {
> struct ifnet *ifp;
> - u_short idx;
>
> KASSERT(numa_domain <= IF_NODOM, ("numa_domain too large"));
> if (numa_domain == IF_NODOM)
> @@ -550,37 +593,7 @@ if_alloc_domain(u_char type, int numa_domain)
> ifp->if_get_counter = if_get_counter_default;
> ifp->if_pcp = IFNET_PCP_NONE;
>
> - /* Allocate an ifindex array entry. */
> - IFNET_WLOCK();
> - /*
> - * Try to find an empty slot below if_index. If we fail, take the
> - * next slot.
> - */
> - for (idx = 1; idx <= if_index; idx++) {
> - if (ifindex_table[idx] == NULL)
> - break;
> - }
> -
> - /* Catch if_index overflow. */
> - if (idx >= if_indexlim) {
> - struct ifnet **new, **old;
> - int newlim;
> -
> - newlim = if_indexlim * 2;
> - new = malloc(newlim * sizeof(*new), M_IFNET, M_WAITOK |
> M_ZERO);
> - memcpy(new, ifindex_table, if_indexlim * sizeof(*new));
> - old = ifindex_table;
> - ck_pr_store_ptr(&ifindex_table, new);
> - if_indexlim = newlim;
> - epoch_wait_preempt(net_epoch_preempt);
> - free(old, M_IFNET);
> - }
> - if (idx > if_index)
> - if_index = idx;
> -
> - ifp->if_index = idx;
> - ck_pr_store_ptr(&ifindex_table[idx], ifp);
> - IFNET_WUNLOCK();
> + ifindex_alloc(ifp);
>
> return (ifp);
> }
> @@ -650,18 +663,23 @@ if_free(struct ifnet *ifp)
> * epoch and then dereferencing ifp while we perform if_free(),
> * and after if_free() finished, too.
> *
> - * This early index freeing was important back when ifindex was
> - * virtualized and interface would outlive the vnet.
> + * The reason is the VIMAGE. For some reason it was designed
> + * to require all sockets drained before destroying, but not all
> + * ifnets. A vnet destruction calls if_vmove() on ifnet, which
> + * causes ID change. But ID change and a possible
> misidentification
> + * of an ifnet later is a lesser problem, as it doesn't crash
> kernel.
> + * A worse problem is that removed interface may outlive the vnet
> it
> + * belongs too! The if_free_deferred() would see ifp->if_vnet
> freed.
> */
> + CURVNET_SET_QUIET(ifp->if_vnet);
> IFNET_WLOCK();
> - MPASS(ifindex_table[ifp->if_index] == ifp);
> - ck_pr_store_ptr(&ifindex_table[ifp->if_index], NULL);
> - while (if_index > 0 && ifindex_table[if_index] == NULL)
> - if_index--;
> + MPASS(V_ifindex_table[ifp->if_index] == ifp);
> + ifindex_free(ifp->if_index);
> IFNET_WUNLOCK();
>
> if (refcount_release(&ifp->if_refcount))
> NET_EPOCH_CALL(if_free_deferred, &ifp->if_epoch_ctx);
> + CURVNET_RESTORE();
> }
>
> /*
> @@ -805,7 +823,7 @@ if_attach_internal(struct ifnet *ifp, bool vmove)
> struct sockaddr_dl *sdl;
> struct ifaddr *ifa;
>
> - MPASS(ifindex_table[ifp->if_index] == ifp);
> + MPASS(V_ifindex_table[ifp->if_index] == ifp);
>
> #ifdef VIMAGE
> ifp->if_vnet = curvnet;
> @@ -1255,6 +1273,17 @@ if_vmove(struct ifnet *ifp, struct vnet *new_vnet)
> if (rc != 0)
> return (rc);
>
> + /*
> + * Unlink the ifnet from ifindex_table[] in current vnet, and
> shrink
> + * the if_index for that vnet if possible.
> + *
> + * NOTE: IFNET_WLOCK/IFNET_WUNLOCK() are assumed to be
> unvirtualized,
> + * or we'd lock on one vnet and unlock on another.
> + */
> + IFNET_WLOCK();
> + ifindex_free(ifp->if_index);
> + IFNET_WUNLOCK();
> +
> /*
> * Perform interface-specific reassignment tasks, if provided by
> * the driver.
> @@ -1266,6 +1295,7 @@ if_vmove(struct ifnet *ifp, struct vnet *new_vnet)
> * Switch to the context of the target vnet.
> */
> CURVNET_SET_QUIET(new_vnet);
> + ifindex_alloc(ifp);
> if_attach_internal(ifp, true);
>
> #ifdef DEV_BPF
> @@ -1901,6 +1931,7 @@ ifa_ifwithnet(const struct sockaddr *addr, int
> ignore_ptp, int fibnum)
> struct ifaddr *ifa_maybe = NULL;
> u_int af = addr->sa_family;
> const char *addr_data = addr->sa_data, *cplim;
> + const struct sockaddr_dl *sdl;
>
> NET_EPOCH_ASSERT();
> /*
> @@ -1908,9 +1939,14 @@ ifa_ifwithnet(const struct sockaddr *addr, int
> ignore_ptp, int fibnum)
> * so do that if we can.
> */
> if (af == AF_LINK) {
> - ifp = ifnet_byindex(
> - ((const struct sockaddr_dl *)addr)->sdl_index);
> - return (ifp ? ifp->if_addr : NULL);
> + sdl = (const struct sockaddr_dl *)addr;
> + if (sdl->sdl_index && sdl->sdl_index <= V_if_index) {
> + ifp = ifnet_byindex(sdl->sdl_index);
> + if (ifp == NULL)
> + return (NULL);
> +
> + return (ifp->if_addr);
> + }
> }
>
> /*
> @@ -4546,16 +4582,24 @@ DB_SHOW_COMMAND(ifnet, db_show_ifnet)
>
> DB_SHOW_ALL_COMMAND(ifnets, db_show_all_ifnets)
> {
> + VNET_ITERATOR_DECL(vnet_iter);
> struct ifnet *ifp;
> u_short idx;
>
> - for (idx = 1; idx <= if_index; idx++) {
> - ifp = ifindex_table[idx];
> - if (ifp == NULL)
> - continue;
> - db_printf( "%20s ifp=%p\n", ifp->if_xname, ifp);
> - if (db_pager_quit)
> - break;
> + VNET_FOREACH(vnet_iter) {
> + CURVNET_SET_QUIET(vnet_iter);
> +#ifdef VIMAGE
> + db_printf("vnet=%p\n", curvnet);
> +#endif
> + for (idx = 1; idx <= V_if_index; idx++) {
> + ifp = V_ifindex_table[idx];
> + if (ifp == NULL)
> + continue;
> + db_printf( "%20s ifp=%p\n", ifp->if_xname, ifp);
> + if (db_pager_quit)
> + break;
> + }
> + CURVNET_RESTORE();
> }
> }
> #endif /* DDB */
>