ping?
> On 1 Jul 2020, at 00:02, Vitaliy Makkoveev <henscheltig...@yahoo.com> wrote:
>
> 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,
> \
> }
> <panic.png>
