linux-kernel - Re: [PATCH v2 3/3] VFS/knfsd: Teach dentry_create() to use atomic

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: <845984BB-7A05-4A4C-8EAD-C28C93D43679@hammerspace.com>
Date: Thu, 20 Nov 2025 15:48:24 -0500
From: Benjamin Coddington <bcodding@...merspace.com>
To: Chuck Lever <chuck.lever@...cle.com>
Cc: Alexander Viro <viro@...iv.linux.org.uk>,
 Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
 Jeff Layton <jlayton@...nel.org>, NeilBrown <neil@...wn.name>,
 Olga Kornievskaia <okorniev@...hat.com>, Dai Ngo <Dai.Ngo@...cle.com>,
 Tom Talpey <tom@...pey.com>, Trond Myklebust <trondmy@...nel.org>,
 Mike Snitzer <snitzer@...nel.org>, linux-fsdevel@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-nfs@...r.kernel.org
Subject: Re: [PATCH v2 3/3] VFS/knfsd: Teach dentry_create() to use
 atomic_open()

On 20 Nov 2025, at 15:09, Chuck Lever wrote:

> On 11/20/25 10:57 AM, Benjamin Coddington wrote:
>
>> +
>> +		dentry = atomic_open(path, dentry, file, flags, mode);
>> +		error = PTR_ERR_OR_ZERO(dentry);
>> +
>> +		if (unlikely(create_error) && error == -ENOENT)
>> +			error = create_error;
>> +
>> +		if (!error) {
>> +			if (file->f_mode & FMODE_CREATED)
>> +				fsnotify_create(dir->d_inode, dentry);
>> +			if (file->f_mode & FMODE_OPENED)
>> +				fsnotify_open(file);
>> +		}
>> +
>> +		path->dentry = dentry;
>
> When atomic_open() fails, it returns ERR_PTR. Then path->dentry gets set
> to ERR_PTR unconditionally here.
>
> Should path->dentry restoration be conditional, only updating on
> success? Or perhaps should the original dentry be preserved in the local
> variable and restored on error?

No, we want to assign it and pass it along because there's a conditional
dput() at the bottom of nfsd4_create_file() that wants to know what happened
to the dentry.  And that conditional dput() needs to be there in case other
paths error out before we get to atomic_open().

>> +
>> +	} else {
>> +		error = vfs_create(idmap, dir_inode, dentry, mode, true);
>> +		if (!error)
>> +			error = vfs_open(path, file);
>
> Revisiting this, I wonder if the non-atomic error flow needs specific
> code to clean up after creation/open failures.

I think the fput() is all you need here.  I have throughly exercised the
non-atomic vfs_create() failure as well as the vfs_create() success then
vfs_open() failure paths in my testing and development.

Thanks for the review!

Ben