This patchset aims to tighten up the locking rules around stateids to
ensure that knfsd does not reuse stateids that have already been closed
or invalidated. The aim is to ensure we enforce the RFC5661 and RFC7530
rules concerning stateid initialisation and updates.
Trond Myklebust (5):
nfsd: Fix stateid races between OPEN and CLOSE
nfsd: CLOSE SHOULD return the invalid special stateid for NFSv4.x
(x>0)
nfsd: Ensure we don't recognise lock stateids after freeing them
nfsd: Ensure we check stateid validity in the seqid operation checks
nfsd: Fix races with check_stateid_generation()
fs/nfsd/nfs4state.c | 128 +++++++++++++++++++++++++++++++++++++++-------------
1 file changed, 97 insertions(+), 31 deletions(-)
--
2.13.6
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfsd/nfs4state.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index b246aaa20863..0fa42c704b9a 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -63,6 +63,9 @@ static const stateid_t zero_stateid = {
static const stateid_t currentstateid = {
.si_generation = 1,
};
+static const stateid_t close_stateid = {
+ .si_generation = 0xffffffffU,
+};
static u64 current_sessionid = 1;
@@ -5390,6 +5393,11 @@ nfsd4_close(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
nfsd4_close_open_stateid(stp);
mutex_unlock(&stp->st_mutex);
+ /* See RFC5661 sectionm 18.2.4 */
+ if (stp->st_stid.sc_client->cl_minorversion)
+ memcpy(&close->cl_stateid, &close_stateid,
+ sizeof(close->cl_stateid));
+
/* put reference from nfs4_preprocess_seqid_op */
nfs4_put_stid(&stp->st_stid);
out:
--
2.13.6
In order to deal with lookup races, nfsd4_free_lock_stateid() needs
to be able to signal to other stateful functions that the lock stateid
is no longer valid. Right now, nfsd_lock() will check whether or not an
existing stateid is still hashed, but only in the "new lock" path.
To ensure the stateid invalidation is also recognised by the "existing lock"
path, and also by a second call to nfsd4_free_lock_stateid() itself, we can
change the type to NFS4_CLOSED_STID under the stp->st_mutex.
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfsd/nfs4state.c | 19 ++++++++-----------
1 file changed, 8 insertions(+), 11 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 0fa42c704b9a..cc8fff93146d 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -5078,7 +5078,9 @@ 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 = nfsd4_lock_ol_stateid(stp);
+ if (ret)
+ goto out_put_stid;
ret = check_stateid_generation(stateid, &s->sc_stateid, 1);
if (ret)
@@ -5089,11 +5091,13 @@ nfsd4_free_lock_stateid(stateid_t *stateid, struct nfs4_stid *s)
lockowner(stp->st_stateowner)))
goto out;
+ stp->st_stid.sc_type = NFS4_CLOSED_STID;
release_lock_stateid(stp);
ret = nfs_ok;
out:
mutex_unlock(&stp->st_mutex);
+out_put_stid:
nfs4_put_stid(s);
return ret;
}
@@ -5662,6 +5666,8 @@ find_lock_stateid(struct nfs4_lockowner *lo, struct nfs4_file *fp)
lockdep_assert_held(&clp->cl_lock);
list_for_each_entry(lst, &lo->lo_owner.so_stateids, st_perstateowner) {
+ if (lst->st_stid.sc_type != NFS4_LOCK_STID)
+ continue;
if (lst->st_stid.sc_file == fp) {
atomic_inc(&lst->st_stid.sc_count);
return lst;
@@ -5736,7 +5742,6 @@ lookup_or_create_lock_state(struct nfsd4_compound_state *cstate,
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) {
@@ -5759,15 +5764,7 @@ lookup_or_create_lock_state(struct nfsd4_compound_state *cstate,
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);
+ if (nfsd4_lock_ol_stateid(lst) != nfs_ok) {
nfs4_put_stid(&lst->st_stid);
goto retry;
}
--
2.13.6
After taking the stateid st_mutex, we want to know that the stateid
still represents valid state before performing any non-idempotent
actions.
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfsd/nfs4state.c | 12 +++---------
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index cc8fff93146d..ab56d265ef68 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -5163,15 +5163,9 @@ static __be32 nfs4_seqid_op_checks(struct nfsd4_compound_state *cstate, stateid_
status = nfsd4_check_seqid(cstate, sop, seqid);
if (status)
return status;
- if (stp->st_stid.sc_type == NFS4_CLOSED_STID
- || stp->st_stid.sc_type == NFS4_REVOKED_DELEG_STID)
- /*
- * "Closed" stateid's exist *only* to return
- * nfserr_replay_me from the previous step, and
- * revoked delegations are kept only for free_stateid.
- */
- return nfserr_bad_stateid;
- mutex_lock(&stp->st_mutex);
+ status = nfsd4_lock_ol_stateid(stp);
+ if (status != nfs_ok)
+ return status;
status = check_stateid_generation(stateid, &stp->st_stid.sc_stateid, nfsd4_has_session(cstate));
if (status == nfs_ok)
status = nfs4_check_fh(current_fh, &stp->st_stid);
--
2.13.6
Open file stateids can linger on the nfs4_file list of stateids even
after they have been closed. In order to avoid reusing such a
stateid, and confusing the client, we need to recheck the
nfs4_stid's type after taking the mutex.
Otherwise, we risk reusing an old stateid that was already closed,
which will confuse clients that expect new stateids to conform to
RFC7530 Sections 9.1.4.2 and 16.2.5 or RFC5661 Sections 8.2.2 and 18.2.4.
Signed-off-by: Trond Myklebust <[email protected]>
Cc: [email protected]
---
fs/nfsd/nfs4state.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 59 insertions(+), 8 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 0c04f81aa63b..b246aaa20863 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3512,7 +3512,9 @@ nfsd4_find_existing_open(struct nfs4_file *fp, struct nfsd4_open *open)
/* ignore lock owners */
if (local->st_stateowner->so_is_open_owner == 0)
continue;
- if (local->st_stateowner == &oo->oo_owner) {
+ if (local->st_stateowner != &oo->oo_owner)
+ continue;
+ if (local->st_stid.sc_type == NFS4_OPEN_STID) {
ret = local;
atomic_inc(&ret->st_stid.sc_count);
break;
@@ -3521,6 +3523,52 @@ nfsd4_find_existing_open(struct nfs4_file *fp, struct nfsd4_open *open)
return ret;
}
+static __be32
+nfsd4_verify_open_stid(struct nfs4_stid *s)
+{
+ __be32 ret = nfs_ok;
+
+ switch (s->sc_type) {
+ default:
+ break;
+ case NFS4_CLOSED_STID:
+ case NFS4_CLOSED_DELEG_STID:
+ ret = nfserr_bad_stateid;
+ break;
+ case NFS4_REVOKED_DELEG_STID:
+ ret = nfserr_deleg_revoked;
+ }
+ return ret;
+}
+
+/* Lock the stateid st_mutex, and deal with races with CLOSE */
+static __be32
+nfsd4_lock_ol_stateid(struct nfs4_ol_stateid *stp)
+{
+ __be32 ret;
+
+ mutex_lock(&stp->st_mutex);
+ ret = nfsd4_verify_open_stid(&stp->st_stid);
+ if (ret != nfs_ok)
+ mutex_unlock(&stp->st_mutex);
+ return ret;
+}
+
+static struct nfs4_ol_stateid *
+nfsd4_find_and_lock_existing_open(struct nfs4_file *fp, struct nfsd4_open *open)
+{
+ struct nfs4_ol_stateid *stp;
+ for (;;) {
+ spin_lock(&fp->fi_lock);
+ stp = nfsd4_find_existing_open(fp, open);
+ spin_unlock(&fp->fi_lock);
+ if (!stp || nfsd4_lock_ol_stateid(stp) == nfs_ok)
+ break;
+ nfs4_put_stid(&stp->st_stid);
+ }
+ return stp;
+}
+
static struct nfs4_openowner *
alloc_init_open_stateowner(unsigned int strhashval, struct nfsd4_open *open,
struct nfsd4_compound_state *cstate)
@@ -3565,6 +3613,7 @@ init_open_stateid(struct nfs4_file *fp, struct nfsd4_open *open)
mutex_init(&stp->st_mutex);
mutex_lock(&stp->st_mutex);
+retry:
spin_lock(&oo->oo_owner.so_client->cl_lock);
spin_lock(&fp->fi_lock);
@@ -3589,7 +3638,11 @@ init_open_stateid(struct nfs4_file *fp, struct nfsd4_open *open)
spin_unlock(&fp->fi_lock);
spin_unlock(&oo->oo_owner.so_client->cl_lock);
if (retstp) {
- mutex_lock(&retstp->st_mutex);
+ /* Handle races with CLOSE */
+ if (nfsd4_lock_ol_stateid(retstp) != nfs_ok) {
+ nfs4_put_stid(&retstp->st_stid);
+ goto retry;
+ }
/* To keep mutex tracking happy */
mutex_unlock(&stp->st_mutex);
stp = retstp;
@@ -4403,9 +4456,7 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
status = nfs4_check_deleg(cl, open, &dp);
if (status)
goto out;
- spin_lock(&fp->fi_lock);
- stp = nfsd4_find_existing_open(fp, open);
- spin_unlock(&fp->fi_lock);
+ stp = nfsd4_find_and_lock_existing_open(fp, open);
} else {
open->op_file = NULL;
status = nfserr_bad_stateid;
@@ -4419,7 +4470,6 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
*/
if (stp) {
/* Stateid was found, this is an OPEN upgrade */
- mutex_lock(&stp->st_mutex);
status = nfs4_upgrade_open(rqstp, fp, current_fh, stp, open);
if (status) {
mutex_unlock(&stp->st_mutex);
@@ -5294,7 +5344,6 @@ static void nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
bool unhashed;
LIST_HEAD(reaplist);
- s->st_stid.sc_type = NFS4_CLOSED_STID;
spin_lock(&clp->cl_lock);
unhashed = unhash_open_stateid(s, &reaplist);
@@ -5334,10 +5383,12 @@ nfsd4_close(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
nfsd4_bump_seqid(cstate, status);
if (status)
goto out;
+
+ stp->st_stid.sc_type = NFS4_CLOSED_STID;
nfs4_inc_and_copy_stateid(&close->cl_stateid, &stp->st_stid);
- mutex_unlock(&stp->st_mutex);
nfsd4_close_open_stateid(stp);
+ mutex_unlock(&stp->st_mutex);
/* put reference from nfs4_preprocess_seqid_op */
nfs4_put_stid(&stp->st_stid);
--
2.13.6
The various functions that call check_stateid_generation() in order
to compare a client-supplied stateid with the nfs4_stid state, usually
need to atomically check for closed state. Those that perform the
check after locking the st_mutex using nfsd4_lock_ol_stateid()
should now be OK, but we do want to fix up the others.
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfsd/nfs4state.c | 22 +++++++++++++++++++---
1 file changed, 19 insertions(+), 3 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index ab56d265ef68..0603937c674f 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4851,6 +4851,18 @@ static __be32 check_stateid_generation(stateid_t *in, stateid_t *ref, bool has_s
return nfserr_old_stateid;
}
+static __be32 nfsd4_stid_check_stateid_generation(stateid_t *in, struct nfs4_stid *s, bool has_session)
+{
+ __be32 ret;
+
+ spin_lock(&s->sc_lock);
+ ret = nfsd4_verify_open_stid(s);
+ if (ret == nfs_ok)
+ ret = check_stateid_generation(in, &s->sc_stateid, has_session);
+ spin_unlock(&s->sc_lock);
+ return ret;
+}
+
static __be32 nfsd4_check_openowner_confirmed(struct nfs4_ol_stateid *ols)
{
if (ols->st_stateowner->so_is_open_owner &&
@@ -4879,7 +4891,7 @@ static __be32 nfsd4_validate_stateid(struct nfs4_client *cl, stateid_t *stateid)
s = find_stateid_locked(cl, stateid);
if (!s)
goto out_unlock;
- status = check_stateid_generation(stateid, &s->sc_stateid, 1);
+ status = nfsd4_stid_check_stateid_generation(stateid, s, 1);
if (status)
goto out_unlock;
switch (s->sc_type) {
@@ -5024,7 +5036,7 @@ nfs4_preprocess_stateid_op(struct svc_rqst *rqstp,
&s, nn);
if (status)
return status;
- status = check_stateid_generation(stateid, &s->sc_stateid,
+ status = nfsd4_stid_check_stateid_generation(stateid, s,
nfsd4_has_session(cstate));
if (status)
goto out;
@@ -5117,6 +5129,7 @@ nfsd4_free_stateid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
s = find_stateid_locked(cl, stateid);
if (!s)
goto out_unlock;
+ spin_lock(&s->sc_lock);
switch (s->sc_type) {
case NFS4_DELEG_STID:
ret = nfserr_locks_held;
@@ -5128,11 +5141,13 @@ nfsd4_free_stateid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
ret = nfserr_locks_held;
break;
case NFS4_LOCK_STID:
+ spin_unlock(&s->sc_lock);
atomic_inc(&s->sc_count);
spin_unlock(&cl->cl_lock);
ret = nfsd4_free_lock_stateid(stateid, s);
goto out;
case NFS4_REVOKED_DELEG_STID:
+ spin_unlock(&s->sc_lock);
dp = delegstateid(s);
list_del_init(&dp->dl_recall_lru);
spin_unlock(&cl->cl_lock);
@@ -5141,6 +5156,7 @@ nfsd4_free_stateid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
goto out;
/* Default falls through and returns nfserr_bad_stateid */
}
+ spin_unlock(&s->sc_lock);
out_unlock:
spin_unlock(&cl->cl_lock);
out:
@@ -5420,7 +5436,7 @@ nfsd4_delegreturn(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
if (status)
goto out;
dp = delegstateid(s);
- status = check_stateid_generation(stateid, &dp->dl_stid.sc_stateid, nfsd4_has_session(cstate));
+ status = nfsd4_stid_check_stateid_generation(stateid, &dp->dl_stid, nfsd4_has_session(cstate));
if (status)
goto put_stateid;
--
2.13.6
On 10/30/2017 05:09 PM, Trond Myklebust wrote:
> Signed-off-by: Trond Myklebust <[email protected]>
> ---
> fs/nfsd/nfs4state.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index b246aaa20863..0fa42c704b9a 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -63,6 +63,9 @@ static const stateid_t zero_stateid = {
> static const stateid_t currentstateid = {
> .si_generation = 1,
> };
> +static const stateid_t close_stateid = {
> + .si_generation = 0xffffffffU,
> +};
>
> static u64 current_sessionid = 1;
>
> @@ -5390,6 +5393,11 @@ nfsd4_close(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> nfsd4_close_open_stateid(stp);
> mutex_unlock(&stp->st_mutex);
>
> + /* See RFC5661 sectionm 18.2.4 */
> + if (stp->st_stid.sc_client->cl_minorversion)
> + memcpy(&close->cl_stateid, &close_stateid,
> + sizeof(close->cl_stateid));
> +
> /* put reference from nfs4_preprocess_seqid_op */
> nfs4_put_stid(&stp->st_stid);
> out:
Ooh, I'd never noticed that... Ganesha needs to do that also.
Bruce, can we get a pynfs test case to verify this?
Thanks
Frank
Not directly related to this patch series, but:
I keep staring at the failure path in nfsd4_process_open2() from the
call to nfs4_get_vfs_file() and thinking that there's a missing state
change to the stateid that's still hashed before the mutex is dropped
and the call to release_open_stateid()?
Thanks,
Andy
--
Andrew W. Elble
[email protected]
Infrastructure Engineer, Communications Technical Lead
Rochester Institute of Technology
PGP: BFAD 8461 4CCF DC95 DA2C B0EB 965B 082E 863E C912
T24gVHVlLCAyMDE3LTEwLTMxIGF0IDE4OjMzIC0wNDAwLCBBbmRyZXcgVyBFbGJsZSB3cm90ZToN
Cj4gTm90IGRpcmVjdGx5IHJlbGF0ZWQgdG8gdGhpcyBwYXRjaCBzZXJpZXMsIGJ1dDoNCj4gDQo+
IEkga2VlcCBzdGFyaW5nIGF0IHRoZSBmYWlsdXJlIHBhdGggaW4gbmZzZDRfcHJvY2Vzc19vcGVu
MigpIGZyb20gdGhlDQo+IGNhbGwgdG8gbmZzNF9nZXRfdmZzX2ZpbGUoKSBhbmQgdGhpbmtpbmcg
dGhhdCB0aGVyZSdzIGEgbWlzc2luZyBzdGF0ZQ0KPiBjaGFuZ2UgdG8gdGhlIHN0YXRlaWQgdGhh
dCdzIHN0aWxsIGhhc2hlZCBiZWZvcmUgdGhlIG11dGV4IGlzIGRyb3BwZWQNCj4gYW5kIHRoZSBj
YWxsIHRvIHJlbGVhc2Vfb3Blbl9zdGF0ZWlkKCk/DQo+IA0KDQpJZiB0aGUgc2VxaWQ9PTEsIHNv
IHRoYXQgd2Uga25vdyB0aGlzIE9QRU4gb3AgY3JlYXRlZCB0aGF0IHN0YXRlaWQsDQp0aGVuIGl0
IHByb2JhYmx5IHNob3VsZCBiZSB1bmhhc2hlZCBhbmQgbWFya2VkIGFzIGNsb3NlZCwgc2luY2Ug
dGhlbiB3ZQ0Ka25vdyBpdCByZXByZXNlbnRzIG5vIHN0YXRlLiBPdGhlcndpc2UsIEFGQUlDUyBp
dCBzaG91bGQga2VlcCBpdHMNCmN1cnJlbnQgc3RhdGUgKyBzZXFpZC4NCg0KRG8geW91IHdhbnQg
dG8gc2VuZCBhIHBhdGNoLCBvciBzaG91bGQgSSB1cGRhdGUgdGhpcyBwYXRjaCBzZXJpZXM/IFN1
Y2gNCmEgZml4IHByb2JhYmx5IGRvZXMgd2FudCB0byBiZSBhIHN0YWJsZSBwYXRjaCwgc2luY2Ug
aXQgd2lsbCBhZmZlY3QNCmNsaWVudHMgdGhhdCBleHBlY3QgY29tcGxpYW5jZSB3aXRoIFJGQzU2
NjEvUkZDNzUzMC4NCg0KLS0gDQpUcm9uZCBNeWtsZWJ1c3QNCkxpbnV4IE5GUyBjbGllbnQgbWFp
bnRhaW5lciwgUHJpbWFyeURhdGENCnRyb25kLm15a2xlYnVzdEBwcmltYXJ5ZGF0YS5jb20NCg==
Trond Myklebust <[email protected]> writes:
> On Tue, 2017-10-31 at 18:33 -0400, Andrew W Elble wrote:
>> Not directly related to this patch series, but:
>>
>> I keep staring at the failure path in nfsd4_process_open2() from the
>> call to nfs4_get_vfs_file() and thinking that there's a missing state
>> change to the stateid that's still hashed before the mutex is dropped
>> and the call to release_open_stateid()?
>>
>
> If the seqid==1, so that we know this OPEN op created that stateid,
> then it probably should be unhashed and marked as closed, since then we
> know it represents no state. Otherwise, AFAICS it should keep its
> current state + seqid.
>
> Do you want to send a patch, or should I update this patch series? Such
> a fix probably does want to be a stable patch, since it will affect
> clients that expect compliance with RFC5661/RFC7530.
Please feel free to go ahead and update this series.
Thanks,
Andy
--
Andrew W. Elble
[email protected]
Infrastructure Engineer, Communications Technical Lead
Rochester Institute of Technology
PGP: BFAD 8461 4CCF DC95 DA2C B0EB 965B 082E 863E C912