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>

Reply via email to