linux-kernel - Re: [PATCH v1 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <E119B07D-4F3B-481B-9EB7-372FEB6203B9@hammerspace.com>
Date: Wed, 19 Nov 2025 08:02:17 -0500
From: Benjamin Coddington <bcodding@...merspace.com>
To: NeilBrown <neil@...wn.name>
Cc: Alexander Viro <viro@...iv.linux.org.uk>,
 Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
 Chuck Lever <chuck.lever@...cle.com>, Jeff Layton <jlayton@...nel.org>,
 Olga Kornievskaia <okorniev@...hat.com>, Dai Ngo <Dai.Ngo@...cle.com>,
 Tom Talpey <tom@...pey.com>, linux-fsdevel@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-nfs@...r.kernel.org,
 Trond Myklebust <trondmy@...nel.org>, Mike Snitzer <snitzer@...nel.org>
Subject: Re: [PATCH v1 3/3] VFS/knfsd: Teach dentry_create() to use
 atomic_open()

On 18 Nov 2025, at 20:41, NeilBrown wrote:

> On Wed, 19 Nov 2025, Benjamin Coddington wrote:
>> While knfsd offers combined exclusive create and open results to clients,
>> on some filesystems those results may not be atomic.  This behavior can be
>> observed.  For example, an open O_CREAT with mode 0 will succeed in creating
>> the file but unexpectedly return -EACCES from vfs_open().
>>
>> Additionally reducing the number of remote RPC calls required for O_CREAT
>> on network filesystem provides a performance benefit in the open path.
>>
>> Teach knfsd's helper create_dentry() to use atomic_open() for filesystems
>> that support it.
>>
>> Signed-off-by: Benjamin Coddington <bcodding@...merspace.com>
>> ---
>>  fs/namei.c         | 43 ++++++++++++++++++++++++++++++++++++-------
>>  fs/nfsd/nfs4proc.c |  8 +++++---
>>  include/linux/fs.h |  2 +-
>>  3 files changed, 42 insertions(+), 11 deletions(-)
>>
>> diff --git a/fs/namei.c b/fs/namei.c
>> index 9c0aad5bbff7..70ab74fb5e95 100644
>> --- a/fs/namei.c
>> +++ b/fs/namei.c
>> @@ -4208,21 +4208,50 @@ EXPORT_SYMBOL(user_path_create);
>>   * On success, returns a "struct file *". Otherwise a ERR_PTR
>>   * is returned.
>>   */
>> -struct file *dentry_create(const struct path *path, int flags, umode_t mode,
>> +struct file *dentry_create(struct path *path, int flags, umode_t mode,
>
> I don't like that you dropped "const" without telling us why.
> It is because we not assign to path->dentry, which is because
> atomic_open() returns a dentry....  which will only be different for
> directories (I think).
>
> But do we need to update path?  The returned file will point to the
> correct dentry - isn't that all that matters?
>
> I guess that I'd like an explanation for why the const is being dropped,
> and why 'path' is being changed.

Well, the first reason was that I was embarrassed at all the new local
variables being added and atomic_open() wanted path->parent and
dentry_create() already had path->child, it was convenient to just re-use
it.  Then it became clear that nfsd4_create_file() really wants to clean up
(or not) the reference to its "child" dentry based on whether the dentry had
been consumed or was an error - so passing back that dentry rather than
re-arrange the tail of nfsd4_create_file() seemed nicer.

Its true that we can acquire the dentry from file->f_path.dentry, but only
in the successful case for both the atomic_open() and the
vfs_create()/vfs_open() path.  Atomic_open() does the work of swapping and
fiddling with the dentry refcounts for us so we don't need to check, so that
is the 2nd reason I passed the dentry back on struct path.

I don't understand the cases atomic_open() is handling here:

3555 static struct dentry *atomic_open(const struct path *path, struct dentry *dentry,
...
3568     if (!error) {
3569         if (file->f_mode & FMODE_OPENED) {
3570             if (unlikely(dentry != file->f_path.dentry)) {
3571                 dput(dentry);
3572                 dentry = dget(file->f_path.dentry);
3573             }

You think this can only happen for a directory?  I figured VFS trying to
work around whatever might have happened inside the filesystem.

One thing that's not happening is that if knfsd /does/ get a different
dentry back, its not updating struct svc_fh->fh_dentry.

Ben