This patchset fixes a regression due to this patch:
----------------[snip]----------------
commit 55ef1274dddd4de387c54d110e354ffbb6cdc706
Author: J. Bruce Fields <[email protected]>
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(-)
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 <[email protected]>
---
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
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 <[email protected]>
---
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
On Wed, Jan 21, 2009 at 11:34:50AM -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.
I think of the lock system as supporting two types of objects, both
stored in "struct lock"'s:
- Heavyweight locks: these have callbacks set and the filesystem
or lock manager could in theory have some private data
associated with them, so it's important that the appropriate
callbacks be called when they're released or copied. These
are what are actually passed to posix_lock_file() and kept on
the inode lock lists.
- Lightweight locks: just start, end, pid, flags, and type, with
everything zeroed out and/or ignored.
I don't see any reason why the lock passed into dlm_posix_get() needs to
be a heavyweight lock. In any case, if it were, then dlm_posix_get()
would need to release the passed-in-lock before initializing the new one
that it's returning.
The returned lock should probably also be a lightweight lock that's a
copy of whatever conflicting lock was found; otherwise we need to
require the caller to for example release the thing correctly.
That's unfortunate for nfsv4 since that doesn't allow returning the
lockowner information to the client. But it's not terribly important
to get that right.
Since gfs2 doesn't report the conflicting lock, I guess we just punt and
return a copy of the passed-in lock, OK.
--b.
>
> Fix this by making DLM reinitialize the file_lock before copying the
> fields from the conflicting lock.
>
> Signed-off-by: Jeff Layton <[email protected]>
> ---
> 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
>
> _______________________________________________
> NFSv4 mailing list
> [email protected]
> http://linux-nfs.org/cgi-bin/mailman/listinfo/nfsv4
On Wed, 21 Jan 2009 18:42:39 -0500
"J. Bruce Fields" <[email protected]> wrote:
> On Wed, Jan 21, 2009 at 11:34:50AM -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.
>
> I think of the lock system as supporting two types of objects, both
> stored in "struct lock"'s:
>
> - Heavyweight locks: these have callbacks set and the filesystem
> or lock manager could in theory have some private data
> associated with them, so it's important that the appropriate
> callbacks be called when they're released or copied. These
> are what are actually passed to posix_lock_file() and kept on
> the inode lock lists.
> - Lightweight locks: just start, end, pid, flags, and type, with
> everything zeroed out and/or ignored.
>
> I don't see any reason why the lock passed into dlm_posix_get() needs to
> be a heavyweight lock. In any case, if it were, then dlm_posix_get()
> would need to release the passed-in-lock before initializing the new one
> that it's returning.
>
>From what I can tell, dlm_posix_lock is always passed a "lightweight"
lock.
> The returned lock should probably also be a lightweight lock that's a
> copy of whatever conflicting lock was found; otherwise we need to
> require the caller to for example release the thing correctly.
>
> That's unfortunate for nfsv4 since that doesn't allow returning the
> lockowner information to the client. But it's not terribly important
> to get that right.
>
> Since gfs2 doesn't report the conflicting lock, I guess we just punt and
> return a copy of the passed-in lock, OK.
>
I'm not sure I follow you here...
GFS2/DLM does report the conflicting lock. It's just that when there is
one, it's only overwriting some of the fields in the lock. The idea
with this patch is to basically try and make dlm_posix_get() fill out
the same fields as __locks_copy_lock() and make sure the rest are
initialized.
> >
> > Fix this by making DLM reinitialize the file_lock before copying the
> > fields from the conflicting lock.
> >
> > Signed-off-by: Jeff Layton <[email protected]>
> > ---
> > 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
> >
> > _______________________________________________
> > NFSv4 mailing list
> > [email protected]
> > http://linux-nfs.org/cgi-bin/mailman/listinfo/nfsv4
--
Jeff Layton <[email protected]>
On Wed, Jan 21, 2009 at 06:42:39PM -0500, J. Bruce Fields wrote:
> On Wed, Jan 21, 2009 at 11:34:50AM -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.
>
> I think of the lock system as supporting two types of objects, both
> stored in "struct lock"'s:
>
> - Heavyweight locks: these have callbacks set and the filesystem
> or lock manager could in theory have some private data
> associated with them, so it's important that the appropriate
> callbacks be called when they're released or copied. These
> are what are actually passed to posix_lock_file() and kept on
> the inode lock lists.
> - Lightweight locks: just start, end, pid, flags, and type, with
> everything zeroed out and/or ignored.
>
> I don't see any reason why the lock passed into dlm_posix_get() needs to
> be a heavyweight lock. In any case, if it were, then dlm_posix_get()
> would need to release the passed-in-lock before initializing the new one
> that it's returning.
It seems the nfs code is mixing those two types up a bit. Regardless, the
rationale I see in Jeff's dlm patch is to make the two different locking paths
equivalent:
Without cfs/dlm,
nfsd4_lockt -> nfsd_test_lock -> vfs_test_lock -> posix_test_lock
With cfs/dlm,
nfsd4_lockt -> nfsd_test_lock -> vfs_test_lock -> (cfs) -> dlm_posix_get
When there's a conflict, dlm_posix_get() and posix_test_lock() should do the
same/equivalent things to the fl they are given.
posix_test_lock() does __locks_copy_lock() on the fl and then sets the pid.
dlm_posix_get() isn't using __locks_copy_lock() because it doesn't have a
conflicting file_lock to copy from. Jeff's patch does nearly the same thing
using locks_init_lock() plus the existing assignments. But, I think the best
solution may be for dlm_posix_get() to set up a new lightweight file_lock with
the values we need, and then call __locks_copy_lock() with it, just like
posix_test_lock().
Dave
On Wed, Jan 21, 2009 at 09:26:08PM -0500, Jeff Layton wrote:
> On Wed, 21 Jan 2009 18:42:39 -0500
> "J. Bruce Fields" <[email protected]> wrote:
>
> > On Wed, Jan 21, 2009 at 11:34:50AM -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.
> >
> > I think of the lock system as supporting two types of objects, both
> > stored in "struct lock"'s:
> >
> > - Heavyweight locks: these have callbacks set and the filesystem
> > or lock manager could in theory have some private data
> > associated with them, so it's important that the appropriate
> > callbacks be called when they're released or copied. These
> > are what are actually passed to posix_lock_file() and kept on
> > the inode lock lists.
> > - Lightweight locks: just start, end, pid, flags, and type, with
> > everything zeroed out and/or ignored.
> >
> > I don't see any reason why the lock passed into dlm_posix_get() needs to
> > be a heavyweight lock. In any case, if it were, then dlm_posix_get()
> > would need to release the passed-in-lock before initializing the new one
> > that it's returning.
> >
>
>
> From what I can tell, dlm_posix_lock is always passed a "lightweight"
> lock.
Right, so in your second patch, I think the fl_lmops assignment in
nfsd4_lockt should also be removed.
> > The returned lock should probably also be a lightweight lock that's a
> > copy of whatever conflicting lock was found; otherwise we need to
> > require the caller to for example release the thing correctly.
> >
> > That's unfortunate for nfsv4 since that doesn't allow returning the
> > lockowner information to the client. But it's not terribly important
> > to get that right.
> >
> > Since gfs2 doesn't report the conflicting lock, I guess we just punt and
> > return a copy of the passed-in lock, OK.
> >
>
> I'm not sure I follow you here...
>
> GFS2/DLM does report the conflicting lock. It's just that when there is
> one, it's only overwriting some of the fields in the lock.
Whoops, sorry, OK.
> The idea with this patch is to basically try and make dlm_posix_get()
> fill out the same fields as __locks_copy_lock() and make sure the rest
> are initialized.
Yes, this patch seems fine. I'm less sure of the second.
--b.
On Thu, 22 Jan 2009 13:32:41 -0500
"J. Bruce Fields" <[email protected]> wrote:
> On Wed, Jan 21, 2009 at 09:26:08PM -0500, Jeff Layton wrote:
> > On Wed, 21 Jan 2009 18:42:39 -0500
> > "J. Bruce Fields" <[email protected]> wrote:
> >
> > > On Wed, Jan 21, 2009 at 11:34:50AM -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.
> > >
> > > I think of the lock system as supporting two types of objects, both
> > > stored in "struct lock"'s:
> > >
> > > - Heavyweight locks: these have callbacks set and the filesystem
> > > or lock manager could in theory have some private data
> > > associated with them, so it's important that the appropriate
> > > callbacks be called when they're released or copied. These
> > > are what are actually passed to posix_lock_file() and kept on
> > > the inode lock lists.
> > > - Lightweight locks: just start, end, pid, flags, and type, with
> > > everything zeroed out and/or ignored.
> > >
> > > I don't see any reason why the lock passed into dlm_posix_get() needs to
> > > be a heavyweight lock. In any case, if it were, then dlm_posix_get()
> > > would need to release the passed-in-lock before initializing the new one
> > > that it's returning.
> > >
> >
> >
> > From what I can tell, dlm_posix_lock is always passed a "lightweight"
> > lock.
>
> Right, so in your second patch, I think the fl_lmops assignment in
> nfsd4_lockt should also be removed.
>
Ok, that works too. I'll respin and repost once we come to some
concensus on the first patch.
> > > The returned lock should probably also be a lightweight lock that's a
> > > copy of whatever conflicting lock was found; otherwise we need to
> > > require the caller to for example release the thing correctly.
> > >
> > > That's unfortunate for nfsv4 since that doesn't allow returning the
> > > lockowner information to the client. But it's not terribly important
> > > to get that right.
> > >
> > > Since gfs2 doesn't report the conflicting lock, I guess we just punt and
> > > return a copy of the passed-in lock, OK.
> > >
> >
> > I'm not sure I follow you here...
> >
> > GFS2/DLM does report the conflicting lock. It's just that when there is
> > one, it's only overwriting some of the fields in the lock.
>
> Whoops, sorry, OK.
>
> > The idea with this patch is to basically try and make dlm_posix_get()
> > fill out the same fields as __locks_copy_lock() and make sure the rest
> > are initialized.
>
> Yes, this patch seems fine. I'm less sure of the second.
>
> --b.
Thanks,
--
Jeff Layton <[email protected]>
On Thu, 22 Jan 2009 12:05:43 -0600
David Teigland <[email protected]> wrote:
> On Wed, Jan 21, 2009 at 06:42:39PM -0500, J. Bruce Fields wrote:
> > On Wed, Jan 21, 2009 at 11:34:50AM -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.
> >
> > I think of the lock system as supporting two types of objects, both
> > stored in "struct lock"'s:
> >
> > - Heavyweight locks: these have callbacks set and the filesystem
> > or lock manager could in theory have some private data
> > associated with them, so it's important that the appropriate
> > callbacks be called when they're released or copied. These
> > are what are actually passed to posix_lock_file() and kept on
> > the inode lock lists.
> > - Lightweight locks: just start, end, pid, flags, and type, with
> > everything zeroed out and/or ignored.
> >
> > I don't see any reason why the lock passed into dlm_posix_get() needs to
> > be a heavyweight lock. In any case, if it were, then dlm_posix_get()
> > would need to release the passed-in-lock before initializing the new one
> > that it's returning.
>
> It seems the nfs code is mixing those two types up a bit. Regardless, the
> rationale I see in Jeff's dlm patch is to make the two different locking paths
> equivalent:
>
> Without cfs/dlm,
> nfsd4_lockt -> nfsd_test_lock -> vfs_test_lock -> posix_test_lock
>
> With cfs/dlm,
> nfsd4_lockt -> nfsd_test_lock -> vfs_test_lock -> (cfs) -> dlm_posix_get
>
> When there's a conflict, dlm_posix_get() and posix_test_lock() should do the
> same/equivalent things to the fl they are given.
>
> posix_test_lock() does __locks_copy_lock() on the fl and then sets the pid.
> dlm_posix_get() isn't using __locks_copy_lock() because it doesn't have a
> conflicting file_lock to copy from. Jeff's patch does nearly the same thing
> using locks_init_lock() plus the existing assignments. But, I think the best
> solution may be for dlm_posix_get() to set up a new lightweight file_lock with
> the values we need, and then call __locks_copy_lock() with it, just like
> posix_test_lock().
>
Why would we want to make another lock here? Is that just to make sure
that if new fields are added later that we deal with them appropriately?
--
Jeff Layton <[email protected]>
On Thu, Jan 22, 2009 at 12:05:43PM -0600, David Teigland wrote:
> On Wed, Jan 21, 2009 at 06:42:39PM -0500, J. Bruce Fields wrote:
> > On Wed, Jan 21, 2009 at 11:34:50AM -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.
> >
> > I think of the lock system as supporting two types of objects, both
> > stored in "struct lock"'s:
> >
> > - Heavyweight locks: these have callbacks set and the filesystem
> > or lock manager could in theory have some private data
> > associated with them, so it's important that the appropriate
> > callbacks be called when they're released or copied. These
> > are what are actually passed to posix_lock_file() and kept on
> > the inode lock lists.
> > - Lightweight locks: just start, end, pid, flags, and type, with
> > everything zeroed out and/or ignored.
> >
> > I don't see any reason why the lock passed into dlm_posix_get() needs to
> > be a heavyweight lock. In any case, if it were, then dlm_posix_get()
> > would need to release the passed-in-lock before initializing the new one
> > that it's returning.
>
> It seems the nfs code is mixing those two types up a bit.
There may be more cleanup needed. (Pointers welcomed.)
> Regardless, the
> rationale I see in Jeff's dlm patch is to make the two different locking paths
> equivalent:
>
> Without cfs/dlm,
> nfsd4_lockt -> nfsd_test_lock -> vfs_test_lock -> posix_test_lock
>
> With cfs/dlm,
> nfsd4_lockt -> nfsd_test_lock -> vfs_test_lock -> (cfs) -> dlm_posix_get
>
> When there's a conflict, dlm_posix_get() and posix_test_lock() should do the
> same/equivalent things to the fl they are given.
>
> posix_test_lock() does __locks_copy_lock() on the fl and then sets the pid.
> dlm_posix_get() isn't using __locks_copy_lock() because it doesn't have a
> conflicting file_lock to copy from. Jeff's patch does nearly the same thing
> using locks_init_lock() plus the existing assignments.
Right, and that's fine.
> But, I think the best solution may be for dlm_posix_get() to set up a
> new lightweight file_lock with the values we need, and then call
> __locks_copy_lock() with it, just like posix_test_lock().
That sounds like overkill.
--b.
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 <[email protected]>
> ---
> 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
> [email protected]
> http://linux-nfs.org/cgi-bin/mailman/listinfo/nfsv4
On Thu, 22 Jan 2009 13:52:32 -0500
"J. Bruce Fields" <[email protected]> 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 <[email protected]>
> > ---
> > 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
> > [email protected]
> > http://linux-nfs.org/cgi-bin/mailman/listinfo/nfsv4
--
Jeff Layton <[email protected]>
On Thu, 22 Jan 2009 13:52:32 -0500
"J. Bruce Fields" <[email protected]> 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 <[email protected]>
> > ---
> > 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
> > [email protected]
> > http://linux-nfs.org/cgi-bin/mailman/listinfo/nfsv4
--
Jeff Layton <[email protected]>
On Thu, 22 Jan 2009 13:59:30 -0500
Jeff Layton <[email protected]> wrote:
> On Thu, 22 Jan 2009 13:52:32 -0500
> "J. Bruce Fields" <[email protected]> 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 <[email protected]>
> > > ---
> > > 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
> > > [email protected]
> > > http://linux-nfs.org/cgi-bin/mailman/listinfo/nfsv4
>
>
> --
> Jeff Layton <[email protected]>
Thanks,
--
Jeff Layton <[email protected]>
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" <[email protected]> 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 <[email protected]>
> > > ---
> > > 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.
On Thu, Jan 22, 2009 at 02:09:02PM -0500, Jeff Layton wrote:
> On Thu, 22 Jan 2009 13:59:30 -0500
> Jeff Layton <[email protected]> wrote:
>
> > On Thu, 22 Jan 2009 13:52:32 -0500
> > "J. Bruce Fields" <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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.