> On Aug 7, 2021, at 1:00 PM, Dai Ngo <dai.ngo@xxxxxxxxxx> wrote:
>
> Currently svc_run does not handle poll time and rendezvous_request
> does not handle EMFILE error returned from accept(2 as it used to.
> These two missing functionality were removed by commit b2c9430f46c4.
>
> The effect of not handling poll timeout allows idle TCP conections
> to remain ESTABLISHED indefinitely. When the number of connections
> reaches the limit of the open file descriptors (ulimit -n) then
> accept(2) fails with EMFILE. Since there is no handling of EMFILE
> error this causes svc_run() to get in a tight loop calling accept(2).
> This resulting in the RPC service of svc_run is being down, it's
> no longer able to service any requests.
>
> The below script, td.sh, with nc (nmap-ncat-7.80-3) can be used
> to take down the RPC service:
>
> if [ $# -ne 3 ]; then
> echo "$0: server dst_port conn_cnt"
> exit
> fi
> server=$1
> dport=$2
> conn_cnt=$3
> echo "dport[$dport] server[$server] conn_cnt[$conn_cnt]"
>
> pcnt=0
> while [ $pcnt -lt $conn_cnt ]
> do
> nc -v --recv-only $server $dport &
> pcnt=`expr $pcnt + 1`
> done
>
> Fix by restoring code in svc_run to cleanup idle conncetions when
> poll(2) returns 0 (timeout) and in rendezvous_request to handle
> EMFILE error returned from accept(2).
>
> Fixes: b2c9430f46c4 Use poll() instead of select() in svc_run()
> Signed-off-by: dai.ngo@xxxxxxxxxx
> ---
> src/libtirpc.map | 2 +-
> src/rpc_com.h | 2 ++
> src/svc.c | 2 +-
> src/svc_run.c | 2 ++
> src/svc_vc.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
> 5 files changed, 54 insertions(+), 2 deletions(-)
>
> diff --git a/src/libtirpc.map b/src/libtirpc.map
> index 21d60651ff57..b754110faadb 100644
> --- a/src/libtirpc.map
> +++ b/src/libtirpc.map
> @@ -331,5 +331,5 @@ TIRPC_PRIVATE {
> global:
> __libc_clntudp_bufcreate;
> # private, but used by rpcbind:
> - __svc_clean_idle; svc_auth_none; libtirpc_set_debug;
> + __svc_destroy_idle; __svc_clean_idle; svc_auth_none; libtirpc_set_debug;
As I stated yesterday, as a reviewer I agree with everything
in this patch except for the addition of __svc_destroy_idle
to the library's private API list.
IMO the private __svc_clean_idle() API was a mistake we
shouldn't make again. (That might be why Thorsten removed
it years ago).
Thanks, Dai, for your work nailing this down!
> };
> diff --git a/src/rpc_com.h b/src/rpc_com.h
> index 76badefcfe90..ede6ec8b1d4e 100644
> --- a/src/rpc_com.h
> +++ b/src/rpc_com.h
> @@ -55,6 +55,7 @@ struct netbuf *__rpcb_findaddr_timed(rpcprog_t, rpcvers_t,
> bool_t __rpc_control(int,void *);
>
> bool_t __svc_clean_idle(fd_set *, int, bool_t);
> +void __svc_destroy_idle(int, bool_t);
> bool_t __xdrrec_setnonblock(XDR *, int);
> bool_t __xdrrec_getrec(XDR *, enum xprt_stat *, bool_t);
> void __xprt_unregister_unlocked(SVCXPRT *);
> @@ -62,6 +63,7 @@ void __xprt_set_raddr(SVCXPRT *, const struct sockaddr_storage *);
>
>
> extern int __svc_maxrec;
> +extern SVCXPRT **__svc_xports;
>
> #ifdef __cplusplus
> }
> diff --git a/src/svc.c b/src/svc.c
> index 6db164bbd76b..aa0c92591914 100644
> --- a/src/svc.c
> +++ b/src/svc.c
> @@ -57,7 +57,7 @@
>
> #define max(a, b) (a > b ? a : b)
>
> -static SVCXPRT **__svc_xports;
> +SVCXPRT **__svc_xports;
> int __svc_maxrec;
>
> /*
> diff --git a/src/svc_run.c b/src/svc_run.c
> index f40314b9948e..4eba36174524 100644
> --- a/src/svc_run.c
> +++ b/src/svc_run.c
> @@ -44,6 +44,7 @@
> #include "rpc_com.h"
> #include <sys/select.h>
>
> +
> void
> svc_run()
> {
> @@ -86,6 +87,7 @@ svc_run()
> warn ("svc_run: - poll failed");
> break;
> case 0:
> + __svc_destroy_idle(30, FALSE);
> continue;
> default:
> svc_getreq_poll (my_pollfd, i);
> diff --git a/src/svc_vc.c b/src/svc_vc.c
> index f1d9f001fcdc..4880ab5dbc26 100644
> --- a/src/svc_vc.c
> +++ b/src/svc_vc.c
> @@ -58,6 +58,7 @@
>
> #include <rpc/rpc.h>
>
> +#include "debug.h"
> #include "rpc_com.h"
>
> #include <getpeereid.h>
> @@ -337,6 +338,10 @@ again:
> if (sock < 0) {
> if (errno == EINTR)
> goto again;
> + if (errno == EMFILE || errno == ENFILE) {
> + /* remove least active fd */
> + __svc_destroy_idle(0, FALSE);
> + }
> return (FALSE);
> }
> /*
> @@ -820,3 +825,46 @@ __svc_clean_idle(fd_set *fds, int timeout, bool_t cleanblock)
> {
> return FALSE;
> }
> +
> +void
> +__svc_destroy_idle(int timeout, bool_t cleanblock)
> +{
> + int i;
> + SVCXPRT *xprt, *least_active;
> + struct timeval tv, tdiff, tmax;
> + struct cf_conn *cd;
> +
> + gettimeofday(&tv, NULL);
> + tmax.tv_sec = tmax.tv_usec = 0;
> + least_active = NULL;
> + rwlock_wrlock(&svc_fd_lock);
> +
> + for (i = 0; i <= svc_max_pollfd; i++) {
> + if (svc_pollfd[i].fd == -1)
> + continue;
> + xprt = __svc_xports[i];
> + if (xprt == NULL || xprt->xp_ops == NULL ||
> + xprt->xp_ops->xp_recv != svc_vc_recv)
> + continue;
> + cd = (struct cf_conn *)xprt->xp_p1;
> + if (!cleanblock && !cd->nonblock)
> + continue;
> + if (timeout == 0) {
> + timersub(&tv, &cd->last_recv_time, &tdiff);
> + if (timercmp(&tdiff, &tmax, >)) {
> + tmax = tdiff;
> + least_active = xprt;
> + }
> + continue;
> + }
> + if (tv.tv_sec - cd->last_recv_time.tv_sec > timeout) {
> + __xprt_unregister_unlocked(xprt);
> + __svc_vc_dodestroy(xprt);
> + }
> + }
> + if (timeout == 0 && least_active != NULL) {
> + __xprt_unregister_unlocked(least_active);
> + __svc_vc_dodestroy(least_active);
> + }
> + rwlock_unlock(&svc_fd_lock);
> +}
> --
> 2.26.2
--
Chuck Lever
