[PATCH 0/2] nfsd/dlm: fix knfsd panic when NFSv4 client does GETLK call

[PATCH 0/2] nfsd/dlm: fix knfsd panic when NFSv4 client does GETLK call

[Jeff Layton]

This patchset fixes a regression due to this patch:

----------------[snip]----------------
commit 55ef1274dddd4de387c54d110e354ffbb6cdc706
Author: J. Bruce Fields <bfields@citi.umich.edu>
Date:   Sat Dec 20 11:58:38 2008 -0800

    nfsd: Ensure nfsv4 calls the underlying filesystem on LOCKT
----------------[snip]----------------

To reproduce, set up a nfs server that is serving out a GFS2 filesystem.
Set a lock locally on a file on the GFS2 export on the server. From a
NFSv4 client, do a GETLK against the same file. The server will oops due
to a NULL pointer dereference. The fl_lmops will be set, but the
fl_owner will be a NULL pointer. The knfsd code does not account for
this possibility. It assumes that when fl_lmops is set this way that
the fl_owner will point to a valid nfs4_stateowner struct.

In actuality, Bruce's patch is correct, but it exposes a bug in DLM's
GETLK codepath. The first patch in this set fixes that.

The second patch fixes knfsd to be a little more careful about the
file_lock struct it builds to pass to vfs_test_lock.

Either patch should prevent the panic, though I think applying both
patches is the best approach to fixing this.

Jeff Layton (2):
  dlm: initialize file_lock struct in GETLK before copying conflicting
    lock
  nfsd: only set file_lock.fl_lmops in nfsd4_lockt if a stateowner is
    found

 fs/dlm/plock.c      |    2 ++
 fs/nfsd/nfs4state.c |    1 -
 2 files changed, 2 insertions(+), 1 deletions(-)

[PATCH 1/2] dlm: initialize file_lock struct in GETLK before copying conflicting lock

[Jeff Layton]

dlm_posix_get fills out the relevant fields in the file_lock before
returning when there is a lock conflict, but doesn't clean out any of
the other fields in the file_lock.

When nfsd does a NFSv4 lockt call, it sets the fl_lmops to
nfsd_posix_mng_ops before calling the lower fs. When the lock comes back
after testing a lock on GFS2, it still has that field set. This confuses
nfsd into thinking that the file_lock is a nfsd4 lock.

Fix this by making DLM reinitialize the file_lock before copying the
fields from the conflicting lock.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/dlm/plock.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/fs/dlm/plock.c b/fs/dlm/plock.c
index eba87ff..ca46f11 100644
--- a/fs/dlm/plock.c
+++ b/fs/dlm/plock.c
@@ -304,7 +304,9 @@ int dlm_posix_get(dlm_lockspace_t *lockspace, u64 number, struct file *file,
 	if (rv == -ENOENT)
 		rv = 0;
 	else if (rv > 0) {
+		locks_init_lock(fl);
 		fl->fl_type = (op->info.ex) ? F_WRLCK : F_RDLCK;
+		fl->fl_flags = FL_POSIX;
 		fl->fl_pid = op->info.pid;
 		fl->fl_start = op->info.start;
 		fl->fl_end = op->info.end;
-- 
1.5.5.6

Re: [PATCH 1/2] dlm: initialize file_lock struct in GETLK before copying conflicting lock

[J. Bruce Fields]

On Thu, Jan 22, 2009 at 02:16:03PM -0500, Jeff Layton wrote:
> dlm_posix_get fills out the relevant fields in the file_lock before
> returning when there is a lock conflict, but doesn't clean out any of
> the other fields in the file_lock.
> 
> When nfsd does a NFSv4 lockt call, it sets the fl_lmops to
> nfsd_posix_mng_ops before calling the lower fs. When the lock comes back
> after testing a lock on GFS2, it still has that field set. This confuses
> nfsd into thinking that the file_lock is a nfsd4 lock.
> 
> Fix this by making DLM reinitialize the file_lock before copying the
> fields from the conflicting lock.
> 
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

I'll leave this one to gfs2 people to apply unless I'm told otherwise.

--b.

> ---
>  fs/dlm/plock.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/dlm/plock.c b/fs/dlm/plock.c
> index eba87ff..ca46f11 100644
> --- a/fs/dlm/plock.c
> +++ b/fs/dlm/plock.c
> @@ -304,7 +304,9 @@ int dlm_posix_get(dlm_lockspace_t *lockspace, u64 number, struct file *file,
>  	if (rv == -ENOENT)
>  		rv = 0;
>  	else if (rv > 0) {
> +		locks_init_lock(fl);
>  		fl->fl_type = (op->info.ex) ? F_WRLCK : F_RDLCK;
> +		fl->fl_flags = FL_POSIX;
>  		fl->fl_pid = op->info.pid;
>  		fl->fl_start = op->info.start;
>  		fl->fl_end = op->info.end;
> -- 
> 1.5.5.6
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] dlm: initialize file_lock struct in GETLK before copying conflicting lock

[Jeff Layton]

On Tue, 27 Jan 2009 17:34:01 -0500
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Thu, Jan 22, 2009 at 02:16:03PM -0500, Jeff Layton wrote:
> > dlm_posix_get fills out the relevant fields in the file_lock before
> > returning when there is a lock conflict, but doesn't clean out any of
> > the other fields in the file_lock.
> > 
> > When nfsd does a NFSv4 lockt call, it sets the fl_lmops to
> > nfsd_posix_mng_ops before calling the lower fs. When the lock comes back
> > after testing a lock on GFS2, it still has that field set. This confuses
> > nfsd into thinking that the file_lock is a nfsd4 lock.
> > 
> > Fix this by making DLM reinitialize the file_lock before copying the
> > fields from the conflicting lock.
> > 
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> 
> I'll leave this one to gfs2 people to apply unless I'm told otherwise.
> 
> --b.
> 

That should be fine. Dave T. has taken this into his tree and is
pushing it to Linus.

Thanks,
-- 
Jeff Layton <jlayton@redhat.com>

[PATCH 2/2] nfsd: only set file_lock.fl_lmops in nfsd4_lockt if a stateowner is found

[Jeff Layton]

nfsd4_lockt does a search for a lockstateowner when building the lock
struct to test. If one is found, it'll set fl_owner to it. Regardless of
whether that happens, it'll also set fl_lmops. Given that this lock is
basically a "lightweight" lock that's just used for checking conflicts,
setting fl_lmops is probably not appropriate for it.

This behavior exposed a bug in DLM's GETLK implementation where it
wasn't clearing out the fields in the file_lock before filling in
conflicting lock info. While we were able to fix this in DLM, it
still seems pointless and dangerous to set the fl_lmops this way
when we may have a NULL lockstateowner.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nfsd/nfs4state.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 88db7d3..b6f60f4 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -2871,7 +2871,6 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		file_lock.fl_owner = (fl_owner_t)lockt->lt_stateowner;
 	file_lock.fl_pid = current->tgid;
 	file_lock.fl_flags = FL_POSIX;
-	file_lock.fl_lmops = &nfsd_posix_mng_ops;
 
 	file_lock.fl_start = lockt->lt_offset;
 	file_lock.fl_end = last_byte_offset(lockt->lt_offset, lockt->lt_length);
-- 
1.5.5.6

Re: [PATCH 2/2] nfsd: only set file_lock.fl_lmops in nfsd4_lockt if a stateowner is found

[J. Bruce Fields]

On Thu, Jan 22, 2009 at 02:16:04PM -0500, Jeff Layton wrote:
> nfsd4_lockt does a search for a lockstateowner when building the lock
> struct to test. If one is found, it'll set fl_owner to it. Regardless of
> whether that happens, it'll also set fl_lmops. Given that this lock is
> basically a "lightweight" lock that's just used for checking conflicts,
> setting fl_lmops is probably not appropriate for it.
> 
> This behavior exposed a bug in DLM's GETLK implementation where it
> wasn't clearing out the fields in the file_lock before filling in
> conflicting lock info. While we were able to fix this in DLM, it
> still seems pointless and dangerous to set the fl_lmops this way
> when we may have a NULL lockstateowner.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>

Thanks, applied.--b.

> ---
>  fs/nfsd/nfs4state.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 88db7d3..b6f60f4 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -2871,7 +2871,6 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  		file_lock.fl_owner = (fl_owner_t)lockt->lt_stateowner;
>  	file_lock.fl_pid = current->tgid;
>  	file_lock.fl_flags = FL_POSIX;
> -	file_lock.fl_lmops = &nfsd_posix_mng_ops;
>  
>  	file_lock.fl_start = lockt->lt_offset;
>  	file_lock.fl_end = last_byte_offset(lockt->lt_offset, lockt->lt_length);
> -- 
> 1.5.5.6
> 

Re: [PATCH 0/2] nfsd/dlm: fix knfsd panic when NFSv4 client does GETLK call

[J. Bruce Fields]

On Thu, Jan 22, 2009 at 02:16:02PM -0500, Jeff Layton wrote:
> This patchset fixes a regression due to this patch:

These look reasonable, thanks.

My testing infrastructure and stuff is still down, so I may be a little
slow getting these upstream.... Bug me again next week if I haven't done
anything and you think of it.

--b.

> 
> ----------------[snip]----------------
> commit 55ef1274dddd4de387c54d110e354ffbb6cdc706
> Author: J. Bruce Fields <bfields@citi.umich.edu>
> Date:   Sat Dec 20 11:58:38 2008 -0800
> 
>     nfsd: Ensure nfsv4 calls the underlying filesystem on LOCKT
> ----------------[snip]----------------
> 
> To reproduce, set up a nfs server that is serving out a GFS2 filesystem.
> Set a lock locally on a file on the GFS2 export on the server. From a
> NFSv4 client, do a GETLK against the same file. The server will oops due
> to a NULL pointer dereference. The fl_lmops will be set, but the
> fl_owner will be a NULL pointer. The knfsd code does not account for
> this possibility. It assumes that when fl_lmops is set this way that
> the fl_owner will point to a valid nfs4_stateowner struct.
> 
> In actuality, Bruce's patch is correct, but it exposes a bug in DLM's
> GETLK codepath. The first patch in this set fixes that.
> 
> The second patch fixes knfsd to be a little more careful about the
> file_lock struct it builds to pass to vfs_test_lock.
> 
> Either patch should prevent the panic, though I think applying both
> patches is the best approach to fixing this.
> 
> Jeff Layton (2):
>   dlm: initialize file_lock struct in GETLK before copying conflicting
>     lock
>   nfsd: only set file_lock.fl_lmops in nfsd4_lockt if a stateowner is
>     found
> 
>  fs/dlm/plock.c      |    2 ++
>  fs/nfsd/nfs4state.c |    1 -
>  2 files changed, 2 insertions(+), 1 deletions(-)
> 

[PATCH 0/2] nfsd/dlm: fix knfsd panic when NFSv4 client does GETLK call on GFS2 (regression)

[Jeff Layton]

This patchset fixes a regression due to this patch:

----------------[snip]----------------
commit 55ef1274dddd4de387c54d110e354ffbb6cdc706
Author: J. Bruce Fields <bfields@citi.umich.edu>
Date:   Sat Dec 20 11:58:38 2008 -0800

    nfsd: Ensure nfsv4 calls the underlying filesystem on LOCKT
----------------[snip]----------------

To reproduce, set up a nfs server that is serving out a GFS2 filesystem.
Set a lock locally on a file on the GFS2 export on the server. From a
NFSv4 client, do a GETLK against the same file. The server will oops due
to a NULL pointer dereference. The fl_lmops will be set, but the
fl_owner will be a NULL pointer. The knfsd code does not account for
this possibility. It assumes that when fl_lmops is set this way that
the fl_owner will point to a valid nfs4_stateowner struct.

In actuality, Bruce's patch is correct, but it exposes a bug in DLM's
GETLK codepath. The first patch in this set fixes that.

The second patch fixes knfsd to be a little more careful about the
file_lock struct it builds to pass to vfs_test_lock.

Either patch should prevent the panic, though I think applying both
patches is the best approach to fixing this.

Jeff Layton (2):
  dlm: initialize file_lock struct in GETLK before copying conflicting
    lock
  nfsd: only set file_lock.fl_lmops in nfsd4_lockt if a stateowner is
    found

 fs/dlm/plock.c      |    2 ++
 fs/nfsd/nfs4state.c |    6 ++++--
 2 files changed, 6 insertions(+), 2 deletions(-)

[PATCH 2/2] nfsd: only set file_lock.fl_lmops in nfsd4_lockt if a stateowner is found

[Jeff Layton]

nfsd4_lockt does a search for a lockstateowner when building the lock
struct to test. If one is found, it'll set fl_owner to it. Regardless of
whether that happens, it'll also set fl_lmops.

If a lockstateowner is not found, then we'll have fl_owner set to NULL
and fl_lmops set pointing to nfsd_posix_mng_ops. Other parts of the
NFSv4 server code assume that fl_owner will point to a valid
nfs4_stateowner if fl_lmops is set this way.

This behavior exposed a bug in DLM's GETLK implementation where it
wasn't clearing out the fields in the file_lock before filling in
conflicting lock info. While we were able to fix this in DLM, it
still seems pointless and dangerous to set the fl_lmops this way
when we have a NULL lockstateowner.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nfsd/nfs4state.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 88db7d3..07d196a 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -2867,11 +2867,13 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 
 	lockt->lt_stateowner = find_lockstateowner_str(inode,
 			&lockt->lt_clientid, &lockt->lt_owner);
-	if (lockt->lt_stateowner)
+	if (lockt->lt_stateowner) {
 		file_lock.fl_owner = (fl_owner_t)lockt->lt_stateowner;
+		file_lock.fl_lmops = &nfsd_posix_mng_ops;
+	}
+
 	file_lock.fl_pid = current->tgid;
 	file_lock.fl_flags = FL_POSIX;
-	file_lock.fl_lmops = &nfsd_posix_mng_ops;
 
 	file_lock.fl_start = lockt->lt_offset;
 	file_lock.fl_end = last_byte_offset(lockt->lt_offset, lockt->lt_length);
-- 
1.5.5.6

Re: [PATCH 2/2] nfsd: only set file_lock.fl_lmops in nfsd4_lockt if a stateowner is found

[J. Bruce Fields]

On Wed, Jan 21, 2009 at 11:34:51AM -0500, Jeff Layton wrote:
> nfsd4_lockt does a search for a lockstateowner when building the lock
> struct to test. If one is found, it'll set fl_owner to it. Regardless of
> whether that happens, it'll also set fl_lmops.
> 
> If a lockstateowner is not found, then we'll have fl_owner set to NULL
> and fl_lmops set pointing to nfsd_posix_mng_ops. Other parts of the
> NFSv4 server code assume that fl_owner will point to a valid
> nfs4_stateowner if fl_lmops is set this way.
> 
> This behavior exposed a bug in DLM's GETLK implementation where it
> wasn't clearing out the fields in the file_lock before filling in
> conflicting lock info. While we were able to fix this in DLM, it
> still seems pointless and dangerous to set the fl_lmops this way
> when we have a NULL lockstateowner.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  fs/nfsd/nfs4state.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 88db7d3..07d196a 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -2867,11 +2867,13 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  
>  	lockt->lt_stateowner = find_lockstateowner_str(inode,
>  			&lockt->lt_clientid, &lockt->lt_owner);
> -	if (lockt->lt_stateowner)
> +	if (lockt->lt_stateowner) {
>  		file_lock.fl_owner = (fl_owner_t)lockt->lt_stateowner;
> +		file_lock.fl_lmops = &nfsd_posix_mng_ops;

So I think we just shouldn't need this second assignment at all.

--b.

> +	}
> +
>  	file_lock.fl_pid = current->tgid;
>  	file_lock.fl_flags = FL_POSIX;
> -	file_lock.fl_lmops = &nfsd_posix_mng_ops;
>  
>  	file_lock.fl_start = lockt->lt_offset;
>  	file_lock.fl_end = last_byte_offset(lockt->lt_offset, lockt->lt_length);
> -- 
> 1.5.5.6
> 
> _______________________________________________
> NFSv4 mailing list
> NFSv4@linux-nfs.org
> http://linux-nfs.org/cgi-bin/mailman/listinfo/nfsv4

Re: [PATCH 2/2] nfsd: only set file_lock.fl_lmops in nfsd4_lockt if a stateowner is found

[Jeff Layton]

On Thu, 22 Jan 2009 13:52:32 -0500
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Wed, Jan 21, 2009 at 11:34:51AM -0500, Jeff Layton wrote:
> > nfsd4_lockt does a search for a lockstateowner when building the lock
> > struct to test. If one is found, it'll set fl_owner to it. Regardless of
> > whether that happens, it'll also set fl_lmops.
> > 
> > If a lockstateowner is not found, then we'll have fl_owner set to NULL
> > and fl_lmops set pointing to nfsd_posix_mng_ops. Other parts of the
> > NFSv4 server code assume that fl_owner will point to a valid
> > nfs4_stateowner if fl_lmops is set this way.
> > 
> > This behavior exposed a bug in DLM's GETLK implementation where it
> > wasn't clearing out the fields in the file_lock before filling in
> > conflicting lock info. While we were able to fix this in DLM, it
> > still seems pointless and dangerous to set the fl_lmops this way
> > when we have a NULL lockstateowner.
> > 
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> >  fs/nfsd/nfs4state.c |    6 ++++--
> >  1 files changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 88db7d3..07d196a 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -2867,11 +2867,13 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >  
> >  	lockt->lt_stateowner = find_lockstateowner_str(inode,
> >  			&lockt->lt_clientid, &lockt->lt_owner);
> > -	if (lockt->lt_stateowner)
> > +	if (lockt->lt_stateowner) {
> >  		file_lock.fl_owner = (fl_owner_t)lockt->lt_stateowner;
> > +		file_lock.fl_lmops = &nfsd_posix_mng_ops;
> 
> So I think we just shouldn't need this second assignment at all.
> 
> --b.
> 

Do we even need to worry about the lockstateowner at all then? If
fl_lmops isn't set then I think the fl_owner will be basically ignored
by nfs4_set_lock_denied anyway.

> > +	}
> > +
> >  	file_lock.fl_pid = current->tgid;
> >  	file_lock.fl_flags = FL_POSIX;
> > -	file_lock.fl_lmops = &nfsd_posix_mng_ops;
> >  
> >  	file_lock.fl_start = lockt->lt_offset;
> >  	file_lock.fl_end = last_byte_offset(lockt->lt_offset, lockt->lt_length);
> > -- 
> > 1.5.5.6
> > 
> > _______________________________________________
> > NFSv4 mailing list
> > NFSv4@linux-nfs.org
> > http://linux-nfs.org/cgi-bin/mailman/listinfo/nfsv4


-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 2/2] nfsd: only set file_lock.fl_lmops in nfsd4_lockt if a stateowner is found

[J. Bruce Fields]

On Thu, Jan 22, 2009 at 01:58:38PM -0500, Jeff Layton wrote:
> On Thu, 22 Jan 2009 13:52:32 -0500
> "J. Bruce Fields" <bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org> wrote:
> 
> > On Wed, Jan 21, 2009 at 11:34:51AM -0500, Jeff Layton wrote:
> > > nfsd4_lockt does a search for a lockstateowner when building the lock
> > > struct to test. If one is found, it'll set fl_owner to it. Regardless of
> > > whether that happens, it'll also set fl_lmops.
> > > 
> > > If a lockstateowner is not found, then we'll have fl_owner set to NULL
> > > and fl_lmops set pointing to nfsd_posix_mng_ops. Other parts of the
> > > NFSv4 server code assume that fl_owner will point to a valid
> > > nfs4_stateowner if fl_lmops is set this way.
> > > 
> > > This behavior exposed a bug in DLM's GETLK implementation where it
> > > wasn't clearing out the fields in the file_lock before filling in
> > > conflicting lock info. While we were able to fix this in DLM, it
> > > still seems pointless and dangerous to set the fl_lmops this way
> > > when we have a NULL lockstateowner.
> > > 
> > > Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > > ---
> > >  fs/nfsd/nfs4state.c |    6 ++++--
> > >  1 files changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > > index 88db7d3..07d196a 100644
> > > --- a/fs/nfsd/nfs4state.c
> > > +++ b/fs/nfsd/nfs4state.c
> > > @@ -2867,11 +2867,13 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > >  
> > >  	lockt->lt_stateowner = find_lockstateowner_str(inode,
> > >  			&lockt->lt_clientid, &lockt->lt_owner);
> > > -	if (lockt->lt_stateowner)
> > > +	if (lockt->lt_stateowner) {
> > >  		file_lock.fl_owner = (fl_owner_t)lockt->lt_stateowner;
> > > +		file_lock.fl_lmops = &nfsd_posix_mng_ops;
> > 
> > So I think we just shouldn't need this second assignment at all.
> > 
> > --b.
> > 
> 
> Do we even need to worry about the lockstateowner at all then? If
> fl_lmops isn't set then I think the fl_owner will be basically ignored
> by nfs4_set_lock_denied anyway.

Yeah, I think nfs4_set_lock_denied should just set dummy values for now.

If we don't, then nfsd_test_lock is passing back a lock with a pointer
to a real reference-counted object, and I worry about what happens if
e.g.  locks are being freed concurrently with our processing of the
conflicting lock here.

Our holding the nfs4_state_lock() here may be enough to prevent
problems, but it seems fragile.

And getting the conflicting lock completely right just isn't that high a
priority.

--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] nfsd: only set file_lock.fl_lmops in nfsd4_lockt if a stateowner is found

[Jeff Layton]

On Thu, 22 Jan 2009 13:52:32 -0500
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Wed, Jan 21, 2009 at 11:34:51AM -0500, Jeff Layton wrote:
> > nfsd4_lockt does a search for a lockstateowner when building the lock
> > struct to test. If one is found, it'll set fl_owner to it. Regardless of
> > whether that happens, it'll also set fl_lmops.
> > 
> > If a lockstateowner is not found, then we'll have fl_owner set to NULL
> > and fl_lmops set pointing to nfsd_posix_mng_ops. Other parts of the
> > NFSv4 server code assume that fl_owner will point to a valid
> > nfs4_stateowner if fl_lmops is set this way.
> > 
> > This behavior exposed a bug in DLM's GETLK implementation where it
> > wasn't clearing out the fields in the file_lock before filling in
> > conflicting lock info. While we were able to fix this in DLM, it
> > still seems pointless and dangerous to set the fl_lmops this way
> > when we have a NULL lockstateowner.
> > 
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> >  fs/nfsd/nfs4state.c |    6 ++++--
> >  1 files changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 88db7d3..07d196a 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -2867,11 +2867,13 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >  
> >  	lockt->lt_stateowner = find_lockstateowner_str(inode,
> >  			&lockt->lt_clientid, &lockt->lt_owner);
> > -	if (lockt->lt_stateowner)
> > +	if (lockt->lt_stateowner) {
> >  		file_lock.fl_owner = (fl_owner_t)lockt->lt_stateowner;
> > +		file_lock.fl_lmops = &nfsd_posix_mng_ops;
> 
> So I think we just shouldn't need this second assignment at all.
> 
> --b.
> 

Do we even need to worry about the lockstateowner at all then? If
fl_lmops isn't set then I think the fl_owner will be basically ignored
by nfs4_set_lock_denied anyway.

> > +	}
> > +
> >  	file_lock.fl_pid = current->tgid;
> >  	file_lock.fl_flags = FL_POSIX;
> > -	file_lock.fl_lmops = &nfsd_posix_mng_ops;
> >  
> >  	file_lock.fl_start = lockt->lt_offset;
> >  	file_lock.fl_end = last_byte_offset(lockt->lt_offset, lockt->lt_length);
> > -- 
> > 1.5.5.6
> > 
> > _______________________________________________
> > NFSv4 mailing list
> > NFSv4@linux-nfs.org
> > http://linux-nfs.org/cgi-bin/mailman/listinfo/nfsv4


-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 2/2] nfsd: only set file_lock.fl_lmops in nfsd4_lockt if a stateowner is found

[Jeff Layton]

On Thu, 22 Jan 2009 13:59:30 -0500
Jeff Layton <jlayton@redhat.com> wrote:

> On Thu, 22 Jan 2009 13:52:32 -0500
> "J. Bruce Fields" <bfields@fieldses.org> wrote:
> 
> > On Wed, Jan 21, 2009 at 11:34:51AM -0500, Jeff Layton wrote:
> > > nfsd4_lockt does a search for a lockstateowner when building the lock
> > > struct to test. If one is found, it'll set fl_owner to it. Regardless of
> > > whether that happens, it'll also set fl_lmops.
> > > 
> > > If a lockstateowner is not found, then we'll have fl_owner set to NULL
> > > and fl_lmops set pointing to nfsd_posix_mng_ops. Other parts of the
> > > NFSv4 server code assume that fl_owner will point to a valid
> > > nfs4_stateowner if fl_lmops is set this way.
> > > 
> > > This behavior exposed a bug in DLM's GETLK implementation where it
> > > wasn't clearing out the fields in the file_lock before filling in
> > > conflicting lock info. While we were able to fix this in DLM, it
> > > still seems pointless and dangerous to set the fl_lmops this way
> > > when we have a NULL lockstateowner.
> > > 
> > > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > > ---
> > >  fs/nfsd/nfs4state.c |    6 ++++--
> > >  1 files changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > > index 88db7d3..07d196a 100644
> > > --- a/fs/nfsd/nfs4state.c
> > > +++ b/fs/nfsd/nfs4state.c
> > > @@ -2867,11 +2867,13 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > >  
> > >  	lockt->lt_stateowner = find_lockstateowner_str(inode,
> > >  			&lockt->lt_clientid, &lockt->lt_owner);
> > > -	if (lockt->lt_stateowner)
> > > +	if (lockt->lt_stateowner) {
> > >  		file_lock.fl_owner = (fl_owner_t)lockt->lt_stateowner;
> > > +		file_lock.fl_lmops = &nfsd_posix_mng_ops;
> > 
> > So I think we just shouldn't need this second assignment at all.
> > 
> > --b.
> > 
> 
> Do we even need to worry about the lockstateowner at all then? If
> fl_lmops isn't set then I think the fl_owner will be basically ignored
> by nfs4_set_lock_denied anyway.
> 

Ahh, nm. I think we do need to set fl_owner so that posix_same_owner
does the right thing. I'll just get rid of the fl_lmops setting and I
think that'll be done.

> > > +	}
> > > +
> > >  	file_lock.fl_pid = current->tgid;
> > >  	file_lock.fl_flags = FL_POSIX;
> > > -	file_lock.fl_lmops = &nfsd_posix_mng_ops;
> > >  
> > >  	file_lock.fl_start = lockt->lt_offset;
> > >  	file_lock.fl_end = last_byte_offset(lockt->lt_offset, lockt->lt_length);
> > > -- 
> > > 1.5.5.6
> > > 
> > > _______________________________________________
> > > NFSv4 mailing list
> > > NFSv4@linux-nfs.org
> > > http://linux-nfs.org/cgi-bin/mailman/listinfo/nfsv4
> 
> 
> -- 
> Jeff Layton <jlayton@redhat.com>

Thanks,
-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 2/2] nfsd: only set file_lock.fl_lmops in nfsd4_lockt if a stateowner is found

[J. Bruce Fields]

On Thu, Jan 22, 2009 at 02:09:02PM -0500, Jeff Layton wrote:
> On Thu, 22 Jan 2009 13:59:30 -0500
> Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> 
> > On Thu, 22 Jan 2009 13:52:32 -0500
> > "J. Bruce Fields" <bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org> wrote:
> > 
> > > On Wed, Jan 21, 2009 at 11:34:51AM -0500, Jeff Layton wrote:
> > > > nfsd4_lockt does a search for a lockstateowner when building the lock
> > > > struct to test. If one is found, it'll set fl_owner to it. Regardless of
> > > > whether that happens, it'll also set fl_lmops.
> > > > 
> > > > If a lockstateowner is not found, then we'll have fl_owner set to NULL
> > > > and fl_lmops set pointing to nfsd_posix_mng_ops. Other parts of the
> > > > NFSv4 server code assume that fl_owner will point to a valid
> > > > nfs4_stateowner if fl_lmops is set this way.
> > > > 
> > > > This behavior exposed a bug in DLM's GETLK implementation where it
> > > > wasn't clearing out the fields in the file_lock before filling in
> > > > conflicting lock info. While we were able to fix this in DLM, it
> > > > still seems pointless and dangerous to set the fl_lmops this way
> > > > when we have a NULL lockstateowner.
> > > > 
> > > > Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > > > ---
> > > >  fs/nfsd/nfs4state.c |    6 ++++--
> > > >  1 files changed, 4 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > > > index 88db7d3..07d196a 100644
> > > > --- a/fs/nfsd/nfs4state.c
> > > > +++ b/fs/nfsd/nfs4state.c
> > > > @@ -2867,11 +2867,13 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > > >  
> > > >  	lockt->lt_stateowner = find_lockstateowner_str(inode,
> > > >  			&lockt->lt_clientid, &lockt->lt_owner);
> > > > -	if (lockt->lt_stateowner)
> > > > +	if (lockt->lt_stateowner) {
> > > >  		file_lock.fl_owner = (fl_owner_t)lockt->lt_stateowner;
> > > > +		file_lock.fl_lmops = &nfsd_posix_mng_ops;
> > > 
> > > So I think we just shouldn't need this second assignment at all.
> > > 
> > > --b.
> > > 
> > 
> > Do we even need to worry about the lockstateowner at all then? If
> > fl_lmops isn't set then I think the fl_owner will be basically ignored
> > by nfs4_set_lock_denied anyway.
> > 
> 
> Ahh, nm. I think we do need to set fl_owner so that posix_same_owner
> does the right thing. I'll just get rid of the fl_lmops setting and I
> think that'll be done.

Right, but that does mean set_lock_denied is never going to see fl_lmops
set and hence isn't really going to use the returned fl_owner.  Which I
can live with.

--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html