Re: PATCH: rtsol support for RA DNS options

[Sebastian Benoit]

Charles Musser(cmus...@sonic.net) on 2014.09.20 14:44:45 -0700:
 + /*
 +  * XXX validate that domain name only contains valid characters
 +  * for two reasons: 1) correctness, 2) we do not want to pass
 +  * possible malicious, unescaped characters like `` to a script
 +  * or program that could be exploited that way.
 +  */
 +
 +   return (src - src_origin);

this alone is a reason why this diff is wrong.

/Benno

(and removing all of the ifndef SMALL is another)

Re: Fix to diskless(8) manpage: add amd64 and i386 to the list of clients that needs rpc.bootparamd(8)

[Sebastian Benoit]

Mark Kettenis(mark.kette...@xs4all.nl) on 2013.07.14 17:06:24 +0200:
  Date: Sun, 14 Jul 2013 14:09:26 +0200
  From: Henning Brauer lists-openbsdt...@bsws.de
  
  * Rafael Neves rafaelne...@gmail.com [2013-07-14 11:01]:
   Amd64 and i386 diskless(8) setups need rpc.bootparamd(8)
  
  no, they don't.
 
 True diskless(4) operation (with root on nfs) needs rpc.bootparamd(8)
 an *all* architectures.  Merely booting a bsd.rd kernel only needs
 rpc.bootparamd(8) on the architectures mentioned under 11 in the
 EXAMPLES section of the diskless(4) page.

yes, and having run though diskless 4 months ago and discovering that it was
missing, i tought about a similar manpage diff.

I mean, the page starts with

 When booting a system over the network, there are three phases of
 interaction between client and server:

 1.   The PROM (or stage-1 bootstrap) loads a boot program.
 2.   The boot program loads a kernel.
 3.   The kernel does NFS mounts for root and swap.

and i would expect thats what i get when doing what it describes.

Maybe just add this?

--- diskless.8~ Sun Jul 14 18:44:42 2013
+++ diskless.8  Sun Jul 14 18:44:42 2013
@@ -382,6 +382,10 @@
 .Pp
 For older alpha and vax clients:
 .Xr mopd 8
+.Pp
+All architectures need
+.Xr rpc.bootparamd 8
+to retrieve the location of the root and swap device.
 .It
 Net boot the client.
 .El

Re: relayd: crash with two listen on (one is ssl)

[Sebastian Benoit]

This has been commited, thanks!

Erik Lax(e...@halon.se) on 2013.11.19 22:40:38 +0100:
 Hi,
 
 In relayd, if a relay is configured with two listen on directives, one
 with ssl and one without. In the relay_inherit function the ssl pointers
 (cert and key) are copied to the latter, and used/freed even if F_SSL is
 not set. This causes a double free later in purge_relay.
 
 relay http {
   listen on 127.0.0.1 port 4433 ssl
   listen on 127.0.0.1 port 8080
   forward with ssl to 127.0.0.1 port 443
 }
 
 There following patch fixes this.
 
 --- usr.sbin/relayd/parse.y.orig  Tue Nov 19 22:10:48 2013
 +++ usr.sbin/relayd/parse.y   Tue Nov 19 22:09:41 2013
 @@ -2809,6 +2809,12 @@
   rb-rl_conf.port = rc.port;
   rb-rl_conf.flags =
   (ra-rl_conf.flags  ~F_SSL) | (rc.flags  F_SSL);
 + if (!(rb-rl_conf.flags  F_SSL)) {
 + rb-rl_ssl_cert = NULL;
 + rb-rl_conf.ssl_cert_len = 0;
 + rb-rl_ssl_key = NULL;
 + rb-rl_conf.ssl_key_len = 0;
 + }
   TAILQ_INIT(rb-rl_tables);
 
   rb-rl_conf.id = ++last_relay_id;
 

-- 

Re: pflow(4) with optional flowsrc

[Sebastian Benoit]

this has been commited, thanks!

Nathanael Rensen(nathanael.open...@list.polymorpheus.com) on 2014.01.18 
23:49:26 +0800:
 Some time ago I proposed a diff to allow pflow(4) to determine the src IP
 address based on the route table if flowsrc was not specified. That diff
 was not accepted because having multiple places look up route tables is
 undesirable.
 
 Since then henning@ moved UDP checksum calcs into ip_output. That makes
 it very simple to allow the pflow flowsrc parameter to be optional. This
 diff permits the flowsrc parameter to be unspecified, as was permitted
 prior to version 1.35 of if_flow.c, except that now, thanks to henning@,
 it works.
 
 Nathanael
 
 
 Index: sbin/ifconfig/ifconfig.8
 ===
 RCS file: /cvs/src/sbin/ifconfig/ifconfig.8,v
 retrieving revision 1.237
 diff -u -p -u -p -r1.237 ifconfig.8
 --- sbin/ifconfig/ifconfig.8  13 Oct 2013 10:45:34 -  1.237
 +++ sbin/ifconfig/ifconfig.8  15 Jan 2014 18:20:18 -
 @@ -1224,12 +1224,11 @@ Pflow data will be sent to this address/
  Unset the receiver address and stop sending pflow data.
  .It Cm flowsrc Ar addr
  Set the source IP address for pflow packets.
 -Must be defined to export pflow data.
  .Ar addr
  is the IP address used as sender of the UDP packets and may be used to
  identify the source of the data on the pflow collector.
  .It Fl flowsrc
 -Unset the source address and stop sending pflow data.
 +Unset the source address.
  .It Cm pflowproto Ar n
  Set the protocol version.
  The default is version 5.
 Index: sbin/ifconfig/ifconfig.c
 ===
 RCS file: /cvs/src/sbin/ifconfig/ifconfig.c,v
 retrieving revision 1.280
 diff -u -p -u -p -r1.280 ifconfig.c
 --- sbin/ifconfig/ifconfig.c  1 Dec 2013 10:05:29 -   1.280
 +++ sbin/ifconfig/ifconfig.c  15 Jan 2014 18:20:18 -
 @@ -3879,8 +3879,9 @@ pflow_status(void)
   if (ioctl(s, SIOCGETPFLOW, (caddr_t)ifr) == -1)
return;
  
 - printf(\tpflow: sender: %s , preq.sender_ip.s_addr != INADDR_ANY ?
 - inet_ntoa(preq.sender_ip) : INVALID);
 + printf(\tpflow: );
 + if (preq.sender_ip.s_addr != INADDR_ANY)
 + printf(sender: %s , inet_ntoa(preq.sender_ip));
   printf(receiver: %s:, preq.receiver_ip.s_addr != INADDR_ANY ?
   inet_ntoa(preq.receiver_ip) : INVALID);
   if (preq.receiver_port == 0)
 Index: share/man/man4/pflow.4
 ===
 RCS file: /cvs/src/share/man/man4/pflow.4,v
 retrieving revision 1.16
 diff -u -p -u -p -r1.16 pflow.4
 --- share/man/man4/pflow.414 Sep 2013 14:54:30 -  1.16
 +++ share/man/man4/pflow.415 Jan 2014 18:20:18 -
 @@ -42,8 +42,7 @@ Multiple
  interfaces can be created at runtime using the
  .Ic ifconfig pflow Ns Ar N Ic create
  command.
 -Each interface must be configured with a flow sender IP address,
 -a flow receiver IP address,
 +Each interface must be configured with a flow receiver IP address
  and a flow receiver port number.
  .Pp
  Only states created by a rule marked with the
 @@ -92,8 +91,6 @@ collector.
  .Cm flowdst
  defines the collector IP address and the port.
  The
 -.Cm flowsrc
 -IP address and
  .Cm flowdst
  IP address and port must be defined to enable the export of flows.
  .Pp
 Index: sys/net/if_pflow.c
 ===
 RCS file: /cvs/src/sys/net/if_pflow.c,v
 retrieving revision 1.38
 diff -u -p -u -p -r1.38 if_pflow.c
 --- sys/net/if_pflow.c1 Nov 2013 14:34:27 -   1.38
 +++ sys/net/if_pflow.c15 Jan 2014 18:20:23 -
 @@ -426,7 +426,6 @@ pflowioctl(struct ifnet *ifp, u_long cmd
   if ((ifp-if_flags  IFF_UP) 
   sc-sc_receiver_ip.s_addr != INADDR_ANY 
   sc-sc_receiver_port != 0 
 - sc-sc_sender_ip.s_addr != INADDR_ANY 
   sc-sc_sender_port != 0) {
   ifp-if_flags |= IFF_RUNNING;
   sc-sc_gcounter=pflowstats.pflow_flows;
 @@ -506,7 +505,6 @@ pflowioctl(struct ifnet *ifp, u_long cmd
   if ((ifp-if_flags  IFF_UP) 
   sc-sc_receiver_ip.s_addr != INADDR_ANY 
   sc-sc_receiver_port != 0 
 - sc-sc_sender_ip.s_addr != INADDR_ANY 
   sc-sc_sender_port != 0) {
   ifp-if_flags |= IFF_RUNNING;
   sc-sc_gcounter=pflowstats.pflow_flows;
 

-- 

relayd imsg race

[Sebastian Benoit]

Hi,

I have a relayd config with two tables webhosts and web9k containing the 
same
hosts and with two redirects on different ports using these tables.

  ext_addr=10.12.33.59
  webhost1=10.12.77.10
  webhost2=10.12.77.11
  #interval 2
  #timeout 500
  log all
  table webhosts { $webhost1 $webhost2 }
  table web9k { $webhost1 $webhost2 }
  redirect www {
 listen on $ext_addr port http
 match tag RELAYD
 forward to webhosts check http / code 200
  }
  redirect www9k {
 listen on $ext_addr port 9000
 match tag RELAYD
 forward to web9k check http / code 200
  } 

I see relayd crashes like this: (1)

startup
host 10.12.77.10, check http code (1ms), state unknown - up, availability 
100.00%
host 10.12.77.11, check http code (2ms), state unknown - up, availability 
100.00%
fatal: relay_dispatch_pfe: invalid host id
pfe exiting, pid 30837
hce exiting, pid 20192
fatal: relay_dispatch_pfe: invalid host id
relay exiting, pid 23812
parent terminating, pid 14729
relay exiting, pid 6320
relay exiting, pid 5411

or like this: (2)

startup
host 10.12.77.10, check http code (1ms), state unknown - up, availability 
100.00%
host 10.12.77.11, check http code (2ms), state unknown - up, availability 
100.00%
fatal: pfe_dispatch_hce: invalid host id
hce exiting, pid 20299
lost child: pfe exited abnormally
relay exiting, pid 23329
relay exiting, pid 13210
relay exiting, pid 7180
relay exiting, pid 23676
relay exiting, pid 4130
parent terminating, pid 23016

An easy way to cause this is to have no target servers running at all
(in this case i stopped the webservers), but it also happens when they
are really fast. When they are a little slower it does not happen.

There is a race of the hce and the other childs (pfe and relays)
between loading the configuration and start of processing IMSG_HOST_STATUS
messages.

The problem is that in hce_setup_events() the host checks are started before
all childs have all of the configuration.

A quick hack is to insert a sleep(1) at the beginning of hce_setup_events().

A fix might be to make 'invalid host id' non fatal:

diff --git a/usr.sbin/relayd/pfe.c b/usr.sbin/relayd/pfe.c
index 3830d33..281e8e4 100644
--- a/usr.sbin/relayd/pfe.c
+++ b/usr.sbin/relayd/pfe.c
@@ -110,8 +110,10 @@ pfe_dispatch_hce(int fd, struct privsep_proc *p, struct 
imsg *imsg)
case IMSG_HOST_STATUS:
IMSG_SIZE_CHECK(imsg, st);
memcpy(st, imsg-data, sizeof(st));
-   if ((host = host_find(env, st.id)) == NULL)
-   fatalx(pfe_dispatch_hce: invalid host id);
+   if ((host = host_find(env, st.id)) == NULL) {
+   log_warnx(pfe_dispatch_hce: invalid host id);
+   break;
+   }
host-he = st.he;
if (host-flags  F_DISABLE)
break;
diff --git a/usr.sbin/relayd/relay.c b/usr.sbin/relayd/relay.c
index 62ab44e..7c8494e 100644
--- a/usr.sbin/relayd/relay.c
+++ b/usr.sbin/relayd/relay.c
@@ -2471,8 +2471,10 @@ relay_dispatch_pfe(int fd, struct privsep_proc *p, 
struct imsg *imsg)
case IMSG_HOST_STATUS:
IMSG_SIZE_CHECK(imsg, st);
memcpy(st, imsg-data, sizeof(st));
-   if ((host = host_find(env, st.id)) == NULL)
-   fatalx(relay_dispatch_pfe: invalid host id);
+   if ((host = host_find(env, st.id)) == NULL) {
+   log_warnx(relay_dispatch_pfe: invalid host id);
+   break;
+   }
if (host-flags  F_DISABLE)
break;
if (host-up == st.up) {



Another might be to inhibit the processing of IMSG_HOST_STATUS only until
the configuration has been completed (that is after receiving IMSG_CFG_DONE):



diff --git a/usr.sbin/relayd/config.c b/usr.sbin/relayd/config.c
index ef185dc..8ade55f 100644
--- a/usr.sbin/relayd/config.c
+++ b/usr.sbin/relayd/config.c
@@ -131,6 +131,8 @@ config_init(struct relayd *env)
TAILQ_INIT(env-sc_routes);
}
 
+   env-active = PROC_INACTIVE;
+
return (0);
 }
 
diff --git a/usr.sbin/relayd/pfe.c b/usr.sbin/relayd/pfe.c
index 3830d33..343a822 100644
--- a/usr.sbin/relayd/pfe.c
+++ b/usr.sbin/relayd/pfe.c
@@ -110,8 +110,14 @@ pfe_dispatch_hce(int fd, struct privsep_proc *p, struct 
imsg *imsg)
case IMSG_HOST_STATUS:
IMSG_SIZE_CHECK(imsg, st);
memcpy(st, imsg-data, sizeof(st));
-   if ((host = host_find(env, st.id)) == NULL)
+   if ((host = host_find(env, st.id)) == NULL) {
+   if (env-active == PROC_INACTIVE) {
+   log_warnx(pfe_dispatch_hce: 
+   invalid host id (not active));
+   break;
+   }
fatalx(pfe_dispatch_hce: invalid host id);
+   }

possible problem with vr(4) in -current?

[Sebastian Benoit]

Hi,

i have a soekris 5501. after an update from 4.9 to -current (#115: Sun Dec
11) yesterday i saw errors like these this morning:

Dec 17 08:48:22 intern-gw rtadvd[23145]: sendmsg on vr3: No route to host
Dec 17 08:49:31 intern-gw ospfd[18212]: send_packet: error sending packet on
inerface vr0: No route to host

and the box didn't ping for a moment (i have only seen the nagios alarms, so
i dont know for how long).

/Benno


OpenBSD 5.0-current (GENERIC) #115: Sun Dec 11 14:28:08 MST 2011
dera...@i386.openbsd.org:/usr/src/sys/arch/i386/compile/GENERIC
cpu0: Geode(TM) Integrated Processor by AMD PCS (AuthenticAMD 586-class) 500 
MHz
cpu0: FPU,DE,PSE,TSC,MSR,CX8,SEP,PGE,CMOV,CFLUSH,MMX,MMXX,3DNOW2,3DNOW
real mem  = 536408064 (511MB)
avail mem = 517554176 (493MB)
mainbus0 at root
bios0 at mainbus0: AT/286+ BIOS, date 20/70/03, BIOS32 rev. 0 @ 0xfac40
pcibios0 at bios0: rev 2.0 @ 0xf/0x1
pcibios0: pcibios_get_intr_routing - function not supported
pcibios0: PCI IRQ Routing information unavailable.
pcibios0: PCI bus #0 is the last bus
bios0: ROM list: 0xc8000/0xa800
cpu0 at mainbus0: (uniprocessor)
amdmsr0 at mainbus0
pci0 at mainbus0 bus 0: configuration mode 1 (bios)
io address conflict 0x6100/0x100
io address conflict 0x6200/0x200
pchb0 at pci0 dev 1 function 0 AMD Geode LX rev 0x33
glxsb0 at pci0 dev 1 function 2 AMD Geode LX Crypto rev 0x00: RNG AES
vr0 at pci0 dev 6 function 0 VIA VT6105M RhineIII rev 0x96: irq 11, address 
00:00:24:ca:da:24
ukphy0 at vr0 phy 1: Generic IEEE 802.3u media interface, rev. 3: OUI 0x004063, 
model 0x0034
vr1 at pci0 dev 7 function 0 VIA VT6105M RhineIII rev 0x96: irq 5, address 
00:00:24:ca:da:25
ukphy1 at vr1 phy 1: Generic IEEE 802.3u media interface, rev. 3: OUI 0x004063, 
model 0x0034
vr2 at pci0 dev 8 function 0 VIA VT6105M RhineIII rev 0x96: irq 9, address 
00:00:24:ca:da:26
ukphy2 at vr2 phy 1: Generic IEEE 802.3u media interface, rev. 3: OUI 0x004063, 
model 0x0034
vr3 at pci0 dev 9 function 0 VIA VT6105M RhineIII rev 0x96: irq 12, address 
00:00:24:ca:da:27
ukphy3 at vr3 phy 1: Generic IEEE 802.3u media interface, rev. 3: OUI 0x004063, 
model 0x0034
puc0 at pci0 dev 14 function 0 Sunix 40XX rev 0x01: ports: 8 com
com3 at puc0 port 0 irq 10: ti16750, 64 byte fifo
com4 at puc0 port 1 irq 10: ti16750, 64 byte fifo
com5 at puc0 port 2 irq 10: ti16750, 64 byte fifo
com6 at puc0 port 3 irq 10: ti16750, 64 byte fifo
com7 at puc0 port 4 irq 10: ti16750, 64 byte fifo
com8 at puc0 port 5 irq 10: ti16750, 64 byte fifo
com9 at puc0 port 6 irq 10: ti16750, 64 byte fifo
com10 at puc0 port 7 irq 10: ti16750, 64 byte fifo
glxpcib0 at pci0 dev 20 function 0 AMD CS5536 ISA rev 0x03: rev 3, 32-bit 
3579545Hz timer, watchdog, gpio
gpio0 at glxpcib0: 32 pins
pciide0 at pci0 dev 20 function 2 AMD CS5536 IDE rev 0x01: DMA, channel 0 
wired to compatibility, channel 1 wired to compatibility
wd0 at pciide0 channel 0 drive 1: FUJITSU MHZ2320BH G2
wd0: 16-sector PIO, LBA48, 305245MB, 625142448 sectors
wd0(pciide0:0:1): using PIO mode 4, Ultra-DMA mode 2
pciide0: channel 1 ignored (disabled)
ohci0 at pci0 dev 21 function 0 AMD CS5536 USB rev 0x02: irq 15, version 1.0, 
legacy support
ehci0 at pci0 dev 21 function 1 AMD CS5536 USB rev 0x02: irq 15
usb0 at ehci0: USB revision 2.0
uhub0 at usb0 AMD EHCI root hub rev 2.00/1.00 addr 1
isa0 at glxpcib0
isadma0 at isa0
com0 at isa0 port 0x3f8/8 irq 4: ns16550a, 16 byte fifo
com0: console
com1 at isa0 port 0x2f8/8 irq 3: ns16550a, 16 byte fifo
pckbc0 at isa0 port 0x60/5
pckbd0 at pckbc0 (kbd slot)
pckbc0: using irq 1 for kbd slot
wskbd0 at pckbd0: console keyboard
pcppi0 at isa0 port 0x61
spkr0 at pcppi0
nsclpcsio0 at isa0 port 0x2e/2: NSC PC87366 rev 9: GPIO VLM TMS
gpio1 at nsclpcsio0: 29 pins
npx0 at isa0 port 0xf0/16: reported by CPUID; using exception 16
usb1 at ohci0: USB revision 1.0
uhub1 at usb1 AMD OHCI root hub rev 1.00/1.00 addr 1
mtrr: K6-family MTRR support (2 registers)
vscsi0 at root
scsibus0 at vscsi0: 256 targets
softraid0 at root
scsibus1 at softraid0: 256 targets
root on wd0a swap on wd0b dump on wd0b
carp10: state transition: INIT - BACKUP
carp10: state transition: BACKUP - INIT
carp10: state transition: INIT - BACKUP
carp2: state transition: INIT - BACKUP
carp2: state transition: BACKUP - INIT
carp2: state transition: INIT - BACKUP
carp2: state transition: BACKUP - INIT
carp2: state transition: INIT - BACKUP
carp10: state transition: BACKUP - MASTER
arp_rtrequest: bad gateway value
carp2: state transition: BACKUP - MASTER
arp_rtrequest: bad gateway value

netflow v9/ipfix for pflow

[Sebastian Benoit]

+.Rs
+.%R RFC 3954
+.%T Cisco Systems NetFlow Services Export Version 9
+.%D October 2004
+.Re
+.Rs
+.%R RFC 5101
+.%T Specification of the IP Flow Information Export (IPFIX) Protocol for the 
Exchange of IP Traffic Flow Information
+.%D January 2008
+.Re
 .Sh HISTORY
 The
 .Nm
 device first appeared in
 .Ox 4.5 .
+.Sh BUGS
+The IPFIX implementation is incomplete:
+The required transport protocol SCTP is not supported.
+Transport over TCP and DTLS protected flow export is also not supported.
Index: sys/net/if_pflow.c
===
RCS file: /opt/OpenBSD-CVS/src/sys/net/if_pflow.c,v
retrieving revision 1.18
diff -u -p -u -r1.18 if_pflow.c
--- sys/net/if_pflow.c  25 Nov 2011 12:52:10 -  1.18
+++ sys/net/if_pflow.c  21 Dec 2011 11:19:16 -
@@ -1,6 +1,8 @@
 /* $OpenBSD: if_pflow.c,v 1.18 2011/11/25 12:52:10 dlg Exp $   */
 
 /*
+ * Copyright (c) 2011 Florian Obser flor...@narrans.de
+ * Copyright (c) 2011 Sebastian Benoit benoit-li...@fb12.de
  * Copyright (c) 2008 Henning Brauer henn...@openbsd.org
  * Copyright (c) 2008 Joerg Goltermann j...@osn.de
  *
@@ -69,22 +71,38 @@ struct pflowstatspflowstats;
 void   pflowattach(int);
 intpflow_clone_create(struct if_clone *, int);
 intpflow_clone_destroy(struct ifnet *);
+void   pflow_init_timeouts(struct pflow_softc *);
 void   pflow_setmtu(struct pflow_softc *, int);
 intpflowoutput(struct ifnet *, struct mbuf *, struct sockaddr *,
struct rtentry *);
 intpflowioctl(struct ifnet *, u_long, caddr_t);
 void   pflowstart(struct ifnet *);
 
-struct mbuf *pflow_get_mbuf(struct pflow_softc *);
-intpflow_sendout(struct pflow_softc *);
+struct mbuf *pflow_get_mbuf( void );
+struct mbuf *pflow_get_mbuf_v5(struct pflow_softc *);
+struct mbuf *pflow_get_mbuf_ipfix(struct pflow_softc *, u_int16_t);
+void   pflow_flush(struct pflow_softc *);
+intpflow_sendout_v5(struct pflow_softc *);
+intpflow_sendout_ipfix(struct pflow_softc *, sa_family_t);
+intpflow_sendout_ipfix_tmpl(struct pflow_softc *);
 intpflow_sendout_mbuf(struct pflow_softc *, struct mbuf *);
 void   pflow_timeout(void *);
+void   pflow_timeout6(void *);
+void   pflow_timeout_tmpl(void *);
 void   copy_flow_data(struct pflow_flow *, struct pflow_flow *,
struct pf_state *, int, int);
+void   copy_flow4_data(struct pflow_flow4 *, struct pflow_flow4 *,
+   struct pf_state *, int, int);
+void   copy_flow6_data(struct pflow_flow6 *, struct pflow_flow6 *,
+   struct pf_state *, int, int);
 intpflow_pack_flow(struct pf_state *, struct pflow_softc *);
+intpflow_pack_flow_ipfix(struct pf_state *, struct pflow_softc *);
 intpflow_get_dynport(void);
 intexport_pflow_if(struct pf_state*, struct pflow_softc *);
+intexport_pflow_if_ipfix(struct pf_state*, struct pflow_softc *);
 intcopy_flow_to_m(struct pflow_flow *flow, struct pflow_softc *sc);
+intcopy_flow4_to_m(struct pflow_flow4 *flow, struct pflow_softc *sc);
+intcopy_flow6_to_m(struct pflow_flow6 *flow, struct pflow_softc *sc);
 
 struct if_clonepflow_cloner =
 IF_CLONE_INITIALIZER(pflow, pflow_clone_create,
@@ -128,6 +146,84 @@ pflow_clone_create(struct if_clone *ifc,
pflowif-sc_receiver_port = 0;
pflowif-sc_sender_ip.s_addr = INADDR_ANY;
pflowif-sc_sender_port = pflow_get_dynport();
+   pflowif-sc_version = PFLOW_PROTO_DEFAULT;
+   bzero(pflowif-sc_tmpl,sizeof (pflowif-sc_tmpl));
+   pflowif-sc_tmpl.set_header.set_id =
+   htons(pflowif-sc_version == PFLOW_PROTO_9?
+   PFLOW_V9_TMPL_SET_ID:PFLOW_V10_TMPL_SET_ID);
+   pflowif-sc_tmpl.set_header.set_length =
+   htons(sizeof (struct pflow_tmpl));
+
+   /* v9/v10 IPv4 template */
+   pflowif-sc_tmpl.ipv4_tmpl.h.tmpl_id = htons(PFLOW_TMPL_IPV4_ID);
+   pflowif-sc_tmpl.ipv4_tmpl.h.field_count
+   = htons(PFLOW_TMPL_IPV4_FIELD_COUNT);
+   pflowif-sc_tmpl.ipv4_tmpl.src_ip.field_id =
+   htons(PFIX_IE_sourceIPv4Address);
+   pflowif-sc_tmpl.ipv4_tmpl.src_ip.len = htons(4);
+   pflowif-sc_tmpl.ipv4_tmpl.dest_ip.field_id =
+   htons(PFIX_IE_destinationIPv4Address);
+   pflowif-sc_tmpl.ipv4_tmpl.dest_ip.len = htons(4);
+   pflowif-sc_tmpl.ipv4_tmpl.packets.field_id =
+   htons(PFIX_IE_packetDeltaCount);
+   pflowif-sc_tmpl.ipv4_tmpl.packets.len = htons(8);
+   pflowif-sc_tmpl.ipv4_tmpl.octets.field_id =
+   htons(PFIX_IE_octetDeltaCount);
+   pflowif-sc_tmpl.ipv4_tmpl.octets.len = htons(8);
+   pflowif-sc_tmpl.ipv4_tmpl.start.field_id =
+   htons(PFIX_IE_flowStartSysUpTime);
+   pflowif-sc_tmpl.ipv4_tmpl.start.len = htons(4);
+   pflowif-sc_tmpl.ipv4_tmpl.finish.field_id =
+   htons(PFIX_IE_flowEndSysUpTime);
+   pflowif-sc_tmpl.ipv4_tmpl.finish.len = htons(4);
+   pflowif-sc_tmpl.ipv4_tmpl.src_port.field_id =
+   htons(PFIX_IE_sourceTransportPort

pf.conf.5: document self keyword (Re: PF rule match only packets for local machine)

[Sebastian Benoit]

from misc:

Rafal Bisingier(ra...@man.poznan.pl) on 2012.01.05 09:21:16 +0100:
 Just replace to any to to self. Should do what you want.
 
  I have read PF manual but not found any possibility to tell pf to
  LOCAL-HOST. I have search with google but no relevant articles found, 
  maybe I
  have not asked correct.
 
 Well, it's not very easy to find, but the self word is explained in
 the manual.

self is only in the GRAMMAR section,
maybe say something more about it?

/Benno

Index: share/man/man5/pf.conf.5
===
RCS file: /opt/OpenBSD-CVS/src/share/man/man5/pf.conf.5,v
retrieving revision 1.509
diff -u -p -u -r1.509 pf.conf.5
--- share/man/man5/pf.conf.527 Nov 2011 19:55:18 -  1.509
+++ share/man/man5/pf.conf.57 Jan 2012 17:16:47 -
@@ -317,6 +317,8 @@ Any address matching the given table.
 Any source address that fails a unicast reverse path forwarding (URPF)
 check, i.e. packets coming in on an interface other than that which holds
 the route back to the packet's source address.
+.It Ar self
+Expands to all addresses assigned to all interfaces.
 .El
 .Pp
 Ranges of addresses are specified using the
@@ -327,7 +329,9 @@ For instance:
 means all addresses from 10.1.1.10 to 10.1.1.12,
 hence addresses 10.1.1.10, 10.1.1.11, and 10.1.1.12.
 .Pp
-Interface names and interface group names can have modifiers appended:
+Interface names, interface group names and
+.Ar self
+can have modifiers appended:
 .Pp
 .Bl -tag -width  -compact
 .It Ar :0

Re: iwn0 firmware errors

[Sebastian Benoit]

Edd Barrett(vex...@gmail.com) on 2012.02.11 11:57:34 +:
 Hi,
 
 I have just upgraded from a thinkpad x60s to an x61s.
 
 The machine came with an iwn wireless card (no suprise really).
 
 After installing the firmware with fw_update, you can start using the
 card but very shortly start to see firmware errors. Once I managed to
 get an address via DHCP, but that is about as much as I can achieve.
 
 Probably not a huge amount we can do if the firmware is buggy, but worth
 mentioning anyway. In the meantime I am using a usb urtwn.

[...]
 
   error type  = NMI_INTERRUPT_WDG (0x0004)
   program counter = 0x046C
   source line = 0x00D0
   error data  = 0x00020203
   branch link = 0x2F3E04C2
   interrupt link  = 0x06DE2FCE
   time= 3303793537
 driver status:
   tx ring  0: qid=0  cur=10  queued=0  
   tx ring  1: qid=1  cur=0   queued=0  
   tx ring  2: qid=2  cur=0   queued=0  
   tx ring  3: qid=3  cur=0   queued=0  
   tx ring  4: qid=4  cur=45  queued=0  
   tx ring  5: qid=5  cur=0   queued=0  
   tx ring  6: qid=6  cur=0   queued=0  
   tx ring  7: qid=7  cur=0   queued=0  
   tx ring  8: qid=8  cur=0   queued=0  
   tx ring  9: qid=9  cur=0   queued=0  
   tx ring 10: qid=10 cur=0   queued=0  
   tx ring 11: qid=11 cur=0   queued=0  
   tx ring 12: qid=12 cur=0   queued=0  
   tx ring 13: qid=13 cur=0   queued=0  
   tx ring 14: qid=14 cur=0   queued=0  
   tx ring 15: qid=15 cur=0   queued=0  
   rx ring: cur=11
   802.11 state 4

Hi,

i was using a x61s until a month ago. I had these errors too, but only with
certain wifi networks (specifically 2 events with 30+ access points and 1000+
wifi users) using cisco ap's. I never saw these errors somewhere else.

i can find out which cisco access points were used if someone is interested.

/Benno

ssl certificate chains (in relayd)

[Sebastian Benoit]

Hi,

i did not find a place where it is documented explicitly how to use a
certificate chain with relayd.

Should this be documented? Or maybe in ssl(8)?

/Benno

Index: relayd.conf.5
===
RCS file: /cvs/src/usr.sbin/relayd/relayd.conf.5,v
retrieving revision 1.125
diff -u -p -u -r1.125 relayd.conf.5
--- relayd.conf.5   20 Jan 2012 12:16:41 -  1.125
+++ relayd.conf.5   21 Mar 2012 15:17:47 -
@@ -639,6 +639,7 @@ and a public certificate in
 where
 .Ar address
 is the specified IP address of the relay to listen on.
+A certificate chain can be appended to the server certificate.
 See
 .Xr ssl 8
 for details about SSL server certificates.

Re: How to have more than 15 pflog interfaces?

[Sebastian Benoit]

Henning Brauer(henn...@openbsd.org) on 2012.04.13 10:10:41 +0200:
 if nobody tests this beyond my extremely light tests (try actually
 USING the pflog interfaces to log to, I didn't), I can't get this in :)

works somewhat.

destroying an interface breaks things:

# ifconfig pflog17 create   
   
# tcpdump -n -e -ttt -i pflog17

-- i get output

# ifconfig pflog18 create   
# ifconfig pflog19 create   
# mg pf.conf -- replace to pflog17 with to
pflog19
# pfctl -f ./pf.conf
# tcpdump -n -e -ttt -i pflog19  

-- i get output

# ifconfig pflog17 destroy  
 
# tcpdump -n -e -ttt -i pflog19 

-- no more output

/Benno

relayd.conf(5): relay mode hash/loadbalance documentation

[Sebastian Benoit]

Hi,

i think the manpage of relayd has the description in the mode ...
paragraphs a little wrong. The patch below changes

  mode hash

  Balances the outgoing connections across the active hosts based on the
  hashed name of the table.  Additional input can be fed into the hash by
  looking at HTTP headers and GET variables [...]

  mode loadbalance

  Balances the outgoing connections across the active hosts based on the
  hashed name of the table, the source and destination addresses, and the
  corresponding ports. [...]

into

  mode hash

  Balances the outgoing connections across the active hosts based on the
  hashed name of the relay, the hashed name of the the table and the ip
  address and port of the relay. Additional input can be fed into the hash
  by looking at HTTP headers and GET variables [...]

  mode loadbalance

  Balances the outgoing connections across the active hosts based on the
  hashed name of the relay, the hashed name of the the table, the ip address
  and port of the relay and the source address of the client.

In case someone wants to look over this, the code is in relay.c:
relay_launch(), relay_from_table() and a bit in relay_handle_http().

ok?
/Benno

Index: relayd.conf.5
===
RCS file: /opt/OpenBSD-CVS/src/usr.sbin/relayd/relayd.conf.5,v
retrieving revision 1.127
diff -u -p -r1.127 relayd.conf.5
--- relayd.conf.5   24 Apr 2012 14:56:09 -  1.127
+++ relayd.conf.5   5 May 2012 13:59:19 -
@@ -385,16 +385,18 @@ host from the specified table:
 .Bl -tag -width Ds
 .It Ic mode hash
 Balances the outgoing connections across the active hosts based on the
-hashed name of the table.
-Additional input can be fed into the hash by looking at HTTP
-headers and GET variables; see the
+hashed name of the relay, the hashed name of the the table and the ip
+address and port of the relay.
+Additional input can be fed into the
+hash by looking at HTTP headers and GET variables;
+see the
 .Sx PROTOCOLS
 section below.
 This mode is only supported by relays.
 .It Ic mode loadbalance
 Balances the outgoing connections across the active hosts based on the
-hashed name of the table, the source and destination addresses,
-and the corresponding ports.
+hashed name of the relay, the hashed name of the the table, the ip
+address and port of the relay and the source address of the client.
 This mode is only supported by relays.
 .It Ic mode roundrobin
 Distributes the outgoing connections using a round-robin scheduler

ipv6 /sbin/route prefixlen annoyance

[Sebastian Benoit]

Consider 

  route add -inet6 -prefixlen 64 2a00:cafe::: -prefixlen 56 ::1

This currently works (sets the route with /56), as does

  route add -inet6 -prefixlen 56 2a00:cafe::: ::1

(sets the route with /64).

patch:

* dissallow use of argument -prefixlen twice

* when  -prefixlen is given before an ipv6 destination,
  the prefixlen argument is is used instead of implicit /64

/Benno

Index: route.c
===
RCS file: /cvs/src/sbin/route/route.c,v
retrieving revision 1.156
diff -u -p -r1.156 route.c
--- route.c 17 Mar 2012 10:16:40 -  1.156
+++ route.c 8 Jul 2012 18:10:01 -
@@ -71,6 +71,7 @@ int   rtm_addrs, s;
 intforcehost, forcenet, Fflag, nflag, af, qflag, tflag, Tflag;
 intiflag, verbose, aflen = sizeof(struct sockaddr_in);
 intlocking, lockrest, debugonly;
+intseenprefixlen = 0;
 u_long mpls_flags = MPLS_OP_LOCAL;
 u_long rtm_inits;
 uid_t  uid;
@@ -550,6 +551,9 @@ newroute(int argc, char **argv)
case K_PREFIXLEN:
if (!--argc)
usage(1+*argv);
+   if (seenprefixlen)
+   errx(1, cannot set prefixlen twice);
+   seenprefixlen = 1;
ishost = prefixlen(*++argv);
break;
case K_MPATH:
@@ -755,6 +759,9 @@ inet6_makenetandmask(struct sockaddr_in6
if (!plen || strcmp(plen, 128) == 0)
return (1);
else {
+   if (rtm_addrs  RTA_NETMASK) {
+   return (0);
+   }
rtm_addrs |= RTA_NETMASK;
prefixlen(plen);
return (0);

typo in dhcrelay comment

[Sebastian Benoit]

subject says it all.
ok?

diff --git dhcrelay.c dhcrelay.c
index a2f39d0..4782f65 100644
--- dhcrelay.c
+++ dhcrelay.c
@@ -380,7 +380,7 @@ got_response(struct protocol *l)
if ((result = recv(l-fd, u.packbuf, sizeof(u), 0)) == -1 
errno != ECONNREFUSED) {
/*
-* Ignore ECONNREFUSED as to many dhcp server send a bogus
+* Ignore ECONNREFUSED as too many dhcp servers send a bogus
 * icmp unreach for every request.
 */
warning(recv failed for %s: %m,

Re: re(4)/atom freezes (was Re: [SOLVED] Re: OpenBSD 4.8 freezes on certain activities)

[Sebastian Benoit]

Mark Kettenis(mark.kette...@xs4all.nl) on 2010.11.27 20:12:14 +0100:
  Date: Fri, 12 Nov 2010 14:39:41 -0700
  From: Theo de Raadt dera...@cvs.openbsd.org
  
  commit.  someone will eventually fix MCLGETI, since it is in the tree.
 
 The problem is that re(4) has a forever loop from which we only
 escape if none of the RL_INTRS_CPLUS bits are set in the interrupt
 status register.  One of those is the Rx Descriptor Unavailable bit
 (RL_ISR_RX_OVERRUN).  So if we run out of populated descriptors and
 can't allocate any new ones, we'll spin forever.  Obviously chances of
 that happening are much larger with MCLGETI, but it may also happen
 without if you run out of mbufs.
 
 The diff below fixes the issue for me (first diff is relative to the
 code before the backout, second diff includes a revert of the
 backout).  This makes sure we drop out of the interrupt handler such
 that we have a chance to free some mbufs.  None of the drivers I'm
 familliar with have this loop that repeatedly reads the interrupt
 status register.  Except for vr(4), which seems to suffer from the
 same problem as re(4) with MCLGETI.
 
 I did some tests with tcpbench(1) and it seems throughput is slightly
 smaller (compared to non-MCLGETI), but the difference is less than 1%.
 
 I'd appreciate it if the people that experienced the issues can test
 this diff (use the 2nd diff if you have a -current source tree).

Hi,

i tested the patch against 4.8 (as that was easier to test atm).
no more hangs.

/Benno

relayd: exec program on gateway change

[Sebastian Benoit]

Hi,

i am using relayd in router mode for a cable-modem link that sometimes
does not work.

I need to run a programm to load/unload pf-rules and to restart a
proxy with a different config whenever this happens.

Here is a patch that adds an exec option to the router section like this:

router uplinks {
route 0.0.0.0/0
forward to gateways check icmp
exec /usr/local/sbin/relayd_test timeout 10
}

The code that does the exec is taken from check_script.c.

One thing i'm not quite sure about: is the timeout useful or should relayd
just start the program and forget about it?

/Benno


Index: parse.y
===
RCS file: /cvs/src/usr.sbin/relayd/parse.y,v
retrieving revision 1.149
diff -u -r1.149 parse.y
--- parse.y 26 Oct 2010 15:04:37 -  1.149
+++ parse.y 2 Dec 2010 20:53:54 -
@@ -141,7 +141,7 @@
 %}
 
 %token ALL APPEND BACKLOG BACKUP BUFFER CA CACHE CHANGE CHECK
-%token CIPHERS CODE COOKIE DEMOTE DIGEST DISABLE ERROR EXPECT
+%token CIPHERS CODE COOKIE DEMOTE DIGEST DISABLE ERROR EXEC EXPECT
 %token EXTERNAL FILENAME FILTER FORWARD FROM HASH HEADER HOST ICMP
 %token INCLUDE INET INET6 INTERFACE INTERVAL IP LABEL LISTEN
 %token LOADBALANCE LOG LOOKUP MARK MARKED MODE NAT NO
@@ -1523,6 +1523,18 @@
}
free($2);
}
+   | EXEC STRING TIMEOUT timeout {
+   bcopy($4, table-conf.timeout,
+   sizeof(struct timeval));
+   if (strlcpy(router-rt_conf.exec, $2,
+   sizeof(router-rt_conf.exec)) =
+   sizeof(router-rt_conf.exec)) {
+   yyerror(exec command truncated);
+   free($2);
+   YYERROR;
+   }
+   free($2);
+   }
| DISABLE   { rlay-rl_conf.flags |= F_DISABLE; }
| include
;
@@ -1719,6 +1731,7 @@
{ digest, DIGEST },
{ disable,DISABLE },
{ error,  ERROR },
+   { exec,   EXEC },
{ expect, EXPECT },
{ external,   EXTERNAL },
{ file,   FILENAME },
Index: pfe_route.c
===
RCS file: /cvs/src/usr.sbin/relayd/pfe_route.c,v
retrieving revision 1.1
diff -u -r1.1 pfe_route.c
--- pfe_route.c 13 Aug 2009 13:51:21 -  1.1
+++ pfe_route.c 2 Dec 2010 20:53:54 -
@@ -32,6 +32,10 @@
 #include string.h
 #include errno.h
 
+#include sys/wait.h
+#include signal.h
+#include pwd.h
+
 #include openssl/ssl.h
 
 #include relayd.h
@@ -56,6 +60,9 @@
}rm_u;
 };
 
+pid_t route_exec_child = -1;
+void pfe_route_exec_sig_alarm(int);
+
 void
 init_routes(struct relayd *env)
 {
@@ -237,4 +244,115 @@
errno, strerror(errno));
 
return (-1);
+}
+
+/* code from check_script.c */
+
+void
+pfe_route_exec_sig_alarm(int sig)
+{
+   int save_errno = errno;
+
+   if (route_exec_child != -1)
+   kill(route_exec_child, SIGKILL);
+   errno = save_errno;
+}
+
+int
+pfe_route_exec(struct relayd *env, struct ctl_netroute *crt)
+{
+   struct netroute *nr;
+
+   int  status = 0, ret = 0;
+   sig_tsave_quit, save_int, save_chld;
+   struct itimerval it;
+   struct timeval  *tv;
+   const char  *file, *arg_host, *arg_action;
+   struct host *host;
+   struct passwd   *pw;
+
+   if ((nr = route_find(env, crt-id)) == NULL ||
+   (host = host_find(env, crt-hostid)) == NULL) {
+   log_debug(pfe_route: invalid host or route id);
+   return (-1);
+   }
+
+   arg_host = host-conf.name;
+   arg_action = HOST_ISUP(crt-up) ? added : deleted;
+   file = nr-nr_router-rt_conf.exec;
+   tv = nr-nr_router-rt_conf.exec_timeout;
+
+   log_info(pfe_route_exec: %s %s %s,
+file,
+arg_host,
+arg_action);
+
+   save_quit = signal(SIGQUIT, SIG_IGN);
+   save_int = signal(SIGINT, SIG_IGN);
+   save_chld = signal(SIGCHLD, SIG_DFL);
+
+   switch (route_exec_child = fork()) {
+   case -1:
+   ret = -1;
+   goto done;
+   case 0:
+   signal(SIGQUIT, SIG_DFL);
+   signal(SIGINT, SIG_DFL);
+   signal(SIGCHLD, SIG_DFL);
+
+   if ((pw = getpwnam(RELAYD_USER)) == NULL)
+   fatal(pfe_route_exec: getpwnam);
+   if (chdir(/) == -1)
+   fatal(pfe_route_exec: chdir(\/\));
+   if (setgroups(1, pw-pw_gid) ||
+   

bgpd: fix error message enforce remote-as enabled

[Sebastian Benoit]

Hi,

the configuration option in bgpd.conf is called enforce neighbor-as, not
enforce remote-as.

/Benno


--- rde.c.orig  Thu Jan 27 17:02:08 2011
+++ rde.c   Thu Jan 27 17:02:51 2011
@@ -921,7 +921,7 @@
if (peer-conf.remote_as !=
aspath_neighbor(asp-aspath)) {
log_peer_warnx(peer-conf, bad path, 
-   enforce remote-as enabled);
+   enforce neighbor-as enabled);
rde_update_err(peer, ERR_UPDATE, ERR_UPD_ASPATH,
NULL, 0);
goto done;

Re: feed l4 information into trunk(4) hash

[Sebastian Benoit]

Stuart Henderson(st...@openbsd.org) on 2014.12.11 23:52:44 +:
 I'm wondering what reception this will get. It feeds TCP/UDP port
 numbers into the hash for trunk(4) load balancing, so connections
 between a single pair of hosts will get distributed across NICs.
 Taken from FreeBSD r232629, they also added SIOC[CS]LAGGHASH ioctls
 so that the hash type can be configured in ifconfig (l2/l3/l4),
 I haven't done that but could if there's no general objection to
 this.

i like this, and i think it might be good to have it optional in case it
causes problems with other equipment.

Benno
 
 This method is used on some switches too (e.g. trunk-load-balance
 L4-based on HP 2530 and 5400zl).
 
 Index: if_trunk.c
 ===
 RCS file: /cvs/src/sys/net/if_trunk.c,v
 retrieving revision 1.93
 diff -u -p -r1.93 if_trunk.c
 --- if_trunk.c4 Dec 2014 00:01:53 -   1.93
 +++ if_trunk.c11 Dec 2014 23:45:20 -
 @@ -978,12 +978,23 @@ trunk_hashmbuf(struct mbuf *m, SIPHASH_K
   int off;
   struct ether_header *eh;
  #ifdef INET
 - struct ip *ip, ipbuf;
 + struct ip *ip;
 + u_int32_t *ports;
 + int iphlen;
  #endif
  #ifdef INET6
   u_int32_t flow;
   struct ip6_hdr *ip6, ip6buf;
  #endif
 + union {
 +#ifdef INET
 + struct ip ip;
 +#endif
 +#ifdef INET6
 + struct ip6_hdr ip6;
 +#endif
 + uint32_t port;
 +} buf;
   SIPHASH_CTX ctx;
  
   SipHash24_Init(ctx, key);
 @@ -1013,10 +1024,25 @@ trunk_hashmbuf(struct mbuf *m, SIPHASH_K
  #ifdef INET
   case ETHERTYPE_IP:
   if ((ip = (struct ip *)
 - trunk_gethdr(m, off, sizeof(*ip), ipbuf)) == NULL)
 + trunk_gethdr(m, off, sizeof(*ip), buf)) == NULL)
   return (p);
   SipHash24_Update(ctx, ip-ip_src, sizeof(struct in_addr));
   SipHash24_Update(ctx, ip-ip_dst, sizeof(struct in_addr));
 +
 + /* l4 hash */
 + switch (ip-ip_p) {
 + case IPPROTO_TCP:
 + case IPPROTO_UDP:
 + iphlen = ip-ip_hl  2;
 + if (iphlen  sizeof(*ip))
 + break;
 + off += iphlen;
 + ports = (u_int32_t *) trunk_gethdr(m, off, 
 sizeof(*ports), buf);
 + if (ports == NULL)
 + break;
 + SipHash24_Update(ctx, ports, sizeof(*ports));
 + break;
 + }
   break;
  #endif
  #ifdef INET6
 

-- 

route show -priority n

[Sebastian Benoit]

Hi,

this adds a -priority argument to route show to filter on routes of a
certain priority.

With this you can see all ospf routes

  route -n show -priority 32

or any other priority. You can also name some common priorities:

  route -n show -priority bgp

This is useful on a router with a full view routing table where

  route show | grep foo

takes a little bit long.

any oks or other feedback?

diff --git sbin/route/keywords.h sbin/route/keywords.h
index 6174989..2d322ac 100644
--- sbin/route/keywords.h
+++ sbin/route/keywords.h
@@ -1,4 +1,4 @@
-/* $OpenBSD: keywords.h,v 1.29 2015/02/06 03:22:00 reyk Exp $ */
+/* $OpenBSD$ */
 
 /* WARNING!  This file was generated by keywords.sh  */
 
@@ -10,6 +10,7 @@ struct keytab {
 enum {
K_NULL,
K_ADD,
+   K_BGP,
K_BLACKHOLE,
K_CHANGE,
K_CLONING,
@@ -33,6 +34,7 @@ enum {
K_LABEL,
K_LINK,
K_LLINFO,
+   K_LOCAL,
K_LOCK,
K_LOCKREST,
K_MONITOR,
@@ -44,6 +46,7 @@ enum {
K_NETMASK,
K_NOJUMBO,
K_NOSTATIC,
+   K_OSPF,
K_OUT,
K_POP,
K_PREFIXLEN,
@@ -53,6 +56,7 @@ enum {
K_PUSH,
K_RECVPIPE,
K_REJECT,
+   K_RIP,
K_RTT,
K_RTTVAR,
K_SA,
@@ -66,6 +70,7 @@ enum {
 
 struct keytab keywords[] = {
{ add,K_ADD },
+   { bgp,K_BGP },
{ blackhole,  K_BLACKHOLE },
{ change, K_CHANGE },
{ cloning,K_CLONING },
@@ -89,6 +94,7 @@ struct keytab keywords[] = {
{ label,  K_LABEL },
{ link,   K_LINK },
{ llinfo, K_LLINFO },
+   { local,  K_LOCAL },
{ lock,   K_LOCK },
{ lockrest,   K_LOCKREST },
{ monitor,K_MONITOR },
@@ -100,6 +106,7 @@ struct keytab keywords[] = {
{ netmask,K_NETMASK },
{ nojumbo,K_NOJUMBO },
{ nostatic,   K_NOSTATIC },
+   { ospf,   K_OSPF },
{ out,K_OUT },
{ pop,K_POP },
{ prefixlen,  K_PREFIXLEN },
@@ -109,6 +116,7 @@ struct keytab keywords[] = {
{ push,   K_PUSH },
{ recvpipe,   K_RECVPIPE },
{ reject, K_REJECT },
+   { rip,K_RIP },
{ rtt,K_RTT },
{ rttvar, K_RTTVAR },
{ sa, K_SA },
diff --git sbin/route/keywords.sh sbin/route/keywords.sh
index db99593..d4844e2 100644
--- sbin/route/keywords.sh
+++ sbin/route/keywords.sh
@@ -12,6 +12,7 @@ awk=${AWK:-awk}
 cat  _EOF_ | sort  _keywords.t1
 add
 blackhole
+bgp
 change
 cloning
 delete
@@ -34,6 +35,7 @@ jumbo
 label
 link
 llinfo
+local
 lock
 lockrest
 monitor
@@ -45,6 +47,7 @@ net
 netmask
 nojumbo
 nostatic
+ospf
 out
 pop
 prefixlen
@@ -54,6 +57,7 @@ proto2
 push
 recvpipe
 reject
+rip
 rtt
 rttvar
 sa
diff --git sbin/route/route.8 sbin/route/route.8
index d867e87..efe1c77 100644
--- sbin/route/route.8
+++ sbin/route/route.8
@@ -165,6 +165,7 @@ are shown.
 .Op Ar family
 .Op Fl gateway
 .Op Fl label Ar label
+.Op Fl priority Ar priority
 .Xc
 Print out the route table similar to netstat -r (see
 .Xr netstat 1 ) .
@@ -177,6 +178,11 @@ same address family as the destination are shown.
 If
 .Fl label
 is specified, only routes with the specified label are shown.
+.Pp
+If
+.Fl priority
+is specified, only routes with the specified (numeric) priority are shown.
+Some well known priorities can be given by name.
 .El
 .Pp
 The other commands relating to adding, changing, or deleting routes
diff --git sbin/route/route.c sbin/route/route.c
index c1445db..de37b6f 100644
--- sbin/route/route.c
+++ sbin/route/route.c
@@ -662,7 +662,10 @@ newroute(int argc, char **argv)
 void
 show(int argc, char *argv[])
 {
-   int af = 0;
+   int  af = 0;
+   u_char   prio = 0;
+   char*priostr;
+   const char  *errstr;
 
while (--argc  0) {
if (**(++argv)== '-')
@@ -687,6 +690,34 @@ show(int argc, char *argv[])
usage(1+*argv);
getlabel(*++argv);
break;
+   case K_PRIORITY:
+   if (!--argc)
+   usage(1+*argv);
+   priostr = *++argv;
+   switch (keyword(priostr)) {
+   case K_LOCAL:
+   prio = RTP_LOCAL;
+   break;
+   case K_STATIC:
+   prio = RTP_STATIC;
+   break;
+   case K_OSPF:
+   prio = RTP_OSPF;
+   break;
+   case K_RIP:
+   

Re: Byte range implementation for httpd(8)

[Sebastian Benoit]

Florian Obser(flor...@openbsd.org) on 2015.05.03 12:39:02 +:
 On Sun, May 03, 2015 at 01:46:56PM +0200, Sunil Nimmagadda wrote:
  On Sat, May 02, 2015 at 02:49:30PM +, Florian Obser wrote:
   Sorry for the very late reply, I'm currently very busy :/
  
  Thank you for taking time to review it. A new patch with style nits
  fixed and a gratuitous NULL check removed.
  
  [trimming some text]
   
   this is missing the server_file_method() song and dance from
   server_file_request()
  
  Fixed in a slightly different way than using server_file_method().
  Since range request should be ignored for methods other than GET,
  fallback to server_file_request() for further processing/validation.
 
 yep, this is good.
 
  
+   if ((range = parse_range(range_str, st-st_size, nranges)) == 
NULL) {
+   code = 416;
+   (void)snprintf(content_range, sizeof(content_range),
+   bytes */%lld, st-st_size);
+   errstr = content_range;
+   goto abort;
+   }
   
   hm, apache answers with the full file and 200 if the range header is
   syntactically incorrect or if end  start. As far as I can tell it
   only answers 416 if the range actually lies outside of the file.
  
  I am confused about the RFC here, section 4.4 states that 416 is
  returned if ...the set of ranges requested has been rejected due
  to invalid ranges... and a note follows immediately, Because
  servers are free to ignore Range, many implementations will simply
  respond with the entire selected representation in a 200 response
  
  As you noted apache returns 200 while nginx returns 416 for an
  incorrect range. What should httpd do ideally?
 
 Thanks for checking nginx, I don't have one around.
 I noted this for 2 reasons:
 1) I guess apache is the defacto standard, so if it behaves
 differently there might be a reason, maybe broken clients in the wild.
 2) The way the code is currently structured it will require a bit of
 shuffling around if we ever want apache's behaviour.
 
 That being said, my understanding of the RFC is that your
 implementation is correct. And with nginx behaving this way, too, it
 shouldn't bite us in the future. :)
 
  
+
+   /* Accept byte ranges */
+   if (code == 200 
+   kv_add(resp-http_headers, Accept-Ranges, bytes) == 
NULL)
return (-1);
   
   I don't think we should advertise ranges for all 200 results, for
   example we don't support it for directory indexes.
  
  Agree, since RFC says server MAY send Accept-Range header and
  clients MAY generate range requests without having received this
  header field, dropping this header shouldn't make a difference.
  
  Comments?
 
 This looks good.
 
 OK florian@ if someone wants to commit it. Or give me an OK and I'll
 commit.


fwiw i dont know much about ranges, but this reads ok. two whitespace bits
below. otherwise ok benno.

one question though: whats the reasoning behind MAX_RANGES 4? nginx seems to
have a default of unlimited (which i think questionable), but what is
reasonably seen on the internet?

  
  Index: server_file.c
  ===
  RCS file: /cvs/src/usr.sbin/httpd/server_file.c,v
  retrieving revision 1.52
  diff -u -p -r1.52 server_file.c
  --- server_file.c   25 Apr 2015 14:40:35 -  1.52
  +++ server_file.c   3 May 2015 11:18:07 -
  @@ -36,12 +36,25 @@
   
   #define MINIMUM(a, b)  (((a)  (b)) ? (a) : (b))
   #define MAXIMUM(a, b)  (((a)  (b)) ? (a) : (b))
  +#define MAX_RANGES 4
   
  -int server_file_access(struct httpd *, struct client *, char *, 
  size_t);
  -int server_file_request(struct httpd *, struct client *, char *,
  -   struct stat *);
  -int server_file_index(struct httpd *, struct client *, struct stat 
  *);
  -int server_file_method(struct client *);
  +struct range {
  +   off_t   start;
  +   off_t   end;
  +};
  +
  +int server_file_access(struct httpd *, struct client *,
  +   char *, size_t);
  +int server_file_request(struct httpd *, struct client *,
  +   char *, struct stat *);
  +int server_partial_file_request(struct httpd *, struct 
  client *,
  +   char *, struct stat *, char *);
  +int server_file_index(struct httpd *, struct client *,
  +   struct stat *);
  +int server_file_method(struct client *);
  +int parse_range_spec(char *, size_t, struct range *);
  +struct range   *parse_range(char *, size_t, int *);
  +int buffer_add_range(int, struct evbuffer *, struct range 
  *);
   
   int
   server_file_access(struct httpd *env, struct client *clt,
  @@ -50,6 +63,7 @@ server_file_access(struct httpd *env, st
  struct http_descriptor  *desc = clt-clt_descreq;
  struct server_config

Re: pfctl -ss -R

[Sebastian Benoit]

Mike Belopuhov(m...@belopuhov.com) on 2015.06.09 16:23:04 +0200:
 Hi,
 
 Any idea why don't we support filtering the show states output
 by the associated rule number?

indeed, why not?
 
 Diff below works fine here, OK?

ok!
 
 Index: pfctl.c
 ===
 RCS file: /home/cvs/src/sbin/pfctl/pfctl.c,v
 retrieving revision 1.329
 diff -u -p -r1.329 pfctl.c
 --- pfctl.c   16 Jan 2015 06:40:00 -  1.329
 +++ pfctl.c   9 Jun 2015 14:14:49 -
 @@ -84,7 +84,7 @@ void pfctl_print_rule_counters(struct p
  int   pfctl_show_rules(int, char *, int, enum pfctl_show, char *, int, int,
   long);
  int   pfctl_show_src_nodes(int, int);
 -int   pfctl_show_states(int, const char *, int);
 +int   pfctl_show_states(int, const char *, int, long);
  int   pfctl_show_status(int, int);
  int   pfctl_show_timeouts(int, int);
  int   pfctl_show_limits(int, int);
 @@ -945,7 +945,7 @@ done:
  }
  
  int
 -pfctl_show_states(int dev, const char *iface, int opts)
 +pfctl_show_states(int dev, const char *iface, int opts, long shownr)
  {
   struct pfioc_states ps;
   struct pfsync_state *p;
 @@ -985,7 +985,8 @@ pfctl_show_states(int dev, const char *i
   pfctl_print_title(STATES:);
   dotitle = 0;
   }
 - print_state(p, opts);
 + if (shownr  0 || ntohl(p-rule) == shownr)
 + print_state(p, opts);
   }
  done:
   free(inbuf);
 @@ -2309,7 +2310,7 @@ main(int argc, char *argv[])
   opts  PF_OPT_VERBOSE2);
   break;
   case 's':
 - pfctl_show_states(dev, ifaceopt, opts);
 + pfctl_show_states(dev, ifaceopt, opts, shownr);
   break;
   case 'S':
   pfctl_show_src_nodes(dev, opts);
 @@ -2329,7 +2330,7 @@ main(int argc, char *argv[])
  
   pfctl_show_rules(dev, path, opts, 0, anchorname,
   0, 0, -1);
 - pfctl_show_states(dev, ifaceopt, opts);
 + pfctl_show_states(dev, ifaceopt, opts, -1);
   pfctl_show_src_nodes(dev, opts);
   pfctl_show_status(dev, opts);
   pfctl_show_rules(dev, path, opts, 1, anchorname,
 

-- 

Re: syslogd in foreground

[Sebastian Benoit]

Alexander Bluhm(alexander.bl...@gmx.net) on 2015.06.12 01:07:57 +0200:
 Hi,
 
 I need a syslogd running in foreground for a project.  FreeBSD
 also uses the option -F for that.
 
 Do we want this feature in OpenBSD?

i dont see why not, and -d does obviously too much.
-F is fine, nobody else has another flag

 ok?

ok.

maybe the manpage should use the same wording as for -d: 
do not disassociate from the controlling terminal?

 bluhm
 
 Index: usr.sbin/syslogd/privsep.c
 ===
 RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/privsep.c,v
 retrieving revision 1.51
 diff -u -p -r1.51 privsep.c
 --- usr.sbin/syslogd/privsep.c19 Jan 2015 16:40:49 -  1.51
 +++ usr.sbin/syslogd/privsep.c11 Jun 2015 22:58:31 -
 @@ -151,10 +151,10 @@ priv_init(char *conf, int numeric, int l
   }
  
   if (!Debug) {
 - close(lockfd);
   dup2(nullfd, STDIN_FILENO);
   dup2(nullfd, STDOUT_FILENO);
   dup2(nullfd, STDERR_FILENO);
 + close(lockfd);
   }
   if (nullfd  2)
   close(nullfd);
 Index: usr.sbin/syslogd/syslogd.8
 ===
 RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/syslogd.8,v
 retrieving revision 1.33
 diff -u -p -r1.33 syslogd.8
 --- usr.sbin/syslogd/syslogd.830 Jan 2015 14:09:49 -  1.33
 +++ usr.sbin/syslogd/syslogd.811 Jun 2015 22:55:14 -
 @@ -39,7 +39,7 @@
  .Sh SYNOPSIS
  .Nm syslogd
  .Bk -words
 -.Op Fl 46dhnuV
 +.Op Fl 46dFhnuV
  .Op Fl a Ar path
  .Op Fl C Ar CAfile
  .Op Fl f Ar config_file
 @@ -85,6 +85,8 @@ and do not disassociate from the control
  Specify the pathname of an alternate configuration file;
  the default is
  .Pa /etc/syslog.conf .
 +.It Fl F
 +Do not daemonize and stay in foreground.
  .It Fl h
  Include the hostname when forwarding messages to a remote host.
  .It Fl m Ar mark_interval
 Index: usr.sbin/syslogd/syslogd.c
 ===
 RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/syslogd.c,v
 retrieving revision 1.161
 diff -u -p -r1.161 syslogd.c
 --- usr.sbin/syslogd/syslogd.c30 Mar 2015 09:21:42 -  1.161
 +++ usr.sbin/syslogd/syslogd.c11 Jun 2015 22:59:10 -
 @@ -205,6 +205,7 @@ structfiled consfile;
  int  nunix = 1;  /* Number of Unix domain sockets requested */
  char *path_unix[MAXUNIX] = { _PATH_LOG }; /* Paths to Unix domain sockets */
  int  Debug;  /* debug flag */
 +int  Foreground; /* run in foreground, instead of daemonizing */
  int  Startup = 1;/* startup flag */
  char LocalHostName[HOST_NAME_MAX+1]; /* our hostname */
  char *LocalDomain;   /* our local domain name */
 @@ -328,7 +329,7 @@ main(int argc, char *argv[])
   int  ch, i;
   int  lockpipe[2] = { -1, -1}, pair[2], nullfd, fd;
  
 - while ((ch = getopt(argc, argv, 46C:dhnuf:m:p:a:s:V)) != -1)
 + while ((ch = getopt(argc, argv, 46C:dhnuf:Fm:p:a:s:V)) != -1)
   switch (ch) {
   case '4':   /* disable IPv6 */
   IPv4Only = 1;
 @@ -347,6 +348,9 @@ main(int argc, char *argv[])
   case 'f':   /* configuration file */
   ConfFile = optarg;
   break;
 + case 'F':   /* foreground */
 + Foreground = 1;
 + break;
   case 'h':   /* RFC 3164 hostnames */
   IncludeHostname = 1;
   break;
 @@ -557,7 +561,7 @@ main(int argc, char *argv[])
  
   tzset();
  
 - if (!Debug) {
 + if (!Debug  !Foreground) {
   char c;
  
   pipe(lockpipe);
 @@ -969,7 +973,7 @@ usage(void)
  {
  
   (void)fprintf(stderr,
 - usage: syslogd [-46dhnuV] [-a path] [-C CAfile] [-f config_file]\n
 + usage: syslogd [-46dFhnuV] [-a path] [-C CAfile] [-f 
 config_file]\n
  [-m mark_interval] [-p log_socket] [-s 
 reporting_socket]\n);
   exit(1);
  }
 

-- 

Re: [PATCH] Enable -f in ndp(8)

[Sebastian Benoit]

There is no reason to remove this in arp.

As for ndp/ipv6, i'm not sure.
Is there anyone adding large numbers of ndp entries? Why?

/Benno


Dimitris Papastamos(s...@2f30.org) on 2015.07.25 21:11:41 +0100:
 On Sat, Jul 25, 2015 at 09:20:18PM +0200, Martin Pieuchot wrote:
  On 13/07/15(Mon) 14:04, Dimitris Papastamos wrote:
   Hi,
   
   I noticed -f in ndp(8) did nothing at all so I've enabled it and
   documented the file syntax in the man page.
  
  If it does nothing, I'd say let's kill it.
  
 
 Index: ndp.8
 ===
 RCS file: /cvs/src/usr.sbin/ndp/ndp.8,v
 retrieving revision 1.33
 diff -u -p -r1.33 ndp.8
 --- ndp.8 3 Sep 2014 10:39:41 -   1.33
 +++ ndp.8 25 Jul 2015 20:10:08 -
 @@ -44,7 +44,6 @@
  .Op Fl H | P | R
  .Op Fl A Ar wait
  .Op Fl d Ar hostname
 -.Op Fl f Ar filename
  .Op Fl i Ar interface Op Ar flag ...
  .Op Fl s Ar nodename etheraddr Oo Ic temp Oc Op Ic proxy
  .Op Fl V Ar rdomain
 @@ -119,9 +118,6 @@ the node has sent during the current sta
  Erase all the NDP entries.
  .It Fl d Ar hostname
  Delete the specified NDP entry.
 -.It Fl f Ar filename
 -Parse the file specified by
 -.Ar filename .
  .It Fl H
  Harmonize consistency between the routing table and the default router
  list; install the top entry of the list into the kernel routing table.
 Index: ndp.c
 ===
 RCS file: /cvs/src/usr.sbin/ndp/ndp.c,v
 retrieving revision 1.61
 diff -u -p -r1.61 ndp.c
 --- ndp.c 3 Jun 2015 08:10:53 -   1.61
 +++ ndp.c 25 Jul 2015 20:10:08 -
 @@ -123,7 +123,6 @@ char ntop_buf[INET6_ADDRSTRLEN];  /* inet
  char host_buf[NI_MAXHOST];   /* getnameinfo() */
  char ifix_buf[IFNAMSIZ]; /* if_indextoname() */
  
 -int file(char *);
  void getsocket(void);
  int set(int, char **);
  void get(char *);
 @@ -294,41 +293,6 @@ main(int argc, char *argv[])
   exit(0);
  }
  
 -/*
 - * Process a file to set standard ndp entries
 - */
 -int
 -file(char *name)
 -{
 - FILE *fp;
 - int i, retval;
 - char line[100], arg[5][50], *args[5];
 -
 - if ((fp = fopen(name, r)) == NULL) {
 - fprintf(stderr, ndp: cannot open %s\n, name);
 - exit(1);
 - }
 - args[0] = arg[0][0];
 - args[1] = arg[1][0];
 - args[2] = arg[2][0];
 - args[3] = arg[3][0];
 - args[4] = arg[4][0];
 - retval = 0;
 - while (fgets(line, sizeof(line), fp) != NULL) {
 - i = sscanf(line, %49s %49s %49s %49s %49s,
 - arg[0], arg[1], arg[2], arg[3], arg[4]);
 - if (i  2) {
 - fprintf(stderr, ndp: bad line: %s\n, line);
 - retval = 1;
 - continue;
 - }
 - if (set(i, args))
 - retval = 1;
 - }
 - fclose(fp);
 - return (retval);
 -}
 -
  void
  getsocket(void)
  {
 @@ -792,7 +756,7 @@ usage(void)
  {
   printf(usage: ndp [-nrt] [-a | -c | -p] [-H | -P | -R] );
   printf([-A wait] [-d hostname]\n);
 - printf(\t[-f filename] [-i interface [flag ...]]\n);
 + printf(\t[-i interface [flag ...]]\n);
   printf(\t[-s nodename etheraddr [temp] [proxy]] );
   printf([-V rdomain] [hostname]\n);
   exit(1);
 

-- 

Re: [PATCH] Enable -f in ndp(8)

[Sebastian Benoit]

Martin Pieuchot(m...@openbsd.org) on 2015.08.02 16:20:15 +0200:
 On 02/08/15(Sun) 14:24, Sebastian Benoit wrote:
  There is no reason to remove this in arp.
 
 Does that mean you use it?  If yes, could you take care of the first
 diff?

i used arp -f in the past. i'm not sure where you need it in v6, but i
surely can commit this after unlock.

/Benno

Re: enable unbound-control in default config

[Sebastian Benoit]

ok

Stuart Henderson(st...@openbsd.org) on 2015.07.19 17:55:00 +0100:
 In the past, the only option for unbound-control was a TCP socket
 using SSL/TLS, but nowadays it also supports unix domain sockets,
 so it seems like it would be reasonable to enable it by default
 in our configuration so that users added to the _unbound group
 can access stats and do various types of runtime reconfiguration.
 We can then also get rid of the not-squeaky-clean config check
 and cert generation from the rc script.
 
 I've been using this for ages. Any comments? OK?
 
 srw-rw  1 _unbound  _unbound  0 Jul 18 08:43 /var/run/unbound.sock
 
 Index: rc.d/unbound
 ===
 RCS file: /cvs/src/etc/rc.d/unbound,v
 retrieving revision 1.2
 diff -u -p -r1.2 unbound
 --- rc.d/unbound  29 Dec 2014 11:17:43 -  1.2
 +++ rc.d/unbound  19 Jul 2015 16:47:12 -
 @@ -10,14 +10,6 @@ daemon_flags=-c /var/unbound/etc/unboun
  pexp=unbound${daemon_flags:+ ${daemon_flags}}
  
  rc_pre() {
 - if grep '^[[:space:]]*control-enable:[[:space:]]*yes' \
 - /var/unbound/etc/unbound.conf  /dev/null 21  \
 - ! [[ -f /var/unbound/etc/unbound_server.key ||
 - -f /var/unbound/etc/unbound_server.pem ||
 - -f /var/unbound/etc/unbound_control.key ||
 - -f /var/unbound/etc/unbound_control.pem ]]; then
 - /usr/sbin/unbound-control-setup 2 /dev/null
 - fi
   if grep '^[[:space:]]*auto-trust-anchor-file:' \
  /var/unbound/etc/unbound.conf  /dev/null 21; then
   /usr/sbin/unbound-anchor -v || true
 Index: unbound.conf
 ===
 RCS file: /cvs/src/etc/unbound.conf,v
 retrieving revision 1.4
 diff -u -p -r1.4 unbound.conf
 --- unbound.conf  2 Apr 2014 21:43:30 -   1.4
 +++ unbound.conf  19 Jul 2015 16:47:12 -
 @@ -37,6 +37,11 @@ server:
   #
   #tcp-upstream: yes
  
 +remote-control:
 + control-enable: yes
 + control-use-cert: no
 + control-interface: /var/run/unbound.sock
 +
  # Use an upstream forwarder (recursive resolver) for specific zones.
  # Example addresses given below are public resolvers valid as of 2014/03.
  #
 

-- 

Re: [PATCH] Fix ospfd segmentation fault on startup

[Sebastian Benoit]

Johan Ymerson(johan.ymer...@transmode.com) on 2015.07.20 21:32:20 +:
 On Mon, 2015-07-20 at 22:58 +0200, Martin Pieuchot wrote:
  On 20/07/15(Mon) 19:10, Johan Ymerson wrote:
   On 2015-07-18 16:03:00, Martin Pieuchot wrote:
Committed!  Thanks and sorry for the delay.
   
   Hi!
   
   You missed the previous patch Fix ospfd segmentation fault on startup
   witch prevent ospfd from segfaulting on startup. Without this first
   patch, ospfd will almost always segfault on startup (instead of just
   sometime, which it does today).
  
  Could you send a single diff for all these issues?  Apparently ospf
  hackers are slacking ;)
 
 Yes, I have it as a single diff. I actually broke it up in two diffs
 because the two issues are not really related. The second patch only
 makes the first problem very obvious.
 It would of course be best if we could make sure we set up event
 handling (iev_ospfe et al.) before scanning interfaces, but it's kind of
 a catch 22 here. The event handlers need the interface info to not
 segfault... So the easy fix was to just check for null pointers in
 main_imsg_compose_*.
 
 /Johan

fixed it.

ok benno@

 
 
 
 Index: interface.c
 ===
 RCS file: /cvs/src/usr.sbin/ospfd/interface.c,v
 retrieving revision 1.75
 diff -u -p -r1.75 interface.c
 --- interface.c 14 May 2012 10:17:21 -  1.75
 +++ interface.c 27 May 2015 16:42:51 -
 @@ -338,8 +338,10 @@ if_act_start(struct iface *iface)
 struct in_addr   addr;
 struct timeval   now;
  
 -   if (!((iface-flags  IFF_UP) 
 -   LINK_STATE_IS_UP(iface-linkstate)))
 +   if (!(iface-flags  IFF_UP) ||
 +   (!LINK_STATE_IS_UP(iface-linkstate) 
 +   !(iface-media_type == IFT_CARP 
 +   iface-linkstate == LINK_STATE_DOWN)))
 return (0);
  
 if (iface-media_type == IFT_CARP  iface-passive == 0) {
 Index: kroute.c
 ===
 RCS file: /cvs/src/usr.sbin/ospfd/kroute.c,v
 retrieving revision 1.98
 diff -u -p -r1.98 kroute.c
 --- kroute.c11 Feb 2015 05:57:44 -  1.98
 +++ kroute.c27 May 2015 16:42:51 -
 @@ -1019,6 +1019,9 @@ if_change(u_short ifindex, int flags, st
 return;
 }
  
 +   /* notify ospfe about interface link state */
 +   main_imsg_compose_ospfe(IMSG_IFINFO, 0, kif, sizeof(struct kif));
 +
 reachable = (kif-flags  IFF_UP) 
 LINK_STATE_IS_UP(kif-link_state);
  
 @@ -1026,9 +1029,6 @@ if_change(u_short ifindex, int flags, st
 return; /* nothing changed wrt nexthop validity */
  
 kif-nh_reachable = reachable;
 -
 -   /* notify ospfe about interface link state */
 -   main_imsg_compose_ospfe(IMSG_IFINFO, 0, kif, sizeof(struct kif));
  
 /* update redistribute list */
 RB_FOREACH(kr, kroute_tree, krt) {
 Index: ospfd.c
 ===
 RCS file: /cvs/src/usr.sbin/ospfd/ospfd.c,v
 retrieving revision 1.83
 diff -u -p -r1.83 ospfd.c
 --- ospfd.c 10 Feb 2015 05:24:48 -  1.83
 +++ ospfd.c 27 May 2015 16:42:51 -
 @@ -511,13 +511,15 @@ main_dispatch_rde(int fd, short event, v
  void
  main_imsg_compose_ospfe(int type, pid_t pid, void *data, u_int16_t datalen)
  {
 -   imsg_compose_event(iev_ospfe, type, 0, pid, -1, data, datalen);
 +   if (iev_ospfe)
 +   imsg_compose_event(iev_ospfe, type, 0, pid, -1, data, 
 datalen);
  }
  
  void
  main_imsg_compose_rde(int type, pid_t pid, void *data, u_int16_t datalen)
  {
 -   imsg_compose_event(iev_rde, type, 0, pid, -1, data, datalen);
 +   if (iev_rde)
 +   imsg_compose_event(iev_rde, type, 0, pid, -1, data, datalen);
  }
  
  void
 

-- 

Re: [PATCH] Fix ospfd segmentation fault on startup

[Sebastian Benoit]

commited, thx for your diff.

/Benno

Johan Ymerson(johan.ymer...@transmode.com) on 2015.07.20 21:32:20 +:
 On Mon, 2015-07-20 at 22:58 +0200, Martin Pieuchot wrote:
  On 20/07/15(Mon) 19:10, Johan Ymerson wrote:
   On 2015-07-18 16:03:00, Martin Pieuchot wrote:
Committed!  Thanks and sorry for the delay.
   
   Hi!
   
   You missed the previous patch Fix ospfd segmentation fault on startup
   witch prevent ospfd from segfaulting on startup. Without this first
   patch, ospfd will almost always segfault on startup (instead of just
   sometime, which it does today).
  
  Could you send a single diff for all these issues?  Apparently ospf
  hackers are slacking ;)
 
 Yes, I have it as a single diff. I actually broke it up in two diffs
 because the two issues are not really related. The second patch only
 makes the first problem very obvious.
 It would of course be best if we could make sure we set up event
 handling (iev_ospfe et al.) before scanning interfaces, but it's kind of
 a catch 22 here. The event handlers need the interface info to not
 segfault... So the easy fix was to just check for null pointers in
 main_imsg_compose_*.
 
 /Johan
 
 
 
 Index: interface.c
 ===
 RCS file: /cvs/src/usr.sbin/ospfd/interface.c,v
 retrieving revision 1.75
 diff -u -p -r1.75 interface.c
 --- interface.c 14 May 2012 10:17:21 -  1.75
 +++ interface.c 27 May 2015 16:42:51 -
 @@ -338,8 +338,10 @@ if_act_start(struct iface *iface)
 struct in_addr   addr;
 struct timeval   now;
  
 -   if (!((iface-flags  IFF_UP) 
 -   LINK_STATE_IS_UP(iface-linkstate)))
 +   if (!(iface-flags  IFF_UP) ||
 +   (!LINK_STATE_IS_UP(iface-linkstate) 
 +   !(iface-media_type == IFT_CARP 
 +   iface-linkstate == LINK_STATE_DOWN)))
 return (0);
  
 if (iface-media_type == IFT_CARP  iface-passive == 0) {
 Index: kroute.c
 ===
 RCS file: /cvs/src/usr.sbin/ospfd/kroute.c,v
 retrieving revision 1.98
 diff -u -p -r1.98 kroute.c
 --- kroute.c11 Feb 2015 05:57:44 -  1.98
 +++ kroute.c27 May 2015 16:42:51 -
 @@ -1019,6 +1019,9 @@ if_change(u_short ifindex, int flags, st
 return;
 }
  
 +   /* notify ospfe about interface link state */
 +   main_imsg_compose_ospfe(IMSG_IFINFO, 0, kif, sizeof(struct kif));
 +
 reachable = (kif-flags  IFF_UP) 
 LINK_STATE_IS_UP(kif-link_state);
  
 @@ -1026,9 +1029,6 @@ if_change(u_short ifindex, int flags, st
 return; /* nothing changed wrt nexthop validity */
  
 kif-nh_reachable = reachable;
 -
 -   /* notify ospfe about interface link state */
 -   main_imsg_compose_ospfe(IMSG_IFINFO, 0, kif, sizeof(struct kif));
  
 /* update redistribute list */
 RB_FOREACH(kr, kroute_tree, krt) {
 Index: ospfd.c
 ===
 RCS file: /cvs/src/usr.sbin/ospfd/ospfd.c,v
 retrieving revision 1.83
 diff -u -p -r1.83 ospfd.c
 --- ospfd.c 10 Feb 2015 05:24:48 -  1.83
 +++ ospfd.c 27 May 2015 16:42:51 -
 @@ -511,13 +511,15 @@ main_dispatch_rde(int fd, short event, v
  void
  main_imsg_compose_ospfe(int type, pid_t pid, void *data, u_int16_t datalen)
  {
 -   imsg_compose_event(iev_ospfe, type, 0, pid, -1, data, datalen);
 +   if (iev_ospfe)
 +   imsg_compose_event(iev_ospfe, type, 0, pid, -1, data, 
 datalen);
  }
  
  void
  main_imsg_compose_rde(int type, pid_t pid, void *data, u_int16_t datalen)
  {
 -   imsg_compose_event(iev_rde, type, 0, pid, -1, data, datalen);
 +   if (iev_rde)
 +   imsg_compose_event(iev_rde, type, 0, pid, -1, data, datalen);
  }
  
  void
 

-- 

Re: [patch] tcpdump segfault on invalid DECnet packet

[Sebastian Benoit]

Stuart Henderson(st...@openbsd.org) on 2015.10.20 16:37:58 +0100:
> On 2015/10/14 11:11, Kevin Reay wrote:
> > Thanks for the review and feedback.
> > Updated patch with removed whitespace changes included.
> 
> This is fine with me. Any OKs to commit it?

yes, ok
 
> > Index: print-decnet.c
> > ===
> > RCS file: /cvs/src/usr.sbin/tcpdump/print-decnet.c,v
> > retrieving revision 1.14
> > diff -u -p -r1.14 print-decnet.c
> > --- print-decnet.c  21 Aug 2015 02:07:32 -  1.14
> > +++ print-decnet.c  14 Oct 2015 22:49:03 -
> > @@ -44,13 +44,13 @@ struct rtentry;
> >  #include "addrtoname.h"
> >  
> >  /* Forwards */
> > -static void print_decnet_ctlmsg(const union routehdr *, u_int);
> > +static int print_decnet_ctlmsg(const union routehdr *, u_int, u_int);
> >  static void print_t_info(int);
> > -static void print_l1_routes(const char *, u_int);
> > -static void print_l2_routes(const char *, u_int);
> > +static int print_l1_routes(const char *, u_int);
> > +static int print_l2_routes(const char *, u_int);
> >  static void print_i_info(int);
> > -static void print_elist(const char *, u_int);
> > -static void print_nsp(const u_char *, u_int);
> > +static int print_elist(const char *, u_int);
> > +static int print_nsp(const u_char *, u_int);
> >  static void print_reason(int);
> >  #ifdef PRINT_NSPDATA
> >  static void pdata(u_char *, int);
> > @@ -76,12 +76,23 @@ decnet_print(register const u_char *ap, 
> > return;
> > }
> >  
> > +   TCHECK2(*ap, sizeof(short));
> > pktlen = EXTRACT_LE_16BITS(ap);
> > +   if (pktlen < sizeof(struct shorthdr)) {
> > +   (void)printf("[|decnet]");
> > +   return;
> > +   }
> > +   if (pktlen > length) {
> > +   (void)printf("[|decnet]");
> > +   return;
> > +   }
> > +   length = pktlen;
> >  
> > rhlen = min(length, caplen);
> > rhlen = min(rhlen, sizeof(*rhp));
> > memcpy((char *)rhp, (char *)&(ap[sizeof(short)]), rhlen);
> >  
> > +   TCHECK(rhp->rh_short.sh_flags);
> > mflags = EXTRACT_LE_8BITS(rhp->rh_short.sh_flags);
> >  
> > if (mflags & RMF_PAD) {
> > @@ -89,6 +100,11 @@ decnet_print(register const u_char *ap, 
> > u_int padlen = mflags & RMF_PADMASK;
> > if (vflag)
> > (void) printf("[pad:%d] ", padlen);
> > +   if (length < padlen + 2) {
> > +   (void)printf("[|decnet]");
> > +   return;
> > +   }
> > +   TCHECK2(ap[sizeof(short)], padlen);
> > ap += padlen;
> > length -= padlen;
> > caplen -= padlen;
> > @@ -100,38 +116,43 @@ decnet_print(register const u_char *ap, 
> >  
> > if (mflags & RMF_FVER) {
> > (void) printf("future-version-decnet");
> > -   default_print(ap, length);
> > +   default_print(ap, min(length, caplen));
> > return;
> > }
> >  
> > /* is it a control message? */
> > if (mflags & RMF_CTLMSG) {
> > -   print_decnet_ctlmsg(rhp, min(length, caplen));
> > +   if(!print_decnet_ctlmsg(rhp, length, caplen))
> > +   goto trunc;
> > return;
> > }
> >  
> > switch (mflags & RMF_MASK) {
> > case RMF_LONG:
> > +   if (length < sizeof(struct longhdr)) {
> > +   (void)printf("[|decnet]");
> > +   return;
> > +   }
> > +   TCHECK(rhp->rh_long);
> > dst =
> > EXTRACT_LE_16BITS(rhp->rh_long.lg_dst.dne_remote.dne_nodeaddr);
> > src =
> > EXTRACT_LE_16BITS(rhp->rh_long.lg_src.dne_remote.dne_nodeaddr);
> > hops = EXTRACT_LE_8BITS(rhp->rh_long.lg_visits);
> > nspp = &(ap[sizeof(short) + sizeof(struct longhdr)]);
> > -   nsplen = min((length - sizeof(struct longhdr)),
> > -(caplen - sizeof(struct longhdr)));
> > +   nsplen = length - sizeof(struct longhdr);
> > break;
> > case RMF_SHORT:
> > +   TCHECK(rhp->rh_short);
> > dst = EXTRACT_LE_16BITS(rhp->rh_short.sh_dst);
> > src = EXTRACT_LE_16BITS(rhp->rh_short.sh_src);
> > hops = (EXTRACT_LE_8BITS(rhp->rh_short.sh_visits) & VIS_MASK)+1;
> > nspp = &(ap[sizeof(short) + sizeof(struct shorthdr)]);
> > -   nsplen = min((length - sizeof(struct shorthdr)),
> > -(caplen - sizeof(struct shorthdr)));
> > +   nsplen = length - sizeof(struct shorthdr);
> > break;
> > default:
> > (void) printf("unknown message flags under mask");
> > -   default_print((u_char *)ap, length);
> > +   default_print((u_char *)ap, min(length, caplen));
> > return;
> > }
> >  
> > @@ -147,11 +168,18 @@ decnet_print(register const u_char *ap, 
> > (void)printf("%d hops ", hops);
> > }
> >  
> > -   print_nsp(nspp, nsplen);
> > +   if (!print_nsp(nspp, nsplen))
> > +   goto trunc;
> > +   return;
> > +
> > +trunc:
> > +   (void)printf("[|decnet]");
> > +   return;
> >  }
> > 

Re: The router doesn't know the size of the internet...

[Sebastian Benoit]

Alexander Bluhm(alexander.bl...@gmx.net) on 2015.10.24 17:21:27 +0200:
> On Sat, Oct 24, 2015 at 04:02:59PM +0200, Martin Pieuchot wrote:
> > ...at least better than OpenBSD's source code.
> > 
> > This diff gets rid of the horrible per-ifp autoconf'd-ndp only hoplimit.
> > Alexander verified that this is not mandatory in the corresponding RFCs
> > and what really matters is the per-PCB specified hop limit.
> > 
> > See how this simplifies a lot of rt_ifp usages?
> > 
> > Ok?
> 
> It is a SHOULD in RFC 4861 and we are OpenBSD and don't believe in
> what those Cisco routers claim anyway.
> 
>Router Advertisement messages also contain Internet parameters such
>as the hop limit that hosts should use in outgoing packets and,
>optionally, link parameters such as the link MTU.  This facilitates
>centralized administration of critical parameters that can be set on
>routers and automatically propagated to all attached hosts.
> 
>If the received Cur Hop Limit value is non-zero, the host SHOULD set
>its CurHopLimit variable to the received value.
> 
> > +   if (nd_ra->nd_ra_curhoplimit) {
> > +   /*
> > +* Ignore it.  The router doesn't know the size of the
> > +* internet better than this source code.
> > +*/
> > +   }
> 
> s/size/diameter/ to quote RFC 2461 correctly
> 
> The value should be set to that current diameter of the Internet.

who measures that and tells me what to set there?

ok on the diff.

Re: route6d: another pidfile() removal

[Sebastian Benoit]

ok!

J??r??mie Courr??ges-Anglas(j...@wxcvbn.org) on 2015.10.25 23:15:12 +0100:
> 
> Following the recent discussions, here's another pidfile(3) removal.
> route6d(8) doesn't document it.
> 
> ok?
> 
> Index: Makefile
> ===
> RCS file: /cvs/src/usr.sbin/route6d/Makefile,v
> retrieving revision 1.7
> diff -u -p -r1.7 Makefile
> --- Makefile  19 Dec 2006 15:06:10 -  1.7
> +++ Makefile  25 Oct 2015 22:12:00 -
> @@ -2,7 +2,5 @@
>  
>  PROG=route6d
>  MAN= route6d.8
> -LDADD+=  -lutil
> -DPADD+=  ${LIBUTIL}
>  
>  .include 
> Index: route6d.c
> ===
> RCS file: /cvs/src/usr.sbin/route6d/route6d.c,v
> retrieving revision 1.70
> diff -u -p -r1.70 route6d.c
> --- route6d.c 18 Oct 2015 14:35:36 -  1.70
> +++ route6d.c 25 Oct 2015 22:12:22 -
> @@ -372,8 +372,6 @@ main(int argc, char *argv[])
>   if (dflag)
>   ifrtdump(0);
>  
> - pidfile(NULL);
> -
>   if ((ripbuf = malloc(RIP6_MAXMTU)) == NULL) {
>   fatal("malloc");
>   /*NOTREACHED*/
> 
> 
> -- 
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE
> 

-- 

Re: calloc -> malloc in get_data() and get_string()

[Sebastian Benoit]

Michael McConville(mm...@mykolab.com) on 2015.10.28 12:05:24 -0400:
> Relayd, httpd, and ntpd define the functions get_data() and
> get_string(). Both call calloc and then immediately memcpy. Calloc's
> zeroing isn't optimized out. These functions are called in network data
> paths in at least a couple places, so all this extra writing could have
> a meaningful performance impact.

in relayd these functions are *not* called in the network path, they are used
when loading the (TLS) configuration data.

they dont have a performance impact.

> 
> 
> Index: relayd.c
> ===
> RCS file: /cvs/src/usr.sbin/relayd/relayd.c,v
> retrieving revision 1.144
> diff -u -p -r1.144 relayd.c
> --- relayd.c  14 Oct 2015 07:58:14 -  1.144
> +++ relayd.c  28 Oct 2015 15:57:16 -
> @@ -1501,9 +1501,10 @@ get_string(u_int8_t *ptr, size_t len)
>   isspace((unsigned char)ptr[i])))
>   break;
>  
> - if ((str = calloc(1, i + 1)) == NULL)
> + if ((str = malloc(i + 1)) == NULL)
>   return (NULL);
>   memcpy(str, ptr, i);
> + str[i] = '\0';
>  
>   return (str);
>  }
> @@ -1513,7 +1514,7 @@ get_data(u_int8_t *ptr, size_t len)
>  {
>   u_int8_t*data;
>  
> - if ((data = calloc(1, len)) == NULL)
> + if ((data = malloc(len)) == NULL)
>   return (NULL);
>   memcpy(data, ptr, len);
>  
> 

-- 

Re: Kill rtable_mpath_match

[Sebastian Benoit]

Martin Pieuchot(m...@openbsd.org) on 2015.10.25 16:14:27 +0100:
> Diff below merges the guts of rtable_mpath_match() into rtable_lookup().
> As for the previous rtable_mpath_* diff this is a step towards MPATH by
> default.
> 
> This diff introduces a behavior change for RTM_GET.  If multiple route
> exists a gateway MUST be specified and the first one in the tree won't
> be automagically selected by the kernel.
> 
> Example:
> 
>   # route -n show -inet |grep 192.168.178/24
>   192.168.178/24 192.168.178.1  UGSP   1  230 - 8 
> vio0 
>   192.168.178/24 192.168.178.2  UGSP   00 - 8 
> vio0 

greping through 500k routes isnt really nice.

>   # route -n get 192.168.178/24
>   route: writing to routing socket: No such process

the error msg is misleading, something like "multiple routes available" or
so would be nice. Otherwise i am lead to think there is no route.

>   # route -n get 192.168.178/24 192.168.178.1
>  route to: 192.168.178.0
>   destination: 192.168.178.0
>  mask: 255.255.255.0
>   gateway: 192.168.178.1
> interface: vio0
>if address: 192.168.178.5
>  priority: 8 (static)
> flags: 
>use   mtuexpire
>326 0 0 
> 
> 
> Is it acceptable?
> 
> Index: net/if.c
> ===
> RCS file: /cvs/src/sys/net/if.c,v
> retrieving revision 1.397
> diff -u -p -r1.397 if.c
> --- net/if.c  25 Oct 2015 13:52:45 -  1.397
> +++ net/if.c  25 Oct 2015 15:06:44 -
> @@ -2360,7 +2360,7 @@ if_group_egress_build(void)
>   bzero(_in, sizeof(sa_in));
>   sa_in.sin_len = sizeof(sa_in);
>   sa_in.sin_family = AF_INET;
> - rt0 = rtable_lookup(0, sintosa(_in), sintosa(_in));
> + rt0 = rtable_lookup(0, sintosa(_in), sintosa(_in), NULL, RTP_ANY);
>   if (rt0 != NULL) {
>   rt = rt0;
>   do {
> @@ -2377,7 +2377,8 @@ if_group_egress_build(void)
>  
>  #ifdef INET6
>   bcopy(_any, _in6, sizeof(sa_in6));
> - rt0 = rtable_lookup(0, sin6tosa(_in6), sin6tosa(_in6));
> + rt0 = rtable_lookup(0, sin6tosa(_in6), sin6tosa(_in6), NULL,
> + RTP_ANY);
>   if (rt0 != NULL) {
>   rt = rt0;
>   do {
> Index: net/route.c
> ===
> RCS file: /cvs/src/sys/net/route.c,v
> retrieving revision 1.263
> diff -u -p -r1.263 route.c
> --- net/route.c   25 Oct 2015 14:48:51 -  1.263
> +++ net/route.c   25 Oct 2015 15:06:45 -
> @@ -733,25 +733,10 @@ rtrequest1(int req, struct rt_addrinfo *
>   switch (req) {
>   case RTM_DELETE:
>   rt = rtable_lookup(tableid, info->rti_info[RTAX_DST],
> - info->rti_info[RTAX_NETMASK]);
> + info->rti_info[RTAX_NETMASK], info->rti_info[RTAX_GATEWAY],
> + prio );
>   if (rt == NULL)
>   return (ESRCH);
> -#ifndef SMALL_KERNEL
> - rt = rtable_mpath_match(tableid, rt,
> - info->rti_info[RTAX_GATEWAY], prio);
> - if (rt == NULL)
> - return (ESRCH);
> -
> - /*
> -  * If we got multipath routes, we require users to specify
> -  * a matching gateway.
> -  */
> - if ((rt->rt_flags & RTF_MPATH) &&
> - info->rti_info[RTAX_GATEWAY] == NULL) {
> - rtfree(rt);
> - return (ESRCH);
> - }
> -#endif
>  
>   /*
>* Since RTP_LOCAL cannot be set by userland, make
> Index: net/rtable.c
> ===
> RCS file: /cvs/src/sys/net/rtable.c,v
> retrieving revision 1.15
> diff -u -p -r1.15 rtable.c
> --- net/rtable.c  25 Oct 2015 14:48:51 -  1.15
> +++ net/rtable.c  25 Oct 2015 15:06:45 -
> @@ -249,7 +249,7 @@ rtable_free(unsigned int rtableid)
>  
>  struct rtentry *
>  rtable_lookup(unsigned int rtableid, struct sockaddr *dst,
> -struct sockaddr *mask)
> +struct sockaddr *mask, struct sockaddr *gateway, uint8_t prio)
>  {
>   struct radix_node_head  *rnh;
>   struct radix_node   *rn;
> @@ -264,6 +264,22 @@ rtable_lookup(unsigned int rtableid, str
>   return (NULL);
>  
>   rt = ((struct rtentry *)rn);
> +
> +#ifndef SMALL_KERNEL
> + if (rnh->rnh_multipath) {
> + rt = rt_mpath_matchgate(rt, gateway, prio);
> + if (rt == NULL)
> + return (NULL);
> + }
> +
> + /*
> +  * If we got multipath routes, we require users to specify
> +  * a matching gateway.
> +  */
> + if (ISSET(rt->rt_flags, RTF_MPATH) && gateway == NULL)
> + return (NULL);
> +#endif /* !SMALL_KERNEL */
> +
>   rtref(rt);
>  
>   return (rt);
> @@ -372,26 +388,6 @@ 

Re: tcpbench pledge

[Sebastian Benoit]

David Hill(dh...@mindcry.org) on 2015.11.10 11:44:39 -0500:
> Hello -
> 
> pledge starts after getopt because of setrtable. 
> 
> rpath needed incase -k (kvm_openfile) 
> proc needed for drop_gid (setresgid)
> 
> I believe I've hit every code path.  More eyes are welcome.

Hi,

two changes

-T at this time does not work with pledge. setsockopt(...IP_TOS) is not
allowed. so i disable it except for the last pledge in the client case.

with -s, we need "inet" in the event loop because there we will do
accept(). move the pledge("stdin") into the client case only.

ok?


diff --git usr.bin/tcpbench/tcpbench.c usr.bin/tcpbench/tcpbench.c
index 6bd2027..e753fbf 100644
--- usr.bin/tcpbench/tcpbench.c
+++ usr.bin/tcpbench/tcpbench.c
@@ -986,8 +986,6 @@ quit(int sig, short event, void *arg)
 int
 main(int argc, char **argv)
 {
-   extern int optind;
-   extern char *optarg;
struct timeval tv;
unsigned int secs, rtable;
 
@@ -1108,6 +1106,16 @@ main(int argc, char **argv)
}
}
 
+   /*
+* XXX pledge
+* -T toskeyword cannot be used with pledge() at this time because
+* the IP_TOS setsockopt(2) is not allowed even with promise "inet"
+* other pledge() calls below need revisiting as well.
+*/
+   if (ptb->Tflag == -1 &&
+   pledge("stdio rpath dns inet id proc", NULL) == -1)
+   err(1, "pledge");
+
argv += optind;
argc -= optind;
if ((argc != (ptb->sflag ? 0 : 1)) ||
@@ -1125,6 +1133,10 @@ main(int argc, char **argv)
} else
drop_gid();
 
+   if (ptb->Tflag == -1 &&
+   pledge("stdio id dns inet", NULL) == -1)
+   err(1, "pledge");
+
if (!ptb->sflag)
host = argv[0];
/*
@@ -1169,6 +1181,10 @@ main(int argc, char **argv)
errx(1, "getaddrinfo: %s", gai_strerror(herr));
}
 
+   if (ptb->Tflag == -1 &&
+   pledge("stdio id inet", NULL) == -1)
+   err(1, "pledge");
+
if (getrlimit(RLIMIT_NOFILE, ) == -1)
err(1, "getrlimit");
if (rl.rlim_cur < MAX_FD)
@@ -1177,7 +1193,11 @@ main(int argc, char **argv)
err(1, "setrlimit");
if (getrlimit(RLIMIT_NOFILE, ) == -1)
err(1, "getrlimit");
-   
+
+   if (ptb->Tflag == -1 &&
+   pledge("stdio inet", NULL) == -1)
+   err(1, "pledge");
+
/* Init world */
TAILQ_INIT(_queue);
if ((ptb->dummybuf = malloc(ptb->dummybuf_len)) == NULL)
@@ -1215,8 +1235,11 @@ main(int argc, char **argv)
evtimer_add(_progtimer, );
}
client_init(aitop, nconn, udp_sc, aib);
+
+   if (pledge("stdio", NULL) == -1)
+   err(1, "pledge");
}
-   
+
/* libevent main loop*/
event_dispatch();
 

Re: pledge route(8) with '-n' flag

[Sebastian Benoit]

Ricardo Mestre(ser...@helheim.mooo.com) on 2015.11.13 18:00:11 +:
> Hello,
> 
> If '-n' argument is used on route(8) then nflag will be active and dns
> transactions won't be needed, am I correct?

please find out yourself.

at least the pledge call in monitor will fail with -n and your diff,
so some more work is required.

> Index: route.c
> ===
> RCS file: /cvs/src/sbin/route/route.c,v
> retrieving revision 1.179
> diff -u -p -u -r1.179 route.c
> --- route.c   25 Oct 2015 09:37:08 -  1.179
> +++ route.c   13 Nov 2015 15:37:37 -
> @@ -227,7 +227,7 @@ main(int argc, char **argv)
>   }
>   
>   if (nflag) {
> - if (pledge("stdio rpath dns", NULL) == -1)
> + if (pledge("stdio rpath", NULL) == -1)
>   err(1, "pledge");
>   } else {
>   if (pledge("stdio rpath dns", NULL) == -1)
> @@ -330,7 +330,7 @@ flushroutes(int argc, char **argv)
>   }
>  
>   if (nflag) {
> - if (pledge("stdio rpath dns", NULL) == -1)
> + if (pledge("stdio rpath", NULL) == -1)
>   err(1, "pledge");
>   } else {
>   if (pledge("stdio rpath dns", NULL) == -1)
> Index: show.c
> ===
> RCS file: /cvs/src/sbin/route/show.c,v
> retrieving revision 1.102
> diff -u -p -u -r1.102 show.c
> --- show.c23 Oct 2015 15:03:25 -  1.102
> +++ show.c13 Nov 2015 15:37:37 -
> @@ -146,7 +146,7 @@ p_rttables(int af, u_int tableid, int ha
>   }
>  
>   if (nflag) {
> - if (pledge("stdio rpath dns", NULL) == -1)
> + if (pledge("stdio rpath", NULL) == -1)
>   err(1, "pledge");
>   } else {
>   if (pledge("stdio rpath dns", NULL) == -1)
> 
> Best regards,
> Ricardo Mestre
> 

-- 

pledge newsyslog

[Sebastian Benoit]

hi,

this is pledge() in newsyslog.

please check & test...

and is someone using monitormode, please say so ;)

(oh, and oks?)

diff --git usr.bin/newsyslog/newsyslog.c usr.bin/newsyslog/newsyslog.c
index 761da36..acfd871 100644
--- usr.bin/newsyslog/newsyslog.c
+++ usr.bin/newsyslog/newsyslog.c
@@ -191,11 +191,20 @@ main(int argc, char **argv)
struct pidinfo *pidlist, *pl;
int status, listlen;
char **av;
+
+   if (pledge("stdio rpath wpath cpath fattr exec proc", NULL) == -1)
+   err(1,"pledge");

parse_args(argc, argv);
argc -= optind;
argv += optind;
 
+   if (noaction && pledge("stdio rpath", NULL) == -1)
+   err(1,"pledge");
+   else if (!monitormode && pledge("stdio rpath wpath cpath fattr proc",
+   NULL) == -1)
+   err(1,"pledge");
+   
if (needroot && getuid() && geteuid())
errx(1, "You must be root.");
 

Re: Exclude invalid sensors from the sensors MIB

[Sebastian Benoit]

ok

Stuart Henderson(st...@openbsd.org) on 2015.11.17 11:33:50 +:
> On 2015/11/17 11:47, Gerhard Roth wrote:
> > Sensors marked as invalid should be excluded by snmpd(8) from the sensors
> > MIB just as sysctl(8) excludes them from the 'hw.sensors' tree.
> 
> Agreed - any OKs to commit?
> 
> After:
> 
> $ snmptable -v2c -c public 127.0.0.1 sensorTable
> SNMP table: OPENBSD-SENSORS-MIB::sensorTable
> 
>  sensorIndexsensorDescr  sensorType sensorDevice sensorValue 
> sensorUnits sensorStatus
>1"temp0" temperature   "cpu0" "36.00"  
> "degC"  unspecified
>2 "zone temperature" temperature"acpitz0" "27.80"  
> "degC"  unspecified
>3 "zone temperature" temperature"acpitz1" "29.80"  
> "degC"  unspecified
>4"temp0" temperature"sdtemp0" "34.25"  
> "degC"  unspecified
>5"temp0" temperature"sdtemp1" "33.50"  
> "degC"  unspecified
>6"inner" temperature "ugold0" "21.37"  
> "degC"  unspecified
>7  "sd3"   drive  "softraid0""online"  
> ""   ok
> 
> Before:
> 
> $ snmptable -v2c -c public 127.0.0.1 sensorTable
> SNMP table: OPENBSD-SENSORS-MIB::sensorTable
> 
>  sensorIndex  sensorDescr  sensorType sensorDevice sensorValue 
> sensorUnits sensorStatus
>1  "temp0" temperature   "cpu0" "38.00"  
> "degC"  unspecified
>2   "zone temperature" temperature"acpitz0" "27.80"  
> "degC"  unspecified
>3   "zone temperature" temperature"acpitz1" "29.80"  
> "degC"  unspecified
>4"voltage" voltsdc   "acpibat0"  "0.00"  
> "V DC"  unspecified
>5"current voltage" voltsdc   "acpibat0"  "0.00"  
> "V DC"  unspecified
>6   "rate"   power   "acpibat0"  "0.00"
>  "W"  unspecified
>7 "last full capacity"watthour   "acpibat0"  "0.00"
> "Wh"  unspecified
>8   "warning capacity"watthour   "acpibat0"  "0.00"
> "Wh"  unspecified
>9   "low capacity"watthour   "acpibat0"  "0.00"
> "Wh"  unspecified
>   10 "remaining capacity"watthour   "acpibat0"  "0.00"
> "Wh"  unspecified
>   11"design capacity"watthour   "acpibat0"  "0.00"
> "Wh"  unspecified
>   12"battery removed" raw   "acpibat0" "0"
>   ""  unspecified
>   13"voltage" voltsdc   "acpibat1"  "0.00"  
> "V DC"  unspecified
>   14"current voltage" voltsdc   "acpibat1"  "0.00"  
> "V DC"  unspecified
>   15   "rate"   power   "acpibat1"  "0.00"
>  "W"  unspecified
>   16 "last full capacity"watthour   "acpibat1"  "0.00"
> "Wh"  unspecified
>   17   "warning capacity"watthour   "acpibat1"  "0.00"
> "Wh"  unspecified
>   18   "low capacity"watthour   "acpibat1"  "0.00"
> "Wh"  unspecified
>   19 "remaining capacity"watthour   "acpibat1"  "0.00"
> "Wh"  unspecified
>   20"design capacity"watthour   "acpibat1"  "0.00"
> "Wh"  unspecified
>   21"battery removed" raw   "acpibat1" "0"
>   ""  unspecified
>   22"voltage" voltsdc   "acpibat2"  "0.00"  
> "V DC"  unspecified
>   23"current voltage" voltsdc   "acpibat2"  "0.00"  
> "V DC"  unspecified
>   24   "rate"   power   "acpibat2"  "0.00"
>  "W"  unspecified
>   25 "last full capacity"watthour   "acpibat2"  "0.00"
> "Wh"  unspecified
>   26   "warning capacity"watthour   "acpibat2"  "0.00"
> "Wh"  unspecified
>   27   "low capacity"watthour   "acpibat2"  "0.00"
> "Wh"  unspecified
>   28 "remaining capacity"watthour   "acpibat2"  "0.00"
> "Wh"  unspecified
>   29"design capacity"watthour   "acpibat2"  "0.00"
> "Wh"  unspecified
>   30"battery removed" raw   "acpibat2" "0"
>   ""  unspecified
>   31  "temp0" temperature"sdtemp0" "34.50"  
> "degC"  unspecified
>   32  "temp0" temperature"sdtemp1" "33.75"  
> "degC"  unspecified
>   33  "inner" temperature "ugold0" "21.25"  
> "degC"  unspecified
>   34"sd3"   drive  "softraid0""online"
>   ""   ok
> 

-- 

relayd: improving sessions distribution across hosts in hash mode

[Sebastian Benoit]

Hi,

relayd (when running relays) will distribute client sessions over hosts
using various algorithms. Some of them generate a hash from different data
and calculate modulo rlt->rlt_nhosts to find the host the session should go
to. If this host is down, the current algorithm simply selects the next host
that is up.

This puts heavier load on this next host, it would be nicer if the
connections would be distributed to more destinations.

The following diff changes this algorithm: if the chosen host is not
available, the hash value p is shifted to the right and the calculation is
retried until a host that is usable is found or a maximum of retires is
reached (in that case the old method is used).

ok?

diff --git usr.sbin/relayd/relay.c usr.sbin/relayd/relay.c
index f14d7c9..9bb69ba 100644
--- usr.sbin/relayd/relay.c
+++ usr.sbin/relayd/relay.c
@@ -1220,6 +1220,8 @@ relay_from_table(struct rsession *con)
struct relay_table  *rlt = NULL;
struct table*table = NULL;
int  idx = -1;
+   int  cnt = 0;
+   int  maxtries;
u_int64_tp = 0;
 
/* the table is already selected */
@@ -1275,18 +1277,38 @@ relay_from_table(struct rsession *con)
/* NOTREACHED */
}
if (idx == -1) {
+   /* handle all hashing algorithms */
p = SipHash24_End(>se_siphashctx);
 
/* Reset hash context */
SipHash24_Init(>se_siphashctx,
>rl_conf.hashkey.siphashkey);
 
-   if ((idx = p % rlt->rlt_nhosts) >= RELAY_MAXHOSTS)
-   return (-1);
+   maxtries = (rlt->rlt_nhosts < RELAY_MAX_HASH_RETRIES ?
+   rlt->rlt_nhosts : RELAY_MAX_HASH_RETRIES);
+   while (cnt < maxtries) {
+   if ((idx = p % rlt->rlt_nhosts) >= RELAY_MAXHOSTS)
+   return (-1);
+   
+   host = rlt->rlt_host[idx];
+
+   DPRINTF("%s: session %d: table %s host %s, "
+   "p 0x%016llx, idx %d, cnt %d, max %d",
+   __func__, con->se_id, table->conf.name,
+   host->conf.name, p, idx, cnt, maxtries);
+
+   if (!table->conf.check || host->up == HOST_UP)
+   goto found;
+   p = p >> 1;
+   cnt++;
+   }
+   } else {
+   /* handle all non-hashing algorithms */
+   host = rlt->rlt_host[idx];
+   DPRINTF("%s: session %d: table %s host %s, p 0x%016llx, idx %d",
+   __func__, con->se_id, table->conf.name, host->conf.name, p, 
idx);
}
-   host = rlt->rlt_host[idx];
-   DPRINTF("%s: session %d: table %s host %s, p 0x%016llx, idx %d",
-   __func__, con->se_id, table->conf.name, host->conf.name, p, idx);
+
while (host != NULL) {
DPRINTF("%s: session %d: host %s", __func__,
con->se_id, host->conf.name);
diff --git usr.sbin/relayd/relayd.h usr.sbin/relayd/relayd.h
index 3c7dc89..2e0fd58 100644
--- usr.sbin/relayd/relayd.h
+++ usr.sbin/relayd/relayd.h
@@ -76,6 +76,7 @@
 #define RELAY_BACKLOG  10
 #define RELAY_MAXLOOKUPLEVELS  5
 #define RELAY_OUTOF_FD_RETRIES 5
+#define RELAY_MAX_HASH_RETRIES 5
 
 #define CONFIG_RELOAD  0x00
 #define CONFIG_TABLES  0x01

Re: pair(4) + pf(4): reset all state on "reinjected" packets

[Sebastian Benoit]

Reyk Floeter(r...@openbsd.org) on 2015.10.30 19:25:28 +0100:
> On Fri, Oct 30, 2015 at 06:16:53PM +0100, Sebastian Benoit wrote:
> > 
> > i think it should be documented ;)
> > 
> > otherwise ok
> > 
> 
> Ooops, good point, I missed the manpage.
> 
> It looks about right, but maybe it is better to have it less pf-
> specific (also regarding bluhm's update)?
> 
> Otherwise OK

thanks, just saw your mail.


> 
> Reyk
> 
> > Index: mbuf.9
> > ===
> > RCS file: /cvs/src/share/man/man9/mbuf.9,v
> > retrieving revision 1.91
> > diff -u -p -u -r1.91 mbuf.9
> > --- mbuf.9  8 Oct 2015 14:09:34 -   1.91
> > +++ mbuf.9  30 Oct 2015 17:15:40 -
> > @@ -44,6 +44,8 @@
> >  .Fn MGET "struct mbuf *m" "int how" "int type"
> >  .Ft struct mbuf *
> >  .Fn m_getclr "int how" "int type"
> > +.Ft void
> > +.Fn m_resethdr struct mbuf *
> >  .Ft struct mbuf *
> >  .Fn m_gethdr "int how" "int type"
> >  .Fn MGETHDR "struct mbuf *m" "int how" "int type"
> > @@ -445,6 +447,11 @@ See
> >  .Fn m_get
> >  for a description of
> >  .Fa how .
> > +.It Fn m_resethdr "struct mbuf *"
> > +Deletes all
> > +.Xr pf 4
> > +data and all tags attached to packet
> > +.Fa mbuf .
> >  .It Fn m_gethdr "int how" "int type"
> >  Return a pointer to an mbuf of the type specified after initializing
> >  it to contain a packet header.
> > 
> > 
> > Reyk Floeter(r...@openbsd.org) on 2015.10.30 14:04:52 +0100:
> > > On Fri, Oct 30, 2015 at 01:40:19PM +0100, Alexander Bluhm wrote:
> > > > On Fri, Oct 30, 2015 at 12:56:34PM +0100, Reyk Floeter wrote:
> > > > > --- sys/sys/mbuf.h22 Oct 2015 05:26:06 -  1.198
> > > > > +++ sys/sys/mbuf.h30 Oct 2015 11:30:33 -
> > > > > @@ -410,6 +410,7 @@ structmbuf *m_get(int, int);
> > > > >  struct   mbuf *m_getclr(int, int);
> > > > >  struct   mbuf *m_gethdr(int, int);
> > > > >  struct   mbuf *m_inithdr(struct mbuf *);
> > > > > +void m_resethdr(struct mbuf *);
> > > > >  intm_defrag(struct mbuf *, int);
> > > > 
> > > > The m_resethdr should have the same indent as m_defrag.
> > > > 
> > > 
> > > Really?  The indent of m_defrag() is wrong - many spaces and not like
> > > the other int functions further down - but I didn't dare to touch it.
> > > It sticks in the eye, so I'll fix m_defrag indent as well.
> > > 
> > > > > --- sys/net/if_pair.c 25 Oct 2015 12:59:57 -  1.4
> > > > > +++ sys/net/if_pair.c 30 Oct 2015 11:30:33 -
> > > > > @@ -30,6 +30,11 @@
> > > > >  #include 
> > > > >  #include 
> > > > >  
> > > > > +#include "pf.h"
> > > > > +#if NPF > 0
> > > > > +#include 
> > > > > +#endif
> > > > 
> > > > I think you don't need that anymore.
> > > > 
> > > > > @@ -182,9 +187,13 @@ pairstart(struct ifnet *ifp)
> > > > >  #endif /* NBPFILTER > 0 */
> > > > >  
> > > > >   ifp->if_opackets++;
> > > > > - if (pairedifp != NULL)
> > > > > + if (pairedifp != NULL) {
> > > > > +#if NPF > 0
> > > > > + if (m->m_flags & M_PKTHDR)
> > > > > + m_resethdr(m);
> > > > > +#endif
> > > > 
> > > > Calling m_tag_delete_chain() is not pf specific, so I would do it
> > > > without the #if NPF.
> > > > 
> > > > Otherwise OK bluhm@
> > > > 
> > > > Socket splicing somove() does the same thing.  I will change it to
> > > > use m_resethdr() after that got commited.
> > > 
> > > Cool.
> > > 
> > > I'm going to commit the attached diff for now.
> > > 
> > > Reyk
> > > 
> > > Index: sys/kern/uipc_mbuf.c
> > > ===
> > > RCS file: /cvs/src/sys/kern/uipc_mbuf.c,v
> > > retrieving revision 1.208
> > > diff -u -p -u -p -r1.208 uipc_mbuf.c
> > > --- sys/kern/uipc_mbuf.c  22 Oct 2015 05:26:06 -  1.20

Re: pair(4) + bridge(4): use stp to prevent bridge loops

[Sebastian Benoit]

i like this.
ok

Reyk Floeter(r...@openbsd.org) on 2015.10.30 11:34:39 +0100:
> Hi,
> 
> as documented below, pairs in bridges can lead to a loop.
> 
> I looked at "fixing" it but came to the conclusion a) there is no
> satisfying way with mbuf flags/tags to prevent the loop, b) it would
> limit the use cases of pair(4) for network testing in many ways, c)
> the bridge loop causes heavy load but does not lock the system (our
> stack is already preventing this), and d) it can be documented -
> so it is a feature, and not a bug ;)
> 
> Thoughts? OK?
> 
> Reyk
> 
> Index: share/man/man4/pair.4
> ===
> RCS file: /cvs/src/share/man/man4/pair.4,v
> retrieving revision 1.3
> diff -u -p -u -p -r1.3 pair.4
> --- share/man/man4/pair.4 24 Oct 2015 15:46:10 -  1.3
> +++ share/man/man4/pair.4 30 Oct 2015 10:09:34 -
> @@ -47,6 +47,25 @@ Set up a pair of interfaces where each o
>  # ifconfig pair1 patch pair2
>  # route -T 1 exec ping 10.1.1.2
>  .Ed
> +.Pp
> +When adding multiple
> +.Nm
> +to multiple
> +.Xr bridge 4
> +interfaces, it is possible to create a loop;
> +the system load will go up while it is busy sending packets from one
> +bridge to another and back.
> +By design, the driver does not prevent such loops by itself, but it is
> +possible to use the Spanning Tree Protocol (STP) to detect and remove
> +loops in the virtual network topology:
> +.Bd -literal -offset indent
> +# ifconfig pair0 up
> +# ifconfig pair1 rdomain 1 patch pair0 up
> +# ifconfig pair2 up
> +# ifconfig pair3 rdomain 1 patch pair2 up
> +# ifconfig bridge0 add pair0 add pair2 stp pair0 stp pair2 up
> +# ifconfig bridge1 add pair1 add pair3 stp pair1 stp pair3 up
> +.Ed
>  .Sh SEE ALSO
>  .Xr bridge 4 ,
>  .Xr inet 4 ,
> 

-- 

Re: pair(4) + pf(4): reset all state on "reinjected" packets

[Sebastian Benoit]

i think it should be documented ;)

otherwise ok

Index: mbuf.9
===
RCS file: /cvs/src/share/man/man9/mbuf.9,v
retrieving revision 1.91
diff -u -p -u -r1.91 mbuf.9
--- mbuf.9  8 Oct 2015 14:09:34 -   1.91
+++ mbuf.9  30 Oct 2015 17:15:40 -
@@ -44,6 +44,8 @@
 .Fn MGET "struct mbuf *m" "int how" "int type"
 .Ft struct mbuf *
 .Fn m_getclr "int how" "int type"
+.Ft void
+.Fn m_resethdr struct mbuf *
 .Ft struct mbuf *
 .Fn m_gethdr "int how" "int type"
 .Fn MGETHDR "struct mbuf *m" "int how" "int type"
@@ -445,6 +447,11 @@ See
 .Fn m_get
 for a description of
 .Fa how .
+.It Fn m_resethdr "struct mbuf *"
+Deletes all
+.Xr pf 4
+data and all tags attached to packet
+.Fa mbuf .
 .It Fn m_gethdr "int how" "int type"
 Return a pointer to an mbuf of the type specified after initializing
 it to contain a packet header.


Reyk Floeter(r...@openbsd.org) on 2015.10.30 14:04:52 +0100:
> On Fri, Oct 30, 2015 at 01:40:19PM +0100, Alexander Bluhm wrote:
> > On Fri, Oct 30, 2015 at 12:56:34PM +0100, Reyk Floeter wrote:
> > > --- sys/sys/mbuf.h22 Oct 2015 05:26:06 -  1.198
> > > +++ sys/sys/mbuf.h30 Oct 2015 11:30:33 -
> > > @@ -410,6 +410,7 @@ structmbuf *m_get(int, int);
> > >  struct   mbuf *m_getclr(int, int);
> > >  struct   mbuf *m_gethdr(int, int);
> > >  struct   mbuf *m_inithdr(struct mbuf *);
> > > +void m_resethdr(struct mbuf *);
> > >  intm_defrag(struct mbuf *, int);
> > 
> > The m_resethdr should have the same indent as m_defrag.
> > 
> 
> Really?  The indent of m_defrag() is wrong - many spaces and not like
> the other int functions further down - but I didn't dare to touch it.
> It sticks in the eye, so I'll fix m_defrag indent as well.
> 
> > > --- sys/net/if_pair.c 25 Oct 2015 12:59:57 -  1.4
> > > +++ sys/net/if_pair.c 30 Oct 2015 11:30:33 -
> > > @@ -30,6 +30,11 @@
> > >  #include 
> > >  #include 
> > >  
> > > +#include "pf.h"
> > > +#if NPF > 0
> > > +#include 
> > > +#endif
> > 
> > I think you don't need that anymore.
> > 
> > > @@ -182,9 +187,13 @@ pairstart(struct ifnet *ifp)
> > >  #endif /* NBPFILTER > 0 */
> > >  
> > >   ifp->if_opackets++;
> > > - if (pairedifp != NULL)
> > > + if (pairedifp != NULL) {
> > > +#if NPF > 0
> > > + if (m->m_flags & M_PKTHDR)
> > > + m_resethdr(m);
> > > +#endif
> > 
> > Calling m_tag_delete_chain() is not pf specific, so I would do it
> > without the #if NPF.
> > 
> > Otherwise OK bluhm@
> > 
> > Socket splicing somove() does the same thing.  I will change it to
> > use m_resethdr() after that got commited.
> 
> Cool.
> 
> I'm going to commit the attached diff for now.
> 
> Reyk
> 
> Index: sys/kern/uipc_mbuf.c
> ===
> RCS file: /cvs/src/sys/kern/uipc_mbuf.c,v
> retrieving revision 1.208
> diff -u -p -u -p -r1.208 uipc_mbuf.c
> --- sys/kern/uipc_mbuf.c  22 Oct 2015 05:26:06 -  1.208
> +++ sys/kern/uipc_mbuf.c  30 Oct 2015 12:45:50 -
> @@ -250,6 +250,18 @@ m_inithdr(struct mbuf *m)
>   return (m);
>  }
>  
> +void
> +m_resethdr(struct mbuf *m)
> +{
> + /* like the previous, but keep any associated data and mbufs */
> + m->m_flags = M_PKTHDR;
> + memset(>m_pkthdr.pf, 0, sizeof(m->m_pkthdr.pf));
> + m->m_pkthdr.pf.prio = IFQ_DEFPRIO;
> +
> + /* also delete all mbuf tags to reset the state */
> + m_tag_delete_chain(m);
> +}
> +
>  struct mbuf *
>  m_getclr(int nowait, int type)
>  {
> Index: sys/sys/mbuf.h
> ===
> RCS file: /cvs/src/sys/sys/mbuf.h,v
> retrieving revision 1.198
> diff -u -p -u -p -r1.198 mbuf.h
> --- sys/sys/mbuf.h22 Oct 2015 05:26:06 -  1.198
> +++ sys/sys/mbuf.h30 Oct 2015 12:45:50 -
> @@ -410,7 +410,8 @@ structmbuf *m_get(int, int);
>  struct   mbuf *m_getclr(int, int);
>  struct   mbuf *m_gethdr(int, int);
>  struct   mbuf *m_inithdr(struct mbuf *);
> -intm_defrag(struct mbuf *, int);
> +void m_resethdr(struct mbuf *);
> +int  m_defrag(struct mbuf *, int);
>  struct   mbuf *m_prepend(struct mbuf *, int, int);
>  struct   mbuf *m_pulldown(struct mbuf *, int, int, int *);
>  struct   mbuf *m_pullup(struct mbuf *, int);
> Index: sys/net/if_pair.c
> ===
> RCS file: /cvs/src/sys/net/if_pair.c,v
> retrieving revision 1.4
> diff -u -p -u -p -r1.4 if_pair.c
> --- sys/net/if_pair.c 25 Oct 2015 12:59:57 -  1.4
> +++ sys/net/if_pair.c 30 Oct 2015 12:45:50 -
> @@ -182,9 +182,11 @@ pairstart(struct ifnet *ifp)
>  #endif /* NBPFILTER > 0 */
>  
>   ifp->if_opackets++;
> - if (pairedifp != NULL)
> + if (pairedifp != NULL) {
> + if (m->m_flags & M_PKTHDR)
> + 

Re: make iked not static

[Sebastian Benoit]

Christian Weisgerber(na...@mips.inka.de) on 2015.10.20 20:46:12 +:
> On 2015-10-20, Reyk Floeter  wrote:
> 
> > For historical reasons, isakmpd and iked are compiled static:
> > people used NFS over ipsec.
> >
> > Is anyone still using this?  Is it more than one person?
> >
> > Otherwise I'd suggest to make iked dynamic.
> 
> Already, iked is started after /usr has been mounted, so why the
> static requirement?
> 
> > --- etc/rc  18 Oct 2015 21:33:18 -  1.467
> > +++ etc/rc  20 Oct 2015 18:03:58 -
> > @@ -353,7 +353,7 @@ make_keys
> >  
> >  echo -n 'starting early daemons:'
> >  start_daemon syslogd ldattach pflogd nsd unbound ntpd
> > -start_daemon iscsid isakmpd iked sasyncd ldapd npppd
> > +start_daemon iscsid isakmpd sasyncd ldapd npppd
> >  echo '.'
> 
> Most of these are dynamically linked.
> 
> You can make iked dynamic without moving it in the startup sequence.

In a lot of cases it will need the routing daemons to work anyway, so why
start it so much earlier?

Re: doas closefrom

[Sebastian Benoit]

ok, but in other places we have closefrom(STDERR_FILENO + 1)

Ted Unangst(t...@tedunangst.com) on 2015.09.17 12:11:26 -0400:
> doas doesn't need any other open files and should probably shut them all.
> 
> 
> Index: doas.c
> ===
> RCS file: /cvs/src/usr.bin/doas/doas.c,v
> retrieving revision 1.41
> diff -u -p -r1.41 doas.c
> --- doas.c3 Sep 2015 20:05:58 -   1.41
> +++ doas.c17 Sep 2015 16:09:52 -
> @@ -323,6 +323,8 @@ main(int argc, char **argv, char **envp)
>   char cwdpath[PATH_MAX];
>   const char *cwd;
>  
> + closefrom(3);
> +
>   uid = getuid();
>  
>   while ((ch = getopt(argc, argv, "C:nsu:")) != -1) {
> 

-- 

relayd maintainance diff for OpenBSD 5.7

[Sebastian Benoit]

OpenBSD 5.7 errata:
http://www.openbsd.org/errata57.html#015_relayd

015: RELIABILITY FIX: September 28, 2015   All architectures
Various problems were identified in relayd and merged back from current to
5.7 in this maintanance update.

This patch is for 5.7 only, it fixes reliability problems that where
identified during the OpenBSD 5.8 release cycle.

Re: doas closefrom

[Sebastian Benoit]

Ted Unangst(t...@tedunangst.com) on 2015.09.17 21:12:28 -0400:
> Sebastian Benoit wrote:
> > ok, but in other places we have closefrom(STDERR_FILENO + 1)
> 
> is that really more clear? it only makes sense if you know stderr is 2.

sure, but writing closefrom(3) requires the same or equivalent knowledge, no?
 
> if you sometimes forget which is 1 and which is 2, then the macro only makes
> it more confusing because now you have to decide what comes after stderr. is
> it stdin? or stdout?

if you sometimes forget whats 1 and 2 you have probably more problems than
that ;-)

anyway, i only wanted to point this out, in case someone thinks this is some
matter of style. i'm fine with the 3.

Re: [patch] tame.2 documentation about systrace.4

[Sebastian Benoit]

Sebastien Marie(sema...@openbsd.org) on 2015.09.20 14:27:01 +0200:
> Hi,
> 
> Mentions that using systrace(4) isn't possible when a program has called
> tame(2).
> 
> Comments ? OK ?
> -- 
> Sebastien Marie
> 
> Index: lib/libc/sys/tame.2
> ===
> RCS file: /cvs/src/lib/libc/sys/tame.2,v
> retrieving revision 1.27
> diff -u -p -r1.27 tame.2
> --- lib/libc/sys/tame.2   11 Sep 2015 09:01:16 -  1.27
> +++ lib/libc/sys/tame.2   20 Sep 2015 12:26:47 -
> @@ -431,3 +431,7 @@ The
>  .Fn tame
>  system call appeared in
>  .Ox 5.8 .
> +.Sh CAVEATS
> +The use of
> +.Xr systrace 4
> +in a tamed program is disable.

there is a "d" missing at the end.

And "In a tamed program, systrace(4) is disabled" sounds better to me, but
jmc might have an opinion on that.

This could also be mentioned in the systrace(4) manpage, after all, if your
wondering why systrace doesnt work, you won't know that the program is
tamed and thus won't look in tame(2)?

Re: Merge rt_use counters

[Sebastian Benoit]

Claudio Jeker(cje...@diehard.n-r-g.com) on 2015.09.22 16:01:34 +0200:
> On Tue, Sep 22, 2015 at 03:14:18PM +0200, Martin Pieuchot wrote:
> > Instead of incrementing the rt_use counter when a rtalloc(9) call
> > succeeds, let's do it inside ralloc(9).
> > 
> > The route(8) regress tests will need to be updated because all the
> > paths calling rtalloc(9) do not increment rt_use.
> > 
> > This change gives us a better understanding of which routes are queried
> > and might need a cache.
> > 
> > It will also help me with upcoming counter handling for MP.
> > 
> > ok?

yes ok
 
> It kind of changes the meaning of the use counter but I think that is
> fair. In the end I think this is only used for debugging. As mentioned
> earlier I have no problems to remove this counter since I don't see a huge
> benefit having it.

regarding removing it: i havent needed it often and can do without.


> -- 
> :wq Claudio
>  
> > Index: net/pf.c
> > ===
> > RCS file: /cvs/src/sys/net/pf.c,v
> > retrieving revision 1.944
> > diff -u -p -r1.944 pf.c
> > --- net/pf.c13 Sep 2015 17:53:44 -  1.944
> > +++ net/pf.c22 Sep 2015 13:02:42 -
> > @@ -5520,7 +5520,6 @@ pf_route(struct mbuf **m, struct pf_rule
> > }
> >  
> > ifp = rt->rt_ifp;
> > -   rt->rt_use++;
> >  
> > if (rt->rt_flags & RTF_GATEWAY)
> > dst = satosin(rt->rt_gateway);
> > Index: net/route.c
> > ===
> > RCS file: /cvs/src/sys/net/route.c,v
> > retrieving revision 1.241
> > diff -u -p -r1.241 route.c
> > --- net/route.c 22 Sep 2015 10:05:00 -  1.241
> > +++ net/route.c 22 Sep 2015 13:04:07 -
> > @@ -350,12 +350,15 @@ rtalloc(struct sockaddr *dst, int flags,
> > rt0 = rt;
> > error = rtrequest1(RTM_RESOLVE, , RTP_DEFAULT,
> > , tableid);
> > -   if (error)
> > +   if (error) {
> > +   rt0->rt_use++;
> > goto miss;
> > +   }
> > /* Inform listeners of the new route */
> > rt_sendmsg(rt, RTM_ADD, tableid);
> > rtfree(rt0);
> > }
> > +   rt->rt_use++;
> > } else {
> > rtstat.rts_unreach++;
> >  miss:
> > Index: netinet/ip_icmp.c
> > ===
> > RCS file: /cvs/src/sys/netinet/ip_icmp.c,v
> > retrieving revision 1.140
> > diff -u -p -r1.140 ip_icmp.c
> > --- netinet/ip_icmp.c   11 Sep 2015 15:12:29 -  1.140
> > +++ netinet/ip_icmp.c   22 Sep 2015 13:02:36 -
> > @@ -758,7 +758,6 @@ icmp_reflect(struct mbuf *m, struct mbuf
> > }
> >  
> > ia = ifatoia(rt->rt_ifa);
> > -   rt->rt_use++;
> > rtfree(rt);
> > }
> >  
> > Index: netinet/ip_output.c
> > ===
> > RCS file: /cvs/src/sys/netinet/ip_output.c,v
> > retrieving revision 1.298
> > diff -u -p -r1.298 ip_output.c
> > --- netinet/ip_output.c 13 Sep 2015 17:53:44 -  1.298
> > +++ netinet/ip_output.c 22 Sep 2015 13:02:33 -
> > @@ -207,7 +207,6 @@ reroute:
> > ifp = if_ref(ro->ro_rt->rt_ifp);
> > if ((mtu = ro->ro_rt->rt_rmx.rmx_mtu) == 0)
> > mtu = ifp->if_mtu;
> > -   ro->ro_rt->rt_use++;
> >  
> > if (ro->ro_rt->rt_flags & RTF_GATEWAY)
> > dst = satosin(ro->ro_rt->rt_gateway);
> > Index: netinet6/ip6_output.c
> > ===
> > RCS file: /cvs/src/sys/netinet6/ip6_output.c,v
> > retrieving revision 1.188
> > diff -u -p -r1.188 ip6_output.c
> > --- netinet6/ip6_output.c   13 Sep 2015 13:57:07 -  1.188
> > +++ netinet6/ip6_output.c   22 Sep 2015 13:02:29 -
> > @@ -558,12 +558,6 @@ reroute:
> > *dst = dstsock;
> > }
> >  
> > -   /*
> > -* then rt (for unicast) and ifp must be non-NULL valid values.
> > -*/
> > -   if (rt)
> > -   rt->rt_use++;
> > -
> > if (rt && !IN6_IS_ADDR_MULTICAST(>ip6_dst)) {
> > if (opt && opt->ip6po_nextroute.ro_rt) {
> > /*
> > Index: netmpls/mpls_input.c
> > ===
> > RCS file: /cvs/src/sys/netmpls/mpls_input.c,v
> > retrieving revision 1.49
> > diff -u -p -r1.49 mpls_input.c
> > --- netmpls/mpls_input.c13 Sep 2015 17:53:44 -  1.49
> > +++ netmpls/mpls_input.c22 Sep 2015 13:02:23 -
> > @@ -182,7 +182,6 @@ do_v6:
> > goto done;
> > }
> >  
> > -   rt->rt_use++;
> > rt_mpls = (struct rt_mpls *)rt->rt_llinfo;
> >  
> > if (rt_mpls == NULL 

Re: Purge route entries when an address is removed

[Sebastian Benoit]

Martin Pieuchot(m...@openbsd.org) on 2015.09.13 16:08:50 +0200:
> On 13/09/15(Sun) 15:51, Alexander Bluhm wrote:
> > On Sun, Sep 13, 2015 at 11:15:50AM +0200, Martin Pieuchot wrote:
> > > This makes the kernel simpler as it no longer try to find a new ifa
> > > when a route with a stale address is being used.
> > 
> > This makes the code simpler, which is good.
> > 
> > I am still not convinced that we want to loose the feature that the
> > routes jump to another interface address.  When we have multiple
> > suiteable addresses and one gets deleted, the system can use another
> > one.
> 
> This is the price to pay for making the code simpler.  I strongly
> believe this "feature" is a side effect of history that should not
> have been added in the first place.
> 
> However I'd like to fix potential issues with this diff before committing
> it, so tests are welcome :)

Hi,

i found the time to play with your diff.

On a router (where you probably wouldn't to this operationaly) you get tons
of "route xxx vanished before delete" from bgpd and ospfd, but they continue
to work, apparently as intended.

I havent found a box with pppoe to test. it would be nice if someone could
test that.

as far as it goes, ok from me.

/Benno
 
> > The patch itself looks correct.  Just one question:
> > 
> > > @@ -850,22 +850,10 @@ rtrequest1(int req, struct rt_addrinfo *
> > >   return (EINVAL);
> > >   if ((rt->rt_flags & RTF_CLONING) == 0)
> > >   return (EINVAL);
> > > - if (rt->rt_ifa->ifa_ifp) {
> > > - info->rti_ifa = rt->rt_ifa;
> > > - } else {
> > > - /*
> > > -  * The address of the cloning route is not longer
> > > -  * configured on an interface, but its descriptor
> > > -  * is still there because of reference counting.
> > > -  *
> > > -  * Try to find a similar active address and use
> > > -  * it for the cloned route.  The cloning route
> > > -  * will get the new address and interface later.
> > > -  */
> > > - info->rti_ifa = NULL;
> > > - info->rti_info[RTAX_IFA] = rt->rt_ifa->ifa_addr;
> > > - }
> > > + if (rt->rt_ifa->ifa_ifp == NULL)
> > > + return (EAGAIN);
> > 
> > Why return EAGAIN here?  Should it be EINVAL like in the other
> > cases?  Can this happen at all?
> 
> This should never happen but I'd like to take a safe approach and be
> able to differentiate the error code if this still happens.  I'd
> happily turn this into a KASSERT() but not right now.

+1

Re: ping(8) / ping6(8) source address

[Sebastian Benoit]

ok

Florian Obser(flor...@openbsd.org) on 2015.11.29 14:07:24 +:
> Inspired by the traceroute / traceroute6 merge.
> At least reduces the diff in the option parser :)
> OK?
> 
> diff --git ping/ping.c ping/ping.c
> index f5ccaca..4944f77 100644
> --- ping/ping.c
> +++ ping/ping.c
> @@ -110,7 +110,7 @@ int options;
>  #define  F_SO_DEBUG  0x0040
>  /*   0x0080 */
>  #define  F_VERBOSE   0x0100
> -#define  F_SADDR 0x0200
> +/*   0x0200 */
>  #define  F_HDRINCL   0x0400
>  #define  F_TTL   0x0800
>  #define  F_AUD_RECV  0x2000
> @@ -132,7 +132,6 @@ int mx_dup_ck = MAX_DUP_CHK;
>  char rcvd_tbl[MAX_DUP_CHK / 8];
>  
>  struct sockaddr_in whereto;  /* who to ping */
> -struct sockaddr_in whence;   /* Which interface we come from */
>  unsigned int datalen = DEFDATALEN;
>  int s;   /* socket file descriptor */
>  u_char outpackhdr[IP_MAXPACKET]; /* Max packet size = 65535 */
> @@ -186,11 +185,13 @@ int
>  main(int argc, char *argv[])
>  {
>   struct hostent *hp;
> + struct addrinfo hints, *res;
> + struct sockaddr_in  from4;
>   struct sockaddr_in *to;
> - struct in_addr saddr;
>   int ch, i, optval = 1, packlen, preload, maxsize, df = 0, tos = 0;
> + int error;
>   u_char *datap, *packet, ttl = MAXTTL, loop = 1;
> - char *target, hnamebuf[HOST_NAME_MAX+1];
> + char *target, hnamebuf[HOST_NAME_MAX+1], *source = NULL;
>   char rspace[3 + 4 * NROUTES + 1];   /* record route space */
>   socklen_t maxsizelen;
>   const char *errstr;
> @@ -238,13 +239,7 @@ main(int argc, char *argv[])
>   break;
>   case 'I':
>   case 'S':   /* deprecated */
> - if (inet_aton(optarg, ) == 0) {
> - if ((hp = gethostbyname(optarg)) == NULL)
> - errx(1, "bad interface address: %s",
> - optarg);
> - memcpy(, hp->h_addr, sizeof(saddr));
> - }
> - options |= F_SADDR;
> + source = optarg;
>   break;
>   case 'i':   /* wait between sending packets */
>   interval = strtod(optarg, NULL);
> @@ -360,16 +355,27 @@ main(int argc, char *argv[])
>   hostname = hnamebuf;
>   }
>  
> - if (options & F_SADDR) {
> + if (source) {
>   if (IN_MULTICAST(ntohl(to->sin_addr.s_addr)))
>   moptions |= MULTICAST_IF;
>   else {
> - memset(, 0, sizeof(whence));
> - whence.sin_len = sizeof(whence);
> - whence.sin_family = AF_INET;
> - memcpy(_addr.s_addr, , sizeof(saddr));
> - if (bind(s, (struct sockaddr *),
> - sizeof(whence)) < 0)
> + memset(, 0, sizeof(from4));
> + from4.sin_family = AF_INET;
> + if (inet_aton(source, _addr) == 0) {
> + memset(, 0, sizeof(hints));
> + hints.ai_family = AF_INET;
> + hints.ai_socktype = SOCK_DGRAM; /*dummy*/
> + if ((error = getaddrinfo(source, NULL, ,
> + )))
> + errx(1, "%s: %s", source,
> + gai_strerror(error));
> + if (res->ai_addrlen != sizeof(from4))
> + errx(1, "size of sockaddr mismatch");
> + memcpy(, res->ai_addr, res->ai_addrlen);
> + freeaddrinfo(res);
> + }
> + if (bind(s, (struct sockaddr *), sizeof(from4))
> + < 0)
>   err(1, "bind");
>   }
>   }
> @@ -426,8 +432,8 @@ main(int argc, char *argv[])
>   ip->ip_off = htons(df ? IP_DF : 0);
>   ip->ip_ttl = ttl;
>   ip->ip_p = IPPROTO_ICMP;
> - if (options & F_SADDR)
> - ip->ip_src = saddr;
> + if (source)
> + ip->ip_src = from4.sin_addr;
>   else
>   ip->ip_src.s_addr = INADDR_ANY;
>   ip->ip_dst = to->sin_addr;
> @@ -457,8 +463,8 @@ main(int argc, char *argv[])
>   sizeof(ttl)) < 0)
>   err(1, "setsockopt IP_MULTICAST_TTL");
>   if ((moptions & MULTICAST_IF) &&
> - setsockopt(s, IPPROTO_IP, IP_MULTICAST_IF, ,
> - sizeof(saddr)) < 0)
> + setsockopt(s, IPPROTO_IP, IP_MULTICAST_IF, _addr,
> + sizeof(from4.sin_addr)) < 0)
>   err(1, "setsockopt IP_MULTICAST_IF");
>  
>   

Re: relayd patch - delayed failover

[Sebastian Benoit]

Brian S. Vangsgaard(b...@avalanic.dk) on 2015.12.04 09:04:19 +0100:
> Hi Sebastian
> 
> You commited the wrong patch.
> 
> Please see http://marc.info/?l=openbsd-tech=144378086813524=2
> 
> The patch below, results in a relayd panic if more than one host is
> available in the group.

i believe i committed the correct one, i just replied to the wrong mail here
on the list. Here is what i put in:

http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.sbin/relayd/pfe.c.diff?r1=1.82=1.83=date

/Benno

> Sebastian Benoit skrev den 2015-12-03 17:43:
> >thanks, commited
> >
> >Brian S. Vangsgaard(b...@avalanic.dk) on 2015.10.01 13:27:12 +0200:
> >>Hi,
> >>
> >>Problem:
> >>If a client have a state entry in the relayd anchor, and the target
> >>server goes down, the client will be unable to "failover" for 10 sec +
> >>(10 sec - elapsed time since last SLA check).
> >>
> >>There are two issues here, this patch only fix the problem about 
> >>delayed
> >>(10 seconds) failover.
> >>
> >>When the host fails the SLA check, it will be marked as being down.
> >>However it will not be removed from the achor before the next SLA 
> >>check.
> >>
> >>Reproduce:
> >>Start relayd with -dvvv, let it run for 10-20 seconds, then make a 
> >>host
> >>fail its SLA check. Relayd will mark the host as being down when it
> >>reach next SLA check, but the sync_table() will not be called until 10
> >>sec. later (at the next SLA check).
> >>
> >>Solution:
> >>The logic is already in the code, but right now it only handle the
> >>statistics and set the host as being down.
> >>
> >>Call sync_table() when a host goes from UP to DOWN.
> >>
> >>
> >>Index: pfe.c
> >>===
> >>RCS file: /cvs/src/usr.sbin/relayd/pfe.c,v
> >>retrieving revision 1.79.2.1
> >>diff -u -p -u -p -r1.79.2.1 pfe.c
> >>--- pfe.c   20 Sep 2015 11:20:16 -  1.79.2.1
> >>+++ pfe.c   1 Oct 2015 10:48:59 -
> >>@@ -152,6 +152,7 @@ pfe_dispatch_hce(int fd, struct privsep_
> >>table->conf.flags |= F_CHANGED;
> >>host->flags |= F_DEL;
> >>host->flags &= ~(F_ADD);
> >>+   pfe_sync();
> >>}
> >>
> >>host->up = st.up;
> >>
> >>
> >>If you need more details or want to fix the scheduler issue, please
> >>contact me :)
> >>
> >>
> >>--
> >>bsv
> >>
> 

-- 

Re: relayd patch - delayed failover

[Sebastian Benoit]

thanks, commited

Brian S. Vangsgaard(b...@avalanic.dk) on 2015.10.01 13:27:12 +0200:
> Hi,
> 
> Problem:
> If a client have a state entry in the relayd anchor, and the target 
> server goes down, the client will be unable to "failover" for 10 sec + 
> (10 sec - elapsed time since last SLA check).
> 
> There are two issues here, this patch only fix the problem about delayed 
> (10 seconds) failover.
> 
> When the host fails the SLA check, it will be marked as being down. 
> However it will not be removed from the achor before the next SLA check.
> 
> Reproduce:
> Start relayd with -dvvv, let it run for 10-20 seconds, then make a host 
> fail its SLA check. Relayd will mark the host as being down when it 
> reach next SLA check, but the sync_table() will not be called until 10 
> sec. later (at the next SLA check).
> 
> Solution:
> The logic is already in the code, but right now it only handle the 
> statistics and set the host as being down.
> 
> Call sync_table() when a host goes from UP to DOWN.
> 
> 
> Index: pfe.c
> ===
> RCS file: /cvs/src/usr.sbin/relayd/pfe.c,v
> retrieving revision 1.79.2.1
> diff -u -p -u -p -r1.79.2.1 pfe.c
> --- pfe.c   20 Sep 2015 11:20:16 -  1.79.2.1
> +++ pfe.c   1 Oct 2015 10:48:59 -
> @@ -152,6 +152,7 @@ pfe_dispatch_hce(int fd, struct privsep_
> table->conf.flags |= F_CHANGED;
> host->flags |= F_DEL;
> host->flags &= ~(F_ADD);
> +   pfe_sync();
> }
> 
> host->up = st.up;
> 
> 
> If you need more details or want to fix the scheduler issue, please 
> contact me :)
> 
> 
> --
> bsv
> 

-- 

Re: fuser(1): Fix pledge when `u' flag is used

[Sebastian Benoit]

Michael Reed(m.r...@mykolab.com) on 2016.01.01 22:29:08 -0500:
> Hi,
> 
> `fuser -u -c /' doesn't seem to work for me:
> 
> fuser(28663): syscall 33 "getpw"
> 
> The patch below fixes my issue.  The pledge condition was already a bit
> long, so I just switched to snprintf(3); not sure what's normally done
> in such situations.
> 
> Regards,
>   Michael

Hi,

thanks for your bug report. I think we still want pledge calls and arguments
grepable, so i would like to commit this instead:

ok?

diff --git usr.bin/fstat/fstat.c usr.bin/fstat/fstat.c
index cc51086..98c9760 100644
--- usr.bin/fstat/fstat.c
+++ usr.bin/fstat/fstat.c
@@ -276,9 +276,19 @@ main(int argc, char *argv[])
errx(1, "%s", kvm_geterr(kd));
 
if (fuser) {
-   if (sflg) { /* fuser might call kill(2) */
+   if (sflg && uflg) {
+   /*
+* sflg (signal) calls kill(2) -> proc
+* uflg needs getpw
+*/
+   if (pledge("stdio getpw rpath proc", NULL) == -1)
+   err(1, "pledge");
+   } else if (sflg && !uflg) {
if (pledge("stdio rpath proc", NULL) == -1)
err(1, "pledge");
+   } else if (!sflg && uflg) {
+   if (pledge("stdio getpw rpath", NULL) == -1)
+   err(1, "pledge");
} else {
if (pledge("stdio rpath", NULL) == -1)
err(1, "pledge");

Re: ferror in ntpd (Re: bugs in printf(3))

[Sebastian Benoit]

as jca@ says, the clearerr() should be out of the loop, so ok benno@ too.

J??r??mie Courr??ges-Anglas(j...@wxcvbn.org) on 2015.12.29 19:18:55 +0100:
> "Todd C. Miller"  writes:
> 
> > On Tue, 29 Dec 2015 13:25:16 +0100, 
> > =?utf-8?Q?J=C3=A9r=C3=A9mie_Courr=C3=A8ges-
> > Anglas?= wrote:
> >
> >> I think it makes sense to try to recover, so calling clearerr() is
> >> needed.  But as said by millert you can't rely on fprintf to set the
> >> error indicator; the write might not be committed to disk, and the
> >> ftruncate and fsync calls below won't magically update the FILE's error
> >> indicator.
> >> 
> >> What about using "r = fflush(freqfp);" instead of ferror?
> >
> > How does this look?
> >
> >  - todd
> >
> > Index: ntpd.c
> > ===
> > RCS file: /cvs/src/usr.sbin/ntpd/ntpd.c,v
> > retrieving revision 1.101
> > diff -u -p -u -r1.101 ntpd.c
> > --- ntpd.c  19 Dec 2015 17:55:29 -  1.101
> > +++ ntpd.c  29 Dec 2015 17:51:56 -
> > @@ -552,12 +552,12 @@ writefreq(double d)
> > if (freqfp == NULL)
> > return 0;
> > rewind(freqfp);
> > -   fprintf(freqfp, "%.3f\n", d * 1e6); /* scale to ppm */
> > -   r = ferror(freqfp);
> > -   if (r != 0) {
> > +   r = fprintf(freqfp, "%.3f\n", d * 1e6); /* scale to ppm */
> > +   if (r < 0 || fflush(freqfp) != 0) {
> 
> Fine with me.
> 
> > if (warnonce) {
> > log_warnx("can't write %s", DRIFTFILE);
> > warnonce = 0;
> > +   clearerr(freqfp);
> > }
> 
> Looking closer, the clearerr() call should probably be out of the
> conditional.
> 
> With that fixed, ok jca@
> 
> > return 0;
> > }
> >
> 
> -- 
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE
> 

-- 

Re: Fix netstat(1) -P

[Sebastian Benoit]

ok, see nit below

Martin Pieuchot(m...@openbsd.org) on 2015.12.29 11:52:34 +0100:
> The "-P" option does not need to read routing table symbols, so there's
> no reason to bail if we cannot find them.
> 
> Index: main.c
> ===
> RCS file: /cvs/src/usr.bin/netstat/main.c,v
> retrieving revision 1.108
> diff -u -p -r1.108 main.c
> --- main.c23 Oct 2015 08:18:57 -  1.108
> +++ main.c29 Dec 2015 10:49:27 -
> @@ -124,7 +124,8 @@ main(int argc, char *argv[])
>   int Tflag = 0;
>   int repeatcount = 0;
>   int proto = 0;
> - int need_nlist;
> + int need_nlist, kvm_flags = O_RDONLY;
> +

needles whitespace?

>  
>   af = AF_UNSPEC;
>   tableid = getrtable();
> @@ -325,10 +326,11 @@ main(int argc, char *argv[])
>* The remaining code may need kvm so lets try to open it.
>* -r and -P are the only bits left that actually can use this.
>*/
> - need_nlist = nlistf != NULL || memf != NULL || Pflag || (Aflag && 
> rflag);
> + need_nlist = (nlistf != NULL) || (memf != NULL) || (Aflag && rflag);
> + if (!need_nlist && !Pflag)
> + kvm_flags |= KVM_NO_FILES;
>  
> - if ((kvmd = kvm_openfiles(nlistf, memf, NULL, O_RDONLY |
> - (need_nlist ? 0 : KVM_NO_FILES), buf)) == NULL)
> + if ((kvmd = kvm_openfiles(nlistf, memf, NULL, kvm_flags, buf)) == NULL)
>   errx(1, "kvm_openfiles: %s", buf);
>  
>   if (need_nlist && (kvm_nlist(kvmd, nl) < 0 || nl[0].n_type == 0)) {
> 

-- 

dead assignements in usr.bin/systat

[Sebastian Benoit]

an earlier commit today prompted florian@ to run clang, these fixes are a
result of issues found.

ok?

[PATCH 1/4] remove unused variable cur. code probably c from
print_bar_title().

diff --git usr.bin/systat/engine.c usr.bin/systat/engine.c
index 51c0b7f..bc9f6ef 100644
--- usr.bin/systat/engine.c
+++ usr.bin/systat/engine.c
@@ -361,7 +361,7 @@ print_bar_title(field_def *fld)
 void
 print_fld_bar(field_def *fld, int value)
 {
-   int i, tw, val, cur;
+   int i, tw, val;
 
if (fld->width < 1)
return;
@@ -370,7 +370,7 @@ print_fld_bar(field_def *fld, int value)
tw = fld->arg / 2;
 
tb_start();
-   cur = 0;
+
for(i = 0; i < fld->width; i++) {
tw += fld->arg;
 
[PATCH 2/4] remove useless assignement to variable change.

diff --git usr.bin/systat/engine.c usr.bin/systat/engine.c
index bc9f6ef..faefaa0 100644
--- usr.bin/systat/engine.c
+++ usr.bin/systat/engine.c
@@ -488,7 +488,6 @@ field_setup(void)
width -= fwid;
}
 
-   change = 0;
while (width > 0) {
change = 0;
for (fp = curr_view->view; *fp != NULL; fp++) {

[PATCH 3/4] garbage collect unused variable tm

diff --git usr.bin/systat/cpu.c usr.bin/systat/cpu.c
index 520bb93..8bb5be1 100644
--- usr.bin/systat/cpu.c
+++ usr.bin/systat/cpu.c
@@ -246,12 +246,10 @@ initcpu(void)
 void
 print_cpu(void)
 {
-   time_t  tm;
int cur = 0, c, i;
int end = dispstart + maxprint;
int64_t *states;
double  value[CPUSTATES];
-   tm = time(NULL);
 
if (end > num_disp)
end = num_disp;

[PATCH 4/4] make sure debug will be initialized, choose "unknown"
 because pfctl loglevel_to_string() uses "unknown" as well.

diff --git usr.bin/systat/pf.c usr.bin/systat/pf.c
index 0833618..58c0d69 100644
--- usr.bin/systat/pf.c
+++ usr.bin/systat/pf.c
@@ -259,6 +259,9 @@ print_pf(void)
case LOG_DEBUG:
debug = "debug";
break;
+   default:
+   debug = "unknown";
+   break;
}
ADD_LINE_S("pf", "Debug", debug);
 

Re: Remove unused variable in usr.bin/systat/main.c

[Sebastian Benoit]

thanks, committed

evh(e...@riseup.net) on 2016.01.02 15:34:50 +0100:
> int ms in cmd_count is unused as of 1.63
> 
> Index: main.c
> ===
> RCS file: /cvs/src/usr.bin/systat/main.c,v
> retrieving revision 1.63
> diff -u -p -r1.63 main.c
> --- main.c18 Apr 2015 18:28:38 -  1.63
> +++ main.c1 Jan 2016 19:26:39 -
> @@ -297,7 +297,6 @@ void
>  cmd_count(const char *buf)
>  {
>   const char *errstr;
> - int ms;
>  
>   maxprint = strtonum(buf, 1, lines - HEADER_LINES, );
>   if (errstr)
> 

-- 

ferror in ntpd (Re: bugs in printf(3))

[Sebastian Benoit]

Todd C. Miller(todd.mil...@courtesan.com) on 2015.12.28 10:46:08 -0700:
> On Fri, 25 Dec 2015 00:30:29 +0100, Ingo Schwarze wrote:
> 
> > Besides, i don't see the point in messing with FILE flags at all
> > in case of encoding errors.  As opposed to fgetwc(3) and fputwc(3),
> > the manual doesn't document this fiddling, and POSIX doesn't ask
> > for it.  No other conversions in printf(3) set the error indicator.
> > It isn't required because printf(3) provides a proper error return
> > (-1) in the first place.  Has anybody ever seen any code calling
> > ferror(3) after printf(3)?  That just wouldn't make sense.
> > Of course, printf(3) can result in the error indicator getting set,
> > but only if the underlying low-level write calls fail.
> 
> You are correct that neither our man page nor ISO C document
> setting the error indicator.  I get your expected output on Linux
> and Solaris so I think this diff is correct.  I wonder if it is
> worth documenting that the error indicator is not set in fprintf(3)?
> 
> We do have code in our tree that checks ferror() after doing writes
> via fprintf.  One example src/usr.sbin/smtpd, but I have not done
> an exhaustive check.

ntpd is one of these.

if the error is set, then ntpd has a bug i think, since it never calls
clearerr()

ok?

(sorry for hijacking the thread)

diff --git usr.sbin/ntpd/ntpd.c usr.sbin/ntpd/ntpd.c
index cf88fe8..df7dedb 100644
--- usr.sbin/ntpd/ntpd.c
+++ usr.sbin/ntpd/ntpd.c
@@ -558,6 +558,7 @@ writefreq(double d)
if (warnonce) {
log_warnx("can't write %s", DRIFTFILE);
warnonce = 0;
+   clearerr(freqfp);
}
return 0;
}

Re: vlan(4): better checks for valid vlan ids

[Sebastian Benoit]

David Gwynne(da...@gwynne.id.au) on 2015.12.22 11:06:21 +1000:
> the spec says vlan 0 and vlan 4095 are reserved, so we probably
> shouldnt use them.
> 
> this tweaks the vlan tag check only allow valid ids per the spec.
> 
> ok?

code reads ok

however, this could be tweaked in ifconfig too:
3589:   __tag = tag = strtonum(val, 0, 4095, );

i can do that if you commit this (or you do it).

> 
> Index: if_vlan.c
> ===
> RCS file: /cvs/src/sys/net/if_vlan.c,v
> retrieving revision 1.150
> diff -u -p -r1.150 if_vlan.c
> --- if_vlan.c 8 Dec 2015 11:35:42 -   1.150
> +++ if_vlan.c 22 Dec 2015 01:04:24 -
> @@ -156,6 +156,8 @@ vlan_clone_create(struct if_clone *ifc, 
>   else
>   ifv->ifv_type = ETHERTYPE_VLAN;
>  
> + ifv->ifv_tag = EVL_VLID_MIN;
> +
>   refcnt_init(>ifv_refcnt);
>  
>   ifp->if_start = vlan_start;
> @@ -586,6 +588,7 @@ vlan_ioctl(struct ifnet *ifp, u_long cmd
>   struct ifvlan   *ifv;
>   struct vlanreq   vlr;
>   int  error = 0, s;
> + uint16_t tag;
>  
>   ifr = (struct ifreq *)data;
>   ifa = (struct ifaddr *)data;
> @@ -630,15 +633,18 @@ vlan_ioctl(struct ifnet *ifp, u_long cmd
>   error = ENOENT;
>   break;
>   }
> +
>   /*
>* Don't let the caller set up a VLAN tag with
>* anything except VLID bits.
>*/
> - if (vlr.vlr_tag & ~EVL_VLID_MASK) {
> + tag = vlr.vlr_tag;
> + if (tag < EVL_VLID_MIN || tag > EVL_VLID_MAX) {
>   error = EINVAL;
>   break;
>   }
> - error = vlan_config(ifv, pr, vlr.vlr_tag);
> +
> + error = vlan_config(ifv, pr, tag);
>   if (error)
>   break;
>   ifp->if_flags |= IFF_RUNNING;
> Index: if_vlan_var.h
> ===
> RCS file: /cvs/src/sys/net/if_vlan_var.h,v
> retrieving revision 1.31
> diff -u -p -r1.31 if_vlan_var.h
> --- if_vlan_var.h 3 Dec 2015 16:27:32 -   1.31
> +++ if_vlan_var.h 22 Dec 2015 01:04:24 -
> @@ -42,7 +42,10 @@ struct ether_vlan_header {
>   u_int16_t evl_proto;
>  };
>  
> -#define  EVL_VLID_MASK   0x0FFF
> +#define  EVL_VLID_MASK   0xFFF
> +/* 0x000 and 0xFFF are reserved */
> +#define  EVL_VLID_MIN0x001
> +#define  EVL_VLID_MAX0xFFE
>  #define  EVL_VLANOFTAG(tag) ((tag) & EVL_VLID_MASK)
>  #define  EVL_PRIOFTAG(tag) (((tag) >> EVL_PRIO_BITS) & 7)
>  #define  EVL_ENCAPLEN4   /* length in octets of encapsulation */
> 

-- 

Re: [patch] uname(1) tweaks

[Sebastian Benoit]

frit...@alokat.org(frit...@alokat.org) on 2015.12.24 14:45:51 +0100:
> On Thu, Dec 24, 2015 at 02:19:36PM +0100, Theo Buehler wrote:
> > On Thu, Dec 24, 2015 at 01:52:56PM +0100, frit...@alokat.org wrote:
> > > Hi tech@,
> > > 
> > > here are some tweaks about uname(1):
> > > 
> > > - change the parameter order to the same order as in the manpage
> > > - change err(1, NULL) to errx(1, "uname")
> > 
> > Why?  This doesn't make sense to me.  Like this you'll suppress the
> > error string generated from errno.
> >
> 
> Thanks for the hint.
> 
> > apart from this, this would be ok tb@

ok benno@ with whitespace on empty lines fixed (see below).
tb? you do it?

> 
> New version attached below.
>  
> > > - change statements like: if (condition) statement 
> > >   if (condition)
> > >   statement
> > > 
> > > - activate the stack protector
> > > 
> > > --F.
> 
> 
> Index: uname.c
> ===
> RCS file: /cvs/src/usr.bin/uname/uname.c,v
> retrieving revision 1.16
> diff -u -r1.16 uname.c
> --- uname.c   9 Oct 2015 01:37:09 -   1.16
> +++ uname.c   24 Dec 2015 13:41:16 -
> @@ -73,6 +73,9 @@
>   case 'n':
>   print_mask |= PRINT_NODENAME;
>   break;
> + case 'p':
> + print_mask |= PRINT_MACHINE_ARCH;
> + break;
>   case 'r':
>   print_mask |= PRINT_RELEASE;
>   break;
> @@ -82,9 +85,6 @@
>   case 'v':
>   print_mask |= PRINT_VERSION;
>   break;
> - case 'p':
> - print_mask |= PRINT_MACHINE_ARCH;
> - break;
>   default:
>   usage();
>   /* NOTREACHED */
> @@ -100,39 +100,46 @@
>   print_mask = PRINT_SYSNAME;
>   }
>  
> - if (uname()) {
> - err(1, NULL);
> - /* NOTREACHED */
> - }
> + if (uname())
> + err(1, "uname");
>  
>   if (print_mask & PRINT_SYSNAME) {
>   space++;
>   fputs(u.sysname, stdout);
>   }
>   if (print_mask & PRINT_NODENAME) {
> - if (space++) putchar(' ');
> + if (space++)
> + putchar(' ');
> + 

whitespace

>   fputs(u.nodename, stdout);
>   }
>   if (print_mask & PRINT_RELEASE) {
> - if (space++) putchar(' ');
> + if (space++)
> + putchar(' ');
> +
>   fputs(u.release, stdout);
>   }
>   if (print_mask & PRINT_VERSION) {
> - if (space++) putchar(' ');
> + if (space++)
> + putchar(' ');
> +
>   fputs(u.version, stdout);
>   }
>   if (print_mask & PRINT_MACHINE) {
> - if (space++) putchar(' ');
> + if (space++)
> + putchar(' ');
> + 

whitespace

>   fputs(u.machine, stdout);
>   }
>   if (print_mask & PRINT_MACHINE_ARCH) {
> - if (space++) putchar(' ');
> + if (space++)
> + putchar(' ');
> + 

whitespace

>   fputs(MACHINE_ARCH, stdout);
>   }
>   putchar('\n');
>  
> - exit(0);
> - /* NOTREACHED */
> + return 0;
>  }
>  
>  static void
> 

-- 

Re: ifconfig: remove undocumented -carpdev

[Sebastian Benoit]

Fabian Raetz(fabian.ra...@gmail.com) on 2015.12.30 13:32:54 +0100:
> On Wed, Dec 30, 2015 at 07:24:01AM -0500, Ted Unangst wrote:
> > Fabian Raetz wrote:
> > > Hi,
> > > 
> > > please find below a patch to remove the undocumented -carpdev command from
> > > ifconfig(8).
> > 
> > wouldn't it make more sense to document the command?
> 
> as the command is broken [0] and nobody noticed it so far, i'm
> wondering if anybody needs -carpdev
> 
> [0] https://www.marc.info/?l=openbsd-tech=145147632420539=2

-carpdev was valid back when you did not have to specify the device. Until
some time ago there was magic that chose the parent dev based on the ip on
the carp - the idea was that it must be in the same net as the parent.

This was removed (by mpi i think) to reduce complexety. So now you always
have to specify the carpdev.

I believe the option can go.

Re: [nc] rename sun to s_un (for building on Solaris)

[Sebastian Benoit]

Brent Cook(bust...@gmail.com) on 2015.11.22 16:32:49 -0600:
> 
> Finally getting around to trying out nc on some more platforms for
> LibreSSL-portable, and ran into Sun/Oracle's silly definition of 'sun'
> in the system headers. OK to rename the local sockaddr_un variables?

ok benno@
 
> Portable contains a patch full of #ifdef's around other systems-specific
> nc features (e.g. rdomain support). I'm not sure if any of those
> should also move into cvs or stay in the portable patch.

while for nc it might be fine (small programm etc) i would not like to have
this elsewhere (i.e. ntpd, relayd, bgpd...) because it will make reading the
code harder.

i'm ok with restructuring our code a bit if it reduces your ifdef hell.
 
> Index: netcat.c
> ===
> RCS file: /cvs/src/usr.bin/nc/netcat.c,v
> retrieving revision 1.143
> diff -u -p -u -p -r1.143 netcat.c
> --- netcat.c  13 Nov 2015 18:13:13 -  1.143
> +++ netcat.c  22 Nov 2015 22:26:08 -
> @@ -643,7 +643,7 @@ main(int argc, char *argv[])
>  int
>  unix_bind(char *path, int flags)
>  {
> - struct sockaddr_un sun;
> + struct sockaddr_un s_un;
>   int s;
> 
>   /* Create unix domain socket. */
> @@ -651,17 +651,17 @@ unix_bind(char *path, int flags)
>   0)) < 0)
>   return (-1);
> 
> - memset(, 0, sizeof(struct sockaddr_un));
> - sun.sun_family = AF_UNIX;
> + memset(_un, 0, sizeof(struct sockaddr_un));
> + s_un.sun_family = AF_UNIX;
> 
> - if (strlcpy(sun.sun_path, path, sizeof(sun.sun_path)) >=
> - sizeof(sun.sun_path)) {
> + if (strlcpy(s_un.sun_path, path, sizeof(s_un.sun_path)) >=
> + sizeof(s_un.sun_path)) {
>   close(s);
>   errno = ENAMETOOLONG;
>   return (-1);
>   }
> 
> - if (bind(s, (struct sockaddr *), sizeof(sun)) < 0) {
> + if (bind(s, (struct sockaddr *)_un, sizeof(s_un)) < 0) {
>   close(s);
>   return (-1);
>   }
> @@ -737,7 +737,7 @@ tls_setup_server(struct tls *tls_ctx, in
>  int
>  unix_connect(char *path)
>  {
> - struct sockaddr_un sun;
> + struct sockaddr_un s_un;
>   int s;
> 
>   if (uflag) {
> @@ -748,16 +748,16 @@ unix_connect(char *path)
>   return (-1);
>   }
> 
> - memset(, 0, sizeof(struct sockaddr_un));
> - sun.sun_family = AF_UNIX;
> + memset(_un, 0, sizeof(struct sockaddr_un));
> + s_un.sun_family = AF_UNIX;
> 
> - if (strlcpy(sun.sun_path, path, sizeof(sun.sun_path)) >=
> - sizeof(sun.sun_path)) {
> + if (strlcpy(s_un.sun_path, path, sizeof(s_un.sun_path)) >=
> + sizeof(s_un.sun_path)) {
>   close(s);
>   errno = ENAMETOOLONG;
>   return (-1);
>   }
> - if (connect(s, (struct sockaddr *), sizeof(sun)) < 0) {
> + if (connect(s, (struct sockaddr *)_un, sizeof(s_un)) < 0) {
>   close(s);
>   return (-1);
>   }
> 

-- 

Re: use ping6(8)'s engine in ping(8)

[Sebastian Benoit]

ok

Florian Obser(flor...@openbsd.org) on 2015.11.29 15:57:34 +:
> This shoves a round peg into a square hole with considerable force...
> I was only concerned with moving the functionality over from ping6,
> further cleanup will happen on top of this.
> 
> OK?
> 
> diff --git ping.c ping.c
> index 4944f77..a3a6fe3 100644
> --- ping.c
> +++ ping.c
> @@ -147,8 +147,7 @@ unsigned long nreceived;  /* # of packets we got back */
>  unsigned long nrepeats;  /* number of duplicates */
>  unsigned long ntransmitted;  /* sequence # for outbound packets = #sent */
>  unsigned long nmissedmax = 1;/* max value of ntransmitted - 
> nreceived - 1 */
> -double interval = 1; /* interval between packets */
> -struct itimerval interstr;   /* interval structure for use with setitimer */
> +struct timeval interval = {1, 0}; /* interval between packets */
>  
>  /* timing */
>  int timing;  /* flag to do timing */
> @@ -163,17 +162,21 @@ int bufspace = IP_MAXPACKET;
>  struct tv64 tv64_offset;
>  SIPHASH_KEY mac_key;
>  
> +volatile sig_atomic_t seenalrm;
> +volatile sig_atomic_t seenint;
> +volatile sig_atomic_t seeninfo;
> +
>  void fill(char *, char *);
> -void catcher(int signo);
> -void prtsig(int signo);
> -__dead void finish(int signo);
> -void summary(int, int);
> +void summary(int);
>  int in_cksum(u_short *, int);
> -void pinger(void);
> +void onsignal(int);
> +void retransmit(void);
> +void onint(int);
> +int pinger(void);
>  char *pr_addr(in_addr_t);
>  int check_icmph(struct ip *);
>  void pr_icmph(struct icmp *);
> -void pr_pack(char *, int, struct sockaddr_in *);
> +void pr_pack(char *, int, struct msghdr *);
>  void pr_retip(struct ip *);
>  void pr_iph(struct ip *);
>  #ifndef SMALL
> @@ -186,15 +189,17 @@ main(int argc, char *argv[])
>  {
>   struct hostent *hp;
>   struct addrinfo hints, *res;
> - struct sockaddr_in  from4;
> + struct itimerval itimer;
> + struct sockaddr_in  from, from4;
>   struct sockaddr_in *to;
>   int ch, i, optval = 1, packlen, preload, maxsize, df = 0, tos = 0;
>   int error;
>   u_char *datap, *packet, ttl = MAXTTL, loop = 1;
> - char *target, hnamebuf[HOST_NAME_MAX+1], *source = NULL;
> + char *e, *target, hnamebuf[HOST_NAME_MAX+1], *source = NULL;
>   char rspace[3 + 4 * NROUTES + 1];   /* record route space */
>   socklen_t maxsizelen;
>   const char *errstr;
> + double intval;
>   uid_t uid;
>   u_int rtableid = 0;
>  
> @@ -242,19 +247,23 @@ main(int argc, char *argv[])
>   source = optarg;
>   break;
>   case 'i':   /* wait between sending packets */
> - interval = strtod(optarg, NULL);
> -
> - if (interval <= 0 || interval >= INT_MAX)
> - errx(1, "bad timing interval: %s", optarg);
> -
> - if (interval < 1)
> - if (getuid())
> - errx(1, "%s: only root may use interval 
> < 1s",
> - strerror(EPERM));
> -
> - if (interval < 0.01)
> - interval = 0.01;
> -
> + intval = strtod(optarg, );
> + if (*optarg == '\0' || *e != '\0')
> + errx(1, "illegal timing interval %s", optarg);
> + if (intval < 1 && getuid()) {
> + errx(1, "%s: only root may use interval < 1s",
> + strerror(EPERM));
> + }
> + interval.tv_sec = (time_t)intval;
> + interval.tv_usec =
> + (long)((intval - interval.tv_sec) * 100);
> + if (interval.tv_sec < 0)
> + errx(1, "illegal timing interval %s", optarg);
> + /* less than 1/Hz does not make sense */
> + if (interval.tv_sec == 0 && interval.tv_usec < 1) {
> + warnx("too small interval, raised to 0.01");
> + interval.tv_usec = 1;
> + }
>   options |= F_INTERVAL;
>   break;
>   case 'L':
> @@ -387,12 +396,6 @@ main(int argc, char *argv[])
>   arc4random_buf(_offset, sizeof(tv64_offset));
>   arc4random_buf(_key, sizeof(mac_key));
>  
> - memset(, 0, sizeof(interstr));
> -
> - interstr.it_value.tv_sec = interval;
> - interstr.it_value.tv_usec =
> - (long) ((interval - interstr.it_value.tv_sec) * 100);
> -
>   if (options & F_FLOOD && options & F_INTERVAL)
>   errx(1, "-f and -i options are incompatible");
>  
> @@ -508,26 +511,54 @@ main(int argc, char *argv[])
>   err(1, "pledge");
>   }
>  
> - (void)signal(SIGINT, 

Re: Memory corruptions in bc(1)

[Sebastian Benoit]

ok

Otto Moerbeek(o...@drijf.net) on 2015.11.20 14:22:12 +0100:
> On Fri, Nov 20, 2015 at 11:52:16AM +0100, Otto Moerbeek wrote:
> 
> > On Thu, Nov 19, 2015 at 05:52:39PM -0500, Michael McConville wrote:
> > 
> > > I'm already cache-thrashing with all of my side projects, so if anyone's
> > > interested I'll leave this to them.
> > > 
> > > A few days ago, I wanted to try American Fuzzy Lop (afl), and bc(1)
> > > seemed like a good first target: it pretty much just goes from stdin to
> > > stdout, so there's no code reorganization needed.
> > > 
> > > For those not familiar, bc compiles its input to dc(1)'s syntax and
> > > forks to dc.
> > > 
> > > There are many unique crash paths - 1041 before I killed afl. Most
> > > center around emit(), which emits a dc instr. Many pass NULL to fputs()
> > > in emit(). I found at least one (crashes/id:001041*) that
> > > nondeterministically passes the str pointer 0xdfdfdfdfdfdfdfdf to
> > > fputs(), which is probably uninitialized or already-freed memory.
> > > Backtrace below.
> > > 
> > > malloc.conf(5) may be useful.
> > > 
> > > Here's the full afl directory:
> > > 
> > >   http://www.sccs.swarthmore.edu/users/16/mmcconv1/bc-afl/
> > > 
> > > 
> > > Core was generated by `bc'.
> > > Program terminated with signal SIGBUS, Bus error.
> > > #0  strlen () at /usr/src/lib/libc/arch/amd64/string/strlen.S:152
> > > 152 movq(%rax),%rdx /* first data in high 
> > > bytes */
> > > (gdb) bt
> > > #0  strlen () at /usr/src/lib/libc/arch/amd64/string/strlen.S:152
> > > #1  0x19f79fa7c43d in *_libc_fputs (s=0xdfdfdfdfdfdfdfdf  > > Cannot access memory at address 0xdfdfdfdfdfdfdfdf>, fp=0x1) at 
> > > /usr/src/lib/libc/stdio/fputs.c:50
> > > #2  0x19f4ecb0f401 in emit (i=28548786530304) at 
> > > /usr/src/usr.bin/bc/bc.y:810
> > > #3  yyparse () at /usr/src/usr.bin/bc/bc.y:178
> > > #4  0x19f4ecb13f3e in main (argc=1, argv=0x7f7fa570) at 
> > > /usr/src/usr.bin/bc/bc.y:1188
> > 
> > This fixes at least one case  (id-000141*) and make the printing of
> > non-ascci chars better
> > 
> > -Otto
> > 
> 
> New version, which solves all cases found in crashes, hangs and queue
> above.  The remaining cases were emit going into an infinite recursion
> becuse the tree wasn't a tree but a cyclic graph.
> 
> Regress still succeeds.
> 
>   -Otto
> 
> Index: bc.y
> ===
> RCS file: /cvs/src/usr.bin/bc/bc.y,v
> retrieving revision 1.48
> diff -u -p -r1.48 bc.y
> --- bc.y  10 Oct 2015 19:28:54 -  1.48
> +++ bc.y  20 Nov 2015 13:19:07 -
> @@ -72,7 +72,7 @@ static void grow(void);
>  static ssize_t   cs(const char *);
>  static ssize_t   as(const char *);
>  static ssize_t   node(ssize_t, ...);
> -static void  emit(ssize_t);
> +static void  emit(ssize_t, int);
>  static void  emit_macro(int, ssize_t);
>  static void  free_tree(void);
>  static ssize_t   numnode(int);
> @@ -175,7 +175,7 @@ program   : /* empty */
>  
>  input_item   : semicolon_list NEWLINE
>   {
> - emit($1);
> + emit($1, 0);
>   macro_char = reset_macro_char;
>   putchar('\n');
>   free_tree();
> @@ -803,12 +803,17 @@ node(ssize_t arg, ...)
>  }
>  
>  static void
> -emit(ssize_t i)
> +emit(ssize_t i, int level)
>  {
> - if (instructions[i].index >= 0)
> - while (instructions[i].index != END_NODE)
> - emit(instructions[i++].index);
> - else
> + if (level > 1000)
> + errx(1, "internal error: tree level > 1000");
> + if (instructions[i].index >= 0) {
> + while (instructions[i].index != END_NODE &&
> + instructions[i].index != i)  {
> + emit(instructions[i].index, level + 1);
> + i++;
> + }
> + } else if (instructions[i].index != END_NODE)
>   fputs(instructions[i].u.cstr, stdout);
>  }
>  
> @@ -816,7 +821,7 @@ static void
>  emit_macro(int node, ssize_t code)
>  {
>   putchar('[');
> - emit(code);
> + emit(code, 0);
>   printf("]s%s\n", instructions[node].u.cstr);
>   nesting--;
>  }
> @@ -951,7 +956,7 @@ yyerror(char *s)
>   !isprint((unsigned char)yytext[0]))
>   n = asprintf(,
>   "%s: %s:%d: %s: ascii char 0x%02x unexpected",
> - __progname, filename, lineno, s, yytext[0]);
> + __progname, filename, lineno, s, yytext[0] & 0xff);
>   else
>   n = asprintf(, "%s: %s:%d: %s: %s unexpected",
>   __progname, filename, lineno, s, yytext);
> 

-- 

Re: bgpd: print AS range

[Sebastian Benoit]

hei,

thanks! i forgot that we print the config.

ok benno@, with whitespace fixed.

Denis Fondras(open...@ledeuns.net) on 2016.06.05 10:06:29 +0200:
> > This didn't quite work, as log_as will override itself when used twice
> > in the same printf.
> > 
> 
> I should not have sent this late at night...
> 
> 
> Index: printconf.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/printconf.c,v
> retrieving revision 1.96
> diff -u -p -r1.96 printconf.c
> --- printconf.c   21 Sep 2015 09:47:15 -  1.96
> +++ printconf.c   5 Jun 2016 08:00:59 -
> @@ -41,8 +41,9 @@ void print_peer(struct peer_config *, 
>  const char   *print_auth_alg(u_int8_t);
>  const char   *print_enc_alg(u_int8_t);
>  void  print_announce(struct peer_config *, const char *);
> +void  print_as(struct filter_rule *);
>  void  print_rule(struct peer *, struct filter_rule *);
> -const char *  mrt_type(enum mrt_type);
> +const char   *mrt_type(enum mrt_type);
>  void  print_mrt(struct bgpd_config *, u_int32_t, u_int32_t,
>   const char *, const char *);
>  void  print_groups(struct bgpd_config *, struct peer *);
> @@ -506,6 +507,26 @@ print_announce(struct peer_config *p, co
>   printf("%s\tannounce %s\n", c, aid2str(aid));
>  }
>  
> +void print_as(struct filter_rule *r)
> +{
> + switch(r->match.as.op) {
> + case OP_RANGE:
> +printf("%s - ", log_as(r->match.as.as_min));

whitespace

> + printf("%s ", log_as(r->match.as.as_max));
> + break;  
> + case OP_XRANGE:
> + printf("%s >< ", log_as(r->match.as.as_min));
> + printf("%s ", log_as(r->match.as.as_max));
> + break;
> + case OP_NE:
> + printf("!= %s ", log_as(r->match.as.as));
> + break;
> + default:
> + printf("%s ", log_as(r->match.as.as));
> + break;
> + }
> +}
> +
>  void
>  print_rule(struct peer *peer_l, struct filter_rule *r)
>  {
> @@ -577,15 +598,16 @@ print_rule(struct peer *peer_l, struct f
>  
>   if (r->match.as.type) {
>   if (r->match.as.type == AS_ALL)
> - printf("AS %s ", log_as(r->match.as.as));
> + printf("AS ");
>   else if (r->match.as.type == AS_SOURCE)
> - printf("source-as %s ", log_as(r->match.as.as));
> + printf("source-as ");
>   else if (r->match.as.type == AS_TRANSIT)
> - printf("transit-as %s ", log_as(r->match.as.as));
> + printf("transit-as ");
>   else if (r->match.as.type == AS_PEER)
> - printf("peer-as %s ", log_as(r->match.as.as));
> + printf("peer-as ");
>   else
> - printf("unfluffy-as %s ", log_as(r->match.as.as));
> + printf("unfluffy-as ");
> + print_as(r);
>   }
>  
>   if (r->match.aslen.type) {
> 

-- 

Re: bgpd: add format attributes

[Sebastian Benoit]

Martin Pieuchot(m...@openbsd.org) on 2016.06.05 20:06:17 +0200:
> On 04/06/16(Sat) 18:33, Sebastian Benoit wrote:
> > Add format attributes to the proper functions and then fix the warning in
> > session.c.
> 
> Shouldn't you introduce a log.h instead an make sure all daemons share
> the same log.h and log.c?

That question came up in the past. I can certainly do that.

However, can i put in the attributes first?

In bgpd i missed fatalx(), updated diff.

ok?


diff --git bgpd.h bgpd.h
index 5fa046e..e5145d4 100644
--- bgpd.h
+++ bgpd.h
@@ -989,16 +989,26 @@ struct in6_addr   *prefixlen2mask6(u_int8_t prefixlen);
 /* log.c */
 voidlog_init(int);
 voidlog_verbose(int);
-voidlogit(int, const char *, ...);
-voidvlog(int, const char *, va_list);
-voidlog_peer_warn(const struct peer_config *, const char *, ...);
-voidlog_peer_warnx(const struct peer_config *, const char *, ...);
-voidlog_warn(const char *, ...);
-voidlog_warnx(const char *, ...);
-voidlog_info(const char *, ...);
-voidlog_debug(const char *, ...);
-voidfatal(const char *, ...) __dead;
-voidfatalx(const char *) __dead;
+voidlogit(int, const char *, ...)
+   __attribute__((__format__ (printf, 2, 3)));
+voidvlog(int, const char *, va_list)
+   __attribute__((__format__ (printf, 2, 0)));
+voidlog_peer_warn(const struct peer_config *, const char *, ...)
+   __attribute__((__format__ (printf, 2, 3)));
+voidlog_peer_warnx(const struct peer_config *, const char *, ...)
+   __attribute__((__format__ (printf, 2, 3)));
+voidlog_warn(const char *, ...)
+   __attribute__((__format__ (printf, 1, 2)));
+voidlog_warnx(const char *, ...)
+   __attribute__((__format__ (printf, 1, 2)));
+voidlog_info(const char *, ...)
+   __attribute__((__format__ (printf, 1, 2)));
+voidlog_debug(const char *, ...)
+   __attribute__((__format__ (printf, 1, 2)));
+voidfatal(const char *, ...) __dead
+   __attribute__((__format__ (printf, 1, 2)));
+voidfatalx(const char *) __dead
+   __attribute__((__format__ (printf, 1, 0)));
 
 /* mrt.c */
 voidmrt_clear_seq(void);
diff --git session.c session.c
index 8c853a1..e736b76 100644
--- session.c
+++ session.c
@@ -2017,7 +2017,7 @@ parse_open(struct peer *peer)
 
/* check bgpid for validity - just disallow 0 */
if (ntohl(bgpid) == 0) {
-   log_peer_warnx(>conf, "peer BGPID %lu unacceptable",
+   log_peer_warnx(>conf, "peer BGPID %u unacceptable",
ntohl(bgpid));
session_notification(peer, ERR_OPEN, ERR_OPEN_BGPID,
NULL, 0);

Re: using srp inside art

[Sebastian Benoit]

Jonathan Matthew(jonat...@d14n.org) on 2016.06.06 17:14:53 +1000:
> We've finally got srp and art to the point where we can use srp to manage the
> internal links in the art data structures.  This allows us to do route lookups
> without holding any locks, which is kind of nice.
> 
> As we're not doing unlocked insert, delete or walk (yet), those art functions
> use the locked variants of the srp functions.  Currently the lock that
> protects those operations is the kernel lock.
> 
> Callers to art_lookup and art_match now pass in a pointer to an srp_ref, which
> is always active on return, even if no route is found.  In some situations we
> use these functions while modifying the routing table, in which case the
> kernel lock already protects against concurrent modifications so the srp_ref
> is unnecessary, so we srp_leave it immediately.
> 
> I'd also like to draw attention to the comment in rtable_match.  Is it OK that
> we might choose a multipath route at random if the set of available routes
> changes while we're choosing?

i dont see why this would be a problem

however:

+   ... if we were going to use
+* the last available route, but it got removed, we'll hit
+* the end of the list and then pick the first route.

would this make the first route get more traffic than the others,
statistically?

Re: using srp inside art

[Sebastian Benoit]

Martin Pieuchot(m...@openbsd.org) on 2016.06.08 20:50:29 +0200:
> On 08/06/16(Wed) 19:51, Sebastian Benoit wrote:
> > [...] 
> > i dont see why this would be a problem
> > 
> > however:
> > 
> > +   ... if we were going to use
> > +* the last available route, but it got removed, we'll hit
> > +* the end of the list and then pick the first route.
> > 
> > would this make the first route get more traffic than the others,
> > statistically?
> 
> What do you mean by "statistically"?  The comment apply to route lookups
> done by a CPU while another CPU is modifying the multipath list.  This
> should be a completely negligible amount of lookups.

thanks for explaining that. i thougt this might happen more often.

> Now assuming that you are voluntarily generating a lot of multipath route
> changes to trigger this case, what you call "the first" route will most
> likely be a different route after every change.  So it should not matter ;)

Re: ssl(8) kill "generating dsa server certificates"

[Sebastian Benoit]

ok!

Stuart Henderson(s...@spacehopper.org) on 2016.06.06 13:40:00 +0100:
> I don't think we should be encouraging anyone to do this...ok?
> 
> Index: ssl.8
> ===
> RCS file: /cvs/src/share/man/man8/ssl.8,v
> retrieving revision 1.63
> diff -u -p -r1.63 ssl.8
> --- ssl.8 8 Feb 2016 19:29:58 -   1.63
> +++ ssl.8 6 Jun 2016 12:38:26 -
> @@ -112,38 +112,6 @@ you can switch to using the new certific
>  with the certificate signed by your Certificate Authority, and then
>  restarting
>  .Xr httpd 8 .
> -.Sh GENERATING DSA SERVER CERTIFICATES
> -Generating a DSA certificate involves several steps.
> -First, generate parameters for DSA keys.
> -The following command will generate 1024-bit keys:
> -.Bd -literal -offset indent
> -# openssl dsaparam 1024 -out dsa1024.pem
> -.Ed
> -.Pp
> -Once you have the DSA parameters generated, you can generate a
> -CSR and unencrypted private key using the command:
> -.Bd -literal -offset indent
> -# openssl req -nodes -newkey dsa:dsa1024.pem \e
> -  -out /etc/ssl/dsacert.csr -keyout /etc/ssl/private/dsakey.pem
> -.Ed
> -.Pp
> -To generate an encrypted private key, you would use:
> -.Bd -literal -offset indent
> -# openssl req -newkey dsa:dsa1024.pem \e
> -  -out /etc/ssl/dsacert.csr -keyout /etc/ssl/private/dsakey.pem
> -.Ed
> -.Pp
> -This
> -.Pa server.csr
> -file can then be given to a CA who will sign the key.
> -.Pp
> -You can also sign the key yourself, using the command:
> -.Bd -literal -offset indent
> -# openssl x509 -sha256 -req -days 365 \e
> -  -in /etc/ssl/private/dsacert.csr \e
> -  -signkey /etc/ssl/private/dsacert.key \e
> -  -out /etc/ssl/dsacert.crt
> -.Ed
>  .Sh GENERATING ECDSA SERVER CERTIFICATES
>  First, generate parameters for ECDSA keys.
>  The following command will use a NIST/SECG curve over a 384-bit
> 

-- 

bgpd: add format attributes

[Sebastian Benoit]

Add format attributes to the proper functions and then fix the warning in
session.c.

ok?

diff --git bgpd.h bgpd.h
index 5fa046e..eaf93e6 100644
--- bgpd.h
+++ bgpd.h
@@ -989,15 +989,24 @@ struct in6_addr   *prefixlen2mask6(u_int8_t prefixlen);
 /* log.c */
 voidlog_init(int);
 voidlog_verbose(int);
-voidlogit(int, const char *, ...);
-voidvlog(int, const char *, va_list);
-voidlog_peer_warn(const struct peer_config *, const char *, ...);
-voidlog_peer_warnx(const struct peer_config *, const char *, ...);
-voidlog_warn(const char *, ...);
-voidlog_warnx(const char *, ...);
-voidlog_info(const char *, ...);
-voidlog_debug(const char *, ...);
-voidfatal(const char *, ...) __dead;
+voidlogit(int, const char *, ...)
+   __attribute__((__format__ (printf, 2, 3)));
+voidvlog(int, const char *, va_list)
+   __attribute__((__format__ (printf, 2, 0)));
+voidlog_peer_warn(const struct peer_config *, const char *, ...)
+   __attribute__((__format__ (printf, 2, 3)));
+voidlog_peer_warnx(const struct peer_config *, const char *, ...)
+   __attribute__((__format__ (printf, 2, 3)));
+voidlog_warn(const char *, ...)
+   __attribute__((__format__ (printf, 1, 2)));
+voidlog_warnx(const char *, ...)
+   __attribute__((__format__ (printf, 1, 2)));
+voidlog_info(const char *, ...)
+   __attribute__((__format__ (printf, 1, 2)));
+voidlog_debug(const char *, ...)
+   __attribute__((__format__ (printf, 1, 2)));
+voidfatal(const char *, ...) __dead
+   __attribute__((__format__ (printf, 1, 2)));
 voidfatalx(const char *) __dead;
 
 /* mrt.c */
diff --git session.c session.c
index 8c853a1..e736b76 100644
--- session.c
+++ session.c
@@ -2017,7 +2017,7 @@ parse_open(struct peer *peer)
 
/* check bgpid for validity - just disallow 0 */
if (ntohl(bgpid) == 0) {
-   log_peer_warnx(>conf, "peer BGPID %lu unacceptable",
+   log_peer_warnx(>conf, "peer BGPID %u unacceptable",
ntohl(bgpid));
session_notification(peer, ERR_OPEN, ERR_OPEN_BGPID,
NULL, 0);

ospfd: add format attributes

[Sebastian Benoit]

In ospfd, add format attributes to the proper functions and then fix the
warning in rde.c.

ok?

diff --git log.h log.h
index e0034e8..a682f67 100644
--- log.h
+++ log.h
@@ -23,13 +23,21 @@
 
 void   log_init(int);
 void   log_verbose(int);
-void   logit(int, const char *, ...);
-void   vlog(int, const char *, va_list);
-void   log_warn(const char *, ...);
-void   log_warnx(const char *, ...);
-void   log_info(const char *, ...);
-void   log_debug(const char *, ...);
-void   fatal(const char *) __dead;
-void   fatalx(const char *) __dead;
+void   logit(int, const char *, ...)
+   __attribute__((__format__ (printf, 2, 3)));
+void   vlog(int, const char *, va_list)
+   __attribute__((__format__ (printf, 2, 0)));
+void   log_warn(const char *, ...)
+   __attribute__((__format__ (printf, 1, 2)));
+void   log_warnx(const char *, ...)
+   __attribute__((__format__ (printf, 1, 2)));
+void   log_info(const char *, ...)
+   __attribute__((__format__ (printf, 1, 2)));
+void   log_debug(const char *, ...)
+   __attribute__((__format__ (printf, 1, 2)));
+void   fatal(const char *) __dead
+   __attribute__((__format__ (printf, 1, 0)));
+void   fatalx(const char *) __dead
+   __attribute__((__format__ (printf, 1, 0)));
 
 #endif /* _LOG_H_ */
diff --git rde.c rde.c
index 6d53eb3..eca497a 100644
--- rde.c
+++ rde.c
@@ -374,7 +374,7 @@ rde_dispatch_imsg(int fd, short event, void *bula)
}
}
if (l != 0 && !error)
-   log_warnx("rde_dispatch_imsg: peerid %lu, "
+   log_warnx("rde_dispatch_imsg: peerid %u, "
"trailing garbage in Database Description "
"packet", imsg.hdr.peerid);
 
@@ -411,7 +411,7 @@ rde_dispatch_imsg(int fd, short event, void *bula)
ntohs(v->lsa->hdr.len));
}
if (l != 0)
-   log_warnx("rde_dispatch_imsg: peerid %lu, "
+   log_warnx("rde_dispatch_imsg: peerid %u, "
"trailing garbage in LS Request "
"packet", imsg.hdr.peerid);
break;

ospf6d: add format attributes

[Sebastian Benoit]

In ospf6d, add format attributes to the proper functions and then fix the
warning in rde.c

ok?

diff --git log.h log.h
index 0cc7403..8cccd8f 100644
--- log.h
+++ log.h
@@ -23,14 +23,22 @@
 
 voidlog_init(int);
 voidlog_verbose(int);
-voidlogit(int, const char *, ...);
-voidvlog(int, const char *, va_list);
-voidlog_warn(const char *, ...);
-voidlog_warnx(const char *, ...);
-voidlog_info(const char *, ...);
-voidlog_debug(const char *, ...);
-voidfatal(const char *) __dead;
-voidfatalx(const char *) __dead;
+voidlogit(int, const char *, ...)
+   __attribute__((__format__ (printf, 2, 3)));
+voidvlog(int, const char *, va_list)
+   __attribute__((__format__ (printf, 2, 0)));
+voidlog_warn(const char *, ...)
+   __attribute__((__format__ (printf, 1, 2)));
+voidlog_warnx(const char *, ...)
+   __attribute__((__format__ (printf, 1, 2)));
+voidlog_info(const char *, ...)
+   __attribute__((__format__ (printf, 1, 2)));
+voidlog_debug(const char *, ...)
+   __attribute__((__format__ (printf, 1, 2)));
+voidfatal(const char *) __dead
+   __attribute__((__format__ (printf, 1, 0)));
+voidfatalx(const char *) __dead
+   __attribute__((__format__ (printf, 1, 0)));
 
 const char *log_in6addr(const struct in6_addr *);
 const char *log_in6addr_scope(const struct in6_addr *, unsigned int);
diff --git rde.c rde.c
index 1de5d14..1d4b426 100644
--- rde.c
+++ rde.c
@@ -356,7 +356,7 @@ rde_dispatch_imsg(int fd, short event, void *bula)
}
}
if (l != 0)
-   log_warnx("rde_dispatch_imsg: peerid %lu, "
+   log_warnx("rde_dispatch_imsg: peerid %u, "
"trailing garbage in Database Description "
"packet", imsg.hdr.peerid);
 
@@ -387,7 +387,7 @@ rde_dispatch_imsg(int fd, short event, void *bula)
ntohs(v->lsa->hdr.len));
}
if (l != 0)
-   log_warnx("rde_dispatch_imsg: peerid %lu, "
+   log_warnx("rde_dispatch_imsg: peerid %u, "
"trailing garbage in LS Request "
"packet", imsg.hdr.peerid);
break;

Re: bgpd: filter as path with operators

[Sebastian Benoit]

Claudio Jeker(cje...@diehard.n-r-g.com) on 2016.05.31 08:10:22 +0200:
> On Mon, May 30, 2016 at 10:43:49PM +0200, Sebastian Benoit wrote:
> > Hi,
> > 
> > this allows to have
> > 
> >   allow from any AS 64512 - 65534 ...
> >   allow from any AS > 100
> > 
> > etc in bgpd.conf.
> > 
> > Ignore the example file for now, i will commit that seperatly anyway.
> > 
> > One obvious improvment would be to be able to use this in bgpctl to restrict
> > the output of "show rib" a bit more. However, that is for another commit i
> > think, the bgpctl bit in this diff is just to make it compile and work.
> > 
> > ok?
> 
> I see no reason to add the unary operators (>, <, ...). Please provide a
> real live example that makes sense. IMO there is no reason to do something
> like AS >  because the selectivity is random.

Yes, i wanted the - and ><, i put the others in because that allows me to
use them in 'bgpctl sh rib', which i thought might be nice there. I planned
to change bgpctl seperatly though.

Opinons? Its easy enough to remove them again.

> I don't mind the - and >< operators and I think they make sense to use.
> I just don't see the point of added complexity for a case that never
> happens.
> 
> more comments inline
>  
> > /Benno
> > 
> > diff --git etc/examples/bgpd.conf etc/examples/bgpd.conf
> > index 8ffa8a8..02a31f9 100644
> > --- etc/examples/bgpd.conf
> > +++ etc/examples/bgpd.conf
> > @@ -119,3 +119,14 @@ deny from any prefix fc00::/7 prefixlen >= 7   
> > # unique local unicast
> >  deny from any prefix fe80::/10 prefixlen >= 10 # link local 
> > unicast
> >  deny from any prefix fec0::/10 prefixlen >= 10 # old site 
> > local unicast
> >  deny from any prefix ff00::/8 prefixlen >= 8   # multicast
> > +
> > +# filter bogon AS numbers
> > +# http://www.iana.org/assignments/as-numbers/as-numbers.xhtml
> > +deny from any AS 23456 # AS_TRANS
> > +deny from any AS 64496 - 64511 # Reserved for use in 
> > docs and code RFC5398
> > +deny from any AS 64512 - 65534 # Reserved for Private 
> > Use RFC6996
> > +deny from any AS 65535 # Reserved RFC7300
> > +deny from any AS 65536 - 65551 # Reserved for use in 
> > docs and code RFC5398 
> > +deny from any AS 65552 - 131071# Reserved
> > +deny from any AS 42 - 4294967294   # Reserved for Private Use 
> > RFC6996
> > +deny from any AS 4294967295# Reserved RFC7300
> 
> 
> Did you check how many pathes in a regular feed hit any of those rules?

Yes, AS_TRANS is in the wild constantly, the others appear from time, probably
because spammers try things. As they are not legaly to be used on the public
net there should be a way to block them, if only to spare yourself from
public embarrassment.

> I have seen a few pathes with private or AS_TRANS ASs in them in the wild.
> For a default filterset this may be a bit too restrictive.

Thats why i said i would leave that out of the commit for seperate
discussion.

> > diff --git usr.sbin/bgpctl/bgpctl.c usr.sbin/bgpctl/bgpctl.c
> > index 62569bf..afdcf2c 100644
> > --- usr.sbin/bgpctl/bgpctl.c
> > +++ usr.sbin/bgpctl/bgpctl.c
> > @@ -1788,7 +1788,7 @@ show_mrt_dump(struct mrt_rib *mr, struct mrt_peer 
> > *mp, void *arg)
> > /* filter by AS */
> > if (req->as.type != AS_NONE &&
> >!aspath_match(mre->aspath, mre->aspath_len,
> > -  req->as.type, req->as.as))
> > +  >as, req->as.as))
> > continue;
> >  
> > if (req->flags & F_CTL_DETAIL) {
> > @@ -1853,7 +1853,7 @@ network_mrt_dump(struct mrt_rib *mr, struct mrt_peer 
> > *mp, void *arg)
> > /* filter by AS */
> > if (req->as.type != AS_NONE &&
> >!aspath_match(mre->aspath, mre->aspath_len,
> > -  req->as.type, req->as.as))
> > +  >as, req->as.as))
> 
> Would be nice if it is not required to pass both the as obj and the
> req->as.as but them matching on neighbor-as needs some work...
> Anyway this is a bikeshed problem.

It is needed in the neighbor-as case. I thought leaving it as it is makes it
obvious that there is a special case.

I will resend the diff when i know if i should throw out =,>=,> etc...

> > co

Re: kdump relative timestamps

[Sebastian Benoit]

ok

Ted Unangst(t...@tedunangst.com) on 2016.06.01 13:41:01 -0400:
> Relative timestamps are much easier to decipher than absolute, when attempting
> to determine a program's behavior. Most of the time I care about how long
> since the last time. However, if I grep the output, then I lose the basis for
> relative times, and am back to doing large number substraction in my head.
> 
> What I really want in many cases, is timestamps relative to the start of the
> trace. I didn't know what letter to pick, so since I want a mix of T and R, I
> just made the option be both together.
> 
> 
> Index: kdump.c
> ===
> RCS file: /cvs/src/usr.bin/kdump/kdump.c,v
> retrieving revision 1.127
> diff -u -p -r1.127 kdump.c
> --- kdump.c   30 Mar 2016 08:00:01 -  1.127
> +++ kdump.c   1 Jun 2016 17:34:29 -
> @@ -185,11 +185,11 @@ main(int argc, char *argv[])
>   if (errstr)
>   errx(1, "-p %s: %s", optarg, errstr);
>   break;
> - case 'R':
> - timestamp = 2;  /* relative timestamp */
> + case 'R':   /* relative timestamp */
> + timestamp = timestamp == 1 ? 3 : 2;
>   break;
>   case 'T':
> - timestamp = 1;
> + timestamp = timestamp == 2 ? 3 : 1;
>   break;
>   case 't':
>   trpoints = getpoints(optarg);
> @@ -349,7 +349,11 @@ dumpheader(struct ktr_header *kth)
>   basecol += printf("/%-7ld", (long)kth->ktr_tid);
>   basecol += printf(" %-8.*s ", MAXCOMLEN, kth->ktr_comm);
>   if (timestamp) {
> - if (timestamp == 2) {
> + if (timestamp == 3) {
> + if (prevtime.tv_sec == 0)
> + prevtime = kth->ktr_time;
> + timespecsub(>ktr_time, , );
> + } else if (timestamp == 2) {
>   timespecsub(>ktr_time, , );
>   prevtime = kth->ktr_time;
>   } else
> Index: kdump.1
> ===
> RCS file: /cvs/src/usr.bin/kdump/kdump.1,v
> retrieving revision 1.30
> diff -u -p -r1.30 kdump.1
> --- kdump.1   6 Mar 2016 20:25:27 -   1.30
> +++ kdump.1   1 Jun 2016 17:39:24 -
> @@ -92,6 +92,8 @@ specified.
>  Display relative timestamps (time since previous entry).
>  .It Fl T
>  Display absolute timestamps for each entry (seconds since the Epoch).
> +.It Fl TR
> +If both options are specified, display timestamps relative to trace start.
>  .It Fl t Op Cm cinstuxX+
>  Selects which tracepoints to display.
>  See the
> 

-- 

Re: bgpd: filter as path with operators

[Sebastian Benoit]

with feedback from florian, sthen and claudio:

- i removed operators < <= > >=
- i kept != and = for symmetry.
- i thought about just using ! , but then it would be different from the
  prefix operators. Willing to change it if you want that.
- i left the forth argument to aspath_match(), as its easier to undestand
  than other things i could think of
- i removed a debug comment
- the usecase for this is mostly whats shown in the example. I think i can
  also simplyfy some filters setting policy with != in my configs.
- as phessler said, the example filters any AS that should not be seen in
  the wild at the moment. You want those filtered because its bad practice
  not to. Also, if you use private AS inside your network, the example
  config will remind you to make sure not to leak them. I'm reasonably
  convinced that these will not change in the near future.

ok?

diff --git etc/examples/bgpd.conf etc/examples/bgpd.conf
index 8ffa8a8..02a31f9 100644
--- etc/examples/bgpd.conf
+++ etc/examples/bgpd.conf
@@ -119,3 +119,14 @@ deny from any prefix fc00::/7 prefixlen >= 7   
# unique local unicast
 deny from any prefix fe80::/10 prefixlen >= 10 # link local unicast
 deny from any prefix fec0::/10 prefixlen >= 10 # old site local unicast
 deny from any prefix ff00::/8 prefixlen >= 8   # multicast
+
+# filter bogon AS numbers
+# http://www.iana.org/assignments/as-numbers/as-numbers.xhtml
+deny from any AS 23456 # AS_TRANS
+deny from any AS 64496 - 64511 # Reserved for use in docs and 
code RFC5398
+deny from any AS 64512 - 65534 # Reserved for Private Use 
RFC6996
+deny from any AS 65535 # Reserved RFC7300
+deny from any AS 65536 - 65551 # Reserved for use in docs and 
code RFC5398 
+deny from any AS 65552 - 131071# Reserved
+deny from any AS 42 - 4294967294   # Reserved for Private Use 
RFC6996
+deny from any AS 4294967295# Reserved RFC7300
diff --git usr.sbin/bgpctl/bgpctl.c usr.sbin/bgpctl/bgpctl.c
index 62569bf..afdcf2c 100644
--- usr.sbin/bgpctl/bgpctl.c
+++ usr.sbin/bgpctl/bgpctl.c
@@ -1788,7 +1788,7 @@ show_mrt_dump(struct mrt_rib *mr, struct mrt_peer *mp, 
void *arg)
/* filter by AS */
if (req->as.type != AS_NONE &&
   !aspath_match(mre->aspath, mre->aspath_len,
-  req->as.type, req->as.as))
+  >as, req->as.as))
continue;
 
if (req->flags & F_CTL_DETAIL) {
@@ -1853,7 +1853,7 @@ network_mrt_dump(struct mrt_rib *mr, struct mrt_peer *mp, 
void *arg)
/* filter by AS */
if (req->as.type != AS_NONE &&
   !aspath_match(mre->aspath, mre->aspath_len,
-  req->as.type, req->as.as))
+  >as, req->as.as))
continue;
 
bzero(, sizeof(net));
diff --git usr.sbin/bgpd/bgpd.conf.5 usr.sbin/bgpd/bgpd.conf.5
index 84a01ed..126a037 100644
--- usr.sbin/bgpd/bgpd.conf.5
+++ usr.sbin/bgpd/bgpd.conf.5
@@ -1027,7 +1027,10 @@ If a parameter is specified, the rule only applies to 
packets with
 matching attributes.
 .Pp
 .Bl -tag -width Ds -compact
-.It Ar as-type as-number
+.It Xo
+.Ar as-type Op Ar operator
+.Ar as-number
+.Xc
 This rule applies only to
 .Em UPDATES
 where the
@@ -1038,13 +1041,7 @@ The
 is matched against a part of the
 .Em AS path
 specified by the
-.Ar as-type .
-.Ar as-number
-may be set to
-.Ic neighbor-as ,
-which is expanded to the current neighbor remote AS number.
-.Ar as-type
-is one of the following operators:
+.Ar as-type :
 .Pp
 .Bl -tag -width transmit-as -compact
 .It Ic AS
@@ -1057,6 +1054,29 @@ is one of the following operators:
 (all but the rightmost AS number)
 .El
 .Pp
+.Ar as-number
+is an AS number as explained above under
+.Sx GLOBAL CONFIGURATION .
+It may be set to
+.Ic neighbor-as ,
+which is expanded to the current neighbor remote AS number.
+.Pp
+The
+.Ar operator
+can be unspecified (this case is identical to the equality operator), or one 
of the numerical operators
+.Bd -literal -offset indent
+=  (equal)
+!= (unequal)
+-  (range including boundaries)
+>< (except range)
+.Ed
+.Pp
+>< and -
+are binary operators (they take two arguments), with these,
+.Ar as-number
+cannot be set to
+.Ic neighbor-as .
+.Pp
 Multiple
 .Ar as-number
 entries for a given type or
diff --git usr.sbin/bgpd/bgpd.h usr.sbin/bgpd/bgpd.h
index 86dfee9..9c635a8 100644
--- usr.sbin/bgpd/bgpd.h
+++ usr.sbin/bgpd/bgpd.h
@@ -625,6 +625,9 @@ struct filter_as {
u_int32_t   as;
u_int16_t   flags;
enum as_spectype;
+   u_int8_top;
+   u_int32_t   as_min;
+   u_int32_t   as_max;
 };
 
 struct filter_aslen {
@@ -660,7 +663,6 @@ struct filter_extcommunity {
}   data;
 };
 
-
 struct 

Re: dhclient reboot interval

[Sebastian Benoit]

Yes please.

I played with lower values in the past too and saw no problems.
And if we notice problems, we can fine tune it further.

ok.

Ted Unangst(t...@tedunangst.com) on 2016.06.01 15:37:53 -0400:
> Is there a reason the reboot timeout is so long?
> 
> Here's what I observe. I connect to one network and get a lease. Later, I
> connect to a different network. dhclient then spends an annoyingly long time
> doing this:
> 
> DHCPREQUEST on iwm0 to 255.255.255.255
> DHCPREQUEST on iwm0 to 255.255.255.255
> DHCPREQUEST on iwm0 to 255.255.255.255
> DHCPREQUEST on iwm0 to 255.255.255.255
> 
> Finally it gives up, and reverts to discover:
> 
> DHCPDISCOVER on iwm0 - interval 3
> 
> At which point I immediately get an offer, request, and ack.
> 
> In theory, the server should perhaps nack the request, but in pratice, dhcp
> servers don't do that. Instead we wait for a very long timeout.
> 
> Reducing the timeout makes things go much smoother when switching networks,
> but doesn't seem to have any ill effect when renewing on the same network.
> 
> Index: clparse.c
> ===
> RCS file: /cvs/src/sbin/dhclient/clparse.c,v
> retrieving revision 1.94
> diff -u -p -r1.94 clparse.c
> --- clparse.c 6 Feb 2016 19:30:52 -   1.94
> +++ clparse.c 1 Jun 2016 19:30:16 -
> @@ -86,7 +86,7 @@ read_client_conf(void)
>   config->link_timeout = 10;
>   config->timeout = 60;
>   config->select_interval = 0;
> - config->reboot_timeout = 10;
> + config->reboot_timeout = 1;
>   config->retry_interval = 300;
>   config->backoff_cutoff = 15;
>   config->initial_interval = 3;
> 

-- 

Re: netcat service lookup

[Sebastian Benoit]

Bob Beck(b...@openbsd.org) on 2016.05.31 23:25:47 -0600:
> Honestly, I care little about the incompatibility because we are
> already different.
> 
> However I do not think this is any "easier" - I never use
> /etc/services because frankly I can't
> predict what other non-openbsd systems will have in it.
> 
> even openssl s_client doens't add code complexity to do this - and
> that's saying something ;)
> 
> so frankly, I'd rather not. /etc/services is frankly IMO, obsolete..
> nobody looks up shit by name
> out of there anymore

Its not obsolete.

pf.conf uses names from /etc/services.
tcpdump does...

I certainly like to use names wherever i can.
 
> On Tue, May 31, 2016 at 11:17 PM, Theo de Raadt  
> wrote:
> >> This diff allows users to use the name of a service in /etc/services
> >> instead of a port number when using netcat. Hopefully, this will make
> >> using netcat easier for some users.
> >
> > I don't see how it makes it easier.  There are a number of netcat
> > versions out there, mostly trying to be somewhat compatible.  On a
> > whim, this introduces an incompatibility --> scripts become less
> > portable.

bgpd: filter as path with operators

[Sebastian Benoit]

Hi,

this allows to have

  allow from any AS 64512 - 65534 ...
  allow from any AS > 100

etc in bgpd.conf.

Ignore the example file for now, i will commit that seperatly anyway.

One obvious improvment would be to be able to use this in bgpctl to restrict
the output of "show rib" a bit more. However, that is for another commit i
think, the bgpctl bit in this diff is just to make it compile and work.

ok?

/Benno

diff --git etc/examples/bgpd.conf etc/examples/bgpd.conf
index 8ffa8a8..02a31f9 100644
--- etc/examples/bgpd.conf
+++ etc/examples/bgpd.conf
@@ -119,3 +119,14 @@ deny from any prefix fc00::/7 prefixlen >= 7   
# unique local unicast
 deny from any prefix fe80::/10 prefixlen >= 10 # link local unicast
 deny from any prefix fec0::/10 prefixlen >= 10 # old site local unicast
 deny from any prefix ff00::/8 prefixlen >= 8   # multicast
+
+# filter bogon AS numbers
+# http://www.iana.org/assignments/as-numbers/as-numbers.xhtml
+deny from any AS 23456 # AS_TRANS
+deny from any AS 64496 - 64511 # Reserved for use in docs and 
code RFC5398
+deny from any AS 64512 - 65534 # Reserved for Private Use 
RFC6996
+deny from any AS 65535 # Reserved RFC7300
+deny from any AS 65536 - 65551 # Reserved for use in docs and 
code RFC5398 
+deny from any AS 65552 - 131071# Reserved
+deny from any AS 42 - 4294967294   # Reserved for Private Use 
RFC6996
+deny from any AS 4294967295# Reserved RFC7300
diff --git usr.sbin/bgpctl/bgpctl.c usr.sbin/bgpctl/bgpctl.c
index 62569bf..afdcf2c 100644
--- usr.sbin/bgpctl/bgpctl.c
+++ usr.sbin/bgpctl/bgpctl.c
@@ -1788,7 +1788,7 @@ show_mrt_dump(struct mrt_rib *mr, struct mrt_peer *mp, 
void *arg)
/* filter by AS */
if (req->as.type != AS_NONE &&
   !aspath_match(mre->aspath, mre->aspath_len,
-  req->as.type, req->as.as))
+  >as, req->as.as))
continue;
 
if (req->flags & F_CTL_DETAIL) {
@@ -1853,7 +1853,7 @@ network_mrt_dump(struct mrt_rib *mr, struct mrt_peer *mp, 
void *arg)
/* filter by AS */
if (req->as.type != AS_NONE &&
   !aspath_match(mre->aspath, mre->aspath_len,
-  req->as.type, req->as.as))
+  >as, req->as.as))
continue;
 
bzero(, sizeof(net));
diff --git usr.sbin/bgpd/bgpd.conf.5 usr.sbin/bgpd/bgpd.conf.5
index 84a01ed..0017124 100644
--- usr.sbin/bgpd/bgpd.conf.5
+++ usr.sbin/bgpd/bgpd.conf.5
@@ -1027,7 +1027,10 @@ If a parameter is specified, the rule only applies to 
packets with
 matching attributes.
 .Pp
 .Bl -tag -width Ds -compact
-.It Ar as-type as-number
+.It Xo
+.Ar as-type Op Ar operator
+.Ar as-number
+.Xc
 This rule applies only to
 .Em UPDATES
 where the
@@ -1038,13 +1041,7 @@ The
 is matched against a part of the
 .Em AS path
 specified by the
-.Ar as-type .
-.Ar as-number
-may be set to
-.Ic neighbor-as ,
-which is expanded to the current neighbor remote AS number.
-.Ar as-type
-is one of the following operators:
+.Ar as-type :
 .Pp
 .Bl -tag -width transmit-as -compact
 .It Ic AS
@@ -1057,6 +1054,33 @@ is one of the following operators:
 (all but the rightmost AS number)
 .El
 .Pp
+.Ar as-number
+is an AS number as explained above under
+.Sx GLOBAL CONFIGURATION .
+It may be set to
+.Ic neighbor-as ,
+which is expanded to the current neighbor remote AS number.
+.Pp
+The
+.Ar operator
+can be unspecified (this case is identical to the equality operator), or one 
of the numerical operators
+.Bd -literal -offset indent
+=  (equal)
+!= (unequal)
+<  (less than)
+<= (less than or equal)
+>  (greater than)
+>= (greater than or equal)
+-  (range including boundaries)
+>< (except range)
+.Ed
+.Pp
+>< and -
+are binary operators (they take two arguments), with these,
+.Ar as-number
+cannot be set to
+.Ic neighbor-as .
+.Pp
 Multiple
 .Ar as-number
 entries for a given type or
diff --git usr.sbin/bgpd/bgpd.h usr.sbin/bgpd/bgpd.h
index 86dfee9..9c635a8 100644
--- usr.sbin/bgpd/bgpd.h
+++ usr.sbin/bgpd/bgpd.h
@@ -625,6 +625,9 @@ struct filter_as {
u_int32_t   as;
u_int16_t   flags;
enum as_spectype;
+   u_int8_top;
+   u_int32_t   as_min;
+   u_int32_t   as_max;
 };
 
 struct filter_aslen {
@@ -660,7 +663,6 @@ struct filter_extcommunity {
}   data;
 };
 
-
 struct ctl_show_rib_request {
charrib[PEER_DESCR_LEN];
struct ctl_neighbor neighbor;
@@ -1051,7 +1053,8 @@ const char*log_ext_subtype(u_int8_t);
 int aspath_snprint(char *, size_t, void *, u_int16_t);
 int aspath_asprint(char **, void *, u_int16_t);
 size_t  aspath_strlen(void *, u_int16_t);
-int   

Re: nd6 timers vs ticks

[Sebastian Benoit]

David Gwynne(da...@gwynne.id.au) on 2016.05.30 17:16:24 +1000:
> llinfo_nd6 thinks its expiry may extend beyond a timeout interval.
> 
> so it keeps track of the number of ticks it really wants via ln_ntick
> in llinfo_nd6 and schedules multiple timeouts to reach it.
> 
> i think this is a waste of time for two reasons:
> 
> 1. nd6_llinfo_settimer() (which sets this up) doesnt seem to be
> called with a timeout greater than what timeouts can handle. timeouts
> off the wire are ignored if theyre greater than an hour
> (MAX_REACHABLE_TIME), and the largest constant that ends up being
> passed is a day via nd6_gctimer. the fastest ticking arch we have
> is alpha with HZ at 1024, which wraps at about 24 days. we have
> space.
> 
> 2. ln_ntick is a long, which is the same size as int on 32 bit
> archs. the semantics it wants dont exist on a bunch of platforms.
> it is kind of arguing its own uselesness.
> 
> ok?

sounds reasonable. 
ok benno

> 
> Index: nd6.c
> ===
> RCS file: /cvs/src/sys/netinet6/nd6.c,v
> retrieving revision 1.179
> diff -u -p -r1.179 nd6.c
> --- nd6.c 17 May 2016 08:29:14 -  1.179
> +++ nd6.c 30 May 2016 07:08:09 -
> @@ -308,21 +308,16 @@ nd6_llinfo_settimer(struct llinfo_nd6 *l
>  {
>   int s;
>  
> + KASSERT(tick <= INT_MAX);
> +
>   s = splsoftnet();
>  
>   if (tick < 0) {
>   ln->ln_expire = 0;
> - ln->ln_ntick = 0;
>   timeout_del(>ln_timer_ch);
>   } else {
>   ln->ln_expire = time_second + tick / hz;
> - if (tick > INT_MAX) {
> - ln->ln_ntick = tick - INT_MAX;
> - timeout_add(>ln_timer_ch, INT_MAX);
> - } else {
> - ln->ln_ntick = 0;
> - timeout_add(>ln_timer_ch, tick);
> - }
> + timeout_add(>ln_timer_ch, tick);
>   }
>  
>   splx(s);
> @@ -341,18 +336,6 @@ nd6_llinfo_timer(void *arg)
>   s = splsoftnet();
>  
>   ln = (struct llinfo_nd6 *)arg;
> -
> - if (ln->ln_ntick > 0) {
> - if (ln->ln_ntick > INT_MAX) {
> - ln->ln_ntick -= INT_MAX;
> - nd6_llinfo_settimer(ln, INT_MAX);
> - } else {
> - ln->ln_ntick = 0;
> - nd6_llinfo_settimer(ln, ln->ln_ntick);
> - }
> - splx(s);
> - return;
> - }
>  
>   if ((rt = ln->ln_rt) == NULL)
>   panic("ln->ln_rt == NULL");
> Index: nd6.h
> ===
> RCS file: /cvs/src/sys/netinet6/nd6.h,v
> retrieving revision 1.58
> diff -u -p -r1.58 nd6.h
> --- nd6.h 30 Mar 2016 10:13:14 -  1.58
> +++ nd6.h 30 May 2016 07:08:09 -
> @@ -150,7 +150,6 @@ structllinfo_nd6 {
>   short   ln_state;   /* reachability state */
>   short   ln_router;  /* 2^0: ND6 router bit */
>  
> - longln_ntick;
>   struct  timeout ln_timer_ch;
>  };
>  
> 

-- 

Re: add mirror discovery to pkg_add

[Sebastian Benoit]

Ted Unangst(t...@tedunangst.com) on 2016.06.22 12:25:04 -0400:
> Marc Espie wrote:
> > This would allow pkg_add to auto-configure a mirror, for the case where
> > PKG_PATH was not specified and where pkg.conf does not exist.
> > 
> > It only triggers when a location ends up empty and when run in interactive
> > mode, e.g., it shouldn't interfere with local lookups.
> > 
> > Good idea, or awful ?
> 
> This would be pretty surprising to me I think. If for some reason I have
> failed to configure a mirror, I would prefer to get an error so I can fix the
> underlying problem. If we can't contact a DNS server, we don't fallback on a
> list of known public servers.
> 
> Auto config at install time is helpful, but this sort of dynamic auto config
> violates an important principle: it should be possible to unconfig something.

Aside from that, i dont think we should be hardcoding ip-adresses like that.

A name can be changed in DNS, but this will cause http requests to that ip
for quite some time.

Re: pf.conf macro with space

[Sebastian Benoit]

Henning Brauer(hb-openbsdt...@ml.bsws.de) on 2016.06.21 13:11:16 +0200:
> * Stefan Sperling  [2016-06-21 11:15]:
> > Generally, I would appreciate more detailed error messages from the pf.conf
> > parser. I recall several occasions where pfctl threw "syntax error" and more
> > specific error reporting would have saved me some time with finding the
> > silly mistake I made. And in this case the ruleset loads successfully even
> > though, while parsing, we already know it's not going to work as intended...
> 
> true, that's shared by all yacc-style parsers, if the grammar doesn't
> match you just get syntax error without much of a hint what's wrong.

the quality of the error message depends both on the language (some make
it harder to error out early) and on the parser.

fail early is better.

> however, the ruleset in this case does NOT load.
> 
>   $ echo '"a macro with spaces"="foo"\npass from $a\ macro\ 
> with\ spaces"' | pfctl -nvf - 
> a macro with spaces = "foo"
> stdin:2: macro 'a' not defined
> stdin:2: syntax error
>

sure, thats what the op reported

>
> > Only as long as it doesn't make the parser code overly complex, of course.
> > But currently the balance is tilted too much towards terse error messages
> > for my taste. So I liked benno's first diff.
> 
> it's just a tiny check indeed, which swings the "cost" (not in
> financial terms) vs benefit pendulum towards the benefit side, yes.

thx.

here is a diff for all daemons.

ok for this?


diff --git sbin/iked/parse.y sbin/iked/parse.y
index 958e51a..6455a00 100644
--- sbin/iked/parse.y
+++ sbin/iked/parse.y
@@ -73,6 +73,7 @@ intlookup(char *);
 int lgetc(int);
 int lungetc(int);
 int findeol(void);
+int has_whitespace(const char *);
 
 TAILQ_HEAD(symhead, sym)symhead = TAILQ_HEAD_INITIALIZER(symhead);
 struct sym {
@@ -1006,6 +1007,10 @@ string   : string STRING
 varset : STRING '=' string
{
log_debug("%s = \"%s\"\n", $1, $3);
+   if (has_whitespace($1)) {
+   yyerror("macro name cannot contain whitespace");
+   YYERROR;
+   }
if (symset($1, $3, 0) == -1)
err(1, "cannot store variable");
free($1);
@@ -1234,6 +1239,16 @@ findeol(void)
 }
 
 int
+has_whitespace(const char *s) {
+   while (*s != '\0') {
+   if (isspace((unsigned char)*s))
+   return 1;
+   s++;
+   }
+   return 0;
+}
+
+int
 yylex(void)
 {
unsigned charbuf[8096];
diff --git sbin/ipsecctl/parse.y sbin/ipsecctl/parse.y
index fe9cee0..67fbd72 100644
--- sbin/ipsecctl/parse.y
+++ sbin/ipsecctl/parse.y
@@ -71,6 +71,7 @@ intlookup(char *);
 int lgetc(int);
 int lungetc(int);
 int findeol(void);
+int has_whitespace(const char *);
 
 TAILQ_HEAD(symhead, sym)symhead = TAILQ_HEAD_INITIALIZER(symhead);
 struct sym {
@@ -883,6 +884,10 @@ varset : STRING '=' string
{
if (ipsec->opts & IPSECCTL_OPT_VERBOSE)
printf("%s = \"%s\"\n", $1, $3);
+   if (has_whitespace($1)) {
+   yyerror("macro name cannot contain whitespace");
+   YYERROR;
+   }
if (symset($1, $3, 0) == -1)
err(1, "cannot store variable");
free($1);
@@ -1092,6 +1097,16 @@ findeol(void)
 }
 
 int
+has_whitespace(const char *s) {
+   while (*s != '\0') {
+   if (isspace((unsigned char)*s))
+   return 1;
+   s++;
+   }
+   return 0;
+}
+
+int
 yylex(void)
 {
u_char   buf[8096];
diff --git sbin/pfctl/parse.y sbin/pfctl/parse.y
index 934438c..9846063 100644
--- sbin/pfctl/parse.y
+++ sbin/pfctl/parse.y
@@ -85,6 +85,7 @@ intlookup(char *);
 int lgetc(int);
 int lungetc(int);
 int findeol(void);
+int has_whitespace(const char *);
 
 TAILQ_HEAD(symhead, sym)symhead = TAILQ_HEAD_INITIALIZER(symhead);
 struct sym {
@@ -714,6 +715,10 @@ numberstring   : NUMBER
{
 varset : STRING '=' varstring  {
if (pf->opts & PF_OPT_VERBOSE)
printf("%s = \"%s\"\n", $1, $3);
+   if (has_whitespace($1)) {
+   yyerror("macro name cannot contain whitespace");
+   YYERROR;
+   }
if (symset($1, $3, 0) == -1)
err(1, "cannot store variable %s", $1);

Re: pf.conf macro with space

[Sebastian Benoit]

same thing without a stupid helper function, pointed out by henning.

diff --git sbin/pfctl/parse.y sbin/pfctl/parse.y
index 934438c..426cd93 100644
--- sbin/pfctl/parse.y
+++ sbin/pfctl/parse.y
@@ -714,6 +714,10 @@ numberstring   : NUMBER
{
 varset : STRING '=' varstring  {
if (pf->opts & PF_OPT_VERBOSE)
printf("%s = \"%s\"\n", $1, $3);
+   if (strchr($1, ' ') != NULL) {
+   yyerror("macro name cannot contain whitespace");
+   YYERROR;
+   }
if (symset($1, $3, 0) == -1)
err(1, "cannot store variable %s", $1);
    free($1);


Sebastian Benoit(be...@openbsd.org) on 2016.06.21 01:18:33 +0200:
> sven falempin(sven.falem...@gmail.com) on 2016.06.20 17:38:40 -0400:
> > Dear Tech Readers,
> > 
> > in a pf.conf file one can do
> > "silly things" = egress
> 
> Thanks for your diff, but
> 
> one, i dont think spaces in macros are useful in pf.conf.
> 
> second, we want to keep this consistent across all the parse.y in our code,
> so we have to think how this affects these(*)
> 
> Below is a diff that disallows "silly things".
> 
> I thinks it's easier to check that spaces in macros can be done without.
> 

Re: pf.conf macro with space

[Sebastian Benoit]

Stefan Sperling(s...@stsp.name) on 2016.06.21 10:23:13 +0200:
> On Tue, Jun 21, 2016 at 10:14:52AM +0200, Sebastian Benoit wrote:
> > 
> > same thing without a stupid helper function, pointed out by henning.
> > 
> > diff --git sbin/pfctl/parse.y sbin/pfctl/parse.y
> > index 934438c..426cd93 100644
> > --- sbin/pfctl/parse.y
> > +++ sbin/pfctl/parse.y
> > @@ -714,6 +714,10 @@ numberstring   : NUMBER
> > {
> >  varset : STRING '=' varstring  {
> > if (pf->opts & PF_OPT_VERBOSE)
> > printf("%s = \"%s\"\n", $1, $3);
> > +   if (strchr($1, ' ') != NULL) {
> 
> The previous version used isspace(3). Now, what about tabs? Do we not care?

*sigh*

i need coffee. two. big.
 
> > +   yyerror("macro name cannot contain whitespace");
> > +   YYERROR;
> > +   }
> > if (symset($1, $3, 0) == -1)
> > err(1, "cannot store variable %s", $1);
> > free($1);
> 

-- 

bgpd logging nexthop valid

[Sebastian Benoit]

i would like to make bgpd a bit more quiet.

This type of message

 bgpd[59424]: nexthop 1.2.3.4 now valid: via 192.168.0.1

happens quite often depending on your upstreams. This makes it a debug
message only.

ok?

diff --git usr.sbin/bgpd/bgpd.c usr.sbin/bgpd/bgpd.c
index 8e0031e..8925086 100644
--- usr.sbin/bgpd/bgpd.c
+++ usr.sbin/bgpd/bgpd.c
@@ -771,7 +771,7 @@ send_nexthop_update(struct kroute_nexthop *msg)
quit = 1;
}
 
-   log_info("nexthop %s now %s%s%s", log_addr(>nexthop),
+   log_debug("nexthop %s now %s%s%s", log_addr(>nexthop),
msg->valid ? "valid" : "invalid",
msg->connected ? ": directly connected" : "",
msg->gateway.aid ? gw : "");

Re: af-to on pass out should be a parser error

[Sebastian Benoit]

Mike Belopuhov(m...@belopuhov.com) on 2016.06.20 00:11:03 +0200:
> On Sun, Jun 19, 2016 at 23:43 +0200, Sebastian Benoit wrote:
> > manpage documents that af-to does not work on pass out rules, but the
> > pf.conf parser allows it, which leads a non working configuration being
> > loaded.
> > 
> > this changes the parser to make pass out .. af-to an error.
> > 
> > ok?
> > 
> 
> forgot to mention in my previous mail that af-to follows route-to
> in this regard.  you can say "pass out route-to" but in fact it's
> sort of pointless since the routing decision has already been made
> by the forwarding code.  i'm not certain doing route-to at this
> point produces a working result regarding created states, but that
> would indeed contrast with af-to where this is not a supported
> configuration.
> 
> to some extent "pass out af-to" also follows "pass out rdr-to" and
> "pass in nat-to" in a sense that they're not common and might not
> produce results one would expect, yet are parsed and installed into
> the kernel successfully.

yes,

i thought these were checked, but there is only a check to make sure
rdr/nat-to have a direction, not which one. i'll look at that tomorrow.

thanks.

af-to on pass out should be a parser error

[Sebastian Benoit]

manpage documents that af-to does not work on pass out rules, but the
pf.conf parser allows it, which leads a non working configuration being
loaded.

this changes the parser to make pass out .. af-to an error.

ok?

diff --git sbin/pfctl/parse.y sbin/pfctl/parse.y
index 934438c..0fecba8 100644
--- sbin/pfctl/parse.y
+++ sbin/pfctl/parse.y
@@ -1518,6 +1518,9 @@ pfrule: action dir logquick interface af 
proto fromto
}
if ($8.marker & FOM_AFTO)
r.rule_flag |= PFRULE_AFTO;
+   if ($8.marker & FOM_AFTO && r.direction == PF_OUT)
+   yyerror("af-to not possible with direction 
out");
+   YYERROR;
r.af = $5;
 
if ($8.tag)

Re: af-to on pass out should be a parser error

[Sebastian Benoit]

Mike Belopuhov(m...@belopuhov.com) on 2016.06.20 00:01:28 +0200:
> On Sun, Jun 19, 2016 at 23:43 +0200, Sebastian Benoit wrote:
> > manpage documents that af-to does not work on pass out rules, but the
> > pf.conf parser allows it, which leads a non working configuration being
> > loaded.
> > 
> > this changes the parser to make pass out .. af-to an error.
> >
> 
> what happens if the direction is not specified?

this works better i hope.

diff --git sbin/pfctl/parse.y sbin/pfctl/parse.y
index 934438c..c491b8e 100644
--- sbin/pfctl/parse.y
+++ sbin/pfctl/parse.y
@@ -1518,6 +1518,9 @@ pfrule: action dir logquick interface af 
proto fromto
}
if ($8.marker & FOM_AFTO)
r.rule_flag |= PFRULE_AFTO;
+   if ($8.marker & FOM_AFTO && r.direction != PF_IN)
+   yyerror("af-to can only be used with direction 
in");
+   YYERROR;
r.af = $5;
 
if ($8.tag)

Re: ascii.7: use standard name for ASCII LF and FF

[Sebastian Benoit]

Christian Weisgerber(na...@mips.inka.de) on 2016.01.30 17:45:14 +0100:
> From a similar FreeBSD commit:
>   Use standard name for ASCII LF and FF control codes.
> 
> Only overdue by a few decades.  OK?

ok
 
> Index: ascii.7
> ===
> RCS file: /cvs/src/share/man/man7/ascii.7,v
> retrieving revision 1.7
> diff -u -p -r1.7 ascii.7
> --- ascii.7   19 Jan 2014 10:39:00 -  1.7
> +++ ascii.7   30 Jan 2016 16:39:52 -
> @@ -42,7 +42,7 @@ The
>  set:
>  .Bd -literal -offset left
>  000 nul  001 soh  002 stx  003 etx  004 eot  005 enq  006 ack  007 bel
> -010 bs   011 ht   012 nl   013 vt   014 np   015 cr   016 so   017 si
> +010 bs   011 ht   012 lf   013 vt   014 ff   015 cr   016 so   017 si
>  020 dle  021 dc1  022 dc2  023 dc3  024 dc4  025 nak  026 syn  027 etb
>  030 can  031 em   032 sub  033 esc  034 fs   035 gs   036 rs   037 us
>  040 sp   041  !   042  "   043  #   044  $   045  %   046  &   047  '
> @@ -64,7 +64,7 @@ The
>  set:
>  .Bd -literal -offset left
>  00 nul   01 soh   02 stx   03 etx   04 eot   05 enq   06 ack   07 bel
> -08 bs09 ht0a nl0b vt0c np0d cr0e so0f si
> +08 bs09 ht0a lf0b vt0c ff0d cr0e so0f si
>  10 dle   11 dc1   12 dc2   13 dc3   14 dc4   15 nak   16 syn   17 etb
>  18 can   19 em1a sub   1b esc   1c fs1d gs1e rs1f us
>  20 sp21  !22  "23  #24  $25  %26  &27  '
> @@ -86,7 +86,7 @@ The
>  set:
>  .Bd -literal -offset left
>0 nul1 soh2 stx3 etx4 eot5 enq6 ack7 bel
> -  8 bs 9 ht10 nl11 vt12 np13 cr14 so15 si
> +  8 bs 9 ht10 lf11 vt12 ff13 cr14 so15 si
>   16 dle   17 dc1   18 dc2   19 dc3   20 dc4   21 nak   22 syn   23 etb
>   24 can   25 em26 sub   27 esc   28 fs29 gs30 rs31 us
>   32 sp33  !34  "35  #36  $37  %38  &39  '
> -- 
> Christian "naddy" Weisgerber  na...@mips.inka.de
> 

-- 

Re: ntpd: really enable debug messages

[Sebastian Benoit]

in relayd we use -v for that, so you need to run -d to get lots of
output. check main() there?

i think thats more intuitive, but maybe i'm just used to it.

Brent Cook(bust...@gmail.com) on 2016.01.20 06:31:44 -0600:
> Since the relatively recent logging unification, log_init needs a
> debug level > 1 in order for log_debug to print anything. This change
> makes it so 'ntpd -d' stays in the foreground but doesn't log much
> (the current behavior, different than previous releases though), 'ntpd
> -dd' actually prints more verbose debug messages.
> 
> Index: ntpd.8
> ===
> RCS file: /cvs/src/usr.sbin/ntpd/ntpd.8,v
> retrieving revision 1.40
> diff -u -p -u -p -r1.40 ntpd.8
> --- ntpd.8  30 Oct 2015 16:41:53 -  1.40
> +++ ntpd.8  20 Jan 2016 12:31:16 -
> @@ -50,6 +50,7 @@ If this option is specified,
>  .Nm
>  will run in the foreground and log to
>  .Em stderr .
> +It may be specified again to enable more verbose debug logs.
>  .It Fl f Ar file
>  Use
>  .Ar file
> Index: ntpd.c
> ===
> RCS file: /cvs/src/usr.sbin/ntpd/ntpd.c,v
> retrieving revision 1.103
> diff -u -p -u -p -r1.103 ntpd.c
> --- ntpd.c  11 Jan 2016 15:30:56 -  1.103
> +++ ntpd.c  20 Jan 2016 12:31:16 -
> @@ -137,7 +137,7 @@ main(int argc, char *argv[])
> while ((ch = getopt(argc, argv, "df:nsSv")) != -1) {
> switch (ch) {
> case 'd':
> -   lconf.debug = 1;
> +   lconf.debug++;
> log_verbose(1);
> break;
> case 'f':
> 

-- 

Re: Print ifindex in ifconfig(8)

[Sebastian Benoit]

Christian Weisgerber(na...@mips.inka.de) on 2016.04.12 14:43:50 +:
> On 2016-04-12, Martin Pieuchot  wrote:
> 
> > Relying on the "scopeid" field is not a viable long-term solution.  I'm
> > spending too much time these days trying to figure out which interface
> > correspond to which index.
> 
> What's the general use case for this?  I mean apart from network
> stack hacking?

snmp hacking ;)

Re: Print ifindex in ifconfig(8)

[Sebastian Benoit]

Martin Pieuchot(m...@openbsd.org) on 2016.04.12 16:25:36 +0200:
> On 12/04/16(Tue) 14:03, Stuart Henderson wrote:
> > On 2016/04/12 14:18, Claudio Jeker wrote:
> > > On Tue, Apr 12, 2016 at 01:47:53PM +0200, Stefan Sperling wrote:
> > > > On Tue, Apr 12, 2016 at 12:27:10PM +0100, Stuart Henderson wrote:
> > > > > On 2016/04/12 13:00, Martin Pieuchot wrote:
> > > > > > Relying on the "scopeid" field is not a viable long-term solution.  
> > > > > > I'm
> > > > > > spending too much time these days trying to figure out which 
> > > > > > interface
> > > > > > correspond to which index.
> > > > > > 
> > > > > > Here's a difference in output, then the diff itself.  ok?
> > > > > > 
> > > > > > @@ -1,31 +1,29 @@
> > > > > >  lo0: flags=8049 mtu 32768
> > > > > > +   index 4
> > > > > > priority: 0
> > > > > > groups: lo
> > > > > > inet6 ::1 prefixlen 128
> > > > > > inet6 fe80::1%lo0 prefixlen 64 scopeid 0x4
> > > > > > inet 127.0.0.1 netmask 0xff00
> > > > > >  em0: 
> > > > > > flags=18b43
> > > > > >  mtu 1500
> > > > > > -   lladdr f0:de:f9:1d:88:53
> > > > > > +   index 1 lladdr f0:de:f9:1d:88:53
> > > > > 
> > > > > This will break scripts, e.g. "awk '/lladdr/ {print $2}'"
> > > > > 
> > > > > I would expect putting it after lladdr would be better for the sort
> > > > > of scripts a user is likely to write, but bsd.rd would need a change
> > > > > if that was done, it uses sed 's/.*lladdr \(.*\)/\1/p;d'
> > > > > 
> > > > > On a new line would be safer.
> > > > 
> > > > How about appending to the flags line, like this?
> > > > 
> > > > lo0: flags=8049 mtu 32768 index 4
> > > > 
> > > 
> > > Or on the line with priority? The risk of breaking scripts that way is
> > > probably smaller.
> > 
> > I'd be happier with priority, the flags line can get quite long these days!
> 
> Like that?

yes, ok
(not an extra line -> ifconfig output is long enough)

> 
> em0: 
> flags=18b43 
> mtu 1500
> lladdr f1:ce:f9:1d:77:49
> index 1 priority 0
> trunk: trunkdev trunk0
> media: Ethernet autoselect (100baseTX full-duplex,rxpause,txpause)
> status: active
> 
> Index: ifconfig.c
> ===
> RCS file: /cvs/src/sbin/ifconfig/ifconfig.c,v
> retrieving revision 1.319
> diff -u -p -r1.319 ifconfig.c
> --- ifconfig.c6 Apr 2016 11:48:51 -   1.319
> +++ ifconfig.c12 Apr 2016 14:25:03 -
> @@ -2929,8 +2929,14 @@ status(int link, struct sockaddr_dl *sdl
>   strlen(ifrdesc.ifr_data))
>   printf("\tdescription: %s\n", ifrdesc.ifr_data);
>  
> - if (!is_bridge(name) && ioctl(s, SIOCGIFPRIORITY, ) == 0)
> - printf("\tpriority: %d\n", ifrdesc.ifr_metric);
> + if (sdl != NULL)
> + printf("\tindex %u", sdl->sdl_index);
> + if (!is_bridge(name) && ioctl(s, SIOCGIFPRIORITY, ) == 0) {
> + printf("%cpriority %d\n", (sdl != NULL) ? ' ' : '\t',
> + ifrdesc.ifr_metric);
> + } else if (sdl != NULL) {
> + putchar('\n');
> + }
>   (void) memset(, 0, sizeof(ikardesc));
>   (void) strlcpy(ikardesc.ikar_name, name, sizeof(ikardesc.ikar_name));
>   if (ioctl(s, SIOCGETKALIVE, ) == 0 &&
> 

-- 

route(4) diff

[Sebastian Benoit]

add missing RTF_CONNECTED.
remove ESIS (End System to Intermediate System Protocol), ann NDP in
comment.
add information about RTF_FMASK.

ok?

diff --git share/man/man4/route.4 share/man/man4/route.4
index 7c1402c..d17dbf3 100644
--- share/man/man4/route.4
+++ share/man/man4/route.4
@@ -356,23 +356,37 @@ Flags include the values:
 #defineRTF_DYNAMIC   0x10  /* created dynamically (by redirect) */
 #defineRTF_MODIFIED  0x20  /* modified dynamically (by redirect) */
 #defineRTF_DONE  0x40  /* message confirmed */
 #defineRTF_MASK  0x80  /* subnet mask present */
 #defineRTF_CLONING   0x100 /* generate new routes on use */
-#defineRTF_LLINFO0x400 /* generated by ARP or ESIS */
+#defineRTF_LLINFO0x400 /* generated by ARP or NDP */
 #defineRTF_STATIC0x800 /* manually added */
 #defineRTF_BLACKHOLE 0x1000/* just discard pkts (during updates) */
 #defineRTF_PROTO30x2000/* protocol specific routing flag */
 #defineRTF_PROTO20x4000/* protocol specific routing flag */
 #defineRTF_PROTO10x8000/* protocol specific routing flag */
 #defineRTF_CLONED0x1   /* this is a cloned route */
 #define RTF_MPATH 0x4   /* multipath route or operation */
 #define RTF_MPLS  0x10  /* MPLS additional infos */
 #define RTF_LOCAL 0x20 /* route to a local address */
 #define RTF_BROADCAST 0x40 /* route associated to a bcast addr. */
+#define RTF_CONNECTED 0x80 /* interface route */
 .Ed
 .Pp
+The following flags (defined as
+.Dv RTF_FMASK )
+can be changed by an RTM_CHANGE request:
+.Dv RTF_LLINFO ,
+.Dv RTF_PROTO1 ,
+.Dv RTF_PROTO2 ,
+.Dv RTF_PROTO3 ,
+.Dv RTF_BLACKHOLE ,
+.Dv RTF_REJECT ,
+.Dv RTF_STATIC
+and
+.Dv RTF_MPLS .
+.Pp
 Specifiers for metric values in
 .Va rmx_locks
 and
 .Va rtm_inits
 are:

Re: [patch] login_yubikey: delete keys

[Sebastian Benoit]

Hi Fritjof,

frit...@alokat.org(frit...@alokat.org) on 2016.03.31 11:43:58 +0200:
> Wipe out the key from "user.key".
> 
> --f.
> 
> Index: login_yubikey.c
> ===
> RCS file: /cvs/src/libexec/login_yubikey/login_yubikey.c,v
> retrieving revision 1.10
> diff -u -p -u -r1.10 login_yubikey.c
> --- login_yubikey.c 16 Jan 2015 06:39:50 -  1.10
> +++ login_yubikey.c 31 Mar 2016 09:38:01 -
> @@ -224,6 +224,8 @@ yubikey_login(const char *username, cons
> yubikey_hex_decode(uid, hexuid, YUBIKEY_UID_SIZE);
> yubikey_hex_decode(key, hexkey, YUBIKEY_KEY_SIZE);
>  
> +   explicit_bzero(hexkey, sizeof(hexkey));
> +
> /* 
>  * Cycle through the key mapping table.
>   * XXX brute force, unoptimized; a lookup table for valid mappings 
> may
> @@ -268,6 +270,8 @@ yubikey_login(const char *username, cons
> }
> break; /* only reached through the bottom of case 0 */
> }
> +
> +   explicit_bzero(key, sizeof(key));

The while loop above has return(AUTH_FAILED) so you dont zero in those
cases. Can you change that?

>  
> syslog(LOG_INFO, "user %s uid %s: %d matching keymaps (%d checked), "
> "%d crc ok", username, hexuid, mapok, i, crcok);
> 

Also your diff does not apply, i think it has tab vs space issues.

Re: [PATCH] Proposal to remove -f for arp(8) and ndp(8)

[Sebastian Benoit]

ok

Jeremie Courreges-Anglas(j...@wxcvbn.org) on 2016.03.31 19:16:14 +0200:
> Jeremie Courreges-Anglas  writes:
> 
> > Mike Belopuhov  writes:
> >
> >> Good day, Dimitris.
> >>
> >> Long time ago in a galaxy far far away I've been using this
> >> alongside the -F option that I've added.  While managed
> >> switches are becoming cheaper, I don't see a reason for a
> >> working feature to go away, especially since there has been
> >> zero rationale provided apart from "ndp -f" doesn't work.
> >> Well, then how about fixing ndp?
> > [...]
> >
> > Diff below, seems to work fine.
> 
> Now with the manpage improvements initially proposed by Dimitris.
> I have kept my ndp.c diff, because it is more consistent with the style
> of the rest of the file.  Dimitris's diff points out that main() never
> reports failures (exit(0)), it would be nice to fix.
> 
> ok?
> 
> Index: ndp.8
> ===
> RCS file: /cvs/src/usr.sbin/ndp/ndp.8,v
> retrieving revision 1.35
> diff -u -p -r1.35 ndp.8
> --- ndp.8 5 Oct 2015 10:25:19 -   1.35
> +++ ndp.8 31 Mar 2016 16:49:53 -
> @@ -117,6 +117,12 @@ Delete the specified NDP entry.
>  .It Fl f Ar filename
>  Parse the file specified by
>  .Ar filename .
> +Entries in the file should be of the form:
> +.Bd -ragged -offset indent -compact
> +.Ar nodename etheraddr
> +.Op Ar temp
> +.Op Ar proxy
> +.Ed
>  .It Fl H
>  Harmonize consistency between the routing table and the default router
>  list; install the top entry of the list into the kernel routing table.
> Index: ndp.c
> ===
> RCS file: /cvs/src/usr.sbin/ndp/ndp.c,v
> retrieving revision 1.69
> diff -u -p -r1.69 ndp.c
> --- ndp.c 26 Jan 2016 18:26:19 -  1.69
> +++ ndp.c 31 Mar 2016 16:49:53 -
> @@ -241,6 +241,11 @@ main(int argc, char *argv[])
>   }
>   delete(arg);
>   break;
> + case 'f':
> + if (argc != 0)
> + usage();
> + file(arg);
> + break;
>   case 'p':
>   if (argc != 0) {
>   usage();
> 
> 
> -- 
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE
> 

-- 

Re: use libtls in ldapd

[Sebastian Benoit]

Jonathan Matthew(jonat...@d14n.org) on 2016.04.18 07:17:55 +1000:
> On Sun, Apr 10, 2016 at 04:36:15PM +1000, Jonathan Matthew wrote:
> > A while back (s2k15?), reyk@ suggested I take a look at converting ldapd to 
> > use
> > libtls rather than the openssl api.  Today I finally got around to it,
> > resulting in the diff below.  Most of the diff just removes ssl.c and
> > ssl_privsep.c, and replaces some of it with evbuffer_tls.c (copied from
> > syslogd, unmodified).  A reasonable amount of code just went away because
> > libtls is sensible.  The few remaining bits of ssl.c moved to wherever 
> > seemed
> > most suitable.
> > 
> > I've tested a few things with the openldap clients, which apparently only do
> > starttls, and otherwise checked that it negotiates ssl successfully.
> > 
> > ok?
> 
> ldapd is too boring?

:)

ok from me, but i cant test it.

> 
> > 
> > 
> > Index: Makefile
> > ===
> > RCS file: /cvs/src/usr.sbin/ldapd/Makefile,v
> > retrieving revision 1.12
> > diff -u -p -u -p -r1.12 Makefile
> > --- Makefile16 Jul 2014 20:07:03 -  1.12
> > +++ Makefile10 Apr 2016 06:15:50 -
> > @@ -5,11 +5,11 @@ MAN=  ldapd.8 ldapd.conf.5
> >  SRCS=  ber.c log.c control.c \
> > util.c ldapd.c ldape.c conn.c attributes.c namespace.c \
> > btree.c filter.c search.c parse.y \
> > -   auth.c modify.c index.c ssl.c ssl_privsep.c \
> > +   auth.c modify.c index.c evbuffer_tls.c \
> > validate.c uuid.c schema.c imsgev.c syntax.c matching.c
> >  
> > -LDADD= -levent -lssl -lcrypto -lz -lutil
> > -DPADD= ${LIBEVENT} ${LIBCRYPTO} ${LIBSSL} ${LIBZ} ${LIBUTIL}
> > +LDADD= -ltls -levent -lz -lutil
> > +DPADD= ${LIBEVENT} ${LIBTLS} ${LIBCRYPTO} ${LIBSSL} ${LIBZ} 
> > ${LIBUTIL}
> >  CFLAGS+=   -I${.CURDIR} -g
> >  CFLAGS+=   -Wall -Wstrict-prototypes -Wmissing-prototypes
> >  CFLAGS+=   -Wmissing-declarations
> > Index: btree.c
> > ===
> > RCS file: /cvs/src/usr.sbin/ldapd/btree.c,v
> > retrieving revision 1.36
> > diff -u -p -u -p -r1.36 btree.c
> > --- btree.c 20 Mar 2016 00:01:22 -  1.36
> > +++ btree.c 10 Apr 2016 06:15:51 -
> > @@ -34,6 +34,8 @@
> >  #include 
> >  #include 
> >  
> > +#include 
> > +
> >  #include "btree.h"
> >  
> >  /* #define DEBUG */
> > Index: btree.h
> > ===
> > RCS file: /cvs/src/usr.sbin/ldapd/btree.h,v
> > retrieving revision 1.6
> > diff -u -p -u -p -r1.6 btree.h
> > --- btree.h 2 Jul 2010 01:43:00 -   1.6
> > +++ btree.h 10 Apr 2016 06:15:51 -
> > @@ -19,8 +19,6 @@
> >  #ifndef _btree_h_
> >  #define _btree_h_
> >  
> > -#include 
> > -
> >  struct mpage;
> >  struct cursor;
> >  struct btree_txn;
> > Index: conn.c
> > ===
> > RCS file: /cvs/src/usr.sbin/ldapd/conn.c,v
> > retrieving revision 1.12
> > diff -u -p -u -p -r1.12 conn.c
> > --- conn.c  2 Nov 2015 06:32:51 -   1.12
> > +++ conn.c  10 Apr 2016 06:15:51 -
> > @@ -26,6 +26,7 @@
> >  #include "ldapd.h"
> >  
> >  int conn_dispatch(struct conn *conn);
> > +int conn_tls_init(struct conn *);
> >  unsigned long   ldap_application(struct ber_element *elm);
> >  
> >  struct conn_listconn_list;
> > @@ -61,7 +62,7 @@ conn_close(struct conn *conn)
> > /* Cancel any queued requests on this connection. */
> > namespace_cancel_conn(conn);
> >  
> > -   ssl_session_destroy(conn);
> > +   tls_free(conn->tls);
> >  
> > TAILQ_REMOVE(_list, conn, next);
> > ber_free(>ber);
> > @@ -225,9 +226,8 @@ conn_write(struct bufferevent *bev, void
> > conn_close(conn);
> > else if (conn->s_flags & F_STARTTLS) {
> > conn->s_flags &= ~F_STARTTLS;
> > -   bufferevent_free(conn->bev);
> > -   conn->bev = NULL;
> > -   ssl_session_init(conn);
> > +   if (conn_tls_init(conn) == -1)
> > +   conn_close(conn);
> > }
> >  }
> >  
> > @@ -296,24 +296,22 @@ conn_accept(int fd, short event, void *d
> > goto giveup;
> > }
> > conn->ber.fd = -1;
> > -   conn->s_l = l;
> > ber_set_application(>ber, ldap_application);
> > conn->fd = afd;
> > conn->listener = l;
> >  
> > -   if (l->flags & F_LDAPS) {
> > -   ssl_session_init(conn);
> > -   } else {
> > -   conn->bev = bufferevent_new(afd, conn_read, conn_write,
> > -   conn_err, conn);
> > -   if (conn->bev == NULL) {
> > -   log_warn("conn_accept: bufferevent_new");
> > -   free(conn);
> > -   goto giveup;
> > -   }
> > -   bufferevent_enable(conn->bev, EV_READ);
> > -   

Re: [patch] login_yubikey: delete keys

[Sebastian Benoit]

frit...@alokat.org(frit...@alokat.org) on 2016.03.31 23:43:54 +0200:
> On Thu, Mar 31, 2016 at 10:17:45PM +0200, Sebastian Benoit wrote:
> > Hi Fritjof,
> > 
> > frit...@alokat.org(frit...@alokat.org) on 2016.03.31 11:43:58 +0200:
> > > Wipe out the key from "user.key".
> > > 
> > > --f.
> > > 
> > The while loop above has return(AUTH_FAILED) so you dont zero in those
> > cases. Can you change that?
> > 
> 
> Yeah, sure. See patch below.
> 
> > 
> > Also your diff does not apply, i think it has tab vs space issues.
> > 
> 
> Ah, shit. Should work now.

Thanks.

ok benno@
 
> Index: login_yubikey.c
> ===
> RCS file: /cvs/src/libexec/login_yubikey/login_yubikey.c,v
> retrieving revision 1.13
> diff -u -r1.13 login_yubikey.c
> --- login_yubikey.c   22 Oct 2015 23:56:30 -  1.13
> +++ login_yubikey.c   31 Mar 2016 21:35:28 -
> @@ -228,6 +228,8 @@
>   yubikey_hex_decode(uid, hexuid, YUBIKEY_UID_SIZE);
>   yubikey_hex_decode(key, hexkey, YUBIKEY_KEY_SIZE);
>  
> + explicit_bzero(hexkey, sizeof(hexkey));
> +
>   /*
>* Cycle through the key mapping table.
>   * XXX brute force, unoptimized; a lookup table for valid mappings 
> may
> @@ -239,6 +241,7 @@
>   case EMSGSIZE:
>   syslog(LOG_INFO, "user %s failed: password too short.",
>   username);
> + explicit_bzero(key, sizeof(key));
>   return (AUTH_FAILED);
>   case EINVAL:/* keyboard mapping invalid */
>   continue;
> @@ -264,14 +267,18 @@
>   syslog(LOG_INFO, "user %s: could not decode password "
>   "with any keymap (%d crc ok)",
>   username, crcok);
> + explicit_bzero(key, sizeof(key));
>   return (AUTH_FAILED);
>   default:
>   syslog(LOG_DEBUG, "user %s failed: %s",
>   username, strerror(r));
> + explicit_bzero(key, sizeof(key));
>   return (AUTH_FAILED);
>   }
>   break; /* only reached through the bottom of case 0 */
>   }
> +
> + explicit_bzero(key, sizeof(key));
>  
>   syslog(LOG_INFO, "user %s uid %s: %d matching keymaps (%d checked), "
>   "%d crc ok", username, hexuid, mapok, i, crcok);
> 

-- 

Re: netstat -W counters for 11n

[Sebastian Benoit]

ok benno@

Stefan Sperling(s...@stsp.name) on 2016.04.27 13:36:51 +0200:
> I'd like to add some 802.11n-related counters to netstat -W output.
> 
> The first diff below is for the kernel, the second for netstat.
> 
> ok?
> 
> Index: ieee80211_input.c
> ===
> RCS file: /cvs/src/sys/net80211/ieee80211_input.c,v
> retrieving revision 1.171
> diff -u -p -r1.171 ieee80211_input.c
> --- ieee80211_input.c 15 Apr 2016 03:04:27 -  1.171
> +++ ieee80211_input.c 27 Apr 2016 11:30:08 -
> @@ -707,7 +707,7 @@ ieee80211_input_ba(struct ieee80211com *
>   timeout_add_usec(>ba_to, ba->ba_timeout_val);
>  
>   if (SEQ_LT(sn, ba->ba_winstart)) {  /* SN < WinStartB */
> - ic->ic_stats.is_rx_dup++;
> + ic->ic_stats.is_ht_rx_frame_below_ba_winstart++;
>   m_freem(m); /* discard the MPDU */
>   return;
>   }
> @@ -730,6 +730,7 @@ ieee80211_input_ba(struct ieee80211com *
>   "%d, expecting %d:%d\n", __func__,
>   sn, ba->ba_winstart, ba->ba_winend);
>  #endif
> + ic->ic_stats.is_ht_rx_frame_above_ba_winend++;
>   if (count > ba->ba_winsize) {
>   if (ba->ba_winmiss < IEEE80211_BA_MAX_WINMISS) { 
>   if (ba->ba_missedsn == sn - 1)
> @@ -743,6 +744,7 @@ ieee80211_input_ba(struct ieee80211com *
>   }
>  
>   /* It appears the window has moved for real. */
> + ic->ic_stats.is_ht_rx_ba_window_jump++;
>   ba->ba_winmiss = 0;
>   ba->ba_missedsn = 0;
>  
> @@ -754,7 +756,8 @@ ieee80211_input_ba(struct ieee80211com *
>   ieee80211_input(ifp, ba->ba_buf[ba->ba_head].m,
>   ni, >ba_buf[ba->ba_head].rxi);
>   ba->ba_buf[ba->ba_head].m = NULL;
> - }
> + } else
> + ic->ic_stats.is_ht_rx_ba_frame_lost++;
>   ba->ba_head = (ba->ba_head + 1) %
>   IEEE80211_BA_MAX_WINSZ;
>   }
> @@ -769,6 +772,7 @@ ieee80211_input_ba(struct ieee80211com *
>   /* store the received MPDU in the buffer */
>   if (ba->ba_buf[idx].m != NULL) {
>   ifp->if_ierrors++;
> + ic->ic_stats.is_ht_rx_ba_no_buf++;
>   m_freem(m);
>   return;
>   }
> @@ -820,6 +824,8 @@ ieee80211_input_ba_gap_timeout(void *arg
>   struct ieee80211com *ic = ni->ni_ic;
>   int s, skipped;
>  
> + ic->ic_stats.is_ht_rx_ba_window_gap_timeout++;
> +
>   s = splnet();
>  
>   skipped = 0;
> @@ -828,6 +834,7 @@ ieee80211_input_ba_gap_timeout(void *arg
>   ba->ba_head = (ba->ba_head + 1) % IEEE80211_BA_MAX_WINSZ;
>   ba->ba_winstart = (ba->ba_winstart + 1) & 0xfff;
>   skipped++;
> + ic->ic_stats.is_ht_rx_ba_frame_lost++;
>   }
>   if (skipped > 0)
>   ba->ba_winend = (ba->ba_winstart + ba->ba_winsize - 1) & 0xfff;
> @@ -861,7 +868,8 @@ ieee80211_ba_move_window(struct ieee8021
>   ieee80211_input(ifp, ba->ba_buf[ba->ba_head].m, ni,
>   >ba_buf[ba->ba_head].rxi);
>   ba->ba_buf[ba->ba_head].m = NULL;
> - }
> + } else
> + ic->ic_stats.is_ht_rx_ba_frame_lost++;
>   ba->ba_head = (ba->ba_head + 1) % IEEE80211_BA_MAX_WINSZ;
>   }
>   /* move window forward */
> @@ -1580,6 +1588,7 @@ ieee80211_recv_probe_resp(struct ieee802
>   DPRINTF(("[%s] htprot change: was %d, now %d\n",
>   ether_sprintf((u_int8_t *)wh->i_addr2),
>   htprot_last, htprot));
> + ic->ic_stats.is_ht_prot_change++;
>   ic->ic_bss->ni_htop1 = ni->ni_htop1;
>   ic->ic_update_htprot(ic, ic->ic_bss);
>   }
> @@ -2491,6 +2500,7 @@ ieee80211_recv_addba_req(struct ieee8021
>   goto resp;
>   }
>   ba->ba_state = IEEE80211_BA_AGREED;
> + ic->ic_stats.is_ht_rx_ba_agreements++;
>   /* start Block Ack inactivity timer */
>   if (ba->ba_timeout_val != 0)
>   timeout_add_usec(>ba_to, ba->ba_timeout_val);
> @@ -2561,6 +2571,7 @@ ieee80211_recv_addba_resp(struct ieee802
>   }
>   /* MLME-ADDBA.confirm(Success) */
>   ba->ba_state = IEEE80211_BA_AGREED;
> + ic->ic_stats.is_ht_tx_ba_agreements++;
>  
>   /* notify drivers of this new Block Ack agreement */
>   if (ic->ic_ampdu_tx_start != NULL)
> Index: ieee80211_ioctl.h
> ===
> RCS file: /cvs/src/sys/net80211/ieee80211_ioctl.h,v
> 

Re: bioctl errx

[Sebastian Benoit]

ok

Ted Unangst(t...@tedunangst.com) on 2016.05.13 15:00:22 -0400:
> overzealous use of errx() hides useful information about the error.
> 
> 
> Index: bioctl.c
> ===
> RCS file: /cvs/src/sbin/bioctl/bioctl.c,v
> retrieving revision 1.130
> diff -u -p -r1.130 bioctl.c
> --- bioctl.c  4 Feb 2016 08:31:26 -   1.130
> +++ bioctl.c  13 May 2016 18:59:36 -
> @@ -1299,7 +1299,7 @@ derive_key_pkcs(int rounds, u_int8_t *ke
>   } else {
>   if (readpassphrase(prompt, passphrase, sizeof(passphrase),
>   rpp_flag) == NULL)
> - errx(1, "unable to read passphrase");
> + err(1, "unable to read passphrase");
>   }
>  
>   if (verify && !password) {
> @@ -1307,7 +1307,7 @@ derive_key_pkcs(int rounds, u_int8_t *ke
>   if (readpassphrase("Re-type passphrase: ", verifybuf,
>   sizeof(verifybuf), rpp_flag) == NULL) {
>   explicit_bzero(passphrase, sizeof(passphrase));
> - errx(1, "unable to read passphrase");
> + err(1, "unable to read passphrase");
>   }
>   if ((strlen(passphrase) != strlen(verifybuf)) ||
>   (strcmp(passphrase, verifybuf) != 0)) {
> 

-- 

Re: ndp(8) CPPFLAGS

[Sebastian Benoit]

ok

J??r??mie Courr??ges-Anglas(j...@wxcvbn.org) on 2016.05.02 13:21:51 +0200:
> 
> ndp.c doesn't have any #ifdef INET6 preprocessor directive, I can't see
> how keeping that in CPPFLAGS changes anything.  While here, -I${.CURDIR}
> isn't needed either.  Verified with sha256(1).
> 
> ok?
> 
> Index: Makefile
> ===
> RCS file: /cvs/src/usr.sbin/ndp/Makefile,v
> retrieving revision 1.3
> diff -u -p -r1.3 Makefile
> --- Makefile  9 Aug 2013 17:52:12 -   1.3
> +++ Makefile  2 May 2016 11:15:58 -
> @@ -4,6 +4,4 @@ PROG= ndp
>  SRCS=ndp.c gmt2local.c
>  MAN= ndp.8
>  
> -CPPFLAGS+=-DINET6 -I${.CURDIR}
> -
>  .include 
> 
> 
> -- 
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE
> 

-- 

Re: changelist: adds iked pub/private key ?

[Sebastian Benoit]

Sebastien Marie(sema...@openbsd.org) on 2016.04.20 08:38:40 +0200:
> Hi,
> 
> I noted that iked(8) default key (generated at boot time by rc(8) if it
> doesn't exist yet) aren't present in changelist(5), whereas the same
> keys for isakmpd(8) are.
> 
> Does adding /etc/iked/local.pub and /etc/iked/private/local.key to
> changelist(5) makes sens ?

of course, ok

> 
> -- 
> Sebastien Marie
> 
> 
> Index: changelist
> ===
> RCS file: /cvs/src/etc/changelist,v
> retrieving revision 1.110
> diff -u -p -r1.110 changelist
> --- changelist3 Oct 2015 18:57:11 -   1.110
> +++ changelist20 Apr 2016 06:33:59 -
> @@ -45,6 +45,8 @@
>  /etc/httpd.conf
>  /etc/ifstated.conf
>  +/etc/iked.conf
> +/etc/iked/local.pub
> ++/etc/iked/private/local.key
>  /etc/inetd.conf
>  +/etc/ipsec.conf
>  +/etc/isakmpd/isakmpd.conf
> 

-- 

Re: Alternative control socket location in ripd

[Sebastian Benoit]

reads ok benno@

Jeremie Courreges-Anglas(j...@wxcvbn.org) on 2016.08.02 13:48:11 +0200:
> Nima GHOTBI  writes:
> 
> > please try the attachments
> >
> > On Sun, Jul 31, 2016 at 7:27 PM, Jeremie Courreges-Anglas 
> > wrote:
> >
> >> Nima GHOTBI  writes:
> >>
> >> > Hi everyone
> >> >
> >> > In one of our projects we had to run multiple instances of ripd on
> >> > different rdomains so I made a patch to add "-s" argument to ripd and
> >> > ripctl to let the user change control socket path from /var/run/ripd.sock
> 
> Here's an updated diff that fixes whitespace and tries to match ospfd
> a bit more closely.
> 
> Comments / oks?
> 
> 
> Index: usr.sbin/ripd/control.c
> ===
> RCS file: /cvs/src/usr.sbin/ripd/control.c,v
> retrieving revision 1.22
> diff -u -p -r1.22 control.c
> --- usr.sbin/ripd/control.c   5 Dec 2015 13:13:47 -   1.22
> +++ usr.sbin/ripd/control.c   2 Aug 2016 11:35:57 -
> @@ -39,7 +39,7 @@ struct ctl_conn *control_connbypid(pid_t
>  void  control_close(int);
>  
>  int
> -control_init(void)
> +control_init(char *path)
>  {
>   struct sockaddr_un   sun;
>   int  fd;
> @@ -53,28 +53,28 @@ control_init(void)
>  
>   bzero(, sizeof(sun));
>   sun.sun_family = AF_UNIX;
> - strlcpy(sun.sun_path, RIPD_SOCKET, sizeof(sun.sun_path));
> + strlcpy(sun.sun_path, path, sizeof(sun.sun_path));
>  
> - if (unlink(RIPD_SOCKET) == -1)
> + if (unlink(path) == -1)
>   if (errno != ENOENT) {
> - log_warn("control_init: unlink %s", RIPD_SOCKET);
> + log_warn("control_init: unlink %s", path);
>   close(fd);
>   return (-1);
>   }
>  
>   old_umask = umask(S_IXUSR|S_IXGRP|S_IWOTH|S_IROTH|S_IXOTH);
>   if (bind(fd, (struct sockaddr *), sizeof(sun)) == -1) {
> - log_warn("control_init: bind: %s", RIPD_SOCKET);
> + log_warn("control_init: bind: %s", path);
>   close(fd);
>   umask(old_umask);
>   return (-1);
>   }
>   umask(old_umask);
>  
> - if (chmod(RIPD_SOCKET, S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP) == -1) {
> + if (chmod(path, S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP) == -1) {
>   log_warn("control_init: chmod");
>   close(fd);
> - (void)unlink(RIPD_SOCKET);
> + (void)unlink(path);
>   return (-1);
>   }
>  
> @@ -101,11 +101,11 @@ control_listen(void)
>  }
>  
>  void
> -control_cleanup(void)
> +control_cleanup(char *path)
>  {
>   event_del(_state.ev);
>   event_del(_state.evt);
> - unlink(RIPD_SOCKET);
> + unlink(path);
>  }
>  
>  /* ARGSUSED */
> Index: usr.sbin/ripd/control.h
> ===
> RCS file: /cvs/src/usr.sbin/ripd/control.h,v
> retrieving revision 1.4
> diff -u -p -r1.4 control.h
> --- usr.sbin/ripd/control.h   9 Feb 2015 12:13:42 -   1.4
> +++ usr.sbin/ripd/control.h   2 Aug 2016 11:34:44 -
> @@ -34,11 +34,11 @@ struct ctl_conn {
>   struct imsgev   iev;
>  };
>  
> -int  control_init(void);
> +int  control_init(char *);
>  int  control_listen(void);
>  void control_accept(int, short, void *);
>  void control_dispatch_imsg(int, short, void *);
>  int  control_imsg_relay(struct imsg *);
> -void control_cleanup(void);
> +void control_cleanup(char *);
>  
>  #endif   /* _CONTROL_H_ */
> Index: usr.sbin/ripd/ripd.c
> ===
> RCS file: /cvs/src/usr.sbin/ripd/ripd.c,v
> retrieving revision 1.27
> diff -u -p -r1.27 ripd.c
> --- usr.sbin/ripd/ripd.c  2 Feb 2016 17:51:11 -   1.27
> +++ usr.sbin/ripd/ripd.c  2 Aug 2016 11:44:24 -
> @@ -70,7 +70,8 @@ usage(void)
>  {
>   extern char *__progname;
>  
> - fprintf(stderr, "usage: %s [-dnv] [-D macro=value] [-f file]\n",
> + fprintf(stderr,
> + "usage: %s [-dnv] [-D macro=value] [-f file] [-s socket]\n",
>   __progname);
>   exit(1);
>  }
> @@ -122,15 +123,17 @@ main(int argc, char *argv[])
>   int  ch;
>   int  opts = 0;
>   char*conffile;
> + char*sockname;
>   size_t   len;
>  
>   conffile = CONF_FILE;
>   ripd_process = PROC_MAIN;
> + sockname = RIPD_SOCKET;
>  
>   log_init(1);/* log to stderr until daemonized */
>   log_verbose(1);
>  
> - while ((ch = getopt(argc, argv, "cdD:f:nv")) != -1) {
> + while ((ch = getopt(argc, argv, "cdD:f:ns:v")) != -1) {
>   switch (ch) {
>   case 'c':
>   opts |= RIPD_OPT_FORCE_DEMOTE;
> @@ -149,6 +152,9 @@ main(int argc, char *argv[])
>   case 'n':
>   opts |= RIPD_OPT_NOACTION;
> 

fix usermod -Z / -S

[Sebastian Benoit]

I am locking an account with

  # doas usermod -Z foobar

after that, i want to remove the user from all groups:

  # doas usermod -S '' foobar
  usermod: Invalid password: 
`*$2b$09$Pp.mDUEORDRbCUUy4D.Vf.EhvxVA.B1u0T7VAlsKN7sU7wqhs0l3W'

This happens in -current and is caused by usr.sbin/user/user.c as of

  /* $OpenBSD: user.c,v 1.111 2016/05/03 21:05:14 mestre Exp $ */

which was done to fix another bug.

The problem with that change is this change

+   if (up != NULL) {
+   if ((*pwp->pw_passwd != '\0') && (up->u_flags &~ F_PASSWORD)) {
+   up->u_flags |= F_PASSWORD;

which sets F_PASSWORD and causes this bit to check the password

if (up->u_flags & F_PASSWORD) {
if (up->u_password != NULL) {
if (!valid_password_length(up->u_password)) {
(void) close(ptmpfd);
pw_abort();
errx(EXIT_FAILURE, "Invalid password: 
`%s'",
up->u_password);
}
pwp->pw_passwd = up->u_password;
}
}


Of course this will not only affect -S, but all other changes that are done
on an account with an invalid password.

from usermod(8):

 -Z  Lock the account by appending a `-' to the user's shell and
 prefixing the password with `*'.  -Z and -U are mutually
 exclusive and cannot be used with -p.

and

 -S secondary-group[,group,...]
 Sets the secondary groups the user will be a member of in the
 /etc/group file.  Setting secondary-group to an empty value
 (e.g. '') removes the user from all secondary groups.

This fix uses a new flag for the check and does not verify if the password
is valid in that case.

ok?

diff --git usr.sbin/user/user.c usr.sbin/user/user.c
index 27344a7..c4c33d6 100644
--- usr.sbin/user/user.c
+++ usr.sbin/user/user.c
@@ -103,7 +103,8 @@ enum {
F_CLASS = 0x1000,
F_SETSECGROUP   = 0x4000,
F_ACCTLOCK  = 0x8000,
-   F_ACCTUNLOCK= 0x1
+   F_ACCTUNLOCK= 0x1,
+   F_KEEPPASSWORD  = 0x2
 };
 
 #define CONFFILE   "/etc/usermgmt.conf"
@@ -1410,7 +1411,7 @@ moduser(char *login_name, char *newlogin, user_t *up)
}
if (up != NULL) {
if ((*pwp->pw_passwd != '\0') && (up->u_flags &~ F_PASSWORD)) {
-   up->u_flags |= F_PASSWORD;
+   up->u_flags |= F_KEEPPASSWORD;
memsave(>u_password, pwp->pw_passwd,
strlen(pwp->pw_passwd));
memset(pwp->pw_passwd, 'X', strlen(pwp->pw_passwd));
@@ -1486,6 +1487,9 @@ moduser(char *login_name, char *newlogin, user_t *up)
pwp->pw_passwd = up->u_password;
}
}
+   if (up->u_flags & F_KEEPPASSWORD) {
+   pwp->pw_passwd = up->u_password;
+   }
if (up->u_flags & F_ACCTLOCK) {
/* lock the account */
if (*shell_last_char != *acctlock_str) {