Re: [PATCH 1/3] treewide: Add SOCK_CLOEXEC to accept() calls that are missing it

[Stefano Brivio <sbrivio@redhat.com>]

On Wed, 20 May 2026 22:52:47 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, May 20, 2026 at 01:36:48PM +0200, Stefano Brivio wrote:
> > On Wed, 20 May 2026 11:04:58 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > On Wed, May 20, 2026 at 02:37:02AM +0200, Stefano Brivio wrote:  
> > > > On Mon, 18 May 2026 12:28:57 +1000
> > > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > >     
> > > > > On Sat, May 16, 2026 at 05:46:11PM +0200, Stefano Brivio wrote:    
> > > > > > On Wed, 13 May 2026 14:14:21 +1000
> > > > > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > > > >       
> > > > > > > Generally we try to set the O_CLOEXEC flag on every fd we create.  This
> > > > > > > seems to be generally accepted security best practice these days, and we
> > > > > > > never fork(), so certainly have no need to pass fds to children.      
> > > > > > 
> > > > > > But we do clone() with CLONE_FILES (even though when we clone() to call
> > > > > > execvp() later, we don't set CLONE_FILES), so, even though I don't see
> > > > > > a reason to skip O_CLOEXEC for c->fd_tap, this conclusion shouldn't be
> > > > > > automatic from the fact we don't fork().      
> > > > > 
> > > > > So, I did think about that when  wrote it, but went for the short
> > > > > version rather than saying clone() with CLONE_FILES doesn't count.
> > > > > 
> > > > > Now, I realised that we've both fallen for the trap again, forgetting
> > > > > that this has nothing to do with fork() or clone() and is, as it says
> > > > > right there in the name, about exec().    
> > > > 
> > > > No, wait, I didn't fall for it, not this time. :) That's why I was
> > > > mentioning that when we call clone() and execvp() later (which would be    
> > > 
> > > Uh...?  I'm pretty sure the only execve(2) in the entire program is
> > > where we spawn passt.avx2.  That's essentially the very first thing we
> > > do, long before this point.  
> > 
> > Well, grep would beg to differ, as we don't call execve() at all, but:  
> 
> I meant the system call execve(2), which execv() and execvp() are
> library wrappers around.

Oops, I missed the (2).

> > $ grep execv *.c | grep -v qrap
> > arch.c:		execv(new_path, argv);
> > pasta.c:	execvp(a->exe, a->argv);  
> 
> Ah, I did miss the one in pasta_spawn_cmd().  Of course, we definitely
> don't want to leak our internal fds into the spawned command, so
> CLOEXEC is what we want.
> 
> > O_CLOEXEC (or lack thereof) also matters on execvp().
> >   
> > > > the only path that matters), we don't set CLONE_FILES anyway.    
> > > 
> > > CLONE_FILES is irrelevant, it's lost during execve(2).  
> > 
> > Yes, but if you first clone(), which we actually do before calling
> > pasta_spawn_cmd(), and then execvp(), CLONE_FILES on clone() *would*
> > matter, because the cloned process would inherit the open files, and
> > the process started by execvp() would then get those files as well.  
> 
> No, it doesn't matter.  If you clone() without CLONE_FILES, the new
> thread/process gets a copy of the handles, which do or don't survive
> exec() depending on O_CLOEXEC.  If you clone with CLONE_FILES, the new
> process shares the fd table.  The fd table is unshared again as part
> of the exec().
> 
> From execve(2):
> >        •  The file descriptor table is unshared, undoing the effect of the CLONE_FILES flag of clone(2).  
> 
> .. then the now copied files do or don't survive depending on
> O_CLOEXEC.  Either way, O_CLOEXEC has the same final effect.

Ah, right, I forgot about this part. But anyway, O_CLOEXEC is always
relevant, as long as we call execvp() not right after start, regardless
of having called clone() before.

And in this case (pasta_spawn_cmd()) we call it rather "late", so the
fact we don't call fork() is not really relevant for this purpose.

-- 
Stefano