On Fri, Aug 07, 2020 at 11:56:09PM +0300, Vitaliy Makkoveev wrote: > On Fri, Aug 07, 2020 at 09:29:13PM +0100, Jason McIntyre wrote: > > On Fri, Aug 07, 2020 at 10:19:05PM +0300, Vitaliy Makkoveev wrote: > > > Some times ago we disabled in-kernel timeout for pppx(4) related > > > pipex(4) sessions. We did this for prevent use after free issue caused > > > by pipex_timer [1]. By default "idle-timeout" is not set in > > > npppd.conf(5) and I guess this is reason for we forgot to describe this > > > exception in npppd.conf(5). > > > > > > But looks like one user caught this [2]. So I propose to describe this > > > in BUGS section of npppd.conf(5). > > > > > > Also current "idle-timeout" description looks incorrect. If this option > > > is missing, there is not in-kernel timeout for this session, but > > > npppd(8) uses it's own timeout for. And we can't configure this value. > > > > > > YASUOKA, what do you think? May be we can kill in-kernel timeout feature > > > for pipex(4)?, and make npppd(8)'s idle timeout configurable by this > > > option? > > > > > > 1. > > > https://cvsweb.openbsd.org/src/sys/net/if_pppx.c?rev=1.78&content-type=text/x-cvsweb-markup > > > 2. https://marc.info/?l=openbsd-misc&m=159655468504864&w=2 > > > > > > > > > Index: usr.sbin/npppd/npppd/npppd.conf.5 > > > =================================================================== > > > RCS file: /cvs/src/usr.sbin/npppd/npppd/npppd.conf.5,v > > > retrieving revision 1.27 > > > diff -u -p -r1.27 npppd.conf.5 > > > --- usr.sbin/npppd/npppd/npppd.conf.5 23 Apr 2020 21:10:54 -0000 > > > 1.27 > > > +++ usr.sbin/npppd/npppd/npppd.conf.5 7 Aug 2020 19:17:00 -0000 > > > @@ -699,3 +699,9 @@ The current version of > > > .Xr npppd 8 > > > does not support adding or removing tunnel settings or changing listener > > > settings (listen address, port and l2tp-ipsec-require). > > > +.Pp > > > +This time > > > +.Xr pppx 4 > > > +does not allow to create sessions with non null > > > +.Ic idle-timeout > > > +option. > > > > > > > Thanks for your feedback. My English is bad, so thanks for fixing. > > > is this an actual bug? i'm just asking - it might be that the > > idle-timeout text is the best place to warn users, and not BUGS. > > It is pppx(4) related bug. Unfortunately it wasn't solved and we just > disabled this feature to avoid panics. May be pipex(4) man page is the > best place to describe this issue in BUGS section. > > > > > regarding your text: > > > > - "this time" is better written as "At this time" or "currently". > > - "allow to create" is not good sentence structure > > > > i think the text would read better something like: > > > > .Xr pppx 4 > > does not allow sessions with > > .Ic idle-timeout > > set to any value other than 0. > > > > I added this to pipex(4) BUGS section. > > > if the text was better placed in the idle-timeout section: > > > > This value must be 0 for > > .Xr pppx 4 > > sessions. > > And this to npppd.conf(5) idle-timeout section. >
i think that's fine. jmc > > Index: share/man/man4/pipex.4 > =================================================================== > RCS file: /cvs/src/share/man/man4/pipex.4,v > retrieving revision 1.12 > diff -u -p -r1.12 pipex.4 > --- share/man/man4/pipex.4 3 Apr 2020 07:46:04 -0000 1.12 > +++ share/man/man4/pipex.4 7 Aug 2020 20:54:32 -0000 > @@ -288,3 +288,8 @@ The > .Nm > was written by > .An Internet Initiative Japan Inc . > +.Sh BUGS > +.Xr pppx 4 > +does not allow sessions with > +.Ic pr_timeout_sec > +set to any value other than 0. > Index: usr.sbin/npppd/npppd/npppd.conf.5 > =================================================================== > RCS file: /cvs/src/usr.sbin/npppd/npppd/npppd.conf.5,v > retrieving revision 1.27 > diff -u -p -r1.27 npppd.conf.5 > --- usr.sbin/npppd/npppd/npppd.conf.5 23 Apr 2020 21:10:54 -0000 1.27 > +++ usr.sbin/npppd/npppd/npppd.conf.5 7 Aug 2020 20:54:32 -0000 > @@ -325,6 +325,9 @@ The link is disconnected if there are no > for more than the amount of the > .Ar idle-timeout . > The default is 0, which disables the idle timer. > +This value must be 0 for > +.Xr pppx 4 > +sessions. > .It Ic tcp-mss-adjust Ar yes | no > If > .Dq yes >
