On Thu, Jan 17, 2019 at 03:21:58PM -0700, Theo de Raadt wrote:
> - if (la_hold_total < LA_HOLD_TOTAL && la_hold_total < nmbclust / 64) {
> + if (la_hold_total < nmbclust / 64) {
>
> I have disagreed with claudio about this aspect of the diff.
>
> The refresh attempt is the crucial problem to fix, because of what it has
> done to tcp with slowstart etc.
>
> If refresh works, we may not need to increase the hold mbufs count at all.
>
> Secondly, I don't like how this is coupled to the MD nmbclust value.
> Some architectures have that very large, others very small, for other
> reasons. Bringing it into play here isn't right.
Updated diff that changes this limit back to LA_HOLD_TOTAL (100) but
removes the la_hold_total < nmbclust / 64 check.
--
:wq Claudio
Index: if_ether.c
===================================================================
RCS file: /cvs/src/sys/netinet/if_ether.c,v
retrieving revision 1.237
diff -u -p -r1.237 if_ether.c
--- if_ether.c 30 Nov 2018 09:27:56 -0000 1.237
+++ if_ether.c 17 Jan 2019 22:59:59 -0000
@@ -67,8 +67,9 @@
struct llinfo_arp {
LIST_ENTRY(llinfo_arp) la_list;
struct rtentry *la_rt; /* backpointer to rtentry */
- long la_asked; /* last time we QUERIED */
struct mbuf_list la_ml; /* packet hold queue */
+ time_t la_refreshed; /* when was refresh sent */
+ int la_asked; /* number of queries sent */
};
#define LA_HOLD_QUEUE 10
#define LA_HOLD_TOTAL 100
@@ -119,7 +120,7 @@ arptimer(void *arg)
LIST_FOREACH_SAFE(la, &arp_list, la_list, nla) {
struct rtentry *rt = la->la_rt;
- if (rt->rt_expire && rt->rt_expire <= time_uptime)
+ if (rt->rt_expire && rt->rt_expire < time_uptime)
arptfree(rt); /* timer has expired; clear */
}
NET_UNLOCK();
@@ -130,7 +131,6 @@ arp_rtrequest(struct ifnet *ifp, int req
{
struct sockaddr *gate = rt->rt_gateway;
struct llinfo_arp *la = (struct llinfo_arp *)rt->rt_llinfo;
- struct ifaddr *ifa;
if (!arpinit_done) {
static struct timeout arptimer_to;
@@ -140,7 +140,7 @@ arp_rtrequest(struct ifnet *ifp, int req
IPL_SOFTNET, 0, "arp", NULL);
timeout_set_proc(&arptimer_to, arptimer, &arptimer_to);
- timeout_add_sec(&arptimer_to, 1);
+ timeout_add_sec(&arptimer_to, arpt_prune);
}
if (ISSET(rt->rt_flags, RTF_GATEWAY|RTF_BROADCAST|RTF_MULTICAST))
@@ -149,17 +149,12 @@ arp_rtrequest(struct ifnet *ifp, int req
switch (req) {
case RTM_ADD:
- if (rt->rt_flags & RTF_CLONING ||
- ((rt->rt_flags & (RTF_LLINFO | RTF_LOCAL)) && !la)) {
- /*
- * Give this route an expiration time, even though
- * it's a "permanent" route, so that routes cloned
- * from it do not need their expiration time set.
- */
- rt->rt_expire = time_uptime;
- if ((rt->rt_flags & RTF_CLONING) != 0)
- break;
+ if (rt->rt_flags & RTF_CLONING) {
+ rt->rt_expire = 0;
+ break;
}
+ if ((rt->rt_flags & RTF_LOCAL) && !la)
+ rt->rt_expire = 0;
/*
* Announce a new entry if requested or warn the user
* if another station has this IP address.
@@ -179,7 +174,7 @@ arp_rtrequest(struct ifnet *ifp, int req
}
satosdl(gate)->sdl_type = ifp->if_type;
satosdl(gate)->sdl_index = ifp->if_index;
- if (la != 0)
+ if (la != NULL)
break; /* This happens on a route change */
/*
* Case 2: This route may come from cloning, or a manual route
@@ -195,18 +190,9 @@ arp_rtrequest(struct ifnet *ifp, int req
ml_init(&la->la_ml);
la->la_rt = rt;
rt->rt_flags |= RTF_LLINFO;
+ if ((rt->rt_flags & RTF_LOCAL) == 0)
+ rt->rt_expire = time_uptime;
LIST_INSERT_HEAD(&arp_list, la, la_list);
-
- TAILQ_FOREACH(ifa, &ifp->if_addrlist, ifa_list) {
- if ((ifa->ifa_addr->sa_family == AF_INET) &&
- ifatoia(ifa)->ia_addr.sin_addr.s_addr ==
- satosin(rt_key(rt))->sin_addr.s_addr)
- break;
- }
- if (ifa) {
- KASSERT(ifa == rt->rt_ifa);
- rt->rt_expire = 0;
- }
break;
case RTM_DELETE:
@@ -314,7 +300,7 @@ arpresolve(struct ifnet *ifp, struct rte
struct sockaddr *dst, u_char *desten)
{
struct arpcom *ac = (struct arpcom *)ifp;
- struct llinfo_arp *la = NULL;
+ struct llinfo_arp *la;
struct sockaddr_dl *sdl;
struct rtentry *rt = NULL;
char addr[INET_ADDRSTRLEN];
@@ -331,7 +317,7 @@ arpresolve(struct ifnet *ifp, struct rte
rt = rt_getll(rt0);
if (ISSET(rt->rt_flags, RTF_REJECT) &&
- (rt->rt_expire == 0 || time_uptime < rt->rt_expire)) {
+ (rt->rt_expire == 0 || rt->rt_expire > time_uptime )) {
m_freem(m);
return (rt == rt0 ? EHOSTDOWN : EHOSTUNREACH);
}
@@ -352,6 +338,9 @@ arpresolve(struct ifnet *ifp, struct rte
goto bad;
}
+ la = (struct llinfo_arp *)rt->rt_llinfo;
+ KASSERT(la != NULL);
+
/*
* Check the address family and length is valid, the address
* is resolved; otherwise, try to resolve.
@@ -359,6 +348,17 @@ arpresolve(struct ifnet *ifp, struct rte
if ((rt->rt_expire == 0 || rt->rt_expire > time_uptime) &&
sdl->sdl_family == AF_LINK && sdl->sdl_alen != 0) {
memcpy(desten, LLADDR(sdl), sdl->sdl_alen);
+
+ /* refresh ARP entry when timeout gets close */
+ if (rt->rt_expire != 0 &&
+ rt->rt_expire - arpt_keep / 8 < time_uptime &&
+ la->la_refreshed + 30 < time_uptime) {
+ la->la_refreshed = time_uptime;
+ arprequest(ifp,
+ &satosin(rt->rt_ifa->ifa_addr)->sin_addr.s_addr,
+ &satosin(dst)->sin_addr.s_addr,
+ ac->ac_enaddr);
+ }
return (0);
}
@@ -370,9 +370,7 @@ arpresolve(struct ifnet *ifp, struct rte
* response yet. Insert mbuf in hold queue if below limit
* if above the limit free the queue without queuing the new packet.
*/
- la = (struct llinfo_arp *)rt->rt_llinfo;
- KASSERT(la != NULL);
- if (la_hold_total < LA_HOLD_TOTAL && la_hold_total < nmbclust / 64) {
+ if (la_hold_total < LA_HOLD_TOTAL) {
struct mbuf *mh;
if (ml_len(&la->la_ml) >= LA_HOLD_QUEUE) {
@@ -411,6 +409,7 @@ arpresolve(struct ifnet *ifp, struct rte
rt->rt_flags |= RTF_REJECT;
rt->rt_expire += arpt_down;
la->la_asked = 0;
+ la->la_refreshed = 0;
la_hold_total -= ml_purge(&la->la_ml);
}
}
@@ -668,6 +667,7 @@ arpcache(struct ifnet *ifp, struct ether
}
la->la_asked = 0;
+ la->la_refreshed = 0;
while ((len = ml_len(&la->la_ml)) != 0) {
struct mbuf *mh;
