Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
From:   Luis Henriques <lhenriques@suse.de>
To:     Al Viro <viro@zeniv.linux.org.uk>
Cc:     Christian Brauner <brauner@kernel.org>,
        Jens Axboe <axboe@kernel.dk>,
        Pavel Begunkov <asml.silence@gmail.com>,
        linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
        io-uring@vger.kernel.org
Subject: Re: [RFC PATCH] fs: add AT_EMPTY_PATH support to unlinkat()
In-Reply-To: <20231009020623.GB800259@ZenIV> (Al Viro's message of "Mon, 9 Oct
        2023 03:06:23 +0100")
References: <20230929140456.23767-1-lhenriques@suse.de>
        <20231009020623.GB800259@ZenIV>
Date:   Mon, 09 Oct 2023 16:14:27 +0100
Message-ID: <87lecbrfos.fsf@suse.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

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 =3D NULL;
>>  	struct inode *delegated_inode =3D NULL;
>>  	unsigned int lookup_flags =3D 0;
>> -retry:
>> -	error =3D filename_parentat(dfd, name, lookup_flags, &path, &last, &ty=
pe);
>> -	if (error)
>> -		goto exit1;
>> +	bool empty_path =3D (flags & AT_EMPTY_PATH);
>>=20=20
>> -	error =3D -EISDIR;
>> -	if (type !=3D LAST_NORM)
>> -		goto exit2;
>> +retry:
>> +	if (empty_path) {
>> +		error =3D filename_lookup(dfd, name, 0, &path, NULL);
>> +		if (error)
>> +			goto exit1;
>> +		parent =3D path.dentry->d_parent;
>> +		dentry =3D path.dentry;
>> +	} else {
>> +		error =3D filename_parentat(dfd, name, lookup_flags, &path, &last, &t=
ype);
>> +		if (error)
>> +			goto exit1;
>> +		error =3D -EISDIR;
>> +		if (type !=3D LAST_NORM)
>> +			goto exit2;
>> +		parent =3D path.dentry;
>> +	}
>>=20=20
>>  	error =3D mnt_want_write(path.mnt);
>>  	if (error)
>>  		goto exit2;
>>  retry_deleg:
>> -	inode_lock_nested(path.dentry->d_inode, I_MUTEX_PARENT);
>> -	dentry =3D lookup_one_qstr_excl(&last, path.dentry, lookup_flags);
>> +	inode_lock_nested(parent->d_inode, I_MUTEX_PARENT);
>> +	if (!empty_path)
>> +		dentry =3D 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.  F=
or
> 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_RI=
GHTS,
> 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,
--=20
Lu=C3=ADs