On Tue, Apr 19, 2022 at 10:49:44PM +0200, Alexander Bluhm wrote:
> Hi,
>
> I had a look in route timer queues in netinet and netinet6 and found
> some inconsistencies.
>
> - Timeout was a mixture of int, u_int and long. Make timeout
> int with sysctl bounds checking and make absolute time time_t.
>
> - Some code assumes that ..._timeout_q can be NULL and at some
> places this is checked. Better make sure that all queues always
> exist. The pool_get is only called from initialization and from
> syscall, so PR_WAITOK is possible.
>
> - The only special hack I kept is when ip_mtudisc is set to 0.
> Then I destroy the queue and generate an empty one.
>
> - If redirect timeout is 0, it does not time out. Adopt IPv6 to
> behavior of IPv4.
>
> - sysctl net.inet6.icmp6.redirtimeout had no effect as the queue
> timeout was not modified. Make icmp6_sysctl() look like
> icmp_sysctl().
>
> ok?
Looks good to me. OK claudio@
Btw. the rt_timer implementation of rt_timer_queue_change() has a bug.
Actually it is rt_timer_timer() that has the bug. It assumes that the
TAILQ of all rttimers on a queue are sorted but that is not true if the
timeout is lowered in which case objects may live longer then expected.
There are other things in the rt_timer implementation that is questionable
like how rt_expire is changed.
> bluhm
>
> Index: net/route.c
> ===================================================================
> RCS file: /cvs/src/sys/net/route.c,v
> retrieving revision 1.404
> diff -u -p -r1.404 route.c
> --- net/route.c 19 Apr 2022 19:19:31 -0000 1.404
> +++ net/route.c 19 Apr 2022 20:31:49 -0000
> @@ -1399,13 +1399,11 @@ rt_timer_init(void)
> }
>
> struct rttimer_queue *
> -rt_timer_queue_create(u_int timeout)
> +rt_timer_queue_create(int timeout)
> {
> struct rttimer_queue *rtq;
>
> - rtq = pool_get(&rttimer_queue_pool, PR_NOWAIT | PR_ZERO);
> - if (rtq == NULL)
> - return (NULL);
> + rtq = pool_get(&rttimer_queue_pool, PR_WAITOK | PR_ZERO);
>
> rtq->rtq_timeout = timeout;
> rtq->rtq_count = 0;
> @@ -1416,7 +1414,7 @@ rt_timer_queue_create(u_int timeout)
> }
>
> void
> -rt_timer_queue_change(struct rttimer_queue *rtq, long timeout)
> +rt_timer_queue_change(struct rttimer_queue *rtq, int timeout)
> {
> rtq->rtq_timeout = timeout;
> }
> @@ -1470,10 +1468,10 @@ rt_timer_add(struct rtentry *rt, void (*
> struct rttimer *), struct rttimer_queue *queue, u_int rtableid)
> {
> struct rttimer *r;
> - long current_time;
> + time_t current_time;
>
> current_time = getuptime();
> - rt->rt_expire = getuptime() + queue->rtq_timeout;
> + rt->rt_expire = current_time + queue->rtq_timeout;
>
> /*
> * If there's already a timer with this action, destroy it before
> @@ -1514,7 +1512,7 @@ rt_timer_timer(void *arg)
> struct timeout *to = (struct timeout *)arg;
> struct rttimer_queue *rtq;
> struct rttimer *r;
> - long current_time;
> + time_t current_time;
>
> current_time = getuptime();
>
> Index: net/route.h
> ===================================================================
> RCS file: /cvs/src/sys/net/route.h,v
> retrieving revision 1.188
> diff -u -p -r1.188 route.h
> --- net/route.h 19 Apr 2022 15:44:56 -0000 1.188
> +++ net/route.h 19 Apr 2022 20:31:49 -0000
> @@ -411,10 +411,10 @@ struct rttimer {
> };
>
> struct rttimer_queue {
> - long rtq_timeout;
> - unsigned long rtq_count;
> TAILQ_HEAD(, rttimer) rtq_head;
> LIST_ENTRY(rttimer_queue) rtq_link;
> + unsigned long rtq_count;
> + int rtq_timeout;
> };
>
> const char *rtlabel_id2name(u_int16_t);
> @@ -456,8 +456,8 @@ int rt_timer_add(struct rtentry *,
> void(*)(struct rtentry *, struct rttimer *),
> struct rttimer_queue *, u_int);
> void rt_timer_remove_all(struct rtentry *);
> -struct rttimer_queue *rt_timer_queue_create(u_int);
> -void rt_timer_queue_change(struct rttimer_queue *, long);
> +struct rttimer_queue *rt_timer_queue_create(int);
> +void rt_timer_queue_change(struct rttimer_queue *, int);
> void rt_timer_queue_destroy(struct rttimer_queue *);
> unsigned long rt_timer_queue_count(struct rttimer_queue *);
> void rt_timer_timer(void *);
> Index: netinet/ip_icmp.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet/ip_icmp.c,v
> retrieving revision 1.187
> diff -u -p -r1.187 ip_icmp.c
> --- netinet/ip_icmp.c 26 Jul 2021 20:44:44 -0000 1.187
> +++ netinet/ip_icmp.c 19 Apr 2022 20:31:50 -0000
> @@ -120,7 +120,7 @@ int icmp_redirtimeout = 10 * 60;
> static int icmperrpps_count = 0;
> static struct timeval icmperrppslim_last;
>
> -static struct rttimer_queue *icmp_redirect_timeout_q = NULL;
> +struct rttimer_queue *icmp_redirect_timeout_q;
> struct cpumem *icmpcounters;
>
> const struct sysctl_bounded_args icmpctl_vars[] = {
> @@ -141,15 +141,8 @@ int icmp_sysctl_icmpstat(void *, size_t
> void
> icmp_init(void)
> {
> + icmp_redirect_timeout_q = rt_timer_queue_create(icmp_redirtimeout);
> icmpcounters = counters_alloc(icps_ncounters);
> - /*
> - * This is only useful if the user initializes redirtimeout to
> - * something other than zero.
> - */
> - if (icmp_redirtimeout != 0) {
> - icmp_redirect_timeout_q =
> - rt_timer_queue_create(icmp_redirtimeout);
> - }
> }
>
> struct mbuf *
> @@ -640,12 +633,11 @@ reflect:
> #endif
> rtredirect(sintosa(&sdst), sintosa(&sgw),
> sintosa(&ssrc), &newrt, m->m_pkthdr.ph_rtableid);
> - if (newrt != NULL && icmp_redirtimeout != 0) {
> - (void)rt_timer_add(newrt, icmp_redirect_timeout,
> + if (newrt != NULL && icmp_redirtimeout > 0) {
> + rt_timer_add(newrt, icmp_redirect_timeout,
> icmp_redirect_timeout_q, m->m_pkthdr.ph_rtableid);
> }
> - if (newrt != NULL)
> - rtfree(newrt);
> + rtfree(newrt);
> pfctlinput(PRC_REDIRECT_HOST, sintosa(&sdst));
> break;
> }
> @@ -889,21 +881,11 @@ icmp_sysctl(int *name, u_int namelen, vo
>
> switch (name[0]) {
> case ICMPCTL_REDIRTIMEOUT:
> -
> NET_LOCK();
> - error = sysctl_int(oldp, oldlenp, newp, newlen,
> - &icmp_redirtimeout);
> - if (icmp_redirect_timeout_q != NULL) {
> - if (icmp_redirtimeout == 0) {
> - rt_timer_queue_destroy(icmp_redirect_timeout_q);
> - icmp_redirect_timeout_q = NULL;
> - } else
> - rt_timer_queue_change(icmp_redirect_timeout_q,
> - icmp_redirtimeout);
> - } else if (icmp_redirtimeout > 0) {
> - icmp_redirect_timeout_q =
> - rt_timer_queue_create(icmp_redirtimeout);
> - }
> + error = sysctl_int_bounded(oldp, oldlenp, newp, newlen,
> + &icmp_redirtimeout, 0, INT_MAX);
> + rt_timer_queue_change(icmp_redirect_timeout_q,
> + icmp_redirtimeout);
> NET_UNLOCK();
> break;
>
> Index: netinet/ip_input.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet/ip_input.c,v
> retrieving revision 1.366
> diff -u -p -r1.366 ip_input.c
> --- netinet/ip_input.c 22 Feb 2022 01:35:40 -0000 1.366
> +++ netinet/ip_input.c 19 Apr 2022 20:31:50 -0000
> @@ -91,10 +91,10 @@ int ipsendredirects = 1;
> int ip_dosourceroute = 0;
> int ip_defttl = IPDEFTTL;
> int ip_mtudisc = 1;
> -u_int ip_mtudisc_timeout = IPMTUDISCTIMEOUT;
> +int ip_mtudisc_timeout = IPMTUDISCTIMEOUT;
> int ip_directedbcast = 0;
>
> -struct rttimer_queue *ip_mtudisc_timeout_q = NULL;
> +struct rttimer_queue *ip_mtudisc_timeout_q;
>
> /* Protects `ipq' and `ip_frags'. */
> struct mutex ipq_mutex = MUTEX_INITIALIZER(IPL_SOFTNET);
> @@ -201,9 +201,7 @@ ip_init(void)
> pr->pr_protocol < IPPROTO_MAX)
> ip_protox[pr->pr_protocol] = pr - inetsw;
> LIST_INIT(&ipq);
> - if (ip_mtudisc != 0)
> - ip_mtudisc_timeout_q =
> - rt_timer_queue_create(ip_mtudisc_timeout);
> + ip_mtudisc_timeout_q = rt_timer_queue_create(ip_mtudisc_timeout);
>
> /* Fill in list of ports not to allocate dynamically. */
> memset(&baddynamicports, 0, sizeof(baddynamicports));
> @@ -1616,24 +1614,20 @@ ip_sysctl(int *name, u_int namelen, void
> return (error);
> case IPCTL_MTUDISC:
> NET_LOCK();
> - error = sysctl_int(oldp, oldlenp, newp, newlen,
> - &ip_mtudisc);
> - if (ip_mtudisc != 0 && ip_mtudisc_timeout_q == NULL) {
> + error = sysctl_int(oldp, oldlenp, newp, newlen, &ip_mtudisc);
> + if (ip_mtudisc == 0) {
> + rt_timer_queue_destroy(ip_mtudisc_timeout_q);
> ip_mtudisc_timeout_q =
> rt_timer_queue_create(ip_mtudisc_timeout);
> - } else if (ip_mtudisc == 0 && ip_mtudisc_timeout_q != NULL) {
> - rt_timer_queue_destroy(ip_mtudisc_timeout_q);
> - ip_mtudisc_timeout_q = NULL;
> }
> NET_UNLOCK();
> return error;
> case IPCTL_MTUDISCTIMEOUT:
> NET_LOCK();
> - error = sysctl_int(oldp, oldlenp, newp, newlen,
> - &ip_mtudisc_timeout);
> - if (ip_mtudisc_timeout_q != NULL)
> - rt_timer_queue_change(ip_mtudisc_timeout_q,
> - ip_mtudisc_timeout);
> + error = sysctl_int_bounded(oldp, oldlenp, newp, newlen,
> + &ip_mtudisc_timeout, 0, INT_MAX);
> + rt_timer_queue_change(ip_mtudisc_timeout_q,
> + ip_mtudisc_timeout);
> NET_UNLOCK();
> return (error);
> #ifdef IPSEC
> Index: netinet/ip_var.h
> ===================================================================
> RCS file: /cvs/src/sys/netinet/ip_var.h,v
> retrieving revision 1.90
> diff -u -p -r1.90 ip_var.h
> --- netinet/ip_var.h 25 Feb 2022 23:51:03 -0000 1.90
> +++ netinet/ip_var.h 19 Apr 2022 20:31:50 -0000
> @@ -204,7 +204,7 @@ extern int ip_defttl; /* default IP tt
> #define IPMTUDISCTIMEOUT (10 * 60) /* as per RFC 1191 */
>
> extern int ip_mtudisc; /* mtu discovery */
> -extern u_int ip_mtudisc_timeout; /* seconds to timeout mtu discovery */
> +extern int ip_mtudisc_timeout; /* seconds to timeout mtu
> discovery */
>
> extern int ipport_firstauto; /* min port for port allocation */
> extern int ipport_lastauto; /* max port for port allocation */
> Index: netinet6/icmp6.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet6/icmp6.c,v
> retrieving revision 1.238
> diff -u -p -r1.238 icmp6.c
> --- netinet6/icmp6.c 22 Feb 2022 01:15:02 -0000 1.238
> +++ netinet6/icmp6.c 19 Apr 2022 20:31:52 -0000
> @@ -118,7 +118,7 @@ struct icmp6_mtudisc_callback {
> LIST_HEAD(, icmp6_mtudisc_callback) icmp6_mtudisc_callbacks =
> LIST_HEAD_INITIALIZER(icmp6_mtudisc_callbacks);
>
> -struct rttimer_queue *icmp6_mtudisc_timeout_q = NULL;
> +struct rttimer_queue *icmp6_mtudisc_timeout_q;
>
> /* XXX do these values make any sense? */
> static int icmp6_mtudisc_hiwat = 1280;
> @@ -127,7 +127,7 @@ static int icmp6_mtudisc_lowat = 256;
> /*
> * keep track of # of redirect routes.
> */
> -static struct rttimer_queue *icmp6_redirect_timeout_q = NULL;
> +struct rttimer_queue *icmp6_redirect_timeout_q;
>
> /* XXX experimental, turned off */
> static int icmp6_redirect_lowat = -1;
> @@ -1404,12 +1404,11 @@ icmp6_redirect_input(struct mbuf *m, int
> memcpy(&ssrc.sin6_addr, &src6, sizeof(struct in6_addr));
> rtredirect(sin6tosa(&sdst), sin6tosa(&sgw), sin6tosa(&ssrc),
> &newrt, m->m_pkthdr.ph_rtableid);
> -
> - if (newrt) {
> - (void)rt_timer_add(newrt, icmp6_redirect_timeout,
> + if (newrt != NULL && icmp6_redirtimeout > 0) {
> + rt_timer_add(newrt, icmp6_redirect_timeout,
> icmp6_redirect_timeout_q, m->m_pkthdr.ph_rtableid);
> - rtfree(newrt);
> }
> + rtfree(newrt);
> }
> /* finally update cached route in each socket via pfctlinput */
> {
> @@ -1881,7 +1880,6 @@ icmp6_redirect_timeout(struct rtentry *r
> }
>
> const struct sysctl_bounded_args icmpv6ctl_vars[] = {
> - { ICMPV6CTL_REDIRTIMEOUT, &icmp6_redirtimeout, 0, INT_MAX },
> { ICMPV6CTL_ND6_DELAY, &nd6_delay, 0, INT_MAX },
> { ICMPV6CTL_ND6_UMAXTRIES, &nd6_umaxtries, 0, INT_MAX },
> { ICMPV6CTL_ND6_MMAXTRIES, &nd6_mmaxtries, 0, INT_MAX },
> @@ -1916,19 +1914,30 @@ icmp6_sysctl(int *name, u_int namelen, v
>
> /* All sysctl names at this level are terminal. */
> if (namelen != 1)
> - return ENOTDIR;
> + return (ENOTDIR);
>
> switch (name[0]) {
> + case ICMPV6CTL_REDIRTIMEOUT:
> + NET_LOCK();
> + error = sysctl_int_bounded(oldp, oldlenp, newp, newlen,
> + &icmp6_redirtimeout, 0, INT_MAX);
> + rt_timer_queue_change(icmp6_redirect_timeout_q,
> + icmp6_redirtimeout);
> + NET_UNLOCK();
> + break;
>
> case ICMPV6CTL_STATS:
> - return icmp6_sysctl_icmp6stat(oldp, oldlenp, newp);
> + error = icmp6_sysctl_icmp6stat(oldp, oldlenp, newp);
> + break;
> +
> default:
> NET_LOCK();
> error = sysctl_bounded_arr(icmpv6ctl_vars,
> nitems(icmpv6ctl_vars), name, namelen, oldp, oldlenp, newp,
> newlen);
> NET_UNLOCK();
> - return (error);
> + break;
> }
> - /* NOTREACHED */
> +
> + return (error);
> }
> Index: netinet6/ip6_input.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet6/ip6_input.c,v
> retrieving revision 1.240
> diff -u -p -r1.240 ip6_input.c
> --- netinet6/ip6_input.c 22 Feb 2022 01:35:41 -0000 1.240
> +++ netinet6/ip6_input.c 19 Apr 2022 20:31:52 -0000
> @@ -1454,11 +1454,10 @@ ip6_sysctl(int *name, u_int namelen, voi
> #endif
> case IPV6CTL_MTUDISCTIMEOUT:
> NET_LOCK();
> - error = sysctl_int(oldp, oldlenp, newp, newlen,
> - &ip6_mtudisc_timeout);
> - if (icmp6_mtudisc_timeout_q != NULL)
> - rt_timer_queue_change(icmp6_mtudisc_timeout_q,
> - ip6_mtudisc_timeout);
> + error = sysctl_int_bounded(oldp, oldlenp, newp, newlen,
> + &ip6_mtudisc_timeout, 0, INT_MAX);
> + rt_timer_queue_change(icmp6_mtudisc_timeout_q,
> + ip6_mtudisc_timeout);
> NET_UNLOCK();
> return (error);
> case IPV6CTL_IFQUEUE:
>
--
:wq Claudio
