| Message ID | 20220714152819.128276-3-jlayton@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nfsd: close potential race between open and setting delegation | expand |
> On Jul 14, 2022, at 11:28 AM, Jeff Layton <jlayton@kernel.org> wrote: > > We'll need the nfs4_open to vet the filename. Change nfs4_set_delegation > to take the same arguments are nfs4_open_delegation. ^are^as Nit: Considering that in the next patch you change the synopsis of nfs4_open_delegation again but not nfs4_set_delegation, this description causes a little whiplash. > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/nfsd/nfs4state.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index 4f81c0bbd27b..347794028c98 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -5260,10 +5260,12 @@ static int nfsd4_check_conflicting_opens(struct nfs4_client *clp, > } > > static struct nfs4_delegation * > -nfs4_set_delegation(struct nfs4_client *clp, > - struct nfs4_file *fp, struct nfs4_clnt_odstate *odstate) > +nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp) > { > int status = 0; > + struct nfs4_client *clp = stp->st_stid.sc_client; > + struct nfs4_file *fp = stp->st_stid.sc_file; > + struct nfs4_clnt_odstate *odstate = stp->st_clnt_odstate; > struct nfs4_delegation *dp; > struct nfsd_file *nf; > struct file_lock *fl; > @@ -5405,7 +5407,7 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp) > default: > goto out_no_deleg; > } > - dp = nfs4_set_delegation(clp, stp->st_stid.sc_file, stp->st_clnt_odstate); > + dp = nfs4_set_delegation(open, stp); > if (IS_ERR(dp)) > goto out_no_deleg; > > -- > 2.36.1 > -- Chuck Lever
On Thu, 2022-07-14 at 16:47 +0000, Chuck Lever III wrote: > > > On Jul 14, 2022, at 11:28 AM, Jeff Layton <jlayton@kernel.org> wrote: > > > > We'll need the nfs4_open to vet the filename. Change nfs4_set_delegation > > to take the same arguments are nfs4_open_delegation. > > ^are^as > > Nit: Considering that in the next patch you change the synopsis of > nfs4_open_delegation again but not nfs4_set_delegation, this > description causes a little whiplash. > > Yeah, I should have squashed a couple of those together. I _did_ say it was an RFC. I can resend a cleaned-up version later if you want to take this in. > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > fs/nfsd/nfs4state.c | 8 +++++--- > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > > index 4f81c0bbd27b..347794028c98 100644 > > --- a/fs/nfsd/nfs4state.c > > +++ b/fs/nfsd/nfs4state.c > > @@ -5260,10 +5260,12 @@ static int nfsd4_check_conflicting_opens(struct nfs4_client *clp, > > } > > > > static struct nfs4_delegation * > > -nfs4_set_delegation(struct nfs4_client *clp, > > - struct nfs4_file *fp, struct nfs4_clnt_odstate *odstate) > > +nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp) > > { > > int status = 0; > > + struct nfs4_client *clp = stp->st_stid.sc_client; > > + struct nfs4_file *fp = stp->st_stid.sc_file; > > + struct nfs4_clnt_odstate *odstate = stp->st_clnt_odstate; > > struct nfs4_delegation *dp; > > struct nfsd_file *nf; > > struct file_lock *fl; > > @@ -5405,7 +5407,7 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp) > > default: > > goto out_no_deleg; > > } > > - dp = nfs4_set_delegation(clp, stp->st_stid.sc_file, stp->st_clnt_odstate); > > + dp = nfs4_set_delegation(open, stp); > > if (IS_ERR(dp)) > > goto out_no_deleg; > > > > -- > > 2.36.1 > > > > -- > Chuck Lever > > >
> On Jul 14, 2022, at 1:12 PM, Jeff Layton <jlayton@kernel.org> wrote: > > On Thu, 2022-07-14 at 16:47 +0000, Chuck Lever III wrote: >> >>> On Jul 14, 2022, at 11:28 AM, Jeff Layton <jlayton@kernel.org> wrote: >>> >>> We'll need the nfs4_open to vet the filename. Change nfs4_set_delegation >>> to take the same arguments are nfs4_open_delegation. >> >> ^are^as >> >> Nit: Considering that in the next patch you change the synopsis of >> nfs4_open_delegation again but not nfs4_set_delegation, this >> description causes a little whiplash. >> >> > > Yeah, I should have squashed a couple of those together. I _did_ say it > was an RFC. I can resend a cleaned-up version later if you want to take > this in. I'm interested in Neil's thoughts about this approach, but I'm willing to run with it unless test results show a regression. >>> Signed-off-by: Jeff Layton <jlayton@kernel.org> >>> --- >>> fs/nfsd/nfs4state.c | 8 +++++--- >>> 1 file changed, 5 insertions(+), 3 deletions(-) >>> >>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c >>> index 4f81c0bbd27b..347794028c98 100644 >>> --- a/fs/nfsd/nfs4state.c >>> +++ b/fs/nfsd/nfs4state.c >>> @@ -5260,10 +5260,12 @@ static int nfsd4_check_conflicting_opens(struct nfs4_client *clp, >>> } >>> >>> static struct nfs4_delegation * >>> -nfs4_set_delegation(struct nfs4_client *clp, >>> - struct nfs4_file *fp, struct nfs4_clnt_odstate *odstate) >>> +nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp) >>> { >>> int status = 0; >>> + struct nfs4_client *clp = stp->st_stid.sc_client; >>> + struct nfs4_file *fp = stp->st_stid.sc_file; >>> + struct nfs4_clnt_odstate *odstate = stp->st_clnt_odstate; >>> struct nfs4_delegation *dp; >>> struct nfsd_file *nf; >>> struct file_lock *fl; >>> @@ -5405,7 +5407,7 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp) >>> default: >>> goto out_no_deleg; >>> } >>> - dp = nfs4_set_delegation(clp, stp->st_stid.sc_file, stp->st_clnt_odstate); >>> + dp = nfs4_set_delegation(open, stp); >>> if (IS_ERR(dp)) >>> goto out_no_deleg; >>> >>> -- >>> 2.36.1 >>> >> >> -- >> Chuck Lever >> >> >> > > -- > Jeff Layton <jlayton@kernel.org> -- Chuck Lever
On Thu, 2022-07-14 at 17:14 +0000, Chuck Lever III wrote: > > > > On Jul 14, 2022, at 1:12 PM, Jeff Layton <jlayton@kernel.org> wrote: > > > > On Thu, 2022-07-14 at 16:47 +0000, Chuck Lever III wrote: > > > > > > > On Jul 14, 2022, at 11:28 AM, Jeff Layton <jlayton@kernel.org> wrote: > > > > > > > > We'll need the nfs4_open to vet the filename. Change nfs4_set_delegation > > > > to take the same arguments are nfs4_open_delegation. > > > > > > ^are^as > > > > > > Nit: Considering that in the next patch you change the synopsis of > > > nfs4_open_delegation again but not nfs4_set_delegation, this > > > description causes a little whiplash. > > > > > > > > > > Yeah, I should have squashed a couple of those together. I _did_ say it > > was an RFC. I can resend a cleaned-up version later if you want to take > > this in. > > I'm interested in Neil's thoughts about this approach, but I'm > willing to run with it unless test results show a regression. > > ...and there is a regression. Partial lockdep pop here: Jul 14 14:46:54 quad3 kernel: ============================================ Jul 14 14:46:54 quad3 kernel: WARNING: possible recursive locking detected Jul 14 14:46:54 quad3 kernel: 5.19.0-rc5+ #316 Tainted: G OE Jul 14 14:46:54 quad3 kernel: -------------------------------------------- Jul 14 14:46:54 quad3 kernel: nfsd/1148 is trying to acquire lock: Jul 14 14:46:54 quad3 kernel: ffff88812a3a7388 (&inode->i_sb->s_type->i_mutex_dir_key/1){++++}-{3:3}, at: nfsd4_process_open2+0x1890/0x2710 [nfsd] Jul 14 14:46:54 quad3 kernel: but task is already holding lock: Jul 14 14:46:54 quad3 kernel: ffff88812a3a7388 (&inode->i_sb->s_type->i_mutex_dir_key/1){++++}-{3:3}, at: nfsd_lookup_dentry+0x16f/0x6a0 [nfsd] Jul 14 14:46:54 quad3 kernel: other info that might help us debug this: Jul 14 14:46:54 quad3 kernel: Possible unsafe locking scenario: Jul 14 14:46:54 quad3 kernel: CPU0 Jul 14 14:46:54 quad3 kernel: ---- Jul 14 14:46:54 quad3 kernel: lock(&inode->i_sb->s_type->i_mutex_dir_key/1); Jul 14 14:46:54 quad3 kernel: lock(&inode->i_sb->s_type->i_mutex_dir_key/1); Jul 14 14:46:54 quad3 kernel: *** DEADLOCK *** Jul 14 14:46:54 quad3 kernel: May be due to missing lock nesting notation Jul 14 14:46:54 quad3 kernel: 1 lock held by nfsd/1148: Jul 14 14:46:54 quad3 kernel: #0: ffff88812a3a7388 (&inode->i_sb->s_type->i_mutex_dir_key/1){++++}-{3:3}, at: nfsd_lookup_dentry+0x16f/0x6a0 [nfsd] Jul 14 14:46:54 quad3 kernel: The core problem is the unclear locking in nfsd_lookup_dentry. Sometimes that returns with the i_rwsem held, but there's no clear indication of whether that's the case when the function returns. I guess fh_unlock just takes care of that (which is a little scary, tbqh). Now that I've taken a stab at it, I don't see how we can fix this w/o taking Neil's locking cleanups series first. I think I'll pull that in and try to redo this series on top of it. Cheers,
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 4f81c0bbd27b..347794028c98 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -5260,10 +5260,12 @@ static int nfsd4_check_conflicting_opens(struct nfs4_client *clp, } static struct nfs4_delegation * -nfs4_set_delegation(struct nfs4_client *clp, - struct nfs4_file *fp, struct nfs4_clnt_odstate *odstate) +nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp) { int status = 0; + struct nfs4_client *clp = stp->st_stid.sc_client; + struct nfs4_file *fp = stp->st_stid.sc_file; + struct nfs4_clnt_odstate *odstate = stp->st_clnt_odstate; struct nfs4_delegation *dp; struct nfsd_file *nf; struct file_lock *fl; @@ -5405,7 +5407,7 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp) default: goto out_no_deleg; } - dp = nfs4_set_delegation(clp, stp->st_stid.sc_file, stp->st_clnt_odstate); + dp = nfs4_set_delegation(open, stp); if (IS_ERR(dp)) goto out_no_deleg;
We'll need the nfs4_open to vet the filename. Change nfs4_set_delegation to take the same arguments are nfs4_open_delegation. Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/nfsd/nfs4state.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)