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 <m...@openbsd.org> 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, &tv);
---- 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, &req);
...
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?
