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