Re: netstart: implicit up and explicit down for hostname.if conf files

[Theo de Raadt]

No.  I don't see any study of who this affects negatively.

interface queue transmit mitigation (again)

[David Gwynne]

this adds transmit mitigation back to the tree.

it is basically the same diff as last time. the big difference this
time is that all the tunnel drivers all defer ip_output calls, which
avoids having to play games with NET_LOCK in the ifq transmit paths.

tests? ok?

Index: ifq.c
===
RCS file: /cvs/src/sys/net/ifq.c,v
retrieving revision 1.22
diff -u -p -r1.22 ifq.c
--- ifq.c   25 Jan 2018 14:04:36 -  1.22
+++ ifq.c   14 Mar 2018 02:58:13 -
@@ -70,9 +70,16 @@ struct priq {
 void   ifq_start_task(void *);
 void   ifq_restart_task(void *);
 void   ifq_barrier_task(void *);
+void   ifq_bundle_task(void *);
 
 #define TASK_ONQUEUE 0x1
 
+static inline void
+ifq_run_start(struct ifqueue *ifq)
+{
+   ifq_serialize(ifq, >ifq_start);
+}
+
 void
 ifq_serialize(struct ifqueue *ifq, struct task *t)
 {
@@ -114,6 +121,16 @@ ifq_is_serialized(struct ifqueue *ifq)
 }
 
 void
+ifq_start(struct ifqueue *ifq)
+{
+   if (ifq_len(ifq) >= min(4, ifq->ifq_maxlen)) {
+   task_del(ifq->ifq_softnet, >ifq_bundle);
+   ifq_run_start(ifq);
+   } else
+   task_add(ifq->ifq_softnet, >ifq_bundle);
+}
+
+void
 ifq_start_task(void *p)
 {
struct ifqueue *ifq = p;
@@ -137,11 +154,33 @@ ifq_restart_task(void *p)
 }
 
 void
+ifq_bundle_task(void *p)
+{
+   struct ifqueue *ifq = p;
+
+   ifq_run_start(ifq);
+}
+
+void
 ifq_barrier(struct ifqueue *ifq)
 {
struct cond c = COND_INITIALIZER();
struct task t = TASK_INITIALIZER(ifq_barrier_task, );
 
+   NET_ASSERT_UNLOCKED();
+
+   if (!task_del(ifq->ifq_softnet, >ifq_bundle)) {
+   int netlocked = (rw_status() == RW_WRITE);
+
+   if (netlocked) /* XXXSMP breaks atomicity */
+   NET_UNLOCK();
+
+   taskq_barrier(ifq->ifq_softnet);
+
+   if (netlocked)
+   NET_LOCK();
+   }
+
if (ifq->ifq_serializer == NULL)
return;
 
@@ -166,6 +205,7 @@ void
 ifq_init(struct ifqueue *ifq, struct ifnet *ifp, unsigned int idx)
 {
ifq->ifq_if = ifp;
+   ifq->ifq_softnet = net_tq(ifp->if_index);
ifq->ifq_softc = NULL;
 
mtx_init(>ifq_mtx, IPL_NET);
@@ -187,6 +227,7 @@ ifq_init(struct ifqueue *ifq, struct ifn
mtx_init(>ifq_task_mtx, IPL_NET);
TAILQ_INIT(>ifq_task_list);
ifq->ifq_serializer = NULL;
+   task_set(>ifq_bundle, ifq_bundle_task, ifq);
 
task_set(>ifq_start, ifq_start_task, ifq);
task_set(>ifq_restart, ifq_restart_task, ifq);
@@ -237,6 +278,8 @@ void
 ifq_destroy(struct ifqueue *ifq)
 {
struct mbuf_list ml = MBUF_LIST_INITIALIZER();
+
+   ifq_barrier(ifq); /* ensure nothing is running with the ifq */
 
/* don't need to lock because this is the last use of the ifq */
 
Index: ifq.h
===
RCS file: /cvs/src/sys/net/ifq.h,v
retrieving revision 1.20
diff -u -p -r1.20 ifq.h
--- ifq.h   4 Jan 2018 11:02:57 -   1.20
+++ ifq.h   14 Mar 2018 02:58:13 -
@@ -25,6 +25,7 @@ struct ifq_ops;
 
 struct ifqueue {
struct ifnet*ifq_if;
+   struct taskq*ifq_softnet;
union {
void*_ifq_softc;
/*
@@ -57,6 +58,7 @@ struct ifqueue {
struct mutex ifq_task_mtx;
struct task_list ifq_task_list;
void*ifq_serializer;
+   struct task  ifq_bundle;
 
/* work to be serialised */
struct task  ifq_start;
@@ -405,6 +407,7 @@ void ifq_attach(struct ifqueue *, cons
 voidifq_destroy(struct ifqueue *);
 voidifq_add_data(struct ifqueue *, struct if_data *);
 int ifq_enqueue(struct ifqueue *, struct mbuf *);
+voidifq_start(struct ifqueue *);
 struct mbuf*ifq_deq_begin(struct ifqueue *);
 voidifq_deq_commit(struct ifqueue *, struct mbuf *);
 voidifq_deq_rollback(struct ifqueue *, struct mbuf *);
@@ -438,12 +441,6 @@ static inline unsigned int
 ifq_is_oactive(struct ifqueue *ifq)
 {
return (ifq->ifq_oactive);
-}
-
-static inline void
-ifq_start(struct ifqueue *ifq)
-{
-   ifq_serialize(ifq, >ifq_start);
 }
 
 static inline void

Re: netstart: implicit up and explicit down for hostname.if conf files

[Bryan Vyhmeister]

On Wed, Mar 14, 2018 at 10:27:10AM +1000, David Gwynne wrote:
> this makes netstart run "hostname foo0 up" for every interface that
> is configured. however, if you do not want an interface to be brought
> up on boot (you may just want to configure it) you can disable this
> by putting "down" in the file.
> 
> this has two uses. the first is to simplify config for things like
> bridge. right now you need an explicit "up" for those interfaces,
> which is inconsistent with the configuration of most of our other
> interfaces.
> 
> the other is to help move us away from using address configuration
> as an implicit up. this is particularly desirable for tunnel
> interfaces, where it doesn't make sense to bring an interface up
> until it has been properly configured. having netstart bring the
> interface up after all configuration, both tunnel and addresses,
> makes it possible for the kernel to check the tunnel configuration
> when it is brought up, rather than have it stumble along if it is
> misconfigured.
> 
> thoughts? ok?

I, for one, would be very happy with this change. The inconsistency of
"up" only being sometimes required ends up causing me to add "up" to
most interfaces if multiple lines of configuration are present like for
vlan(4) interfaces and that type of thing. I had an issue where I
upgraded from 6.0 to 6.1 or possibly 6.1 to 6.2 and, because I did not
have "up" in some of the hostname.if files, I had some issues with the
upgrade. It was not a big problem but, as I understand it, this would
solve it completely.

Bryan

Re: Bugfix: acme-client 301 redirect issue

[Stuart Henderson]

On 2018/03/11 17:52, Florian Obser wrote:
> 
> I think we should just follow the 301.

I didn't hear back from @letsencrypt_ops about why they were
issue 301s, but I do agree it makes sense to follow them.

> OK?
> 
> diff --git netproc.c netproc.c
> index 26033a3fc3c..14da5a8c1a9 100644
> --- netproc.c
> +++ netproc.c
> @@ -180,15 +180,18 @@ nreq(struct conn *c, const char *addr)
>  {
>   struct httpget  *g;
>   struct sourcesrc[MAX_SERVERS_DNS];
> + struct httphead *st;
>   char*host, *path;
>   shortport;
>   size_t   srcsz;
>   ssize_t  ssz;
>   long code;
> + int  redirects = 0;
>  
>   if ((host = url2host(addr, , )) == NULL)
>   return -1;
>  
> +again:
>   if ((ssz = urlresolve(c->dfd, host, src)) < 0) {
>   free(host);
>   free(path);
> @@ -202,6 +205,28 @@ nreq(struct conn *c, const char *addr)
>   if (g == NULL)
>   return -1;
>  
> + if (g->code == 301) {

301 is the one we saw, but shouldn't we follow others as well?

https://http.cat/302
https://http.cat/303
https://http.cat/307


> + redirects++;
> + if (redirects > 3) {
> + warnx("too many redirects");
> + http_get_free(g);
> + return -1;
> + }
> +
> + if ((st = http_head_get("Location", g->head, g->headsz)) ==
> + NULL) {
> + warnx("redirect without location header");
> + return -1;
> + }
> +
> + dodbg("Location: %s", st->val);
> + host = url2host(st->val, , );
> + http_get_free(g);
> + if (host == NULL)   

nitpicking, trailing tabs

> + return -1;
> + goto again;
> + }
> +
>   code = g->code;
>  
>   /* Copy the body part into our buffer. */
> 
> -- 
> I'm not entirely sure you are real.
> 

netstart: implicit up and explicit down for hostname.if conf files

[David Gwynne]

this makes netstart run "hostname foo0 up" for every interface that
is configured. however, if you do not want an interface to be brought
up on boot (you may just want to configure it) you can disable this
by putting "down" in the file.

this has two uses. the first is to simplify config for things like
bridge. right now you need an explicit "up" for those interfaces,
which is inconsistent with the configuration of most of our other
interfaces.

the other is to help move us away from using address configuration
as an implicit up. this is particularly desirable for tunnel
interfaces, where it doesn't make sense to bring an interface up
until it has been properly configured. having netstart bring the
interface up after all configuration, both tunnel and addresses,
makes it possible for the kernel to check the tunnel configuration
when it is brought up, rather than have it stumble along if it is
misconfigured.

thoughts? ok?

Index: etc/netstart
===
RCS file: /cvs/src/etc/netstart,v
retrieving revision 1.190
diff -u -p -r1.190 netstart
--- etc/netstart10 Feb 2018 08:46:10 -  1.190
+++ etc/netstart14 Mar 2018 00:09:12 -
@@ -62,6 +62,9 @@ parse_hn_line() {
_cmds[${#_cmds[*]}]="ifconfig $_if ${_c[@]} down;dhclient $_if"
V4_DHCPCONF=true
;;
+   down)   _c[0]=
+   _ifup=down
+   ;;
'!'*)   _cmd=$(print -- "${_c[@]}" | sed 's/\$if/'$_if'/g')
_cmds[${#_cmds[*]}]="${_cmd#!}"
;;
@@ -77,6 +80,7 @@ parse_hn_line() {
 ifstart() {
local _if=$1 _hn=/etc/hostname.$1 _cmds _i=0 _line _stat
set -A _cmds
+   _ifup=up
 
# Interface names must be alphanumeric only.  We check to avoid
# configuring backup or temp files, and to catch the "*" case.
@@ -107,6 +111,8 @@ ifstart() {
while IFS= read -- _line; do
parse_hn_line $_line
done <$_hn
+
+   _cmds[${#_cmds[*]}]="ifconfig $_if $_ifup"
 
# Apply the interface configuration commands stored in _cmds array.
while ((_i < ${#_cmds[*]})); do
Index: share/man/man5/hostname.if.5
===
RCS file: /cvs/src/share/man/man5/hostname.if.5,v
retrieving revision 1.65
diff -u -p -r1.65 hostname.if.5
--- share/man/man5/hostname.if.510 Mar 2017 18:28:11 -  1.65
+++ share/man/man5/hostname.if.514 Mar 2018 00:09:12 -
@@ -57,6 +57,9 @@ the administrator should not expect magi
 and the
 per-driver manual pages to see what arguments are permitted.
 .Pp
+Interfaces are implicitly configured to be brought up and running.
+This behaviour can be disabled by adding a line containing down to the file.
+.Pp
 Arguments containing either whitespace or single quote
 characters must be double quoted.
 For example:

netinet6 - bcopy -> memcpy

[David Hill]

Hello -

A few bcopy to memcpy conversions where the memory does not overlap.

OK?

Index: netinet6/icmp6.c
===
RCS file: /cvs/src/sys/netinet6/icmp6.c,v
retrieving revision 1.221
diff -u -p -r1.221 icmp6.c
--- netinet6/icmp6.c14 Dec 2017 14:26:50 -  1.221
+++ netinet6/icmp6.c13 Mar 2018 21:32:32 -
@@ -1075,7 +1075,7 @@ icmp6_reflect(struct mbuf *m, size_t off
if ((m = m_pullup(m, l)) == NULL)
return;
}
-   bcopy((caddr_t), mtod(m, caddr_t), sizeof(nip6));
+   memcpy(mtod(m, caddr_t), (caddr_t), sizeof(nip6));
} else /* off == sizeof(struct ip6_hdr) */ {
size_t l;
l = sizeof(struct ip6_hdr) + sizeof(struct icmp6_hdr);
@@ -1268,7 +1268,7 @@ icmp6_redirect_input(struct mbuf *m, int
bzero(, sizeof(sin6));
sin6.sin6_family = AF_INET6;
sin6.sin6_len = sizeof(struct sockaddr_in6);
-   bcopy(, _addr, sizeof(reddst6));
+   memcpy(_addr, , sizeof(reddst6));
rt = rtalloc(sin6tosa(), 0, m->m_pkthdr.ph_rtableid);
if (rt) {
if (rt->rt_gateway == NULL ||
@@ -1376,9 +1376,9 @@ icmp6_redirect_input(struct mbuf *m, int
sdst.sin6_family = sgw.sin6_family = ssrc.sin6_family = 
AF_INET6;
sdst.sin6_len = sgw.sin6_len = ssrc.sin6_len =
sizeof(struct sockaddr_in6);
-   bcopy(, _addr, sizeof(struct in6_addr));
-   bcopy(, _addr, sizeof(struct in6_addr));
-   bcopy(, _addr, sizeof(struct in6_addr));
+   memcpy(_addr, , sizeof(struct in6_addr));
+   memcpy(_addr, , sizeof(struct in6_addr));
+   memcpy(_addr, , sizeof(struct in6_addr));
rtredirect(sin6tosa(), sin6tosa(), sin6tosa(),
, m->m_pkthdr.ph_rtableid);
 
@@ -1395,7 +1395,7 @@ icmp6_redirect_input(struct mbuf *m, int
bzero(, sizeof(sdst));
sdst.sin6_family = AF_INET6;
sdst.sin6_len = sizeof(struct sockaddr_in6);
-   bcopy(, _addr, sizeof(struct in6_addr));
+   memcpy(_addr, , sizeof(struct in6_addr));
pfctlinput(PRC_REDIRECT_HOST, sin6tosa());
}
 
Index: netinet6/in6_ifattach.c
===
RCS file: /cvs/src/sys/netinet6/in6_ifattach.c,v
retrieving revision 1.106
diff -u -p -r1.106 in6_ifattach.c
--- netinet6/in6_ifattach.c 13 Mar 2018 13:58:03 -  1.106
+++ netinet6/in6_ifattach.c 13 Mar 2018 21:32:33 -
@@ -165,7 +165,7 @@ in6_get_hw_ifid(struct ifnet *ifp, struc
 
/* make EUI64 address */
if (addrlen == 8)
-   bcopy(addr, >s6_addr[8], 8);
+   memcpy(>s6_addr[8], addr, 8);
else if (addrlen == 6) {
in6->s6_addr[8] = addr[0];
in6->s6_addr[9] = addr[1];
@@ -244,7 +244,7 @@ in6_get_soii_ifid(struct ifnet *ifp, str
SHA512Update(, ip6_soiikey, sizeof(ip6_soiikey));
SHA512Final(digest, );
 
-   bcopy(digest + (sizeof(digest) - 8), >s6_addr[8], 8);
+   memcpy(>s6_addr[8], digest + (sizeof(digest) - 8), 8);
 
return 0;
 }
@@ -464,7 +464,7 @@ in6_nigroup(struct ifnet *ifp, const cha
sa6->sin6_addr.s6_addr16[0] = htons(0xff02);
sa6->sin6_addr.s6_addr16[1] = htons(ifp->if_index);
sa6->sin6_addr.s6_addr8[11] = 2;
-   bcopy(digest, >sin6_addr.s6_addr32[3],
+   memcpy(>sin6_addr.s6_addr32[3], digest,
sizeof(sa6->sin6_addr.s6_addr32[3]));
 
return 0;
Index: netinet6/ip6_output.c
===
RCS file: /cvs/src/sys/netinet6/ip6_output.c,v
retrieving revision 1.234
diff -u -p -r1.234 ip6_output.c
--- netinet6/ip6_output.c   19 Feb 2018 08:59:53 -  1.234
+++ netinet6/ip6_output.c   13 Mar 2018 21:32:33 -
@@ -850,7 +850,7 @@ ip6_copyexthdr(struct mbuf **mp, caddr_t
}
m->m_len = hlen;
if (hdr)
-   bcopy(hdr, mtod(m, caddr_t), hlen);
+   memcpy(mtod(m, caddr_t), hdr, hlen);
 
*mp = m;
return (0);
@@ -918,7 +918,7 @@ ip6_insert_jumboopt(struct ip6_exthdrs *
if (!n)
return (ENOBUFS);
n->m_len = oldoptlen + JUMBOOPTLEN;
-   bcopy(mtod(mopt, caddr_t), mtod(n, caddr_t),
+   memcpy(mtod(n, caddr_t), mtod(mopt, caddr_t),
  oldoptlen);
optbuf = mtod(n, u_int8_t *) + oldoptlen;
m_freem(mopt);

Re: Kernel size beyond 16 MB on amd64

[Stuart Henderson]

On 2018/03/12 17:50, Franco Fichtner wrote:
> Hi,
> 
> With regard to a commit[1] by Theo in 2013, several questions
> in the years before and a partial lift of the limitation on
> i386 a while back (2015?) I'd like to ask what the future plans
> are for OpenBSD.
> 
> Peeking at NetBSD, where the amd64 was bootstrapped, they are at
> 48 MB kernel size at the moment.
> 
> Will a future OpenBSD release increase it?
> 
> What can we do to help?
> 
> 
> Cheers,
> Franco
> 
> --
> [1] https://github.com/openbsd/src/commit/453010f2034
> 

I guess you'd be looking at this for ramdisks, if so, see the vnconfig
method used by flashrd instead.

https://github.com/yellowman/flashrd

Re: Kernel size beyond 16 MB on amd64

[Mike Larkin]

On Tue, Mar 13, 2018 at 04:15:23PM +0100, Franco Fichtner wrote:
> 
> > On 13. Mar 2018, at 4:04 PM, Ted Unangst  wrote:
> > 
> > Franco Fichtner wrote:
> >> What can we do to help?
> > 
> > Write smaller code...
> 
> Fair enough.  ;)
> 
> On a more serious note, I'm referring to:
> 
> https://marc.info/?l=openbsd-tech=112152576800634=2
> 
> "Immediate reboots" is what can still be reproduced
> much under the size that bsd.gdb may offer.
> 
> i386 copes better with this since 2015.  It's odd, so
> I thought I'd ask.
> 
> 
> Cheers,
> Franco
> 

I don't know what you're asking. The thread you linked to is more than 12 years
old and concerns OpenBSD 3.7.

-ml

Re: Kernel size beyond 16 MB on amd64

[Franco Fichtner]

> On 13. Mar 2018, at 4:04 PM, Ted Unangst  wrote:
> 
> Franco Fichtner wrote:
>> What can we do to help?
> 
> Write smaller code...

Fair enough.  ;)

On a more serious note, I'm referring to:

https://marc.info/?l=openbsd-tech=112152576800634=2

"Immediate reboots" is what can still be reproduced
much under the size that bsd.gdb may offer.

i386 copes better with this since 2015.  It's odd, so
I thought I'd ask.


Cheers,
Franco

Re: Kernel size beyond 16 MB on amd64

[Ted Unangst]

Franco Fichtner wrote:
> What can we do to help?

Write smaller code...

Re: correctly calculate RFC7217 based IPv6 address

[Mark Kettenis]

> Date: Tue, 13 Mar 2018 13:49:01 +0100
> From: Peter Hessler 
> 
> On 2018 Mar 13 (Tue) at 12:41:17 +0100 (+0100), Florian Obser wrote:
> :(sending this to tech@ so that more people see this)
> :
> :semarie@ pointed out on bugs@ (
> :https://marc.info/?l=openbsd-bugs=152084960013726=2 ) that his
> :RFC7217 IPv6 address changed after an upgrade. Of course it should not.
> :
> :The reason for that was a mistake in the original implementation:
> :
> :In the original implemntation the bits of the sha512 hash and
> :the IPv6 address aligned like this:
> :
> : 511 447   383   127   63  0
> :+-+-[...]-+-+-[...]-+-+-[...]-+-+-[...]-+-+-[...]-+-+
> :|   512 bit sha512 digest   |
> :+-+-[...]-+-+-[...]-+-+-[...]-+-+-[...]-+-+-[...]-+-+
> :
> : 127 63  0 
> :+-+-[...]-+-+-[...]-+-+
> :|  IPv6 address   |
> :+-+-[...]-+-+-[...]-+-+
> :
> :
> :after phessler's change to support non-64 prefix lenght (rev1.22 of
> :engine.c):
> :
> : 511 447   383   127   63  0
> :+-+-[...]-+-+-[...]-+-+-[...]-+-+-[...]-+-+-[...]-+-+
> :|   512 bit sha512 digest   |
> :+-+-[...]-+-+-[...]-+-+-[...]-+-+-[...]-+-+-[...]-+-+
> :
> : 127 630 
> :+-+-[...]-+-+-[...]-+
> :|  IPv6 address |
> :+-+-[...]-+-+-[...]-+
> :
> :So even with a /64 the bits are shifted and you end up with a
> :different v6 address. This is what semarie observed.
> :
> :
> :In section 5, page 9 RFC 7217 states:
> :
> :   2.  The Interface Identifier is finally obtained by taking as many
> :   bits from the RID value (computed in the previous step) as
> :   necessary, starting from the least significant bit.
> :
> :So it should have looked like this:
> :
> : 511 447   383   127   63  0
> :+-+-[...]-+-+-[...]-+-+-[...]-+-+-[...]-+-+-[...]-+-+
> :|   512 bit sha512 digest   |
> :+-+-[...]-+-+-[...]-+-+-[...]-+-+-[...]-+-+-[...]-+-+
> :
> : 127   63  0 
> :+-+-+-[...]-+-+-[...]-+-+
> :|  IPv6 address |
> :+-+-+-[...]-+-+-[...]-+-+
> :
> :
> :I think we should implement what the RFC says. Unfortunately that
> :means addresses change once more, but things change in current... I
> :will put some wording into current.html. Having this churn now while
> :it's a newly introduced feature is better then in two years time if we
> :discover that we should really implement what the rfc says.
> :
> :OK?
> :
> :If you realy hate this and have a good reason why we should stick with
> :the current algorithm speak up now, I'm going to commit this soonish
> :to not drag out the address change for too long. Also this needs to
> :make 6.3.
> :
> 
> I don't see the point in mandating which part of the hash we use.  The
> RFC allows us to use any algorithm to generate the stable identifier, so
> its not expected to be portable to another implementation.  However, if
> there is a cryptographic reason to prefer the least significant bits
> instead of the most significatnt bits, I'm completely in support.  But as
> I understand it, random data is random, *shrug*.
> 
> That being said, while I don't like shifting this around I'm not opposed
> to it.
> 
> If you want to commit it, OK.

I think there is a clear benefit to match what's written in the RFC
since that makes understanding the code easier for people who are new
to it.

> :diff --git sbin/slaacd/engine.c sbin/slaacd/engine.c
> :index f473e3d0b80..e41a7c31751 100644
> :--- sbin/slaacd/engine.c
> :+++ sbin/slaacd/engine.c
> :@@ -1239,6 +1239,8 @@ gen_addr(struct slaacd_iface *iface, struct 
> radv_prefix *prefix, struct
> : int dad_counter = 0; /* XXX not used */
> : u_int8_t digest[SHA512_DIGEST_LENGTH];
> : 
> :+memset(, 0, sizeof(iid));
> :+
> : /* from in6_ifadd() in nd6_rtr.c */
> : /* XXX from in6.h, guarded by #ifdef _KERNEL   XXX nonstandard */
> : #define s6_addr32 __u6_addr.__u6_addr32
> :@@ -1275,7 +1277,8 @@ gen_addr(struct slaacd_iface *iface, struct 
> radv_prefix *prefix, struct
> : sizeof(addr_proposal->soiikey));
> : SHA512Final(digest, );
> : 
> :-memcpy(_addr, digest, sizeof(iid.s6_addr));
> :+memcpy(_addr, digest + (sizeof(digest) -
> :+sizeof(iid.s6_addr)), sizeof(iid.s6_addr));
> : } else {
> : /* This is safe, because we have a 64 prefix len */
> : memcpy(_addr, >ll_address.sin6_addr,
> :diff --git sys/netinet6/in6_ifattach.c sys/netinet6/in6_ifattach.c
> :index 0aa10fad94b..e2a4ab1dd92 100644
> :--- sys/netinet6/in6_ifattach.c
> :+++ sys/netinet6/in6_ifattach.c
> :@@ -244,7 +244,7 @@ in6_get_soii_ifid(struct ifnet *ifp, struct in6_addr 
> *in6)
> : SHA512Update(, ip6_soiikey, sizeof(ip6_soiikey));
> : SHA512Final(digest, );
> : 
> 

Re: ksh: __func__ in warnings

[Boris Radulov]

> Please disregard my concern about consistency, I was missing something,
> not you

Sorry if it came out as out of place. I just feel like starting to
introduce the macro
in places where the function name is outright wrong might be a good idea,
since
if the function was changed once it makes sense that it might be changed
again.
Then, the macro usage would mitigate another potential error/source of
confusion.
Ultimately, I'm not going to go around changing stuff unless I see
something like this
by accident so it's not my decision to make at all. I don't think
introducing this diff
would do any harm though.

On Tue, Mar 13, 2018 at 2:44 PM, Klemens Nanni  wrote:

> On Tue, Mar 13, 2018 at 12:33:36PM +0100, Klemens Nanni wrote:
> > On Tue, Mar 13, 2018 at 04:39:16PM +0800, Michael W. Bombardieri wrote:
> > > Some errors and warnings printed by ksh have the function name
> > > prefixed. __func__ could be used here instead of hard-coding
> > > the name. The names are wrong for tty_init(), j_set_async(),
> > > j_change(), x_file_glob() and c_ulimit() afaics.
> > Wrong error messages should definitely be corrected, although I'd either
> > fix them literally or use __func__ consistently. This diff would
> > introduce the macro for only a handful of functions while leaving the
> > vast majority names untouched.
> >
> > Not sure if touching all error messages for __func__ is worth it or just
> > too much churn in the end.
> Please disregard my concern about consistency, I was missing someting,
> not you.
>
>


-- 
Best Regards,
Bobby

Re: correctly calculate RFC7217 based IPv6 address

[Peter Hessler]

On 2018 Mar 13 (Tue) at 12:41:17 +0100 (+0100), Florian Obser wrote:
:(sending this to tech@ so that more people see this)
:
:semarie@ pointed out on bugs@ (
:https://marc.info/?l=openbsd-bugs=152084960013726=2 ) that his
:RFC7217 IPv6 address changed after an upgrade. Of course it should not.
:
:The reason for that was a mistake in the original implementation:
:
:In the original implemntation the bits of the sha512 hash and
:the IPv6 address aligned like this:
:
: 511 447   383   127   63  0
:+-+-[...]-+-+-[...]-+-+-[...]-+-+-[...]-+-+-[...]-+-+
:|   512 bit sha512 digest   |
:+-+-[...]-+-+-[...]-+-+-[...]-+-+-[...]-+-+-[...]-+-+
:
: 127 63  0 
:+-+-[...]-+-+-[...]-+-+
:|  IPv6 address   |
:+-+-[...]-+-+-[...]-+-+
:
:
:after phessler's change to support non-64 prefix lenght (rev1.22 of
:engine.c):
:
: 511 447   383   127   63  0
:+-+-[...]-+-+-[...]-+-+-[...]-+-+-[...]-+-+-[...]-+-+
:|   512 bit sha512 digest   |
:+-+-[...]-+-+-[...]-+-+-[...]-+-+-[...]-+-+-[...]-+-+
:
: 127 630 
:+-+-[...]-+-+-[...]-+
:|  IPv6 address |
:+-+-[...]-+-+-[...]-+
:
:So even with a /64 the bits are shifted and you end up with a
:different v6 address. This is what semarie observed.
:
:
:In section 5, page 9 RFC 7217 states:
:
:   2.  The Interface Identifier is finally obtained by taking as many
:   bits from the RID value (computed in the previous step) as
:   necessary, starting from the least significant bit.
:
:So it should have looked like this:
:
: 511 447   383   127   63  0
:+-+-[...]-+-+-[...]-+-+-[...]-+-+-[...]-+-+-[...]-+-+
:|   512 bit sha512 digest   |
:+-+-[...]-+-+-[...]-+-+-[...]-+-+-[...]-+-+-[...]-+-+
:
: 127   63  0 
:+-+-+-[...]-+-+-[...]-+-+
:|  IPv6 address |
:+-+-+-[...]-+-+-[...]-+-+
:
:
:I think we should implement what the RFC says. Unfortunately that
:means addresses change once more, but things change in current... I
:will put some wording into current.html. Having this churn now while
:it's a newly introduced feature is better then in two years time if we
:discover that we should really implement what the rfc says.
:
:OK?
:
:If you realy hate this and have a good reason why we should stick with
:the current algorithm speak up now, I'm going to commit this soonish
:to not drag out the address change for too long. Also this needs to
:make 6.3.
:

I don't see the point in mandating which part of the hash we use.  The
RFC allows us to use any algorithm to generate the stable identifier, so
its not expected to be portable to another implementation.  However, if
there is a cryptographic reason to prefer the least significant bits
instead of the most significatnt bits, I'm completely in support.  But as
I understand it, random data is random, *shrug*.

That being said, while I don't like shifting this around I'm not opposed
to it.

If you want to commit it, OK.


:diff --git sbin/slaacd/engine.c sbin/slaacd/engine.c
:index f473e3d0b80..e41a7c31751 100644
:--- sbin/slaacd/engine.c
:+++ sbin/slaacd/engine.c
:@@ -1239,6 +1239,8 @@ gen_addr(struct slaacd_iface *iface, struct radv_prefix 
*prefix, struct
:   int dad_counter = 0; /* XXX not used */
:   u_int8_t digest[SHA512_DIGEST_LENGTH];
: 
:+  memset(, 0, sizeof(iid));
:+
:   /* from in6_ifadd() in nd6_rtr.c */
:   /* XXX from in6.h, guarded by #ifdef _KERNEL   XXX nonstandard */
: #define s6_addr32 __u6_addr.__u6_addr32
:@@ -1275,7 +1277,8 @@ gen_addr(struct slaacd_iface *iface, struct radv_prefix 
*prefix, struct
:   sizeof(addr_proposal->soiikey));
:   SHA512Final(digest, );
: 
:-  memcpy(_addr, digest, sizeof(iid.s6_addr));
:+  memcpy(_addr, digest + (sizeof(digest) -
:+  sizeof(iid.s6_addr)), sizeof(iid.s6_addr));
:   } else {
:   /* This is safe, because we have a 64 prefix len */
:   memcpy(_addr, >ll_address.sin6_addr,
:diff --git sys/netinet6/in6_ifattach.c sys/netinet6/in6_ifattach.c
:index 0aa10fad94b..e2a4ab1dd92 100644
:--- sys/netinet6/in6_ifattach.c
:+++ sys/netinet6/in6_ifattach.c
:@@ -244,7 +244,7 @@ in6_get_soii_ifid(struct ifnet *ifp, struct in6_addr *in6)
:   SHA512Update(, ip6_soiikey, sizeof(ip6_soiikey));
:   SHA512Final(digest, );
: 
:-  bcopy(digest, >s6_addr[8], 8);
:+  bcopy(digest + (sizeof(digest) - 8), >s6_addr[8], 8);
: 
:   return 0;
: }


-- 
The revolution will not be televised.

Re: ksh: __func__ in warnings

[Klemens Nanni]

On Tue, Mar 13, 2018 at 12:33:36PM +0100, Klemens Nanni wrote:
> On Tue, Mar 13, 2018 at 04:39:16PM +0800, Michael W. Bombardieri wrote:
> > Some errors and warnings printed by ksh have the function name
> > prefixed. __func__ could be used here instead of hard-coding
> > the name. The names are wrong for tty_init(), j_set_async(),
> > j_change(), x_file_glob() and c_ulimit() afaics.
> Wrong error messages should definitely be corrected, although I'd either
> fix them literally or use __func__ consistently. This diff would
> introduce the macro for only a handful of functions while leaving the
> vast majority names untouched.
> 
> Not sure if touching all error messages for __func__ is worth it or just
> too much churn in the end.
Please disregard my concern about consistency, I was missing someting,
not you.

Re: SSL_TXT_SSLV2

[Jeremie Courreges-Anglas]

On Sat, Mar 10 2018, Stuart Henderson  wrote:
> mail/dovecot's default config has a problem because SSL_TXT_SSLV2
> is defined but SSLv2 is not allowed in a protocol string. End result
> is that unless you specify your own ssl_protocols line, Dovecot will
> start but client connections will fail. (I ran into this after updating
> an oldish mail server).
>
> dovecot: src/lib-master/master-service-ssl-settings.c
>  42 static const struct master_service_ssl_settings 
> master_service_ssl_default_settings = {
>  43 #ifdef HAVE_SSL
>  44 .ssl = "yes:no:required",
>  45 #else
>  46 .ssl = "no:yes:required",
>  47 #endif
>  48 .ssl_ca = "",
>  49 .ssl_cert = "",
>  50 .ssl_key = "",
>  51 .ssl_alt_cert = "",
>  52 .ssl_alt_key = "",
>  53 .ssl_key_password = "",
>  54 .ssl_cipher_list = "ALL:!LOW:!SSLv2:!EXP:!aNULL",
>  55 #ifdef SSL_TXT_SSLV2
>  56 .ssl_protocols = "!SSLv2 !SSLv3",
>  57 #else
>  58 .ssl_protocols = "!SSLv3",
>  59 #endif
>  60 .ssl_cert_username_field = "commonName",
>  61 .ssl_crypto_device = "",
>  62 .ssl_verify_client_cert = FALSE,
>  63 .ssl_require_crl = TRUE,
>  64 .verbose_ssl = FALSE,
>  65 .ssl_prefer_server_ciphers = FALSE,
>  66 .ssl_options = "",
>  67 };
>
> Looks like there's something related in mail/kopano/core.
>
> SSL_TXT_SSLV2 isn't used anywhere in our tree and looking at Debian
> codesearch results I think it's safe if we just drop the define as
> OpenSSL has also done in 1.1. (I don't think the same is possible for
> SSL_TXT_SSLV3 without causing churn).
>
> Alternatively we could patch the ports, but there doesn't seem much
> point in that. (Obviously those ports would still need REVISION bumps
> in order that users get updated).
>
> OK?

I don't see the point of keeping it.  The code in kopano seems to be
able to cope.  ok jca@

> Index: lib/libssl/ssl.h
> ===
> RCS file: /cvs/src/lib/libssl/ssl.h,v
> retrieving revision 1.146
> diff -u -p -r1.146 ssl.h
> --- lib/libssl/ssl.h  3 Mar 2018 19:58:29 -   1.146
> +++ lib/libssl/ssl.h  10 Mar 2018 11:18:16 -
> @@ -300,7 +300,6 @@ extern "C" {
>  #define SSL_TXT_STREEBOG512  "STREEBOG512"
>  
>  #define SSL_TXT_DTLS1"DTLSv1"
> -#define SSL_TXT_SSLV2"SSLv2"
>  #define SSL_TXT_SSLV3"SSLv3"
>  #define SSL_TXT_TLSV1"TLSv1"
>  #define SSL_TXT_TLSV1_1  "TLSv1.1"
>
>

-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Re: ksh: __func__ in warnings

[Boris Radulov]

 >Not sure if touching all error messages for __func__ is worth it or just
>too much churn in the end.

He already converted some so I don't see a reason to not introduce at least
that diff. If someone comes across something, he could just fix it then and
there if this issue is well-known.

On Tue, Mar 13, 2018 at 1:33 PM, Klemens Nanni  wrote:

> On Tue, Mar 13, 2018 at 04:39:16PM +0800, Michael W. Bombardieri wrote:
> > Some errors and warnings printed by ksh have the function name
> > prefixed. __func__ could be used here instead of hard-coding
> > the name. The names are wrong for tty_init(), j_set_async(),
> > j_change(), x_file_glob() and c_ulimit() afaics.
> Wrong error messages should definitely be corrected, although I'd either
> fix them literally or use __func__ consistently. This diff would
> introduce the macro for only a handful of functions while leaving the
> vast majority names untouched.
>
> Not sure if touching all error messages for __func__ is worth it or just
> too much churn in the end.
>
>


-- 
Best Regards,
Bobby

correctly calculate RFC7217 based IPv6 address

[Florian Obser]

(sending this to tech@ so that more people see this)

semarie@ pointed out on bugs@ (
https://marc.info/?l=openbsd-bugs=152084960013726=2 ) that his
RFC7217 IPv6 address changed after an upgrade. Of course it should not.

The reason for that was a mistake in the original implementation:

In the original implemntation the bits of the sha512 hash and
the IPv6 address aligned like this:

 511 447   383   127   63  0
+-+-[...]-+-+-[...]-+-+-[...]-+-+-[...]-+-+-[...]-+-+
|   512 bit sha512 digest   |
+-+-[...]-+-+-[...]-+-+-[...]-+-+-[...]-+-+-[...]-+-+

 127 63  0 
+-+-[...]-+-+-[...]-+-+
|  IPv6 address   |
+-+-[...]-+-+-[...]-+-+


after phessler's change to support non-64 prefix lenght (rev1.22 of
engine.c):

 511 447   383   127   63  0
+-+-[...]-+-+-[...]-+-+-[...]-+-+-[...]-+-+-[...]-+-+
|   512 bit sha512 digest   |
+-+-[...]-+-+-[...]-+-+-[...]-+-+-[...]-+-+-[...]-+-+

 127 630 
+-+-[...]-+-+-[...]-+
|  IPv6 address |
+-+-[...]-+-+-[...]-+

So even with a /64 the bits are shifted and you end up with a
different v6 address. This is what semarie observed.


In section 5, page 9 RFC 7217 states:

   2.  The Interface Identifier is finally obtained by taking as many
   bits from the RID value (computed in the previous step) as
   necessary, starting from the least significant bit.

So it should have looked like this:

 511 447   383   127   63  0
+-+-[...]-+-+-[...]-+-+-[...]-+-+-[...]-+-+-[...]-+-+
|   512 bit sha512 digest   |
+-+-[...]-+-+-[...]-+-+-[...]-+-+-[...]-+-+-[...]-+-+

 127   63  0 
+-+-+-[...]-+-+-[...]-+-+
|  IPv6 address |
+-+-+-[...]-+-+-[...]-+-+


I think we should implement what the RFC says. Unfortunately that
means addresses change once more, but things change in current... I
will put some wording into current.html. Having this churn now while
it's a newly introduced feature is better then in two years time if we
discover that we should really implement what the rfc says.

OK?

If you realy hate this and have a good reason why we should stick with
the current algorithm speak up now, I'm going to commit this soonish
to not drag out the address change for too long. Also this needs to
make 6.3.

diff --git sbin/slaacd/engine.c sbin/slaacd/engine.c
index f473e3d0b80..e41a7c31751 100644
--- sbin/slaacd/engine.c
+++ sbin/slaacd/engine.c
@@ -1239,6 +1239,8 @@ gen_addr(struct slaacd_iface *iface, struct radv_prefix 
*prefix, struct
int dad_counter = 0; /* XXX not used */
u_int8_t digest[SHA512_DIGEST_LENGTH];
 
+   memset(, 0, sizeof(iid));
+
/* from in6_ifadd() in nd6_rtr.c */
/* XXX from in6.h, guarded by #ifdef _KERNEL   XXX nonstandard */
 #define s6_addr32 __u6_addr.__u6_addr32
@@ -1275,7 +1277,8 @@ gen_addr(struct slaacd_iface *iface, struct radv_prefix 
*prefix, struct
sizeof(addr_proposal->soiikey));
SHA512Final(digest, );
 
-   memcpy(_addr, digest, sizeof(iid.s6_addr));
+   memcpy(_addr, digest + (sizeof(digest) -
+   sizeof(iid.s6_addr)), sizeof(iid.s6_addr));
} else {
/* This is safe, because we have a 64 prefix len */
memcpy(_addr, >ll_address.sin6_addr,
diff --git sys/netinet6/in6_ifattach.c sys/netinet6/in6_ifattach.c
index 0aa10fad94b..e2a4ab1dd92 100644
--- sys/netinet6/in6_ifattach.c
+++ sys/netinet6/in6_ifattach.c
@@ -244,7 +244,7 @@ in6_get_soii_ifid(struct ifnet *ifp, struct in6_addr *in6)
SHA512Update(, ip6_soiikey, sizeof(ip6_soiikey));
SHA512Final(digest, );
 
-   bcopy(digest, >s6_addr[8], 8);
+   bcopy(digest + (sizeof(digest) - 8), >s6_addr[8], 8);
 
return 0;
 }


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

Re: ksh: __func__ in warnings

[Klemens Nanni]

On Tue, Mar 13, 2018 at 04:39:16PM +0800, Michael W. Bombardieri wrote:
> Some errors and warnings printed by ksh have the function name
> prefixed. __func__ could be used here instead of hard-coding
> the name. The names are wrong for tty_init(), j_set_async(),
> j_change(), x_file_glob() and c_ulimit() afaics.
Wrong error messages should definitely be corrected, although I'd either
fix them literally or use __func__ consistently. This diff would
introduce the macro for only a handful of functions while leaving the
vast majority names untouched.

Not sure if touching all error messages for __func__ is worth it or just
too much churn in the end.

ksh: __func__ in warnings

[Michael W. Bombardieri]

Hello,

Some errors and warnings printed by ksh have the function name
prefixed. __func__ could be used here instead of hard-coding
the name. The names are wrong for tty_init(), j_set_async(),
j_change(), x_file_glob() and c_ulimit() afaics.

- Michael


Index: c_ksh.c
===
RCS file: /cvs/src/bin/ksh/c_ksh.c,v
retrieving revision 1.58
diff -u -p -u -r1.58 c_ksh.c
--- c_ksh.c 16 Jan 2018 22:52:32 -  1.58
+++ c_ksh.c 13 Mar 2018 08:16:37 -
@@ -1273,7 +1273,7 @@ c_getopts(char **wp)
}
 
if (genv->loc->next == NULL) {
-   internal_warningf("c_getopts: no argv");
+   internal_warningf("%s: no argv", __func__);
return 1;
}
/* Which arguments are we parsing... */
Index: c_ulimit.c
===
RCS file: /cvs/src/bin/ksh/c_ulimit.c,v
retrieving revision 1.26
diff -u -p -u -r1.26 c_ulimit.c
--- c_ulimit.c  16 Jan 2018 22:52:32 -  1.26
+++ c_ulimit.c  13 Mar 2018 08:16:37 -
@@ -97,7 +97,7 @@ c_ulimit(char **wp)
for (l = limits; l->name && l->option != optc; l++)
;
if (!l->name) {
-   internal_warningf("ulimit: %c", optc);
+   internal_warningf("%s: %c", __func__, optc);
return 1;
}
if (builtin_opt.optarg) {
Index: edit.c
===
RCS file: /cvs/src/bin/ksh/edit.c,v
retrieving revision 1.63
diff -u -p -u -r1.63 edit.c
--- edit.c  16 Jan 2018 22:52:32 -  1.63
+++ edit.c  13 Mar 2018 08:16:37 -
@@ -372,7 +372,7 @@ x_file_glob(int flags, const char *str, 
source = s;
if (yylex(ONEWORD|UNESCAPE) != LWORD) {
source = sold;
-   internal_warningf("fileglob: substitute error");
+   internal_warningf("%s: substitute error", __func__);
return 0;
}
source = sold;
Index: exec.c
===
RCS file: /cvs/src/bin/ksh/exec.c,v
retrieving revision 1.72
diff -u -p -u -r1.72 exec.c
--- exec.c  16 Jan 2018 22:52:32 -  1.72
+++ exec.c  13 Mar 2018 08:16:37 -
@@ -727,7 +727,7 @@ shcomexec(char **wp)
 
tp = ktsearch(, *wp, hash(*wp));
if (tp == NULL)
-   internal_errorf("shcomexec: %s", *wp);
+   internal_errorf("%s: %s", __func__, *wp);
return call_builtin(tp, wp);
 }
 
@@ -1221,7 +1221,7 @@ herein(const char *content, int sub)
s->start = s->str = content;
source = s;
if (yylex(ONEWORD|HEREDOC) != LWORD)
-   internal_errorf("herein: yylex");
+   internal_errorf("%s: yylex", __func__);
source = osource;
shf_puts(evalstr(yylval.cp, 0), shf);
} else
@@ -1446,5 +1446,5 @@ static void
 dbteste_error(Test_env *te, int offset, const char *msg)
 {
te->flags |= TEF_ERROR;
-   internal_warningf("dbteste_error: %s (offset %d)", msg, offset);
+   internal_warningf("%s: %s (offset %d)", __func__, msg, offset);
 }
Index: jobs.c
===
RCS file: /cvs/src/bin/ksh/jobs.c,v
retrieving revision 1.59
diff -u -p -u -r1.59 jobs.c
--- jobs.c  16 Jan 2018 22:52:32 -  1.59
+++ jobs.c  13 Mar 2018 08:16:38 -
@@ -200,13 +200,13 @@ j_suspend(void)
if (restore_ttypgrp >= 0) {
if (tcsetpgrp(tty_fd, restore_ttypgrp) < 0) {
warningf(false,
-   "j_suspend: tcsetpgrp() failed: %s",
-   strerror(errno));
+   "%s: tcsetpgrp() failed: %s",
+   __func__, strerror(errno));
} else {
if (setpgid(0, restore_ttypgrp) < 0) {
warningf(false,
-   "j_suspend: setpgid() failed: %s",
-   strerror(errno));
+   "%s: setpgid() failed: %s",
+   __func__, strerror(errno));
}
}
}
@@ -225,14 +225,14 @@ j_suspend(void)
if (restore_ttypgrp >= 0) {
if (setpgid(0, kshpid) < 0) {
warningf(false,
-   "j_suspend: setpgid() failed: %s",
-   strerror(errno));
+   "%s: setpgid()