On 24/06/20(Wed) 17:10, Vitaliy Makkoveev wrote:
> While `mbuf' enqueued to `pipexinq' or `pipexoutq' it has the reference
> to corresponding pipex(4) session as `ph_cookie'. `ph_cookie' is
> accessed by pipexintr() and it's always defferent context from context
> where we destroy session. `ph_cookie' is protected only while we destroy
> session by pipex_timer() but this protection is not effective. While we
> destroy session related to pppx(4) `ph_cookie' is not potected. While we
> destroy session related to pppac(4) by `PIPEXSMODE' ioctl() or by
> closing pppac(4) device node `ph_cookie' is also not protected.
>
> I'am going to use reference counters to protect `ph_cookie' but some
> additional steps required to be done.
Please no. Store an ifidx in session instead of a pointer. Such
index are guaranteed to be unique and can be used with if_get(9).
We deliberately kept if_ref() private to keep the code base coherent.
> We have `pipex_iface' which holds the reference to external `ifnet'. The
> pipex(4) session also has reference to this `ifnet'. With reference
> counters pipex(4) session can still be in `pipexinq' or `pipexoutq' but
> corresponding `pppx_if' or `pppac_softc' is already destroyed. I want to
> be shure while we do if_detach() no one has reference to this `ifnet'.
>
> Diff below grabs reference to `ifnet' each time it passed to pipex(4).
> Also while we release this session we release the reference.
>
> We attach `ifnet' before we have linked sessions and we detach `ifnet'
> after we release all sessions. Now it's safe.
>
> if_ref() was declared in sys/if.c so I moved if_ref() declaration to
> sys/if.h.
>
> Index: sys/net/if.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if.c,v
> retrieving revision 1.610
> diff -u -p -r1.610 if.c
> --- sys/net/if.c 22 Jun 2020 09:45:13 -0000 1.610
> +++ sys/net/if.c 24 Jun 2020 13:43:33 -0000
> @@ -192,7 +192,6 @@ void if_qstart_compat(struct ifqueue *);
>
> void if_ifp_dtor(void *, void *);
> void if_map_dtor(void *, void *);
> -struct ifnet *if_ref(struct ifnet *);
>
> /*
> * struct if_map
> Index: sys/net/if.h
> ===================================================================
> RCS file: /cvs/src/sys/net/if.h,v
> retrieving revision 1.203
> diff -u -p -r1.203 if.h
> --- sys/net/if.h 25 Jul 2019 15:23:39 -0000 1.203
> +++ sys/net/if.h 24 Jun 2020 13:43:33 -0000
> @@ -548,6 +548,7 @@ int if_delgroup(struct ifnet *, const ch
> void if_group_routechange(struct sockaddr *, struct sockaddr *);
> struct ifnet *ifunit(const char *);
> struct ifnet *if_get(unsigned int);
> +struct ifnet *if_ref(struct ifnet *);
> void if_put(struct ifnet *);
> void ifnewlladdr(struct ifnet *);
> void if_congestion(void);
> Index: sys/net/if_pppx.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_pppx.c,v
> retrieving revision 1.90
> diff -u -p -r1.90 if_pppx.c
> --- sys/net/if_pppx.c 24 Jun 2020 08:52:53 -0000 1.90
> +++ sys/net/if_pppx.c 24 Jun 2020 13:43:33 -0000
> @@ -714,15 +714,15 @@ pppx_add_session(struct pppx_dev *pxd, s
> ifp->if_softc = pxi;
> /* ifp->if_rdomain = req->pr_rdomain; */
>
> - error = pipex_link_session(session, &pxi->pxi_ifcontext);
> - if (error)
> - goto remove;
> -
> /* XXXSMP breaks atomicity */
> NET_UNLOCK();
> if_attach(ifp);
> NET_LOCK();
>
> + error = pipex_link_session(session, &pxi->pxi_ifcontext);
> + if (error)
> + goto detach;
> +
> if_addgroup(ifp, "pppx");
> if_alloc_sadl(ifp);
>
> @@ -771,7 +771,12 @@ pppx_add_session(struct pppx_dev *pxd, s
>
> return (error);
>
> -remove:
> +detach:
> + /* XXXSMP breaks atomicity */
> + NET_UNLOCK();
> + if_detach(ifp);
> + NET_LOCK();
> +
> if (RBT_REMOVE(pppx_ifs, &pppx_ifs, pxi) == NULL)
> panic("%s: inconsistent RB tree", __func__);
> LIST_REMOVE(pxi, pxi_list);
> @@ -860,13 +865,13 @@ pppx_if_destroy(struct pppx_dev *pxd, st
> CLR(ifp->if_flags, IFF_RUNNING);
>
> pipex_unlink_session(session);
> + pipex_rele_session(session);
>
> /* XXXSMP breaks atomicity */
> NET_UNLOCK();
> if_detach(ifp);
> NET_LOCK();
>
> - pipex_rele_session(session);
> if (RBT_REMOVE(pppx_ifs, &pppx_ifs, pxi) == NULL)
> panic("%s: inconsistent RB tree", __func__);
> LIST_REMOVE(pxi, pxi_list);
> Index: sys/net/pipex.c
> ===================================================================
> RCS file: /cvs/src/sys/net/pipex.c,v
> retrieving revision 1.116
> diff -u -p -r1.116 pipex.c
> --- sys/net/pipex.c 22 Jun 2020 09:38:15 -0000 1.116
> +++ sys/net/pipex.c 24 Jun 2020 13:43:34 -0000
> @@ -147,6 +147,7 @@ pipex_iface_init(struct pipex_iface_cont
> {
> struct pipex_session *session;
>
> + if_ref(ifp);
> pipex_iface->pipexmode = 0;
> pipex_iface->ifnet_this = ifp;
>
> @@ -164,6 +165,7 @@ pipex_iface_init(struct pipex_iface_cont
> /* virtual pipex_session entry for multicast */
> session = pool_get(&pipex_session_pool, PR_WAITOK | PR_ZERO);
> session->is_multicast = 1;
> + if_ref(pipex_iface->ifnet_this);
> session->pipex_iface = pipex_iface;
> pipex_iface->multicast_session = session;
> }
> @@ -194,10 +196,11 @@ pipex_iface_stop(struct pipex_iface_cont
> void
> pipex_iface_fini(struct pipex_iface_context *pipex_iface)
> {
> - pool_put(&pipex_session_pool, pipex_iface->multicast_session);
> NET_LOCK();
> pipex_iface_stop(pipex_iface);
> NET_UNLOCK();
> + pipex_rele_session(pipex_iface->multicast_session);
> + if_put(pipex_iface->ifnet_this);
> }
>
> int
> @@ -421,6 +424,9 @@ pipex_init_session(struct pipex_session
> void
> pipex_rele_session(struct pipex_session *session)
> {
> + if (session->pipex_iface) {
> + if_put(session->pipex_iface->ifnet_this);
> + }
> if (session->mppe_recv.old_session_keys)
> pool_put(&mppe_key_pool, session->mppe_recv.old_session_keys);
> pool_put(&pipex_session_pool, session);
> @@ -438,6 +444,7 @@ pipex_link_session(struct pipex_session
> session->session_id))
> return (EEXIST);
>
> + if_ref(iface->ifnet_this);
> session->pipex_iface = iface;
>
> LIST_INSERT_HEAD(&pipex_session_list, session, session_list);
> @@ -655,7 +662,7 @@ pipex_destroy_session(struct pipex_sessi
> }
>
> pipex_unlink_session(session);
> - pool_put(&pipex_session_pool, session);
> + pipex_rele_session(session);
>
> return (0);
> }
>
