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=X4E+8kbL; 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 4A8D55A0269 for ; Thu, 16 Jul 2026 10:02:45 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1784188964; 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=ofZYjxOuJIaTXYZn2GSMVj0aHnop/YXajYDpvF86dkg=; b=X4E+8kbLCUlDxrIfBy1q42wNwGbbMaV/jOPCbliSlSRqJ1ox59I9hazr7e5q7mtPH/kzT5 EVy/gJ/2d853pO9jG4t2gmQm2GPKY0FEkLSJoQ7rHZB/Bi/XqzHJzO/t66YZ5KZ8foCJ6h dlgzHAtsmQ1IEaoAxuuG/LnAwIjwbPI= 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-422-5b1K64u4P8qwPltlHVsAqw-1; Thu, 16 Jul 2026 04:02:42 -0400 X-MC-Unique: 5b1K64u4P8qwPltlHVsAqw-1 X-Mimecast-MFC-AGG-ID: 5b1K64u4P8qwPltlHVsAqw_1784188961 Received: by mail-wr1-f71.google.com with SMTP id ffacd0b85a97d-472bdd6f529so3899545f8f.1 for ; Thu, 16 Jul 2026 01:02:42 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1784188961; x=1784793761; 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=ofZYjxOuJIaTXYZn2GSMVj0aHnop/YXajYDpvF86dkg=; b=azVZlFp8ejru7ojulCCLhVUJi056HIylXsWNPK8jaFAR5yoCfAF8swu7tEkl6y+1v9 o33aalpKEgt8zmaLy/0//BUmKyEfrlvFV98gP7wNaDBrPnQJ/cf8w7Wl65cCuRE3lnBk L+uHFNZ0dN3k/WBzcBPVhK0uzZ69NO7a5pOrrVbdy1zcSFLlMUlyVjD7m8/YT3Ghpb9l cUXDOColccGMBkkz8t3B/M74LajRBnTcDdX6vrR01t+bjgSiw269ulYRZ+d2QxBRrRGJ Mye6J2XwWt5BjMPpUGAXsvSrbTzpktMKoRUcsQR/Xe6jIL1GkVq7OBZTt++Zy9pW9f9g Cccg== X-Gm-Message-State: AOJu0YxHppvs+ZrBynja4Uum1XHvMQICwh+1CLysX7pHLjEO9FMVgyw4 LfElqBtF9GwWVK3x+KL8NljqlYX3SPJEaR5afGXG0cUCElzCvt0szEKRk1AYNtTYcflB6Id/bQp gEpc6Za1yO1YI/LJNfhzYuoQLDCsmggIPGs94c9dnpCJiyBtJrh355TBP2VHcZA== X-Gm-Gg: AfdE7cnaJAQBicgLtUMESmy21yW/ikdBENZT7ddhlDl1x/IfygaUEjoD7l1vnID6nvp qMnXx/1uibJ6AQEumIGsEoqh4xWhVDADOQ2oi8j/So/miBpgPpYp8Lau5YJ7cuDYRl3EBSbnz/t odNr9gU2OZu+XNjGaM7FsJaxzGZ3kSFEJcujgRWW79aJQ2i0avYLREGogZ3CxoB2LMWW70AYKxv 97fLVAL0KeesBnRoCUPtG7xnUGVi+F2k3axJ2cGWeTmPuR2WmMX9y4kk0ovcEjJDtk6DmiQAmVh +HAc8zvytWoMwy0O7qLlZJ1rwXZEMSMoWw3FttwIqipIox6Y6uX2+aVA5/17VOM71By1a+s9qA8 J8jQzI64tY+4= X-Received: by 2002:a05:6000:1acf:b0:47b:69a5:7263 with SMTP id ffacd0b85a97d-47f4886ef48mr11969848f8f.5.1784188960948; Thu, 16 Jul 2026 01:02:40 -0700 (PDT) X-Received: by 2002:a05:6000:1acf:b0:47b:69a5:7263 with SMTP id ffacd0b85a97d-47f4886ef48mr11969736f8f.5.1784188960015; Thu, 16 Jul 2026 01:02:40 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [2a10:fc81:a806:d6a9::1]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-47f4635a940sm21217683f8f.12.2026.07.16.01.02.39 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 16 Jul 2026 01:02:39 -0700 (PDT) From: Stefano Brivio To: David Gibson Subject: Re: [PATCH 0/6] Fix bug 215 and some related issues with fd handling Message-ID: <20260716100237.4bc3c232@elisabeth> In-Reply-To: References: <20260714092926.2881848-1-david@gibson.dropbear.id.au> <20260715183946.6121580a@elisabeth> <20260716092228.698aa80e@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: Thu, 16 Jul 2026 10:02:38 +0200 (CEST) X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: -1wvdVFHo0KExNV2s1e0T2UOyPH537P-rWL0GhsUrFs_1784188961 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: ACUWF3APTRNMEWXIHULODMSDBG47JRHZ X-Message-ID-Hash: ACUWF3APTRNMEWXIHULODMSDBG47JRHZ 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: 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: On Thu, 16 Jul 2026 17:49:16 +1000 David Gibson wrote: > On Thu, Jul 16, 2026 at 09:22:30AM +0200, Stefano Brivio wrote: > > On Thu, 16 Jul 2026 15:25:08 +1000 > > David Gibson wrote: > > > > > On Wed, Jul 15, 2026 at 06:39:47PM +0200, Stefano Brivio wrote: > > > > On Tue, 14 Jul 2026 19:29:20 +1000 > > > > David Gibson wrote: > > > > > > > > > Stefano, you called it correctly. While working on bug 215, as usual, > > > > > I found a bunch of adjacent things to clean up. > > > > > > > > So, I finally finished reviewing the series. > > > > > > > > Other than 1/6 and 2/6 on which I already commented (long story short, > > > > I think we should avoid 1/6, and about 2/6, it would be nice to parse -F > > > > just once in general but I think we shouldn't "force" it like that... > > > > maybe just parse / get it outside conf()?), > > > > > > Hm, I'm not entirely sure of the distinction you're drawing between > > > the two cases. > > > > > > > I don't see any substantial > > > > issue with the other patches, but I have some general comments about the > > > > approach. > > > > > > > > As a detail, though, I would recommend Cc'ing everybody who might be > > > > interested or affected by this (reporter of bug #215, Rich as he fixed > > > > the original https://github.com/libguestfs/libguestfs/issues/360, and > > > > Alyssa as author of aa1cc8922867 ("conf: allow --fd 0"). > > > > > > Ah, good point, I'll do that for the next spin. > > > > > > > > I did start by attempting the appoach you suggested for bug 215 - > > > > > remembering which of the low fds were standard streams and avoiding > > > > > closing them in __daemon(). It is indeed shorter, but only by 1-2 > > > > > lines. Looking at possible interactions with other things, I became > > > > > more and more convinced that leaving anything other than the standard > > > > > streams in fds 0-2 was an accident waiting to happen. > > > > > > > > It did actually happen, see c66f0341d94d ("log: Don't report syslog > > > > failures to stderr after initialisation"). I didn't consider that, > > > > and it's indeed a strong argument in favour of this approach. > > > > > > > > I still have some remarks and doubts about it though: > > > > > > > > 1. we might have users passing a given file descriptor with the > > > > expectation that it won't change its number as seen from procfs > > > > (and dup2() changes that, right?), even just for debugging, and 4/6 > > > > breaks that. It's not a very legitimate expectation maybe but it > > > > might be one, we simply don't know > > > > > > That... really seems like taking bug for bug compatibility too far to > > > me. > > > > > > > > > > > 2. the reason behind my proposal (check if file descriptors are open > > > > when we start and avoid closing them) was simplicity and avoiding > > > > the risk of a number of side effects (more below). > > > > > > > > Yes, it's just a bit shorter (depending on how we count), but this > > > > diff (build tested only) should be equivalent to patches 4/6 and 5/6, > > > > which look considerably more complicated to me (even though the > > > > simplification in close_open_files() is significant... but we could > > > > get the same outcome also with just 4/6): > > > > > > > > --- > > > > diff --git a/passt.c b/passt.c > > > > index 65a07d7..e2ea613 100644 > > > > --- a/passt.c > > > > +++ b/passt.c > > > > @@ -330,8 +330,9 @@ static void passt_worker(void *opaque, int nfds, struct epoll_event *events) > > > > int main(int argc, char **argv) > > > > { > > > > struct epoll_event events[NUM_EPOLL_EVENTS]; > > > > + bool close_low_fd[STDERR_FILENO + 1]; > > > > + int nfds, devnull_fd = -1, i; > > > > struct ctx *c = &passt_ctx; > > > > - int nfds, devnull_fd = -1; > > > > struct rlimit limit; > > > > struct timespec now; > > > > struct sigaction sa; > > > > @@ -339,6 +340,9 @@ int main(int argc, char **argv) > > > > if (clock_gettime(CLOCK_MONOTONIC, &log_start)) > > > > die_perror("Failed to get CLOCK_MONOTONIC time"); > > > > > > > > + for (i = STDIN_FILENO; i <= STDERR_FILENO; i++) > > > > + close_low_fd[i] = !fcntl(i, F_GETFD); > > > > > > Nit: needs to have a >= 0, F_GETFD returns flags, not 0 on success. > > > > > > > + > > > > arch_avx2_exec(argv); > > > > > > > > isolate_initial(argc, argv); > > > > @@ -419,7 +423,10 @@ int main(int argc, char **argv) > > > > die("Failed to sandbox process, exiting"); > > > > > > > > if (!c->foreground) { > > > > - __daemon(c->pidfile_fd, devnull_fd); > > > > + if (c->fd_tap != -1 && c->fd_tap < STDERR_FILENO) > > > > + close_low_fd[c->fd_tap] = false; > > > > + > > > > + __daemon(close_low_fd, c->pidfile_fd, devnull_fd); > > > > close(c->pidfile_fd); > > > > c->pidfile_fd = -1; > > > > log_stderr = false; > > > > diff --git a/util.c b/util.c > > > > index 4bc5d6f..57d42e1 100644 > > > > --- a/util.c > > > > +++ b/util.c > > > > @@ -501,13 +501,14 @@ int output_file_open(const char *path, int flags) > > > > > > > > /** > > > > * __daemon() - daemon()-like function writing PID file before parent exits > > > > + * @close_fd: Standard stream descriptors numbers to close > > > > * @pidfile_fd: Open PID file descriptor > > > > * @devnull_fd: Open file descriptor for /dev/null > > > > * > > > > * Return: 0 in the child process on success. The parent process exits. > > > > * Does not return in either process on failure (calls _exit). > > > > */ > > > > -int __daemon(int pidfile_fd, int devnull_fd) > > > > +int __daemon(bool close_fd[STDERR_FILENO + 1], int pidfile_fd, int devnull_fd) > > > > { > > > > pid_t pid = fork(); > > > > > > > > @@ -522,9 +523,9 @@ int __daemon(int pidfile_fd, int devnull_fd) > > > > } > > > > > > > > if (setsid() < 0 || > > > > - dup2(devnull_fd, STDIN_FILENO) < 0 || > > > > - dup2(devnull_fd, STDOUT_FILENO) < 0 || > > > > - dup2(devnull_fd, STDERR_FILENO) < 0 || > > > > + (close_fd[STDIN_FILENO] && dup2(devnull_fd, STDIN_FILENO) < 0) || > > > > + (close_fd[STDOUT_FILENO] && dup2(devnull_fd, STDOUT_FILENO) < 0) || > > > > + (close_fd[STDERR_FILENO] && dup2(devnull_fd, STDERR_FILENO) < 0) || > > > > close(devnull_fd)) > > > > passt_exit(EXIT_FAILURE); > > > > > > > > diff --git a/util.h b/util.h > > > > index 90e8a20..246ac67 100644 > > > > --- a/util.h > > > > +++ b/util.h > > > > @@ -160,7 +160,7 @@ bool ns_is_init(void); > > > > int open_in_ns(const struct ctx *c, const char *path, int flags); > > > > int output_file_open(const char *path, int flags); > > > > void pidfile_write(int fd, pid_t pid); > > > > -int __daemon(int pidfile_fd, int devnull_fd); > > > > +int __daemon(bool close_fd[STDERR_FILENO + 1], int pidfile_fd, int devnull_fd); > > > > int fls(unsigned long x); > > > > int ilog2(unsigned long x); > > > > int write_file(const char *path, const char *buf); > > > > --- > > > > > > Yes, that will do the job. I still think avoiding anything but the > > > standard streams in 0-2 is worthwhile. > > > > > > > 3. I have a generic worry that LSMs might get in the way. This would be solved > > > > by testing your series against current AppArmor and SELinux policies but I > > > > didn't get the chance yet (it would be nice if you could...) > > > > > > That seems really far-fetched to me. I assume you're meaning they > > > would block the dup2()? That would break nearly anything that spawns > > > child processes. > > > > No, not dup2() specifically. AppArmor and SELinux don't operate (in > > general) on specific system calls. I'm rather thinking of some issue > > manipulating file descriptors that correspond to console devices. > > > > For example, pasta's SELinux policy currently has: > > > > allow pasta_t console_device_t:chr_file { open write getattr ioctl }; > > allow pasta_t user_devpts_t:chr_file { getattr read write ioctl }; > > > > are those sufficient if we call dup2() on those? > > Oh, I see. Am I correct in thinking that those lavels are attached > based on what the descriptor actually refers to, not based on the fd > number? Yes. > If so, we don't call dup2() on console files. Ah, oops, right, I mixed things up. > We use it for 1) Moving > the --fd descriptor out of the 0-2 range, so called on the --fd > socket. 2) For filling up any empty slots in 0-2, so targeting a > /dev/null handle. In short, we never move the standard streams, we > just move things around them. > > Unless someone passes in an actual console descriptor with --fd, in > which case SELinux blowing us up sounds like the correct outcome. Right, good. I'll give things a quick try anyway once you re-spin. -- Stefano