On 27/03/20(Fri) 11:58, Vitaliy Makkoveev wrote:
> Each pipex_session has timeout_sec field and if it is not 0,
> pipex_timer() can destroy pipex_session. Each pppx_if contents
> pipex_session. If userland creates pppx_if and pipex_session has
> timeout (for example, npppd.conf has idle-timeout option), pipex_timer()
> can destroy this pipex_session and break it's pppx_if. Diff below is
> quick and dirty fix for this case. At first look, creation of
> pipex_timer()-like pppx_timer() is more better, but I am little confused
> with pipexintr(). Looks like pipexoutq and pipexinq can contain mbufs
> with holds pointer to already destructed pipex_session, because
> pipex_destroy_session() and pppx_if_destroy() just frees pipex_session
> and potential pppx_timer() is only anoter point to crash kernel.
Do you have a backtrace for the memory corruption? Could you share it?
> I suggest, pipex_session and pppx_if should be refactored for good fix,
> at least:
> 1. add reference counter for pipex_session
> 2. pppx_if should hold a reference to pipex_session
> 3. pipex_session should have handler to inform it's holder about
> destruction
>
> Default npppd.conf has no idle-timeout option, so npppd will not be very
> confused with this diff.
>
> Index: sys/net/if_pppx.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_pppx.c,v
> retrieving revision 1.77
> diff -u -p -r1.77 if_pppx.c
> --- sys/net/if_pppx.c 26 Mar 2020 16:50:46 -0000 1.77
> +++ sys/net/if_pppx.c 27 Mar 2020 08:20:50 -0000
> @@ -665,6 +665,10 @@ pppx_add_session(struct pppx_dev *pxd, s
> struct ifnet *over_ifp = NULL;
> #endif
>
> + /* XXX: prevent pxi destruction by pipex_timer() */
> + if (req->pr_timeout_sec != 0)
> + return (EINVAL);
> +
> switch (req->pr_protocol) {
> #ifdef PIPEX_PPPOE
> case PIPEX_PROTO_PPPOE:
>
