On 20/12/16(Tue) 18:47, Mike Belopuhov wrote:
> On Tue, Dec 20, 2016 at 17:06 +0100, Martin Pieuchot wrote:
> > 
> > You'll need to release the lock before calling ifc->ifc_create in
> > if_clone_create() and do the same dance in if_clone_destroy().
> >
> 
> Indeed.
> 
> > But I think that's the way to go.  If you can create/destroy
> > pseudo-interface without panicing we're good.
> >
> 
> Seems to work here, but it feels wrong.

I don't think we can do much better, let's go with that we'll refine
later.

ok mpi@

> 
> > >  void
> > > @@ -941,6 +941,7 @@ if_detach(struct ifnet *ifp)
> > >  
> > >   ifq_clr_oactive(&ifp->if_snd);
> > >  
> > > + rw_enter_write(&netlock);
> > >   s = splnet();
> > >   /* Other CPUs must not have a reference before we start destroying. */
> > >   if_idxmap_remove(ifp);
> > > @@ -1017,6 +1018,7 @@ if_detach(struct ifnet *ifp)
> > >   /* Announce that the interface is gone. */
> > >   rt_ifannouncemsg(ifp, IFAN_DEPARTURE);
> > >   splx(s);
> > > + rw_exit_write(&netlock);
> > >  
> > >   ifq_destroy(&ifp->if_snd);
> > >  }
> > 
> > Please keep the NET_LOCK/UNLOCK() macro.  The places where we
> > grab/release the `netlock' directly are places that need attention.
> 
> Done.
> 
> diff --git sys/net/if.c sys/net/if.c
> index 1e103564710..bd8b9267d5f 100644
> --- sys/net/if.c
> +++ sys/net/if.c
> @@ -523,15 +523,15 @@ if_attachhead(struct ifnet *ifp)
>  void
>  if_attach(struct ifnet *ifp)
>  {
>       int s;
>  
> -     s = splsoftnet();
> +     NET_LOCK(s);
>       if_attach_common(ifp);
>       TAILQ_INSERT_TAIL(&ifnet, ifp, if_list);
>       if_attachsetup(ifp);
> -     splx(s);
> +     NET_UNLOCK(s);
>  }
>  
>  void
>  if_attach_common(struct ifnet *ifp)
>  {
> @@ -939,10 +939,11 @@ if_detach(struct ifnet *ifp)
>       /* Undo pseudo-driver changes. */
>       if_deactivate(ifp);
>  
>       ifq_clr_oactive(&ifp->if_snd);
>  
> +     NET_LOCK(s);
>       s = splnet();
>       /* Other CPUs must not have a reference before we start destroying. */
>       if_idxmap_remove(ifp);
>  
>       ifp->if_start = if_detached_start;
> @@ -1015,10 +1016,11 @@ if_detach(struct ifnet *ifp)
>       }
>  
>       /* Announce that the interface is gone. */
>       rt_ifannouncemsg(ifp, IFAN_DEPARTURE);
>       splx(s);
> +     NET_UNLOCK(s);
>  
>       ifq_destroy(&ifp->if_snd);
>  }
>  
>  /*
> @@ -1068,11 +1070,14 @@ if_clone_create(const char *name, int rdomain)
>               return (EINVAL);
>  
>       if (ifunit(name) != NULL)
>               return (EEXIST);
>  
> +     /* XXXSMP breaks atomicity */
> +     rw_exit_write(&netlock);
>       ret = (*ifc->ifc_create)(ifc, unit);
> +     rw_enter_write(&netlock);
>  
>       if (ret != 0 || (ifp = ifunit(name)) == NULL)
>               return (ret);
>  
>       if_addgroup(ifp, ifc->ifc_name);
> @@ -1088,10 +1093,11 @@ if_clone_create(const char *name, int rdomain)
>  int
>  if_clone_destroy(const char *name)
>  {
>       struct if_clone *ifc;
>       struct ifnet *ifp;
> +     int ret;
>  
>       splsoftassert(IPL_SOFTNET);
>  
>       ifc = if_clone_lookup(name, NULL);
>       if (ifc == NULL)
> @@ -1109,11 +1115,16 @@ if_clone_destroy(const char *name)
>               s = splnet();
>               if_down(ifp);
>               splx(s);
>       }
>  
> -     return ((*ifc->ifc_destroy)(ifp));
> +     /* XXXSMP breaks atomicity */
> +     rw_exit_write(&netlock);
> +     ret = (*ifc->ifc_destroy)(ifp);
> +     rw_enter_write(&netlock);
> +
> +     return (ret);
>  }
>  
>  /*
>   * Look up a network interface cloner.
>   */

Reply via email to