Re: netstart: implicit up and explicit down for hostname.if conf files
No. I don't see any study of who this affects negatively.
interface queue transmit mitigation (again)
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->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->ifq_bundle); + ifq_run_start(ifq); + } else + task_add(ifq->ifq_softnet, &ifq->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, &c); + NET_ASSERT_UNLOCKED(); + + if (!task_del(ifq->ifq_softnet, &ifq->ifq_bundle)) { + int netlocked = (rw_status(&netlock) == 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->ifq_mtx, IPL_NET); @@ -187,6 +227,7 @@ ifq_init(struct ifqueue *ifq, struct ifn mtx_init(&ifq->ifq_task_mtx, IPL_NET); TAILQ_INIT(&ifq->ifq_task_list); ifq->ifq_serializer = NULL; + task_set(&ifq->ifq_bundle, ifq_bundle_task, ifq); task_set(&ifq->ifq_start, ifq_start_task, ifq); task_set(&ifq->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->ifq_start); } static inline void
Re: netstart: implicit up and explicit down for hostname.if conf files
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
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, &port, &path)) == 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, &port, &path); > + 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
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
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)&nip6, mtod(m, caddr_t), sizeof(nip6)); + memcpy(mtod(m, caddr_t), (caddr_t)&nip6, 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(&sin6, sizeof(sin6)); sin6.sin6_family = AF_INET6; sin6.sin6_len = sizeof(struct sockaddr_in6); - bcopy(&reddst6, &sin6.sin6_addr, sizeof(reddst6)); + memcpy(&sin6.sin6_addr, &reddst6, sizeof(reddst6)); rt = rtalloc(sin6tosa(&sin6), 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(&redtgt6, &sgw.sin6_addr, sizeof(struct in6_addr)); - bcopy(&reddst6, &sdst.sin6_addr, sizeof(struct in6_addr)); - bcopy(&src6, &ssrc.sin6_addr, sizeof(struct in6_addr)); + memcpy(&sgw.sin6_addr, &redtgt6, sizeof(struct in6_addr)); + memcpy(&sdst.sin6_addr, &reddst6, sizeof(struct in6_addr)); + memcpy(&ssrc.sin6_addr, &src6, sizeof(struct in6_addr)); rtredirect(sin6tosa(&sdst), sin6tosa(&sgw), sin6tosa(&ssrc), &newrt, m->m_pkthdr.ph_rtableid); @@ -1395,7 +1395,7 @@ icmp6_redirect_input(struct mbuf *m, int bzero(&sdst, sizeof(sdst)); sdst.sin6_family = AF_INET6; sdst.sin6_len = sizeof(struct sockaddr_in6); - bcopy(&reddst6, &sdst.sin6_addr, sizeof(struct in6_addr)); + memcpy(&sdst.sin6_addr, &reddst6, sizeof(struct in6_addr)); pfctlinput(PRC_REDIRECT_HOST, sin6tosa(&sdst)); } 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, &in6->s6_addr[8], 8); + memcpy(&in6->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(&ctx, ip6_soiikey, sizeof(ip6_soiikey)); SHA512Final(digest, &ctx); - bcopy(digest + (sizeof(digest) - 8), &in6->s6_addr[8], 8); + memcpy(&in6->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, &sa6->sin6_addr.s6_addr32[3], + memcpy(&sa6->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);
Re: Kernel size beyond 16 MB on amd64
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
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&m=112152576800634&w=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
> 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&m=112152576800634&w=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
Franco Fichtner wrote: > What can we do to help? Write smaller code...
Re: correctly calculate RFC7217 based IPv6 address
> 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&m=152084960013726&w=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(&iid, 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, &ctx); > : > :-memcpy(&iid.s6_addr, digest, sizeof(iid.s6_addr)); > :+memcpy(&iid.s6_addr, digest + (sizeof(digest) - > :+sizeof(iid.s6_addr)), sizeof(iid.s6_addr)); > : } else { > : /* This is safe, because we have a 64 prefix len */ > : memcpy(&iid.s6_addr, &iface->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(&ctx, ip6_soiikey, sizeof(ip6_soiikey)); > : SHA512Fina
Re: ksh: __func__ in warnings
> 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
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&m=152084960013726&w=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(&iid, 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, &ctx); : :- memcpy(&iid.s6_addr, digest, sizeof(iid.s6_addr)); :+ memcpy(&iid.s6_addr, digest + (sizeof(digest) - :+ sizeof(iid.s6_addr)), sizeof(iid.s6_addr)); : } else { : /* This is safe, because we have a 64 prefix len */ : memcpy(&iid.s6_addr, &iface->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(&ctx, ip6_soiikey, sizeof(ip6_soiikey)); : SHA512Final(digest, &ctx); : :- bcopy(digest, &in6->s6_addr[8], 8); :+ bcopy(digest + (sizeof(digest) - 8), &in6->s6_addr[8], 8); : : return 0; : } -- The revolution will not be televised.
Re: ksh: __func__ in warnings
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
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
>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
(sending this to tech@ so that more people see this) semarie@ pointed out on bugs@ ( https://marc.info/?l=openbsd-bugs&m=152084960013726&w=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(&iid, 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, &ctx); - memcpy(&iid.s6_addr, digest, sizeof(iid.s6_addr)); + memcpy(&iid.s6_addr, digest + (sizeof(digest) - + sizeof(iid.s6_addr)), sizeof(iid.s6_addr)); } else { /* This is safe, because we have a 64 prefix len */ memcpy(&iid.s6_addr, &iface->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(&ctx, ip6_soiikey, sizeof(ip6_soiikey)); SHA512Final(digest, &ctx); - bcopy(digest, &in6->s6_addr[8], 8); + bcopy(digest + (sizeof(digest) - 8), &in6->s6_addr[8], 8); return 0; } -- I'm not entirely sure you are real.
Re: ksh: __func__ in warnings
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
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(&builtins, *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: setpg