This series passes light testing in my lab. If it looks good I will
pass it along to Alexey to confirm it closes the race.
To aid distributors and stable kernel maintainers, wondering if a
Fixes: tag should be added. Alexey first observed this issue in v4.1
kernels (UEK4). But looks like the race could have been introduced
as early as v3.17. Maybe this one?
commit fc5a96c3b70d00c863f69ff4ea7f5dfddbcbc0d8
Author: Jeff Layton <[email protected]>
Date: Tue Jul 29 21:34:40 2014 -0400
nfsd: close potential race in nfsd4_free_stateid
There have been a lot of changes since then. It's hard to say if the
race can be attributed to a single commit.
Changes since v2:
- Move NFS4_LOCK_STID arm into a helper, for clarity
- Add more detail to patch description
- Add Jeff's patch to fix similar race in nfsd4_lock
Changes since v1:
- Use s->sc_count to preserve stateid while cl_lock is dropped
---
Chuck Lever (1):
nfsd: Fix race between FREE_STATEID and LOCK
Jeff Layton (1):
nfsd: don't return an unhashed lock stateid after taking mutex
fs/nfsd/nfs4state.c | 65 ++++++++++++++++++++++++++++++++++++++-------------
1 file changed, 48 insertions(+), 17 deletions(-)
--
Chuck Lever
When running LTP's nfslock01 test, the Linux client can send a LOCK
and a FREE_STATEID request at the same time. The outcome is:
Frame 324 R OPEN stateid [2,O]
Frame 115004 C LOCK lockowner_is_new stateid [2,O] offset 672000 len 64
Frame 115008 R LOCK stateid [1,L]
Frame 115012 C WRITE stateid [0,L] offset 672000 len 64
Frame 115016 R WRITE NFS4_OK
Frame 115019 C LOCKU stateid [1,L] offset 672000 len 64
Frame 115022 R LOCKU NFS4_OK
Frame 115025 C FREE_STATEID stateid [2,L]
Frame 115026 C LOCK lockowner_is_new stateid [2,O] offset 672128 len 64
Frame 115029 R FREE_STATEID NFS4_OK
Frame 115030 R LOCK stateid [3,L]
Frame 115034 C WRITE stateid [0,L] offset 672128 len 64
Frame 115038 R WRITE NFS4ERR_BAD_STATEID
In other words, the server returns stateid L in a successful LOCK
reply, but it has already released it. Subsequent uses of stateid L
fail.
To address this, protect the generation check in nfsd4_free_stateid
with the st_mutex. This should guarantee that only one of two
outcomes occurs: either LOCK returns a fresh valid stateid, or
FREE_STATEID returns NFS4ERR_LOCKS_HELD.
Reported-by: Alexey Kodanev <[email protected]>
Fix-suggested-by: Jeff Layton <[email protected]>
Signed-off-by: Chuck Lever <[email protected]>
---
fs/nfsd/nfs4state.c | 40 ++++++++++++++++++++++++++++------------
1 file changed, 28 insertions(+), 12 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index b921123..de868fe 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4885,6 +4885,32 @@ nfsd4_test_stateid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
return nfs_ok;
}
+static __be32
+nfsd4_free_lock_stateid(stateid_t *stateid, struct nfs4_stid *s)
+{
+ struct nfs4_ol_stateid *stp = openlockstateid(s);
+ __be32 ret;
+
+ mutex_lock(&stp->st_mutex);
+
+ ret = check_stateid_generation(stateid, &s->sc_stateid, 1);
+ if (ret)
+ goto out;
+
+ ret = nfserr_locks_held;
+ if (check_for_locks(stp->st_stid.sc_file,
+ lockowner(stp->st_stateowner)))
+ goto out;
+
+ release_lock_stateid(stp);
+ ret = nfs_ok;
+
+out:
+ mutex_unlock(&stp->st_mutex);
+ nfs4_put_stid(s);
+ return ret;
+}
+
__be32
nfsd4_free_stateid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
struct nfsd4_free_stateid *free_stateid)
@@ -4892,7 +4918,6 @@ nfsd4_free_stateid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
stateid_t *stateid = &free_stateid->fr_stateid;
struct nfs4_stid *s;
struct nfs4_delegation *dp;
- struct nfs4_ol_stateid *stp;
struct nfs4_client *cl = cstate->session->se_client;
__be32 ret = nfserr_bad_stateid;
@@ -4911,18 +4936,9 @@ nfsd4_free_stateid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
ret = nfserr_locks_held;
break;
case NFS4_LOCK_STID:
- ret = check_stateid_generation(stateid, &s->sc_stateid, 1);
- if (ret)
- break;
- stp = openlockstateid(s);
- ret = nfserr_locks_held;
- if (check_for_locks(stp->st_stid.sc_file,
- lockowner(stp->st_stateowner)))
- break;
- WARN_ON(!unhash_lock_stateid(stp));
+ atomic_inc(&s->sc_count);
spin_unlock(&cl->cl_lock);
- nfs4_put_stid(s);
- ret = nfs_ok;
+ ret = nfsd4_free_lock_stateid(stateid, s);
goto out;
case NFS4_REVOKED_DELEG_STID:
dp = delegstateid(s);
From: Jeff Layton <[email protected]>
nfsd4_lock will take the st_mutex before working with the stateid it
gets, but between the time when we drop the cl_lock and take the mutex,
the stateid could become unhashed (a'la FREE_STATEID). If that happens
the lock stateid returned to the client will be forgotten.
Fix this by first moving the st_mutex acquisition into
lookup_or_create_lock_state. Then, have it check to see if the lock
stateid is still hashed after taking the mutex. If it's not, then put
the stateid and try the find/create again.
Signed-off-by: Jeff Layton <[email protected]>
---
fs/nfsd/nfs4state.c | 25 ++++++++++++++++++++-----
1 file changed, 20 insertions(+), 5 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index de868fe..6a23098 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -5505,7 +5505,7 @@ static __be32
lookup_or_create_lock_state(struct nfsd4_compound_state *cstate,
struct nfs4_ol_stateid *ost,
struct nfsd4_lock *lock,
- struct nfs4_ol_stateid **lst, bool *new)
+ struct nfs4_ol_stateid **plst, bool *new)
{
__be32 status;
struct nfs4_file *fi = ost->st_stid.sc_file;
@@ -5513,7 +5513,9 @@ lookup_or_create_lock_state(struct nfsd4_compound_state *cstate,
struct nfs4_client *cl = oo->oo_owner.so_client;
struct inode *inode = d_inode(cstate->current_fh.fh_dentry);
struct nfs4_lockowner *lo;
+ struct nfs4_ol_stateid *lst;
unsigned int strhashval;
+ bool hashed;
lo = find_lockowner_str(cl, &lock->lk_new_owner);
if (!lo) {
@@ -5529,12 +5531,27 @@ lookup_or_create_lock_state(struct nfsd4_compound_state *cstate,
goto out;
}
- *lst = find_or_create_lock_stateid(lo, fi, inode, ost, new);
- if (*lst == NULL) {
+retry:
+ lst = find_or_create_lock_stateid(lo, fi, inode, ost, new);
+ if (lst == NULL) {
status = nfserr_jukebox;
goto out;
}
+
+ mutex_lock(&lst->st_mutex);
+
+ /* See if it's still hashed to avoid race with FREE_STATEID */
+ spin_lock(&cl->cl_lock);
+ hashed = !list_empty(&lst->st_perfile);
+ spin_unlock(&cl->cl_lock);
+
+ if (!hashed) {
+ mutex_unlock(&lst->st_mutex);
+ nfs4_put_stid(&lst->st_stid);
+ goto retry;
+ }
status = nfs_ok;
+ *plst = lst;
out:
nfs4_put_stateowner(&lo->lo_owner);
return status;
@@ -5601,8 +5618,6 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
goto out;
status = lookup_or_create_lock_state(cstate, open_stp, lock,
&lock_stp, &new);
- if (status == nfs_ok)
- mutex_lock(&lock_stp->st_mutex);
} else {
status = nfs4_preprocess_seqid_op(cstate,
lock->lk_old_lock_seqid,
On Mon, Aug 08, 2016 at 02:59:35PM -0400, Chuck Lever wrote:
> This series passes light testing in my lab. If it looks good I will
> pass it along to Alexey to confirm it closes the race.
>
> To aid distributors and stable kernel maintainers, wondering if a
> Fixes: tag should be added. Alexey first observed this issue in v4.1
> kernels (UEK4). But looks like the race could have been introduced
> as early as v3.17. Maybe this one?
The other reason we didn't see this till now might be client-side
changes (maybe b4019c0e219b "NFSv4.1: Allow parallel LOCK/LOCKU
calls"?). (Not trying to dodge responsibility for a server-side bug
here, but that might still be useful information for the changelog (not
the Fixes: line) if it's correct.)
--b.
>
> commit fc5a96c3b70d00c863f69ff4ea7f5dfddbcbc0d8
> Author: Jeff Layton <[email protected]>
> Date: Tue Jul 29 21:34:40 2014 -0400
>
> nfsd: close potential race in nfsd4_free_stateid
>
> There have been a lot of changes since then. It's hard to say if the
> race can be attributed to a single commit.
>
>
> Changes since v2:
> - Move NFS4_LOCK_STID arm into a helper, for clarity
> - Add more detail to patch description
> - Add Jeff's patch to fix similar race in nfsd4_lock
>
> Changes since v1:
> - Use s->sc_count to preserve stateid while cl_lock is dropped
>
> ---
>
> Chuck Lever (1):
> nfsd: Fix race between FREE_STATEID and LOCK
>
> Jeff Layton (1):
> nfsd: don't return an unhashed lock stateid after taking mutex
>
>
> fs/nfsd/nfs4state.c | 65 ++++++++++++++++++++++++++++++++++++++-------------
> 1 file changed, 48 insertions(+), 17 deletions(-)
>
> --
> Chuck Lever
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> On Aug 8, 2016, at 3:59 PM, [email protected] wrote:
>
> On Mon, Aug 08, 2016 at 02:59:35PM -0400, Chuck Lever wrote:
>> This series passes light testing in my lab. If it looks good I will
>> pass it along to Alexey to confirm it closes the race.
>>
>> To aid distributors and stable kernel maintainers, wondering if a
>> Fixes: tag should be added. Alexey first observed this issue in v4.1
>> kernels (UEK4). But looks like the race could have been introduced
>> as early as v3.17. Maybe this one?
>
> The other reason we didn't see this till now might be client-side
> changes (maybe b4019c0e219b "NFSv4.1: Allow parallel LOCK/LOCKU
> calls"?). (Not trying to dodge responsibility for a server-side bug
> here, but that might still be useful information for the changelog (not
> the Fixes: line) if it's correct.)
I asked Alexey to test as far back as v3.19, where I think the
LOCK parallelism was added. I need to get some clarification of
his test results; one set of test runs reproduced the race, and
a second set of test runs did not.
> --b.
>
>>
>> commit fc5a96c3b70d00c863f69ff4ea7f5dfddbcbc0d8
>> Author: Jeff Layton <[email protected]>
>> Date: Tue Jul 29 21:34:40 2014 -0400
>>
>> nfsd: close potential race in nfsd4_free_stateid
>>
>> There have been a lot of changes since then. It's hard to say if the
>> race can be attributed to a single commit.
>>
>>
>> Changes since v2:
>> - Move NFS4_LOCK_STID arm into a helper, for clarity
>> - Add more detail to patch description
>> - Add Jeff's patch to fix similar race in nfsd4_lock
>>
>> Changes since v1:
>> - Use s->sc_count to preserve stateid while cl_lock is dropped
>>
>> ---
>>
>> Chuck Lever (1):
>> nfsd: Fix race between FREE_STATEID and LOCK
>>
>> Jeff Layton (1):
>> nfsd: don't return an unhashed lock stateid after taking mutex
>>
>>
>> fs/nfsd/nfs4state.c | 65 ++++++++++++++++++++++++++++++++++++++-------------
>> 1 file changed, 48 insertions(+), 17 deletions(-)
>>
>> --
>> Chuck Lever
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Chuck Lever
On Mon, 2016-08-08 at 14:59 -0400, Chuck Lever wrote:
> This series passes light testing in my lab. If it looks good I will
> pass it along to Alexey to confirm it closes the race.
>
> To aid distributors and stable kernel maintainers, wondering if a
> Fixes: tag should be added. Alexey first observed this issue in v4.1
> kernels (UEK4). But looks like the race could have been introduced
> as early as v3.17. Maybe this one?
>
> commit fc5a96c3b70d00c863f69ff4ea7f5dfddbcbc0d8
> Author: Jeff Layton <[email protected]>
> Date: Tue Jul 29 21:34:40 2014 -0400
>
> nfsd: close potential race in nfsd4_free_stateid
>
> There have been a lot of changes since then. It's hard to say if the
> race can be attributed to a single commit.
>
>
> Changes since v2:
> - Move NFS4_LOCK_STID arm into a helper, for clarity
> - Add more detail to patch description
> - Add Jeff's patch to fix similar race in nfsd4_lock
>
> Changes since v1:
> - Use s->sc_count to preserve stateid while cl_lock is dropped
>
Probably this is a regression from when we removed the big client_mutex
from knfsd. That's what serialized all of this stuff before. So, maybe
this one actually?
commit e7d5dc19ce9800b86dd9e41ff36cc418e9da1fce
Author: Trond Myklebust <[email protected]>
Date: Wed Jul 30 08:27:26 2014 -0400
nfsd: Remove nfs4_lock_state(): nfsd4_test_stateid/nfsd4_free_stateid
Signed-off-by: Trond Myklebust <[email protected]>
Signed-off-by: J. Bruce Fields <[email protected]>
...but yeah, around that era.
> ---
>
> Chuck Lever (1):
> nfsd: Fix race between FREE_STATEID and LOCK
>
> Jeff Layton (1):
> nfsd: don't return an unhashed lock stateid after taking mutex
>
>
> fs/nfsd/nfs4state.c | 65 ++++++++++++++++++++++++++++++++++++++---
> ----------
> 1 file changed, 48 insertions(+), 17 deletions(-)
>
> --
> Chuck Lever
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs"
> in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Jeff Layton <[email protected]>
> On Aug 8, 2016, at 4:06 PM, Chuck Lever <[email protected]> wrote:
>
>>
>> On Aug 8, 2016, at 3:59 PM, [email protected] wrote:
>>
>> On Mon, Aug 08, 2016 at 02:59:35PM -0400, Chuck Lever wrote:
>>> This series passes light testing in my lab. If it looks good I will
>>> pass it along to Alexey to confirm it closes the race.
>>>
>>> To aid distributors and stable kernel maintainers, wondering if a
>>> Fixes: tag should be added. Alexey first observed this issue in v4.1
>>> kernels (UEK4). But looks like the race could have been introduced
>>> as early as v3.17. Maybe this one?
>>
>> The other reason we didn't see this till now might be client-side
>> changes (maybe b4019c0e219b "NFSv4.1: Allow parallel LOCK/LOCKU
>> calls"?). (Not trying to dodge responsibility for a server-side bug
>> here, but that might still be useful information for the changelog (not
>> the Fixes: line) if it's correct.)
>
> I asked Alexey to test as far back as v3.19, where I think the
> LOCK parallelism was added. I need to get some clarification of
> his test results; one set of test runs reproduced the race, and
> a second set of test runs did not.
Alexey reports he tried reproducing this with an Oracle Linux UEK3
client, which is roughly the same as v3.8 stable running on RHEL 6.
He says he was not able to reproduce with that configuration.
However, he was able to reproduce with a stock RHEL 7.2 client, and
with a v3.19 stable kernel running on a RHEL 7 client.
--
Chuck Lever
On Tue, Aug 09, 2016 at 11:53:06AM -0400, Chuck Lever wrote:
>
> > On Aug 8, 2016, at 4:06 PM, Chuck Lever <[email protected]> wrote:
> >
> >>
> >> On Aug 8, 2016, at 3:59 PM, [email protected] wrote:
> >>
> >> On Mon, Aug 08, 2016 at 02:59:35PM -0400, Chuck Lever wrote:
> >>> This series passes light testing in my lab. If it looks good I will
> >>> pass it along to Alexey to confirm it closes the race.
> >>>
> >>> To aid distributors and stable kernel maintainers, wondering if a
> >>> Fixes: tag should be added. Alexey first observed this issue in v4.1
> >>> kernels (UEK4). But looks like the race could have been introduced
> >>> as early as v3.17. Maybe this one?
> >>
> >> The other reason we didn't see this till now might be client-side
> >> changes (maybe b4019c0e219b "NFSv4.1: Allow parallel LOCK/LOCKU
> >> calls"?). (Not trying to dodge responsibility for a server-side bug
> >> here, but that might still be useful information for the changelog (not
> >> the Fixes: line) if it's correct.)
> >
> > I asked Alexey to test as far back as v3.19, where I think the
> > LOCK parallelism was added. I need to get some clarification of
> > his test results; one set of test runs reproduced the race, and
> > a second set of test runs did not.
>
> Alexey reports he tried reproducing this with an Oracle Linux UEK3
> client, which is roughly the same as v3.8 stable running on RHEL 6.
> He says he was not able to reproduce with that configuration.
>
> However, he was able to reproduce with a stock RHEL 7.2 client, and
> with a v3.19 stable kernel running on a RHEL 7 client.
That "Allow parallel LOCK/LOCKU calls" patch wasn't in 3.19 (it went
into 4.0). So, OK, I don't know, maybe we've got nothing to say about
that.
--b.