Re: [PATCH v2] feat: Pass open files to child in pasta mode

[Stefano Brivio <sbrivio@redhat.com>]

A couple of ideas, rigorously not tested:

On Thu, 6 Aug 2026 13:20:27 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Mon, Aug 03, 2026 at 08:34:19PM +0000, Lawrence, Richard E wrote:
> > Howdy David,
> > 
> > Why enumerate inherited fds?
> >     Well, I first tried using close_range() 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.  
> 
> 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.
> 
> >     I 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:  
> 
> 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.  
> 
> 
> >     I 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?  
> 
> 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.

...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.  
> 
> 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.
> 
> > 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—2, for clarity (since those 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.  
> 
> 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.
> 
> > 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—2 with devnull. That happens in main, not in isolate fds. My
> > implementation of enumerating the inherited fds excludes 0—2.  
> 
> 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).
> 
> > 
> > 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.  
> 
> 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.
>
> [...]

-- 
Stefano