Received-SPF: pass (google.com: domain of linux-kernel+bounces-215734-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Sat, 15 Jun 2024 07:55:28 +0100
From: Al Viro <viro@zeniv.linux.org.uk>
To: Christoph Hellwig <hch@infradead.org>
Cc: Congjie Zhou <zcjie0802@qq.com>, brauner@kernel.org,
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] fs: modify the annotation of vfs_mkdir() in fs/namei.c
Message-ID: <20240615065528.GP1629371@ZenIV>
References: <20240615030056.GO1629371@ZenIV>
 <tencent_63C013752AD7CA1A22E75CEF6166442E6D05@qq.com>
 <Zm000qL0N6XY7-4O@infradead.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Zm000qL0N6XY7-4O@infradead.org>
Sender: Al Viro <viro@ftp.linux.org.uk>

On Fri, Jun 14, 2024 at 11:29:38PM -0700, Christoph Hellwig wrote:
> On Sat, Jun 15, 2024 at 12:38:32PM +0800, Congjie Zhou wrote:
> > modify the annotation of @dir and @dentry
> 
> Well, it is clearly obvious that you modify them from the patch.  But
> why?

Look at the current comment:

 * @dir:        inode of @dentry

It is an inode of _some_ dentry; it's most definitely not that
of the argument named 'dentry'.

 * @dentry:     pointer to dentry of the base directory

No.  The first thing vfs_mkdir() does is
        int error = may_create(idmap, dir, dentry);

	if (error)
		return error;

Look at may_create().  Starts with
static inline int may_create(struct mnt_idmap *idmap,
                             struct inode *dir, struct dentry *child)
{
     audit_inode_child(dir, child, AUDIT_TYPE_CHILD_CREATE);
     if (child->d_inode)
	     return -EEXIST;

If the last argument (aka. 'dentry' argument of vfs_mkdir()) is currently
referring to *ANY* directory, you get -EEXIST.  For a good and simple
reason: it is the dentry of subdirectory to be created.  Now, the second
argument of vfs_mkdir() ('dir') is the inode of the parent to be (or base
directory, if you will).

While we are at it, the rest of comments coming from the same commit
suffer similar problems.  vfs_create(), vfs_symlink(), et.al.
vfs_unlink() is fine, vfs_rmdir() should match vfs_unlink() (inode of
parent + dentry of victim).