Re: route timer queues
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 - 1.404
> +++ net/route.c 19 Apr 2022 20:31:49 -
> @@ -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 - 1.188
> +++ net/route.h 19 Apr 2022 20:31:49 -
> @@ -411,10 +411,10 @@ struct rttimer {
> };
>
> struct rttimer_queue {
> - longrtq_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 @@ intrt_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 *)
route timer queues
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?
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 - 1.404
+++ net/route.c 19 Apr 2022 20:31:49 -
@@ -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 - 1.188
+++ net/route.h 19 Apr 2022 20:31:49 -
@@ -411,10 +411,10 @@ struct rttimer {
};
struct rttimer_queue {
- longrtq_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);
voidrt_timer_remove_all(struct rtentry *);
-struct rttimer_queue *rt_timer_queue_create(u_int);
-voidrt_timer_queue_change(struct rttimer_queue *, long);
+struct rttimer_queue *rt_timer_queue_create(int);
+voidrt_timer_queue_change(struct rttimer_queue *, int);
voidrt_timer_queue_destroy(struct rttimer_queue *);
unsigned long rt_timer_queue_count(struct rttimer_queue *);
voidrt_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 - 1.187
+++ netinet/ip_icmp.c 19 Apr 2022 20:31:50 -
@@ -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
-
