On Thu, Aug 20, 2009 at 03:21:56AM +0300, Benny Halevy wrote:
> nfsd4_sequence() should not renew the client state if the session was not
> found or if there was a bad slot. This will also avoid dereferencing a
> null session pointer.
>
> FIXME: until we work out the state locking so we can use a
> spin lock to protect the cl_lru we need to take the state_lock
> to renew the client.
Is that first paragraph left over from some previous iteration of this
patch?
>
> Signed-off-by: Benny Halevy <bhalevy@xxxxxxxxxxx>
> [nfsd41: Do not renew state on error]
> Signed-off-by: Ricardo Labiaga <Ricardo.Labiaga@xxxxxxxxxx>
> Signed-off-by: Benny Halevy <bhalevy@xxxxxxxxxxx>
> ---
>
> Bruce, I'm not whether it is too late but this patch fixes
> a bug in 2.6.31-rc that we've hit in the last Bakeathon.
I think we should wait for 2.6.32. (And was the bug you hit due to the
lack of state locking or to the NULL dereference?)
I don't have any fix for the locking problem queued up for 2.6.32.
--b.
>
> Benny
>
> fs/nfsd/nfs4state.c | 30 +++++++++++++++++++-----------
> 1 files changed, 19 insertions(+), 11 deletions(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 1dedde9..3e7b20a 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1436,12 +1436,16 @@ nfsd4_sequence(struct svc_rqst *rqstp,
> spin_lock(&sessionid_lock);
> status = nfserr_badsession;
> session = find_in_sessionid_hashtbl(&seq->sessionid);
> - if (!session)
> - goto out;
> + if (!session) {
> + spin_unlock(&sessionid_lock);
> + goto err;
> + }
>
> status = nfserr_badslot;
> - if (seq->slotid >= session->se_fchannel.maxreqs)
> - goto out;
> + if (seq->slotid >= session->se_fchannel.maxreqs) {
> + spin_unlock(&sessionid_lock);
> + goto err;
> + }
>
> slot = &session->se_slots[seq->slotid];
> dprintk("%s: slotid %d\n", __func__, seq->slotid);
> @@ -1454,10 +1458,12 @@ nfsd4_sequence(struct svc_rqst *rqstp,
> * for nfsd4_proc_compound processing */
> status = nfsd4_replay_cache_entry(resp, seq);
> cstate->status = nfserr_replay_cache;
> - goto replay_cache;
> - }
> - if (status)
> goto out;
> + }
> + if (status) {
> + spin_unlock(&sessionid_lock);
> + goto err;
> + }
>
> /* Success! bump slot seqid */
> slot->sl_inuse = true;
> @@ -1470,15 +1476,17 @@ nfsd4_sequence(struct svc_rqst *rqstp,
> cstate->slot = slot;
> cstate->session = session;
>
> -replay_cache:
> - /* Renew the clientid on success and on replay.
> - * Hold a session reference until done processing the compound:
> + /* Hold a session reference until done processing the compound:
> * nfsd4_put_session called only if the cstate slot is set.
> */
> - renew_client(session->se_client);
> nfsd4_get_session(session);
> out:
> spin_unlock(&sessionid_lock);
> + /* Renew the clientid on success and on replay */
> + nfs4_lock_state();
> + renew_client(session->se_client);
> + nfs4_unlock_state();
> +err:
> dprintk("%s: return %d\n", __func__, ntohl(status));
> return status;
> }
> --
> 1.6.4
>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
