From 46d22571b80fc6c890dd15455ae598c909d518df Mon Sep 17 00:00:00 2001 From: mvs Date: Tue, 18 Apr 2023 22:01:23 +0000 Subject: [PATCH] Remove kernel lock from ifa_ifwithaddr() and ifa_ifwithdstaddr(). Netlock protects `if_list', `ifa_list' and returned `ifa' dereference, so put netlock assertion within. Please note, rtable_setsource() doesn't destroy data pointed by `ar_source'. This is the `ifa_addr' data belongs to `ifa' and exclusive netlock is required to destroy it. So the kernel lock is not required within rt_setsource(). Take netlock by rt_setsource() caller to make `ifa' dereference safe. Suggestions and ok by bluhm@ --- sys/dev/usb/if_umb.c | 6 +++--- sys/net/if.c | 14 +++++++------- sys/net/if_var.h | 5 +++-- sys/net/rtsock.c | 19 +++++++------------ 4 files changed, 20 insertions(+), 24 deletions(-) diff --git a/sys/dev/usb/if_umb.c b/sys/dev/usb/if_umb.c index ca89f55ce1b..77dd5394284 100644 --- a/sys/dev/usb/if_umb.c +++ b/sys/dev/usb/if_umb.c @@ -1,4 +1,4 @@ -/* $OpenBSD: if_umb.c,v 1.50 2023/03/31 23:53:49 dlg Exp $ */ +/* $OpenBSD: if_umb.c,v 1.51 2023/04/18 22:01:23 mvs Exp $ */ /* * Copyright (c) 2016 genua mbH @@ -1829,6 +1829,7 @@ umb_add_inet_config(struct umb_softc *sc, struct in_addr ip, u_int prefixlen, default_sin.sin_len = sizeof (default_sin); memset(&info, 0, sizeof(info)); + NET_LOCK(); info.rti_flags = RTF_GATEWAY /* maybe | RTF_STATIC */; info.rti_ifa = ifa_ifwithaddr(sintosa(&ifra.ifra_addr), ifp->if_rdomain); @@ -1836,7 +1837,6 @@ umb_add_inet_config(struct umb_softc *sc, struct in_addr ip, u_int prefixlen, info.rti_info[RTAX_NETMASK] = sintosa(&default_sin); info.rti_info[RTAX_GATEWAY] = sintosa(&ifra.ifra_dstaddr); - NET_LOCK(); rv = rtrequest(RTM_ADD, &info, 0, &rt, ifp->if_rdomain); NET_UNLOCK(); if (rv) { @@ -1910,6 +1910,7 @@ umb_add_inet6_config(struct umb_softc *sc, struct in6_addr *ip, u_int prefixlen, default_sin6.sin6_len = sizeof (default_sin6); memset(&info, 0, sizeof(info)); + NET_LOCK(); info.rti_flags = RTF_GATEWAY /* maybe | RTF_STATIC */; info.rti_ifa = ifa_ifwithaddr(sin6tosa(&ifra.ifra_addr), ifp->if_rdomain); @@ -1917,7 +1918,6 @@ umb_add_inet6_config(struct umb_softc *sc, struct in6_addr *ip, u_int prefixlen, info.rti_info[RTAX_NETMASK] = sin6tosa(&default_sin6); info.rti_info[RTAX_GATEWAY] = sin6tosa(&ifra.ifra_dstaddr); - NET_LOCK(); rv = rtrequest(RTM_ADD, &info, 0, &rt, ifp->if_rdomain); NET_UNLOCK(); if (rv) { diff --git a/sys/net/if.c b/sys/net/if.c index 1cd91f8fa80..e25fe1c03ac 100644 --- a/sys/net/if.c +++ b/sys/net/if.c @@ -1,4 +1,4 @@ -/* $OpenBSD: if.c,v 1.689 2023/04/18 22:00:19 mvs Exp $ */ +/* $OpenBSD: if.c,v 1.690 2023/04/18 22:01:24 mvs Exp $ */ /* $NetBSD: if.c,v 1.35 1996/05/07 05:26:04 thorpej Exp $ */ /* @@ -1413,8 +1413,9 @@ ifa_ifwithaddr(struct sockaddr *addr, u_int rtableid) struct ifaddr *ifa; u_int rdomain; + NET_ASSERT_LOCKED(); + rdomain = rtable_l2(rtableid); - KERNEL_LOCK(); TAILQ_FOREACH(ifp, &ifnetlist, if_list) { if (ifp->if_rdomain != rdomain) continue; @@ -1424,12 +1425,10 @@ ifa_ifwithaddr(struct sockaddr *addr, u_int rtableid) continue; if (equal(addr, ifa->ifa_addr)) { - KERNEL_UNLOCK(); return (ifa); } } } - KERNEL_UNLOCK(); return (NULL); } @@ -1442,8 +1441,9 @@ ifa_ifwithdstaddr(struct sockaddr *addr, u_int rdomain) struct ifnet *ifp; struct ifaddr *ifa; + NET_ASSERT_LOCKED(); + rdomain = rtable_l2(rdomain); - KERNEL_LOCK(); TAILQ_FOREACH(ifp, &ifnetlist, if_list) { if (ifp->if_rdomain != rdomain) continue; @@ -1453,13 +1453,11 @@ ifa_ifwithdstaddr(struct sockaddr *addr, u_int rdomain) addr->sa_family || ifa->ifa_dstaddr == NULL) continue; if (equal(addr, ifa->ifa_dstaddr)) { - KERNEL_UNLOCK(); return (ifa); } } } } - KERNEL_UNLOCK(); return (NULL); } @@ -3171,12 +3169,14 @@ ifsettso(struct ifnet *ifp, int on) void ifa_add(struct ifnet *ifp, struct ifaddr *ifa) { + NET_ASSERT_LOCKED_EXCLUSIVE(); TAILQ_INSERT_TAIL(&ifp->if_addrlist, ifa, ifa_list); } void ifa_del(struct ifnet *ifp, struct ifaddr *ifa) { + NET_ASSERT_LOCKED_EXCLUSIVE(); TAILQ_REMOVE(&ifp->if_addrlist, ifa, ifa_list); } diff --git a/sys/net/if_var.h b/sys/net/if_var.h index 0eeebdc9bbb..853dbccadbc 100644 --- a/sys/net/if_var.h +++ b/sys/net/if_var.h @@ -1,4 +1,4 @@ -/* $OpenBSD: if_var.h,v 1.124 2023/04/18 22:00:19 mvs Exp $ */ +/* $OpenBSD: if_var.h,v 1.125 2023/04/18 22:01:24 mvs Exp $ */ /* $NetBSD: if.h,v 1.23 1996/05/07 02:40:27 thorpej Exp $ */ /* @@ -240,7 +240,8 @@ struct ifaddr { #define ifa_broadaddr ifa_dstaddr /* broadcast address interface */ struct sockaddr *ifa_netmask; /* used to determine subnet */ struct ifnet *ifa_ifp; /* back-pointer to interface */ - TAILQ_ENTRY(ifaddr) ifa_list; /* list of addresses for interface */ + TAILQ_ENTRY(ifaddr) ifa_list; /* [N] list of addresses for + interface */ u_int ifa_flags; /* interface flags, see below */ struct refcnt ifa_refcnt; /* number of `rt_ifa` references */ int ifa_metric; /* cost of going out this interface */ diff --git a/sys/net/rtsock.c b/sys/net/rtsock.c index b6d3c06894e..d3cc836db49 100644 --- a/sys/net/rtsock.c +++ b/sys/net/rtsock.c @@ -1,4 +1,4 @@ -/* $OpenBSD: rtsock.c,v 1.362 2023/04/18 09:56:54 mvs Exp $ */ +/* $OpenBSD: rtsock.c,v 1.363 2023/04/18 22:01:24 mvs Exp $ */ /* $NetBSD: rtsock.c,v 1.18 1996/03/29 00:32:10 cgd Exp $ */ /* @@ -853,8 +853,10 @@ route_output(struct mbuf *m, struct socket *so) error = EINVAL; goto fail; } - if ((error = - rt_setsource(tableid, info.rti_info[RTAX_IFA])) != 0) + NET_LOCK(); + error = rt_setsource(tableid, info.rti_info[RTAX_IFA]); + NET_UNLOCK(); + if (error) goto fail; } else { error = rtm_output(rtm, &rt, &info, prio, tableid); @@ -2350,7 +2352,6 @@ int rt_setsource(unsigned int rtableid, struct sockaddr *src) { struct ifaddr *ifa; - int error; /* * If source address is 0.0.0.0 or :: * use automatic source selection @@ -2374,20 +2375,14 @@ rt_setsource(unsigned int rtableid, struct sockaddr *src) return (EAFNOSUPPORT); } - KERNEL_LOCK(); /* * Check if source address is assigned to an interface in the * same rdomain */ - if ((ifa = ifa_ifwithaddr(src, rtableid)) == NULL) { - KERNEL_UNLOCK(); + if ((ifa = ifa_ifwithaddr(src, rtableid)) == NULL) return (EINVAL); - } - - error = rtable_setsource(rtableid, src->sa_family, ifa->ifa_addr); - KERNEL_UNLOCK(); - return (error); + return rtable_setsource(rtableid, src->sa_family, ifa->ifa_addr); } /* -- 2.20.1