linux-kernel - Re: [MEH PATCH] fs: sort out a stale comment about races between fd alloc and dup2

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20241210181539.GE3387508@ZenIV>
Date: Tue, 10 Dec 2024 18:15:39 +0000
From: Al Viro <viro@...iv.linux.org.uk>
To: Mateusz Guzik <mjguzik@...il.com>
Cc: Christian Brauner <brauner@...nel.org>, jack@...e.cz,
	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [MEH PATCH] fs: sort out a stale comment about races between fd
 alloc and dup2

On Tue, Dec 10, 2024 at 05:48:40AM +0100, Mateusz Guzik wrote:
 
> Oh huh. I had seen that code before, did not mentally register there
> may be repeat file alloc/free calls due to repeat path_openat.
> 
> Indeed it would be nice if someone(tm) sorted it out, but I don't see
> how this has any relation to installing the file early and thus having
> fget worry about it.

Other than the former being an obvious prereq for the latter?  Not much...

> Suppose the "embryo"/"larval" file pointer is to be installed early
> and populated later. I don't see a benefit but do see a downside: this
> requires protection against close() on the fd (on top of dup2 needed
> now).
> The options that I see are:
> - install the file with a refcount of 2, let dup2/close whack it, do a
> fput in open to bring back to 1 or get rid of it if it raced (yuck)
> (freebsd is doing this)
> - dup2 is already special casing to not mess with it, add that to
> close as well (also yuck imo)

As a possibility (again, I'm not sold on the benefits of that scheme,
just looking into feasibility):
	dup2() when evicting an embryo:
		mark it evicted
		remove from descriptor table
		do nothing to refcount (in effect, transfer it to open())
		then proceed as if it hadn't been there
		[== pretend that dup2() always loses the race]
	close() when running into an embryo
		return -EBADF
		[== pretend that close() always loses the race]
	open() when it's done setting file up:
		if opening failed
			if not marked evicted
				remove from descriptor table
			fput()
			return whatever error we've got
		else
			if marked evicted
				fput()
			return the descriptor
		[== pretend that open() always wins the race]
"open" in the above stands for everything that opens a descriptor - socket(2),
pipe(2), eventfd(2), whatever.

> >From userspace side the only programs which can ever see EBUSY are
> buggy or trying to screw the kernel, so not a concern on that front.

Agreed.  I'm not saying we should go that way.