PF once rule should not trigger removal of parent anchor rule

[Alexandr Nedvedicky]

Hello,

Petr Hoffmann at Oracle discovered a glitch in ONCE rules and anchors.
Petr's test case, which shows a misbehavior looks as follows:

echo 'anchor "foo/*"' | pfctl -f -
pfctl -sr
# anchor "foo/*" all

echo 'pass' | pfctl -a foo/bar -f -
echo 'pass once' | pfctl -a foo/once-bar -f -
pfctl -a "*" -sr
# anchor "foo/*" all {
#   anchor "bar" all {
# pass all flags S/SA
#   }
#   anchor "once-bar" all {
# pass all flags S/SA once
#   }
# }


# let some network traffic to hit once rule. any packet will do
# show rules in main anchor:
pfctl -a "*" -sr
# 
# What the hell is going on here?! why the rules are gone?

We believe it is a bug. Same bug is found in older versions of PF, before
change 'Let purge thread to remove once rules, not packets.' got in. The change
which has been introduced at g2k16 in Cambridge (pf.c @ 1.983).

We think PF should not be attempting to remove anchor rule, which points at
ONCE rule being removed. Though we still need to remove ruleset, if it becomes
empty after 'once rule' has been removed. The fix comes from Dilli Paudel at
Oracle.  After applying patch below, we get expected behavior.

# Once expired rule gets purged we get expected output
pfctl -a "*" -sr
# anchor "foo/*" all {
#   anchor "bar" all {
# pass all flags S/SA
#   }
# }

OK?

thanks and
regards
sasha

8<---8<---8<--8<
diff -r 5b09c9f9c654 src/sys/net/pf.c
--- a/src/sys/net/pf.c  Fri Oct 21 00:17:58 2016 +0200
+++ b/src/sys/net/pf.c  Fri Oct 21 00:28:29 2016 +0200
@@ -3857,12 +3857,6 @@
 #endif /* NPFSYNC > 0 */
 
if (r->rule_flag & PFRULE_ONCE) {
-   if ((a != NULL) && TAILQ_EMPTY(a->ruleset->rules.active.ptr)) {
-   a->rule_flag |= PFRULE_EXPIRED;
-   a->exptime = time_second;
-   SLIST_INSERT_HEAD(_rule_gcl, a, gcle);
-   }
-
r->rule_flag |= PFRULE_EXPIRED;
r->exptime = time_second;
SLIST_INSERT_HEAD(_rule_gcl, r, gcle);
diff -r 5b09c9f9c654 src/sys/net/pf_ioctl.c
--- a/src/sys/net/pf_ioctl.cFri Oct 21 00:17:58 2016 +0200
+++ b/src/sys/net/pf_ioctl.cFri Oct 21 00:28:29 2016 +0200
@@ -315,6 +315,7 @@
rule->nr = nr++;
ruleset->rules.active.ticket++;
pf_calc_skip_steps(ruleset->rules.active.ptr);
+   pf_remove_if_empty_ruleset(ruleset);
 }
 
 u_int16_t

Re: let PF to send challenge ack

[Alexandr Nedvedicky]

Hello,

On Thu, Oct 20, 2016 at 08:45:09PM +0200, Alexander Bluhm wrote:
> On Fri, Sep 30, 2016 at 11:55:48PM +0200, Alexandr Nedvedicky wrote:
> > The patch makes PF to send 'challenge ACK' for SYN packet, which matches
> > session in established state.
> 
> regress/sys/net/pf_forward has found a bug in your code.  Looks
> like the route-to feature was affected.  By splitting the if
> expression and keeping the return (PF_DROP) in the outer block,
> much more packets than before were dropped.
> 

thanks for catching that. Now I remember I've deliberately disabled
run-regress-tcp tests. They were failing and failures were standing in
my way to debug challenge_ack.py bits. I've forgot to uncomment them
later in order to investigate those failures.

I've just commit the challenge ACK patch to pf including your fix.

thanks again for catching my bad just in time.

regards
sasha

ipv6 empty fragment

[Alexander Bluhm]

Hi,

Empty IPv6 fragments are reassembled differently by our stack and
pf.  If the payload length is 0, it does not change the content of
the fragment cache.  So pf just drops it early during processing.
But IPv6 requires that when an overlapping fragment is detected,
the whole queue of the fragement is dropped.  That is what our stack
thinks about such a fragment, which is next to an existing fragment
entry.

I think the pf way is smarter.  An empty fragment can never overlap
existing content, there is no ambiguous payload.  Just dropping it
costs less resources than trying to insert it in the queue.

To make the behavior uniform, I want to adapt the IPv6 network
stack.

ok?

bluhm

Index: netinet6/frag6.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/frag6.c,v
retrieving revision 1.69
diff -u -p -r1.69 frag6.c
--- netinet6/frag6.c24 Aug 2016 09:41:12 -  1.69
+++ netinet6/frag6.c20 Oct 2016 22:40:21 -
@@ -208,6 +208,12 @@ frag6_input(struct mbuf **mp, int *offp,
return ip6f->ip6f_nxt;
}
 
+   /* Ignore empty non atomic fragment, do not classify as overlapping. */
+   if (sizeof(struct ip6_hdr) + ntohs(ip6->ip6_plen) <= offset) {
+   m_freem(m);
+   return IPPROTO_DONE;
+   }
+
IP6Q_LOCK();
 
/*

nsd 4.1.13

[Florian Obser]

I have been prodded by dhill & brad, so here is a diff.
Running with it now but haven't reviewed it, yet.

(I have a git repo with all intermediate diffs if someone wants to
look at those...)

Tests / OKs? (I will review the diff myself before committing)

diff --git Makefile.in Makefile.in
index 3fbd01b..3391cd0 100644
--- Makefile.in
+++ Makefile.in
@@ -317,7 +317,7 @@ DEPEND_TMP2=depend1074.tmp
 DEPEND_TARGET=Makefile
 DEPEND_TARGET2=Makefile.in
 depend:
-   (cd $(srcdir) ; $(CC) -MM $(CPPFLAGS) *.c compat/*.c tpkg/cutest/*.c) | 
\
+   (cd $(srcdir) ; $(CC) -MM $(CPPFLAGS) *.c compat/*.c `if test -d 
tpkg/cutest; then echo tpkg/cutest/*.c; fi`) | \
sed -e 's? *\([^ ]*\.[ch]\)? $$(srcdir)/\1?g' | \
sed -e 's?$$(srcdir)/config.h?config.h?g' \
-e 's?$$(srcdir)/configlexer.c?configlexer.c?g' \
diff --git axfr.c axfr.c
index 09eb082..92d4f2f 100644
--- axfr.c
+++ axfr.c
@@ -91,7 +91,7 @@ query_axfr(struct nsd *nsd, struct query *query)
query->edns.status = EDNS_NOT_PRESENT;
buffer_set_limit(query->packet, QHEADERSZ);
QDCOUNT_SET(query->packet, 0);
-   query_prepare_response(query, nsd);
+   query_prepare_response(query);
}
 
/* Add zone RRs until answer is full.  */
diff --git configlexer.lex configlexer.lex
index d536352..0ce80bd 100644
--- configlexer.lex
+++ configlexer.lex
@@ -7,6 +7,8 @@
  * See LICENSE for the license.
  *
  */
+/* because flex keeps having sign-unsigned compare problems that are unfixed*/
+#pragma GCC diagnostic ignored "-Wsign-compare"
 
 #include "config.h"
 
@@ -273,6 +275,7 @@ max-refresh-time{COLON} { LEXOUT(("v(%s) ", yytext)); 
return VAR_MAX_REFRESH_TIM
 min-refresh-time{COLON}{ LEXOUT(("v(%s) ", yytext)); return 
VAR_MIN_REFRESH_TIME;}
 max-retry-time{COLON}  { LEXOUT(("v(%s) ", yytext)); return 
VAR_MAX_RETRY_TIME;}
 min-retry-time{COLON}  { LEXOUT(("v(%s) ", yytext)); return 
VAR_MIN_RETRY_TIME;}
+multi-master-check{COLON}  { LEXOUT(("v(%s) ", yytext)); return 
VAR_MULTI_MASTER_CHECK;}
 {NEWLINE}  { LEXOUT(("NL\n")); cfg_parser->line++;}
 
/* Quoted strings. Strip leading and ending quotes */
diff --git configparser.y configparser.y
index 9089665..204d236 100644
--- configparser.y
+++ configparser.y
@@ -71,6 +71,7 @@ extern config_parser_state_t* cfg_parser;
 %token VAR_ROUND_ROBIN VAR_ZONESTATS VAR_REUSEPORT VAR_VERSION
 %token VAR_MAX_REFRESH_TIME VAR_MIN_REFRESH_TIME
 %token VAR_MAX_RETRY_TIME VAR_MIN_RETRY_TIME
+%token VAR_MULTI_MASTER_CHECK
 
 %%
 toplevelvars: /* empty */ | toplevelvars toplevelvar ;
@@ -602,7 +603,7 @@ zone_config_item: zone_zonefile | zone_allow_notify | 
zone_request_xfr |
zone_outgoing_interface | zone_allow_axfr_fallback | include_pattern |
zone_rrl_whitelist | zone_zonestats | zone_max_refresh_time |
zone_min_refresh_time | zone_max_retry_time | zone_min_retry_time |
-   zone_size_limit_xfr;
+   zone_size_limit_xfr | zone_multi_master_check;
 pattern_name: VAR_NAME STRING
{ 
OUTYY(("P(pattern_name:%s)\n", $2)); 
@@ -871,6 +872,13 @@ zone_min_retry_time: VAR_MIN_RETRY_TIME STRING
cfg_parser->current_pattern->min_retry_time_is_default = 0;
}
 };
+zone_multi_master_check: VAR_MULTI_MASTER_CHECK STRING
+   {
+   OUTYY(("P(zone_multi_master_check:%s)\n", $2));
+   if(strcmp($2, "yes") != 0 && strcmp($2, "no") != 0)
+   yyerror("expected yes or no.");
+   else cfg_parser->current_pattern->multi_master_check = 
(strcmp($2, "yes")==0);
+   }
 
 /* key: declaration */
 keystart: VAR_KEY
@@ -883,6 +891,7 @@ keystart: VAR_KEY
key_options_insert(cfg_parser->opt, 
cfg_parser->current_key);
}
cfg_parser->current_key = 
key_options_create(cfg_parser->opt->region);
+   cfg_parser->current_key->algorithm = 
region_strdup(cfg_parser->opt->region, "sha256");
}
;
 contents_key: contents_key content_key | content_key;
@@ -907,6 +916,8 @@ key_algorithm: VAR_ALGORITHM STRING
 #ifndef NDEBUG
assert(cfg_parser->current_key);
 #endif
+   if(cfg_parser->current_key->algorithm)
+   region_recycle(cfg_parser->opt->region, 
cfg_parser->current_key->algorithm, 
strlen(cfg_parser->current_key->algorithm)+1);
cfg_parser->current_key->algorithm = 
region_strdup(cfg_parser->opt->region, $2);
if(tsig_get_algorithm_by_name($2) == NULL)
c_error_msg("Bad tsig algorithm %s", $2);
diff --git configure configure
index 4ac0670..41bb666 100644
--- configure
+++ configure
@@ -1,6 +1,6 @@
 #! /bin/sh
 # Guess values for system-dependent variables and create Makefiles.
-# Generated by GNU Autoconf 2.69 for NSD 4.1.12.
+# Generated by GNU Autoconf 2.69 for NSD 4.1.13.
 #
 # Report 

Re: switchd manual pages minor diff

[Jason McIntyre]

On Wed, Oct 19, 2016 at 03:10:53PM +0300, Kapetanakis Giannis wrote:
> Hi,
> 
> just a minor change to manual pages of switch daemon.
> 
> G
> 

fixed, thanks, but note:

> 
> Index: switchd.8
> ===
> RCS file: /cvs/src/usr.sbin/switchd/switchd.8,v
> retrieving revision 1.2
> diff -u -p -r1.2 switchd.8
> --- switchd.8   25 Sep 2016 23:05:29 -  1.2
> +++ switchd.8   19 Oct 2016 12:08:36 -
> @@ -68,6 +68,9 @@ options increase the verbosity.
>  .It Pa /etc/switchd.conf
>  Default configuration file.
>  .El
> +.Sh SEE ALSO
> +.Xr switchd.conf 5 ,
> +.Xr switchctl 8
>  .Sh STANDARDS
>  .Rs
>  .%A Open Networking Foundation (ONF)
> Index: switchd.conf.5
> ===
> RCS file: /cvs/src/usr.sbin/switchd/switchd.conf.5,v
> retrieving revision 1.3
> diff -u -p -r1.3 switchd.conf.5
> --- switchd.conf.5  20 Jul 2016 07:21:24 -  1.3
> +++ switchd.conf.5  19 Oct 2016 12:08:36 -
> @@ -112,4 +112,5 @@ listen on 0.0.0.0 port 6633
>  .\"device "/dev/switch1" forward to tcp:192.168.0.1:6633
>  .Ed
>  .Sh SEE ALSO
> +.Xr switchctl 8 ,
>  .Xr switchd 8
> 
> Index: switchctl.8
> ===
> RCS file: /cvs/src/usr.sbin/switchctl/switchctl.8,v
> retrieving revision 1.2
> diff -u -p -r1.2 switchctl.8
> --- switchctl.8 12 Oct 2016 19:07:42 -  1.2
> +++ switchctl.8 19 Oct 2016 12:09:09 -
> @@ -100,7 +100,8 @@ socket used for communication with
>  .Xr switchd 8
>  .El
>  .Sh SEE ALSO
> -.Xr bridge 4
> +.Xr bridge 4 ,
> +.Xr switchd.conf 8 ,

section 5, not 8/

>  .Xr switchd 8
>  .Sh HISTORY
>  The
> 

jmc

Re: vm.conf(5) manual tweak for switches

[Jason McIntyre]

On Sun, Oct 16, 2016 at 07:15:41PM +0100, Edd Barrett wrote:
> Hi,
> 
> In vm.conf(5):
> 
> ---8<---
> Virtual switches can be configured at any point in the configuration
> file; they allow switchd to add network interfaces of VMs to the
> underlying switch interfaces automatically.
> --->8---
> 
> This confused me, since i've been using virtual switches without
> switchd. I have a suspicion that "switchd" was supposed to be "vmd" in
> that sentence (?).
> 
> The following diff attempts to fix this, and tweaks the surrounding text
> a bit too.
> 
> Comments, OK?
> 

fixed, thanks.
jmc

> 
> Index: vm.conf.5
> ===
> RCS file: /home/edd/cvsync/src/usr.sbin/vmd/vm.conf.5,v
> retrieving revision 1.8
> diff -u -p -r1.8 vm.conf.5
> --- vm.conf.5 15 Oct 2016 14:02:11 -  1.8
> +++ vm.conf.5 16 Oct 2016 18:14:00 -
> @@ -165,23 +165,36 @@ is greater than the number of
>  statements, additional default interfaces will be added.
>  .El
>  .Sh SWITCH CONFIGURATION
> -Virtual switches can be configured at any point in the configuration file;
> -they allow
> -.Nm switchd
> -to add network interfaces of VMs to the underlying switch interfaces
> -automatically.
> -It is possible to pre-configure switch interfaces using
> +A virtual switch allows VMs to communicate with other network interfaces on 
> the
> +host system via either
> +.Xr bridge 4
> +or
> +.Xr switch 4 .
> +The network interface for each virtual switch defined in
> +.Nm
> +is automatically created by
> +.Xr vmd 8 ,
> +but it is also possible to pre-configure switch interfaces using
>  .Xr hostname.if 5
>  or
> -.Xr ifconfig 8 ,
> -see the sections
> +.Xr ifconfig 8
> +(see the
>  .Sx BRIDGE
> -or
> +and
>  .Sx SWITCH
> -in
> +sections in
>  .Xr ifconfig 8
> -accordingly.
> +accordingly).
> +When a VM is started, virtual network interfaces which are assigned to a
> +virtual switch have their
> +.Xr tap 4
> +interface automatically added into the corresponding
> +.Xr bridge 4
> +or
> +.Xr switch 4
> +interface underlying the virtual switch.
>  .Pp
> +Virtual switches can be configured at any point in the configuration file.
>  Each
>  .Ic switch
>  section starts with a declaration of the virtual switch:
> 
> -- 
> Best Regards
> Edd Barrett
> 
> http://www.theunixzoo.co.uk
> 

Re: log mutex

[Mark Kettenis]

> Date: Thu, 20 Oct 2016 15:42:32 +0200
> From: Alexander Bluhm 
> 
> Hi,
> 
> A while ago I made kernel logging interrupt safe by adding a
> splhigh().  When we are going MP this is not sufficient, so replace
> it with a mutex.  The idea is to hold the mutex every time msgbufp
> is dereferenced.  This allows to print to dmesg without kernel lock.
> 
> Note that we take the mutex for every character.  That should be
> not performance critical as when you log too much in kernel, you
> have other problems anyway.
> 
> There is still a race with logsoftc.sc_state |= LOG_RDWAIT, but I
> want to address that separately.
> 
> ok?

I don't think putting a lock in msgbuf_putchar us a good idea.  We
deliberately did not put a lock in kprintf() to make sure it can still
be used when we're in ddb without hitting a deadlock.  Instead we put
the lock (kprintf_mutex) in the higher-level functions.

> Index: kern/subr_log.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/kern/subr_log.c,v
> retrieving revision 1.48
> diff -u -p -u -p -r1.48 subr_log.c
> --- kern/subr_log.c   23 Jun 2016 15:41:42 -  1.48
> +++ kern/subr_log.c   19 Oct 2016 19:07:27 -
> @@ -79,6 +79,7 @@ int msgbufmapped;   /* is the message bu
>  struct   msgbuf *msgbufp;/* the mapped buffer, itself. */
>  struct   msgbuf *consbufp;   /* console message buffer. */
>  struct   file *syslogf;
> +struct mutex log_mtx = MUTEX_INITIALIZER(IPL_HIGH);
>  
>  void filt_logrdetach(struct knote *kn);
>  int filt_logread(struct knote *kn, long hint);
> @@ -140,13 +141,11 @@ initconsbuf(void)
>  void
>  msgbuf_putchar(struct msgbuf *mbp, const char c)
>  {
> - int s;
> -
>   if (mbp->msg_magic != MSG_MAGIC)
>   /* Nothing we can do */
>   return;
>  
> - s = splhigh();
> + mtx_enter(_mtx);
>   mbp->msg_bufc[mbp->msg_bufx++] = c;
>   mbp->msg_bufl = lmin(mbp->msg_bufl+1, mbp->msg_bufs);
>   if (mbp->msg_bufx < 0 || mbp->msg_bufx >= mbp->msg_bufs)
> @@ -157,7 +156,7 @@ msgbuf_putchar(struct msgbuf *mbp, const
>   mbp->msg_bufr = 0;
>   mbp->msg_bufd++;
>   }
> - splx(s);
> + mtx_leave(_mtx);
>  }
>  
>  int
> @@ -186,19 +185,21 @@ logread(dev_t dev, struct uio *uio, int 
>  {
>   struct msgbuf *mbp = msgbufp;
>   size_t l;
> - int s, error = 0;
> + int error = 0;
>  
> - s = splhigh();
> + mtx_enter(_mtx);
>   while (mbp->msg_bufr == mbp->msg_bufx) {
>   if (flag & IO_NDELAY) {
> - error = EWOULDBLOCK;
> - goto out;
> + mtx_leave(_mtx);
> + return (EWOULDBLOCK);
>   }
>   logsoftc.sc_state |= LOG_RDWAIT;
> - error = tsleep(mbp, LOG_RDPRI | PCATCH,
> + error = msleep(mbp, _mtx, LOG_RDPRI | PCATCH,
>  "klog", 0);
> - if (error)
> - goto out;
> + if (error) {
> + mtx_leave(_mtx);
> + return (error);
> + }
>   }
>   logsoftc.sc_state &= ~LOG_RDWAIT;
>  
> @@ -209,13 +210,17 @@ logread(dev_t dev, struct uio *uio, int 
>   "<%d>klog: dropped %ld byte%s, message buffer full\n",
>   LOG_KERN|LOG_WARNING, mbp->msg_bufd,
>  mbp->msg_bufd == 1 ? "" : "s");
> + mtx_leave(_mtx);
>   error = uiomove(buf, ulmin(l, sizeof(buf) - 1), uio);
>   if (error)
> - goto out;
> + return (error);
> + mtx_enter(_mtx);
>   mbp->msg_bufd = 0;
>   }
>  
>   while (uio->uio_resid > 0) {
> + char *buf;
> +
>   if (mbp->msg_bufx >= mbp->msg_bufr)
>   l = mbp->msg_bufx - mbp->msg_bufr;
>   else
> @@ -223,31 +228,34 @@ logread(dev_t dev, struct uio *uio, int 
>   l = ulmin(l, uio->uio_resid);
>   if (l == 0)
>   break;
> - error = uiomove(>msg_bufc[mbp->msg_bufr], l, uio);
> + buf = >msg_bufc[mbp->msg_bufr];
> + mtx_leave(_mtx);
> + error = uiomove(buf, l, uio);
>   if (error)
> - break;
> + return (error);
> + mtx_enter(_mtx);
>   mbp->msg_bufr += l;
>   if (mbp->msg_bufr < 0 || mbp->msg_bufr >= mbp->msg_bufs)
>   mbp->msg_bufr = 0;
>   }
> - out:
> - splx(s);
> + mtx_leave(_mtx);
> +
>   return (error);
>  }
>  
>  int
>  logpoll(dev_t dev, int events, struct proc *p)
>  {
> - int s, revents = 0;
> + int revents = 0;
>  
> - s = splhigh();
> + mtx_enter(_mtx);
>   if (events & (POLLIN | POLLRDNORM)) {
>  

Re: usb disk dirty after every reboot

[Lampshade]

>I found a few cases, where we should send a cache flush but don't. 
>Unfortunately, none of these cases explain the problem seen by
>Jan and Darren.
>The cases I have found are:
>* When a R/W mount is updated to R/O. I will send patches for this in a 
>separate mail.
>* When a R/W mount is unmounted but there is still another partition from the 
>same disk mounted.
>* When sync(2)  is called. Though I am not 100% sure if we really want
> to do a cache flush for every sync. Thoughts?

What if somebody remounts
from normal options
(metadata: sync, data: async)
to fully synced
(metadata: sync, data: sync)

Example:
# mount | grep home 
/dev/sd1h on /home type ffs (local, noatime, nodev, nosuid)
# /sbin/mount -u -o sync,rw,nodev,nosuid,noatime /home 
# mount | grep home
/dev/sd1h on /home type ffs (local, noatime, nodev, nosuid, synchronous)

Should this flush cache for this particular filesystem?

Re: let PF to send challenge ack

[Alexander Bluhm]

On Fri, Sep 30, 2016 at 11:55:48PM +0200, Alexandr Nedvedicky wrote:
> The patch makes PF to send 'challenge ACK' for SYN packet, which matches
> session in established state.

regress/sys/net/pf_forward has found a bug in your code.  Looks
like the route-to feature was affected.  By splitting the if
expression and keeping the return (PF_DROP) in the outer block,
much more packets than before were dropped.

> - if (((pd->hdr.tcp->th_flags & (TH_SYN|TH_ACK)) == TH_SYN) &&
> - dst->state >= TCPS_FIN_WAIT_2 &&
> - src->state >= TCPS_FIN_WAIT_2) {
> + if ((pd->hdr.tcp->th_flags & (TH_SYN|TH_ACK)) == TH_SYN) {
> +
> + if (dst->state >= TCPS_FIN_WAIT_2 &&
> + src->state >= TCPS_FIN_WAIT_2) {
...
> + } else if (dst->state >= TCPS_ESTABLISHED &&
> + src->state >= TCPS_ESTABLISHED) {
...

>   }
...
>   return (PF_DROP);
>   }

With this follow up fix it passes and is OK bluhm@.

diff --git a/net/pf.c b/net/pf.c
index f65bc4e..1a862df 100644
--- a/net/pf.c
+++ b/net/pf.c
@@ -4682,6 +4682,7 @@ pf_test_state(struct pf_pdesc *pd, struct pf_state 
**state, u_short *reason)
pf_remove_state(*state);
*state = NULL;
pd->m->m_pkthdr.pf.inp = inp;
+   return (PF_DROP);
} else if (dst->state >= TCPS_ESTABLISHED &&
src->state >= TCPS_ESTABLISHED) {
 /*
@@ -4693,8 +4694,8 @@ pf_test_state(struct pf_pdesc *pd, struct pf_state 
**state, u_short *reason)
 * to get in sync again.
  */
 pf_send_challenge_ack(pd, *state, src, dst);
+   return (PF_DROP);
}
-   return (PF_DROP);
}
 
if ((*state)->state_flags & PFSTATE_SLOPPY) {

Re: aml_rdpciaddr is busted

[Joerg Jung]

On Wed, Oct 19, 2016 at 09:51:47PM +0200, Mark Kettenis wrote:
> The bus number it reports will be totally bogus for devices behind PCI
> bridges.  As a consequence AML will peek and poke at registers of the
> wrong device.  This is what caused the suspend issues with Joris'
> Macbook.
> 
> The diff below attempts to fix this by using the mapping between AML
> nodes and PCI devices that we establish.  Because _INI methods may end
> up executing AML that ends up calling aml_rdpciaddr(), the mapping is
> now established before we execute _INI.  I'm not 100% sure this is
> correct as there is a possibility that _INI will poke some PCI bridges
> in a way that changes the mapping.  Only one way to find out...
> 
> The code also deals with devices that aren't there in a slightly
> different way.  They are now included in the node -> PCI mapping, but
> they're still not included in the list of PCI device nodes that we
> create.  Writing to config space for devices that aren't there is
> implemented as a no-op.  Reading will return an all-ones pattern.
> 
> So far I've only tested this on my x1.  More testing is needed.

Tested the diff together with the timer diff from Joris on MacBookAir6,2.
Does no blow up the machine. pci/ppb lines in dmesg change with this 
(less Thunderbolt lines), dmesg diff below.

Regards,
Joerg


--- dmesg.old   Thu Oct 20 19:32:44 2016
+++ dmesg.new   Thu Oct 20 19:31:51 2016
@@ -1,4 +1,4 @@
-/bsd: OpenBSD 6.0-current (GENERIC.MP) #0: Wed Oct 19 20:30:41 CEST 2016
+/bsd: OpenBSD 6.0-current (GENERIC.MP) #0: Thu Oct 20 19:18:57 CEST 2016
 /bsd: y...@marvin.hq.umaxx.net:/usr/src/sys/arch/amd64/compile/GENERIC.MP
 /bsd: RTC BIOS diagnostic error 
ff
 /bsd: real mem = 8509276160 (8115MB)
@@ -17,7 +17,7 @@
 /bsd: acpihpet0 at acpi0: 14318179 Hz
 /bsd: acpimadt0 at acpi0 addr 0xfee0: PC-AT compat
 /bsd: cpu0 at mainbus0: apid 0 (boot processor)
-/bsd: cpu0: Intel(R) Core(TM) i7-4650U CPU @ 1.70GHz, 1600.21 MHz
+/bsd: cpu0: Intel(R) Core(TM) i7-4650U CPU @ 1.70GHz, 1600.28 MHz
 /bsd: cpu0: 
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,LONG,LAHF,ABM,PERF,ITSC,FSGSBASE,BMI1,HLE,AVX2,SMEP,BMI2,ERMS,INVPCID,RTM,SENSOR,ARAT
 /bsd: cpu0: 256KB 64b/line 8-way L2 cache
 /bsd: cpu0: smt 0, core 0, package 0
@@ -25,17 +25,17 @@
 /bsd: cpu0: apic clock running at 100MHz
 /bsd: cpu0: mwait min=64, max=64, C-substates=0.2.1.2.4.1.1.1, IBE
 /bsd: cpu1 at mainbus0: apid 2 (application processor)
-/bsd: cpu1: Intel(R) Core(TM) i7-4650U CPU @ 1.70GHz, 1600.00 MHz
+/bsd: cpu1: Intel(R) Core(TM) i7-4650U CPU @ 1.70GHz, 1600.01 MHz
 /bsd: cpu1: 
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,LONG,LAHF,ABM,PERF,ITSC,FSGSBASE,BMI1,HLE,AVX2,SMEP,BMI2,ERMS,INVPCID,RTM,SENSOR,ARAT
 /bsd: cpu1: 256KB 64b/line 8-way L2 cache
 /bsd: cpu1: smt 0, core 1, package 0
 /bsd: cpu2 at mainbus0: apid 1 (application processor)
-/bsd: cpu2: Intel(R) Core(TM) i7-4650U CPU @ 1.70GHz, 1600.00 MHz
+/bsd: cpu2: Intel(R) Core(TM) i7-4650U CPU @ 1.70GHz, 1600.01 MHz
 /bsd: cpu2: 
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,LONG,LAHF,ABM,PERF,ITSC,FSGSBASE,BMI1,HLE,AVX2,SMEP,BMI2,ERMS,INVPCID,RTM,SENSOR,ARAT
 /bsd: cpu2: 256KB 64b/line 8-way L2 cache
 /bsd: cpu2: smt 1, core 0, package 0
 /bsd: cpu3 at mainbus0: apid 3 (application processor)
-/bsd: cpu3: Intel(R) Core(TM) i7-4650U CPU @ 1.70GHz, 1600.00 MHz
+/bsd: cpu3: Intel(R) Core(TM) i7-4650U CPU @ 1.70GHz, 1600.01 MHz
 /bsd: cpu3: 
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,LONG,LAHF,ABM,PERF,ITSC,FSGSBASE,BMI1,HLE,AVX2,SMEP,BMI2,ERMS,INVPCID,RTM,SENSOR,ARAT
 /bsd: cpu3: 256KB 64b/line 8-way L2 cache
 /bsd: cpu3: smt 1, core 1, package 0
@@ -92,22 +92,9 @@
 /bsd: "Broadcom BCM4360" rev 0x03 at pci3 dev 0 function 0 not configured
 /bsd: ppb3 at pci0 dev 28 function 4 "Intel 8 Series PCIE" rev 0xe4: msi
 /bsd: pci4 at ppb3 bus 5
-/bsd: ppb4 at pci4 dev 0 function 0 "Intel DSL3510 Thunderbolt" rev 0x03
-/bsd: pci5 at ppb4 bus 6
-/bsd: ppb5 at pci5 dev 0 function 0 

Re: make !!= extension

[Marc Espie]

On Thu, Oct 20, 2016 at 09:55:33AM -0600, Todd C. Miller wrote:
> On Thu, 20 Oct 2016 15:22:44 +0200, Marc Espie wrote:
> 
> Comments inline.
> I find "Expand the value" to be confusing.  Would the following
> also be accurate?
> 
> Perform variable expansion and pass the result to the shell for
^^^ on the spot
> execution only when the value is needed, assigning the result
> to the variable.
It's impossible to do variable expansion later and have clear semantics
(as the first execution may happen in different locations in the makefile)

> > case '!':
> > if (type & VAR_SHELL)
> > +   if (type & (VAR_APPEND))
> > +   type = VAR_INVALID;
> > +   else
> > +   type = VAR_LAZYSHELL;
> 
> I know this is correct but I'd really like to see braces around
> that child if/else statement so it doesn't end up unbalanced by a
> future edit.
> 

Heh. I did add braces originally, but didn't like them too much
aesthetically. I can definitely put them back.

Re: make !!= extension

[Todd C. Miller]

On Thu, 20 Oct 2016 15:22:44 +0200, Marc Espie wrote:

Comments inline.

> Index: make.1
> ===
> RCS file: /cvs/src/usr.bin/make/make.1,v
> retrieving revision 1.120
> diff -u -p -r1.120 make.1
> --- make.113 Mar 2015 19:58:41 -  1.120
> +++ make.120 Oct 2016 13:16:05 -
> @@ -532,11 +532,23 @@ Normally, expansion is not done until th
>  .It Ic \&!=
>  Expand the value and pass it to the shell for execution and assign
>  the result to the variable.
> -Any newlines in the result are replaced with spaces
> +newlines in the result are also replaced with spaces

I don't think you meant to remove the "Any" there.

>  .Po
>  .Bx
>  extension
>  .Pc .
> +.It Ic \&!!=
> +Expand the value on the spot, pass it to the shell for execution only when
> +the value is needed, and assign the result to the variable.

I find "Expand the value" to be confusing.  Would the following
also be accurate?

Perform variable expansion and pass the result to the shell for
execution only when the value is needed, assigning the result
to the variable.

> +.Pp
> +This is almost identical to
> +.Ic \&!=
> +except that a shell is only run when the variable value is needed.
> +Any newlines in the result are replaced with spaces
> +.Po
> +.Ox
> +extension
> +.Pc .
>  .El
>  .Pp
>  Any whitespace before the assigned
> @@ -557,6 +569,12 @@ and put its output into
>  if
>  .Va A
>  is not yet defined.
> +.Pp
> +Combinations that do not make sense, such as
> +.Bd -literal -offset indent
> +A +!!= cmd
> +.Ed
> +will not work.
>  .Pp
>  Variables are expanded by surrounding the variable name with either
>  curly braces
> Index: parse.c
> ===
> RCS file: /cvs/src/usr.bin/make/parse.c,v
> retrieving revision 1.116
> diff -u -p -r1.116 parse.c
> --- parse.c   13 May 2016 12:18:11 -  1.116
> +++ parse.c   20 Oct 2016 13:16:05 -
> @@ -1322,7 +1322,7 @@ handle_poison(const char *line)
>   line);
>   return false;
>   } else {
> - Var_MarkPoisoned(name, ename, type);
> + Var_Mark(name, ename, type);
>   return true;
>   }
>  }
> Index: parsevar.c
> ===
> RCS file: /cvs/src/usr.bin/make/parsevar.c,v
> retrieving revision 1.15
> diff -u -p -r1.15 parsevar.c
> --- parsevar.c22 Nov 2013 15:47:35 -  1.15
> +++ parsevar.c20 Oct 2016 13:16:05 -
> @@ -79,6 +79,8 @@ parse_variable_assignment(const char *li
>  #define VAR_APPEND   2
>  #define VAR_SHELL4
>  #define VAR_OPT  8
> +#define VAR_LAZYSHELL16
> +#define VAR_SUNSHELL 32
>   int type;
>   struct Name name;
>  
> @@ -90,11 +92,14 @@ parse_variable_assignment(const char *li
>  
>   type = VAR_NORMAL;
>  
> + /* double operators (except for :) are forbidden */
> + /* OPT and APPEND don't match */
> + /* APPEND and LAZYSHELL can't really work */
>   while (*arg != '=') {
>   /* Check operator type.  */
>   switch (*arg++) {
>   case '+':
> - if (type & (VAR_OPT|VAR_APPEND))
> + if (type & (VAR_OPT|VAR_LAZYSHELL|VAR_APPEND))
>   type = VAR_INVALID;
>   else
>   type |= VAR_APPEND;
> @@ -110,7 +115,7 @@ parse_variable_assignment(const char *li
>   case ':':
>   if (FEATURES(FEATURE_SUNSHCMD) &&
>   strncmp(arg, "sh", 2) == 0) {
> - type = VAR_SHELL;
> + type = VAR_SUNSHELL;
>   arg += 2;
>   while (*arg != '=' && *arg != '\0')
>   arg++;
> @@ -124,6 +129,11 @@ parse_variable_assignment(const char *li
>  
>   case '!':
>   if (type & VAR_SHELL)
> + if (type & (VAR_APPEND))
> + type = VAR_INVALID;
> + else
> + type = VAR_LAZYSHELL;

I know this is correct but I'd really like to see braces around
that child if/else statement so it doesn't end up unbalanced by a
future edit.

> + else if (type & (VAR_LAZYSHELL|VAR_SUNSHELL))
>   type = VAR_INVALID;
>   else
>   type |= VAR_SHELL;
> @@ -147,7 +157,7 @@ parse_variable_assignment(const char *li
>   VarName_Free();
>   return true;
>   }
> - if (type & VAR_SHELL) {
> + if (type & (VAR_SHELL|VAR_SUNSHELL)) {
>   char *err;
>  
>   if (strchr(arg, '$') != NULL) {
> @@ -164,6 +174,13 @@ parse_variable_assignment(const char *li
>  

log mutex

[Alexander Bluhm]

Hi,

A while ago I made kernel logging interrupt safe by adding a
splhigh().  When we are going MP this is not sufficient, so replace
it with a mutex.  The idea is to hold the mutex every time msgbufp
is dereferenced.  This allows to print to dmesg without kernel lock.

Note that we take the mutex for every character.  That should be
not performance critical as when you log too much in kernel, you
have other problems anyway.

There is still a race with logsoftc.sc_state |= LOG_RDWAIT, but I
want to address that separately.

ok?

bluhm

Index: kern/subr_log.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/kern/subr_log.c,v
retrieving revision 1.48
diff -u -p -u -p -r1.48 subr_log.c
--- kern/subr_log.c 23 Jun 2016 15:41:42 -  1.48
+++ kern/subr_log.c 19 Oct 2016 19:07:27 -
@@ -79,6 +79,7 @@ int   msgbufmapped;   /* is the message bu
 struct msgbuf *msgbufp;/* the mapped buffer, itself. */
 struct msgbuf *consbufp;   /* console message buffer. */
 struct file *syslogf;
+struct mutex log_mtx = MUTEX_INITIALIZER(IPL_HIGH);
 
 void filt_logrdetach(struct knote *kn);
 int filt_logread(struct knote *kn, long hint);
@@ -140,13 +141,11 @@ initconsbuf(void)
 void
 msgbuf_putchar(struct msgbuf *mbp, const char c)
 {
-   int s;
-
if (mbp->msg_magic != MSG_MAGIC)
/* Nothing we can do */
return;
 
-   s = splhigh();
+   mtx_enter(_mtx);
mbp->msg_bufc[mbp->msg_bufx++] = c;
mbp->msg_bufl = lmin(mbp->msg_bufl+1, mbp->msg_bufs);
if (mbp->msg_bufx < 0 || mbp->msg_bufx >= mbp->msg_bufs)
@@ -157,7 +156,7 @@ msgbuf_putchar(struct msgbuf *mbp, const
mbp->msg_bufr = 0;
mbp->msg_bufd++;
}
-   splx(s);
+   mtx_leave(_mtx);
 }
 
 int
@@ -186,19 +185,21 @@ logread(dev_t dev, struct uio *uio, int 
 {
struct msgbuf *mbp = msgbufp;
size_t l;
-   int s, error = 0;
+   int error = 0;
 
-   s = splhigh();
+   mtx_enter(_mtx);
while (mbp->msg_bufr == mbp->msg_bufx) {
if (flag & IO_NDELAY) {
-   error = EWOULDBLOCK;
-   goto out;
+   mtx_leave(_mtx);
+   return (EWOULDBLOCK);
}
logsoftc.sc_state |= LOG_RDWAIT;
-   error = tsleep(mbp, LOG_RDPRI | PCATCH,
+   error = msleep(mbp, _mtx, LOG_RDPRI | PCATCH,
   "klog", 0);
-   if (error)
-   goto out;
+   if (error) {
+   mtx_leave(_mtx);
+   return (error);
+   }
}
logsoftc.sc_state &= ~LOG_RDWAIT;
 
@@ -209,13 +210,17 @@ logread(dev_t dev, struct uio *uio, int 
"<%d>klog: dropped %ld byte%s, message buffer full\n",
LOG_KERN|LOG_WARNING, mbp->msg_bufd,
 mbp->msg_bufd == 1 ? "" : "s");
+   mtx_leave(_mtx);
error = uiomove(buf, ulmin(l, sizeof(buf) - 1), uio);
if (error)
-   goto out;
+   return (error);
+   mtx_enter(_mtx);
mbp->msg_bufd = 0;
}
 
while (uio->uio_resid > 0) {
+   char *buf;
+
if (mbp->msg_bufx >= mbp->msg_bufr)
l = mbp->msg_bufx - mbp->msg_bufr;
else
@@ -223,31 +228,34 @@ logread(dev_t dev, struct uio *uio, int 
l = ulmin(l, uio->uio_resid);
if (l == 0)
break;
-   error = uiomove(>msg_bufc[mbp->msg_bufr], l, uio);
+   buf = >msg_bufc[mbp->msg_bufr];
+   mtx_leave(_mtx);
+   error = uiomove(buf, l, uio);
if (error)
-   break;
+   return (error);
+   mtx_enter(_mtx);
mbp->msg_bufr += l;
if (mbp->msg_bufr < 0 || mbp->msg_bufr >= mbp->msg_bufs)
mbp->msg_bufr = 0;
}
- out:
-   splx(s);
+   mtx_leave(_mtx);
+
return (error);
 }
 
 int
 logpoll(dev_t dev, int events, struct proc *p)
 {
-   int s, revents = 0;
+   int revents = 0;
 
-   s = splhigh();
+   mtx_enter(_mtx);
if (events & (POLLIN | POLLRDNORM)) {
if (msgbufp->msg_bufr != msgbufp->msg_bufx)
revents |= events & (POLLIN | POLLRDNORM);
else
selrecord(p, _selp);
}
-   splx(s);
+   mtx_leave(_mtx);
return (revents);
 }
 
@@ -289,12 +297,12 @@ int
 filt_logread(struct knote *kn, long hint)
 {
struct  msgbuf *p = (struct  msgbuf *)kn->kn_hook;
-   int s, event = 0;
+   int event = 0;
 
-   s = splhigh();
+  

make !!= extension

[Marc Espie]

This is a new extension to make. I don't think it will hinder portability
all that much, since != is already an expansion.

VAR != cmd

executes cmd on the spot, and replaces the output into VAR. This is a
convenient feature, but it is rather expensive, since each such VAR
will require the execution of an external shell.

In many cases, it is superior to evaluating stuff through the shell
using `` (or $() ), because make will echo the variable value, so you
actually see the result. e.g.,

SYSDIR != cd ${.CURDIR}/../../../..; pwd
CONFDIR != cd ${.CURDIR}/../../conf; pwd

config:
config -b ${.OBJDIR} -s ${SYSDIR} ${CONFDIR}/${.CURDIR:T}

will echo a line with everything fully expanded.

I propose that

VAR !!= cmd

had almost the same semantics:
- still expand the line on-the-spot (no way to determine where to expand
it otherwise)
- only run it through the shell the first time the variable value is
needed.

Making things "lazy" all the time would also be possible, but it would
change the semantics of things (e.g., you may do tricks like
VAR != some cmd with side effects
because you want the side effects, and then that would break.

I'm now looking for okays...

Index: make.1
===
RCS file: /cvs/src/usr.bin/make/make.1,v
retrieving revision 1.120
diff -u -p -r1.120 make.1
--- make.1  13 Mar 2015 19:58:41 -  1.120
+++ make.1  20 Oct 2016 13:16:05 -
@@ -532,11 +532,23 @@ Normally, expansion is not done until th
 .It Ic \&!=
 Expand the value and pass it to the shell for execution and assign
 the result to the variable.
-Any newlines in the result are replaced with spaces
+newlines in the result are also replaced with spaces
 .Po
 .Bx
 extension
 .Pc .
+.It Ic \&!!=
+Expand the value on the spot, pass it to the shell for execution only when
+the value is needed, and assign the result to the variable.
+.Pp
+This is almost identical to
+.Ic \&!=
+except that a shell is only run when the variable value is needed.
+Any newlines in the result are replaced with spaces
+.Po
+.Ox
+extension
+.Pc .
 .El
 .Pp
 Any whitespace before the assigned
@@ -557,6 +569,12 @@ and put its output into
 if
 .Va A
 is not yet defined.
+.Pp
+Combinations that do not make sense, such as
+.Bd -literal -offset indent
+A +!!= cmd
+.Ed
+will not work.
 .Pp
 Variables are expanded by surrounding the variable name with either
 curly braces
Index: parse.c
===
RCS file: /cvs/src/usr.bin/make/parse.c,v
retrieving revision 1.116
diff -u -p -r1.116 parse.c
--- parse.c 13 May 2016 12:18:11 -  1.116
+++ parse.c 20 Oct 2016 13:16:05 -
@@ -1322,7 +1322,7 @@ handle_poison(const char *line)
line);
return false;
} else {
-   Var_MarkPoisoned(name, ename, type);
+   Var_Mark(name, ename, type);
return true;
}
 }
Index: parsevar.c
===
RCS file: /cvs/src/usr.bin/make/parsevar.c,v
retrieving revision 1.15
diff -u -p -r1.15 parsevar.c
--- parsevar.c  22 Nov 2013 15:47:35 -  1.15
+++ parsevar.c  20 Oct 2016 13:16:05 -
@@ -79,6 +79,8 @@ parse_variable_assignment(const char *li
 #define VAR_APPEND 2
 #define VAR_SHELL  4
 #define VAR_OPT8
+#define VAR_LAZYSHELL  16
+#define VAR_SUNSHELL   32
int type;
struct Name name;
 
@@ -90,11 +92,14 @@ parse_variable_assignment(const char *li
 
type = VAR_NORMAL;
 
+   /* double operators (except for :) are forbidden */
+   /* OPT and APPEND don't match */
+   /* APPEND and LAZYSHELL can't really work */
while (*arg != '=') {
/* Check operator type.  */
switch (*arg++) {
case '+':
-   if (type & (VAR_OPT|VAR_APPEND))
+   if (type & (VAR_OPT|VAR_LAZYSHELL|VAR_APPEND))
type = VAR_INVALID;
else
type |= VAR_APPEND;
@@ -110,7 +115,7 @@ parse_variable_assignment(const char *li
case ':':
if (FEATURES(FEATURE_SUNSHCMD) &&
strncmp(arg, "sh", 2) == 0) {
-   type = VAR_SHELL;
+   type = VAR_SUNSHELL;
arg += 2;
while (*arg != '=' && *arg != '\0')
arg++;
@@ -124,6 +129,11 @@ parse_variable_assignment(const char *li
 
case '!':
if (type & VAR_SHELL)
+   if (type & (VAR_APPEND))
+   type = VAR_INVALID;
+   else
+   type = VAR_LAZYSHELL;
+   else if (type & 

Re: pf_route pf_pdesc

[Alexander Bluhm]

On Thu, Oct 20, 2016 at 10:53:17AM +0200, Claudio Jeker wrote:
> > Unfortunately pf_route() is called from pfsync which has no idea
> > of packet descriptors.  As I do not want to rewrite pfsync, I create
> > a temporary pf_pdesc on the stack.
> 
> I'm OK with the direction but am wondering if setting only that little
> information in the pf_pdesc is save in the future when that pd is passed
> further down.

Henning hat the same concern.  Now I fill the pf_pdesc completely
with pf_setup_pdesc().

ok?

bluhm

Index: net/if_pfsync.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/net/if_pfsync.c,v
retrieving revision 1.235
diff -u -p -u -p -r1.235 if_pfsync.c
--- net/if_pfsync.c 4 Oct 2016 13:54:32 -   1.235
+++ net/if_pfsync.c 20 Oct 2016 12:19:31 -
@@ -61,9 +61,13 @@
 #include 
 #include 
 #include 
+#include 
+#include 
+#include 
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -1732,6 +1736,17 @@ void
 pfsync_undefer(struct pfsync_deferral *pd, int drop)
 {
struct pfsync_softc *sc = pfsyncif;
+   struct pf_pdesc pdesc;
+   union pf_headers {
+   struct tcphdr   tcp;
+   struct udphdr   udp;
+   struct icmp icmp;
+#ifdef INET6
+   struct icmp6_hdricmp6;
+   struct mld_hdr  mld;
+   struct nd_neighbor_solicit nd_ns;
+#endif /* INET6 */
+   } pdhdrs;
 
splsoftassert(IPL_SOFTNET);
 
@@ -1743,17 +1758,22 @@ pfsync_undefer(struct pfsync_deferral *p
m_freem(pd->pd_m);
else {
if (pd->pd_st->rule.ptr->rt == PF_ROUTETO) {
+   if (pf_setup_pdesc(, ,
+   pd->pd_st->key[PF_SK_WIRE]->af,
+   pd->pd_st->direction, pd->pd_st->rt_kif,
+   pd->pd_m, NULL) != PF_PASS) {
+   m_freem(pd->pd_m);
+   goto out;
+   }
switch (pd->pd_st->key[PF_SK_WIRE]->af) {
case AF_INET:
-   pf_route(>pd_m, pd->pd_st->rule.ptr,
-   pd->pd_st->direction,
-   pd->pd_st->rt_kif->pfik_ifp, pd->pd_st);
+   pf_route(>pd_m, ,
+   pd->pd_st->rule.ptr, pd->pd_st);
break;
 #ifdef INET6
case AF_INET6:
-   pf_route6(>pd_m, pd->pd_st->rule.ptr,
-   pd->pd_st->direction,
-   pd->pd_st->rt_kif->pfik_ifp, pd->pd_st);
+   pf_route6(>pd_m, ,
+   pd->pd_st->rule.ptr, pd->pd_st);
break;
 #endif /* INET6 */
}
@@ -1772,7 +1792,7 @@ pfsync_undefer(struct pfsync_deferral *p
}
}
}
-
+ out:
pool_put(>sc_pool, pd);
 }
 
Index: net/pf.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf.c,v
retrieving revision 1.992
diff -u -p -u -p -r1.992 pf.c
--- net/pf.c18 Oct 2016 13:28:01 -  1.992
+++ net/pf.c20 Oct 2016 11:29:54 -
@@ -5764,7 +5764,7 @@ pf_rtlabel_match(struct pf_addr *addr, s
 }
 
 void
-pf_route(struct mbuf **m, struct pf_rule *r, int dir, struct ifnet *oifp,
+pf_route(struct mbuf **m, struct pf_pdesc *pd, struct pf_rule *r,
 struct pf_state *s)
 {
struct mbuf *m0, *m1;
@@ -5777,8 +5777,7 @@ pf_route(struct mbuf **m, struct pf_rule
int  error = 0;
unsigned int rtableid;
 
-   if (m == NULL || *m == NULL || r == NULL ||
-   (dir != PF_IN && dir != PF_OUT) || oifp == NULL)
+   if (m == NULL || *m == NULL || r == NULL)
panic("pf_route: invalid parameters");
 
if ((*m)->m_pkthdr.pf.routed++ > 3) {
@@ -5791,7 +5790,7 @@ pf_route(struct mbuf **m, struct pf_rule
if ((m0 = m_dup_pkt(*m, max_linkhdr, M_NOWAIT)) == NULL)
return;
} else {
-   if ((r->rt == PF_REPLYTO) == (r->direction == dir))
+   if ((r->rt == PF_REPLYTO) == (r->direction == pd->dir))
return;
m0 = *m;
}
@@ -5856,7 +5855,7 @@ pf_route(struct mbuf **m, struct pf_rule
goto bad;
 
 
-   if (oifp != ifp) {
+   if (pd->kif->pfik_ifp != ifp) {
if (pf_test(AF_INET, PF_OUT, ifp, ) != PF_PASS)
goto bad;
else if (m0 == NULL)
@@ -5931,7 +5930,7 @@ bad:
 
 #ifdef INET6
 void
-pf_route6(struct mbuf **m, struct pf_rule *r, int dir, struct ifnet *oifp,
+pf_route6(struct 

Re: malloc canaries for > page sized objects

[Otto Moerbeek]

On Thu, Oct 20, 2016 at 11:28:37AM +0200, Otto Moerbeek wrote:

> On Thu, Oct 20, 2016 at 11:21:26AM +0200, Otto Moerbeek wrote:
> 
> > On Thu, Oct 20, 2016 at 11:17:25AM +0200, Otto Moerbeek wrote:
> > 
> > > On Thu, Oct 20, 2016 at 04:42:13AM -0400, Ted Unangst wrote:
> > > 
> > > > Otto Moerbeek wrote:
> > > > > That is certainly not correct: snprintf and friends return the length 
> > > > > as
> > > > > it would have been if an infinite buffer was passed in. 
> > > > > So the strlen should stay. I'll make a new diff soon though with the
> > > > > error checking, although it might be overkill for this case.
> > > > 
> > > > I think we're getting a little weird here. The circumstances under which
> > > > snprintf can return -1 are quite limited.
> > > 
> > > it can only happen on encoding errors, right? These are not relevant
> > > in this case.
> > > 
> > >   -0tto
> > 
> > But what happens if somebody creates an invalid encoded UTF8 __progname?
> > 
> > -Otto
> 
> It seems that as long as we use %s and not %ls we're safe.
> 
>   -Otto

Hi,

This morning I committed the diff but backed it out just a minute ago.
There is a problem with the GC combination. This should fix it. The
change is in omalloc() wrt the filling of the canary bytes. By
compensating for mopts.malloc_guard in a wrong way I overwrote the
wrong bytes.

-Otto

Index: malloc.c
===
RCS file: /cvs/src/lib/libc/stdlib/malloc.c,v
retrieving revision 1.202
diff -u -p -r1.202 malloc.c
--- malloc.c15 Oct 2016 18:24:40 -  1.202
+++ malloc.c20 Oct 2016 11:46:43 -
@@ -31,6 +31,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -199,6 +200,8 @@ static union {
 char   *malloc_options;/* compile-time options */
 
 static u_char getrbyte(struct dir_info *d);
+static __dead void wrterror(struct dir_info *d, char *msg, ...)
+__attribute__((__format__ (printf, 2, 3)));
 
 #ifdef MALLOC_STATS
 void malloc_dump(int, struct dir_info *);
@@ -261,40 +264,26 @@ struct dir_info *getpool(void) 
 }
 
 static __dead void
-wrterror(struct dir_info *d, char *msg, void *p)
+wrterror(struct dir_info *d, char *msg, ...)
 {
-   char*q = " error: ";
-   struct ioveciov[7];
-   charpidbuf[20];
-   charbuf[20];
-   int saved_errno = errno, i;
-
-   iov[0].iov_base = __progname;
-   iov[0].iov_len = strlen(__progname);
-   iov[1].iov_base = pidbuf;
-   snprintf(pidbuf, sizeof(pidbuf), "(%d) in ", getpid());
-   iov[1].iov_len = strlen(pidbuf);
-   if (d != NULL) {
-   iov[2].iov_base = d->func;
-   iov[2].iov_len = strlen(d->func);
-   } else {
-   iov[2].iov_base = "unknown";
-   iov[2].iov_len = 7;
-   }
-   iov[3].iov_base = q;
-   iov[3].iov_len = strlen(q);
-   iov[4].iov_base = msg;
-   iov[4].iov_len = strlen(msg);
-   iov[5].iov_base = buf;
-   if (p == NULL)
-   iov[5].iov_len = 0;
-   else {
-   snprintf(buf, sizeof(buf), " %010p", p);
-   iov[5].iov_len = strlen(buf);
-   }
-   iov[6].iov_base = "\n";
-   iov[6].iov_len = 1;
-   writev(STDERR_FILENO, iov, 7);
+   struct ioveciov[3];
+   charpidbuf[80];
+   charbuf[80];
+   int saved_errno = errno;
+   va_list ap;
+
+   iov[0].iov_base = pidbuf;
+   snprintf(pidbuf, sizeof(pidbuf), "%s(%d) in %s(): ", __progname,
+   getpid(), d->func ? d->func : "unknown");
+   iov[0].iov_len = strlen(pidbuf);
+   iov[1].iov_base = buf;
+   va_start(ap, msg);
+   vsnprintf(buf, sizeof(buf), msg, ap);
+   va_end(ap);
+   iov[1].iov_len = strlen(buf);
+   iov[2].iov_base = "\n";
+   iov[2].iov_len = 1;
+   writev(STDERR_FILENO, iov, 3);
 
 #ifdef MALLOC_STATS
if (mopts.malloc_stats)
@@ -342,12 +331,12 @@ unmap(struct dir_info *d, void *p, size_
u_int i, offset;
 
if (sz != PAGEROUND(sz))
-   wrterror(d, "munmap round", NULL);
+   wrterror(d, "munmap round");
 
if (psz > mopts.malloc_cache) {
i = munmap(p, sz);
if (i)
-   wrterror(d, "munmap", p);
+   wrterror(d, "munmap %p", p);
STATS_SUB(d->malloc_used, sz);
return;
}
@@ -361,7 +350,7 @@ unmap(struct dir_info *d, void *p, size_
if (r->p != NULL) {
rsz = r->size << MALLOC_PAGESHIFT;
if (munmap(r->p, rsz))
-   wrterror(d, "munmap", r->p);
+   wrterror(d, "munmap %p", r->p);
r->p = NULL;
if (tounmap > r->size)
tounmap -= 

Re: malloc canaries for > page sized objects

[Otto Moerbeek]

On Thu, Oct 20, 2016 at 11:21:26AM +0200, Otto Moerbeek wrote:

> On Thu, Oct 20, 2016 at 11:17:25AM +0200, Otto Moerbeek wrote:
> 
> > On Thu, Oct 20, 2016 at 04:42:13AM -0400, Ted Unangst wrote:
> > 
> > > Otto Moerbeek wrote:
> > > > That is certainly not correct: snprintf and friends return the length as
> > > > it would have been if an infinite buffer was passed in. 
> > > > So the strlen should stay. I'll make a new diff soon though with the
> > > > error checking, although it might be overkill for this case.
> > > 
> > > I think we're getting a little weird here. The circumstances under which
> > > snprintf can return -1 are quite limited.
> > 
> > it can only happen on encoding errors, right? These are not relevant
> > in this case.
> > 
> > -0tto
> 
> But what happens if somebody creates an invalid encoded UTF8 __progname?
> 
>   -Otto

It seems that as long as we use %s and not %ls we're safe.

-Otto

Re: malloc canaries for > page sized objects

[Otto Moerbeek]

On Thu, Oct 20, 2016 at 11:17:25AM +0200, Otto Moerbeek wrote:

> On Thu, Oct 20, 2016 at 04:42:13AM -0400, Ted Unangst wrote:
> 
> > Otto Moerbeek wrote:
> > > That is certainly not correct: snprintf and friends return the length as
> > > it would have been if an infinite buffer was passed in. 
> > > So the strlen should stay. I'll make a new diff soon though with the
> > > error checking, although it might be overkill for this case.
> > 
> > I think we're getting a little weird here. The circumstances under which
> > snprintf can return -1 are quite limited.
> 
> it can only happen on encoding errors, right? These are not relevant
> in this case.
> 
>   -0tto

But what happens if somebody creates an invalid encoded UTF8 __progname?

-Otto

Re: malloc canaries for > page sized objects

[Otto Moerbeek]

On Thu, Oct 20, 2016 at 04:42:13AM -0400, Ted Unangst wrote:

> Otto Moerbeek wrote:
> > That is certainly not correct: snprintf and friends return the length as
> > it would have been if an infinite buffer was passed in. 
> > So the strlen should stay. I'll make a new diff soon though with the
> > error checking, although it might be overkill for this case.
> 
> I think we're getting a little weird here. The circumstances under which
> snprintf can return -1 are quite limited.

it can only happen on encoding errors, right? These are not relevant
in this case.

-0tto

Re: pf_route pf_pdesc

[Claudio Jeker]

On Wed, Oct 19, 2016 at 11:49:56PM +0200, Alexander Bluhm wrote:
> Hi,
> 
> I would like to pass a struct pf_pdesc to pf_route() like it is
> done in the other pf functions.  That means less parameters, more
> consistency and later I can call functions that need an pd from
> pf_route().
> 
> Unfortunately pf_route() is called from pfsync which has no idea
> of packet descriptors.  As I do not want to rewrite pfsync, I create
> a temporary pf_pdesc on the stack.
> 
> ok?
> 

I'm OK with the direction but am wondering if setting only that little
information in the pf_pdesc is save in the future when that pd is passed
further down.

> bluhm
> 
> Index: net/if_pfsync.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/net/if_pfsync.c,v
> retrieving revision 1.235
> diff -u -p -u -p -r1.235 if_pfsync.c
> --- net/if_pfsync.c   4 Oct 2016 13:54:32 -   1.235
> +++ net/if_pfsync.c   19 Oct 2016 21:22:27 -
> @@ -1732,6 +1732,7 @@ void
>  pfsync_undefer(struct pfsync_deferral *pd, int drop)
>  {
>   struct pfsync_softc *sc = pfsyncif;
> + struct pf_pdesc pdesc;
>  
>   splsoftassert(IPL_SOFTNET);
>  
> @@ -1743,17 +1744,18 @@ pfsync_undefer(struct pfsync_deferral *p
>   m_freem(pd->pd_m);
>   else {
>   if (pd->pd_st->rule.ptr->rt == PF_ROUTETO) {
> + memset(, 0, sizeof(pdesc));
> + pdesc.dir = pd->pd_st->direction;
> + pdesc.kif = pd->pd_st->rt_kif;
>   switch (pd->pd_st->key[PF_SK_WIRE]->af) {
>   case AF_INET:
> - pf_route(>pd_m, pd->pd_st->rule.ptr,
> - pd->pd_st->direction,
> - pd->pd_st->rt_kif->pfik_ifp, pd->pd_st);
> + pf_route(>pd_m, ,
> + pd->pd_st->rule.ptr, pd->pd_st);
>   break;
>  #ifdef INET6
>   case AF_INET6:
> - pf_route6(>pd_m, pd->pd_st->rule.ptr,
> - pd->pd_st->direction,
> - pd->pd_st->rt_kif->pfik_ifp, pd->pd_st);
> + pf_route6(>pd_m, ,
> + pd->pd_st->rule.ptr, pd->pd_st);
>   break;
>  #endif /* INET6 */
>   }
> Index: net/pf.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf.c,v
> retrieving revision 1.992
> diff -u -p -u -p -r1.992 pf.c
> --- net/pf.c  18 Oct 2016 13:28:01 -  1.992
> +++ net/pf.c  19 Oct 2016 21:26:59 -
> @@ -5764,7 +5764,7 @@ pf_rtlabel_match(struct pf_addr *addr, s
>  }
>  
>  void
> -pf_route(struct mbuf **m, struct pf_rule *r, int dir, struct ifnet *oifp,
> +pf_route(struct mbuf **m, struct pf_pdesc *pd, struct pf_rule *r,
>  struct pf_state *s)
>  {
>   struct mbuf *m0, *m1;
> @@ -5777,8 +5777,7 @@ pf_route(struct mbuf **m, struct pf_rule
>   int  error = 0;
>   unsigned int rtableid;
>  
> - if (m == NULL || *m == NULL || r == NULL ||
> - (dir != PF_IN && dir != PF_OUT) || oifp == NULL)
> + if (m == NULL || *m == NULL || r == NULL)
>   panic("pf_route: invalid parameters");
>  
>   if ((*m)->m_pkthdr.pf.routed++ > 3) {
> @@ -5791,7 +5790,7 @@ pf_route(struct mbuf **m, struct pf_rule
>   if ((m0 = m_dup_pkt(*m, max_linkhdr, M_NOWAIT)) == NULL)
>   return;
>   } else {
> - if ((r->rt == PF_REPLYTO) == (r->direction == dir))
> + if ((r->rt == PF_REPLYTO) == (r->direction == pd->dir))
>   return;
>   m0 = *m;
>   }
> @@ -5856,7 +5855,7 @@ pf_route(struct mbuf **m, struct pf_rule
>   goto bad;
>  
>  
> - if (oifp != ifp) {
> + if (pd->kif->pfik_ifp != ifp) {
>   if (pf_test(AF_INET, PF_OUT, ifp, ) != PF_PASS)
>   goto bad;
>   else if (m0 == NULL)
> @@ -5931,7 +5930,7 @@ bad:
>  
>  #ifdef INET6
>  void
> -pf_route6(struct mbuf **m, struct pf_rule *r, int dir, struct ifnet *oifp,
> +pf_route6(struct mbuf **m, struct pf_pdesc *pd, struct pf_rule *r,
>  struct pf_state *s)
>  {
>   struct mbuf *m0;
> @@ -5944,8 +5943,7 @@ pf_route6(struct mbuf **m, struct pf_rul
>   struct m_tag*mtag;
>   unsigned int rtableid;
>  
> - if (m == NULL || *m == NULL || r == NULL ||
> - (dir != PF_IN && dir != PF_OUT) || oifp == NULL)
> + if (m == NULL || *m == NULL || r == NULL)
>   panic("pf_route6: invalid parameters");
>  
>   if ((*m)->m_pkthdr.pf.routed++ > 3) {
> @@ -5958,7 +5956,7 @@ pf_route6(struct mbuf **m, struct pf_rul
>   if ((m0 = m_dup_pkt(*m, 

Re: malloc canaries for > page sized objects

[Ted Unangst]

Otto Moerbeek wrote:
> That is certainly not correct: snprintf and friends return the length as
> it would have been if an infinite buffer was passed in. 
> So the strlen should stay. I'll make a new diff soon though with the
> error checking, although it might be overkill for this case.

I think we're getting a little weird here. The circumstances under which
snprintf can return -1 are quite limited.

Re: pf_route pf_pdesc

[Alexandr Nedvedicky]

Hello,


On Wed, Oct 19, 2016 at 11:49:56PM +0200, Alexander Bluhm wrote:
> Hi,
> 
> I would like to pass a struct pf_pdesc to pf_route() like it is
> done in the other pf functions.  That means less parameters, more
> consistency and later I can call functions that need an pd from
> pf_route().
> 
> Unfortunately pf_route() is called from pfsync which has no idea
> of packet descriptors.  As I do not want to rewrite pfsync, I create
> a temporary pf_pdesc on the stack.
> 
> ok?
> 

The idea makes sense to me. I'm O.K. with patch.

regards
sasha

Re: remove useless extern declaration

[Theo Buehler]

On Thu, Oct 20, 2016 at 09:11:17AM +0200, Martin Natano wrote:
> On Wed, Oct 19, 2016 at 11:48:05PM +0200, Jan Stary wrote:
> > extern char *optarg is already declared in unistd.h
> > This is the only occurence in src/sbin and src/bin;
> > others will follow in separate mails.
> > 
> > Jan
> > 
> 
> OK.

committed, thanks

Re: remove useless extern declaration

[Martin Natano]

On Wed, Oct 19, 2016 at 11:48:05PM +0200, Jan Stary wrote:
> extern char *optarg is already declared in unistd.h
> This is the only occurence in src/sbin and src/bin;
> others will follow in separate mails.
> 
>   Jan
> 

OK.


> 
> Index: bioctl.c
> ===
> RCS file: /cvs/src/sbin/bioctl/bioctl.c,v
> retrieving revision 1.139
> diff -u -p -r1.139 bioctl.c
> --- bioctl.c  21 Sep 2016 17:50:05 -  1.139
> +++ bioctl.c  19 Oct 2016 21:43:43 -
> @@ -59,7 +59,7 @@ struct timing {
>   int start;
>  };
>  
> -void usage(void);
> +static void __dead   usage(void);
>  const char   *str2locator(const char *, struct locator *);
>  const char   *str2patrol(const char *, struct timing *);
>  void bio_status(struct bio_status *);
> @@ -100,7 +100,6 @@ int
>  main(int argc, char *argv[])
>  {
>   struct bio_locate   bl;
> - extern char *optarg;
>   u_int64_t   func = 0;
>   char*devicename = NULL;
>   char*realname = NULL, *al_arg = NULL;
> @@ -273,7 +272,7 @@ main(int argc, char *argv[])
>   return (0);
>  }
>  
> -void
> +static void __dead
>  usage(void)
>  {
>   extern char *__progname;
>