[PATCH v1 0/3] Allow knfsd to use atomic_open()

Benjamin Coddington posted 3 patches 1 week, 6 days ago
There is a newer version of this series
fs/namei.c         | 84 ++++++++++++++++++++++++++++++++++++++++++----
fs/nfsd/nfs4proc.c |  8 +++--
fs/open.c          | 41 ----------------------
include/linux/fs.h |  2 +-
4 files changed, 83 insertions(+), 52 deletions(-)
[PATCH v1 0/3] Allow knfsd to use atomic_open()
Posted by Benjamin Coddington 1 week, 6 days ago
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.

Thanks for any comment and critique.

Benjamin Coddington (3):
  VFS: move dentry_create() from fs/open.c to fs/namei.c
  VFS: Prepare atomic_open() for dentry_create()
  VFS/knfsd: Teach dentry_create() to use atomic_open()

 fs/namei.c         | 84 ++++++++++++++++++++++++++++++++++++++++++----
 fs/nfsd/nfs4proc.c |  8 +++--
 fs/open.c          | 41 ----------------------
 include/linux/fs.h |  2 +-
 4 files changed, 83 insertions(+), 52 deletions(-)

-- 
2.50.1
Re: [PATCH v1 0/3] Allow knfsd to use atomic_open()
Posted by NeilBrown 1 week, 6 days ago
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.

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.

Thanks,
NeilBrown


> 
> Thanks for any comment and critique.
> 
> Benjamin Coddington (3):
>   VFS: move dentry_create() from fs/open.c to fs/namei.c
>   VFS: Prepare atomic_open() for dentry_create()
>   VFS/knfsd: Teach dentry_create() to use atomic_open()
> 
>  fs/namei.c         | 84 ++++++++++++++++++++++++++++++++++++++++++----
>  fs/nfsd/nfs4proc.c |  8 +++--
>  fs/open.c          | 41 ----------------------
>  include/linux/fs.h |  2 +-
>  4 files changed, 83 insertions(+), 52 deletions(-)
> 
> -- 
> 2.50.1
> 
>
Re: [PATCH v1 0/3] Allow knfsd to use atomic_open()
Posted by Benjamin Coddington 1 week, 5 days ago
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
Re: [PATCH v1 0/3] Allow knfsd to use atomic_open()
Posted by NeilBrown 1 week, 4 days ago
On Wed, 19 Nov 2025, Benjamin Coddington wrote:
> 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.

Holding the directory lock is sufficient for providing O_EXCL for local
filesystems which will be blocked from creating while that lock is held.
It is *not* sufficient for remote filesystems which are precisely those
which provide ->atomic_open.

The fact that you are adding support for atomic_open means that O_EXCL
isn't moot.

I don't know what you mean by "since it doesn't need to do lookup because
it already has a filehandle".  What filehandle does it already have?

Thanks,
NeilBrown


> 
> > 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
> 
Re: [PATCH v1 0/3] Allow knfsd to use atomic_open()
Posted by Benjamin Coddington 1 week, 4 days ago
On 20 Nov 2025, at 17:26, NeilBrown wrote:

> On Wed, 19 Nov 2025, Benjamin Coddington wrote:
>
>> 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.
>
> Holding the directory lock is sufficient for providing O_EXCL for local
> filesystems which will be blocked from creating while that lock is held.
> It is *not* sufficient for remote filesystems which are precisely those
> which provide ->atomic_open.
>
> The fact that you are adding support for atomic_open means that O_EXCL
> isn't moot.

I mean to say: knfsd doesn't need to pass O_EXCL because its already taking
care to produce an exclusive open via nfsv4 semantics.

> I don't know what you mean by "since it doesn't need to do lookup because
> it already has a filehandle".  What filehandle does it already have?

The client has sent along the filehandle of the parent directory, and knfsd
has already done lookup_one() on the child name, and we pass along that
negative dentry thet we looked up while holding the directory's inode lock.

Ben
Re: [PATCH v1 0/3] Allow knfsd to use atomic_open()
Posted by NeilBrown 5 days, 5 hours ago
On Fri, 21 Nov 2025, Benjamin Coddington wrote:
> On 20 Nov 2025, at 17:26, NeilBrown wrote:
> 
> > On Wed, 19 Nov 2025, Benjamin Coddington wrote:
> >
> >> 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.
> >
> > Holding the directory lock is sufficient for providing O_EXCL for local
> > filesystems which will be blocked from creating while that lock is held.
> > It is *not* sufficient for remote filesystems which are precisely those
> > which provide ->atomic_open.
> >
> > The fact that you are adding support for atomic_open means that O_EXCL
> > isn't moot.
> 
> I mean to say: knfsd doesn't need to pass O_EXCL because its already taking
> care to produce an exclusive open via nfsv4 semantics.

Huh?

The interesting circumstance here is an NFS re-export of an NFS
filesystem - is that right?

The only way that an exclusive create can be achieved on the target
filesystem is if an NFS4_CREATE_EXCLUSIVE4_1 (or similar) create request
is sent to the ultimate sever.  There is nothing knfsd can do to
produce exclusive open semantics on a remote NFS serve except to
explicitly request them.

> 
> > I don't know what you mean by "since it doesn't need to do lookup because
> > it already has a filehandle".  What filehandle does it already have?
> 
> The client has sent along the filehandle of the parent directory, and knfsd
> has already done lookup_one() on the child name, and we pass along that
> negative dentry thet we looked up while holding the directory's inode lock.

This (holding the directory's inode lock) works perfectly well for local
filesystems (which don't implement ->atomic_open).  It has no effect on
remote filesystems (which is why we have ->atomic_open).

Thanks,
NeilBrown
Re: [PATCH v1 0/3] Allow knfsd to use atomic_open()
Posted by Benjamin Coddington 5 days, 4 hours ago
On 26 Nov 2025, at 15:59, NeilBrown wrote:

> On Fri, 21 Nov 2025, Benjamin Coddington wrote:
>> On 20 Nov 2025, at 17:26, NeilBrown wrote:
>>
>>> On Wed, 19 Nov 2025, Benjamin Coddington wrote:
>>>
>>>> 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.
>>>
>>> Holding the directory lock is sufficient for providing O_EXCL for local
>>> filesystems which will be blocked from creating while that lock is held.
>>> It is *not* sufficient for remote filesystems which are precisely those
>>> which provide ->atomic_open.
>>>
>>> The fact that you are adding support for atomic_open means that O_EXCL
>>> isn't moot.
>>
>> I mean to say: knfsd doesn't need to pass O_EXCL because its already taking
>> care to produce an exclusive open via nfsv4 semantics.
>
> Huh?
>
> The interesting circumstance here is an NFS re-export of an NFS
> filesystem - is that right?

That's right.

> The only way that an exclusive create can be achieved on the target
> filesystem is if an NFS4_CREATE_EXCLUSIVE4_1 (or similar) create request
> is sent to the ultimate sever.  There is nothing knfsd can do to
> produce exclusive open semantics on a remote NFS serve except to
> explicitly request them.

True - but I haven't really been worried about that, so I think I see what
you're getting at now - you'd like kNFSD to start using O_EXCL when it
receives NFS4_CREATE_EXCLUSIVE4_1.

I think that's a whole different change on its own, but not necessary
here because these changes are targeting a very specific problem - the
problem where open(O_CREAT) is done in two operations on the remote
filesystem.  That problem is solved by this patchset, and I don't think the
solution is incomplete because we're not passing O_EXCL for the
NFS4_CREATE_EXCLUSIVE{4_1} case.  I think that's a new enhancement - one
that I haven't thought through (yet) or tested.

Up until now, kNFSD has not bothered fiddling with O_EXCL because of the
reasons I listed above - for local filesystems or remote.

Do you disagree that the changes here for the open(O_CREAT) problem is
incomplete without new O_EXCL passing to atomic_open()?  If so, do we also
need to consider passing O_EXCL when kNFSD does vfs_open() for the case when
the filesystem does not have atomic_open()?

Thanks for engaging with me,
Ben
Re: [PATCH v1 0/3] Allow knfsd to use atomic_open()
Posted by NeilBrown 5 days, 1 hour ago
On Thu, 27 Nov 2025, Benjamin Coddington wrote:
> On 26 Nov 2025, at 15:59, NeilBrown wrote:
> 
> > On Fri, 21 Nov 2025, Benjamin Coddington wrote:
> >> On 20 Nov 2025, at 17:26, NeilBrown wrote:
> >>
> >>> On Wed, 19 Nov 2025, Benjamin Coddington wrote:
> >>>
> >>>> 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.
> >>>
> >>> Holding the directory lock is sufficient for providing O_EXCL for local
> >>> filesystems which will be blocked from creating while that lock is held.
> >>> It is *not* sufficient for remote filesystems which are precisely those
> >>> which provide ->atomic_open.
> >>>
> >>> The fact that you are adding support for atomic_open means that O_EXCL
> >>> isn't moot.
> >>
> >> I mean to say: knfsd doesn't need to pass O_EXCL because its already taking
> >> care to produce an exclusive open via nfsv4 semantics.
> >
> > Huh?
> >
> > The interesting circumstance here is an NFS re-export of an NFS
> > filesystem - is that right?
> 
> That's right.
> 
> > The only way that an exclusive create can be achieved on the target
> > filesystem is if an NFS4_CREATE_EXCLUSIVE4_1 (or similar) create request
> > is sent to the ultimate sever.  There is nothing knfsd can do to
> > produce exclusive open semantics on a remote NFS serve except to
> > explicitly request them.
> 
> True - but I haven't really been worried about that, so I think I see what
> you're getting at now - you'd like kNFSD to start using O_EXCL when it
> receives NFS4_CREATE_EXCLUSIVE4_1.
> 
> I think that's a whole different change on its own, but not necessary
> here because these changes are targeting a very specific problem - the
> problem where open(O_CREAT) is done in two operations on the remote
> filesystem.  That problem is solved by this patchset, and I don't think the
> solution is incomplete because we're not passing O_EXCL for the
> NFS4_CREATE_EXCLUSIVE{4_1} case.  I think that's a new enhancement - one
> that I haven't thought through (yet) or tested.
> 
> Up until now, kNFSD has not bothered fiddling with O_EXCL because of the
> reasons I listed above - for local filesystems or remote.
> 
> Do you disagree that the changes here for the open(O_CREAT) problem is
> incomplete without new O_EXCL passing to atomic_open()? 

It isn't so much that the change is incomplete.  Rather, the change
introduces a regression.

The old code was

-	error = vfs_create(mnt_idmap(path->mnt),
-			   d_inode(path->dentry->d_parent),
-			   path->dentry, mode, true);


Note the "true" at the end.  This instructs nfs_create() to pass O_EXCL
to nfs_do_create() so an over-the-wire exclusive create is performed.

The new code is

+		dentry = atomic_open(path, dentry, file, flags, mode);

Where "flags" is oflags from nfsd4_vfs_create() which is 
   O_CREAT| O_LARGEFILE | O_(read/write/rdwr)
and no O_EXCL.
(When atomic_open is called by lookup_open, "open_flag" is passed which
might contain O_EXCL).

>                                                          If so, do we also
> need to consider passing O_EXCL when kNFSD does vfs_open() for the case when
> the filesystem does not have atomic_open()?

No as vfs_open() doesn't do the create, vfs_create() does that.
And we do need to pass (the equivalent of) O_EXCL when calling
vfs_create().  In fact we do - that 'true' as the large arg means
exactly O_EXCL.
(really we shouldn't be passing 'true' if an exclusive create wasn't
requested, but it only makes a difference for filesystems that support
->atomic_open, so it doesn't actually matter what we pass - and Jeff
has a patch to remove that last arg to vfs_create()).


> 
> Thanks for engaging with me,
> Ben
> 

Thanks,
NeilBrown
Re: [PATCH v1 0/3] Allow knfsd to use atomic_open()
Posted by Benjamin Coddington 4 days, 13 hours ago
On 26 Nov 2025, at 19:36, NeilBrown wrote:
> It isn't so much that the change is incomplete.  Rather, the change
> introduces a regression.
>
> The old code was
>
> -	error = vfs_create(mnt_idmap(path->mnt),
> -			   d_inode(path->dentry->d_parent),
> -			   path->dentry, mode, true);
>
>
> Note the "true" at the end.  This instructs nfs_create() to pass O_EXCL
> to nfs_do_create() so an over-the-wire exclusive create is performed.
>
> The new code is
>
> +		dentry = atomic_open(path, dentry, file, flags, mode);
>
> Where "flags" is oflags from nfsd4_vfs_create() which is
>    O_CREAT| O_LARGEFILE | O_(read/write/rdwr)
> and no O_EXCL.
> (When atomic_open is called by lookup_open, "open_flag" is passed which
> might contain O_EXCL).

Of course, you're quite right, I should put more effort into trying to
understand your very first reply.

Fixing this up seems simple enough, I think we just need to do this on top
of what's here:

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 7e39234e0649..6990ba92bca1 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -202,6 +202,9 @@ nfsd4_vfs_create(struct svc_fh *fhp, struct dentry **child,
        int oflags;

        oflags = O_CREAT | O_LARGEFILE;
+       if (nfsd4_create_is_exclusive(open->op_createmode))
+               oflags |= O_EXCL;
+
        switch (open->op_share_access & NFS4_SHARE_ACCESS_BOTH) {
        case NFS4_SHARE_ACCESS_WRITE:
                oflags |= O_WRONLY;

I will send this through my re-export testing, but I don't think that its
going to produce different results because we lack a multi-client test to
detect cases for O_EXCL.

Ben
Re: [PATCH v1 0/3] Allow knfsd to use atomic_open()
Posted by Jeff Layton 1 week, 6 days ago
On Tue, 2025-11-18 at 11:33 -0500, 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.
> 
> Thanks for any comment and critique.
> 
> Benjamin Coddington (3):
>   VFS: move dentry_create() from fs/open.c to fs/namei.c
>   VFS: Prepare atomic_open() for dentry_create()
>   VFS/knfsd: Teach dentry_create() to use atomic_open()
> 
>  fs/namei.c         | 84 ++++++++++++++++++++++++++++++++++++++++++----
>  fs/nfsd/nfs4proc.c |  8 +++--
>  fs/open.c          | 41 ----------------------
>  include/linux/fs.h |  2 +-
>  4 files changed, 83 insertions(+), 52 deletions(-)

Nice work, Ben. This looks pretty reasonable to me, and I agree that
using atomic_open is desirable for preventing races.

Reviewed-by: Jeff Layton <jlayton@kernel.org>
Re: [PATCH v1 0/3] Allow knfsd to use atomic_open()
Posted by Chuck Lever 1 week, 6 days ago
On 11/18/25 11:33 AM, 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.

Commit fb70bf124b05 ("NFSD: Instantiate a struct file when creating a
regular NFSv4 file") was supposed to address this. If there are still
issues, then a Fixes: tag and some explanation of where there are gaps
would be welcome in the commit message or cover letter. We might need
to identify LTS backport requirements, in that case.


> 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.
> 
> Thanks for any comment and critique.
> 
> Benjamin Coddington (3):
>   VFS: move dentry_create() from fs/open.c to fs/namei.c
>   VFS: Prepare atomic_open() for dentry_create()
>   VFS/knfsd: Teach dentry_create() to use atomic_open()
> 
>  fs/namei.c         | 84 ++++++++++++++++++++++++++++++++++++++++++----
>  fs/nfsd/nfs4proc.c |  8 +++--
>  fs/open.c          | 41 ----------------------
>  include/linux/fs.h |  2 +-
>  4 files changed, 83 insertions(+), 52 deletions(-)
> 


-- 
Chuck Lever
Re: [PATCH v1 0/3] Allow knfsd to use atomic_open()
Posted by Trond Myklebust 1 week, 6 days ago
On Tue, 2025-11-18 at 11:58 -0500, Chuck Lever wrote:
> On 11/18/25 11:33 AM, 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.
> 
> Commit fb70bf124b05 ("NFSD: Instantiate a struct file when creating a
> regular NFSv4 file") was supposed to address this. If there are still
> issues, then a Fixes: tag and some explanation of where there are
> gaps
> would be welcome in the commit message or cover letter. We might need
> to identify LTS backport requirements, in that case.
> 

That patch only fixes the case where you're creating a local file and
then exporting it over NFSv4.

The case where we see a permissions problem is when creating a file
over NFSv4, and then exporting it over NFSv3.
i.e. it is the re-exporting over NFSv3 case.

Note that independently of the permissions issues, atomic_open also
solves races in open(O_CREAT|O_TRUNC). The NFS client now uses it for
both NFSv4 and NFSv3 for that reason.
See commit 7c6c5249f061 "NFS: add atomic_open for NFSv3 to handle
O_TRUNC correctly."

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trondmy@kernel.org, trond.myklebust@hammerspace.com
Re: [PATCH v1 0/3] Allow knfsd to use atomic_open()
Posted by Benjamin Coddington 1 week, 6 days ago
On 18 Nov 2025, at 11:58, Chuck Lever wrote:

> On 11/18/25 11:33 AM, 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.
>
> Commit fb70bf124b05 ("NFSD: Instantiate a struct file when creating a
> regular NFSv4 file") was supposed to address this. If there are still
> issues, then a Fixes: tag and some explanation of where there are gaps
> would be welcome in the commit message or cover letter. We might need
> to identify LTS backport requirements, in that case.

The problem was noticed on a test that did open O_CREAT with mode 0 which
will succeed in creating the file but will return -EACCES from vfs_open() -
this specific test is mentioned in 3/3 description.  Insane case, but I
suppose someone might want it to behave properly.

Thanks for the commit pointer, I will check out the BugLink on it.  I will
add the Fixes tag in the next version if one emerges.

Ben