Hi,
These timeouts in sunrpc need to be based on the monotonic
clock to avoid a race with adjtime(2), settimeofday(2), etc.
There are obvious possible improvements here and elsewhere
in sunrpc. Here especially the time-related variable names
could be made more descriptive, the various BSD time macros
could be more effectively employed, and ppoll(2) can (now) be
leveraged to ignore EINTR and simplify those loops. But that
all belongs in a separate diff.
Likewise, there are dead timevals elsewhere in RPC
that I can remove in a separate diff.
Then again, this is ancient upstream code. Does cleanup here
put too much burden on the project? Like, I imagine that the
sort of cleanup I'm seeing here could make merging changes
from elsewhere painful.
I have also included here my sys/time.h diff from like two
seconds ago, which enables the use of TIMEVAL_TO_TIMESPEC
in the body of the if statement in clnt_udp.c. Its use makes
the diff simpler.
This is probably not adequately tested, though it compiles
and I'm not seeing any issues in my (small) NFS setup.
Thoughts and feedback?
--
Scott Cheloha
Index: lib/libc/rpc/clnt_tcp.c
===================================================================
RCS file: /cvs/src/lib/libc/rpc/clnt_tcp.c,v
retrieving revision 1.29
diff -u -p -r1.29 clnt_tcp.c
--- lib/libc/rpc/clnt_tcp.c 1 Nov 2015 03:45:29 -0000 1.29
+++ lib/libc/rpc/clnt_tcp.c 10 Dec 2017 06:08:55 -0000
@@ -385,25 +385,26 @@ static int
readtcp(struct ct_data *ct, caddr_t buf, int len)
{
struct pollfd pfd[1];
- struct timeval start, after, duration, tmp;
- int delta, r, save_errno;
+ struct timespec start, after, duration, tmp, delta, wait;
+ int r, save_errno;
if (len == 0)
return (0);
pfd[0].fd = ct->ct_sock;
pfd[0].events = POLLIN;
- delta = ct->ct_wait.tv_sec * 1000 + ct->ct_wait.tv_usec / 1000;
- gettimeofday(&start, NULL);
+ TIMEVAL_TO_TIMESPEC(&ct->ct_wait, &wait);
+ delta = wait;
+ clock_gettime(CLOCK_MONOTONIC, &start);
for (;;) {
- r = poll(pfd, 1, delta);
+ r = ppoll(pfd, 1, &delta, NULL);
save_errno = errno;
- gettimeofday(&after, NULL);
- timersub(&start, &after, &duration);
- timersub(&ct->ct_wait, &duration, &tmp);
- delta = tmp.tv_sec * 1000 + tmp.tv_usec / 1000;
- if (delta <= 0)
+ clock_gettime(CLOCK_MONOTONIC, &after);
+ timespecsub(&start, &after, &duration);
+ timespecsub(&wait, &duration, &tmp);
+ delta = tmp;
+ if (delta.tv_sec < 0 || !timespecisset(&delta))
r = 0;
switch (r) {
Index: lib/libc/rpc/clnt_udp.c
===================================================================
RCS file: /cvs/src/lib/libc/rpc/clnt_udp.c,v
retrieving revision 1.32
diff -u -p -r1.32 clnt_udp.c
--- lib/libc/rpc/clnt_udp.c 1 Nov 2015 03:45:29 -0000 1.32
+++ lib/libc/rpc/clnt_udp.c 10 Dec 2017 06:08:55 -0000
@@ -216,19 +216,20 @@ clntudp_call(CLIENT *cl, /* client handl
struct sockaddr_in from;
struct rpc_msg reply_msg;
XDR reply_xdrs;
- struct timeval time_waited, start, after, tmp1, tmp2;
+ struct timespec time_waited, start, after, tmp1, tmp2, wait;
bool_t ok;
int nrefreshes = 2; /* number of times to refresh cred */
- struct timeval timeout;
+ struct timespec timeout;
if (cu->cu_total.tv_usec == -1)
- timeout = utimeout; /* use supplied timeout */
+ TIMEVAL_TO_TIMESPEC(&utimeout, &timeout); /* use supplied
timeout */
else
- timeout = cu->cu_total; /* use default timeout */
+ TIMEVAL_TO_TIMESPEC(&cu->cu_total, &timeout); /* use default
timeout */
pfd[0].fd = cu->cu_sock;
pfd[0].events = POLLIN;
- timerclear(&time_waited);
+ timespecclear(&time_waited);
+ TIMEVAL_TO_TIMESPEC(&cu->cu_wait, &wait);
call_again:
xdrs = &(cu->cu_outxdrs);
xdrs->x_op = XDR_ENCODE;
@@ -254,7 +255,7 @@ send_again:
/*
* Hack to provide rpc-based message passing
*/
- if (!timerisset(&timeout))
+ if (!timespecisset(&timeout))
return (cu->cu_error.re_status = RPC_TIMEDOUT);
/*
@@ -266,14 +267,13 @@ send_again:
reply_msg.acpted_rply.ar_results.where = resultsp;
reply_msg.acpted_rply.ar_results.proc = xresults;
- gettimeofday(&start, NULL);
+ clock_gettime(CLOCK_MONOTONIC, &start);
for (;;) {
- switch (poll(pfd, 1,
- cu->cu_wait.tv_sec * 1000 + cu->cu_wait.tv_usec / 1000)) {
+ switch (ppoll(pfd, 1, &wait, NULL)) {
case 0:
- timeradd(&time_waited, &cu->cu_wait, &tmp1);
+ timespecadd(&time_waited, &wait, &tmp1);
time_waited = tmp1;
- if (timercmp(&time_waited, &timeout, <))
+ if (timespeccmp(&time_waited, &timeout, <))
goto send_again;
return (cu->cu_error.re_status = RPC_TIMEDOUT);
case 1:
@@ -286,11 +286,11 @@ send_again:
/* FALLTHROUGH */
case -1:
if (errno == EINTR) {
- gettimeofday(&after, NULL);
- timersub(&after, &start, &tmp1);
- timeradd(&time_waited, &tmp1, &tmp2);
+ clock_gettime(CLOCK_MONOTONIC, &after);
+ timespecsub(&after, &start, &tmp1);
+ timespecadd(&time_waited, &tmp1, &tmp2);
time_waited = tmp2;
- if (timercmp(&time_waited, &timeout, <))
+ if (timespeccmp(&time_waited, &timeout, <))
continue;
return (cu->cu_error.re_status = RPC_TIMEDOUT);
}
Index: lib/libc/rpc/svc_tcp.c
===================================================================
RCS file: /cvs/src/lib/libc/rpc/svc_tcp.c,v
retrieving revision 1.37
diff -u -p -r1.37 svc_tcp.c
--- lib/libc/rpc/svc_tcp.c 1 Nov 2015 03:45:29 -0000 1.37
+++ lib/libc/rpc/svc_tcp.c 10 Dec 2017 06:08:55 -0000
@@ -321,7 +321,7 @@ svctcp_destroy(SVCXPRT *xprt)
* All read operations timeout after 35 seconds.
* A timeout is fatal for the connection.
*/
-static struct timeval wait_per_try = { 35, 0 };
+static struct timespec wait_per_try = { 35, 0 };
/*
* reads data from the tcp conection.
@@ -332,31 +332,30 @@ static int
readtcp(SVCXPRT *xprt, caddr_t buf, int len)
{
int sock = xprt->xp_sock;
- int delta, nready;
- struct timeval start;
- struct timeval tmp1, tmp2;
+ int nready;
+ struct timespec start, delta, tmp1, tmp2;
struct pollfd pfd[1];
/*
* All read operations timeout after 35 seconds.
* A timeout is fatal for the connection.
*/
- delta = wait_per_try.tv_sec * 1000;
- gettimeofday(&start, NULL);
+ delta = wait_per_try;
+ clock_gettime(CLOCK_MONOTONIC, &start);
pfd[0].fd = sock;
pfd[0].events = POLLIN;
do {
- nready = poll(pfd, 1, delta);
+ nready = ppoll(pfd, 1, &delta, NULL);
switch (nready) {
case -1:
if (errno != EINTR)
goto fatal_err;
- gettimeofday(&tmp1, NULL);
- timersub(&tmp1, &start, &tmp2);
- timersub(&wait_per_try, &tmp2, &tmp1);
- if (tmp1.tv_sec < 0 || !timerisset(&tmp1))
+ clock_gettime(CLOCK_MONOTONIC, &tmp1);
+ timespecsub(&tmp1, &start, &tmp2);
+ timespecsub(&wait_per_try, &tmp2, &tmp1);
+ if (tmp1.tv_sec < 0 || !timespecisset(&tmp1))
goto fatal_err;
- delta = tmp1.tv_sec * 1000 + tmp1.tv_usec / 1000;
+ delta = tmp1;
continue;
case 0:
goto fatal_err;
Index: sys/sys/time.h
===================================================================
RCS file: /cvs/src/sys/sys/time.h,v
retrieving revision 1.36
diff -u -p -r1.36 time.h
--- sys/sys/time.h 12 Sep 2016 19:41:20 -0000 1.36
+++ sys/sys/time.h 10 Dec 2017 06:09:22 -0000
@@ -60,14 +60,14 @@ struct timespec {
};
#endif
-#define TIMEVAL_TO_TIMESPEC(tv, ts) {
\
+#define TIMEVAL_TO_TIMESPEC(tv, ts) do {
\
(ts)->tv_sec = (tv)->tv_sec; \
(ts)->tv_nsec = (tv)->tv_usec * 1000; \
-}
-#define TIMESPEC_TO_TIMEVAL(tv, ts) {
\
+} while (0)
+#define TIMESPEC_TO_TIMEVAL(tv, ts) do {
\
(tv)->tv_sec = (ts)->tv_sec; \
(tv)->tv_usec = (ts)->tv_nsec / 1000; \
-}
+} while (0)
struct timezone {
int tz_minuteswest; /* minutes west of Greenwich */
