linux-kernel - Re: [GIT PULL] pidfd updates

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230425154843.GR3390869@ZenIV>
Date:   Tue, 25 Apr 2023 16:48:43 +0100
From:   Al Viro <viro@...iv.linux.org.uk>
To:     Christian Brauner <brauner@...nel.org>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [GIT PULL] pidfd updates

On Tue, Apr 25, 2023 at 04:36:27PM +0200, Christian Brauner wrote:
> On Tue, Apr 25, 2023 at 02:54:29PM +0100, Al Viro wrote:
> > On Tue, Apr 25, 2023 at 02:34:15PM +0200, Christian Brauner wrote:
> > 
> > > It is rife with misunderstandings just looking at what we did in
> > > kernel/fork.c earlier:
> > > 
> > > 	retval = get_unused_fd_flags(O_RDWR | O_CLOEXEC);
> > >         [...]
> > >         pidfile = anon_inode_getfile("[pidfd]", &pidfd_fops, pid,
> > >                                      O_RDWR | O_CLOEXEC);
> > > 
> > > seeing where both get_unused_fd_flags() and both *_getfile() take flag
> > > arguments. Sure, for us this is pretty straightforward since we've seen
> > > that code a million times. For others it's confusing why there's two
> > > flag arguments. Sure, we could use one flags argument but it still be
> > > weird to look at.
> > 
> > First of all, get_unused_fd_flags() doesn't give a damn about O_RDWR and
> > anon_inode_getfile() - about O_CLOEXEC.  Duplicating the expression in
> > places like that is cargo-culting.
> 
> I distinctly remember us having that conversation about how to do this
> nicely back then and fwiw this is your patch... ;)
> 6fd2fe494b17 ("copy_process(): don't use ksys_close() on cleanups")

Should've followed with "no need to pass nonsense flags to get_unused_fd_flags()
and anon_inode_getfile()"...

> So sure, that was my point: people are confused why there's two flag
> arguments and what exactly has to go into them and just copy-paste
> whatever other users already have.

> There's definitely one where people allocate a file descriptor early on
> and then sometimes maybe way later allocate a struct file and install
> it. And that's where exposing and using get_unused_fd_flags() and
> fd_install() is great and works fine.

FWIW, there's something I toyed with a while ago - a primitive along the
lines of
fd_set_file(fd, file)
{
	if (IS_ERR(file)) {
		put_unused_fd(fd);
		return ERR_PTR(file);
	}
	fd_install(fd, file);
	return fd;
}

That simplifies quite a few places, collapsing failure exit handling.
Not sure how many can be massaged into that form, though...