linux-kernel - Re: [PATCH v1 0/3] Allow knfsd 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: <0C9008B1-2C70-43C4-8532-52D91D6B7ED1@hammerspace.com>
Date: Wed, 19 Nov 2025 07:46:43 -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 0/3] Allow knfsd to use atomic_open()

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

> On Wed, 19 Nov 2025, Benjamin Coddington wrote:
>> We have workloads that will benefit from allowing knfsd to use atomic_open()
>> in the open/create path.  There are two benefits; the first is the original
>> matter of correctness: when knfsd must perform both vfs_create() and
>> vfs_open() in series there can be races or error results that cause the
>> caller to receive unexpected results.  The second benefit is that for some
>> network filesystems, we can reduce the number of remote round-trip
>> operations by using a single atomic_open() path which provides a performance
>> benefit.
>>
>> I've implemented this with the simplest possible change - by modifying
>> dentry_create() which has a single user: knfsd.  The changes cause us to
>> insert ourselves part-way into the previously closed/static atomic_open()
>> path, so I expect VFS folks to have some good ideas about potentially
>> superior approaches.
>
> I think using atomic_open is important - thanks for doing this.
>
> I think there is another race this fixes.
> If the client ends and unchecked v4 OPEN request, nfsd does a lookup and
> finds the name doesn't exist, it will then (currently) use vfs_create()
> requesting an exclusive create.  If this races with a create happening
> from another client, this could result in -EEXIST which is not what the
> client would expect.  Using atomic_open would fix this.
>
> However I cannot see that you ever pass O_EXCL to atomic_open (or did I
> miss something?).  So I don't think the code is quite right yet.  O_EXCL
> should be passed is an exclusive or checked create was requested.

Ah, it's true.  I did not validate knfsd's behaviors, only its interface with
VFS.  IIUC knfsd gets around needing to pass O_EXCL by holding the directory
inode lock over the create, and since it doesn't need to do lookup because
it already has a filehandle, I think O_EXCL is moot.

> With a VFS hat on, I would rather there were more shared code between
> dentry_create() and lookup_open().  I don't know exactly what this would
> look like, and I wouldn't want that desire to hold up this patch, but it
> might be worth thinking about to see if there are any easy similarities
> to exploit.

I agree, that would be nice.  It would definitely be a bigger touch, and I
was going for the minimal change here.

Thanks for looking at this Neil.

Ben