[RFC] fs: add AT_EMPTY_PATH support to unlinkat()

  The linkat() syscall has support for the AT_EMPTY_PATH linux-specific flag
for a long time.  This flag allows callers to use an empty string in the
'oldpath' parameter, and to create the link to the file descriptor instead.

This patch modifies the unlinkat() syscall to use a similar semantic for
this flag, i.e. to allow for an empty string to indicate that we want to
unlink the file descriptor inode.

Signed-off-by: Luis Henriques <lhenriques@suse.de>
---
Hi!

I'm sending an early RFC of this patch specially because I'm not sure how
useful it is, and if support for this flag is really welcome.

The code is already a bit messy, and adding support for this extra flag
makes it even worse.  Maybe it could be refactored; suggestions are
welcome.

Also, this patch is only lightly tested -- it doesn't seem to break
anything, but I'll need to test it some more.  But as I said, I wanted to
get some feedback to make sure this patch is worth the trouble.

To be honest, this patch wasn't my idea: I remember seeing someone
somewhere suggesting or asking this flag to be added.  But it was long
time ago, I've written down the idea but I forgot keep a reference to the
source.  So, I hope the patch _may_ be useful to someone and that it's
purpose isn't just to make these two syscalls more symmetric (which is a
honourable goal in itself, if you ask my opinion! :-) ).

Cheers,
--
Luís

 fs/coredump.c |   2 +-
 fs/init.c     |   4 +-
 fs/internal.h |   4 +-
 fs/namei.c    | 112 +++++++++++++++++++++++++++++++-------------------
 io_uring/fs.c |   4 +-
 5 files changed, 76 insertions(+), 50 deletions(-)
  

On Fri, Sep 29, 2023 at 03:04:56PM +0100, Luis Henriques wrote:

> -int do_unlinkat(int dfd, struct filename *name)
> +int do_unlinkat(int dfd, struct filename *name, int flags)
>  {
>  	int error;
> -	struct dentry *dentry;
> +	struct dentry *dentry, *parent;
>  	struct path path;
>  	struct qstr last;
>  	int type;
>  	struct inode *inode = NULL;
>  	struct inode *delegated_inode = NULL;
>  	unsigned int lookup_flags = 0;
> -retry:
> -	error = filename_parentat(dfd, name, lookup_flags, &path, &last, &type);
> -	if (error)
> -		goto exit1;
> +	bool empty_path = (flags & AT_EMPTY_PATH);
>  
> -	error = -EISDIR;
> -	if (type != LAST_NORM)
> -		goto exit2;
> +retry:
> +	if (empty_path) {
> +		error = filename_lookup(dfd, name, 0, &path, NULL);
> +		if (error)
> +			goto exit1;
> +		parent = path.dentry->d_parent;
> +		dentry = path.dentry;
> +	} else {
> +		error = filename_parentat(dfd, name, lookup_flags, &path, &last, &type);
> +		if (error)
> +			goto exit1;
> +		error = -EISDIR;
> +		if (type != LAST_NORM)
> +			goto exit2;
> +		parent = path.dentry;
> +	}
>  
>  	error = mnt_want_write(path.mnt);
>  	if (error)
>  		goto exit2;
>  retry_deleg:
> -	inode_lock_nested(path.dentry->d_inode, I_MUTEX_PARENT);
> -	dentry = lookup_one_qstr_excl(&last, path.dentry, lookup_flags);
> +	inode_lock_nested(parent->d_inode, I_MUTEX_PARENT);
> +	if (!empty_path)
> +		dentry = lookup_one_qstr_excl(&last, parent, lookup_flags);

For starters, your 'parent' might have been freed under you, just as you'd
been trying to lock its inode.  Or it could have become negative just as you'd
been fetching its ->d_inode, while we are at it.

Races aside, you are changing permissions required for removing files.  For
unlink() you need to be able to get to the parent directory; if it's e.g.
outside of your namespace, you can't do anything to it.  If file had been
opened there by somebody who could reach it and passed to you (via SCM_RIGHTS,
for example) you currently can't remove the sucker.  With this change that
is no longer true.

The same goes for the situation when file is bound into your namespace (or
chroot jail, for that matter).  path.dentry might very well be equal to
root of path.mnt; path.dentry->d_parent might be in part of tree that is
no longer visible *anywhere*.  rmdir() should not be able to do anything
with it...

IMO it's fundamentally broken; not just implementation, but the concept
itself.

NAKed-by: Al Viro <viro@zeniv.linux.org.uk>
  

Al Viro <viro@zeniv.linux.org.uk> writes:

> On Fri, Sep 29, 2023 at 03:04:56PM +0100, Luis Henriques wrote:
>
>> -int do_unlinkat(int dfd, struct filename *name)
>> +int do_unlinkat(int dfd, struct filename *name, int flags)
>>  {
>>  	int error;
>> -	struct dentry *dentry;
>> +	struct dentry *dentry, *parent;
>>  	struct path path;
>>  	struct qstr last;
>>  	int type;
>>  	struct inode *inode = NULL;
>>  	struct inode *delegated_inode = NULL;
>>  	unsigned int lookup_flags = 0;
>> -retry:
>> -	error = filename_parentat(dfd, name, lookup_flags, &path, &last, &type);
>> -	if (error)
>> -		goto exit1;
>> +	bool empty_path = (flags & AT_EMPTY_PATH);
>>  
>> -	error = -EISDIR;
>> -	if (type != LAST_NORM)
>> -		goto exit2;
>> +retry:
>> +	if (empty_path) {
>> +		error = filename_lookup(dfd, name, 0, &path, NULL);
>> +		if (error)
>> +			goto exit1;
>> +		parent = path.dentry->d_parent;
>> +		dentry = path.dentry;
>> +	} else {
>> +		error = filename_parentat(dfd, name, lookup_flags, &path, &last, &type);
>> +		if (error)
>> +			goto exit1;
>> +		error = -EISDIR;
>> +		if (type != LAST_NORM)
>> +			goto exit2;
>> +		parent = path.dentry;
>> +	}
>>  
>>  	error = mnt_want_write(path.mnt);
>>  	if (error)
>>  		goto exit2;
>>  retry_deleg:
>> -	inode_lock_nested(path.dentry->d_inode, I_MUTEX_PARENT);
>> -	dentry = lookup_one_qstr_excl(&last, path.dentry, lookup_flags);
>> +	inode_lock_nested(parent->d_inode, I_MUTEX_PARENT);
>> +	if (!empty_path)
>> +		dentry = lookup_one_qstr_excl(&last, parent, lookup_flags);
>
> For starters, your 'parent' might have been freed under you, just as you'd
> been trying to lock its inode.  Or it could have become negative just as you'd
> been fetching its ->d_inode, while we are at it.
>
> Races aside, you are changing permissions required for removing files.  For
> unlink() you need to be able to get to the parent directory; if it's e.g.
> outside of your namespace, you can't do anything to it.  If file had been
> opened there by somebody who could reach it and passed to you (via SCM_RIGHTS,
> for example) you currently can't remove the sucker.  With this change that
> is no longer true.
>
> The same goes for the situation when file is bound into your namespace (or
> chroot jail, for that matter).  path.dentry might very well be equal to
> root of path.mnt; path.dentry->d_parent might be in part of tree that is
> no longer visible *anywhere*.  rmdir() should not be able to do anything
> with it...
>
> IMO it's fundamentally broken; not just implementation, but the concept
> itself.
>
> NAKed-by: Al Viro <viro@zeniv.linux.org.uk>

Thank you for your review, which made me glad I sent out the patch early
as an RFC.  I (think I) understand the issues you pointed out and,
although some of them could be fixed (the races), I guess there's no point
pursuing this any further, since you consider the concept itself to be
broken.  Again, thank you for your time.

Cheers,
  

Apologies, this message was intended as a reply to Al Viro, but I accidentally
deleted that message so I'm replying to the reply instead.

On Mon, Oct 09 2023 at 16:14:27 +0100, Luis Henriques quoth thus:

> Al Viro <viro@zeniv.linux.org.uk> writes:
> 
> > On Fri, Sep 29, 2023 at 03:04:56PM +0100, Luis Henriques wrote:
> >
> >> -int do_unlinkat(int dfd, struct filename *name)
> >> +int do_unlinkat(int dfd, struct filename *name, int flags)
> >>  {
> >>  	int error;
> >> -	struct dentry *dentry;
> >> +	struct dentry *dentry, *parent;
> >>  	struct path path;
> >>  	struct qstr last;
> >>  	int type;
> >>  	struct inode *inode = NULL;
> >>  	struct inode *delegated_inode = NULL;
> >>  	unsigned int lookup_flags = 0;
> >> -retry:
> >> -	error = filename_parentat(dfd, name, lookup_flags, &path, &last, &type);
> >> -	if (error)
> >> -		goto exit1;
> >> +	bool empty_path = (flags & AT_EMPTY_PATH);
> >>  
> >> -	error = -EISDIR;
> >> -	if (type != LAST_NORM)
> >> -		goto exit2;
> >> +retry:
> >> +	if (empty_path) {
> >> +		error = filename_lookup(dfd, name, 0, &path, NULL);
> >> +		if (error)
> >> +			goto exit1;
> >> +		parent = path.dentry->d_parent;
> >> +		dentry = path.dentry;
> >> +	} else {
> >> +		error = filename_parentat(dfd, name, lookup_flags, &path, &last, &type);
> >> +		if (error)
> >> +			goto exit1;
> >> +		error = -EISDIR;
> >> +		if (type != LAST_NORM)
> >> +			goto exit2;
> >> +		parent = path.dentry;
> >> +	}
> >>  
> >>  	error = mnt_want_write(path.mnt);
> >>  	if (error)
> >>  		goto exit2;
> >>  retry_deleg:
> >> -	inode_lock_nested(path.dentry->d_inode, I_MUTEX_PARENT);
> >> -	dentry = lookup_one_qstr_excl(&last, path.dentry, lookup_flags);
> >> +	inode_lock_nested(parent->d_inode, I_MUTEX_PARENT);
> >> +	if (!empty_path)
> >> +		dentry = lookup_one_qstr_excl(&last, parent, lookup_flags);
> >
> > For starters, your 'parent' might have been freed under you, just as you'd
> > been trying to lock its inode.  Or it could have become negative just as you'd
> > been fetching its ->d_inode, while we are at it.
> >
> > Races aside, you are changing permissions required for removing files.  For
> > unlink() you need to be able to get to the parent directory; if it's e.g.
> > outside of your namespace, you can't do anything to it.  If file had been
> > opened there by somebody who could reach it and passed to you (via SCM_RIGHTS,
> > for example) you currently can't remove the sucker.  With this change that
> > is no longer true.
> >
> > The same goes for the situation when file is bound into your namespace (or
> > chroot jail, for that matter).  path.dentry might very well be equal to
> > root of path.mnt; path.dentry->d_parent might be in part of tree that is
> > no longer visible *anywhere*.  rmdir() should not be able to do anything
> > with it...
> >
> > IMO it's fundamentally broken; not just implementation, but the concept
> > itself.
> >
> > NAKed-by: Al Viro <viro@zeniv.linux.org.uk>

Al, thank you for this information.  It does shine a little light on where
dragons may be hiding.  I was wondering if you could comment on a related
issue.

linkat does allow specifing AT_EMPTY_PATH.  However, it requires
CAP_DAC_READ_SEARCH.  I saw that a patch went into the kernel to remove
this restriction, but was shortly thereafter reverted with a comment
to the effect of "We'll have to think about this a little more".  Then,
radio silence.  Other than requiring /proc be mounted to bypass, what
problem does this restriction solve?

Also related, the thing I'm even more interested in is the ability to
create an O_TMPFILE, populate it, set permissions, etc, and then make
it appear in a directory.  The problem is I almost always don't want it
to just appear, but rather atomically replace an older version of the
file.

dfd = openat(x, "y", O_RDONLY | O_CLOEXEC | O_DIRECTORY, 0)
fd = openat(dfd, ".", O_RDWR | O_CLOEXEC | O_TMPFILE, 0600)
do stuff with fd
fsync(fd)
linkat(fd, "", dfd, "z", AT_EMPTY_PATH | AT_REPLACE?)
close(fd)
fsync(dfd)
close(dfd)

The AT_REPLACE flag has been suggested before to work around the EEXIST
behavior.  Alternatively, renameat2 could accept AT_EMPTY_PATH for
olddirfd/oldpath, but fixing up linkat seems a little cleaner.  Without
this, it hardly seems worthwhile to use O_TMPFILE at all, and instead
just go through the hassle of creating the file with a random name
(plus exposing that file and having to possibly rm it in case of an error).

I haven't been able to find any explanation for the AT_REPLACE idea not
gaining traction.  Is there some security reason for this?

Thanks


> Thank you for your review, which made me glad I sent out the patch early
> as an RFC.  I (think I) understand the issues you pointed out and,
> although some of them could be fixed (the races), I guess there's no point
> pursuing this any further, since you consider the concept itself to be
> broken.  Again, thank you for your time.
> 
> Cheers,
> -- 
> Luís