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 */