Fix two known problems in clnt_rc.c, plus issues w.r.t. smp noted
during reading of the code. Change the code so that it never accesses rc_connecting, rc_closed or rc_client when the rc_lock mutex is not held. Also, it now performs the CLNT_CLOSE(client) and CLNT_RELEASE(client) calls after the rc_lock mutex has been released, since those calls do msleep()s with another mutex held. Change clnt_reconnect_call() so that releasing the reference count is delayed until after the "if (rc->rc_client == client)" check, so that rc_client cannot have been recycled. Tested by: pho Reviewed by: dfr Approved by: kib (mentor)
This commit is contained in:
Rick Macklem
parent
b7c3d41368
commit
72263475c4
Notes:
svn2git
2020-12-20 02:59:44 +00:00
svn path=/head/; revision=194934
@ -146,33 +146,33 @@ clnt_reconnect_connect(CLIENT *cl)
|
||||
int error;
|
||||
int one = 1;
|
||||
struct ucred *oldcred;
|
||||
CLIENT *newclient = NULL;
|
||||
|
||||
mtx_lock(&rc->rc_lock);
|
||||
again:
|
||||
while (rc->rc_connecting) {
|
||||
error = msleep(rc, &rc->rc_lock,
|
||||
rc->rc_intr ? PCATCH : 0, "rpcrecon", 0);
|
||||
if (error) {
|
||||
mtx_unlock(&rc->rc_lock);
|
||||
return (RPC_INTR);
|
||||
}
|
||||
}
|
||||
if (rc->rc_closed) {
|
||||
mtx_unlock(&rc->rc_lock);
|
||||
return (RPC_CANTSEND);
|
||||
}
|
||||
if (rc->rc_connecting) {
|
||||
while (!rc->rc_closed && !rc->rc_client && rc->rc_connecting) {
|
||||
error = msleep(rc, &rc->rc_lock,
|
||||
rc->rc_intr ? PCATCH : 0, "rpcrecon", 0);
|
||||
if (error) {
|
||||
mtx_unlock(&rc->rc_lock);
|
||||
return (RPC_INTR);
|
||||
}
|
||||
}
|
||||
/*
|
||||
* If the other guy failed to connect, we might as
|
||||
* well have another go.
|
||||
*/
|
||||
if (!rc->rc_client || rc->rc_closed)
|
||||
goto again;
|
||||
if (rc->rc_client) {
|
||||
mtx_unlock(&rc->rc_lock);
|
||||
return (RPC_SUCCESS);
|
||||
} else {
|
||||
rc->rc_connecting = TRUE;
|
||||
}
|
||||
|
||||
/*
|
||||
* My turn to attempt a connect. The rc_connecting variable
|
||||
* serializes the following code sequence, so it is guaranteed
|
||||
* that rc_client will still be NULL after it is re-locked below,
|
||||
* since that is the only place it is set non-NULL.
|
||||
*/
|
||||
rc->rc_connecting = TRUE;
|
||||
mtx_unlock(&rc->rc_lock);
|
||||
|
||||
so = __rpc_nconf2socket(rc->rc_nconf);
|
||||
@ -188,43 +188,52 @@ clnt_reconnect_connect(CLIENT *cl)
|
||||
bindresvport(so, NULL);
|
||||
|
||||
if (rc->rc_nconf->nc_semantics == NC_TPI_CLTS)
|
||||
rc->rc_client = clnt_dg_create(so,
|
||||
newclient = clnt_dg_create(so,
|
||||
(struct sockaddr *) &rc->rc_addr, rc->rc_prog, rc->rc_vers,
|
||||
rc->rc_sendsz, rc->rc_recvsz);
|
||||
else
|
||||
rc->rc_client = clnt_vc_create(so,
|
||||
newclient = clnt_vc_create(so,
|
||||
(struct sockaddr *) &rc->rc_addr, rc->rc_prog, rc->rc_vers,
|
||||
rc->rc_sendsz, rc->rc_recvsz);
|
||||
td->td_ucred = oldcred;
|
||||
|
||||
if (!rc->rc_client) {
|
||||
if (!newclient) {
|
||||
soclose(so);
|
||||
rc->rc_err = rpc_createerr.cf_error;
|
||||
stat = rpc_createerr.cf_stat;
|
||||
goto out;
|
||||
}
|
||||
|
||||
CLNT_CONTROL(rc->rc_client, CLSET_FD_CLOSE, 0);
|
||||
CLNT_CONTROL(rc->rc_client, CLSET_CONNECT, &one);
|
||||
CLNT_CONTROL(rc->rc_client, CLSET_TIMEOUT, &rc->rc_timeout);
|
||||
CLNT_CONTROL(rc->rc_client, CLSET_RETRY_TIMEOUT, &rc->rc_retry);
|
||||
CLNT_CONTROL(rc->rc_client, CLSET_WAITCHAN, rc->rc_waitchan);
|
||||
CLNT_CONTROL(rc->rc_client, CLSET_INTERRUPTIBLE, &rc->rc_intr);
|
||||
CLNT_CONTROL(newclient, CLSET_FD_CLOSE, 0);
|
||||
CLNT_CONTROL(newclient, CLSET_CONNECT, &one);
|
||||
CLNT_CONTROL(newclient, CLSET_TIMEOUT, &rc->rc_timeout);
|
||||
CLNT_CONTROL(newclient, CLSET_RETRY_TIMEOUT, &rc->rc_retry);
|
||||
CLNT_CONTROL(newclient, CLSET_WAITCHAN, rc->rc_waitchan);
|
||||
CLNT_CONTROL(newclient, CLSET_INTERRUPTIBLE, &rc->rc_intr);
|
||||
stat = RPC_SUCCESS;
|
||||
|
||||
out:
|
||||
mtx_lock(&rc->rc_lock);
|
||||
if (rc->rc_closed) {
|
||||
if (rc->rc_client) {
|
||||
CLNT_CLOSE(rc->rc_client);
|
||||
CLNT_RELEASE(rc->rc_client);
|
||||
rc->rc_client = NULL;
|
||||
}
|
||||
KASSERT(rc->rc_client == NULL, ("rc_client not null"));
|
||||
if (!rc->rc_closed) {
|
||||
rc->rc_client = newclient;
|
||||
newclient = NULL;
|
||||
}
|
||||
rc->rc_connecting = FALSE;
|
||||
wakeup(rc);
|
||||
mtx_unlock(&rc->rc_lock);
|
||||
|
||||
if (newclient) {
|
||||
/*
|
||||
* It has been closed, so discard the new client.
|
||||
* nb: clnt_[dg|vc]_close()/clnt_[dg|vc]_destroy() cannot
|
||||
* be called with the rc_lock mutex held, since they may
|
||||
* msleep() while holding a different mutex.
|
||||
*/
|
||||
CLNT_CLOSE(newclient);
|
||||
CLNT_RELEASE(newclient);
|
||||
}
|
||||
|
||||
return (stat);
|
||||
}
|
||||
|
||||
@ -240,19 +249,24 @@ clnt_reconnect_call(
|
||||
struct rc_data *rc = (struct rc_data *)cl->cl_private;
|
||||
CLIENT *client;
|
||||
enum clnt_stat stat;
|
||||
int tries;
|
||||
int tries, error;
|
||||
|
||||
tries = 0;
|
||||
do {
|
||||
mtx_lock(&rc->rc_lock);
|
||||
if (rc->rc_closed) {
|
||||
mtx_unlock(&rc->rc_lock);
|
||||
return (RPC_CANTSEND);
|
||||
}
|
||||
|
||||
if (!rc->rc_client) {
|
||||
mtx_unlock(&rc->rc_lock);
|
||||
stat = clnt_reconnect_connect(cl);
|
||||
if (stat == RPC_SYSTEMERROR) {
|
||||
(void) tsleep(&fake_wchan, 0,
|
||||
"rpccon", hz);
|
||||
error = tsleep(&fake_wchan,
|
||||
rc->rc_intr ? PCATCH : 0, "rpccon", hz);
|
||||
if (error == EINTR || error == ERESTART)
|
||||
return (RPC_INTR);
|
||||
tries++;
|
||||
if (tries >= rc->rc_retries)
|
||||
return (stat);
|
||||
@ -260,9 +274,9 @@ clnt_reconnect_call(
|
||||
}
|
||||
if (stat != RPC_SUCCESS)
|
||||
return (stat);
|
||||
mtx_lock(&rc->rc_lock);
|
||||
}
|
||||
|
||||
mtx_lock(&rc->rc_lock);
|
||||
if (!rc->rc_client) {
|
||||
mtx_unlock(&rc->rc_lock);
|
||||
stat = RPC_FAILED;
|
||||
@ -279,7 +293,6 @@ clnt_reconnect_call(
|
||||
CLNT_GETERR(client, &rc->rc_err);
|
||||
}
|
||||
|
||||
CLNT_RELEASE(client);
|
||||
if (stat == RPC_TIMEDOUT) {
|
||||
/*
|
||||
* Check for async send misfeature for NLM
|
||||
@ -290,6 +303,7 @@ clnt_reconnect_call(
|
||||
|| (rc->rc_timeout.tv_sec == -1
|
||||
&& utimeout.tv_sec == 0
|
||||
&& utimeout.tv_usec == 0)) {
|
||||
CLNT_RELEASE(client);
|
||||
break;
|
||||
}
|
||||
}
|
||||
@ -297,8 +311,10 @@ clnt_reconnect_call(
|
||||
if (stat == RPC_TIMEDOUT || stat == RPC_CANTSEND
|
||||
|| stat == RPC_CANTRECV) {
|
||||
tries++;
|
||||
if (tries >= rc->rc_retries)
|
||||
if (tries >= rc->rc_retries) {
|
||||
CLNT_RELEASE(client);
|
||||
break;
|
||||
}
|
||||
|
||||
if (ext && ext->rc_feedback)
|
||||
ext->rc_feedback(FEEDBACK_RECONNECT, proc,
|
||||
@ -307,14 +323,22 @@ clnt_reconnect_call(
|
||||
mtx_lock(&rc->rc_lock);
|
||||
/*
|
||||
* Make sure that someone else hasn't already
|
||||
* reconnected.
|
||||
* reconnected by checking if rc_client has changed.
|
||||
* If not, we are done with the client and must
|
||||
* do CLNT_RELEASE(client) twice to dispose of it,
|
||||
* because there is both an initial refcnt and one
|
||||
* acquired by CLNT_ACQUIRE() above.
|
||||
*/
|
||||
if (rc->rc_client == client) {
|
||||
CLNT_RELEASE(rc->rc_client);
|
||||
rc->rc_client = NULL;
|
||||
mtx_unlock(&rc->rc_lock);
|
||||
CLNT_RELEASE(client);
|
||||
} else {
|
||||
mtx_unlock(&rc->rc_lock);
|
||||
}
|
||||
mtx_unlock(&rc->rc_lock);
|
||||
CLNT_RELEASE(client);
|
||||
} else {
|
||||
CLNT_RELEASE(client);
|
||||
break;
|
||||
}
|
||||
} while (stat != RPC_SUCCESS);
|
||||
@ -333,6 +357,10 @@ clnt_reconnect_geterr(CLIENT *cl, struct rpc_err *errp)
|
||||
*errp = rc->rc_err;
|
||||
}
|
||||
|
||||
/*
|
||||
* Since this function requires that rc_client be valid, it can
|
||||
* only be called when that is guaranteed to be the case.
|
||||
*/
|
||||
static bool_t
|
||||
clnt_reconnect_freeres(CLIENT *cl, xdrproc_t xdr_res, void *res_ptr)
|
||||
{
|
||||
@ -347,6 +375,10 @@ clnt_reconnect_abort(CLIENT *h)
|
||||
{
|
||||
}
|
||||
|
||||
/*
|
||||
* CLNT_CONTROL() on the client returned by clnt_reconnect_create() must
|
||||
* always be called before CLNT_CALL_MBUF() by a single thread only.
|
||||
*/
|
||||
static bool_t
|
||||
clnt_reconnect_control(CLIENT *cl, u_int request, void *info)
|
||||
{
|
||||
|
||||
Loading…
Reference in New Issue
Block a user