nfsd: ensure that seqid morphing operations are atomic wrt to copies

Bruce points out that the increment of the seqid in stateids is not
serialized in any way, so it's possible for racing calls to bump it
twice and end up sending the same stateid. While we don't have any
reports of this problem it _is_ theoretically possible, and could lead
to spurious state recovery by the client.

In the current code, update_stateid is always followed by a memcpy of
that stateid, so we can combine the two operations. For better
atomicity, we add a spinlock to the nfs4_stid and hold that when bumping
the seqid and copying the stateid.

Signed-off-by: Jeff Layton <jeff.layton@primarydata.com>
Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/fs/nfsd/nfs4layouts.c b/fs/nfsd/nfs4layouts.c
index 4a68ab901b4b3f131645930da50f6e53ee325fe5..9ffef06b30d51e2acb2ae1a6570f14c2c47eac59 100644 (file)
--- a/fs/nfsd/nfs4layouts.c
+++ b/fs/nfsd/nfs4layouts.c
@@ -409,8 +409,7 @@ nfsd4_insert_layout(struct nfsd4_layoutget *lgp, struct nfs4_layout_stateid *ls)
        list_add_tail(&new->lo_perstate, &ls->ls_layouts);
        new = NULL;
 done:
-       update_stateid(&ls->ls_stid.sc_stateid);
-       memcpy(&lgp->lg_sid, &ls->ls_stid.sc_stateid, sizeof(stateid_t));
+       nfs4_inc_and_copy_stateid(&lgp->lg_sid, &ls->ls_stid);
        spin_unlock(&ls->ls_lock);
 out:
        spin_unlock(&fp->fi_lock);
@@ -484,11 +483,8 @@ nfsd4_return_file_layouts(struct svc_rqst *rqstp,
                }
        }
        if (!list_empty(&ls->ls_layouts)) {
-               if (found) {
-                       update_stateid(&ls->ls_stid.sc_stateid);
-                       memcpy(&lrp->lr_sid, &ls->ls_stid.sc_stateid,
-                               sizeof(stateid_t));
-               }
+               if (found)
+                       nfs4_inc_and_copy_stateid(&lrp->lr_sid, &ls->ls_stid);
                lrp->lrs_present = 1;
        } else {
                trace_layoutstate_unhash(&ls->ls_stid.sc_stateid);
@@ -619,8 +615,7 @@ nfsd4_cb_layout_prepare(struct nfsd4_callback *cb)
                container_of(cb, struct nfs4_layout_stateid, ls_recall);
 
        mutex_lock(&ls->ls_mutex);
-       update_stateid(&ls->ls_stid.sc_stateid);
-       memcpy(&ls->ls_recall_sid, &ls->ls_stid.sc_stateid, sizeof(stateid_t));
+       nfs4_inc_and_copy_stateid(&ls->ls_recall_sid, &ls->ls_stid);
 }
 
 static int

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index f2ea343086b85f6b01d6c371e0c2d89c217320e7..0a697158a4ca47f90d059bbc9f0b1abbc06e1fa8 100644 (file)
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -575,6 +575,7 @@ struct nfs4_stid *nfs4_alloc_stid(struct nfs4_client *cl,
        stid->sc_stateid.si_opaque.so_clid = cl->cl_clientid;
        /* Will be incremented before return to client: */
        atomic_set(&stid->sc_count, 1);
+       spin_lock_init(&stid->sc_lock);
 
        /*
         * It shouldn't be a problem to reuse an opaque stateid value.
@@ -745,6 +746,18 @@ nfs4_put_stid(struct nfs4_stid *s)
                put_nfs4_file(fp);
 }
 
+void
+nfs4_inc_and_copy_stateid(stateid_t *dst, struct nfs4_stid *stid)
+{
+       stateid_t *src = &stid->sc_stateid;
+
+       spin_lock(&stid->sc_lock);
+       if (unlikely(++src->si_generation == 0))
+               src->si_generation = 1;
+       memcpy(dst, src, sizeof(*dst));
+       spin_unlock(&stid->sc_lock);
+}
+
 static void nfs4_put_deleg_lease(struct nfs4_file *fp)
 {
        struct file *filp = NULL;
@@ -4221,8 +4234,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;
        }
-       update_stateid(&stp->st_stid.sc_stateid);
-       memcpy(&open->op_stateid, &stp->st_stid.sc_stateid, sizeof(stateid_t));
+       nfs4_inc_and_copy_stateid(&open->op_stateid, &stp->st_stid);
        up_read(&stp->st_rwsem);
 
        if (nfsd4_has_session(&resp->cstate)) {
@@ -4925,8 +4937,7 @@ nfsd4_open_confirm(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
                goto put_stateid;
        }
        oo->oo_flags |= NFS4_OO_CONFIRMED;
-       update_stateid(&stp->st_stid.sc_stateid);
-       memcpy(&oc->oc_resp_stateid, &stp->st_stid.sc_stateid, sizeof(stateid_t));
+       nfs4_inc_and_copy_stateid(&oc->oc_resp_stateid, &stp->st_stid);
        up_write(&stp->st_rwsem);
        dprintk("NFSD: %s: success, seqid=%d stateid=" STATEID_FMT "\n",
                __func__, oc->oc_seqid, STATEID_VAL(&stp->st_stid.sc_stateid));
@@ -4999,11 +5010,8 @@ nfsd4_open_downgrade(struct svc_rqst *rqstp,
                goto put_stateid;
        }
        nfs4_stateid_downgrade(stp, od->od_share_access);
-
        reset_union_bmap_deny(od->od_share_deny, stp);
-
-       update_stateid(&stp->st_stid.sc_stateid);
-       memcpy(&od->od_stateid, &stp->st_stid.sc_stateid, sizeof(stateid_t));
+       nfs4_inc_and_copy_stateid(&od->od_stateid, &stp->st_stid);
        status = nfs_ok;
 put_stateid:
        up_write(&stp->st_rwsem);
@@ -5058,8 +5066,7 @@ nfsd4_close(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
        nfsd4_bump_seqid(cstate, status);
        if (status)
                goto out; 
-       update_stateid(&stp->st_stid.sc_stateid);
-       memcpy(&close->cl_stateid, &stp->st_stid.sc_stateid, sizeof(stateid_t));
+       nfs4_inc_and_copy_stateid(&close->cl_stateid, &stp->st_stid);
        up_write(&stp->st_rwsem);
 
        nfsd4_close_open_stateid(stp);
@@ -5542,9 +5549,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
        err = vfs_lock_file(filp, F_SETLK, file_lock, conflock);
        switch (-err) {
        case 0: /* success! */
-               update_stateid(&lock_stp->st_stid.sc_stateid);
-               memcpy(&lock->lk_resp_stateid, &lock_stp->st_stid.sc_stateid, 
-                               sizeof(stateid_t));
+               nfs4_inc_and_copy_stateid(&lock->lk_resp_stateid, &lock_stp->st_stid);
                status = 0;
                break;
        case (EAGAIN):          /* conflock holds conflicting lock */
@@ -5736,8 +5741,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
                dprintk("NFSD: nfs4_locku: vfs_lock_file failed!\n");
                goto out_nfserr;
        }
-       update_stateid(&stp->st_stid.sc_stateid);
-       memcpy(&locku->lu_stateid, &stp->st_stid.sc_stateid, sizeof(stateid_t));
+       nfs4_inc_and_copy_stateid(&locku->lu_stateid, &stp->st_stid);
 fput:
        fput(filp);
 put_stateid:

diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 1fa0f3848d4eb6324dbf391145ac1ef25f023874..77fdf4de91baa102dbefd2f15c5314be40aff162 100644 (file)
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -84,7 +84,7 @@ struct nfsd4_callback_ops {
  * fields that are of general use to any stateid.
  */
 struct nfs4_stid {
-       atomic_t sc_count;
+       atomic_t                sc_count;
 #define NFS4_OPEN_STID 1
 #define NFS4_LOCK_STID 2
 #define NFS4_DELEG_STID 4
@@ -94,11 +94,12 @@ struct nfs4_stid {
 #define NFS4_REVOKED_DELEG_STID 16
 #define NFS4_CLOSED_DELEG_STID 32
 #define NFS4_LAYOUT_STID 64
-       unsigned char sc_type;
-       stateid_t sc_stateid;
-       struct nfs4_client *sc_client;
-       struct nfs4_file *sc_file;
-       void (*sc_free)(struct nfs4_stid *);
+       unsigned char           sc_type;
+       stateid_t               sc_stateid;
+       spinlock_t              sc_lock;
+       struct nfs4_client      *sc_client;
+       struct nfs4_file        *sc_file;
+       void                    (*sc_free)(struct nfs4_stid *);
 };
 
 /*
@@ -364,15 +365,6 @@ struct nfs4_client_reclaim {
        char                    cr_recdir[HEXDIR_LEN]; /* recover dir */
 };
 
-static inline void
-update_stateid(stateid_t *stateid)
-{
-       stateid->si_generation++;
-       /* Wraparound recommendation from 3530bis-13 9.1.3.2: */
-       if (stateid->si_generation == 0)
-               stateid->si_generation = 1;
-}
-
 /* A reasonable value for REPLAY_ISIZE was estimated as follows:  
  * The OPEN response, typically the largest, requires 
  *   4(status) + 8(stateid) + 20(changeinfo) + 4(rflags) +  8(verifier) + 
@@ -595,6 +587,7 @@ struct nfs4_stid *nfs4_alloc_stid(struct nfs4_client *cl,
                struct kmem_cache *slab);
 void nfs4_unhash_stid(struct nfs4_stid *s);
 void nfs4_put_stid(struct nfs4_stid *s);
+void nfs4_inc_and_copy_stateid(stateid_t *dst, struct nfs4_stid *stid);
 void nfs4_remove_reclaim_record(struct nfs4_client_reclaim *, struct nfsd_net *);
 extern void nfs4_release_reclaim(struct nfsd_net *);
 extern struct nfs4_client_reclaim *nfsd4_find_reclaim_client(const char *recdir,