From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: passt.top; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=eBLaeYBp; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by passt.top (Postfix) with ESMTPS id 7B46B5A0265 for ; Fri, 14 Aug 2026 23:43:32 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1786743811; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=UU3bBNwp0e1CRUd5DTLZkqbZwFTVBvG4AJ5h7s4GXyQ=; b=eBLaeYBpg2ZSKC+1UUHiMnX7TOFiVO+XUdS+2/04JZHx7o8pEfwUjJ0n7TSjaER0OiD4dO ZYXk+egwRe+JOaFh+8ue2soRMpsVm7nAaAx/ctwA7XFaunEzVnm+rMAtXBFgcCeZb+WhAw wDkNUGi06bnlpOSqRzLZFxGHpeSvW+A= Received: from mail-wm1-f71.google.com (mail-wm1-f71.google.com [209.85.128.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-314-vDDnjoEfOhuSchVdzjyLng-1; Fri, 14 Aug 2026 17:43:25 -0400 X-MC-Unique: vDDnjoEfOhuSchVdzjyLng-1 X-Mimecast-MFC-AGG-ID: vDDnjoEfOhuSchVdzjyLng_1786743804 Received: by mail-wm1-f71.google.com with SMTP id 5b1f17b1804b1-490a767c7dcso11968475e9.2 for ; Fri, 14 Aug 2026 14:43:24 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1786743804; x=1787348604; h=date:content-transfer-encoding:content-type:mime-version :organization:references:in-reply-to:message-id:subject:cc:to:from :x-gm-gg:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to:content-type; bh=yCQ/cztPu4Wi8WpXDpOfxzRvJMTRJ08ovXCTW9ur9wY=; b=W1bNydo3WbUI4P6+DWBvfHOPZyPST31cmbSuB42Oyw6LEw+TzepmzeLq5Bu3jXL6AV 37fpzK3qdYJaqNkKwQSVwcjxwlBPWoSyUVkM1fiAOkIGpTEjqSzxlBRHumBiF0+sTTwc 0/tTAeyvc+MCRP6oZ2tLez1gERug68JkRhf37neW8vKka/WRt+OEaMHTneuYv6LCWaqw oFfkLDvC1ortfRKVHulteSuwhfXmG4jdCPWTDiOS4pToJh2hil+lK912kwTwezKtWFuY P75JPLc23P8G0MnkHqYAopyaEUSSLW6FuNCBhI1IhbqvLpL5/6cK/DkrMjzz4Z4BOdpk KIyA== X-Forwarded-Encrypted: i=1; AHgh+Rqn6gTvm3DddGXVGJnSxY+Y2l+Pegaf7RvuTb3dgtppWDQ6DBeR9Xa2iKOjoTFOZN4sjjHjXaf0Two=@passt.top X-Gm-Message-State: AOJu0YyBpahv7jW96E64kM6tEIvdWe2UE9ccW54bX0JfsWJGt3h4S0qp aRgLvZqb88uQC4dhoC708NSKYOjWpj+mW/Etapom2Qiqj70Yer47wnE/pENl+I1rFZZGwvptjbC cjG4kseWEg6eS1UOftFNoeMiJrC4spMj5ru9WueQFQIxzkssV0MYH0w== X-Gm-Gg: AR+sD11VgTVcmTm+zV8mLKJKGnzWCpEXtGRrA+Q4ZmF34Hsf8yrhzJIA70EVdZMJM/e xhaGRHXBx9+fBg2s9Iph+56BflvCt3dBNQ7nhnPbtl+43y4j0AzXA/2HUGm6eIFTUM1Quqac5oH WlMA/kQmeN+ha/Oy7jwitBlo7EWp7/GS+c/RnzWJvhFBwwAMspOx6zPs8Qx/JJFuHeSAgxnzlmW AsjgeEwWQD2Ws19dEJ6961AnwE/CP7qJD2j0G9FgJC3fuwTuiApc/S8zhel3VR226t0rVtQcQmc T6xBgCxfjso12lfnCZWJoBEQv48n3Fn+a61rw+aM5u5we1UF4yQAzbVoqV+SVPvq3huYb3jwk5r d6vR/G/lz88M= X-Received: by 2002:a05:600c:1906:b0:499:8467:3f2d with SMTP id 5b1f17b1804b1-499879b611dmr97669875e9.19.1786743803659; Fri, 14 Aug 2026 14:43:23 -0700 (PDT) X-Received: by 2002:a05:600c:1906:b0:499:8467:3f2d with SMTP id 5b1f17b1804b1-499879b611dmr97669605e9.19.1786743803065; Fri, 14 Aug 2026 14:43:23 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [2a10:fc81:a806:d6a9::1]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-4815f219d3dsm11620503f8f.14.2026.08.14.14.43.19 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 14 Aug 2026 14:43:20 -0700 (PDT) From: Stefano Brivio To: "Lawrence, Richard E" Subject: Re: [PATCH v2] feat: Pass open files to child in pasta mode Message-ID: <20260814234318.1696362b@elisabeth> In-Reply-To: References: <20260731132601.422518-1-rlawrence@tamu.edu> <20260806235422.5d6d8fd3@elisabeth> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.49; x86_64-pc-linux-gnu) MIME-Version: 1.0 Date: Fri, 14 Aug 2026 23:43:19 +0200 (CEST) X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: Volall-CYDpStwrEljhrZPaHWfEMujhEtMMaj4Dm_lM_1786743804 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Message-ID-Hash: AJU3E3KII64VZFMVNSB4DDB5Q6P3ATHC X-Message-ID-Hash: AJU3E3KII64VZFMVNSB4DDB5Q6P3ATHC X-MailFrom: sbrivio@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: David Gibson , "passt-dev@passt.top" X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Hi Richard, On Fri, 14 Aug 2026 18:12:56 +0000 "Lawrence, Richard E" wrote: > Howdy Stefano, David, >=20 > I don't understand the plan. Are you waiting on me? Am I waiting on you? >=20 > I could implement any of these ideas, but I don't wish to implement **all= ** of them. Can we reach a consensus? Sorry for the confusion and the lack of updates. David mentionen, in the latest weekly call (see also https://pad.passt.top/p/weekly), that he plans to look into a way to avoid closing files we open altogether, but in a more lightweight way than the approaches I was suggesting, that is, he wanted to look into whether we can reshuffle the order of some initial operations instead. If that works out, then we should be able to use just the command line part of your patch. If not, I guess it might make sense to *also* look into the approaches I suggested (any comments from your side?) and reach a conclusion. But maybe we don't have to go there at all. --=20 Stefano > ________________________________ > From: Stefano Brivio > Sent: Thursday, August 6, 2026 4:54 PM > To: David Gibson > Cc: Lawrence, Richard E ; passt-dev@passt.top > Subject: Re: [PATCH v2] feat: Pass open files to child in pasta mode >=20 > A couple of ideas, rigorously not tested: On Thu, 6 Aug 2026 13:=E2=80=8A= 20:=E2=80=8A27 +1000 David Gibson wrote: > On Mon, Aug 03, 2026 at 08:=E2=80=8A34:= =E2=80=8A19PM +0000, Lawrence, Richard E wrote: > > Howdy David, > > > > ZjQcmQRYFpfptBannerStart > This Message Is From an External Sender > This message came from outside your organization. >=20 > ZjQcmQRYFpfptBannerEnd >=20 > A couple of ideas, rigorously not tested: >=20 > On Thu, 6 Aug 2026 13:20:27 +1000 > David Gibson wrote: >=20 > > On Mon, Aug 03, 2026 at 08:34:19PM +0000, Lawrence, Richard E wrote: = =20 > > > Howdy David, > > > > > > Why enumerate inherited fds? > > > =E2=80=82=E2=80=82=E2=80=82=E2=80=82Well, I first tried using close_r= ange() from the tap fd up to > > > max, as you suggested, and I found that it was closing file > > > descriptors that passt had itself opened (like the log file), which > > > is obviously not correct. =20 > > > > Oh. Right. Of course. Because this has moved later, we've now > > opened a bunch of stuff before it runs, meaning that closing > > everything no longer works. > > =20 > > > =E2=80=82=E2=80=82=E2=80=82=E2=80=82I considered enumerating all the = possible file descriptors that > > > passt *might* have opened, and pass them to isolate_fds as > > > fds_to_keep: it turns out that there are around 10 or so file > > > descriptors that may or may not have been opened before the call to > > > isolate_fds. I decided I didn't like that strategy, because it would > > > create a maintenance burden: =20 > > > > Right, that doesn't seem practical. =20 >=20 > ...maybe we could make it slightly more practical by, either: >=20 > 1. keeping all the files we open for configuration / logging / control > path purposes (it's not many and they are clearly documented in > struct ctx, unless I missed some) in a separate struct living inside > ctx, maybe even as an array indexed by an enum. >=20 > At that point, it would be clear what those files are, and if > somebody forgets to use that struct or array in the future, they'll > see their change presumably not working at all as we'll close > whatever new file descriptor right away. So I'm not that worried by > the maintenance burden because it looks rather fool-proof (this is > regardless of the separate struct / array I'm proposing) >=20 > 2. introducing a mandatory (or optional) wrapper around open(), which > would open a file and store its descriptor number in an array, along > with a wrapper for close() which would check that array and drop the > item if it's present >=20 > Otherwise: >=20 > > > every time someone tinkers with a file > > > descriptor or reorders the startup sequence, they would have to > > > remember to also check the list of file descriptors to keep. And in > > > particular, some of the file descriptors are non-intuitive. Maybe > > > not everyone realizes that a socket counts a file descriptor. What > > > if there's a bug in someone's new feature where sockets are just > > > disappearing without a trace? I would hate to waste someone's time > > > chasing a subtle bug like that. So, I decided that enumerating > > > fds_to_close at runtime time would be less of a maintenance burden > > > than enumerating fds_to_keep in code. =20 > > > > =20 > > > =E2=80=82=E2=80=82=E2=80=82=E2=80=82I also considered reordering the = startup so that forking happens > > > before opening any files, so isolate fds can run with only the tap > > > fd as its exclusion list, but I didn't see how to make that change > > > without significant refactoring. It might end up being necessary, > > > but I can't make that decision on my own. I don't understand the > > > code well enough. Interested in talking that through? =20 > > > > Fair enough. My best guess is that this would be the best approach, > > but as you say it needs pretty in depth understanding of the code. > > And, as you've seen I've now guessed wrong several times about the > > best way to approach things. > > > > Let me have a look into this and see if I can come up with something > > that actually works. =20 >=20 > ...if this is becoming too complicated, I would almost suggest adding a > command line option to keep all files open *only* in the case where > pasta spawns a command. I'm not enthusiastic about it, but given the > complexity we risk introducing otherwise, I wouldn't find it outrageous > either. >=20 > > > I don't understand your comment about conf_tap_fd becoming > > > static. My implementation parses -F at the same time as the other > > > arguments (not early anymore). Did you mean that you would inline > > > the string to int conversion in conf()? That seems reasonable. I > > > just felt that conf_tap_fd was overall too complex to inline in its > > > current form. =20 > > > > I just meant that since it is now only used in conf.c, it can become > > local to that module, a static function in C terminology. > > =20 > > > While relocating tap fd to 3+ could easily happen later, I still > > > don't think it should happen during isolation, because it actually > > > conceptually has nothing to do with isolation. It's about avoiding a > > > corner case that would cause tap device chatter accidentally being > > > printed to std err or something like that. Isolation should be > > > focused on closing unused fds, not on managing fds that are in > > > use. Perhaps we could relocate the tap fd to 3+ in main, around the > > > same time that we are populating 0=E2=80=942, for clarity (since thos= e two > > > steps need to coordinate). > > > > > > I was not aware that dynamic memory would be off the table. My main > > > reason for doing it that way is because I didn't want a very large > > > array (max fds) to be always in memory even though most of the time > > > it would be unused. I will have to re-think the strategy. Maybe > > > there is another way to avoid a large unused array. =20 > > > > That array isn't particularly large compared to many others we already > > have. And even those are usually only a small fraction of our total > > effective memory usage - most of that comes in the form of kernel > > memory for sockets and buffers. In principle we could also use > > MADV_DONTNEED to discard the allocated memory once we're done with it. > > =20 > > > Thank you for pointing out the flaw in my logic regarding the max > > > open files limit. I will have to think harder about a correct way to > > > close fds. > > > > > > I do not believe that my patch disables your feature of populating > > > fds 0=E2=80=942 with devnull. That happens in main, not in isolate fd= s. My > > > implementation of enumerating the inherited fds excludes 0=E2=80=942.= =20 > > > > You're right, sorry. I got muddled because it was inside > > isolate_fds() in a bunch of draft versions of my patches (I eventually > > realised that wouldn't quite work). > > =20 > > > > > > I hope to hear from you again soon > > > Richard > > > > > > PS. In case anyone else who's reading along has concerns about the > > > potential performance hit caused by abandoning close_range(), I have > > > an argument prepared to explain why that is not a serious concern. = =20 > > > > So, fwiw, the concern isn't the cost of close() on the actually open > > files - as a one time cose that will generally be trivial. The conern > > is discovering the open fds: 2^31 close()s or other syscalls to > > discover if there's an fd there _would_ be too much. Using > > /proc/self/fd is an interesting approach. It's not portable, but then > > neither is close_range() (although FreeBSD does have close_range() > > apparently). Working out how to size things is the tricky bit with > > the /proc/self/fd approach, though. > > > > [...] =20 >=20 > -- > Stefano >=20 >=20