Re: [LSF/MM/BPF TOPIC] Allowing linkat() to replace the destination

[Al Viro <viro@zeniv.linux.org.uk>]

On Fri, Jan 17, 2020 at 12:22:19PM -0800, Omar Sandoval wrote:

> Since manpage troff patches aren't particularly nice to read, here is
> what I had in mind in a readable form:
> 
> int linkat(int olddirfd, const char *oldpath, int newdirfd,
> 	   const char *newpath, int flags);
> 
> 	AT_REPLACE
> 		If newpath exists, replace it atomically. There is no
> 		point at which another process attempting to access
> 		newpath will find it missing.
> 	
> 		newpath must not be a directory.
> 	
> 		If oldpath and newpath are existing hard links referring
> 		to the same file, then this does nothing, and returns a
> 		success status. If newpath is a mount point, then this
> 		comparison considers the mount point, not the mounted
> 		file or directory.
> 	
> 		If newpath refers to a symbolic link, the link will be
> 		overwritten.
> 
> ERRORS
> 	EBUSY	AT_REPLACE was specified in flags, newpath is a mount
> 		point, and the mount point does not refer to the same
> 		file as oldpath. Or, AT_REPLACE was specified in flags
> 		and newpath ends with a . or .. component.

> 	EISDIR	AT_REPLACE was specified in flags and newpath is an
> 		existing directory.

So are <existing directory>/. or <existing directory>/.., so I wonder why
bother with -EBUSY there.

As for the gaps...
	1) emptypath for new.  Should be an error; EINVAL, probably.
Avoids arseloads of fun cases ("what if newfd/newpath refers to something
unlinked?", etc., etc.)
	2) mountpoint vs. mountpoint in the local namespace.  See the
rationale in that area for unlink()/rmdir()/rename().
	3) permission checks need to be specified
	4) as for the hardlinks, I would be very careful with the language;
if anything, that's probably "if the link specified by newfd/newpath points
to the object specified by oldfd/oldpath..."  Another thing is, as you
could've seen for rename(), such "permissive" clauses tend to give surprising
results.  For example, put the check for "it's a hardlink" early enough,
and you've suddenly gotten a situation when it *can* succeed for directory.
Or on a filesystem that never allowed hardlinks (the latter is probably
unavoidable).
	5) what _really_ needs to be said is that other links to
newpath are unaffected and so are previously opened files.
	6) "atomically" is bloody vague; the explanation you've put there
is not enough, IMO - in addition to "nobody sees it gone in the middle
of operation" there's also "if the operation fails, the target remains
undisturbed" and something along the lines of "if filesystem makes any
promises about the state after dirty shutdown in the middle of rename(),
the same promises should apply here".
	7) how do users tell if filesystem supports that?  And no,
references to pathconf, Cthulhu and other equally delightful entities
are not really welcome.

There might be be more - like I said, it needs to go through
linux-abi, fsdevel and security lists.  The above is all I see right
now, but I might be missing something subtle (or not so subtle).

As for the implementation... VFS side should be reasonably easy (OK,
it'll bloat do_symlinkat() quite a bit, since we won't be able to use
filename_create() for the target in there, but it's not _that_
terrible).  I'd probably prefer a separate method rather than
overloading ->link(), with the old method called when the target
is negative and new - when it's positive - less boilerplate and
harder for an instance to get confused.  They can easily use common
helpers with ->link() instances, so the code duplication won't
be an issue.