On Tue, Mar 31, 2020 at 06:15:46PM +0200, Martin Pieuchot wrote: > On 31/03/20(Tue) 18:58, Vitaliy Makkoveev wrote: > > On Tue, Mar 31, 2020 at 05:23:46PM +0200, Martin Pieuchot wrote: > > > On 31/03/20(Tue) 16:48, Vitaliy Makkoveev wrote: > > > > I refactored pppx(4). The idea is to use pipex(4) as it was designed. > > > > No one holds pipex_session outside pipex(4) so pipex_timer() calls are > > > > safe. Unfortunately, this way gives some performance impact, because we > > > > need to call pipex_lookup_by_session_id() in some places. This impact > > > > will gone after pipex_session becames safely shared between multiple > > > > holders and this is my next goal. > > > > > > I'd be more confident if we could go with the one-line change that you > > > submitted in the first mail of this thread without the debugging > > > messages. > > > > > > Mixing bug-fixes (or features) and refactoring is not a great > > > development practise as it might hide the point of the change and > > > introduce or expose new bugs. > > > > > I changed my original patch. Since npppd(8) ignores ioctl() errors and > > client will be connected without pppx(4) interface creation I decide to > > lie npppd(8). > > Well better fix npppd(8), no? Not crashing the kernel is priority 1. I made patch for npppd(8) too. I include it to this email below, without starting new thread, ok? If ioctl(PIPEXASESSION) failed and error != ENXIO it means that pipex is enabled and session creation failed so down this connection.
> Then if the daemon has a bug, should the kernel work around it? In most common cases should :( ---- cut begin Index: usr.sbin/npppd/npppd/ppp.c =================================================================== RCS file: /cvs/src/usr.sbin/npppd/npppd/ppp.c,v retrieving revision 1.28 diff -u -p -r1.28 ppp.c --- usr.sbin/npppd/npppd/ppp.c 27 Feb 2019 04:52:19 -0000 1.28 +++ usr.sbin/npppd/npppd/ppp.c 31 Mar 2020 19:59:21 -0000 @@ -1134,8 +1134,12 @@ ppp_on_network_pipex(npppd_ppp *_this) (!MPPE_MUST_NEGO(_this) || _this->ccp.fsm.state == OPENED || _this->ccp.fsm.state == STOPPED)) { /* IPCP is opened and MPPE is not required or MPPE is opened */ - if (npppd_ppp_pipex_enable(_this->pppd, _this) != 0) + if (npppd_ppp_pipex_enable(_this->pppd, _this) != 0) { ppp_log(_this, LOG_WARNING, "failed enable pipex: %m"); + /* failed to create pipex session */ + ppp_phy_downed(_this); + return; + } ppp_log(_this, LOG_NOTICE, "Using pipex=%s", (_this->pipex_enabled != 0)? "yes" : "no"); _this->pipex_started = 1; ---- cut end And patch for pppx() ---- cut begin 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 31 Mar 2020 20:00:33 -0000 @@ -665,6 +665,12 @@ 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: ---- cut end