[RFC,0/3] nfsd: close potential race between open and setting delegation

Message

Jeff Layton July 14, 2022, 3:28 p.m. UTC

Here's a first stab at a patchset to close a potential race when setting
a delegation on a file. Between the point where we open the file and
where we set the delegation, another task or client could unlink or
rename the dentry. If that occurs, we shouldn't hand out a delegation
in the open response, but we don't prevent that today.

The basic idea here is to re-do the lookup after setting the delegation.
If the resulting dentry is not the one we have in the open, then we can
reject handing out a delegation.

Only lightly tested, so this is an RFC for now.

Jeff Layton (3):
  nfsd: drop fh argument from alloc_init_deleg
  nfsd: rework arguments to nfs4_set_delegation
  nfsd: vet the opened dentry after setting a delegation

 fs/nfsd/nfs4state.c | 65 ++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 55 insertions(+), 10 deletions(-)

Comments

J. Bruce Fields July 18, 2022, 9:18 p.m. UTC | #1

On Thu, Jul 14, 2022 at 11:28:16AM -0400, Jeff Layton wrote:
> Here's a first stab at a patchset to close a potential race when setting
> a delegation on a file. Between the point where we open the file and
> where we set the delegation, another task or client could unlink or
> rename the dentry. If that occurs, we shouldn't hand out a delegation
> in the open response, but we don't prevent that today.
> 
> The basic idea here is to re-do the lookup after setting the delegation.
> If the resulting dentry is not the one we have in the open, then we can
> reject handing out a delegation.

I have this distinct memory of actually doing that before.

But looking through the git history all I find is 4335723e8e9f "nfsd4:
fix delegation-unlink/rename race", from 2014, which claims to fix a
similar-sounding race in a different way.

How are you reproducing this?

--b.

> 
> Only lightly tested, so this is an RFC for now.
> 
> Jeff Layton (3): nfsd: drop fh argument from alloc_init_deleg nfsd:
> rework arguments to nfs4_set_delegation nfsd: vet the opened dentry
> after setting a delegation
> 
>  fs/nfsd/nfs4state.c | 65
>  ++++++++++++++++++++++++++++++++++++++------- 1 file changed, 55
>  insertions(+), 10 deletions(-)
> 
> -- 2.36.1

Jeff Layton July 18, 2022, 9:43 p.m. UTC | #2

On Mon, 2022-07-18 at 17:18 -0400, J. Bruce Fields wrote:
> On Thu, Jul 14, 2022 at 11:28:16AM -0400, Jeff Layton wrote:
> > Here's a first stab at a patchset to close a potential race when setting
> > a delegation on a file. Between the point where we open the file and
> > where we set the delegation, another task or client could unlink or
> > rename the dentry. If that occurs, we shouldn't hand out a delegation
> > in the open response, but we don't prevent that today.
> > 
> > The basic idea here is to re-do the lookup after setting the delegation.
> > If the resulting dentry is not the one we have in the open, then we can
> > reject handing out a delegation.
> 
> I have this distinct memory of actually doing that before.
> 
> But looking through the git history all I find is 4335723e8e9f "nfsd4:
> fix delegation-unlink/rename race", from 2014, which claims to fix a
> similar-sounding race in a different way.
> 
> How are you reproducing this?
> 
> --b.
> 

I'm not at all, so far. This race is entirely theoretical. Probably we
could reproduce it by introducing some artificial delays or something.

> > 
> > Only lightly tested, so this is an RFC for now.
> > 
> > Jeff Layton (3): nfsd: drop fh argument from alloc_init_deleg nfsd:
> > rework arguments to nfs4_set_delegation nfsd: vet the opened dentry
> > after setting a delegation
> > 
> >  fs/nfsd/nfs4state.c | 65
> >  ++++++++++++++++++++++++++++++++++++++------- 1 file changed, 55
> >  insertions(+), 10 deletions(-)
> > 
> > -- 2.36.1