In case an upper layer protocol tries to send a packet but the

L2 code does not have the ethernet address for the destination
within the broadcast domain in the table, we remember the
original mbuf in `la_hold' in arpresolve() and send out a
different packet with an arp request.
In case there will be more upper layer packets to send we will
free an earlier one held in `la_hold' and queue the new one.

Once we get a packet in, with which we can perfect our arp table
entry we send out the original 'on hold' packet, should there
be any.
Rather than continuing to process the packet that we received,
we returned without freeing the packet that came in, which
basically means that we leaked an mbuf for every arp request
we sent.

Rather than freeing the received packet and returning, continue
to process the incoming arp packet as well.
This should (a) improve some setups, also proxy-arp, in case it was an
incoming arp request and (b) resembles the behaviour FreeBSD had
from day 1, which alignes with RFC826 "Packet reception" (merge case).

Rename 'm0' to 'hold' to make the code more understandable as
well as diffable to earlier versions more easily.

Handle the link-layer entry 'la' lock comepletely in the block
where needed and release it as early as possible, rather than
holding it longer, down to the end of the function.

Found by:			pointyhat, ns1
Bug hunting session with:	erwin, simon, rwatson
Tested by:			simon on cluster machines
Reviewed by:			ratson, kmacy, julian
MFC after:			3 days

1 files changed, 10 insertions, 12 deletions

diff --git a/sys/netinet/if_ether.c b/sys/netinet/if_ether.c
index 9d1c13a..e118cec 100644
--- a/sys/netinet/if_ether.c
+++ b/sys/netinet/if_ether.c
@@ -462,11 +462,11 @@ in_arpinput(struct mbuf *m)
 	struct rtentry *rt;
 	struct ifaddr *ifa;
 	struct in_ifaddr *ia;
+	struct mbuf *hold;
 	struct sockaddr sa;
 	struct in_addr isaddr, itaddr, myaddr;
 	u_int8_t *enaddr = NULL;
 	int op, flags;
-	struct mbuf *m0;
 	int req_len;
 	int bridged = 0, is_bridge = 0;
 #ifdef DEV_CARP
@@ -631,11 +631,13 @@ match:
 				    la->lle_tbl->llt_ifp->if_xname,
 				    ifp->if_addrlen, (u_char *)ar_sha(ah), ":",
 				    ifp->if_xname);
+			LLE_WUNLOCK(la);
 			goto reply;
 		}
 		if ((la->la_flags & LLE_VALID) &&
 		    bcmp(ar_sha(ah), &la->ll_addr, ifp->if_addrlen)) {
 			if (la->la_flags & LLE_STATIC) {
+				LLE_WUNLOCK(la);
 				log(LOG_ERR,
 				    "arp: %*D attempts to modify permanent "
 				    "entry for %s on %s\n",
@@ -655,6 +657,7 @@ match:
 		}
 		    
 		if (ifp->if_addrlen != ah->ar_hln) {
+			LLE_WUNLOCK(la);
 			log(LOG_WARNING,
 			    "arp from %*D: addr len: new %d, i/f %d (ignored)",
 			    ifp->if_addrlen, (u_char *) ar_sha(ah), ":",
@@ -671,15 +674,14 @@ match:
 		}
 		la->la_asked = 0;
 		la->la_preempt = V_arp_maxtries;
-		if (la->la_hold != NULL) {
-			m0 = la->la_hold;
-			la->la_hold = 0;
+		hold = la->la_hold;
+		if (hold != NULL) {
+			la->la_hold = NULL;
 			memcpy(&sa, L3_ADDR(la), sizeof(sa));
-			LLE_WUNLOCK(la);
-			
-			(*ifp->if_output)(ifp, m0, &sa, NULL);
-			return;
 		}
+		LLE_WUNLOCK(la);
+		if (hold != NULL)
+			(*ifp->if_output)(ifp, hold, &sa, NULL);
 	}
 reply:
 	if (op != ARPOP_REQUEST)
@@ -750,8 +752,6 @@ reply:
 #endif
 	}
 
-	if (la != NULL)
-		LLE_WUNLOCK(la);
 	if (itaddr.s_addr == myaddr.s_addr &&
 	    IN_LINKLOCAL(ntohl(itaddr.s_addr))) {
 		/* RFC 3927 link-local IPv4; always reply by broadcast. */
@@ -777,8 +777,6 @@ reply:
 	return;
 
 drop:
-	if (la != NULL)
-		LLE_WUNLOCK(la);
 	m_freem(m);
 }
 #endif