From nobody Mon Jan 31 23:46:57 2022 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id CE3DE198212C; Mon, 31 Jan 2022 23:47:08 +0000 (UTC) (envelope-from zec@fer.hr) Received: from EUR04-HE1-obe.outbound.protection.outlook.com (mail-he1eur04on060d.outbound.protection.outlook.com [IPv6:2a01:111:f400:fe0d::60d]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "mail.protection.outlook.com", Issuer "DigiCert Cloud Services CA-1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4Jnl8r3Gprz3hDp; Mon, 31 Jan 2022 23:47:08 +0000 (UTC) (envelope-from zec@fer.hr) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=OW7MZpZXa8xfvx8rJmZAn3AU2XzjreZHlwaiv0wnFGG3lQkcatzEaFXRT4SRlILtzBE4DvzX0fpGyCmlyM0Jp8TKvmb+XykXD1BDsRuQblnjPYuVlE/hTxfsD7HBqrzZsPM/+ZZpe9QXov/DjJ+7Jdj+xIZ+PaP8h5kZtjDP3VbKCkSZR9DteMyXf91faKyS4kPCP1G880jLNCm0IrlKK8QaKz8b7LC3bsxpAg/W0dFogM5YN52/kgh8rxaPOKAIlA6aWEt96/mrZnvC3YRrN69NtMBlrpZS1q0/f9IScl8flbNAj6pxbnOCJm5fngkRVbPZc0gDnvqxIsC1MKddWw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=N7/oN3CvkpU6ca6XqqJTdYM4CrpprKXwq31U5HKRMOE=; b=bsYTNe/pWKRAV5XbZTgvxmQmActaQpcFOIczS8j7h13ZPj7U31v4pXm8bWlwg3nCWWNL7e3HnJsHzHk0zXdP6k4+rm/jpDIadcKSlIc6fawePnpFLRmoeYV/K/ltGXHyBPtkBqLGmQlqND6U6u3tGARhTpY9kB+dHdGFN9SAXLngDEfyiAez1GKp4NvBPotKJAOrFlwJkeJ01b9nXf5uer6+h9rqshIb62RRHvVJUs1O8XZl5kdUNRjeaYCqrUep5crhLhWQ+kKk//ZjfEzACWgdB4TTVrjM/ojW9/vNf5UqY3P6KLsu4UGWiYeeY+Irp3oyMEq0eTdTzV7f/mOpRw== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=none; dmarc=none; dkim=none; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ferhr.onmicrosoft.com; s=selector2-ferhr-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=N7/oN3CvkpU6ca6XqqJTdYM4CrpprKXwq31U5HKRMOE=; b=kJCbHuI2qgRHKzSFKIhe70dvmLJGYMQGQYFY89Pq8/MwP3q2FCzyMOPhvMufWdio80yaW/qvIy6b7DYQWBb9oNGYaaOlMjXmFwSkmB2JIBaAdZajydMGuhiZ/BxDsA8QYLzGVOkVVUm/opEiuQ8maxZk1PTo6oiz54Cwz8uF46E= Received: from VE1PR08MB4783.eurprd08.prod.outlook.com (2603:10a6:802:a9::16) by PAXPR08MB7383.eurprd08.prod.outlook.com (2603:10a6:102:22e::13) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4930.17; Mon, 31 Jan 2022 23:46:59 +0000 Received: from VE1PR08MB4783.eurprd08.prod.outlook.com ([fe80::f077:7274:ad5:303]) by VE1PR08MB4783.eurprd08.prod.outlook.com ([fe80::f077:7274:ad5:303%5]) with mapi id 15.20.4930.022; Mon, 31 Jan 2022 23:46:59 +0000 Date: Tue, 1 Feb 2022 00:46:57 +0100 From: Marko Zec To: Gleb Smirnoff Cc: src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: git: 91f44749c6fe - main - ifnet: make if_index global Message-ID: <20220201004657.0895641e@x25> In-Reply-To: References: <202201270600.20R601qd059593@gitrepo.freebsd.org> <20220129162034.6cda27df@x25> X-Mailer: Claws Mail 3.18.0 (GTK+ 2.24.33; amd64-portbld-freebsd13.0) Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-ClientProxiedBy: VE1PR08CA0006.eurprd08.prod.outlook.com (2603:10a6:803:104::19) To VE1PR08MB4783.eurprd08.prod.outlook.com (2603:10a6:802:a9::16) List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-all@freebsd.org X-BeenThere: dev-commits-src-all@freebsd.org MIME-Version: 1.0 X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: 35a3205c-d07f-4c1f-8af7-08d9e513f8c7 X-MS-TrafficTypeDiagnostic: PAXPR08MB7383:EE_ X-Microsoft-Antispam-PRVS: X-MS-Oob-TLC-OOBClassifiers: OLM:10000; X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: ViLnAL9DeKCTIGEKe8YzgyAkipsnoQiWUYz1gaLUypMzedx0Xa7jq8qHsSpyDUVjeAWxtUKZ8YlifX04sGUyvMuOIhcIs5XjBpzSbq6wWWIWcoeMaqWdLcA7NKnCDVmKtr1DIOBQ0qGNhgqlL9DFgry/ho8IgclcQ12kgTg/li4VUEShT2GHsuw72tt0iNrinv9aG88nzzarYUM6bu0mctz7G7zT3G/KN5uAHc7Lky5DuxSJeKGhskMN4MS5NdQdv4kwfb+TYvh6psontlQovPuFXUnsKOSscaBAGLFiLJhsLsu4+OHrLgrg8lOr9yWKTgDihQa9DEfW/8QiKzh4jlf9UNmwIcUGbulLkDts2mGqeVQnefn8m/X+yP3vnCjFL8dRR2WpvzmaBxNwRmGJbbq7+6aLvR5NMiR/S6yk+cxdL3/2loIqDw5YU3ZPQzoRMUgWDO1ic5h84wHie3nijy8bkyHs0Oo1KvtlzORqbjpjTxcVD+qKpMYG3byBr+rBaNRrbybs5fvirup/ho5zqFyUVRscDsq7DxgV16n0x8PeOlIROWVQdWnJ5vPv4iw0M9K5nDRtIXsF7abTy1oaZdaneVy0kW+ijRWLQYd2QHUo04qkOD9XT4OeGgQXvodEkeS67NJBBkQlpF8ICMTDGmcUmQ4n/pkmaLUBOUulxr1zz0w3KmSqDGdQOuTd3W0/Xz5ojj6ovXk0uoH0zzbWJfWKsDPJQ5CMckp2r2pKUvYRcA7IvGOUmIseM/TEguzcHGhJ2OAwPimpwkhAwDaU0rTlkaZpBgslNc8IBmqeEpY= X-Forefront-Antispam-Report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:VE1PR08MB4783.eurprd08.prod.outlook.com;PTR:;CAT:NONE;SFS:(13230001)(7916004)(366004)(6916009)(86362001)(38100700002)(38350700002)(450100002)(508600001)(8936002)(4326008)(8676002)(66476007)(66946007)(66556008)(316002)(33716001)(786003)(6486002)(966005)(26005)(83380400001)(9686003)(5660300002)(52116002)(6512007)(6506007)(2906002)(1076003)(186003)(20210929001);DIR:OUT;SFP:1101; X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?qqG778V1Sia3/EbfJaFf1eMGm27/M2306i5AY00FNb5GTehdxXevxgUQbLxK?= =?us-ascii?Q?R9Lo6iEqz+4DWzJqYiE8g63Hy0JPhoWBOSgdEA5RPWX6WEaLvSHq78SjrjcF?= =?us-ascii?Q?su/NILVwoGLxnZyNZzttyC7wq9QZjGFtYKIca7MN946efbz2di1BbidT4fXv?= =?us-ascii?Q?no9AEhAOm2dzGgVuqA2gksKnN8QvRb9v5oTkFbbSd75LQ8L3Nd6XmGwI7seA?= =?us-ascii?Q?6VHNlQrC0QgppR/h+REP+WAQUrc2F69ozdUgO0gIci3Cs2b0p6xDNAyNDbBj?= =?us-ascii?Q?VGl3Q5FEn97E6QPWUmB9QYfj/Jqf9x07q+GqLmOhGVMUAmuaRt3k0+PqX+Lb?= =?us-ascii?Q?+EfL1VrV584seOTtJpWv1eKB+goJ9iNO+OycRE2tj3JeDwS2cINYg5sJBWo8?= =?us-ascii?Q?SrcQbnY/9ynDHwCiUFQZflpi7QkscstPuKWmgqnelAgPrnpJMdfiMIl2+aeG?= =?us-ascii?Q?fLZPqwCno/2MckEm7hzl7MejWhDa6Hg36VN2RCxFReePp5Dsblecyyp8iW4+?= =?us-ascii?Q?73v7jkhgU8nizLShXqGJ6fMd+dxzhFiy8oD3rt1ejANI3+HDmdrF9oh/T5p+?= =?us-ascii?Q?WO6BchLBCmaWPJnVZqrS/OSHvh8Z3bRgD2rzhcSVVXsBoPnkpebpSNghaHMw?= =?us-ascii?Q?u7Z4i+jZy2SIcncv86njEeVnizKALxc/tkkGtGL7PT5up6YyXxtossf2pOVS?= =?us-ascii?Q?cELVIzIvVA0ENX62io70R5toOgGfnh6wbPV5f79ziikXaBjbsjz4i9gwYwOl?= =?us-ascii?Q?o1PJSKNmmuv54lEHKpkk8oradvQd0lOAWQzNphFmuBTEuhdi9SgXy6iS04hM?= =?us-ascii?Q?YffdlWEwoDD4CngDLcFVQwAsKDccqrTiF7Fh0d1++MH1wenrYpzUaFUJiB8B?= =?us-ascii?Q?vKQqAAjgt9ErfcnWqri4FvcwlDiOgJrgea4Ys3Y5H1otiq4GgJbsnwpLJtVb?= =?us-ascii?Q?1Hni22lInDwLfaIUjSFn/p6DD/IfrgVb9n/ZtxvaXDTJMR8Vd5HV1h20LLgs?= =?us-ascii?Q?nDGpufm2Xz0UGozVQ5SDAQoPIQsS8BONNIvdlA65JCxeCppJg4xXBUiqH8ff?= =?us-ascii?Q?5goZ7jsQQ6ZRbFWeM8wVyerwVTxBprj4Amcu6yiHNKNmNPOazxh0i+Fsdo/+?= =?us-ascii?Q?sZ7/Y5q2/8tBzRDEud80+uUj1CyAi2wHEbn58MBnGoSj+ExDMO3sDIjdfS9u?= =?us-ascii?Q?DpYoe9p1Xw35UHBYWZe9KJP9r0fqJAoRTaOta+785XV0aDzEkVqyzMYvICGW?= =?us-ascii?Q?eFEajlwB1dK1qCiMbfz3WDrAczC5yTdCmrI5BYlmmMuu/PEKCLhFp1KNxq7v?= =?us-ascii?Q?jjQD+1NX84xxEf90jZyq2B9QnHlwftJU/HNqlQ1enITFZPCvJ3cfnHtP9pZb?= =?us-ascii?Q?jNd5nBruKQbVdZFdekQtgEzyI8IRPSTIK8qRPcmnps2tFKl+w1v9l1VMMfaA?= =?us-ascii?Q?gN0A7uP+UP2R8h699fTAaQoQ4K72NAOPuYl19uARsh9ro0fLzNpXDL9XRais?= =?us-ascii?Q?2nsd4upVGnsyPAvd4RNSbKC5rc25ejfpV6jp7Rct4JkmW5ayHi1GFkQwixMV?= =?us-ascii?Q?3QAemzoRo2YyW53U9MU=3D?= X-OriginatorOrg: fer.hr X-MS-Exchange-CrossTenant-Network-Message-Id: 35a3205c-d07f-4c1f-8af7-08d9e513f8c7 X-MS-Exchange-CrossTenant-AuthSource: VE1PR08MB4783.eurprd08.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 Jan 2022 23:46:59.2320 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: ca71eddc-cc7b-4e5b-95bd-55b658e696be X-MS-Exchange-CrossTenant-MailboxType: HOSTED X-MS-Exchange-CrossTenant-UserPrincipalName: 06I2yBAPbkBbF+4kC4N1v57r3ILvCEFw5nBC2YbnFTVWGYW9bklF5QCHcvERu5hz X-MS-Exchange-Transport-CrossTenantHeadersStamped: PAXPR08MB7383 X-Rspamd-Queue-Id: 4Jnl8r3Gprz3hDp X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[] X-ThisMailContainsUnwantedMimeParts: N On Mon, 31 Jan 2022 11:33:08 -0800 Gleb Smirnoff 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/ >