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=L4G10ic0; 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 196495A026E for ; Thu, 06 Aug 2026 23:54:30 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1786053268; 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=jNyd10soxd/xU+4URq+Pm5AZtLnzg9l6o02VIAAHeHA=; b=L4G10ic0ngO3RihGpdH8j6xk1W6Wl4Cw2CgDPJFb7BDXH3gvVbxP+E+Knp3vAepBfrKIeG Fz5jOP5mR6HGDEKEgXrm3bU+uXieXOUIBh06q7F/5J4YpSO8Fheof/ZXnFKAQIgRXs3rzm CmN8LMGOJmGiXZHuhKgQsaK24WXQ3sU= Received: from mail-wr1-f71.google.com (mail-wr1-f71.google.com [209.85.221.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-602-wWwlAZduMDG1qBETNnSZhg-1; Thu, 06 Aug 2026 17:54:27 -0400 X-MC-Unique: wWwlAZduMDG1qBETNnSZhg-1 X-Mimecast-MFC-AGG-ID: wWwlAZduMDG1qBETNnSZhg_1786053266 Received: by mail-wr1-f71.google.com with SMTP id ffacd0b85a97d-47fecbb7f88so1561334f8f.0 for ; Thu, 06 Aug 2026 14:54:27 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1786053266; x=1786658066; 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=jNyd10soxd/xU+4URq+Pm5AZtLnzg9l6o02VIAAHeHA=; b=cFSJ5jUy0qMgW/4klYhy6M3Wn1sroJgGqL/GrYuDK0G0QYir+rmbRvVN/+bEoFt3V6 xcb8MMQbFgG/NFrzJXwTqU86Snn9wcy8EIcS8BQyVVMh5L8qU6l/Tg5r+Z9i0SfF0y9K 5AQFAmuc4MwGhSi5IUvwh7EYUDUy+lu+SNlp16XlONnTGoowpG6oMFk6YCHjOap1ta+m OeLUADJ8S83e15S2umJdkH2M9m9km3+wDHCjGiXe0OnA9v/KfzkBLlby94/7VuBmakHp Dd5WfkyRUwvKB/rX3t2yl4gbDAZrxXVM9W7LHU3GMegqrykT52NCetqfU/DKZgDNtF/+ FwpQ== X-Forwarded-Encrypted: i=1; AHgh+Ro0aAlAQxlTT/P0SeN4mGwnYQI90FQsDoGyYbgvL7FaFiBavLpNAvsVrgi0H1ZqRVQGCJX7aE9QKRA=@passt.top X-Gm-Message-State: AOJu0YxclqzQJMzbARFdmmdC6mdi/bwjaIsKZe6u43j+QY4mD1iibSYh HyMlYwbiVihZ5w4k8cl9/A2i1zHb3ZZbB1qDUI6pvWu5K4ekHDe5ig0oEo83GZ5Dm8uw8Cly97W DMj6l8n1ZNxf6JqAwlAFKCg0RgI/HrGHsRo68zv7wNcyCwOVTUAoVcynZVhFveA== X-Gm-Gg: AR+sD13bgMq5eW8NZxr+BmI9vYxpGemUFNfSS3hTUvcAOnCAO5o95fg2dIZmMQw1ygM moy2/rnk4JaCvjVV73Z+MwCFBXH7KfDk905EvlM7FtWttMnmtK+Je3tQpk3FStf9KyO8npTGoB6 G4U78z4oyLNUrtuAlmSiAkCBrQBeGTkUY2r9uyfB/b+n558m3WgXSlPz5hQ9urFMd6L4psS1Y3R ohO14RlYL+6K6MQECCuUdrXGHQ0CE7d2AF+iFk126Aw9zxtBUpiote6LFPLdNzmcNgYrHQ3sDao WcVGrYgQpB5DYMmYSUAsGWMOM3IrYrD71zGAtypuBc90MZoDXLaB8zPAmRXD6MM5mX3du3gRy1n PimuyFdRW6BDOdK7IHOCfD0u3/1yz X-Received: by 2002:a05:6000:1845:b0:47f:77a4:fd0 with SMTP id ffacd0b85a97d-47fec522e22mr29011599f8f.22.1786053266364; Thu, 06 Aug 2026 14:54:26 -0700 (PDT) X-Received: by 2002:a05:6000:1845:b0:47f:77a4:fd0 with SMTP id ffacd0b85a97d-47fec522e22mr29011556f8f.22.1786053265832; Thu, 06 Aug 2026 14:54:25 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [176.103.220.4]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-47ff7b285b1sm10897173f8f.31.2026.08.06.14.54.23 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 06 Aug 2026 14:54:23 -0700 (PDT) From: Stefano Brivio To: David Gibson Subject: Re: [PATCH v2] feat: Pass open files to child in pasta mode Message-ID: <20260806235422.5d6d8fd3@elisabeth> In-Reply-To: References: <20260731132601.422518-1-rlawrence@tamu.edu> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.49; x86_64-pc-linux-gnu) MIME-Version: 1.0 Date: Thu, 06 Aug 2026 23:54:23 +0200 (CEST) X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: ZSUcDDvfynjP00KVuEAeCF13nRvHrN4pFwfd3OaEkAo_1786053266 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Message-ID-Hash: IVRCBE4VNIOR7VQLAHJCSPTLPGHVK3IV X-Message-ID-Hash: IVRCBE4VNIOR7VQLAHJCSPTLPGHVK3IV 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: "Lawrence, Richard E" , "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: A couple of ideas, rigorously not tested: On Thu, 6 Aug 2026 13:20:27 +1000 David Gibson wrote: > On Mon, Aug 03, 2026 at 08:34:19PM +0000, Lawrence, Richard E wrote: > > Howdy David, > >=20 > > Why enumerate inherited fds? > > =E2=80=82=E2=80=82=E2=80=82=E2=80=82Well, I first tried using close_ran= ge() 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 >=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 po= ssible 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 >=20 > Right, that doesn't seem practical. ...maybe we could make it slightly more practical by, either: 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. 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) 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 Otherwise: > > 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 >=20 > > =E2=80=82=E2=80=82=E2=80=82=E2=80=82I also considered reordering the st= artup 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 >=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. >=20 > Let me have a look into this and see if I can come up with something > that actually works. ...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. > > 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 >=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 those = two > > steps need to coordinate). > >=20 > > 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 >=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. > >=20 > > 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 fds.= My > > implementation of enumerating the inherited fds excludes 0=E2=80=942. = =20 >=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 > >=20 > > I hope to hear from you again soon > > Richard > >=20 > > 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 >=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 Stefano