On Sun, Dec 10 2017, Scott Cheloha <[email protected]> wrote:
> 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.
Well, afaik upstream can be considered dead, and looking at cvs log
people have implemented a bunch of changes and improvements to these
files, so I think that further improvements are welcome.
> 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.
(The sys/time.h change has since been committed.)
> This is probably not adequately tested, though it compiles
> and I'm not seeing any issues in my (small) NFS setup.
>
> Thoughts and feedback?
I'll schedule some time for a review tomorrow, more eyes would be
welcome.
> --
> 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 */
>
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
