nfsd: fix race with open / open upgrade stateids

We observed multiple open stateids on the server for files that
seemingly should have been closed.

nfsd4_process_open2() tests for the existence of a preexisting
stateid. If one is not found, the locks are dropped and a new
one is created. The problem is that init_open_stateid(), which
is also responsible for hashing the newly initialized stateid,
doesn't check to see if another open has raced in and created
a matching stateid. This fix is to enable init_open_stateid() to
return the matching stateid and have nfsd4_process_open2()
swap to that stateid and switch to the open upgrade path.
In testing this patch, coverage to the newly created
path indicates that the race was indeed happening.

Signed-off-by: Andrew Elble <aweits@rit.edu>
Reviewed-by: Jeff Layton <jlayton@poochiereds.net>
Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 6411c34218706df6c2ce999a079fb7a7bd523fae..6b800b5b8fedb5d60c8bf1a8d1ca6d35f3d54114 100644 (file)
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3392,6 +3392,27 @@ static const struct nfs4_stateowner_operations openowner_ops = {
        .so_free =      nfs4_free_openowner,
 };
 
+static struct nfs4_ol_stateid *
+nfsd4_find_existing_open(struct nfs4_file *fp, struct nfsd4_open *open)
+{
+       struct nfs4_ol_stateid *local, *ret = NULL;
+       struct nfs4_openowner *oo = open->op_openowner;
+
+       lockdep_assert_held(&fp->fi_lock);
+
+       list_for_each_entry(local, &fp->fi_stateids, st_perfile) {
+               /* ignore lock owners */
+               if (local->st_stateowner->so_is_open_owner == 0)
+                       continue;
+               if (local->st_stateowner == &oo->oo_owner) {
+                       ret = local;
+                       atomic_inc(&ret->st_stid.sc_count);
+                       break;
+               }
+       }
+       return ret;
+}
+
 static struct nfs4_openowner *
 alloc_init_open_stateowner(unsigned int strhashval, struct nfsd4_open *open,
                           struct nfsd4_compound_state *cstate)
@@ -3423,9 +3444,20 @@ alloc_init_open_stateowner(unsigned int strhashval, struct nfsd4_open *open,
        return ret;
 }
 
-static void init_open_stateid(struct nfs4_ol_stateid *stp, struct nfs4_file *fp, struct nfsd4_open *open) {
+static struct nfs4_ol_stateid *
+init_open_stateid(struct nfs4_ol_stateid *stp, struct nfs4_file *fp,
+               struct nfsd4_open *open)
+{
+
        struct nfs4_openowner *oo = open->op_openowner;
+       struct nfs4_ol_stateid *retstp = NULL;
 
+       spin_lock(&oo->oo_owner.so_client->cl_lock);
+       spin_lock(&fp->fi_lock);
+
+       retstp = nfsd4_find_existing_open(fp, open);
+       if (retstp)
+               goto out_unlock;
        atomic_inc(&stp->st_stid.sc_count);
        stp->st_stid.sc_type = NFS4_OPEN_STID;
        INIT_LIST_HEAD(&stp->st_locks);
@@ -3436,12 +3468,13 @@ static void init_open_stateid(struct nfs4_ol_stateid *stp, struct nfs4_file *fp,
        stp->st_deny_bmap = 0;
        stp->st_openstp = NULL;
        init_rwsem(&stp->st_rwsem);
-       spin_lock(&oo->oo_owner.so_client->cl_lock);
        list_add(&stp->st_perstateowner, &oo->oo_owner.so_stateids);
-       spin_lock(&fp->fi_lock);
        list_add(&stp->st_perfile, &fp->fi_stateids);
+
+out_unlock:
        spin_unlock(&fp->fi_lock);
        spin_unlock(&oo->oo_owner.so_client->cl_lock);
+       return retstp;
 }
 
 /*
@@ -3852,27 +3885,6 @@ out:
        return nfs_ok;
 }
 
-static struct nfs4_ol_stateid *
-nfsd4_find_existing_open(struct nfs4_file *fp, struct nfsd4_open *open)
-{
-       struct nfs4_ol_stateid *local, *ret = NULL;
-       struct nfs4_openowner *oo = open->op_openowner;
-
-       spin_lock(&fp->fi_lock);
-       list_for_each_entry(local, &fp->fi_stateids, st_perfile) {
-               /* ignore lock owners */
-               if (local->st_stateowner->so_is_open_owner == 0)
-                       continue;
-               if (local->st_stateowner == &oo->oo_owner) {
-                       ret = local;
-                       atomic_inc(&ret->st_stid.sc_count);
-                       break;
-               }
-       }
-       spin_unlock(&fp->fi_lock);
-       return ret;
-}
-
 static inline int nfs4_access_to_access(u32 nfs4_access)
 {
        int flags = 0;
@@ -4258,6 +4270,7 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
        struct nfs4_client *cl = open->op_openowner->oo_owner.so_client;
        struct nfs4_file *fp = NULL;
        struct nfs4_ol_stateid *stp = NULL;
+       struct nfs4_ol_stateid *swapstp = NULL;
        struct nfs4_delegation *dp = NULL;
        __be32 status;
 
@@ -4271,7 +4284,9 @@ 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);
        } else {
                open->op_file = NULL;
                status = nfserr_bad_stateid;
@@ -4294,7 +4309,19 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
        } else {
                stp = open->op_stp;
                open->op_stp = NULL;
-               init_open_stateid(stp, fp, open);
+               swapstp = init_open_stateid(stp, fp, open);
+               if (swapstp) {
+                       nfs4_put_stid(&stp->st_stid);
+                       stp = swapstp;
+                       down_read(&stp->st_rwsem);
+                       status = nfs4_upgrade_open(rqstp, fp, current_fh,
+                                               stp, open);
+                       if (status) {
+                               up_read(&stp->st_rwsem);
+                               goto out;
+                       }
+                       goto upgrade_out;
+               }
                down_read(&stp->st_rwsem);
                status = nfs4_get_vfs_file(rqstp, fp, current_fh, stp, open);
                if (status) {
@@ -4308,6 +4335,7 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
                if (stp->st_clnt_odstate == open->op_odstate)
                        open->op_odstate = NULL;
        }
+upgrade_out:
        nfs4_inc_and_copy_stateid(&open->op_stateid, &stp->st_stid);
        up_read(&stp->st_rwsem);