Re: describe 'idle-timeout' exception in npppd.conf man page
On Sat, 8 Aug 2020 16:01:59 +0300 Vitaliy Makkoveev wrote: > On Sat, Aug 08, 2020 at 08:49:24PM +0900, YASUOKA Masahiko wrote: >> On Fri, 7 Aug 2020 22:19:05 +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? >> >> I think we should mention this to the man page until we fix it. >> So I'd like you to update the man page first. >> >> I'll try to review the problem. >> > > Thanks. I updated my diff with changes proposed by jmc@. Are you agree > with them? Yes. ok yasuoka >> > 1. >> > https://cvsweb.openbsd.org/src/sys/net/if_pppx.c?rev=1.78=text/x-cvsweb-markup >> > 2. https://marc.info/?l=openbsd-misc=159655468504864=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 - >> > 1.27 >> > +++ usr.sbin/npppd/npppd/npppd.conf.5 7 Aug 2020 19:17:00 - >> > @@ -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. >> >
Re: describe 'idle-timeout' exception in npppd.conf man page
I did audit for "idle-timeout" option.
On Sat, Aug 08, 2020 at 08:49:24PM +0900, YASUOKA Masahiko wrote:
> On Fri, 7 Aug 2020 22:19:05 +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.
> >
I was a little wrong with this. This is a different timeout timer.
In my case `l2tp_ctrl_timeout' kills idle sessions. It's totally
npppd(8) related.
The case for "idle-timeout" described below.
> > 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?
>
> I think we should mention this to the man page until we fix it.
> So I'd like you to update the man page first.
>
> I'll try to review the problem.
>
We got this option from npppd.conf(5) and store it as `idle_timeout'
within `struct tunnconf'. While we set npppd(8) related session context
we set `timeout_sec' of `npppd_ppp' at npppd/ppp.c:169 by this value.
Also we initialize timeout timer at npppd/pppc.c:172. We have
ppp_reset_idle_timeout() routime which stops and restart this timer if
`idle_timeout > 0'.
cut begin
125 ppp_init(npppd *pppd, npppd_ppp *_this)
126 {
...
167
168 /* load the idle timer configuration */
169 _this->timeout_sec = conf->idle_timeout;
170
171 if (!evtimer_initialized(&_this->idle_event))
172 evtimer_set(&_this->idle_event, ppp_idle_timeout, _this);
173
632 ppp_reset_idle_timeout(npppd_ppp *_this)
633 {
...
636 evtimer_del(&_this->idle_event);
637 if (_this->timeout_sec > 0) {
638 tv.tv_usec = 0;
639 tv.tv_sec = _this->timeout_sec;
640
641 evtimer_add(&_this->idle_event, );
cut end
While we create pipex(4) session, we initialize request and pass this
this timeout value to kernel as `req->pr_timeout_sec = ppp->timeout_sec'
at npppd/npppd.c:1013.
If ioctl() at npppd/npppd.c:1153 was successful and in-kernel session
was created we check `timeout_sec' and disable npppd(8) related timer at
npppd/npppd.c:1178. But this timer was not started before.
cut begin
986 pipex_setup_common(npppd_ppp *ppp, struct pipex_session_req *req)
987 {
...
1013 req->pr_timeout_sec = ppp->timeout_sec;
1040 npppd_ppp_pipex_enable(npppd *_this, npppd_ppp *ppp)
1041 {
...
1059 pipex_setup_common(ppp, );
...
1153 if ((error = ioctl(_this->iface[ppp->ifidx].devf...
...
1175 if (ppp->timeout_sec > 0) {
1176 /* Stop the npppd's idle-timer. We use
pipex's idle-timer */
1177 ppp->timeout_sec = 0;
1178 ppp_reset_idle_timeout(ppp);
1179 }
cut end
So we have two cases:
1. "idle-timeout" is null or not set in npppd.conf(5)
npppd(8) related timer is initialized, but not started, in-kernel
timeout disabled.
2. "idle-timeout" is not null in npppd.conf(5)
npppd(8) related timer is initialized, but not started, in-kernel
timeout enabled for pppac(4) sessions.
So in any cases we never enable npppd(8) related timer.
We have some troubles with pppx(4) sessions: they have two parts:
pipex(4) session and pppx(4) related context. Session is a part of this
context. With in-kernel timer we destroy session within pipex(4) layer
and we can't destroy pppx(4) related part. That's the reason we disabled
this feature for pppx(4).
I propose to kill in-kernel timeout. This simplify code and make
pppac(4) and pppx(4) session usage more identical. Also it's easy to
start using npppd(8) related timer.
Do you have objections?
Re: describe 'idle-timeout' exception in npppd.conf man page
On Sat, Aug 08, 2020 at 08:49:24PM +0900, YASUOKA Masahiko wrote: > On Fri, 7 Aug 2020 22:19:05 +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? > > I think we should mention this to the man page until we fix it. > So I'd like you to update the man page first. > > I'll try to review the problem. > Thanks. I updated my diff with changes proposed by jmc@. Are you agree with them? > > 1. > > https://cvsweb.openbsd.org/src/sys/net/if_pppx.c?rev=1.78=text/x-cvsweb-markup > > 2. https://marc.info/?l=openbsd-misc=159655468504864=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 - > > 1.27 > > +++ usr.sbin/npppd/npppd/npppd.conf.5 7 Aug 2020 19:17:00 - > > @@ -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. >
Re: describe 'idle-timeout' exception in npppd.conf man page
On Fri, 7 Aug 2020 22:19:05 +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? I think we should mention this to the man page until we fix it. So I'd like you to update the man page first. I'll try to review the problem. > 1. > https://cvsweb.openbsd.org/src/sys/net/if_pppx.c?rev=1.78=text/x-cvsweb-markup > 2. https://marc.info/?l=openbsd-misc=159655468504864=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 - 1.27 > +++ usr.sbin/npppd/npppd/npppd.conf.5 7 Aug 2020 19:17:00 - > @@ -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.
Re: describe 'idle-timeout' exception in npppd.conf man page
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=text/x-cvsweb-markup > > > 2. https://marc.info/?l=openbsd-misc=159655468504864=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 - > > > 1.27 > > > +++ usr.sbin/npppd/npppd/npppd.conf.5 7 Aug 2020 19:17:00 - > > > @@ -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.43 Apr 2020 07:46:04 - 1.12 > +++ share/man/man4/pipex.47 Aug 2020 20:54:32 - > @@ -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 - 1.27 > +++ usr.sbin/npppd/npppd/npppd.conf.5 7 Aug 2020 20:54:32 - > @@ -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 >
Re: describe 'idle-timeout' exception in npppd.conf man page
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=text/x-cvsweb-markup > > 2. https://marc.info/?l=openbsd-misc=159655468504864=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 - > > 1.27 > > +++ usr.sbin/npppd/npppd/npppd.conf.5 7 Aug 2020 19:17:00 - > > @@ -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. 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 - 1.12 +++ share/man/man4/pipex.4 7 Aug 2020 20:54:32 - @@ -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 - 1.27 +++ usr.sbin/npppd/npppd/npppd.conf.5 7 Aug 2020 20:54:32 - @@ -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
Re: describe 'idle-timeout' exception in npppd.conf man page
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=text/x-cvsweb-markup > 2. https://marc.info/?l=openbsd-misc=159655468504864=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 - 1.27 > +++ usr.sbin/npppd/npppd/npppd.conf.5 7 Aug 2020 19:17:00 - > @@ -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. > 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. 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. if the text was better placed in the idle-timeout section: This value must be 0 for .Xr pppx 4 sessions. jmc
