pf: time since uptime instead of wall clock?

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

pf: time since uptime instead of wall clock?

Patrick Wildt-3
Hi,

currently the pf status struct contains the time since pf was enabled as
seen on the wall clock.  This means when time drifts, or is set to some
earlier value, the time will be off.  If we use time since uptime it
always increments and shows how long pf has been running compared to
its uptime.

Does this make sense?  Opinions?

Patrick


diff --git a/sbin/pfctl/pfctl_parser.c b/sbin/pfctl/pfctl_parser.c
index e241b11f6fc..3cb321a33e0 100644
--- a/sbin/pfctl/pfctl_parser.c
+++ b/sbin/pfctl/pfctl_parser.c
@@ -520,15 +520,17 @@ void
 print_status(struct pf_status *s, int opts)
 {
  char statline[80], *running, *debug;
- time_t runtime;
+ time_t runtime = 0;
+ struct timespec uptime;
  int i;
  char buf[PF_MD5_DIGEST_LENGTH * 2 + 1];
  static const char hex[] = "0123456789abcdef";
 
- runtime = time(NULL) - s->since;
+ if (!clock_gettime(CLOCK_UPTIME, &uptime))
+ runtime = uptime.tv_sec - s->since;
  running = s->running ? "Enabled" : "Disabled";
 
- if (s->since) {
+ if (runtime) {
  unsigned int sec, min, hrs;
  time_t day = runtime;
 
diff --git a/sys/net/pf_ioctl.c b/sys/net/pf_ioctl.c
index 56a43a55ab8..fc409a1a7d8 100644
--- a/sys/net/pf_ioctl.c
+++ b/sys/net/pf_ioctl.c
@@ -994,7 +994,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, struct proc *p)
  error = EEXIST;
  else {
  pf_status.running = 1;
- pf_status.since = time_second;
+ pf_status.since = time_uptime;
  if (pf_status.stateid == 0) {
  pf_status.stateid = time_second;
  pf_status.stateid = pf_status.stateid << 32;
@@ -1009,7 +1009,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, struct proc *p)
  error = ENOENT;
  else {
  pf_status.running = 0;
- pf_status.since = time_second;
+ pf_status.since = time_uptime;
  pf_remove_queues();
  DPFPRINTF(LOG_NOTICE, "pf: stopped");
  }
@@ -1605,7 +1605,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, struct proc *p)
  bzero(pf_status.counters, sizeof(pf_status.counters));
  bzero(pf_status.fcounters, sizeof(pf_status.fcounters));
  bzero(pf_status.scounters, sizeof(pf_status.scounters));
- pf_status.since = time_second;
+ pf_status.since = time_uptime;
 
  break;
  }
diff --git a/usr.bin/systat/pf.c b/usr.bin/systat/pf.c
index 6e282bb7359..4df9ba11b0e 100644
--- a/usr.bin/systat/pf.c
+++ b/usr.bin/systat/pf.c
@@ -220,7 +220,8 @@ void
 print_pf(void)
 {
  char *debug;
- time_t tm;
+ time_t tm = 0;
+ struct timespec uptime;
  int i;
  struct pf_status *s = &status;
 
@@ -229,7 +230,8 @@ print_pf(void)
  if (end > num_disp)
  end = num_disp;
 
- tm = time(NULL) - s->since;
+ if (!clock_gettime(CLOCK_UPTIME, &uptime))
+ tm = uptime.tv_sec - s->since;
 
  ADD_LINE_S("pf", "Status", s->running ? "Enabled" : "Disabled");
  ADD_LINE_A("pf", "Since", tm);
diff --git a/usr.sbin/snmpd/mib.c b/usr.sbin/snmpd/mib.c
index f53d9379b07..acd3b751563 100644
--- a/usr.sbin/snmpd/mib.c
+++ b/usr.sbin/snmpd/mib.c
@@ -1650,7 +1650,8 @@ int
 mib_pfinfo(struct oid *oid, struct ber_oid *o, struct ber_element **elm)
 {
  struct pf_status s;
- time_t runtime;
+ time_t runtime = 0;
+ struct timespec uptime;
  char str[11];
 
  if (pf_get_stats(&s))
@@ -1661,10 +1662,8 @@ mib_pfinfo(struct oid *oid, struct ber_oid *o, struct ber_element **elm)
  *elm = ber_add_integer(*elm, s.running);
  break;
  case 2:
- if (s.since > 0)
- runtime = time(NULL) - s.since;
- else
- runtime = 0;
+ if (!clock_gettime(CLOCK_UPTIME, &uptime))
+ runtime = uptime.tv_sec - s.since;
  runtime *= 100;
  *elm = ber_add_integer(*elm, runtime);
  ber_set_header(*elm, BER_CLASS_APPLICATION, SNMP_T_TIMETICKS);

Reply | Threaded
Open this post in threaded view
|

Re: pf: time since uptime instead of wall clock?

Patrick Wildt-3
On Tue, Mar 07, 2017 at 10:17:16AM +0100, Patrick Wildt wrote:

> Hi,
>
> currently the pf status struct contains the time since pf was enabled as
> seen on the wall clock.  This means when time drifts, or is set to some
> earlier value, the time will be off.  If we use time since uptime it
> always increments and shows how long pf has been running compared to
> its uptime.
>
> Does this make sense?  Opinions?
>
> Patrick

Alternatively it might be nicer to still use the uptime, but only return
the delta since it was enabled.

diff --git a/sbin/pfctl/pfctl_parser.c b/sbin/pfctl/pfctl_parser.c
index e241b11f6fc..ab8834e605c 100644
--- a/sbin/pfctl/pfctl_parser.c
+++ b/sbin/pfctl/pfctl_parser.c
@@ -525,7 +525,7 @@ print_status(struct pf_status *s, int opts)
  char buf[PF_MD5_DIGEST_LENGTH * 2 + 1];
  static const char hex[] = "0123456789abcdef";
 
- runtime = time(NULL) - s->since;
+ runtime = s->since;
  running = s->running ? "Enabled" : "Disabled";
 
  if (s->since) {
diff --git a/sys/net/pf_ioctl.c b/sys/net/pf_ioctl.c
index 56a43a55ab8..cd5be10e2a1 100644
--- a/sys/net/pf_ioctl.c
+++ b/sys/net/pf_ioctl.c
@@ -994,7 +994,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, struct proc *p)
  error = EEXIST;
  else {
  pf_status.running = 1;
- pf_status.since = time_second;
+ pf_status.since = time_uptime;
  if (pf_status.stateid == 0) {
  pf_status.stateid = time_second;
  pf_status.stateid = pf_status.stateid << 32;
@@ -1009,7 +1009,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, struct proc *p)
  error = ENOENT;
  else {
  pf_status.running = 0;
- pf_status.since = time_second;
+ pf_status.since = time_uptime;
  pf_remove_queues();
  DPFPRINTF(LOG_NOTICE, "pf: stopped");
  }
@@ -1577,6 +1577,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, struct proc *p)
  case DIOCGETSTATUS: {
  struct pf_status *s = (struct pf_status *)addr;
  bcopy(&pf_status, s, sizeof(struct pf_status));
+ s.since = time_uptime - pf_status.since;
  pfi_update_status(s->ifname, s);
  break;
  }
@@ -1605,7 +1606,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, struct proc *p)
  bzero(pf_status.counters, sizeof(pf_status.counters));
  bzero(pf_status.fcounters, sizeof(pf_status.fcounters));
  bzero(pf_status.scounters, sizeof(pf_status.scounters));
- pf_status.since = time_second;
+ pf_status.since = time_uptime;
 
  break;
  }
diff --git a/usr.bin/systat/pf.c b/usr.bin/systat/pf.c
index 6e282bb7359..882985e6594 100644
--- a/usr.bin/systat/pf.c
+++ b/usr.bin/systat/pf.c
@@ -229,7 +229,7 @@ print_pf(void)
  if (end > num_disp)
  end = num_disp;
 
- tm = time(NULL) - s->since;
+ tm = s->since;
 
  ADD_LINE_S("pf", "Status", s->running ? "Enabled" : "Disabled");
  ADD_LINE_A("pf", "Since", tm);
diff --git a/usr.sbin/snmpd/mib.c b/usr.sbin/snmpd/mib.c
index f53d9379b07..7fb82f22cb5 100644
--- a/usr.sbin/snmpd/mib.c
+++ b/usr.sbin/snmpd/mib.c
@@ -1650,7 +1650,6 @@ int
 mib_pfinfo(struct oid *oid, struct ber_oid *o, struct ber_element **elm)
 {
  struct pf_status s;
- time_t runtime;
  char str[11];
 
  if (pf_get_stats(&s))
@@ -1661,12 +1660,7 @@ mib_pfinfo(struct oid *oid, struct ber_oid *o, struct ber_element **elm)
  *elm = ber_add_integer(*elm, s.running);
  break;
  case 2:
- if (s.since > 0)
- runtime = time(NULL) - s.since;
- else
- runtime = 0;
- runtime *= 100;
- *elm = ber_add_integer(*elm, runtime);
+ *elm = ber_add_integer(*elm, s.since * 100);
  ber_set_header(*elm, BER_CLASS_APPLICATION, SNMP_T_TIMETICKS);
  break;
  case 3:

Reply | Threaded
Open this post in threaded view
|

Re: pf: time since uptime instead of wall clock?

Mike Belopuhov-5
On Tue, Mar 07, 2017 at 10:36 +0100, Patrick Wildt wrote:

> On Tue, Mar 07, 2017 at 10:17:16AM +0100, Patrick Wildt wrote:
> > Hi,
> >
> > currently the pf status struct contains the time since pf was enabled as
> > seen on the wall clock.  This means when time drifts, or is set to some
> > earlier value, the time will be off.  If we use time since uptime it
> > always increments and shows how long pf has been running compared to
> > its uptime.
> >
> > Does this make sense?  Opinions?
> >
> > Patrick
>
> Alternatively it might be nicer to still use the uptime, but only return
> the delta since it was enabled.
>

I see nothing wrong with this diff.  OK mikeb

> diff --git a/sbin/pfctl/pfctl_parser.c b/sbin/pfctl/pfctl_parser.c
> index e241b11f6fc..ab8834e605c 100644
> --- a/sbin/pfctl/pfctl_parser.c
> +++ b/sbin/pfctl/pfctl_parser.c
> @@ -525,7 +525,7 @@ print_status(struct pf_status *s, int opts)
>   char buf[PF_MD5_DIGEST_LENGTH * 2 + 1];
>   static const char hex[] = "0123456789abcdef";
>  
> - runtime = time(NULL) - s->since;
> + runtime = s->since;
>   running = s->running ? "Enabled" : "Disabled";
>  
>   if (s->since) {
> diff --git a/sys/net/pf_ioctl.c b/sys/net/pf_ioctl.c
> index 56a43a55ab8..cd5be10e2a1 100644
> --- a/sys/net/pf_ioctl.c
> +++ b/sys/net/pf_ioctl.c
> @@ -994,7 +994,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, struct proc *p)
>   error = EEXIST;
>   else {
>   pf_status.running = 1;
> - pf_status.since = time_second;
> + pf_status.since = time_uptime;
>   if (pf_status.stateid == 0) {
>   pf_status.stateid = time_second;
>   pf_status.stateid = pf_status.stateid << 32;
> @@ -1009,7 +1009,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, struct proc *p)
>   error = ENOENT;
>   else {
>   pf_status.running = 0;
> - pf_status.since = time_second;
> + pf_status.since = time_uptime;
>   pf_remove_queues();
>   DPFPRINTF(LOG_NOTICE, "pf: stopped");
>   }
> @@ -1577,6 +1577,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, struct proc *p)
>   case DIOCGETSTATUS: {
>   struct pf_status *s = (struct pf_status *)addr;
>   bcopy(&pf_status, s, sizeof(struct pf_status));
> + s.since = time_uptime - pf_status.since;
>   pfi_update_status(s->ifname, s);
>   break;
>   }
> @@ -1605,7 +1606,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, struct proc *p)
>   bzero(pf_status.counters, sizeof(pf_status.counters));
>   bzero(pf_status.fcounters, sizeof(pf_status.fcounters));
>   bzero(pf_status.scounters, sizeof(pf_status.scounters));
> - pf_status.since = time_second;
> + pf_status.since = time_uptime;
>  
>   break;
>   }
> diff --git a/usr.bin/systat/pf.c b/usr.bin/systat/pf.c
> index 6e282bb7359..882985e6594 100644
> --- a/usr.bin/systat/pf.c
> +++ b/usr.bin/systat/pf.c
> @@ -229,7 +229,7 @@ print_pf(void)
>   if (end > num_disp)
>   end = num_disp;
>  
> - tm = time(NULL) - s->since;
> + tm = s->since;
>  
>   ADD_LINE_S("pf", "Status", s->running ? "Enabled" : "Disabled");
>   ADD_LINE_A("pf", "Since", tm);
> diff --git a/usr.sbin/snmpd/mib.c b/usr.sbin/snmpd/mib.c
> index f53d9379b07..7fb82f22cb5 100644
> --- a/usr.sbin/snmpd/mib.c
> +++ b/usr.sbin/snmpd/mib.c
> @@ -1650,7 +1650,6 @@ int
>  mib_pfinfo(struct oid *oid, struct ber_oid *o, struct ber_element **elm)
>  {
>   struct pf_status s;
> - time_t runtime;
>   char str[11];
>  
>   if (pf_get_stats(&s))
> @@ -1661,12 +1660,7 @@ mib_pfinfo(struct oid *oid, struct ber_oid *o, struct ber_element **elm)
>   *elm = ber_add_integer(*elm, s.running);
>   break;
>   case 2:
> - if (s.since > 0)
> - runtime = time(NULL) - s.since;
> - else
> - runtime = 0;
> - runtime *= 100;
> - *elm = ber_add_integer(*elm, runtime);
> + *elm = ber_add_integer(*elm, s.since * 100);
>   ber_set_header(*elm, BER_CLASS_APPLICATION, SNMP_T_TIMETICKS);
>   break;
>   case 3:
>

Reply | Threaded
Open this post in threaded view
|

Re: pf: time since uptime instead of wall clock?

Patrick Wildt-3
On Mon, Mar 13, 2017 at 02:33:02PM +0100, Mike Belopuhov wrote:

> On Tue, Mar 07, 2017 at 10:36 +0100, Patrick Wildt wrote:
> > On Tue, Mar 07, 2017 at 10:17:16AM +0100, Patrick Wildt wrote:
> > > Hi,
> > >
> > > currently the pf status struct contains the time since pf was enabled as
> > > seen on the wall clock.  This means when time drifts, or is set to some
> > > earlier value, the time will be off.  If we use time since uptime it
> > > always increments and shows how long pf has been running compared to
> > > its uptime.
> > >
> > > Does this make sense?  Opinions?
> > >
> > > Patrick
> >
> > Alternatively it might be nicer to still use the uptime, but only return
> > the delta since it was enabled.
> >
>
> I see nothing wrong with this diff.  OK mikeb

On the one where we return the delta instead of an absolute time?

>
> > diff --git a/sbin/pfctl/pfctl_parser.c b/sbin/pfctl/pfctl_parser.c
> > index e241b11f6fc..ab8834e605c 100644
> > --- a/sbin/pfctl/pfctl_parser.c
> > +++ b/sbin/pfctl/pfctl_parser.c
> > @@ -525,7 +525,7 @@ print_status(struct pf_status *s, int opts)
> >   char buf[PF_MD5_DIGEST_LENGTH * 2 + 1];
> >   static const char hex[] = "0123456789abcdef";
> >  
> > - runtime = time(NULL) - s->since;
> > + runtime = s->since;
> >   running = s->running ? "Enabled" : "Disabled";
> >  
> >   if (s->since) {
> > diff --git a/sys/net/pf_ioctl.c b/sys/net/pf_ioctl.c
> > index 56a43a55ab8..cd5be10e2a1 100644
> > --- a/sys/net/pf_ioctl.c
> > +++ b/sys/net/pf_ioctl.c
> > @@ -994,7 +994,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, struct proc *p)
> >   error = EEXIST;
> >   else {
> >   pf_status.running = 1;
> > - pf_status.since = time_second;
> > + pf_status.since = time_uptime;
> >   if (pf_status.stateid == 0) {
> >   pf_status.stateid = time_second;
> >   pf_status.stateid = pf_status.stateid << 32;
> > @@ -1009,7 +1009,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, struct proc *p)
> >   error = ENOENT;
> >   else {
> >   pf_status.running = 0;
> > - pf_status.since = time_second;
> > + pf_status.since = time_uptime;
> >   pf_remove_queues();
> >   DPFPRINTF(LOG_NOTICE, "pf: stopped");
> >   }
> > @@ -1577,6 +1577,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, struct proc *p)
> >   case DIOCGETSTATUS: {
> >   struct pf_status *s = (struct pf_status *)addr;
> >   bcopy(&pf_status, s, sizeof(struct pf_status));
> > + s.since = time_uptime - pf_status.since;
> >   pfi_update_status(s->ifname, s);
> >   break;
> >   }
> > @@ -1605,7 +1606,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, struct proc *p)
> >   bzero(pf_status.counters, sizeof(pf_status.counters));
> >   bzero(pf_status.fcounters, sizeof(pf_status.fcounters));
> >   bzero(pf_status.scounters, sizeof(pf_status.scounters));
> > - pf_status.since = time_second;
> > + pf_status.since = time_uptime;
> >  
> >   break;
> >   }
> > diff --git a/usr.bin/systat/pf.c b/usr.bin/systat/pf.c
> > index 6e282bb7359..882985e6594 100644
> > --- a/usr.bin/systat/pf.c
> > +++ b/usr.bin/systat/pf.c
> > @@ -229,7 +229,7 @@ print_pf(void)
> >   if (end > num_disp)
> >   end = num_disp;
> >  
> > - tm = time(NULL) - s->since;
> > + tm = s->since;
> >  
> >   ADD_LINE_S("pf", "Status", s->running ? "Enabled" : "Disabled");
> >   ADD_LINE_A("pf", "Since", tm);
> > diff --git a/usr.sbin/snmpd/mib.c b/usr.sbin/snmpd/mib.c
> > index f53d9379b07..7fb82f22cb5 100644
> > --- a/usr.sbin/snmpd/mib.c
> > +++ b/usr.sbin/snmpd/mib.c
> > @@ -1650,7 +1650,6 @@ int
> >  mib_pfinfo(struct oid *oid, struct ber_oid *o, struct ber_element **elm)
> >  {
> >   struct pf_status s;
> > - time_t runtime;
> >   char str[11];
> >  
> >   if (pf_get_stats(&s))
> > @@ -1661,12 +1660,7 @@ mib_pfinfo(struct oid *oid, struct ber_oid *o, struct ber_element **elm)
> >   *elm = ber_add_integer(*elm, s.running);
> >   break;
> >   case 2:
> > - if (s.since > 0)
> > - runtime = time(NULL) - s.since;
> > - else
> > - runtime = 0;
> > - runtime *= 100;
> > - *elm = ber_add_integer(*elm, runtime);
> > + *elm = ber_add_integer(*elm, s.since * 100);
> >   ber_set_header(*elm, BER_CLASS_APPLICATION, SNMP_T_TIMETICKS);
> >   break;
> >   case 3:
> >
>

Reply | Threaded
Open this post in threaded view
|

Re: pf: time since uptime instead of wall clock?

Mike Belopuhov-5
On 13 March 2017 at 15:09, Patrick Wildt <[hidden email]> wrote:

> On Mon, Mar 13, 2017 at 02:33:02PM +0100, Mike Belopuhov wrote:
>> On Tue, Mar 07, 2017 at 10:36 +0100, Patrick Wildt wrote:
>> > On Tue, Mar 07, 2017 at 10:17:16AM +0100, Patrick Wildt wrote:
>> > > Hi,
>> > >
>> > > currently the pf status struct contains the time since pf was enabled as
>> > > seen on the wall clock.  This means when time drifts, or is set to some
>> > > earlier value, the time will be off.  If we use time since uptime it
>> > > always increments and shows how long pf has been running compared to
>> > > its uptime.
>> > >
>> > > Does this make sense?  Opinions?
>> > >
>> > > Patrick
>> >
>> > Alternatively it might be nicer to still use the uptime, but only return
>> > the delta since it was enabled.
>> >
>>
>> I see nothing wrong with this diff.  OK mikeb
>
> On the one where we return the delta instead of an absolute time?
>

It's only a status. You might have to go through ports that expect a
timestamp and fix those, but that's a different question :-)

Reply | Threaded
Open this post in threaded view
|

Re: pf: time since uptime instead of wall clock?

Patrick Wildt-3
On Mon, Mar 13, 2017 at 03:14:25PM +0100, Mike Belopuhov wrote:

> On 13 March 2017 at 15:09, Patrick Wildt <[hidden email]> wrote:
> > On Mon, Mar 13, 2017 at 02:33:02PM +0100, Mike Belopuhov wrote:
> >> On Tue, Mar 07, 2017 at 10:36 +0100, Patrick Wildt wrote:
> >> > On Tue, Mar 07, 2017 at 10:17:16AM +0100, Patrick Wildt wrote:
> >> > > Hi,
> >> > >
> >> > > currently the pf status struct contains the time since pf was enabled as
> >> > > seen on the wall clock.  This means when time drifts, or is set to some
> >> > > earlier value, the time will be off.  If we use time since uptime it
> >> > > always increments and shows how long pf has been running compared to
> >> > > its uptime.
> >> > >
> >> > > Does this make sense?  Opinions?
> >> > >
> >> > > Patrick
> >> >
> >> > Alternatively it might be nicer to still use the uptime, but only return
> >> > the delta since it was enabled.
> >> >
> >>
> >> I see nothing wrong with this diff.  OK mikeb
> >
> > On the one where we return the delta instead of an absolute time?
> >
>
> It's only a status. You might have to go through ports that expect a
> timestamp and fix those, but that's a different question :-)
>

Thanks to sthen@ I now know that 7 ports are using pf_status.  Only one
of them uses the "since" attribute, and it does so for internally
comparing timestamps to not calculate a diff between the current and
a previous value if the pf has since been toggled.

I wonder if this diff would work, but I don't feel it's reliable enough
to provide the same feature.

Patrick

diff --git a/net/pfstat/Makefile b/net/pfstat/Makefile
index 1f1cd6296b2..33aab9c8422 100644
--- a/net/pfstat/Makefile
+++ b/net/pfstat/Makefile
@@ -9,8 +9,8 @@ PKGNAME-main= ${DISTNAME}
 PKGNAME-daemon= ${DISTNAME:S/-/d-/}
 CATEGORIES= net
 MASTER_SITES= http://www.benzedrine.ch/
-REVISION-daemon=1
-REVISION-main= 1
+REVISION-daemon=2
+REVISION-main= 2
 
 HOMEPAGE= http://www.benzedrine.ch/pfstat.html
 
diff --git a/net/pfstat/patches/patch-pf_c b/net/pfstat/patches/patch-pf_c
index 8b9564e1ac8..0654883d56d 100644
--- a/net/pfstat/patches/patch-pf_c
+++ b/net/pfstat/patches/patch-pf_c
@@ -1,6 +1,6 @@
 $OpenBSD: patch-pf_c,v 1.2 2014/04/22 10:56:37 jca Exp $
---- pf.c.orig Tue Apr 22 05:08:25 2014
-+++ pf.c Tue Apr 22 05:10:01 2014
+--- pf.c.orig Thu Jan 11 17:01:58 2007
++++ pf.c Thu Mar 16 14:16:00 2017
 @@ -38,10 +38,12 @@ static const char rcsid[] = "$Id: pf.c,v 1.1.1.1 2007/
  #include <netinet/in.h>
  #include <net/if.h>
@@ -14,7 +14,11 @@ $OpenBSD: patch-pf_c,v 1.2 2014/04/22 10:56:37 jca Exp $
  #include <arpa/inet.h>
  #include <err.h>
  #include <errno.h>
-@@ -53,6 +55,7 @@ static const char rcsid[] = "$Id: pf.c,v 1.1.1.1 2007/
+@@ -50,9 +52,11 @@ static const char rcsid[] = "$Id: pf.c,v 1.1.1.1 2007/
+ #include <stdlib.h>
+ #include <string.h>
+ #include <unistd.h>
++#include <time.h>
 
  #include "pf.h"
 
@@ -22,7 +26,7 @@ $OpenBSD: patch-pf_c,v 1.2 2014/04/22 10:56:37 jca Exp $
  union altq_stats {
  class_stats_t cbq;
  struct priq_classstats priq;
-@@ -138,6 +141,7 @@ query_queues(int fd, void (*cb)(int, const char *, int
+@@ -138,6 +142,7 @@ query_queues(int fd, void (*cb)(int, const char *, int
  }
  return (0);
  }
@@ -30,7 +34,28 @@ $OpenBSD: patch-pf_c,v 1.2 2014/04/22 10:56:37 jca Exp $
 
  static int
  query_ifaces(int fd, void (*cb)(int, const char *, int, double))
-@@ -195,9 +199,11 @@ pf_query(int fd, void (*cb)(int, const char *, int, do
+@@ -168,6 +173,7 @@ static int
+ query_counters(int fd, void (*cb)(int, const char *, int, double))
+ {
+ struct pf_status s;
++ struct timespec uptime;
+ int i;
+
+ memset(&s, 0, sizeof(s));
+@@ -175,7 +181,11 @@ query_counters(int fd, void (*cb)(int, const char *, i
+ fprintf(stderr, "ioctl: DIOCGETSTATUS: %s\n", strerror(errno));
+ return (1);
+ }
+- (*cb)(COL_TYPE_SINCE, "", 0, s.since);
++ if (clock_gettime(CLOCK_UPTIME, &uptime)) {
++ fprintf(stderr, "query_counters: clock_gettime() failed\n");
++ return (1);
++ }
++ (*cb)(COL_TYPE_SINCE, "", 0, uptime.tv_sec - s.since);
+ (*cb)(COL_TYPE_GLOBAL, "", 0, s.states);
+ for (i = 0; i < FCNT_MAX; ++i)
+ (*cb)(COL_TYPE_GLOBAL, "", 1 + i, s.fcounters[i]);
+@@ -195,9 +205,11 @@ pf_query(int fd, void (*cb)(int, const char *, int, do
  fprintf(stderr, "pf_query: query_ifaces() failed\n");
  return (1);
  }

Reply | Threaded
Open this post in threaded view
|

Re: pf: time since uptime instead of wall clock?

Alexander Bluhm
In reply to this post by Patrick Wildt-3
On Tue, Mar 07, 2017 at 10:17:16AM +0100, Patrick Wildt wrote:
> currently the pf status struct contains the time since pf was enabled as
> seen on the wall clock.  This means when time drifts, or is set to some
> earlier value, the time will be off.  If we use time since uptime it
> always increments and shows how long pf has been running compared to
> its uptime.
>
> Does this make sense?  Opinions?

Kernel interfaces should not return time deltas.  Userland cannot
measure the moment when the syscall was made, so there is always
uncertainty about the scheduler delay.  Returning time_second is
bad as it changes when wall-clock time is set.  Passing values
containing time_uptime is what we want, this is what the diff does.

Switching from time_second to time_uptime based values also produces
less fallout in ports.

OK bluhm@, commit after unlock

> diff --git a/sbin/pfctl/pfctl_parser.c b/sbin/pfctl/pfctl_parser.c
> index e241b11f6fc..3cb321a33e0 100644
> --- a/sbin/pfctl/pfctl_parser.c
> +++ b/sbin/pfctl/pfctl_parser.c
> @@ -520,15 +520,17 @@ void
>  print_status(struct pf_status *s, int opts)
>  {
>   char statline[80], *running, *debug;
> - time_t runtime;
> + time_t runtime = 0;
> + struct timespec uptime;
>   int i;
>   char buf[PF_MD5_DIGEST_LENGTH * 2 + 1];
>   static const char hex[] = "0123456789abcdef";
>  
> - runtime = time(NULL) - s->since;
> + if (!clock_gettime(CLOCK_UPTIME, &uptime))
> + runtime = uptime.tv_sec - s->since;
>   running = s->running ? "Enabled" : "Disabled";
>  
> - if (s->since) {
> + if (runtime) {
>   unsigned int sec, min, hrs;
>   time_t day = runtime;
>  
> diff --git a/sys/net/pf_ioctl.c b/sys/net/pf_ioctl.c
> index 56a43a55ab8..fc409a1a7d8 100644
> --- a/sys/net/pf_ioctl.c
> +++ b/sys/net/pf_ioctl.c
> @@ -994,7 +994,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, struct proc *p)
>   error = EEXIST;
>   else {
>   pf_status.running = 1;
> - pf_status.since = time_second;
> + pf_status.since = time_uptime;
>   if (pf_status.stateid == 0) {
>   pf_status.stateid = time_second;
>   pf_status.stateid = pf_status.stateid << 32;
> @@ -1009,7 +1009,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, struct proc *p)
>   error = ENOENT;
>   else {
>   pf_status.running = 0;
> - pf_status.since = time_second;
> + pf_status.since = time_uptime;
>   pf_remove_queues();
>   DPFPRINTF(LOG_NOTICE, "pf: stopped");
>   }
> @@ -1605,7 +1605,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, struct proc *p)
>   bzero(pf_status.counters, sizeof(pf_status.counters));
>   bzero(pf_status.fcounters, sizeof(pf_status.fcounters));
>   bzero(pf_status.scounters, sizeof(pf_status.scounters));
> - pf_status.since = time_second;
> + pf_status.since = time_uptime;
>  
>   break;
>   }
> diff --git a/usr.bin/systat/pf.c b/usr.bin/systat/pf.c
> index 6e282bb7359..4df9ba11b0e 100644
> --- a/usr.bin/systat/pf.c
> +++ b/usr.bin/systat/pf.c
> @@ -220,7 +220,8 @@ void
>  print_pf(void)
>  {
>   char *debug;
> - time_t tm;
> + time_t tm = 0;
> + struct timespec uptime;
>   int i;
>   struct pf_status *s = &status;
>  
> @@ -229,7 +230,8 @@ print_pf(void)
>   if (end > num_disp)
>   end = num_disp;
>  
> - tm = time(NULL) - s->since;
> + if (!clock_gettime(CLOCK_UPTIME, &uptime))
> + tm = uptime.tv_sec - s->since;
>  
>   ADD_LINE_S("pf", "Status", s->running ? "Enabled" : "Disabled");
>   ADD_LINE_A("pf", "Since", tm);
> diff --git a/usr.sbin/snmpd/mib.c b/usr.sbin/snmpd/mib.c
> index f53d9379b07..acd3b751563 100644
> --- a/usr.sbin/snmpd/mib.c
> +++ b/usr.sbin/snmpd/mib.c
> @@ -1650,7 +1650,8 @@ int
>  mib_pfinfo(struct oid *oid, struct ber_oid *o, struct ber_element **elm)
>  {
>   struct pf_status s;
> - time_t runtime;
> + time_t runtime = 0;
> + struct timespec uptime;
>   char str[11];
>  
>   if (pf_get_stats(&s))
> @@ -1661,10 +1662,8 @@ mib_pfinfo(struct oid *oid, struct ber_oid *o, struct ber_element **elm)
>   *elm = ber_add_integer(*elm, s.running);
>   break;
>   case 2:
> - if (s.since > 0)
> - runtime = time(NULL) - s.since;
> - else
> - runtime = 0;
> + if (!clock_gettime(CLOCK_UPTIME, &uptime))
> + runtime = uptime.tv_sec - s.since;
>   runtime *= 100;
>   *elm = ber_add_integer(*elm, runtime);
>   ber_set_header(*elm, BER_CLASS_APPLICATION, SNMP_T_TIMETICKS);