On Tue, Jun 30, 2020 at 03:48:22PM +0300, Vitaliy Makkoveev wrote:
> On Tue, Jun 30, 2020 at 12:08:03PM +0200, Martin Pieuchot wrote:
> > On 29/06/20(Mon) 11:59, Vitaliy Makkoveev wrote:
> > > [...]
> > > I reworked tool for reproduce. Now I avoided fork()/exec() route and it
> > > takes couple of minutes to take panic on 4 cores. Also some screenshots
> > > attached.
> >
> > Setting kern.pool_debug=2 makes the race reproducible in seconds.
Unfortunately you will catch splassert() caused by kern/sched_bsd.c:304.
malloc() will call yield() while we are holding NET_LOCK(). I attached
screenshot with splassertion to this mail.
> >
> > Could you turn this test into something committable in regress/? We can
> > link it to the build once a fix is committed.
> >
>
> We have 3 races with cloned interfaces:
> 1. if_clone_create() vs if_clone_create()
> 2. if_clone_destroy() vs if_clone_destroy()
> 3. if_clone_destroy() vs the rest of stack
>
> It makes sences to commit unified test to regress/, so I suggest to wait
> a little.
The another solution.
Diff below introduces per-`ifc' serialization for if_clone_create() and
if_clone_destroy(). There is no index bitmap anymore.
Diff fixes the following races:
1. if_clone_create() vs if_clone_create()
2. if_clone_destroy() vs if_clone_destroy()
`ifc_create' will go the same lock path for each cloner, and
`ifc_destroy' will go this path but in reverse order. It seems
reasonable to allow simultaneous create/destroy for different cloners
but since different instances of one cloner will block each other it's
no reason have parallelism here.
Updated test tool
---- cut begin ----
#include <sys/socket.h>
#include <sys/select.h>
#include <sys/ioctl.h>
#include <net/if.h>
#include <stdio.h>
#include <stdlib.h>
#include <err.h>
#include <errno.h>
#include <pthread.h>
#include <string.h>
#include <unistd.h>
static struct ifreq ifr;
static void *clone_create(void *arg)
{
int s;
if((s=socket(AF_INET, SOCK_DGRAM, 0))<0)
err(1, "socket()");
while(1){
if(ioctl(s, SIOCIFCREATE, &ifr)<0)
if(errno==EINVAL)
exit(1);
}
return NULL;
}
static void *clone_destroy(void *arg)
{
int s;
if((s=socket(AF_INET, SOCK_DGRAM, 0))<0)
err(1, "socket()");
while(1){
if(ioctl(s, SIOCIFDESTROY, &ifr)<0)
if(errno==EINVAL)
exit(1);
}
return NULL;
}
int main(int argc, char *argv[])
{
pthread_t thr;
int i;
if(argc!=2){
fprintf(stderr, "usage: %s ifname\n", getprogname());
return 1;
}
if(getuid()!=0){
fprintf(stderr, "should be root\n");
return 1;
}
memset(&ifr, 0, sizeof(ifr));
strlcpy(ifr.ifr_name, argv[1], sizeof(ifr.ifr_name));
for(i=0; i<8*4; ++i){
if(pthread_create(&thr, NULL, clone_create, NULL)!=0)
errx(1, "pthread_create(clone_create)");
if(pthread_create(&thr, NULL, clone_destroy, NULL)!=0)
errx(1, "pthread_create(clone_destroy)");
}
select(0, NULL, NULL, NULL, NULL);
return 0;
}
---- cut end ----
Index: sys/net/if.c
===================================================================
RCS file: /cvs/src/sys/net/if.c,v
retrieving revision 1.611
diff -u -p -r1.611 if.c
--- sys/net/if.c 30 Jun 2020 09:31:38 -0000 1.611
+++ sys/net/if.c 30 Jun 2020 20:41:50 -0000
@@ -155,6 +155,8 @@ int if_getgrouplist(caddr_t);
void if_linkstate(struct ifnet *);
void if_linkstate_task(void *);
+int if_clone_lock(struct if_clone *);
+void if_clone_unlock(struct if_clone *);
int if_clone_list(struct if_clonereq *);
struct if_clone *if_clone_lookup(const char *, int *);
@@ -1244,27 +1246,35 @@ if_clone_create(const char *name, int rd
{
struct if_clone *ifc;
struct ifnet *ifp;
- int unit, ret;
+ int unit, error;
ifc = if_clone_lookup(name, &unit);
if (ifc == NULL)
return (EINVAL);
- if (ifunit(name) != NULL)
- return (EEXIST);
+ error = if_clone_lock(ifc);
+ if (error != 0)
+ return (error);
+
+ if (ifunit(name) != NULL) {
+ error = (EEXIST);
+ goto unlock;
+ }
- ret = (*ifc->ifc_create)(ifc, unit);
+ error = (*ifc->ifc_create)(ifc, unit);
- if (ret != 0 || (ifp = ifunit(name)) == NULL)
- return (ret);
+ if (error != 0 || (ifp = ifunit(name)) == NULL)
+ goto unlock;
NET_LOCK();
if_addgroup(ifp, ifc->ifc_name);
if (rdomain != 0)
if_setrdomain(ifp, rdomain);
NET_UNLOCK();
+unlock:
+ if_clone_unlock(ifc);
- return (ret);
+ return (error);
}
/*
@@ -1275,18 +1285,26 @@ if_clone_destroy(const char *name)
{
struct if_clone *ifc;
struct ifnet *ifp;
- int ret;
+ int error;
ifc = if_clone_lookup(name, NULL);
if (ifc == NULL)
return (EINVAL);
+ error = if_clone_lock(ifc);
+ if (error != 0)
+ return (error);
+
ifp = ifunit(name);
- if (ifp == NULL)
- return (ENXIO);
+ if (ifp == NULL) {
+ error = (ENXIO);
+ goto unlock;
+ }
- if (ifc->ifc_destroy == NULL)
- return (EOPNOTSUPP);
+ if (ifc->ifc_destroy == NULL) {
+ error = (EOPNOTSUPP);
+ goto unlock;
+ }
NET_LOCK();
if (ifp->if_flags & IFF_UP) {
@@ -1296,9 +1314,56 @@ if_clone_destroy(const char *name)
splx(s);
}
NET_UNLOCK();
- ret = (*ifc->ifc_destroy)(ifp);
+ error = (*ifc->ifc_destroy)(ifp);
+unlock:
+ if_clone_unlock(ifc);
+
+ return (error);
+}
+
+/*
+ * Lock a clone network interface.
+ */
+int
+if_clone_lock(struct if_clone *ifc)
+{
+ int error;
+
+ rw_enter_write(&ifc->ifc_lock);
+
+ while (ifc->ifc_flags & IFC_CREATE_LOCKED) {
+ ifc->ifc_flags |= IFC_CREATE_LOCKWAIT;
+ error = rwsleep_nsec(&ifc->ifc_flags, &ifc->ifc_lock,
+ PWAIT|PCATCH, "ifclk", INFSLP);
+ if(error != 0) {
+ ifc->ifc_flags &= ~IFC_CREATE_LOCKWAIT;
+ rw_exit_write(&ifc->ifc_lock);
+ return error;
+ }
+ }
+ ifc->ifc_flags |= IFC_CREATE_LOCKED;
+ ifc->ifc_flags &= ~IFC_CREATE_LOCKWAIT;
+
+ rw_exit_write(&ifc->ifc_lock);
+
+ return 0;
+}
+
+/*
+ * Unlock a clone network interface.
+ */
+void
+if_clone_unlock(struct if_clone *ifc)
+{
+ rw_enter_write(&ifc->ifc_lock);
+
+ ifc->ifc_flags &= ~IFC_CREATE_LOCKED;
+ if (ifc->ifc_flags & IFC_CREATE_LOCKWAIT) {
+ ifc->ifc_flags &= ~IFC_CREATE_LOCKWAIT;
+ wakeup(&ifc->ifc_flags);
+ }
- return (ret);
+ rw_exit_write(&ifc->ifc_lock);
}
/*
Index: sys/net/if_var.h
===================================================================
RCS file: /cvs/src/sys/net/if_var.h,v
retrieving revision 1.105
diff -u -p -r1.105 if_var.h
--- sys/net/if_var.h 12 May 2020 08:49:54 -0000 1.105
+++ sys/net/if_var.h 30 Jun 2020 20:41:50 -0000
@@ -45,6 +45,7 @@
#include <sys/task.h>
#include <sys/time.h>
#include <sys/timeout.h>
+#include <sys/rwlock.h>
#include <net/ifq.h>
@@ -85,16 +86,23 @@ struct if_clone {
LIST_ENTRY(if_clone) ifc_list; /* on list of cloners */
const char *ifc_name; /* name of device, e.g. `gif' */
size_t ifc_namelen; /* length of name */
+ struct rwlock ifc_lock; /* lock for ifc_flags */
+ u_int ifc_flags; /* flags */
int (*ifc_create)(struct if_clone *, int);
int (*ifc_destroy)(struct ifnet *);
};
+#define IFC_CREATE_LOCKED 0x1
+#define IFC_CREATE_LOCKWAIT 0x2
+
#define IF_CLONE_INITIALIZER(name, create, destroy)
\
{ \
.ifc_list = { NULL, NULL }, \
.ifc_name = name, \
.ifc_namelen = sizeof(name) - 1, \
+ .ifc_lock = RWLOCK_INITIALIZER("ifclk"), \
+ .ifc_flags = 0, \
.ifc_create = create, \
.ifc_destroy = destroy, \
}
