On Mon, Jan 27, 2020 at 12:48:49AM +0100, Alexandr Nedvedicky wrote:
> Hello,
>
> </snip>
> > > I'm not sure about your diff see below.
> >
> > visa@ pointed out that PF_LOCK will introduce a potential sleep inside
> > if_vinput(),
> > making this unsafe, so here's a less optimistic diff where only the vlan
> > list
> > traversal is inside the SMR read section, and vlan_softcs are still
> > refcounted.
> > I probably need to rework some other unsent SRP->SMR diffs now too.
>
> less optimistic variant looks better. Though I have one
> question related to refcnt_finalize().
>
> </snip>
>
> > @@ -257,6 +239,7 @@ vlan_clone_destroy(struct ifnet *ifp)
> >
> > ether_ifdetach(ifp);
> > if_detach(ifp);
> > + smr_barrier();
> > refcnt_finalize(&sc->sc_refcnt, "vlanrefs");
> ^^^^
> I think a protection should be added so there will be at most 1 thread
> calling to refcnt_finalize(). I've noticed vlan_clone_destroy() does
> ->sc_dead = 1; couple line earlier under protection of NET_LOCK(). Would
> it
> make sense to do if (->sc_dead == 1) first? We would just bail out if
> ->sc_dead will be set already, because there is vlan_clone_destroy() in
> progress already.
>
> Otherwise the change looks good.
I agree we should protect against that - if I add a sleep there, I can easily
cause a panic with 'ifconfig vlan10 destroy & ifconfig vlan10 destroy'.
I think that's a separate issue though, as the same window is present in
the existing code.
This fixes it, at least in my brief testing:
Index: if_vlan.c
===================================================================
RCS file: /cvs/src/sys/net/if_vlan.c,v
retrieving revision 1.202
diff -u -p -u -p -r1.202 if_vlan.c
--- if_vlan.c 7 Nov 2019 07:36:32 -0000 1.202
+++ if_vlan.c 27 Jan 2020 03:47:24 -0000
@@ -249,6 +249,10 @@ vlan_clone_destroy(struct ifnet *ifp)
struct vlan_softc *sc = ifp->if_softc;
NET_LOCK();
+ if (sc->sc_dead != 0) {
+ NET_UNLOCK();
+ return (ENXIO);
+ }
sc->sc_dead = 1;
if (ISSET(ifp->if_flags, IFF_RUNNING))
