On Tue, Apr 18, 2023 at 03:15:54PM +0200, Alexander Bluhm wrote:
> On Mon, Apr 17, 2023 at 03:16:36AM +0300, Vitaliy Makkoveev wrote:
> > Index: sys/dev/usb/if_umb.c
> This umb chunk is OK bluhm@
>
> > Index: sys/net/if.c
> > ===================================================================
> > RCS file: /cvs/src/sys/net/if.c,v
> > retrieving revision 1.688
> > diff -u -p -r1.688 if.c
> > --- sys/net/if.c 8 Apr 2023 13:49:38 -0000 1.688
> > +++ sys/net/if.c 17 Apr 2023 00:09:17 -0000
> > @@ -1409,8 +1409,9 @@ ifa_ifwithaddr(struct sockaddr *addr, u_
>
> ifa_ifwithdstaddr() is iterating over the same loops. It should
> be changed together.
>
Done.
> if_unit() is iterating over if_list. There should be a NET_LOCK().
>
> TAILQ_ENTRY(ifnet) if_list; /* [K] all struct ifnets are chained
> */
> TAILQ_ENTRY(ifaddr) ifa_list; /* list of addresses for interface */
>
> Could you put an [N] there?
No. As I said in the "Call sysctl_ifnames()..." thread, we use both
kernel and net locks to protect `if_list'. This means we do
modification with both locks held, but for read only access only one
lock required. We need to do this because some places protected by
kernel lock do `if_list' foreach loops and we can't introduce context
switch. So, I proposed in the "Call sysctl_ifnames()..." thread mark
`if_list' as [NK] and add "XXXSMP:" comment above `ifnetlist'
definition. I posted the diff to that thread.
I have no objections to mark `ifa_list' as [N].
>
> Can we put an NET_ASSERT_LOCKED_EXCLUSIVE() in ifa_add() and ifa_del()?
>
Done.
> > Index: sys/net/rtsock.c
> > ===================================================================
> > RCS file: /cvs/src/sys/net/rtsock.c,v
> > retrieving revision 1.359
> > diff -u -p -r1.359 rtsock.c
> > --- sys/net/rtsock.c 22 Jan 2023 12:05:44 -0000 1.359
> > +++ sys/net/rtsock.c 17 Apr 2023 00:09:17 -0000
> > @@ -2374,18 +2374,18 @@ rt_setsource(unsigned int rtableid, stru
> > return (EAFNOSUPPORT);
> > }
> >
> > - KERNEL_LOCK();
> > + NET_LOCK();
> > /*
> > * Check if source address is assigned to an interface in the
> > * same rdomain
> > */
> > if ((ifa = ifa_ifwithaddr(src, rtableid)) == NULL) {
> > - KERNEL_UNLOCK();
> > + NET_UNLOCK();
> > return (EINVAL);
> > }
> >
> > error = rtable_setsource(rtableid, src->sa_family, ifa->ifa_addr);
> > - KERNEL_UNLOCK();
> > + NET_UNLOCK();
> >
> > return (error);
> > }
>
> I would perfer to put the NET_LOCK() into the caller. There are
> two more rtable_setsource() in this function which should be net
> locked.
>
rtable_setsource(... , NULL) actually doesn't destroy object pointed by
`ar_source', so for this call netlock is not required. However this
optimisation doesn't produce any visible effect, so no objections to
call rt_setsource() with netlock held.
ok?
Index: sys/dev/usb/if_umb.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/if_umb.c,v
retrieving revision 1.50
diff -u -p -r1.50 if_umb.c
--- sys/dev/usb/if_umb.c 31 Mar 2023 23:53:49 -0000 1.50
+++ sys/dev/usb/if_umb.c 18 Apr 2023 15:45:04 -0000
@@ -1829,6 +1829,7 @@ umb_add_inet_config(struct umb_softc *sc
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
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 *s
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 *s
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) {
Index: sys/net/if.c
===================================================================
RCS file: /cvs/src/sys/net/if.c,v
retrieving revision 1.688
diff -u -p -r1.688 if.c
--- sys/net/if.c 8 Apr 2023 13:49:38 -0000 1.688
+++ sys/net/if.c 18 Apr 2023 15:45:04 -0000
@@ -1409,8 +1409,9 @@ ifa_ifwithaddr(struct sockaddr *addr, u_
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;
@@ -1420,12 +1421,10 @@ ifa_ifwithaddr(struct sockaddr *addr, u_
continue;
if (equal(addr, ifa->ifa_addr)) {
- KERNEL_UNLOCK();
return (ifa);
}
}
}
- KERNEL_UNLOCK();
return (NULL);
}
@@ -1438,8 +1437,9 @@ ifa_ifwithdstaddr(struct sockaddr *addr,
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;
@@ -1449,13 +1449,11 @@ ifa_ifwithdstaddr(struct sockaddr *addr,
addr->sa_family || ifa->ifa_dstaddr == NULL)
continue;
if (equal(addr, ifa->ifa_dstaddr)) {
- KERNEL_UNLOCK();
return (ifa);
}
}
}
}
- KERNEL_UNLOCK();
return (NULL);
}
@@ -3167,12 +3165,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);
}
Index: sys/net/if_var.h
===================================================================
RCS file: /cvs/src/sys/net/if_var.h,v
retrieving revision 1.123
diff -u -p -r1.123 if_var.h
--- sys/net/if_var.h 5 Apr 2023 19:35:23 -0000 1.123
+++ sys/net/if_var.h 18 Apr 2023 15:45:04 -0000
@@ -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 */
Index: sys/net/rtsock.c
===================================================================
RCS file: /cvs/src/sys/net/rtsock.c,v
retrieving revision 1.362
diff -u -p -r1.362 rtsock.c
--- sys/net/rtsock.c 18 Apr 2023 09:56:54 -0000 1.362
+++ sys/net/rtsock.c 18 Apr 2023 15:45:04 -0000
@@ -853,8 +853,10 @@ route_output(struct mbuf *m, struct sock
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, stru
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);
}
/*
