Re: pppoe(4) should use uptime not microtime() for tracking connection time
On Mon, Nov 22, 2021 at 01:40:34PM +, Klemens Nanni wrote:
> On Mon, Nov 22, 2021 at 09:30:13AM +0100, Claudio Jeker wrote:
> > > Index: sbin/ifconfig/ifconfig.c
> > > ===
> > > RCS file: /cvs/src/sbin/ifconfig/ifconfig.c,v
> > > retrieving revision 1.450
> > > diff -u -p -r1.450 ifconfig.c
> > > --- sbin/ifconfig/ifconfig.c 17 Nov 2021 18:00:24 - 1.450
> > > +++ sbin/ifconfig/ifconfig.c 22 Nov 2021 00:25:04 -
> > > @@ -5362,12 +5362,13 @@ pppoe_status(void)
> > > printf(" PADR retries: %d", state.padr_retry_no);
> > >
> > > if (state.state == PPPOE_STATE_SESSION) {
> > > - struct timeval temp_time;
> > > + struct timespec temp_time;
> > > time_t diff_time, day = 0;
> > > unsigned int hour = 0, min = 0, sec = 0;
> > >
> > > if (state.session_time.tv_sec != 0) {
> > > - gettimeofday(_time, NULL);
> > > + if (clock_gettime(CLOCK_BOOTTIME, _time) == -1)
> > > + goto notime;
> > > diff_time = temp_time.tv_sec -
> > > state.session_time.tv_sec;
> > >
> > > @@ -5387,6 +5388,7 @@ pppoe_status(void)
> > > printf("%lldd ", (long long)day);
> > > printf("%02u:%02u:%02u", hour, min, sec);
> > > }
> > > +notime:
> > > putchar('\n');
> > > }
> > >
> >
> > The way you call clock_gettime() it can't fail. Apart from that this is
> > the right way of fixing this. OK claudio@
>
> Yes, I inferred that from clock_gettime(9)' ERRORS as well, but all
> other CLOCK_BOOTTIME users in base do handle the error case, so I went
> along.
>
> CLOCK_MONOTONIC users in base however consistently ignore the error case
> which made me think there is some pattern I don't yet understand fully.
I think this comes from the fact that CLOCK_BOOTTIME was not available all
the time while CLOCK_MONOTONIC is more or less supported everywhere.
I doubt protability concerns matter since ifconfig(8) is highly OpenBSD
specific. Also CLOCK_BOOTTIME is the same as CLOCK_MONOTONIC on
OpenBSD (both return nanouptime()).
Anyway, commit the way you prefer. You could also put the
clock_gettime(CLOCK_BOOTTIME, _time) == -1 check in the first if
which also removes the goto:
if (state.session_time.tv_sec != 0 &&
clock_gettime(CLOCK_BOOTTIME, _time) == 0) {
...
}
--
:wq Claudio
Re: pppoe(4) should use uptime not microtime() for tracking connection time
On Mon, Nov 22, 2021 at 09:30:13AM +0100, Claudio Jeker wrote:
> > Index: sbin/ifconfig/ifconfig.c
> > ===
> > RCS file: /cvs/src/sbin/ifconfig/ifconfig.c,v
> > retrieving revision 1.450
> > diff -u -p -r1.450 ifconfig.c
> > --- sbin/ifconfig/ifconfig.c17 Nov 2021 18:00:24 - 1.450
> > +++ sbin/ifconfig/ifconfig.c22 Nov 2021 00:25:04 -
> > @@ -5362,12 +5362,13 @@ pppoe_status(void)
> > printf(" PADR retries: %d", state.padr_retry_no);
> >
> > if (state.state == PPPOE_STATE_SESSION) {
> > - struct timeval temp_time;
> > + struct timespec temp_time;
> > time_t diff_time, day = 0;
> > unsigned int hour = 0, min = 0, sec = 0;
> >
> > if (state.session_time.tv_sec != 0) {
> > - gettimeofday(_time, NULL);
> > + if (clock_gettime(CLOCK_BOOTTIME, _time) == -1)
> > + goto notime;
> > diff_time = temp_time.tv_sec -
> > state.session_time.tv_sec;
> >
> > @@ -5387,6 +5388,7 @@ pppoe_status(void)
> > printf("%lldd ", (long long)day);
> > printf("%02u:%02u:%02u", hour, min, sec);
> > }
> > +notime:
> > putchar('\n');
> > }
> >
>
> The way you call clock_gettime() it can't fail. Apart from that this is
> the right way of fixing this. OK claudio@
Yes, I inferred that from clock_gettime(9)' ERRORS as well, but all
other CLOCK_BOOTTIME users in base do handle the error case, so I went
along.
CLOCK_MONOTONIC users in base however consistently ignore the error case
which made me think there is some pattern I don't yet understand fully.
Re: pppoe(4) should use uptime not microtime() for tracking connection time
On Mon, Nov 22, 2021 at 08:17:47AM +0100, Peter J. Philipp wrote:
> On Mon, Nov 22, 2021 at 12:30:19AM +, Klemens Nanni wrote:
> > On Sun, Nov 21, 2021 at 11:18:29AM +0100, p...@delphinusdns.org wrote:
> > > >Synopsis:session uptime is wrong
> > > >Category:system
> > > >Environment:
> > > System : OpenBSD 7.0
> > > Details : OpenBSD 7.0 (GENERIC.MP) #698: Thu Sep 30 21:07:33 MDT
> > > 2021
> > >
> > > dera...@octeon.openbsd.org:/usr/src/sys/arch/octeon/compile/GENERIC.MP
> > >
> > > Architecture: OpenBSD.octeon
> > > Machine : octeon
> > > >Description:
> > > On a router (octeon with no RTC) the uptime looks like so:
> > >
> > > 11:12AM up 2 days, 15:56, 1 user, load averages: 0.01, 0.03, 0.01
> > >
> > > The pppoe(4) interface however displays 51 days uptime for a session:
> > >
> > >
> > > pppoe0: flags=8851 mtu 1500
> > > description: Telekom
> > > index 7 priority 0 llprio 3
> > > dev: vlan7 state: session
> > > sid: 0x3f2f PADI retries: 1 PADR retries: 0 time: 51d 08:03:55
> > >
> >
> > Same here on an edgerouter 4; already seen on tech@ in my reply to
> > bket's "Print learned DNS from sppp(4) in ifconfig(8)" where the
> > freshly rebooted box shows a session of 19 days in ifconfig output.
> >
> > > I reason that my router rebooted (which it did two days ago) and
> > > used microuptime() to fill the session time, and then NTP updated
> > > the time and we have this timejump. What should be done is the
> > > uptime in seconds should be gotten and the ifconfig code that does
> > > the ioctl(2) does the appropriate math.
> >
> > I can't test/reboot my box at the moment, but this minimal diff should
> > fix it. One could also rename the variables and polish further, but
> > I focus on the fix alone until I can test myself.
> >
> >
> > Index: sys/net/if_pppoe.c
> > ===
> > RCS file: /cvs/src/sys/net/if_pppoe.c,v
> > retrieving revision 1.78
> > diff -u -p -r1.78 if_pppoe.c
> > --- sys/net/if_pppoe.c 19 Jul 2021 19:00:58 - 1.78
> > +++ sys/net/if_pppoe.c 21 Nov 2021 23:50:45 -
> > @@ -586,7 +586,7 @@ breakbreak:
> > PPPOEDEBUG(("%s: session 0x%x connected\n",
> > sc->sc_sppp.pp_if.if_xname, session));
> > sc->sc_state = PPPOE_STATE_SESSION;
> > - microtime(>sc_session_time);
> > + getmicrouptime(>sc_session_time);
> > sc->sc_sppp.pp_up(>sc_sppp);/* notify upper layers
> > */
> >
> > break;
> > Index: sbin/ifconfig/ifconfig.c
> > ===
> > RCS file: /cvs/src/sbin/ifconfig/ifconfig.c,v
> > retrieving revision 1.450
> > diff -u -p -r1.450 ifconfig.c
> > --- sbin/ifconfig/ifconfig.c17 Nov 2021 18:00:24 - 1.450
> > +++ sbin/ifconfig/ifconfig.c22 Nov 2021 00:25:04 -
> > @@ -5362,12 +5362,13 @@ pppoe_status(void)
> > printf(" PADR retries: %d", state.padr_retry_no);
> >
> > if (state.state == PPPOE_STATE_SESSION) {
> > - struct timeval temp_time;
> > + struct timespec temp_time;
> > time_t diff_time, day = 0;
> > unsigned int hour = 0, min = 0, sec = 0;
> >
> > if (state.session_time.tv_sec != 0) {
> > - gettimeofday(_time, NULL);
> > + if (clock_gettime(CLOCK_BOOTTIME, _time) == -1)
> > + goto notime;
> > diff_time = temp_time.tv_sec -
> > state.session_time.tv_sec;
> >
> > @@ -5387,6 +5388,7 @@ pppoe_status(void)
> > printf("%lldd ", (long long)day);
> > printf("%02u:%02u:%02u", hour, min, sec);
> > }
> > +notime:
> > putchar('\n');
> > }
> >
>
> This looks wrong to me, is microuptime() and clock_gettime(CLOCK_BOOTTIME,
> ...)
> working on a moving uptime target?
Yes, they're both taking the monotonically increasing time since boot,
without accounting for suspend time.
> I think what one must do is instead of
> absolute timestamps is get the deltas of uptime only and then do a bit of
> math with those. What I came up with Scott Cheloha, as pseudo code is this:
>
> --->
> yes there is a getuptime() taken a value of a time_t I suspect this is seconds
> since uptime? if It passes that on ioctl to ifconfig
>
> > Then userspace will need to subtract that
> > value from the current CLOCK_BOOTTIME.
That's what I'm doing? At least as per my understanding after having
read clock_gettime(2) and microuptime(9).
> So you get another value ctime() as a time_t that's timedelta2,
> timedelta1 = (timedelta2 - clock_boottime) + ioctltime_from_kernel
> and then difftime(timedelta1, timedelta2) afaik gives you the uptime in
> seconds.
> <---
>
> I have an
Re: iwx now stops more often?
On Mon, Nov 22, 2021 at 10:58:08AM +, Laurence Tratt wrote: > On Sun, Nov 21, 2021 at 06:06:33PM +0100, Stefan Sperling wrote: > > Hello Stefan, > > >> As of a kernel from a couple of days ago, iwx semi-regularly stops > >> associating with my wireless AP. An easy way to trigger this is "pkg_add > >> -u": at some point, downloading stops mid-package, and I need to "sh > >> /etc/netstart" to bring the interface back up. > > Please try this patch. I cannot promise that it will help, but it might. > > I don't understand why, but I have since been unable to replicate the > problem, which I feel I need to do in order to meaningfully test your patch. > > I'm clutching at straws to think of possible reasons. The best I can think of > is that `pkg_add -u`'s tendency to cut downloads off mid-stream might have > caused the odd behaviour. That could be a very long way off the mark. > > If/when I can work out how to more reliably trigger the problem, I will post > back, but until then I can only apologise for the noise. This has nothing to do with pkg_add in particular. My recommendation would be to keep running with 'ifconfig iwx0 debug' enabled. The logs generated by debug mode might shed some light on the context of this problem when it occurs again. The IWX_DEBUG driver-internal flag does not really help here. It is more useful when zooming in on a driver bug, once the circumstances of the problem have been understood.
Re: iwx now stops more often?
On Sun, Nov 21, 2021 at 06:06:33PM +0100, Stefan Sperling wrote: Hello Stefan, >> As of a kernel from a couple of days ago, iwx semi-regularly stops >> associating with my wireless AP. An easy way to trigger this is "pkg_add >> -u": at some point, downloading stops mid-package, and I need to "sh >> /etc/netstart" to bring the interface back up. > Please try this patch. I cannot promise that it will help, but it might. I don't understand why, but I have since been unable to replicate the problem, which I feel I need to do in order to meaningfully test your patch. I'm clutching at straws to think of possible reasons. The best I can think of is that `pkg_add -u`'s tendency to cut downloads off mid-stream might have caused the odd behaviour. That could be a very long way off the mark. If/when I can work out how to more reliably trigger the problem, I will post back, but until then I can only apologise for the noise. Laurie
Re: pppoe(4) should use uptime not microtime() for tracking connection time
On Mon, Nov 22, 2021 at 12:30:19AM +, Klemens Nanni wrote:
> On Sun, Nov 21, 2021 at 11:18:29AM +0100, p...@delphinusdns.org wrote:
> > >Synopsis: session uptime is wrong
> > >Category: system
> > >Environment:
> > System : OpenBSD 7.0
> > Details : OpenBSD 7.0 (GENERIC.MP) #698: Thu Sep 30 21:07:33 MDT
> > 2021
> >
> > dera...@octeon.openbsd.org:/usr/src/sys/arch/octeon/compile/GENERIC.MP
> >
> > Architecture: OpenBSD.octeon
> > Machine : octeon
> > >Description:
> > On a router (octeon with no RTC) the uptime looks like so:
> >
> > 11:12AM up 2 days, 15:56, 1 user, load averages: 0.01, 0.03, 0.01
> >
> > The pppoe(4) interface however displays 51 days uptime for a session:
> >
> >
> > pppoe0: flags=8851 mtu 1500
> > description: Telekom
> > index 7 priority 0 llprio 3
> > dev: vlan7 state: session
> > sid: 0x3f2f PADI retries: 1 PADR retries: 0 time: 51d 08:03:55
> >
>
> Same here on an edgerouter 4; already seen on tech@ in my reply to
> bket's "Print learned DNS from sppp(4) in ifconfig(8)" where the
> freshly rebooted box shows a session of 19 days in ifconfig output.
>
> > I reason that my router rebooted (which it did two days ago) and
> > used microuptime() to fill the session time, and then NTP updated
> > the time and we have this timejump. What should be done is the
> > uptime in seconds should be gotten and the ifconfig code that does
> > the ioctl(2) does the appropriate math.
>
> I can't test/reboot my box at the moment, but this minimal diff should
> fix it. One could also rename the variables and polish further, but
> I focus on the fix alone until I can test myself.
>
>
> Index: sys/net/if_pppoe.c
> ===
> RCS file: /cvs/src/sys/net/if_pppoe.c,v
> retrieving revision 1.78
> diff -u -p -r1.78 if_pppoe.c
> --- sys/net/if_pppoe.c19 Jul 2021 19:00:58 - 1.78
> +++ sys/net/if_pppoe.c21 Nov 2021 23:50:45 -
> @@ -586,7 +586,7 @@ breakbreak:
> PPPOEDEBUG(("%s: session 0x%x connected\n",
> sc->sc_sppp.pp_if.if_xname, session));
> sc->sc_state = PPPOE_STATE_SESSION;
> - microtime(>sc_session_time);
> + getmicrouptime(>sc_session_time);
> sc->sc_sppp.pp_up(>sc_sppp);/* notify upper layers
> */
>
> break;
> Index: sbin/ifconfig/ifconfig.c
> ===
> RCS file: /cvs/src/sbin/ifconfig/ifconfig.c,v
> retrieving revision 1.450
> diff -u -p -r1.450 ifconfig.c
> --- sbin/ifconfig/ifconfig.c 17 Nov 2021 18:00:24 - 1.450
> +++ sbin/ifconfig/ifconfig.c 22 Nov 2021 00:25:04 -
> @@ -5362,12 +5362,13 @@ pppoe_status(void)
> printf(" PADR retries: %d", state.padr_retry_no);
>
> if (state.state == PPPOE_STATE_SESSION) {
> - struct timeval temp_time;
> + struct timespec temp_time;
> time_t diff_time, day = 0;
> unsigned int hour = 0, min = 0, sec = 0;
>
> if (state.session_time.tv_sec != 0) {
> - gettimeofday(_time, NULL);
> + if (clock_gettime(CLOCK_BOOTTIME, _time) == -1)
> + goto notime;
> diff_time = temp_time.tv_sec -
> state.session_time.tv_sec;
>
> @@ -5387,6 +5388,7 @@ pppoe_status(void)
> printf("%lldd ", (long long)day);
> printf("%02u:%02u:%02u", hour, min, sec);
> }
> +notime:
> putchar('\n');
> }
>
The way you call clock_gettime() it can't fail. Apart from that this is
the right way of fixing this. OK claudio@
--
:wq Claudio
Re: [External] : Re: pfctl ioctl sparc64 crash
Hello,
> >326 tmpkt = pfr_create_ktable(_nulltable, 0, 0,
> >
> > kt looks quite uninitialized here
>
> It looks like the intent was something like this?
>
> Index: pf_table.c
> ===
> RCS file: /cvs/src/sys/net/pf_table.c,v
> retrieving revision 1.138
> diff -u -p -U7 -r1.138 pf_table.c
> --- pf_table.c16 Nov 2021 20:51:31 - 1.138
> +++ pf_table.c22 Nov 2021 00:23:39 -
> @@ -317,16 +317,14 @@ pfr_add_addrs(struct pfr_table *tbl, str
> struct pfr_addr ad;
> int i, rv, xadd = 0;
> time_t tzero = gettime();
>
> ACCEPT_FLAGS(flags, PFR_FLAG_DUMMY | PFR_FLAG_FEEDBACK);
> if (pfr_validate_table(tbl, 0, flags & PFR_FLAG_USERIOCTL))
> return (EINVAL);
> - if (kt->pfrkt_flags & PFR_TFLAG_CONST)
> - return (EPERM);
> tmpkt = pfr_create_ktable(_nulltable, 0, 0,
> !(flags & PFR_FLAG_USERIOCTL));
> if (tmpkt == NULL)
> return (ENOMEM);
> SLIST_INIT();
> SLIST_INIT();
> for (i = 0; i < size; i++) {
> @@ -346,14 +344,19 @@ pfr_add_addrs(struct pfr_table *tbl, str
> NET_LOCK();
> PF_LOCK();
> kt = pfr_lookup_table(tbl);
> if (kt == NULL || !(kt->pfrkt_flags & PFR_TFLAG_ACTIVE)) {
> PF_UNLOCK();
> NET_UNLOCK();
> senderr(ESRCH);
> + }
> + if (kt->pfrkt_flags & PFR_TFLAG_CONST) {
> + PF_UNLOCK();
> + NET_UNLOCK();
> + senderr(EPERM);
> }
> SLIST_FOREACH(ke, , pfrke_ioq) {
> pfr_kentry_kif_ref(ke);
> p = pfr_lookup_kentry(kt, ke, 1);
> q = pfr_lookup_kentry(tmpkt, ke, 1);
> if (flags & PFR_FLAG_FEEDBACK) {
> if (q != NULL)
sure, diff above is correct. thanks for looking at it.
OK sashan
