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.
> */
