Re: ifconfig.8 doco for vnetid and parent options

[David Gwynne]

> On 6 Jun 2017, at 18:12, Jason McIntyre  wrote:
> 
> On Tue, Jun 06, 2017 at 11:56:28AM +1000, David Gwynne wrote:
>> this adds doco for the parent options in ifconfig, and moves vnetid
>> up into generic options list.
>> 
>> the vlan(4) specific doco has enough lies and omissions that id
>> rather get rid of it.
>> 
>> ok?
>> 
> 
> morning.
> 
> you are breaking the format of this page if you start to move specific
> subsytems into the main body.
> 
> that makes things less clear. for example, can i use the parent-device
> stuff on any type of interface or just vlan? if it's vlan related, how
> can i tell that?

the "parent" ioctls are generic, but it is only implemented in vlan currently. 
i have a plan to replace "carpdev", "syncdev", and "pppoedev" with it, but id 
like to finish the vlan stuff before going further.

vnetid is used by both vlan and vxlan at the moment. i have a diff to add 
support for it to gre, while adding egre (ethernet over gre) and nvgre 
interfaces that also support a network identifier.

> 
> the text reads fine, but i'd rather you tried to integrate it into the
> vlan section. if there are errors in there let's remove them.
> 
> but if it's the case that none of this stuff is specific to vlan, then i
> guess it makes sense to do it your way (but then you'd have to take care
> when documenting stuff like parent-device to say in what situations it
> makes sense).

im happy to take direction here. i could also move things like the ethernet 
options (arp, -arp, instance, wol, -wol) into their own section.

how would you say that vnetid is supported by both vlan and some tunnel 
interfaces? would you put it in the tunnel section, the vlan section, or both, 
or a new section?

dlg


> 
> jmc
> 
>> Index: ifconfig.8
>> ===
>> RCS file: /cvs/src/sbin/ifconfig/ifconfig.8,v
>> retrieving revision 1.282
>> diff -u -p -r1.282 ifconfig.8
>> --- ifconfig.8   12 May 2017 15:11:02 -  1.282
>> +++ ifconfig.8   6 Jun 2017 01:54:10 -
>> @@ -415,6 +415,14 @@ and 0's for the host part.
>> The mask should contain at least the standard network portion,
>> and the subnet field should be contiguous with the network
>> portion.
>> +.It Cm parent Ar parent-interface
>> +Associate with interface
>> +.Ar parent-interface .
>> +Packets transmitted through the interface will be encapsulated and
>> +diverted to the specified parent interface
>> +.It Cm -parent
>> +Disassociate from the parent interface.
>> +This breaks the link between the interface and its parent.
>> .It Cm prefixlen Ar n
>> (inet and inet6 only)
>> Effect is similar to
>> @@ -461,6 +469,20 @@ This may be used to enable an interface 
>> It happens automatically when setting the first address on an interface.
>> If the interface was reset when previously marked down,
>> the hardware will be re-initialized.
>> +.It Cm vnetid Ar network-id Ns | Ns Cm any
>> +Set the virtual network identifier.
>> +This is a number which is used by encapsulation interfaces such as
>> +.Xr vlan 4
>> +or
>> +.Xr vxlan 4
>> +to identify packets with a virtual network.
>> +If supported by the interface,
>> +the value can also be set to
>> +.Ar any
>> +to accept packets with arbitrary network identifiers (for example for
>> +multipoint-to-multipoint modes).
>> +.It Cm -vnetid
>> +Clear the virtual network identifier.
>> .It Cm wol
>> Enable Wake on LAN (WoL).
>> When enabled, reception of a WoL frame will cause the network card to
>> @@ -1531,7 +1553,6 @@ for a complete list of the available pro
>> .Op Oo Fl Oc Ns Cm keepalive Ar period count
>> .Op Cm tunnel Ar src_address dest_address
>> .Op Cm tunneldomain Ar tableid
>> -.Op Oo Fl Oc Ns Cm vnetid Ar network-id
>> .Ek
>> .nr nS 0
>> .Pp
>> @@ -1583,21 +1604,6 @@ can be set to any valid routing table ID
>> the corresponding routing domain is derived from this table.
>> .It Cm tunnelttl Ar ttl
>> Set the IP or multicast TTL of the tunnel packets.
>> -.It Cm vnetid Ar network-id
>> -Set the virtual network identifier.
>> -This is a number which is used by tunnel protocols such as
>> -.Xr vxlan 4
>> -to identify packets with a virtual network.
>> -The accepted size of the number depends on the individual tunnel protocol;
>> -it is a 24-bit number for
>> -.Xr vxlan 4 .
>> -If supported by the tunnel protocol,
>> -the value can also be set to
>> -.Ar any
>> -to accept packets with arbitrary network identifiers (for example for
>> -multipoint-to-multipoint modes).
>> -.It Cm -vnetid
>> -Clear the virtual network identifier.
>> .El
>> .Sh UMB
>> .nr nS 1
>> @@ -1660,52 +1666,6 @@ Disable data roaming.
>> As soon as the interface is marked as "up", the
>> .Xr umb 4
>> device will try to establish a data connection with the service provider.
>> -.El
>> -.Sh VLAN
>> -.nr nS 1
>> -.Bk -words
>> -.Nm ifconfig
>> -.Ar vlan-interface
>> -.Op Cm vlan Ar vlan-tag
>> -.Op Oo Fl Oc Ns Cm vlandev Ar parent-interface
>> -.Ek
>> 

Re: htpasswd: use crypt_newhash instead of bcrypt API

[Theo de Raadt]

> On 20:49:05,  6.06.17, Jason McIntyre wrote:
> > right now this man page suggests that people will use "bcrypt,a"
> > to "automatically suggest rounds based on system performance". is
> > that right? i'd have expected people to just use "bcrypt" (w/o
> > args).

Because you can't change everything in one step.  Seriously, feel free
to pull on this string, and test all the cases.

> > in fact, why have "a" at all? why not just have the default
> > as automatically selecting rounds, but you can optionally specify
> > an amount of rounds?

Waiting for a volunteer who understands the impact.

> > sorry, i know that's not really the main concern of your diff. it just
> > seems a bit weird, and that reflects in the way you're having to word
> > this.
> 
> Yes, the function seems a bit inconsistent, in that "bcrypt" means "bcrypt,a"
> but NULL means "bcrypt,8". awolk@ points out that the function is used in
> just a few places - src and some ports patches, so we should be able to
> change it. Judging by the commit message the author was aware of this
> discrepancy though, and marked is as "for now", so let's see what others say.

Yes, let's change everything at once.

Did you test what you propose?  Your mail seems to be coming in before
you could have changed it, done a full build, and a study.  Am I correct?

Re: htpasswd: use crypt_newhash instead of bcrypt API

[Florian Obser]

On Tue, Jun 06, 2017 at 08:49:32PM +0200, Adam Wolk wrote:
> On Tue, Jun 06, 2017 at 12:28:59PM -0600, Theo de Raadt wrote:
> > > The only thing against using automatic rounds would be having them 
> > > guessed on a
> > > weaker machine and used on a more powerful server - doubt though that 
> > > would ever
> > > pick something below 8 rounds.
> > 
> > I don't see the concern.  It has a lower bound.
> 
> Attaching the diff with rounds changed to auto, results with 9 rounds on my 
> server.
> 

OK florian@

> ? htpasswd
> Index: htpasswd.c
> ===
> RCS file: /cvs/src/usr.bin/htpasswd/htpasswd.c,v
> retrieving revision 1.15
> diff -u -p -r1.15 htpasswd.c
> --- htpasswd.c5 Nov 2015 20:07:15 -   1.15
> +++ htpasswd.c6 Jun 2017 18:46:39 -
> @@ -47,7 +47,7 @@ int nagcount;
>  int
>  main(int argc, char** argv)
>  {
> - char salt[_PASSWORD_LEN], tmpl[sizeof("/tmp/htpasswd-XX")];
> + char tmpl[sizeof("/tmp/htpasswd-XX")];
>   char hash[_PASSWORD_LEN], pass[1024], pass2[1024];
>   char *line = NULL, *login = NULL, *tok;
>   int c, fd, loginlen, batch = 0;
> @@ -133,10 +133,8 @@ main(int argc, char** argv)
>   explicit_bzero(pass2, sizeof(pass2));
>   }
>  
> - if (strlcpy(salt, bcrypt_gensalt(8), sizeof(salt)) >= sizeof(salt))
> - errx(1, "salt too long");
> - if (strlcpy(hash, bcrypt(pass, salt), sizeof(hash)) >= sizeof(hash))
> - errx(1, "hash too long");
> + if (crypt_newhash(pass, "bcrypt,a", hash, sizeof(hash)) != 0)
> + err(1, "can't generate hash");
>   explicit_bzero(pass, sizeof(pass));
>  
>   if (file == NULL)


-- 
I'm not entirely sure you are real.

Re: htpasswd: use crypt_newhash instead of bcrypt API

[Michal Mazurek]

On 20:49:05,  6.06.17, Jason McIntyre wrote:
> right now this man page suggests that people will use "bcrypt,a"
> to "automatically suggest rounds based on system performance". is
> that right? i'd have expected people to just use "bcrypt" (w/o
> args). in fact, why have "a" at all? why not just have the default
> as automatically selecting rounds, but you can optionally specify
> an amount of rounds?
> 
> sorry, i know that's not really the main concern of your diff. it just
> seems a bit weird, and that reflects in the way you're having to word
> this.

Yes, the function seems a bit inconsistent, in that "bcrypt" means "bcrypt,a"
but NULL means "bcrypt,8". awolk@ points out that the function is used in
just a few places - src and some ports patches, so we should be able to
change it. Judging by the commit message the author was aware of this
discrepancy though, and marked is as "for now", so let's see what others say.

-- 
Michal Mazurek

Re: htpasswd: use crypt_newhash instead of bcrypt API

[Jason McIntyre]

On Tue, Jun 06, 2017 at 09:16:08PM +0200, Michal Mazurek wrote:
> When talking about this with mulander@ it came out that the docs could
> use a touch.
> 
> The commit message for the diff that didn't update the docs was:
> 
> permit "bcrypt" as an alias for "blowfish". this is, after all, what
> 99% of the world calls it.
> allow just "bcrypt" without params to mean auto-tune ("bcrypt,a").
> default remains 8 rounds (for now)
> 
> Comments? OK?
> 

the diff itself reads fine. one question:

> Index: lib/libc/crypt/crypt_checkpass.3
> ===
> RCS file: /cvs/src/lib/libc/crypt/crypt_checkpass.3,v
> retrieving revision 1.9
> diff -u -p -r1.9 crypt_checkpass.3
> --- lib/libc/crypt/crypt_checkpass.3  23 Jul 2015 22:20:02 -  1.9
> +++ lib/libc/crypt/crypt_checkpass.3  6 Jun 2017 19:06:59 -
> @@ -58,17 +58,29 @@ The provided
>  .Fa password
>  is randomly salted and hashed and stored in
>  .Fa hash .
> +.Fa hash
> +must already be allocated, and
> +.Fa hashsize
> +must contain its size, which cannot be less than 61 bytes.
>  The
>  .Fa pref
>  argument identifies the preferred hashing algorithm and parameters.
> +If set to
> +.Dv NULL
> +it defaults to 
> +.Dq bcrypt,8 .
>  Possible values are:
>  .Bl -tag -width Ds
> -.It Dq bcrypt,
> +.It Dq bcrypt[,]
>  The bcrypt algorithm, where the value of rounds can be between 4 and 31 and
>  specifies the base 2 logarithm of the number of rounds.
>  The special rounds value
>  .Sq a
>  automatically selects rounds based on system performance.
> +This is the default if rounds is omitted.

right now this man page suggests that people will use "bcrypt,a"
to "automatically suggest rounds based on system performance". is
that right? i'd have expected people to just use "bcrypt" (w/o
args). in fact, why have "a" at all? why not just have the default
as automatically selecting rounds, but you can optionally specify
an amount of rounds?

sorry, i know that's not really the main concern of your diff. it just
seems a bit weird, and that reflects in the way you're having to word
this.

jmc

> +.Dq blowfish
> +can be used as an alias for
> +.Dq bcrypt .
>  .El
>  .Sh RETURN VALUES
>  .Rv -std crypt_checkpass crypt_newhash
> @@ -89,7 +101,9 @@ to
>  .Er EINVAL
>  if
>  .Fa pref
> -is unsupported.
> +is unsupported, or the value of 
> +.Fa hashsize
> +is insufficient.
>  .Sh SEE ALSO
>  .Xr crypt 3 ,
>  .Xr login.conf 5 ,
> 
> -- 
> Michal Mazurek
> 

Re: htpasswd: use crypt_newhash instead of bcrypt API

[Michal Mazurek]

When talking about this with mulander@ it came out that the docs could
use a touch.

The commit message for the diff that didn't update the docs was:

permit "bcrypt" as an alias for "blowfish". this is, after all, what
99% of the world calls it.
allow just "bcrypt" without params to mean auto-tune ("bcrypt,a").
default remains 8 rounds (for now)

Comments? OK?

Index: lib/libc/crypt/crypt_checkpass.3
===
RCS file: /cvs/src/lib/libc/crypt/crypt_checkpass.3,v
retrieving revision 1.9
diff -u -p -r1.9 crypt_checkpass.3
--- lib/libc/crypt/crypt_checkpass.323 Jul 2015 22:20:02 -  1.9
+++ lib/libc/crypt/crypt_checkpass.36 Jun 2017 19:06:59 -
@@ -58,17 +58,29 @@ The provided
 .Fa password
 is randomly salted and hashed and stored in
 .Fa hash .
+.Fa hash
+must already be allocated, and
+.Fa hashsize
+must contain its size, which cannot be less than 61 bytes.
 The
 .Fa pref
 argument identifies the preferred hashing algorithm and parameters.
+If set to
+.Dv NULL
+it defaults to 
+.Dq bcrypt,8 .
 Possible values are:
 .Bl -tag -width Ds
-.It Dq bcrypt,
+.It Dq bcrypt[,]
 The bcrypt algorithm, where the value of rounds can be between 4 and 31 and
 specifies the base 2 logarithm of the number of rounds.
 The special rounds value
 .Sq a
 automatically selects rounds based on system performance.
+This is the default if rounds is omitted.
+.Dq blowfish
+can be used as an alias for
+.Dq bcrypt .
 .El
 .Sh RETURN VALUES
 .Rv -std crypt_checkpass crypt_newhash
@@ -89,7 +101,9 @@ to
 .Er EINVAL
 if
 .Fa pref
-is unsupported.
+is unsupported, or the value of 
+.Fa hashsize
+is insufficient.
 .Sh SEE ALSO
 .Xr crypt 3 ,
 .Xr login.conf 5 ,

-- 
Michal Mazurek

g/c ASPICFLAG

[Miod Vallat]

This used to be necessary in the gcc 2.95 days. Nowadays nothing in base
or X uses it.

Index: share/mk/bsd.own.mk
===
RCS file: /OpenBSD/src/share/mk/bsd.own.mk,v
retrieving revision 1.184
diff -u -p -r1.184 bsd.own.mk
--- share/mk/bsd.own.mk 18 Apr 2017 14:03:08 -  1.184
+++ share/mk/bsd.own.mk 6 Jun 2017 19:12:04 -
@@ -129,10 +129,6 @@ PICFLAG?=-fPIC
 PICFLAG?=-fpic
 .endif
 
-.if ${MACHINE_ARCH} == "sparc64"
-ASPICFLAG=-KPIC
-.endif
-
 .if ${MACHINE_ARCH} == "alpha" || ${MACHINE_ARCH} == "powerpc" || \
 ${MACHINE_ARCH} == "sparc64"
 # big PIE

Re: htpasswd: use crypt_newhash instead of bcrypt API

[Adam Wolk]

On Tue, Jun 06, 2017 at 12:28:59PM -0600, Theo de Raadt wrote:
> > The only thing against using automatic rounds would be having them guessed 
> > on a
> > weaker machine and used on a more powerful server - doubt though that would 
> > ever
> > pick something below 8 rounds.
> 
> I don't see the concern.  It has a lower bound.

Attaching the diff with rounds changed to auto, results with 9 rounds on my 
server.

? htpasswd
Index: htpasswd.c
===
RCS file: /cvs/src/usr.bin/htpasswd/htpasswd.c,v
retrieving revision 1.15
diff -u -p -r1.15 htpasswd.c
--- htpasswd.c  5 Nov 2015 20:07:15 -   1.15
+++ htpasswd.c  6 Jun 2017 18:46:39 -
@@ -47,7 +47,7 @@ int nagcount;
 int
 main(int argc, char** argv)
 {
-   char salt[_PASSWORD_LEN], tmpl[sizeof("/tmp/htpasswd-XX")];
+   char tmpl[sizeof("/tmp/htpasswd-XX")];
char hash[_PASSWORD_LEN], pass[1024], pass2[1024];
char *line = NULL, *login = NULL, *tok;
int c, fd, loginlen, batch = 0;
@@ -133,10 +133,8 @@ main(int argc, char** argv)
explicit_bzero(pass2, sizeof(pass2));
}
 
-   if (strlcpy(salt, bcrypt_gensalt(8), sizeof(salt)) >= sizeof(salt))
-   errx(1, "salt too long");
-   if (strlcpy(hash, bcrypt(pass, salt), sizeof(hash)) >= sizeof(hash))
-   errx(1, "hash too long");
+   if (crypt_newhash(pass, "bcrypt,a", hash, sizeof(hash)) != 0)
+   err(1, "can't generate hash");
explicit_bzero(pass, sizeof(pass));
 
if (file == NULL)

Re: Makefile.cross tweaks

[Miod Vallat]

>> The following diff attempts to cross-build more things, in particular
>> gnu/lib (except for libiberty). It also passes the proper optimization
>> flags so that libstdc++-v3 gets built with optimization.
>
> Doesn't build for arm64, probably because BUILD_GCC4 is transparently
> set by the host and not reset to NO.

Damn. This diff ought to work better.

Index: Makefile.cross
===
RCS file: /OpenBSD/src/Makefile.cross,v
retrieving revision 1.94
diff -u -p -r1.94 Makefile.cross
--- Makefile.cross  23 May 2017 14:57:30 -  1.94
+++ Makefile.cross  6 Jun 2017 18:15:11 -
@@ -43,9 +43,15 @@ MACHINE_IS_LP64 =
 
 #CROSSCPPFLAGS?=   -nostdinc -I${CROSSDIR}/usr/include
 #CROSSLDFLAGS?=-nostdlib -L${CROSSDIR}/usr/lib -static
+DEBUG?=-g
 CROSSCFLAGS?=  ${CROSSCPPFLAGS} -O2 ${PIPE} ${DEBUG}
-CROSSCXXFLAGS?=${CROSSCPPFLAGS}
+CROSSCXXFLAGS?=${CROSSCPPFLAGS} -O2 ${PIPE} ${DEBUG}
 #LDSTATIC?=-static
+.if (${TARGET_ARCH} == "alpha") || (${TARGET_ARCH} == "sparc64")
+CROSSPICFLAG?=-fPIC
+.else
+CROSSPICFLAG?=-fpic
+.endif
 
 CROSSDIR=  ${DESTDIR}/usr/cross/${TARGET}
 CROSSENV=  AR=${CROSSDIR}/usr/${TARGET_CANON}/bin/ar \
@@ -63,6 +69,7 @@ CROSSENV= AR=${CROSSDIR}/usr/${TARGET_CA
CFLAGS=\"${CROSSCFLAGS}\" CPPFLAGS=\"${CROSSCPPFLAGS}\" \
CXXFLAGS=\"${CROSSCXXFLAGS}\" \
LDFLAGS=\"${CROSSLDFLAGS}\" \
+   PICFLAG=\"${CROSSPICFLAG}\" \
CROSSDIR=\"${CROSSDIR}\"
 CROSSADDPATH=:${CROSSDIR}/usr/${TARGET_CANON}/bin
 CROSSPATH= /usr/bin:/bin:/usr/sbin:/sbin${CROSSADDPATH}
@@ -75,18 +82,31 @@ CROSSOBJ=   ${CROSSDIR}/.obj_done
 CROSSINCLUDES= ${CROSSDIR}/.includes_done
 CROSSBINUTILS= ${CROSSDIR}/.binutils_done
 CROSSGCC=  ${CROSSDIR}/.gcc_done
-#NO_CROSS= isakmpd keynote ssh
+NO_CROSS=  libiberty
 
 .include 
 
+BUILD_CLANG=no
+BUILD_GCC3=no
+BUILD_GCC4=no
+
 .for _arch in ${TARGET_ARCH}
-.if !empty(CLANG_ARCH:M${_arch})
-COMPILER_VERSION=clang
-BUILD_CLANG=yes
-.elif !empty(GCC3_ARCH:M${_arch})
+.if !empty(GCC3_ARCH:M${_arch})
 COMPILER_VERSION=gcc3
-.else
+.elif !empty(GCC4_ARCH:M${_arch})
 COMPILER_VERSION=gcc4
+.else
+COMPILER_VERSION=clang
+.endif
+
+.if !empty(CLANG_ARCH:M${_arch})
+BUILD_CLANG=yes
+.endif
+.if !empty(GCC3_ARCH:M${_arch})
+BUILD_GCC3=yes
+.endif
+.if !empty(GCC4_ARCH:M${_arch})
+BUILD_GCC4=yes
 .endif
 
 .if !empty(PIE_ARCH:M${_arch})
@@ -101,6 +121,7 @@ PIE_DEFAULT=
 BINUTILS=  ar as gasp ld nm objcopy objdump ranlib readelf size \
strings strip
 BINUTILS_DIR=gnu/usr.bin/binutils-2.17
+
 .endfor
 
 # no libcrypto these won't build
@@ -113,6 +134,13 @@ NO_CROSS+=nsd
 NO_CROSS+=bind
 NO_CROSS+=unbound
 
+.if ${BUILD_GCC3:L} != "yes"
+NO_CROSS+=libobjc libstdc++
+.endif
+.if ${BUILD_GCC4:L} != "yes"
+NO_CROSS+=../usr.bin/cc/libobjc libstdc++-v3 libsupc++-v3
+.endif
+
 cross-dirs:${CROSSDIRS}
 cross-obj: ${CROSSOBJ}
 cross-includes:${CROSSINCLUDES}
@@ -172,12 +200,18 @@ ${CROSSINCLUDES}: ${CROSSOBJ}
MACHINE_ARCH=${TARGET_ARCH} MACHINE_CPU=${TARGET_CPU} \
MAKEOBJDIR=obj.${MACHINE}.${TARGET} \
TARGET_ARCH=${TARGET_ARCH} TARGET_CPU=${TARGET_CPU} \
+   BUILD_GCC3=${BUILD_GCC3} \
+   BUILD_GCC4=${BUILD_GCC4} \
+   BUILD_CLANG=${BUILD_CLANG} \
${MAKE} prereq && \
COMPILER_VERSION=${COMPILER_VERSION} \
MACHINE=${TARGET} \
MACHINE_ARCH=${TARGET_ARCH} MACHINE_CPU=${TARGET_CPU} \
MAKEOBJDIR=obj.${MACHINE}.${TARGET} \
TARGET_ARCH=${TARGET_ARCH} TARGET_CPU=${TARGET_CPU} \
+   BUILD_GCC3=${BUILD_GCC3} \
+   BUILD_GCC4=${BUILD_GCC4} \
+   BUILD_CLANG=${BUILD_CLANG} \
${MAKE} DESTDIR=${CROSSDIR} includes)
@touch ${CROSSINCLUDES}
 
@@ -222,7 +256,7 @@ ${CROSSBINUTILS}:   ${CROSSINCLUDES}
 
 
 ${CROSSGCC}:   ${CROSSBINUTILS}
-.if ${COMPILER_VERSION:L} == "clang"
+.if ${BUILD_CLANG:L} == "yes"
(cd ${.CURDIR}/gnu/usr.bin/clang; \
MAKEOBJDIR=obj.${MACHINE}.${TARGET} \
MACHINE_ARCH=${TARGET_ARCH} \
@@ -253,7 +287,8 @@ ${CROSSGCC}:${CROSSBINUTILS}
chmod +x ${CROSSDIR}/usr/${TARGET_CANON}/bin/${TARGET_CANON}-cc;
echo 
"#!/bin/sh\n${CROSSDIR}/usr/${TARGET_CANON}/bin/${TARGET_CANON}-clang 
--driver-mode=g++ --sysroot ${CROSSDIR} \"$$""@\"" > 
${CROSSDIR}/usr/${TARGET_CANON}/bin/${TARGET_CANON}-c++; \
chmod +x ${CROSSDIR}/usr/${TARGET_CANON}/bin/${TARGET_CANON}-c++;
-.elif ${COMPILER_VERSION:L} == "gcc3"
+.endif
+.if ${BUILD_GCC3:L} == "yes"
(cd ${.CURDIR}/gnu/usr.bin/gcc; \
MAKEOBJDIR=obj.${MACHINE}.${TARGET} \
TARGET_ARCH=${TARGET_ARCH} TARGET_CPU=${TARGET_CPU} \
@@ -280,21 +315,22 @@ ${CROSSGCC}:  ${CROSSBINUTILS}
chmod ${BINMODE} 

Re: htpasswd: use crypt_newhash instead of bcrypt API

[Theo de Raadt]

> The only thing against using automatic rounds would be having them guessed on 
> a
> weaker machine and used on a more powerful server - doubt though that would 
> ever
> pick something below 8 rounds.

I don't see the concern.  It has a lower bound.

Re: htpasswd: use crypt_newhash instead of bcrypt API

[Adam Wolk]

On Tue, Jun 06, 2017 at 02:20:38PM -0400, Bryan Steele wrote:
> >  
> > -   if (strlcpy(salt, bcrypt_gensalt(8), sizeof(salt)) >= sizeof(salt))
> > -   errx(1, "salt too long");
> > -   if (strlcpy(hash, bcrypt(pass, salt), sizeof(hash)) >= sizeof(hash))
> > -   errx(1, "hash too long");
> > +   if (crypt_newhash(pass, "bcrypt,8", hash, sizeof(hash)) != 0)
> > +   err(1, "can't generate hash");
> > explicit_bzero(pass, sizeof(pass));
> >  
> > if (file == NULL)
> 
> It should be possible to use the automatic rouds, i.e: "bcrypt,a" instead
> of just hardcoding 8.
> 

I wasn't sure about introducing that change, went the minimal changes from
existing behavior.

The only thing against using automatic rounds would be having them guessed on a
weaker machine and used on a more powerful server - doubt though that would ever
pick something below 8 rounds.

Roughly, if the general consensus is to move to automatic rounds then I'm all
for it.

Side note, does anyone know why crypt_newhash defaults to blowfish? The
docs don't mention it.

Re: htpasswd: use crypt_newhash instead of bcrypt API

[Bryan Steele]

On Tue, Jun 06, 2017 at 07:43:02PM +0200, Adam Wolk wrote:
> Hi tech@
> 
> While reading htpasswd and htpasswd handling in httpd I noticed that both use
> different APIs to handle encrypting/decrypting the passwords.
> 
> - htpasswd uses the bcrypt API
> - httpd uses the new crypt API
> 
> The documentation for bcrypt states:
>  These functions are deprecated in favor of crypt_checkpass(3) and
>  crypt_newhash(3).
> 
> I'm attaching a diff moving htpasswd to the new API. Tested with httpd from
> 6.1 with a htpasswd generated with the diff applied on current.
> 
> Feedback? OK's?
> 
> Regards,
> Adam

> ? htpasswd
> Index: htpasswd.c
> ===
> RCS file: /cvs/src/usr.bin/htpasswd/htpasswd.c,v
> retrieving revision 1.15
> diff -u -p -r1.15 htpasswd.c
> --- htpasswd.c5 Nov 2015 20:07:15 -   1.15
> +++ htpasswd.c6 Jun 2017 17:26:31 -
> @@ -47,7 +47,7 @@ int nagcount;
>  int
>  main(int argc, char** argv)
>  {
> - char salt[_PASSWORD_LEN], tmpl[sizeof("/tmp/htpasswd-XX")];
> + char tmpl[sizeof("/tmp/htpasswd-XX")];
>   char hash[_PASSWORD_LEN], pass[1024], pass2[1024];
>   char *line = NULL, *login = NULL, *tok;
>   int c, fd, loginlen, batch = 0;
> @@ -133,10 +133,8 @@ main(int argc, char** argv)
>   explicit_bzero(pass2, sizeof(pass2));
>   }
>  
> - if (strlcpy(salt, bcrypt_gensalt(8), sizeof(salt)) >= sizeof(salt))
> - errx(1, "salt too long");
> - if (strlcpy(hash, bcrypt(pass, salt), sizeof(hash)) >= sizeof(hash))
> - errx(1, "hash too long");
> + if (crypt_newhash(pass, "bcrypt,8", hash, sizeof(hash)) != 0)
> + err(1, "can't generate hash");
>   explicit_bzero(pass, sizeof(pass));
>  
>   if (file == NULL)

It should be possible to use the automatic rouds, i.e: "bcrypt,a" instead
of just hardcoding 8.

htpasswd: use crypt_newhash instead of bcrypt API

[Adam Wolk]

Hi tech@

While reading htpasswd and htpasswd handling in httpd I noticed that both use
different APIs to handle encrypting/decrypting the passwords.

- htpasswd uses the bcrypt API
- httpd uses the new crypt API

The documentation for bcrypt states:
 These functions are deprecated in favor of crypt_checkpass(3) and
 crypt_newhash(3).

I'm attaching a diff moving htpasswd to the new API. Tested with httpd from
6.1 with a htpasswd generated with the diff applied on current.

Feedback? OK's?

Regards,
Adam
? htpasswd
Index: htpasswd.c
===
RCS file: /cvs/src/usr.bin/htpasswd/htpasswd.c,v
retrieving revision 1.15
diff -u -p -r1.15 htpasswd.c
--- htpasswd.c  5 Nov 2015 20:07:15 -   1.15
+++ htpasswd.c  6 Jun 2017 17:26:31 -
@@ -47,7 +47,7 @@ int nagcount;
 int
 main(int argc, char** argv)
 {
-   char salt[_PASSWORD_LEN], tmpl[sizeof("/tmp/htpasswd-XX")];
+   char tmpl[sizeof("/tmp/htpasswd-XX")];
char hash[_PASSWORD_LEN], pass[1024], pass2[1024];
char *line = NULL, *login = NULL, *tok;
int c, fd, loginlen, batch = 0;
@@ -133,10 +133,8 @@ main(int argc, char** argv)
explicit_bzero(pass2, sizeof(pass2));
}
 
-   if (strlcpy(salt, bcrypt_gensalt(8), sizeof(salt)) >= sizeof(salt))
-   errx(1, "salt too long");
-   if (strlcpy(hash, bcrypt(pass, salt), sizeof(hash)) >= sizeof(hash))
-   errx(1, "hash too long");
+   if (crypt_newhash(pass, "bcrypt,8", hash, sizeof(hash)) != 0)
+   err(1, "can't generate hash");
explicit_bzero(pass, sizeof(pass));
 
if (file == NULL)

Re: soassertlocked() & protocol families

[Claudio Jeker]

On Tue, Jun 06, 2017 at 05:37:46PM +0200, Martin Pieuchot wrote:
> Our plan is to work on the socket layer protocol per protocol.  claudio@
> already sent some diffs to make the iteration on the PF_ROUTE socket
> list MP-safe.  However that isn't enough for the inner par of the loop.
> 
> Since socket flag access and socket buffer modifications need to be
> atomic until somebody proves the contrary.  I'd like to assert that
> the KERNEL_LOCK() is held for pfkey/routing and local sockets.
> 
> The diff below only makes sense if socket buffer functions assert for
> the related "socket lock".
> 
> ok?
> 

Yes please. We can fiddle with the switch statement afterwards once we
know stuff is more save.

> Index: kern/uipc_socket2.c
> ===
> RCS file: /cvs/src/sys/kern/uipc_socket2.c,v
> retrieving revision 1.77
> diff -u -p -r1.77 uipc_socket2.c
> --- kern/uipc_socket2.c   27 May 2017 18:50:53 -  1.77
> +++ kern/uipc_socket2.c   6 Jun 2017 15:29:05 -
> @@ -292,10 +292,18 @@ sounlock(int s)
>  void
>  soassertlocked(struct socket *so)
>  {
> - if ((so->so_proto->pr_domain->dom_family != PF_LOCAL) &&
> - (so->so_proto->pr_domain->dom_family != PF_ROUTE) &&
> - (so->so_proto->pr_domain->dom_family != PF_KEY))
> + switch (so->so_proto->pr_domain->dom_family) {
> + case PF_INET:
> + case PF_INET6:
>   NET_ASSERT_LOCKED();
> + break;
> + case PF_LOCAL:
> + case PF_ROUTE:
> + case PF_KEY:
> + default:
> + KERNEL_ASSERT_LOCKED();
> + break;
> + }
>  }
>  
>  int
> 

-- 
:wq Claudio

Re: Towards tcp_input() w/o KERNEL_LOCK()

[Claudio Jeker]

On Tue, Jun 06, 2017 at 05:15:40PM +0200, Martin Pieuchot wrote:
> TCP/UDP are almost ready to run without KERNEL_LOCK() because accesses
> to their sockets are serialized via the NET_LOCK().  On the other hand
> pfkey and routing sockets accesses still rely on the KERNEL_LOCK().
> 
> Since we're going to work at the socket layer, first to remove the
> KERNEL_LOCK() from routing/pfkey sockets then to split the NET_LOCK(),
> we need some tooling to move faster and avoid mistakes.
> 
> Currently all operations on socket buffers are protected by these
> locks.  I'd like to assert that, at least for all functions used in
> TCP/UDP layers.
> 
> The idea is to later change the lock asserted in soassertlocked(). 
> 
> Comments, ok?

I agree with the concept. Doing this may help us to move more aggressivly
forward. OK from my side.
 
> Index: kern/uipc_socket.c
> ===
> RCS file: /cvs/src/sys/kern/uipc_socket.c,v
> retrieving revision 1.186
> diff -u -p -r1.186 uipc_socket.c
> --- kern/uipc_socket.c31 May 2017 08:55:10 -  1.186
> +++ kern/uipc_socket.c6 Jun 2017 14:49:14 -
> @@ -216,7 +216,7 @@ sofree(struct socket *so)
>   so->so_sp = NULL;
>   }
>  #endif /* SOCKET_SPLICE */
> - sbrelease(>so_snd);
> + sbrelease(so, >so_snd);
>   sorflush(so);
>   pool_put(_pool, so);
>  }
> @@ -440,7 +440,7 @@ restart:
>   } else if (addr == 0)
>   snderr(EDESTADDRREQ);
>   }
> - space = sbspace(>so_snd);
> + space = sbspace(so, >so_snd);
>   if (flags & MSG_OOB)
>   space += 1024;
>   if ((atomic && resid > so->so_snd.sb_hiwat) ||
> @@ -1058,7 +1058,9 @@ sorflush(struct socket *so)
>   }
>   if (pr->pr_flags & PR_RIGHTS && pr->pr_domain->dom_dispose)
>   (*pr->pr_domain->dom_dispose)(asb.sb_mb);
> - sbrelease();
> + *sb = asb;
> + sbrelease(so, sb);
> + memset(sb, 0, sizeof (*sb));
>  }
>  
>  #ifdef SOCKET_SPLICE
> @@ -1270,7 +1272,7 @@ somove(struct socket *so, int wait)
>   maxreached = 1;
>   }
>   }
> - space = sbspace(>so_snd);
> + space = sbspace(so, >so_snd);
>   if (so->so_oobmark && so->so_oobmark < len &&
>   so->so_oobmark < space + 1024)
>   space += 1024;
> @@ -1635,7 +1637,7 @@ sosetopt(struct socket *so, int level, i
>   goto bad;
>   }
>   if (sbcheckreserve(cnt, so->so_snd.sb_wat) ||
> - sbreserve(>so_snd, cnt)) {
> + sbreserve(so, >so_snd, cnt)) {
>   error = ENOBUFS;
>   goto bad;
>   }
> @@ -1648,7 +1650,7 @@ sosetopt(struct socket *so, int level, i
>   goto bad;
>   }
>   if (sbcheckreserve(cnt, so->so_rcv.sb_wat) ||
> - sbreserve(>so_rcv, cnt)) {
> + sbreserve(so, >so_rcv, cnt)) {
>   error = ENOBUFS;
>   goto bad;
>   }
> @@ -1991,7 +1993,7 @@ filt_sowrite(struct knote *kn, long hint
>  {
>   struct socket *so = kn->kn_fp->f_data;
>  
> - kn->kn_data = sbspace(>so_snd);
> + kn->kn_data = sbspace(so, >so_snd);
>   if (so->so_state & SS_CANTSENDMORE) {
>   kn->kn_flags |= EV_EOF;
>   kn->kn_fflags = so->so_error;
> Index: kern/uipc_usrreq.c
> ===
> RCS file: /cvs/src/sys/kern/uipc_usrreq.c,v
> retrieving revision 1.117
> diff -u -p -r1.117 uipc_usrreq.c
> --- kern/uipc_usrreq.c13 Mar 2017 20:18:21 -  1.117
> +++ kern/uipc_usrreq.c6 Jun 2017 14:24:35 -
> @@ -222,7 +222,7 @@ uipc_usrreq(struct socket *so, int req, 
>   from = mtod(unp->unp_addr, struct sockaddr *);
>   else
>   from = _noname;
> - if (sbappendaddr(>so_rcv, from, m, control)) {
> + if (sbappendaddr(so2, >so_rcv, from, m, control)) {
>   sorwakeup(so2);
>   m = NULL;
>   control = NULL;
> @@ -252,7 +252,7 @@ uipc_usrreq(struct socket *so, int req, 
>* Wake up readers.
>*/
>   if (control) {
> - if (sbappendcontrol(rcv, m, control))
> + if (sbappendcontrol(so2, rcv, m, control))
>   control = NULL;
>   

qsort(3): restore switch to insertion sort

[Todd C. Miller]

The 4.4BSD qsort has a heuristic for detecting whether the input
is partially sorted, in which case it switches to insertion sort.
This is provides a large speed up for partially sorted workloads,
which are common.

However, this resulted in quadratic behavior for certain inputs.
For more details on this, see:
http://calmerthanyouare.org/2014/06/11/algorithmic-complexity-attacks-and-libc-qsort.html

Removing this optimization in turn resulted in quadratic behavior
for other workloads, as seen by pjanzen@.  We can safely restore
the switch to insertion sort if we bail out at certain point.  In
this diff, if we perform nmemb swaps we stop the insertion sort and
restart using quicksort.  Since swap_cnt is not reset we won't
switch to insertion sort again.

This results in a 2x improvement in the BSD "killer input" compared
to the version without the heuristic.  The entire qsort regress run
time is reduced slightly.

It also restores the performance lost for the particular workload
that went quadratic when we removed the optimization.

 - todd

Index: lib/libc/stdlib/qsort.c
===
RCS file: /cvs/src/lib/libc/stdlib/qsort.c,v
retrieving revision 1.18
diff -u -p -u -r1.18 qsort.c
--- lib/libc/stdlib/qsort.c 30 May 2017 14:54:09 -  1.18
+++ lib/libc/stdlib/qsort.c 6 Jun 2017 15:38:41 -
@@ -126,7 +126,7 @@ introsort(char *a, size_t n, size_t es, 
 int (*cmp)(const void *, const void *))
 {
char *pa, *pb, *pc, *pd, *pl, *pm, *pn;
-   int cmp_result;
+   int cmp_result, swap_cnt;
size_t r, s;
 
 loop:  if (n < 7) {
@@ -141,6 +141,8 @@ loop:   if (n < 7) {
return;
}
maxdepth--;
+   swap_cnt = 0;
+restart:
pm = a + (n / 2) * es;
if (n > 7) {
pl = a;
@@ -159,6 +161,7 @@ loop:   if (n < 7) {
for (;;) {
while (pb <= pc && (cmp_result = cmp(pb, a)) <= 0) {
if (cmp_result == 0) {
+   swap_cnt = 1;
swap(pa, pb);
pa += es;
}
@@ -166,6 +169,7 @@ loop:   if (n < 7) {
}
while (pb <= pc && (cmp_result = cmp(pc, a)) >= 0) {
if (cmp_result == 0) {
+   swap_cnt = 1;
swap(pc, pd);
pd -= es;
}
@@ -173,11 +177,24 @@ loop: if (n < 7) {
}
if (pb > pc)
break;
+   swap_cnt = 1;
swap(pb, pc);
pb += es;
pc -= es;
}
 
+   if (swap_cnt == 0) {/* Switch to insertion sort */
+   for (pm = a + es; pm < a + n * es; pm += es) {
+   for (pl = pm; pl > a && cmp(pl - es, pl) > 0;
+pl -= es) {
+   /* Avoid quadratic behavior */
+   if (++swap_cnt > n)
+   goto restart;
+   swap(pl, pl - es);
+   }
+   }
+   return;
+   }
pn = a + n * es;
r = min(pa - a, pb - pa);
vecswap(a, pb - r, r);

Re: pfsync and option WITH_PF_LOCK

[Martin Pieuchot]

On 06/06/17(Tue) 17:32, Hrvoje Popovski wrote:
> Hi all,
> 
> i'm getting these traces with "option WITH_PF_LOCK" enaled.
> Setup is quite standard, trunk, vlan, carp, pfsync 
> 
> 
> splassert: pf_state_insert: want 1 have 0
> splassert: pf_remove_state: want 1 have 0

This is a known issue.  pfsync(4) needs a complete redesign for MP.

Hopefully David will enjoy doing that :)

> with kern.splassert=2
> 
> splassert: pf_state_insert: want 1 have 0
> Starting stack trace...
> pf_state_insert(80442a00,800020958c58,800020958c50,ff0786c9c898,8087afe8,ff0786c9c898)
> at pf_state_insert+0x64
> pfsync_state_import(ff00754db23c,2,ff00754db23c,108,1,0) at
> pfsync_state_import+0x6bb
> pfsync_in_ins(ff00754db23c,108,1,2,130,2c) at pfsync_in_ins+0x151
> pfsync_input(800020958df0,800020958dfc,f0,2,0,800020958dfc)
> at pfsync_input+0x371
> ip_deliver(800020958df0,800020958dfc,f0,2,0,0) at ip_deliver+0x10f
> ip_local(ff0074da0900,800020958e50,0,0,4,fdbfa4928cfbc85b) at
> ip_local+0x271
> ipintr(5,3b7,8162b4de,800020958e50,ff0074da0900,800020958e50)
> at ipintr+0x1e
> if_netisr(0,800020958eb0,800020958eb0,80019080,8116ebf0,800020958eb0)
> at if_netisr+0x1b5
> taskq_thread(80019080,2a2,80019080,8137bea0,0,800020958f10)
> at taskq_thread+0x79
> end trace frame: 0x0, count: 248
> End of stack trace.
> 
> 
> splassert: pf_remove_state: want 1 have 0
> Starting stack trace...
> pf_remove_state(ff0786c9c9d0,800020958ca0,c,ff0787323f18,ff0074bbc390,800020958d20)
> at pf_remove_state+0x43
> pfsync_in_del_c(ff0074bbc3e8,c,1,2,e0,d8) at pfsync_in_del_c+0x68
> pfsync_input(800020958df0,800020958dfc,f0,2,0,800020958dfc)
> at pfsync_input+0x371
> ip_deliver(800020958df0,800020958dfc,f0,2,0,0) at ip_deliver+0x10f
> ip_local(ff007575e400,800020958e50,0,0,4,fdbfa4928cfbc85b) at
> ip_local+0x271
> ipintr(5,3b7,8162b4de,800020958e50,ff007575e400,800020958e50)
> at ipintr+0x1e
> if_netisr(0,800020958eb0,800020958eb0,80019080,8116ebf0,800020958eb0)
> at if_netisr+0x1b5
> taskq_thread(80019080,2a2,80019080,8137bea0,0,800020958f10)
> at taskq_thread+0x79
> end trace frame: 0x0, count: 249
> End of stack trace.
> 

soassertlocked() & protocol families

[Martin Pieuchot]

Our plan is to work on the socket layer protocol per protocol.  claudio@
already sent some diffs to make the iteration on the PF_ROUTE socket
list MP-safe.  However that isn't enough for the inner par of the loop.

Since socket flag access and socket buffer modifications need to be
atomic until somebody proves the contrary.  I'd like to assert that
the KERNEL_LOCK() is held for pfkey/routing and local sockets.

The diff below only makes sense if socket buffer functions assert for
the related "socket lock".

ok?

Index: kern/uipc_socket2.c
===
RCS file: /cvs/src/sys/kern/uipc_socket2.c,v
retrieving revision 1.77
diff -u -p -r1.77 uipc_socket2.c
--- kern/uipc_socket2.c 27 May 2017 18:50:53 -  1.77
+++ kern/uipc_socket2.c 6 Jun 2017 15:29:05 -
@@ -292,10 +292,18 @@ sounlock(int s)
 void
 soassertlocked(struct socket *so)
 {
-   if ((so->so_proto->pr_domain->dom_family != PF_LOCAL) &&
-   (so->so_proto->pr_domain->dom_family != PF_ROUTE) &&
-   (so->so_proto->pr_domain->dom_family != PF_KEY))
+   switch (so->so_proto->pr_domain->dom_family) {
+   case PF_INET:
+   case PF_INET6:
NET_ASSERT_LOCKED();
+   break;
+   case PF_LOCAL:
+   case PF_ROUTE:
+   case PF_KEY:
+   default:
+   KERNEL_ASSERT_LOCKED();
+   break;
+   }
 }
 
 int

pfsync and option WITH_PF_LOCK

[Hrvoje Popovski]

Hi all,

i'm getting these traces with "option WITH_PF_LOCK" enaled.
Setup is quite standard, trunk, vlan, carp, pfsync 


splassert: pf_state_insert: want 1 have 0
splassert: pf_remove_state: want 1 have 0


with kern.splassert=2

splassert: pf_state_insert: want 1 have 0
Starting stack trace...
pf_state_insert(80442a00,800020958c58,800020958c50,ff0786c9c898,8087afe8,ff0786c9c898)
at pf_state_insert+0x64
pfsync_state_import(ff00754db23c,2,ff00754db23c,108,1,0) at
pfsync_state_import+0x6bb
pfsync_in_ins(ff00754db23c,108,1,2,130,2c) at pfsync_in_ins+0x151
pfsync_input(800020958df0,800020958dfc,f0,2,0,800020958dfc)
at pfsync_input+0x371
ip_deliver(800020958df0,800020958dfc,f0,2,0,0) at ip_deliver+0x10f
ip_local(ff0074da0900,800020958e50,0,0,4,fdbfa4928cfbc85b) at
ip_local+0x271
ipintr(5,3b7,8162b4de,800020958e50,ff0074da0900,800020958e50)
at ipintr+0x1e
if_netisr(0,800020958eb0,800020958eb0,80019080,8116ebf0,800020958eb0)
at if_netisr+0x1b5
taskq_thread(80019080,2a2,80019080,8137bea0,0,800020958f10)
at taskq_thread+0x79
end trace frame: 0x0, count: 248
End of stack trace.


splassert: pf_remove_state: want 1 have 0
Starting stack trace...
pf_remove_state(ff0786c9c9d0,800020958ca0,c,ff0787323f18,ff0074bbc390,800020958d20)
at pf_remove_state+0x43
pfsync_in_del_c(ff0074bbc3e8,c,1,2,e0,d8) at pfsync_in_del_c+0x68
pfsync_input(800020958df0,800020958dfc,f0,2,0,800020958dfc)
at pfsync_input+0x371
ip_deliver(800020958df0,800020958dfc,f0,2,0,0) at ip_deliver+0x10f
ip_local(ff007575e400,800020958e50,0,0,4,fdbfa4928cfbc85b) at
ip_local+0x271
ipintr(5,3b7,8162b4de,800020958e50,ff007575e400,800020958e50)
at ipintr+0x1e
if_netisr(0,800020958eb0,800020958eb0,80019080,8116ebf0,800020958eb0)
at if_netisr+0x1b5
taskq_thread(80019080,2a2,80019080,8137bea0,0,800020958f10)
at taskq_thread+0x79
end trace frame: 0x0, count: 249
End of stack trace.

Towards tcp_input() w/o KERNEL_LOCK()

[Martin Pieuchot]

TCP/UDP are almost ready to run without KERNEL_LOCK() because accesses
to their sockets are serialized via the NET_LOCK().  On the other hand
pfkey and routing sockets accesses still rely on the KERNEL_LOCK().

Since we're going to work at the socket layer, first to remove the
KERNEL_LOCK() from routing/pfkey sockets then to split the NET_LOCK(),
we need some tooling to move faster and avoid mistakes.

Currently all operations on socket buffers are protected by these
locks.  I'd like to assert that, at least for all functions used in
TCP/UDP layers.

The idea is to later change the lock asserted in soassertlocked(). 

Comments, ok?

Index: kern/uipc_socket.c
===
RCS file: /cvs/src/sys/kern/uipc_socket.c,v
retrieving revision 1.186
diff -u -p -r1.186 uipc_socket.c
--- kern/uipc_socket.c  31 May 2017 08:55:10 -  1.186
+++ kern/uipc_socket.c  6 Jun 2017 14:49:14 -
@@ -216,7 +216,7 @@ sofree(struct socket *so)
so->so_sp = NULL;
}
 #endif /* SOCKET_SPLICE */
-   sbrelease(>so_snd);
+   sbrelease(so, >so_snd);
sorflush(so);
pool_put(_pool, so);
 }
@@ -440,7 +440,7 @@ restart:
} else if (addr == 0)
snderr(EDESTADDRREQ);
}
-   space = sbspace(>so_snd);
+   space = sbspace(so, >so_snd);
if (flags & MSG_OOB)
space += 1024;
if ((atomic && resid > so->so_snd.sb_hiwat) ||
@@ -1058,7 +1058,9 @@ sorflush(struct socket *so)
}
if (pr->pr_flags & PR_RIGHTS && pr->pr_domain->dom_dispose)
(*pr->pr_domain->dom_dispose)(asb.sb_mb);
-   sbrelease();
+   *sb = asb;
+   sbrelease(so, sb);
+   memset(sb, 0, sizeof (*sb));
 }
 
 #ifdef SOCKET_SPLICE
@@ -1270,7 +1272,7 @@ somove(struct socket *so, int wait)
maxreached = 1;
}
}
-   space = sbspace(>so_snd);
+   space = sbspace(so, >so_snd);
if (so->so_oobmark && so->so_oobmark < len &&
so->so_oobmark < space + 1024)
space += 1024;
@@ -1635,7 +1637,7 @@ sosetopt(struct socket *so, int level, i
goto bad;
}
if (sbcheckreserve(cnt, so->so_snd.sb_wat) ||
-   sbreserve(>so_snd, cnt)) {
+   sbreserve(so, >so_snd, cnt)) {
error = ENOBUFS;
goto bad;
}
@@ -1648,7 +1650,7 @@ sosetopt(struct socket *so, int level, i
goto bad;
}
if (sbcheckreserve(cnt, so->so_rcv.sb_wat) ||
-   sbreserve(>so_rcv, cnt)) {
+   sbreserve(so, >so_rcv, cnt)) {
error = ENOBUFS;
goto bad;
}
@@ -1991,7 +1993,7 @@ filt_sowrite(struct knote *kn, long hint
 {
struct socket *so = kn->kn_fp->f_data;
 
-   kn->kn_data = sbspace(>so_snd);
+   kn->kn_data = sbspace(so, >so_snd);
if (so->so_state & SS_CANTSENDMORE) {
kn->kn_flags |= EV_EOF;
kn->kn_fflags = so->so_error;
Index: kern/uipc_usrreq.c
===
RCS file: /cvs/src/sys/kern/uipc_usrreq.c,v
retrieving revision 1.117
diff -u -p -r1.117 uipc_usrreq.c
--- kern/uipc_usrreq.c  13 Mar 2017 20:18:21 -  1.117
+++ kern/uipc_usrreq.c  6 Jun 2017 14:24:35 -
@@ -222,7 +222,7 @@ uipc_usrreq(struct socket *so, int req, 
from = mtod(unp->unp_addr, struct sockaddr *);
else
from = _noname;
-   if (sbappendaddr(>so_rcv, from, m, control)) {
+   if (sbappendaddr(so2, >so_rcv, from, m, control)) {
sorwakeup(so2);
m = NULL;
control = NULL;
@@ -252,7 +252,7 @@ uipc_usrreq(struct socket *so, int req, 
 * Wake up readers.
 */
if (control) {
-   if (sbappendcontrol(rcv, m, control))
+   if (sbappendcontrol(so2, rcv, m, control))
control = NULL;
else {
error = ENOBUFS;
Index: kern/uipc_socket2.c
===
RCS file: /cvs/src/sys/kern/uipc_socket2.c,v
retrieving revision 1.77
diff -u -p -r1.77 

Re: routing sockets vs KERNEL_LOCK()

[Claudio Jeker]

On Tue, Jun 06, 2017 at 01:03:33PM +0200, Martin Pieuchot wrote:
> On 05/06/17(Mon) 16:52, Martin Pieuchot wrote:
> > On 05/06/17(Mon) 12:12, Martin Pieuchot wrote:
> > > Routing sockets are not protected by the NET_LOCK().  That's one of the
> > > boundaries of the network stack.  That's whhy claudio@ sent some diffs
> > > to no longer require the KERNEL_LOCK() to protect them.
> > > 
> > > But right now some rtm_* functions can be executed without
> > > KERNEL_LOCK().  That's wrong.  Diff below fixes that and move the
> > > KERNEL_LOCK() further down in rtrequest(9) since the NET_LOCK() is
> > > enough to protect the other data structures.
> > > 
> > > The scariest bits come from default router advertisements, but I guess
> > > that slaacd(8) saved us ;)
> > 
> > Also change a KERNEL_ASSERT_LOCKED() into a NET_ASSERT_LOCK() in
> > rt_{set,put}gwroute().  Theses can now be called without KERNEL_LOCK()
> > when inserting an ARP entry.
> 
> Hrvoje Popovski found the hard way that rtrequest(RTM_RESOLVE...) still
> need the KERNEL_LOCK() for malloc(9)/free(9).
> 
> Here's a more conservative diff.  ok?
> 

Wondering why we don't push the lock into rtm_miss and rtm_send?
Apart from that I think we can start moving in that direction.

> Index: net/route.c
> ===
> RCS file: /cvs/src/sys/net/route.c,v
> retrieving revision 1.357
> diff -u -p -r1.357 route.c
> --- net/route.c   27 May 2017 09:51:18 -  1.357
> +++ net/route.c   6 Jun 2017 11:01:05 -
> @@ -385,7 +385,7 @@ rt_setgwroute(struct rtentry *rt, u_int 
>  {
>   struct rtentry *nhrt;
>  
> - KERNEL_ASSERT_LOCKED();
> + NET_ASSERT_LOCKED();
>  
>   KASSERT(ISSET(rt->rt_flags, RTF_GATEWAY));
>  
> @@ -442,7 +442,7 @@ rt_putgwroute(struct rtentry *rt)
>  {
>   struct rtentry *nhrt = rt->rt_gwroute;
>  
> - KERNEL_ASSERT_LOCKED();
> + NET_ASSERT_LOCKED();
>  
>   if (!ISSET(rt->rt_flags, RTF_GATEWAY) || nhrt == NULL)
>   return;
> @@ -624,7 +624,9 @@ out:
>   info.rti_info[RTAX_DST] = dst;
>   info.rti_info[RTAX_GATEWAY] = gateway;
>   info.rti_info[RTAX_AUTHOR] = src;
> + KERNEL_LOCK();
>   rtm_miss(RTM_REDIRECT, , flags, prio, ifidx, error, rdomain);
> + KERNEL_UNLOCK();
>  }
>  
>  /*
> @@ -653,8 +655,10 @@ rtdeletemsg(struct rtentry *rt, struct i
>   info.rti_flags = rt->rt_flags;
>   ifidx = rt->rt_ifidx;
>   error = rtrequest_delete(, rt->rt_priority, ifp, , tableid);
> + KERNEL_LOCK();
>   rtm_miss(RTM_DELETE, , info.rti_flags, rt->rt_priority, ifidx,
>   error, tableid);
> + KERNEL_UNLOCK();
>   if (error == 0)
>   rtfree(rt);
>   return (error);
> Index: netinet/in_pcb.c
> ===
> RCS file: /cvs/src/sys/netinet/in_pcb.c,v
> retrieving revision 1.220
> diff -u -p -r1.220 in_pcb.c
> --- netinet/in_pcb.c  7 Mar 2017 16:59:40 -   1.220
> +++ netinet/in_pcb.c  6 Jun 2017 10:56:17 -
> @@ -716,8 +716,11 @@ in_losing(struct inpcb *inp)
>   info.rti_info[RTAX_DST] = >inp_route.ro_dst;
>   info.rti_info[RTAX_GATEWAY] = rt->rt_gateway;
>   info.rti_info[RTAX_NETMASK] = rt_plen2mask(rt, _mask);
> +
> + KERNEL_LOCK();
>   rtm_miss(RTM_LOSING, , rt->rt_flags, rt->rt_priority,
>   rt->rt_ifidx, 0, inp->inp_rtableid);
> + KERNEL_UNLOCK();
>   if (rt->rt_flags & RTF_DYNAMIC)
>   (void)rtrequest(RTM_DELETE, , rt->rt_priority,
>   NULL, inp->inp_rtableid);
> Index: netinet6/nd6_rtr.c
> ===
> RCS file: /cvs/src/sys/netinet6/nd6_rtr.c,v
> retrieving revision 1.159
> diff -u -p -r1.159 nd6_rtr.c
> --- netinet6/nd6_rtr.c30 May 2017 08:58:34 -  1.159
> +++ netinet6/nd6_rtr.c6 Jun 2017 10:56:17 -
> @@ -613,7 +613,9 @@ defrouter_addreq(struct nd_defrouter *ne
>   error = rtrequest(RTM_ADD, , RTP_DEFAULT, ,
>   new->ifp->if_rdomain);
>   if (error == 0) {
> + KERNEL_LOCK();
>   rtm_send(rt, RTM_ADD, new->ifp->if_rdomain);
> + KERNEL_UNLOCK();
>   rtfree(rt);
>   new->installed = 1;
>   }
> @@ -717,7 +719,9 @@ defrouter_delreq(struct nd_defrouter *dr
>   error = rtrequest(RTM_DELETE, , RTP_DEFAULT, ,
>   dr->ifp->if_rdomain);
>   if (error == 0) {
> + KERNEL_LOCK();
>   rtm_send(rt, RTM_DELETE, dr->ifp->if_rdomain);
> + KERNEL_UNLOCK();
>   rtfree(rt);
>   }
>  
> 

-- 
:wq Claudio

radix lookup w/o KERNEL_LOCK()

[Martin Pieuchot]

One of the reasons IPsec still requires the KERNEL_LOCK() in the
forwarding path is that it uses the radix tree for the SPD lookup.

Since the radix code is used by other subsystems (NFS export list,
PIPEX, PF) we want it to be able to run on parallel, at least when
callers aren't manipulating the same tree.

That's what the diff below does by getting rid of the globals used
in rn_addmask().  For that we now use a buffer on the stack of the
caller instead of the global ``addmask_key''.

While here I documented other globals, moved local prototypes to the
C file and added a SALEN() macro for readability purpose.

ok?

Index: net/radix.c
===
RCS file: /cvs/src/sys/net/radix.c,v
retrieving revision 1.56
diff -u -p -r1.56 radix.c
--- net/radix.c 24 Jan 2017 10:08:30 -  1.56
+++ net/radix.c 6 Jun 2017 13:23:19 -
@@ -48,24 +48,32 @@
 
 #include 
 
-static unsigned int max_keylen;
-struct radix_node_head *mask_rnhead;
-static char *addmask_key;
-static char normal_chars[] = {0, 0x80, 0xc0, 0xe0, 0xf0, 0xf8, 0xfc, 0xfe, -1};
-static char *rn_zeros, *rn_ones;
+#define SALEN(sa)  (*(u_char *)sa)
+
+/*
+ * Read-only variables, allocated & filled during rn_init().
+ */
+static char*rn_zeros;  /* array of 0s */
+static char*rn_ones;   /* array of 1s */
+static unsigned int max_keylen;/* size of the above arrays */
+#define KEYLEN_LIMIT64 /* maximum allowed keylen */
 
-struct pool rtmask_pool;   /* pool for radix_mask structures */
 
-#define rn_masktop (mask_rnhead->rnh_treetop)
+struct radix_node_head *mask_rnhead;   /* head of shared mask tree */
+struct pool rtmask_pool;   /* pool for radix_mask structures */
 
+static int rn_refines(void *, void *);
+static int rn_inithead0(struct radix_node_head *, int);
 static inline int rn_satisfies_leaf(char *, struct radix_node *, int);
 static inline int rn_lexobetter(void *, void *);
 static inline struct radix_mask *rn_new_radix_mask(struct radix_node *,
 struct radix_mask *);
 
+struct radix_node *rn_addmask(void *, int, int);
 struct radix_node *rn_insert(void *, struct radix_node_head *, int *,
 struct radix_node [2]);
 struct radix_node *rn_newpair(void *, int, struct radix_node[2]);
+void rn_link_dupedkey(struct radix_node *, struct radix_node *, int);
 
 static inline struct radix_node *rn_search(void *, struct radix_node *);
 struct radix_node *rn_search_m(void *, struct radix_node *, void *);
@@ -143,7 +151,7 @@ rn_search_m(void *v_arg, struct radix_no
return x;
 }
 
-int
+static int
 rn_refines(void *m_arg, void *n_arg)
 {
caddr_t m = m_arg;
@@ -414,8 +422,9 @@ rn_addmask(void *n_arg, int search, int 
int b = 0, mlen, j;
int maskduplicated, m0, isnormal;
static int last_zeroed = 0;
+   char addmask_key[KEYLEN_LIMIT];
 
-   if ((mlen = *(u_char *)netmask) > max_keylen)
+   if ((mlen = SALEN(netmask)) > max_keylen)
mlen = max_keylen;
if (skip == 0)
skip = 1;
@@ -439,7 +448,7 @@ rn_addmask(void *n_arg, int search, int 
if (m0 < last_zeroed)
memset(addmask_key + m0, 0, last_zeroed - m0);
*addmask_key = last_zeroed = mlen;
-   tm = rn_search(addmask_key, rn_masktop);
+   tm = rn_search(addmask_key, mask_rnhead->rnh_treetop);
if (memcmp(addmask_key, tm->rn_key, mlen) != 0)
tm = NULL;
if (tm || search)
@@ -452,7 +461,7 @@ rn_addmask(void *n_arg, int search, int 
memcpy(cp, addmask_key, mlen);
tm = rn_insert(cp, mask_rnhead, , tm);
if (maskduplicated) {
-   log(LOG_ERR, "rn_addmask: mask impossibly already in tree\n");
+   log(LOG_ERR, "%s: mask impossibly already in tree\n", __func__);
free(saved_tm, M_RTABLE, 0);
return (tm);
}
@@ -464,6 +473,9 @@ rn_addmask(void *n_arg, int search, int 
for (cp = netmask + skip; (cp < cplim) && *(u_char *)cp == 0xff;)
cp++;
if (cp != cplim) {
+   static const char normal_chars[] = {
+   0, 0x80, 0xc0, 0xe0, 0xf0, 0xf8, 0xfc, 0xfe, -1
+   };
for (j = 0x80; (j & *cp) != 0; j >>= 1)
b++;
if (*cp != normal_chars[b] || cp != (cplim - 1))
@@ -488,9 +500,9 @@ rn_lexobetter(void *m_arg, void *n_arg)
 * first. The netmasks were normalized before calling this function and
 * don't have unneeded trailing zeros.
 */
-   if (*mp > *np)
+   if (SALEN(mp) > SALEN(np))
return 1;
-   if (*mp < *np)
+   if (SALEN(mp) < SALEN(np))
return 0;
/*
 * Must return the first difference between the masks
@@ -756,6 +768,8 @@ rn_addroute(void *v_arg, void *n_arg, st
struct radix_node *tt, *saved_tt, *tm = NULL;
int 

Re: routing sockets vs KERNEL_LOCK()

[Hrvoje Popovski]

On 6.6.2017. 13:03, Martin Pieuchot wrote:
> On 05/06/17(Mon) 16:52, Martin Pieuchot wrote:
>> On 05/06/17(Mon) 12:12, Martin Pieuchot wrote:
>>> Routing sockets are not protected by the NET_LOCK().  That's one of the
>>> boundaries of the network stack.  That's whhy claudio@ sent some diffs
>>> to no longer require the KERNEL_LOCK() to protect them.
>>>
>>> But right now some rtm_* functions can be executed without
>>> KERNEL_LOCK().  That's wrong.  Diff below fixes that and move the
>>> KERNEL_LOCK() further down in rtrequest(9) since the NET_LOCK() is
>>> enough to protect the other data structures.
>>>
>>> The scariest bits come from default router advertisements, but I guess
>>> that slaacd(8) saved us ;)
>> Also change a KERNEL_ASSERT_LOCKED() into a NET_ASSERT_LOCK() in
>> rt_{set,put}gwroute().  Theses can now be called without KERNEL_LOCK()
>> when inserting an ARP entry.
> Hrvoje Popovski found the hard way that rtrequest(RTM_RESOLVE...) still
> need the KERNEL_LOCK() for malloc(9)/free(9).

Hi,

with this patch i can't trigger panic as before.

Thank you.

remove hostname not IP addr test in libtls tls_servername_cb()

[Jonathan Gray]

It turns out that despite RFC 6066 stating
'Literal IPv4 and IPv6 addresses are not permitted in "HostName".'
for SNI the implementations of TLS in python and ruby do this.

While chromium, firefox, lua(sec), java, go, ftp(1), curl, wget,
and others when acting as TLS clients all manage to get it right.

Both apache 2.4.25 and nginx 1.10.2p from ports do not strictly
enforce this on the server side but httpd(8) does as libtls does.

import httplib
import ssl

ctx = ssl._create_unverified_context()
con = httplib.HTTPSConnection('127.0.0.1', 443, context=ctx)

con.request('GET', '/')
res = con.getresponse()
print(res.status)

gives

$ python2.7 test.py
Traceback (most recent call last):
  File "test.py", line 7, in 
con.request('GET', '/')
  File "/usr/local/lib/python2.7/httplib.py", line 1042, in request
self._send_request(method, url, body, headers)
  File "/usr/local/lib/python2.7/httplib.py", line 1082, in _send_request
self.endheaders(body)
  File "/usr/local/lib/python2.7/httplib.py", line 1038, in endheaders
self._send_output(message_body)
  File "/usr/local/lib/python2.7/httplib.py", line 882, in _send_output
self.send(msg)
  File "/usr/local/lib/python2.7/httplib.py", line 844, in send
self.connect()
  File "/usr/local/lib/python2.7/httplib.py", line 1263, in connect
server_hostname=server_hostname)
  File "/usr/local/lib/python2.7/ssl.py", line 363, in wrap_socket
_context=self)
  File "/usr/local/lib/python2.7/ssl.py", line 611, in __init__
self.do_handshake()
  File "/usr/local/lib/python2.7/ssl.py", line 840, in do_handshake
self._sslobj.do_handshake()
ssl.SSLError: [SSL: TLSV1_ALERT_INTERNAL_ERROR] tlsv1 alert internal error 
(_ssl.c:661)

after patching the check out of libtls and restarting httpd

$ python2.7 test.py
200

Index: tls_server.c
===
RCS file: /cvs/src/lib/libtls/tls_server.c,v
retrieving revision 1.37
diff -u -p -r1.37 tls_server.c
--- tls_server.c6 May 2017 20:59:28 -   1.37
+++ tls_server.c6 Jun 2017 11:27:44 -
@@ -74,7 +74,6 @@ tls_servername_cb(SSL *ssl, int *al, voi
 {
struct tls *ctx = (struct tls *)arg;
struct tls_sni_ctx *sni_ctx;
-   union tls_addr addrbuf;
struct tls *conn_ctx;
const char *name;
int match;
@@ -90,11 +89,6 @@ tls_servername_cb(SSL *ssl, int *al, voi
 */
return (SSL_TLSEXT_ERR_NOACK);
}
-
-   /* Per RFC 6066 section 3: ensure that name is not an IP literal. */
-   if (inet_pton(AF_INET, name, ) == 1 ||
-inet_pton(AF_INET6, name, ) == 1)
-   goto err;
 
free((char *)conn_ctx->servername);
if ((conn_ctx->servername = strdup(name)) == NULL)

Re: routing sockets vs KERNEL_LOCK()

[Martin Pieuchot]

On 05/06/17(Mon) 16:52, Martin Pieuchot wrote:
> On 05/06/17(Mon) 12:12, Martin Pieuchot wrote:
> > Routing sockets are not protected by the NET_LOCK().  That's one of the
> > boundaries of the network stack.  That's whhy claudio@ sent some diffs
> > to no longer require the KERNEL_LOCK() to protect them.
> > 
> > But right now some rtm_* functions can be executed without
> > KERNEL_LOCK().  That's wrong.  Diff below fixes that and move the
> > KERNEL_LOCK() further down in rtrequest(9) since the NET_LOCK() is
> > enough to protect the other data structures.
> > 
> > The scariest bits come from default router advertisements, but I guess
> > that slaacd(8) saved us ;)
> 
> Also change a KERNEL_ASSERT_LOCKED() into a NET_ASSERT_LOCK() in
> rt_{set,put}gwroute().  Theses can now be called without KERNEL_LOCK()
> when inserting an ARP entry.

Hrvoje Popovski found the hard way that rtrequest(RTM_RESOLVE...) still
need the KERNEL_LOCK() for malloc(9)/free(9).

Here's a more conservative diff.  ok?

Index: net/route.c
===
RCS file: /cvs/src/sys/net/route.c,v
retrieving revision 1.357
diff -u -p -r1.357 route.c
--- net/route.c 27 May 2017 09:51:18 -  1.357
+++ net/route.c 6 Jun 2017 11:01:05 -
@@ -385,7 +385,7 @@ rt_setgwroute(struct rtentry *rt, u_int 
 {
struct rtentry *nhrt;
 
-   KERNEL_ASSERT_LOCKED();
+   NET_ASSERT_LOCKED();
 
KASSERT(ISSET(rt->rt_flags, RTF_GATEWAY));
 
@@ -442,7 +442,7 @@ rt_putgwroute(struct rtentry *rt)
 {
struct rtentry *nhrt = rt->rt_gwroute;
 
-   KERNEL_ASSERT_LOCKED();
+   NET_ASSERT_LOCKED();
 
if (!ISSET(rt->rt_flags, RTF_GATEWAY) || nhrt == NULL)
return;
@@ -624,7 +624,9 @@ out:
info.rti_info[RTAX_DST] = dst;
info.rti_info[RTAX_GATEWAY] = gateway;
info.rti_info[RTAX_AUTHOR] = src;
+   KERNEL_LOCK();
rtm_miss(RTM_REDIRECT, , flags, prio, ifidx, error, rdomain);
+   KERNEL_UNLOCK();
 }
 
 /*
@@ -653,8 +655,10 @@ rtdeletemsg(struct rtentry *rt, struct i
info.rti_flags = rt->rt_flags;
ifidx = rt->rt_ifidx;
error = rtrequest_delete(, rt->rt_priority, ifp, , tableid);
+   KERNEL_LOCK();
rtm_miss(RTM_DELETE, , info.rti_flags, rt->rt_priority, ifidx,
error, tableid);
+   KERNEL_UNLOCK();
if (error == 0)
rtfree(rt);
return (error);
Index: netinet/in_pcb.c
===
RCS file: /cvs/src/sys/netinet/in_pcb.c,v
retrieving revision 1.220
diff -u -p -r1.220 in_pcb.c
--- netinet/in_pcb.c7 Mar 2017 16:59:40 -   1.220
+++ netinet/in_pcb.c6 Jun 2017 10:56:17 -
@@ -716,8 +716,11 @@ in_losing(struct inpcb *inp)
info.rti_info[RTAX_DST] = >inp_route.ro_dst;
info.rti_info[RTAX_GATEWAY] = rt->rt_gateway;
info.rti_info[RTAX_NETMASK] = rt_plen2mask(rt, _mask);
+
+   KERNEL_LOCK();
rtm_miss(RTM_LOSING, , rt->rt_flags, rt->rt_priority,
rt->rt_ifidx, 0, inp->inp_rtableid);
+   KERNEL_UNLOCK();
if (rt->rt_flags & RTF_DYNAMIC)
(void)rtrequest(RTM_DELETE, , rt->rt_priority,
NULL, inp->inp_rtableid);
Index: netinet6/nd6_rtr.c
===
RCS file: /cvs/src/sys/netinet6/nd6_rtr.c,v
retrieving revision 1.159
diff -u -p -r1.159 nd6_rtr.c
--- netinet6/nd6_rtr.c  30 May 2017 08:58:34 -  1.159
+++ netinet6/nd6_rtr.c  6 Jun 2017 10:56:17 -
@@ -613,7 +613,9 @@ defrouter_addreq(struct nd_defrouter *ne
error = rtrequest(RTM_ADD, , RTP_DEFAULT, ,
new->ifp->if_rdomain);
if (error == 0) {
+   KERNEL_LOCK();
rtm_send(rt, RTM_ADD, new->ifp->if_rdomain);
+   KERNEL_UNLOCK();
rtfree(rt);
new->installed = 1;
}
@@ -717,7 +719,9 @@ defrouter_delreq(struct nd_defrouter *dr
error = rtrequest(RTM_DELETE, , RTP_DEFAULT, ,
dr->ifp->if_rdomain);
if (error == 0) {
+   KERNEL_LOCK();
rtm_send(rt, RTM_DELETE, dr->ifp->if_rdomain);
+   KERNEL_UNLOCK();
rtfree(rt);
}
 

Re: ifconfig.8 doco for vnetid and parent options

[Jason McIntyre]

On Tue, Jun 06, 2017 at 11:56:28AM +1000, David Gwynne wrote:
> this adds doco for the parent options in ifconfig, and moves vnetid
> up into generic options list.
> 
> the vlan(4) specific doco has enough lies and omissions that id
> rather get rid of it.
> 
> ok?
> 

morning.

you are breaking the format of this page if you start to move specific
subsytems into the main body.

that makes things less clear. for example, can i use the parent-device
stuff on any type of interface or just vlan? if it's vlan related, how
can i tell that?

the text reads fine, but i'd rather you tried to integrate it into the
vlan section. if there are errors in there let's remove them.

but if it's the case that none of this stuff is specific to vlan, then i
guess it makes sense to do it your way (but then you'd have to take care
when documenting stuff like parent-device to say in what situations it
makes sense).

jmc

> Index: ifconfig.8
> ===
> RCS file: /cvs/src/sbin/ifconfig/ifconfig.8,v
> retrieving revision 1.282
> diff -u -p -r1.282 ifconfig.8
> --- ifconfig.812 May 2017 15:11:02 -  1.282
> +++ ifconfig.86 Jun 2017 01:54:10 -
> @@ -415,6 +415,14 @@ and 0's for the host part.
>  The mask should contain at least the standard network portion,
>  and the subnet field should be contiguous with the network
>  portion.
> +.It Cm parent Ar parent-interface
> +Associate with interface
> +.Ar parent-interface .
> +Packets transmitted through the interface will be encapsulated and
> +diverted to the specified parent interface
> +.It Cm -parent
> +Disassociate from the parent interface.
> +This breaks the link between the interface and its parent.
>  .It Cm prefixlen Ar n
>  (inet and inet6 only)
>  Effect is similar to
> @@ -461,6 +469,20 @@ This may be used to enable an interface 
>  It happens automatically when setting the first address on an interface.
>  If the interface was reset when previously marked down,
>  the hardware will be re-initialized.
> +.It Cm vnetid Ar network-id Ns | Ns Cm any
> +Set the virtual network identifier.
> +This is a number which is used by encapsulation interfaces such as
> +.Xr vlan 4
> +or
> +.Xr vxlan 4
> +to identify packets with a virtual network.
> +If supported by the interface,
> +the value can also be set to
> +.Ar any
> +to accept packets with arbitrary network identifiers (for example for
> +multipoint-to-multipoint modes).
> +.It Cm -vnetid
> +Clear the virtual network identifier.
>  .It Cm wol
>  Enable Wake on LAN (WoL).
>  When enabled, reception of a WoL frame will cause the network card to
> @@ -1531,7 +1553,6 @@ for a complete list of the available pro
>  .Op Oo Fl Oc Ns Cm keepalive Ar period count
>  .Op Cm tunnel Ar src_address dest_address
>  .Op Cm tunneldomain Ar tableid
> -.Op Oo Fl Oc Ns Cm vnetid Ar network-id
>  .Ek
>  .nr nS 0
>  .Pp
> @@ -1583,21 +1604,6 @@ can be set to any valid routing table ID
>  the corresponding routing domain is derived from this table.
>  .It Cm tunnelttl Ar ttl
>  Set the IP or multicast TTL of the tunnel packets.
> -.It Cm vnetid Ar network-id
> -Set the virtual network identifier.
> -This is a number which is used by tunnel protocols such as
> -.Xr vxlan 4
> -to identify packets with a virtual network.
> -The accepted size of the number depends on the individual tunnel protocol;
> -it is a 24-bit number for
> -.Xr vxlan 4 .
> -If supported by the tunnel protocol,
> -the value can also be set to
> -.Ar any
> -to accept packets with arbitrary network identifiers (for example for
> -multipoint-to-multipoint modes).
> -.It Cm -vnetid
> -Clear the virtual network identifier.
>  .El
>  .Sh UMB
>  .nr nS 1
> @@ -1660,52 +1666,6 @@ Disable data roaming.
>  As soon as the interface is marked as "up", the
>  .Xr umb 4
>  device will try to establish a data connection with the service provider.
> -.El
> -.Sh VLAN
> -.nr nS 1
> -.Bk -words
> -.Nm ifconfig
> -.Ar vlan-interface
> -.Op Cm vlan Ar vlan-tag
> -.Op Oo Fl Oc Ns Cm vlandev Ar parent-interface
> -.Ek
> -.nr nS 0
> -.Pp
> -The following options are available for a
> -.Xr vlan 4
> -interface:
> -.Bl -tag -width Ds
> -.It Cm vlan Ar vlan-tag
> -Set the vlan tag value
> -to
> -.Ar vlan-tag .
> -This value is a 12-bit number which is used to create an 802.1Q
> -vlan header for packets sent from the vlan interface.
> -This value cannot be changed once it is set for an interface.
> -.It Cm vlandev Ar parent-interface
> -Associate with interface
> -.Ar parent-interface .
> -Packets transmitted through the vlan interface will be
> -diverted to the specified interface
> -.Ar parent-interface
> -with 802.1Q vlan tagging.
> -Packets with 802.1Q tagging received
> -by the parent interface with the correct vlan tag will be diverted to
> -the associated vlan pseudo-device.
> -The vlan interface is assigned a
> -copy of the parent interface's flags and the parent's Ethernet address.
> -If
> -.Cm vlandev
> -and
> -.Cm