eugen_grosbein.net created this revision.
Herald added a subscriber: imp.
REVISION SUMMARY
If a system has dynamic ever changing set of IP addresses, it can panic due
to unprotected access to global INADDR_HASH in several kernel subsystems:
ip_input.c, in_mcast.c, if_stf(4).
There was also similar problem in ip_fw2.c fixed recently in head by ae@.
kgdb showed "(struct in_ifaddr *) 0xdeadc0dedeadc0de" for a panic in
INVARIANTS-enabled kernel (PR #220078).
I could provoke such panic in ipfw with my stress-test for net/mpd5
creating/deleting hundreds of ngXXX interfaces having distinct IP addresses.
Proposed change makes use of existing IN_IFADDR_[RW]LOCK macros to protect
access to the set as they are already used in the ip_icmp.c, if_ether.c and
in.c.
TEST PLAN
The nature of the race makes it hard to reproduce the bug, but it can happen
using high loaded (or stress-tested) net/mpd5 installation.
1. Apply the patch.
2. Rebuild a kernel.
3. Run busy mpd5 server with hundreds of users reconnecting very often.
REPOSITORY
rS FreeBSD src repository
REVISION DETAIL
https://reviews.freebsd.org/D12457
AFFECTED FILES
sys/net/if_stf.c
sys/netinet/in_mcast.c
sys/netinet/ip_input.c
EMAIL PREFERENCES
https://reviews.freebsd.org/settings/panel/emailpreferences/
To: eugen_grosbein.net, ae, avg, mav, rwatson
Cc: imp, freebsd-net-list
diff --git a/sys/net/if_stf.c.orig b/sys/net/if_stf.c
--- a/sys/net/if_stf.c.orig
+++ b/sys/net/if_stf.c
@@ -360,6 +360,7 @@
static int
stf_getsrcifa6(struct ifnet *ifp, struct in6_addr *addr, struct in6_addr *mask)
{
+ struct rm_priotracker in_ifa_tracker;
struct ifaddr *ia;
struct in_ifaddr *ia4;
struct in6_ifaddr *ia6;
@@ -375,9 +376,11 @@
continue;
bcopy(GET_V4(&sin6->sin6_addr), &in, sizeof(in));
+ IN_IFADDR_RLOCK(&in_ifa_tracker);
LIST_FOREACH(ia4, INADDR_HASH(in.s_addr), ia_hash)
if (ia4->ia_addr.sin_addr.s_addr == in.s_addr)
break;
+ IN_IFADDR_RUNLOCK(&in_ifa_tracker);
if (ia4 == NULL)
continue;
diff --git a/sys/netinet/in_mcast.c.orig b/sys/netinet/in_mcast.c
--- a/sys/netinet/in_mcast.c.orig
+++ b/sys/netinet/in_mcast.c
@@ -1338,6 +1338,7 @@
inp_block_unblock_source(struct inpcb *inp, struct sockopt *sopt)
{
struct group_source_req gsr;
+ struct rm_priotracker in_ifa_tracker;
sockunion_t *gsa, *ssa;
struct ifnet *ifp;
struct in_mfilter *imf;
@@ -1375,9 +1376,11 @@
ssa->sin.sin_len = sizeof(struct sockaddr_in);
ssa->sin.sin_addr = mreqs.imr_sourceaddr;
- if (!in_nullhost(mreqs.imr_interface))
+ if (!in_nullhost(mreqs.imr_interface)) {
+ IN_IFADDR_RLOCK(&in_ifa_tracker);
INADDR_TO_IFP(mreqs.imr_interface, ifp);
-
+ IN_IFADDR_RUNLOCK(&in_ifa_tracker);
+ }
if (sopt->sopt_name == IP_BLOCK_SOURCE)
doblock = 1;
@@ -1873,7 +1876,6 @@
*
* Returns NULL if no ifp could be found.
*
- * SMPng: TODO: Acquire the appropriate locks for INADDR_TO_IFP.
* FUTURE: Implement IPv4 source-address selection.
*/
static struct ifnet *
@@ -1891,7 +1893,9 @@
ifp = NULL;
if (!in_nullhost(ina)) {
+ IN_IFADDR_RLOCK(&in_ifa_tracker);
INADDR_TO_IFP(ina, ifp);
+ IN_IFADDR_RUNLOCK(&in_ifa_tracker);
} else {
fibnum = inp ? inp->inp_inc.inc_fibnum : 0;
if (fib4_lookup_nh_basic(fibnum, gsin->sin_addr, 0, 0, &nh4)==0)
@@ -2223,6 +2227,7 @@
{
struct group_source_req gsr;
struct ip_mreq_source mreqs;
+ struct rm_priotracker in_ifa_tracker;
sockunion_t *gsa, *ssa;
struct ifnet *ifp;
struct in_mfilter *imf;
@@ -2281,9 +2286,11 @@
* XXX NOTE WELL: The RFC 3678 API is preferred because
* using an IPv4 address as a key is racy.
*/
- if (!in_nullhost(mreqs.imr_interface))
+ if (!in_nullhost(mreqs.imr_interface)) {
+ IN_IFADDR_RLOCK(&in_ifa_tracker);
INADDR_TO_IFP(mreqs.imr_interface, ifp);
-
+ IN_IFADDR_RUNLOCK(&in_ifa_tracker);
+ }
CTR3(KTR_IGMPV3, "%s: imr_interface = 0x%08x, ifp = %p",
__func__, ntohl(mreqs.imr_interface.s_addr), ifp);
@@ -2443,6 +2450,7 @@
static int
inp_set_multicast_if(struct inpcb *inp, struct sockopt *sopt)
{
+ struct rm_priotracker in_ifa_tracker;
struct in_addr addr;
struct ip_mreqn mreqn;
struct ifnet *ifp;
@@ -2481,7 +2489,9 @@
if (in_nullhost(addr)) {
ifp = NULL;
} else {
+ IN_IFADDR_RLOCK(&in_ifa_tracker);
INADDR_TO_IFP(addr, ifp);
+ IN_IFADDR_RUNLOCK(&in_ifa_tracker);
if (ifp == NULL)
return (EADDRNOTAVAIL);
}
diff --git a/sys/netinet/ip_input.c.orig b/sys/netinet/ip_input.c
--- a/sys/netinet/ip_input.c.orig
+++ b/sys/netinet/ip_input.c
@@ -446,6 +446,7 @@
void
ip_input(struct mbuf *m)
{
+ struct rm_priotracker in_ifa_tracker;
struct ip *ip = NULL;
struct in_ifaddr *ia = NULL;
struct ifaddr *ifa;
@@ -677,7 +678,7 @@
/*
* Check for exact addresses in the hash bucket.
*/
- /* IN_IFADDR_RLOCK(); */
+ IN_IFADDR_RLOCK(&in_ifa_tracker);
LIST_FOREACH(ia, INADDR_HASH(ip->ip_dst.s_addr), ia_hash) {
/*
* If the address matches, verify that the packet
@@ -689,11 +690,11 @@
counter_u64_add(ia->ia_ifa.ifa_ipackets, 1);
counter_u64_add(ia->ia_ifa.ifa_ibytes,
m->m_pkthdr.len);
- /* IN_IFADDR_RUNLOCK(); */
+ IN_IFADDR_RUNLOCK(&in_ifa_tracker);
goto ours;
}
}
- /* IN_IFADDR_RUNLOCK(); */
+ IN_IFADDR_RUNLOCK(&in_ifa_tracker);
/*
* Check for broadcast addresses.
_______________________________________________
[email protected] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "[email protected]"
