dont need splvm in pool_gc_pages

[David Gwynne]

jmatthew@ noticed that i still had this code in here, despite making
it mandatory for pools to specify the IPL they operate at.

ok?

Index: subr_pool.c
===
RCS file: /cvs/src/sys/kern/subr_pool.c,v
retrieving revision 1.205
diff -u -p -r1.205 subr_pool.c
--- subr_pool.c 24 Jan 2017 09:54:41 -  1.205
+++ subr_pool.c 7 Feb 2017 07:25:19 -
@@ -1446,10 +1446,8 @@ pool_gc_pages(void *null)
 {
struct pool *pp;
struct pool_page_header *ph, *freeph;
-   int s;
 
rw_enter_read(_lock);
-   s = splvm(); /* XXX go to splvm until all pools _setipl properly */
SIMPLEQ_FOREACH(pp, _head, pr_poollist) {
if (pp->pr_nidle <= pp->pr_minpages || /* guess */
!mtx_enter_try(>pr_mtx)) /* try */
@@ -1469,7 +1467,6 @@ pool_gc_pages(void *null)
if (freeph != NULL)
pool_p_free(pp, freeph);
}
-   splx(s);
rw_exit_read(_lock);
 
timeout_add_sec(_gc_tick, 1);

use mac address from fdt on raspberry pi

[Jonathan Gray]

The videocore part of the raspberry pi that boots the system and runs
the mailbox interface and other functions afterwards knows about a MAC
address that appears to be derived from a portion of a unique serial
number along with the raspberry pi foundation oui

B827EB (base 16)Raspberry Pi Foundation

It modifies the device tree when booting to store the MAC address
in /axi/usb/hub/ethernet/mac-address so fetch and use that value
for the integrated smsc(4) Ethernet.

A different smsc adapter plugged into one of the USB ports
probes later and skips this path.

Index: if_smsc.c
===
RCS file: /cvs/src/sys/dev/usb/if_smsc.c,v
retrieving revision 1.29
diff -u -p -r1.29 if_smsc.c
--- if_smsc.c   22 Jan 2017 10:17:39 -  1.29
+++ if_smsc.c   7 Feb 2017 02:51:13 -
@@ -176,6 +176,32 @@ const struct cfattach smsc_ca = {
sizeof(struct smsc_softc), smsc_match, smsc_attach, smsc_detach,
 };
 
+#if defined(__arm__) || defined(__arm64__)
+
+#include 
+
+void
+smsc_enaddr_OF(struct smsc_softc *sc)
+{
+   int node;
+
+   if (sc->sc_dev.dv_unit != 0)
+   return;
+
+   /*
+* Get the Raspberry Pi MAC address from FDT
+* also available via mailbox interface
+*/
+   if ((node = OF_finddevice("/axi/usb/hub/ethernet")) == -1)
+   return;
+
+   OF_getprop(node, "mac-address", sc->sc_ac.ac_enaddr,
+   sizeof(sc->sc_ac.ac_enaddr));
+}
+#else
+#define smsc_enaddr_OF(x) do {} while(0)
+#endif
+
 int
 smsc_read_reg(struct smsc_softc *sc, uint32_t off, uint32_t *data)
 {
@@ -993,6 +1019,8 @@ smsc_attach(struct device *parent, struc
sc->sc_ac.ac_enaddr[1] = (uint8_t)((mac_l >> 8) & 0xff);
sc->sc_ac.ac_enaddr[0] = (uint8_t)((mac_l) & 0xff);
}
+
+   smsc_enaddr_OF(sc);

printf("%s: address %s\n", sc->sc_dev.dv_xname,
ether_sprintf(sc->sc_ac.ac_enaddr));

Re: specify curves via ecdhe statement in httpd.conf

[Andreas Bartelt]

On 02/06/17 14:44, Joel Sing wrote:

On Sunday 05 February 2017 17:05:40 Andreas Bartelt wrote:

- What type of public certificate are you using (RSA or ECDSA)?


ECDSA with P-256. Certificate signed by letsencrypt (via RSA).
Must-staple is enabled - that's why I'm also using the ocsp line for
testing.


Ah, this was the missing piece of information.

In order to use ECDSA the client must support the curve used for the server
certificate, otherwise when the server signs the server key exchange, the
client will not be able to verify the signature. In the case where you
announce that the client only supports P-384, any ECDSA ciphers are considered
to be invalid for this session, effectively resulting in no shared ciphers and
the handshake failure alert.



Yes, right - thanks. I wasn't aware that this is actually a MUST 
requirement from RFC 4492. I'm quite surprised that the "Supported 
Elliptic Curves Extension" is also used in order to specify any allowed 
curves for use in the context of certificates. I think this is quite 
inconsistent but it's what this RFC seems to mandate. It's inconsistent 
because there is no such binding with regard to -ECDHE-RSA- or -DHE-RSA- 
cipher suites.


I've successfully tested a P-384-based certificate which allowed me to 
explicitly specify -groups secp384r1 for ECDHE on the client side.



In order for this configuration to work you need to include P-256 in the client
supported groups. Specifying groups as "P-384:P-256" should still get you
P-384, depending on the server configuration and whether the preference for a
curve is based on the client or server preference (for libtls and hence httpd,
it will be server preference).



Test 1 (with P-384-based certificate): httpd/libtls's preference seems 
to be hard-coded with the following order: X25519, P-256, P-384. In this 
specific case, the httpd's default tls configuration negotiates 
ECDHE-ECDSA-AES256-GCM-SHA384 which has a target security level of 256 
bits. I suppose P-384 would be the preferred choice for ECDHE in this 
case since the certificate also uses P-384 the other two group choices 
effectively lower the overall security level to ~128 bits? [also see RFC 
7919 for a similar discussion wrt RSA key size and FFDHE parameter choice]


Test 2 (with P-256-based certificate): since httpd's preference is 
hard-coded and P-256 must be specified due to the certificate's public 
key, P-384 is actually never selected for ECDHE - this was what I 
initially noticed with regard to the ecdhe "auto" option. For example, 
secp384r1:prime256v1:X25519 effectively selects X25519 for ECDHE.


I think it would be beneficial to allow to explicitly specify multiple 
groups with the ecdhe statement in httpd.conf, and also to respect their 
order.

Re: tcpstat percpu counters

[Jeremie Courreges-Anglas]

Jeremie Courreges-Anglas  writes:

[...]

I've removed the TCPSTAT_INLINE code.  If we want to experiment with
inlined/not inlined counters, it is probably more interesting to make
that choice elsewhere.

> Anyway, tcpstat diff below, a bit ugly because that struct is a mix of
> uint32_t and uint64_t.  Comments / reviews welcome.

Revised after the change in the counters api.  Passed make release on
amd64, runtime tested on GENERIC armv7 and GENERIC.MP amd64.


Index: net/pf.c
===
RCS file: /d/cvs/src/sys/net/pf.c,v
retrieving revision 1.1014
diff -u -p -r1.1014 pf.c
--- net/pf.c5 Feb 2017 16:04:14 -   1.1014
+++ net/pf.c6 Feb 2017 07:26:32 -
@@ -6026,7 +6026,7 @@ pf_check_tcp_cksum(struct mbuf *m, int o
}
 
/* need to do it in software */
-   tcpstat.tcps_inswcsum++;
+   tcpstat_inc(tcps_inswcsum);
 
switch (af) {
case AF_INET:
@@ -6047,7 +6047,7 @@ pf_check_tcp_cksum(struct mbuf *m, int o
unhandled_af(af);
}
if (sum) {
-   tcpstat.tcps_rcvbadsum++;
+   tcpstat_inc(tcps_rcvbadsum);
m->m_pkthdr.csum_flags |= M_TCP_CSUM_IN_BAD;
return (1);
}
Index: netinet/ip_output.c
===
RCS file: /d/cvs/src/sys/netinet/ip_output.c,v
retrieving revision 1.335
diff -u -p -r1.335 ip_output.c
--- netinet/ip_output.c 1 Feb 2017 20:59:47 -   1.335
+++ netinet/ip_output.c 5 Feb 2017 16:27:51 -
@@ -1789,7 +1789,7 @@ in_proto_cksum_out(struct mbuf *m, struc
if (m->m_pkthdr.csum_flags & M_TCP_CSUM_OUT) {
if (!ifp || !(ifp->if_capabilities & IFCAP_CSUM_TCPv4) ||
ip->ip_hl != 5 || ifp->if_bridgeport != NULL) {
-   tcpstat.tcps_outswcsum++;
+   tcpstat_inc(tcps_outswcsum);
in_delayed_cksum(m);
m->m_pkthdr.csum_flags &= ~M_TCP_CSUM_OUT; /* Clear */
}
Index: netinet/tcp_input.c
===
RCS file: /d/cvs/src/sys/netinet/tcp_input.c,v
retrieving revision 1.337
diff -u -p -r1.337 tcp_input.c
--- netinet/tcp_input.c 29 Jan 2017 19:58:47 -  1.337
+++ netinet/tcp_input.c 31 Jan 2017 13:33:00 -
@@ -234,7 +234,7 @@ tcp_reass(struct tcpcb *tp, struct tcphd
if (tiqe == NULL || th->th_seq != tp->rcv_nxt) {
/* Flush segment queue for this connection */
tcp_freeq(tp);
-   tcpstat.tcps_rcvmemdrop++;
+   tcpstat_inc(tcps_rcvmemdrop);
m_freem(m);
return (0);
}
@@ -261,8 +261,8 @@ tcp_reass(struct tcpcb *tp, struct tcphd
i = phdr->th_seq + phdr->th_reseqlen - th->th_seq;
if (i > 0) {
if (i >= *tlen) {
-   tcpstat.tcps_rcvduppack++;
-   tcpstat.tcps_rcvdupbyte += *tlen;
+   tcpstat_pkt(tcps_rcvduppack, tcps_rcvdupbyte,
+   *tlen);
m_freem(m);
pool_put(_pool, tiqe);
return (0);
@@ -272,8 +272,7 @@ tcp_reass(struct tcpcb *tp, struct tcphd
th->th_seq += i;
}
}
-   tcpstat.tcps_rcvoopack++;
-   tcpstat.tcps_rcvoobyte += *tlen;
+   tcpstat_pkt(tcps_rcvoopack, tcps_rcvoobyte, *tlen);
 
/*
 * While we overlap succeeding segments trim them or,
@@ -389,7 +388,7 @@ tcp_input(struct mbuf **mp, int *offp, i
u_char iptos;
 #endif
 
-   tcpstat.tcps_rcvtotal++;
+   tcpstat_inc(tcps_rcvtotal);
 
opti.ts_present = 0;
opti.maxseg = 0;
@@ -448,7 +447,7 @@ tcp_input(struct mbuf **mp, int *offp, i
 
IP6_EXTHDR_GET(th, struct tcphdr *, m, iphlen, sizeof(*th));
if (!th) {
-   tcpstat.tcps_rcvshort++;
+   tcpstat_inc(tcps_rcvshort);
return IPPROTO_DONE;
}
 
@@ -508,10 +507,10 @@ tcp_input(struct mbuf **mp, int *offp, i
int sum;
 
if (m->m_pkthdr.csum_flags & M_TCP_CSUM_IN_BAD) {
-   tcpstat.tcps_rcvbadsum++;
+   tcpstat_inc(tcps_rcvbadsum);
goto drop;
}
-   tcpstat.tcps_inswcsum++;
+   tcpstat_inc(tcps_inswcsum);
switch (af) {
case AF_INET:
sum = in4_cksum(m, IPPROTO_TCP, iphlen, tlen);
@@ -524,7 +523,7 @@ tcp_input(struct mbuf **mp, int *offp, i
 #endif
}
if (sum != 0) {
-   tcpstat.tcps_rcvbadsum++;
+ 

Re: Xorg vs wifi scanning

[Stefan Sperling]

On Mon, Feb 06, 2017 at 03:54:56PM +0100, Martin Pieuchot wrote:
> tb@ found that another code path needs to be unlocked.  Diff below does
> that.  These ioctl(2)s are only messing at the driver level, so it is
> safe to drop the NET_LOCK() there as well.
> 
> ok?

I am happy with this. OK.

And thank you for taking care of this problem!

void crp_callback

[Alexander Bluhm]

Hi,

The return code of crp_callback is never checked, so it is not
useful to propagate the error.  When an error occures in an
asynchronous network path, incrementing a counter is the right
thing.  By removing the error handling, I have found 4 places where
an error is not accounted.  For now just add a comment as adding
a counter touches even more code.

ok?

bluhm

Index: crypto/cryptodev.h
===
RCS file: /data/mirror/openbsd/cvs/src/sys/crypto/cryptodev.h,v
retrieving revision 1.68
diff -u -p -r1.68 cryptodev.h
--- crypto/cryptodev.h  18 Apr 2016 21:05:55 -  1.68
+++ crypto/cryptodev.h  6 Feb 2017 16:14:49 -
@@ -181,7 +181,7 @@ struct cryptop {
void*crp_opaque;/* Opaque pointer, passed along */
struct cryptodesc *crp_desc;/* Linked list of processing 
descriptors */
 
-   int (*crp_callback)(struct cryptop *); /* Callback function */
+   void (*crp_callback)(struct cryptop *); /* Callback function */
 
caddr_t crp_mac;
 };
Index: dev/softraid_crypto.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/dev/softraid_crypto.c,v
retrieving revision 1.131
diff -u -p -r1.131 softraid_crypto.c
--- dev/softraid_crypto.c   8 Sep 2016 17:39:08 -   1.131
+++ dev/softraid_crypto.c   6 Feb 2017 16:14:49 -
@@ -88,11 +88,11 @@ int sr_crypto_ioctl(struct sr_disciplin
struct bioc_discipline *);
 intsr_crypto_meta_opt_handler(struct sr_discipline *,
struct sr_meta_opt_hdr *);
-intsr_crypto_write(struct cryptop *);
+void   sr_crypto_write(struct cryptop *);
 intsr_crypto_rw(struct sr_workunit *);
 intsr_crypto_dev_rw(struct sr_workunit *, struct sr_crypto_wu *);
 void   sr_crypto_done(struct sr_workunit *);
-intsr_crypto_read(struct cryptop *);
+void   sr_crypto_read(struct cryptop *);
 void   sr_crypto_calculate_check_hmac_sha1(u_int8_t *, int,
   u_int8_t *, int, u_char *);
 void   sr_crypto_hotplug(struct sr_discipline *, struct disk *, int);
@@ -1117,7 +1117,7 @@ sr_crypto_rw(struct sr_workunit *wu)
return (rv);
 }
 
-int
+void
 sr_crypto_write(struct cryptop *crp)
 {
struct sr_crypto_wu *crwu = crp->crp_opaque;
@@ -1135,7 +1135,7 @@ sr_crypto_write(struct cryptop *crp)
splx(s);
}
 
-   return (sr_crypto_dev_rw(wu, crwu));
+   sr_crypto_dev_rw(wu, crwu);
 }
 
 int
@@ -1195,7 +1195,7 @@ sr_crypto_done(struct sr_workunit *wu)
splx(s);
 }
 
-int
+void
 sr_crypto_read(struct cryptop *crp)
 {
struct sr_crypto_wu *crwu = crp->crp_opaque;
@@ -1211,8 +1211,6 @@ sr_crypto_read(struct cryptop *crp)
s = splbio();
sr_scsi_done(wu->swu_dis, wu->swu_xs);
splx(s);
-
-   return (0);
 }
 
 void
Index: netinet/ip_ah.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_ah.c,v
retrieving revision 1.125
diff -u -p -r1.125 ip_ah.c
--- netinet/ip_ah.c 9 Jan 2017 17:10:02 -   1.125
+++ netinet/ip_ah.c 6 Feb 2017 16:14:49 -
@@ -76,8 +76,8 @@
 #define DPRINTF(x)
 #endif
 
-intah_output_cb(struct cryptop *);
-intah_input_cb(struct cryptop *);
+void   ah_output_cb(struct cryptop *);
+void   ah_input_cb(struct cryptop *);
 intah_massage_headers(struct mbuf **, int, int, int, int);
 
 struct ahstat ahstat;
@@ -697,10 +697,10 @@ ah_input(struct mbuf *m, struct tdb *tdb
 /*
  * AH input callback, called directly by the crypto driver.
  */
-int
+void
 ah_input_cb(struct cryptop *crp)
 {
-   int s, roff, rplen, error, skip, protoff;
+   int s, roff, rplen, skip, protoff;
unsigned char calc[AH_ALEN_MAX];
struct mbuf *m1, *m0, *m;
struct auth_hash *ahx;
@@ -724,7 +724,7 @@ ah_input_cb(struct cryptop *crp)
ahstat.ahs_crypto++;
DPRINTF(("ah_input_cb(): bogus returned buffer from "
"crypto\n"));
-   return (EINVAL);
+   return;
}
 
NET_LOCK(s);
@@ -734,7 +734,6 @@ ah_input_cb(struct cryptop *crp)
free(tc, M_XDATA, 0);
ahstat.ahs_notdb++;
DPRINTF(("ah_input_cb(): TDB is expired while in crypto"));
-   error = EPERM;
goto baddone;
}
 
@@ -747,12 +746,12 @@ ah_input_cb(struct cryptop *crp)
if (tdb->tdb_cryptoid != 0)
tdb->tdb_cryptoid = crp->crp_sid;
NET_UNLOCK(s);
-   return crypto_dispatch(crp);
+   crypto_dispatch(crp);
+   return;
}
free(tc, M_XDATA, 0);
ahstat.ahs_noxform++;

Re: PF & CPU hogging

[Mike Belopuhov]

On 6 February 2017 at 17:02, Martin Pieuchot  wrote:
> PF has its own home-brewed solution for dealing with CPU hogging.  It
> has been introduced in r1.88 of net/pf_table.c and I couldn't find any
> explanation why it is different than the idiom we use in other places.
>

The idea was just to be able to load a huge table semi-efficiently.
I don't think SPCF_SHOULDYIELD existed back then.

Re: PF & CPU hogging

[Mike Belopuhov]

On 6 February 2017 at 17:02, Martin Pieuchot  wrote:
> PF has its own home-brewed solution for dealing with CPU hogging.  It
> has been introduced in r1.88 of net/pf_table.c and I couldn't find any
> explanation why it is different than the idiom we use in other places.
>
> So let's use the same idiom, I promise to introduce a macro an unify all
> of them once this is in.
>
> ok?
>

Why not replace YIELD with sched_pause() then?

NFS: Kill so_upcall NET_LOCK() recursion

[Martin Pieuchot]

guenther@ pointed out during a2k17 that nfsrv_rcv() already has a way to
not re-enter the socket layer.  Instead of doing soreceive() directly in
the receiving path, we notify and wakeup nfsd(8) to do it.

I'd like to do that unconditionally to fix one of the blocking
recursions.

I just finished a bake a build on NFS.  I'd appreciate more tests an
oks.

Index: kern/uipc_socket.c
===
RCS file: /cvs/src/sys/kern/uipc_socket.c,v
retrieving revision 1.176
diff -u -p -r1.176 uipc_socket.c
--- kern/uipc_socket.c  1 Feb 2017 20:59:47 -   1.176
+++ kern/uipc_socket.c  6 Feb 2017 14:33:48 -
@@ -1530,12 +1530,8 @@ sorwakeup(struct socket *so)
return;
 #endif
sowakeup(so, >so_rcv);
-   if (so->so_upcall) {
-   /* XXXSMP breaks atomicity */
-   rw_exit_write();
+   if (so->so_upcall)
(*(so->so_upcall))(so, so->so_upcallarg, M_DONTWAIT);
-   rw_enter_write();
-   }
 }
 
 void
Index: nfs/nfs_socket.c
===
RCS file: /cvs/src/sys/nfs/nfs_socket.c,v
retrieving revision 1.112
diff -u -p -r1.112 nfs_socket.c
--- nfs/nfs_socket.c19 Dec 2016 08:36:50 -  1.112
+++ nfs/nfs_socket.c6 Feb 2017 14:16:39 -
@@ -1570,26 +1570,15 @@ nfsrv_rcv(struct socket *so, caddr_t arg
 
if ((slp->ns_flag & SLP_VALID) == 0)
return;
-#ifdef notdef
-   /*
-* Define this to test for nfsds handling this under heavy load.
-*/
+
+   /* Defer soreceive() to an nfsd. */
if (waitflag == M_DONTWAIT) {
-   slp->ns_flag |= SLP_NEEDQ; goto dorecs;
+   slp->ns_flag |= SLP_NEEDQ;
+   goto dorecs;
}
-#endif
+
auio.uio_procp = NULL;
if (so->so_type == SOCK_STREAM) {
-   /*
-* If there are already records on the queue, defer soreceive()
-* to an nfsd so that there is feedback to the TCP layer that
-* the nfs servers are heavily loaded.
-*/
-   if (slp->ns_rec && waitflag == M_DONTWAIT) {
-   slp->ns_flag |= SLP_NEEDQ;
-   goto dorecs;
-   }
-
/*
 * Do soreceive().
 */
Index: conf/GENERIC
===
RCS file: /cvs/src/sys/conf/GENERIC,v
retrieving revision 1.239
diff -u -p -r1.239 GENERIC
--- conf/GENERIC25 Jan 2017 00:48:36 -  1.239
+++ conf/GENERIC6 Feb 2017 14:36:04 -
@@ -59,7 +59,7 @@ optionPPP_DEFLATE
 option PIPEX   # Ppp IP EXtension, for npppd
 option MROUTING# Multicast router
 option MPLS# Multi-Protocol Label Switching
-option BFD # Bi-directional Forwarding Detection
+#optionBFD # Bi-directional Forwarding Detection
 
 #mpath0at root # SCSI Multipathing
 #scsibus*  at mpath?

PF & CPU hogging

[Martin Pieuchot]

PF has its own home-brewed solution for dealing with CPU hogging.  It
has been introduced in r1.88 of net/pf_table.c and I couldn't find any
explanation why it is different than the idiom we use in other places.

So let's use the same idiom, I promise to introduce a macro an unify all
of them once this is in.

ok?

Index: net/pf_table.c
===
RCS file: /cvs/src/sys/net/pf_table.c,v
retrieving revision 1.123
diff -u -p -r1.123 pf_table.c
--- net/pf_table.c  24 Jan 2017 10:08:30 -  1.123
+++ net/pf_table.c  6 Feb 2017 12:33:28 -
@@ -36,6 +36,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 
@@ -72,11 +73,8 @@
(bcopy((from), (to), (size)), 0))
 
 #define YIELD(cnt, ok) \
-   do {\
-   if ((cnt % 1024 == 1023) && \
-   (ok))   \
-   yield();\
-   } while (0)
+   if (curcpu()->ci_schedstate.spc_schedflags & SPCF_SHOULDYIELD) \
+   preempt(NULL)   \
 
 #defineFILLIN_SIN(sin, addr)   \
do {\

Re: Scheduler ping-pong with preempt()

[Bob Beck]

Go for it mpi.. move forward.

ok beck@

On Mon, Feb 6, 2017 at 7:48 AM, Martin Pieuchot  wrote:

> On 24/01/17(Tue) 13:35, Martin Pieuchot wrote:
> > Userland threads are preempt()'d when hogging a CPU or when processing
> > an AST.  Currently when such a thread is preempted the scheduler looks
> > for an idle CPU and puts it on its run queue.  That means the number of
> > involuntary context switch often result in a migration.
> >
> > This is not a problem per se and one could argue that if another CPU
> > is idle it makes sense to move.  However with the KERNEL_LOCK() moving
> > to another CPU won't necessarily allows the preempt()'d thread to run.
> > It's even worse, it increases contention.
> >
> > If you add to this behavior the fact that sched_choosecpu() prefers idle
> > CPUs in a linear order, meaning CPU0 > CPU1 > .. > CPUN, you'll
> > understand that the set of idle CPUs will change every time preempt() is
> > called.
> >
> > I believe this behavior affects kernel threads by side effect, since
> > the set of idle CPU changes every time a thread is preempted.  With this
> > diff the 'softnet' thread didn't move on a 2 CPUs machine during simple
> > benchmarks.  Without, it plays ping-pong between CPU.
> >
> > The goal of this diff is to reduce the number of migrations.  You
> > can compare the value of 'sched_nomigrations' and 'sched_nmigrations'
> > with and without it.
> >
> > As usual, I'd like to know what's the impact of this diff on your
> > favorite benchmark.  Please test and report back.
>
> I only got positive test results so I'd like to commit the diff below.
>
> ok?
>
> Index: kern/sched_bsd.c
> ===
> RCS file: /cvs/src/sys/kern/sched_bsd.c,v
> retrieving revision 1.44
> diff -u -p -r1.44 sched_bsd.c
> --- kern/sched_bsd.c25 Jan 2017 06:15:50 -  1.44
> +++ kern/sched_bsd.c6 Feb 2017 14:47:28 -
> @@ -329,7 +329,6 @@ preempt(struct proc *newp)
> SCHED_LOCK(s);
> p->p_priority = p->p_usrpri;
> p->p_stat = SRUN;
> -   p->p_cpu = sched_choosecpu(p);
> setrunqueue(p);
> p->p_ru.ru_nivcsw++;
> mi_switch();
>
>

Re: Xorg vs wifi scanning

[Martin Pieuchot]

On 06/02/17(Mon) 12:27, Martin Pieuchot wrote:
> Paul and Antoine reported that, since the NET_LOCK() went in, doing a
> channel scan with iwm(4) and iwn(4) freezes X.
> 
> This is a deadlock due to the fact that wireless drivers sleep during
> a long time holding the NET_LOCK() while the X server tries to
> communicate with unix sockets, that still need the NET_LOCK().
> 
> The obvious solution to this problem is to not hold the NET_LOCK() for
> unix socket related operations.  We're working on that.
> 
> However since hardware ioctl(2) can sleep there's no problem to release
> the NET_LOCK() there.  This is true for all ioctls that are not
> modifying any stack state (address, multicast group, etc).  Hence the
> diff below that should fix the problem in a generic way.

tb@ found that another code path needs to be unlocked.  Diff below does
that.  These ioctl(2)s are only messing at the driver level, so it is
safe to drop the NET_LOCK() there as well.

ok?

Index: netinet/in.c
===
RCS file: /cvs/src/sys/netinet/in.c,v
retrieving revision 1.133
diff -u -p -r1.133 in.c
--- netinet/in.c20 Dec 2016 12:35:38 -  1.133
+++ netinet/in.c6 Feb 2017 13:43:06 -
@@ -390,7 +390,11 @@ in_ioctl(u_long cmd, caddr_t data, struc
default:
if (ifp->if_ioctl == NULL)
return (EOPNOTSUPP);
-   return ((*ifp->if_ioctl)(ifp, cmd, data));
+   /* XXXSMP breaks atomicity */
+   rw_exit_write();
+   error = ((*ifp->if_ioctl)(ifp, cmd, data));
+   rw_enter_write();
+   return (error);
}
return (0);
 }
Index: netinet6/in6.c
===
RCS file: /cvs/src/sys/netinet6/in6.c,v
retrieving revision 1.197
diff -u -p -r1.197 in6.c
--- netinet6/in6.c  5 Feb 2017 16:04:14 -   1.197
+++ netinet6/in6.c  6 Feb 2017 13:43:06 -
@@ -190,6 +190,7 @@ in6_ioctl(u_long cmd, caddr_t data, stru
struct  in6_ifaddr *ia6 = NULL;
struct  in6_aliasreq *ifra = (struct in6_aliasreq *)data;
struct sockaddr_in6 *sa6;
+   int error;
 
if (ifp == NULL)
return (EOPNOTSUPP);
@@ -469,9 +470,13 @@ in6_ioctl(u_long cmd, caddr_t data, stru
break;
 
default:
-   if (ifp == NULL || ifp->if_ioctl == 0)
+   if (ifp->if_ioctl == NULL)
return (EOPNOTSUPP);
-   return ((*ifp->if_ioctl)(ifp, cmd, data));
+   /* XXXSMP breaks atomicity */
+   rw_exit_write();
+   error = ((*ifp->if_ioctl)(ifp, cmd, data));
+   rw_enter_write();
+   return (error);
}
 
return (0);
Index: dev/pci/if_iwm.c
===
RCS file: /cvs/src/sys/dev/pci/if_iwm.c,v
retrieving revision 1.162
diff -u -p -r1.162 if_iwm.c
--- dev/pci/if_iwm.c4 Feb 2017 19:20:59 -   1.162
+++ dev/pci/if_iwm.c6 Feb 2017 14:52:24 -
@@ -6133,18 +6133,13 @@ iwm_ioctl(struct ifnet *ifp, u_long cmd,
struct ifreq *ifr;
int s, err = 0;
 
-   /* XXXSMP breaks atomicity */
-   rw_exit_write();
-
/*
 * Prevent processes from entering this function while another
 * process is tsleep'ing in it.
 */
err = rw_enter(>ioctl_rwl, RW_WRITE | RW_INTR);
-   if (err) {
-   rw_enter_write();
+   if (err)
return err;
-   }
 
s = splnet();
 
@@ -6190,7 +6185,6 @@ iwm_ioctl(struct ifnet *ifp, u_long cmd,
 
splx(s);
rw_exit(>ioctl_rwl);
-   rw_enter_write();
 
return err;
 }
Index: net/if.c
===
RCS file: /cvs/src/sys/net/if.c,v
retrieving revision 1.485
diff -u -p -r1.485 if.c
--- net/if.c1 Feb 2017 02:02:01 -   1.485
+++ net/if.c6 Feb 2017 14:27:47 -
@@ -2029,7 +2029,10 @@ ifioctl(struct socket *so, u_long cmd, c
case SIOCGIFPARENT:
if (ifp->if_ioctl == 0)
return (EOPNOTSUPP);
+   /* XXXSMP breaks atomicity */
+   rw_exit_write();
error = (*ifp->if_ioctl)(ifp, cmd, data);
+   rw_enter_write();
break;
 
case SIOCGIFDESCR:

Re: Scheduler ping-pong with preempt()

[Martin Pieuchot]

On 24/01/17(Tue) 13:35, Martin Pieuchot wrote:
> Userland threads are preempt()'d when hogging a CPU or when processing
> an AST.  Currently when such a thread is preempted the scheduler looks
> for an idle CPU and puts it on its run queue.  That means the number of
> involuntary context switch often result in a migration. 
> 
> This is not a problem per se and one could argue that if another CPU
> is idle it makes sense to move.  However with the KERNEL_LOCK() moving
> to another CPU won't necessarily allows the preempt()'d thread to run.
> It's even worse, it increases contention.
> 
> If you add to this behavior the fact that sched_choosecpu() prefers idle
> CPUs in a linear order, meaning CPU0 > CPU1 > .. > CPUN, you'll
> understand that the set of idle CPUs will change every time preempt() is
> called.
> 
> I believe this behavior affects kernel threads by side effect, since
> the set of idle CPU changes every time a thread is preempted.  With this
> diff the 'softnet' thread didn't move on a 2 CPUs machine during simple
> benchmarks.  Without, it plays ping-pong between CPU.
> 
> The goal of this diff is to reduce the number of migrations.  You
> can compare the value of 'sched_nomigrations' and 'sched_nmigrations'
> with and without it.
> 
> As usual, I'd like to know what's the impact of this diff on your
> favorite benchmark.  Please test and report back.

I only got positive test results so I'd like to commit the diff below.

ok?

Index: kern/sched_bsd.c
===
RCS file: /cvs/src/sys/kern/sched_bsd.c,v
retrieving revision 1.44
diff -u -p -r1.44 sched_bsd.c
--- kern/sched_bsd.c25 Jan 2017 06:15:50 -  1.44
+++ kern/sched_bsd.c6 Feb 2017 14:47:28 -
@@ -329,7 +329,6 @@ preempt(struct proc *newp)
SCHED_LOCK(s);
p->p_priority = p->p_usrpri;
p->p_stat = SRUN;
-   p->p_cpu = sched_choosecpu(p);
setrunqueue(p);
p->p_ru.ru_nivcsw++;
mi_switch();

Re: specify curves via ecdhe statement in httpd.conf

[Joel Sing]

On Sunday 05 February 2017 17:05:40 Andreas Bartelt wrote:
> > - What type of public certificate are you using (RSA or ECDSA)?
> 
> ECDSA with P-256. Certificate signed by letsencrypt (via RSA).
> Must-staple is enabled - that's why I'm also using the ocsp line for
> testing.

Ah, this was the missing piece of information.

In order to use ECDSA the client must support the curve used for the server 
certificate, otherwise when the server signs the server key exchange, the 
client will not be able to verify the signature. In the case where you 
announce that the client only supports P-384, any ECDSA ciphers are considered 
to be invalid for this session, effectively resulting in no shared ciphers and 
the handshake failure alert.

In order for this configuration to work you need to include P-256 in the client 
supported groups. Specifying groups as "P-384:P-256" should still get you 
P-384, depending on the server configuration and whether the preference for a 
curve is based on the client or server preference (for libtls and hence httpd, 
it will be server preference).

ssh-keygen: vfprintf %s NULL

[Sebastien Marie]

Hi,

While changing comments on ssh-key, I saw some vfprintf in log:

Feb  6 14:24:44 clyde ssh-keygen: vfprintf %s NULL in "Key now has comment '%s' 
"


Steps to reproduce:

$ ssh-keygen -f test -C ''
Generating public/private rsa key pair.   
Enter passphrase (empty for no passphrase): 
Enter same passphrase again:  
Your identification has been saved in test.   
Your public key has been saved in test.pub. 
The key fingerprint is:   
SHA256:Pz26unhH1NsOx3JDcKTLaBpKvdadRG+tnJXZKI17io4
The key's randomart image is: 
+---[RSA 2048]+   
| ..  |   
|...  | 
|   .oo   |   
|   .  .+.=.o+|   
|  . S.o *==o+|   
| . . B.++**+ |   
|  . +.+ **=. |   
|   o...+ +.  |   
|  ..E==..|   
+[SHA256]-+   
$ ssh-keygen -c -C 'test' -o -f test  
Key now has comment '(null)'  
The comment in your key file has been changed.  


The following diff should correct it.

As side note, I found the output of ssh-keygen a bit confusing as the
printed comment is the old one (but it could be due to english isn't my
native language).

-- 
Sebastien Marie


Index: ssh-keygen.c
===
RCS file: /cvs/src/usr.bin/ssh/ssh-keygen.c,v
retrieving revision 1.292
diff -u -p -r1.292 ssh-keygen.c
--- ssh-keygen.c12 Sep 2016 03:29:16 -  1.292
+++ ssh-keygen.c6 Feb 2017 13:27:40 -
@@ -1425,7 +1425,10 @@ do_change_comment(struct passwd *pw)
sshkey_free(private);
exit(1);
}
-   printf("Key now has comment '%s'\n", comment);
+   if (comment)
+   printf("Key now has comment '%s'\n", comment);
+   else
+   printf("Key now has no comment\n");
 
if (identity_comment) {
strlcpy(new_comment, identity_comment, sizeof(new_comment));

rip6 and icmp6stat percpu counters

[Jeremie Courreges-Anglas]

In icmp6_errcount() we could save a few function calls but I preferred
to keep the conversion as mechanical as possible.  Works fine here on
amd64 (I can test armv7 soon).

ok?


Index: netinet/icmp6.h
===
RCS file: /d/cvs/src/sys/netinet/icmp6.h,v
retrieving revision 1.42
diff -u -p -r1.42 icmp6.h
--- netinet/icmp6.h 9 Sep 2015 15:51:40 -   1.42
+++ netinet/icmp6.h 6 Feb 2017 07:10:21 -
@@ -457,22 +457,6 @@ struct icmp6_filter {
  * Variables related to this implementation
  * of the internet control message protocol version 6.
  */
-struct icmp6errstat {
-   u_int64_t icp6errs_dst_unreach_noroute;
-   u_int64_t icp6errs_dst_unreach_admin;
-   u_int64_t icp6errs_dst_unreach_beyondscope;
-   u_int64_t icp6errs_dst_unreach_addr;
-   u_int64_t icp6errs_dst_unreach_noport;
-   u_int64_t icp6errs_packet_too_big;
-   u_int64_t icp6errs_time_exceed_transit;
-   u_int64_t icp6errs_time_exceed_reassembly;
-   u_int64_t icp6errs_paramprob_header;
-   u_int64_t icp6errs_paramprob_nextheader;
-   u_int64_t icp6errs_paramprob_option;
-   u_int64_t icp6errs_redirect; /* we regard redirect as an error here */
-   u_int64_t icp6errs_unknown;
-};
-
 struct icmp6stat {
 /* statistics related to icmp6 packets generated */
u_int64_t icp6s_error;  /* # of calls to icmp6_error */
@@ -491,25 +475,19 @@ struct icmp6stat {
u_int64_t icp6s_reflect;
u_int64_t icp6s_inhist[256];
u_int64_t icp6s_nd_toomanyopt;  /* too many ND options */
-   struct icmp6errstat icp6s_outerrhist;
-#define icp6s_odst_unreach_noroute \
-   icp6s_outerrhist.icp6errs_dst_unreach_noroute
-#define icp6s_odst_unreach_admin icp6s_outerrhist.icp6errs_dst_unreach_admin
-#define icp6s_odst_unreach_beyondscope \
-   icp6s_outerrhist.icp6errs_dst_unreach_beyondscope
-#define icp6s_odst_unreach_addr icp6s_outerrhist.icp6errs_dst_unreach_addr
-#define icp6s_odst_unreach_noport icp6s_outerrhist.icp6errs_dst_unreach_noport
-#define icp6s_opacket_too_big icp6s_outerrhist.icp6errs_packet_too_big
-#define icp6s_otime_exceed_transit \
-   icp6s_outerrhist.icp6errs_time_exceed_transit
-#define icp6s_otime_exceed_reassembly \
-   icp6s_outerrhist.icp6errs_time_exceed_reassembly
-#define icp6s_oparamprob_header icp6s_outerrhist.icp6errs_paramprob_header
-#define icp6s_oparamprob_nextheader \
-   icp6s_outerrhist.icp6errs_paramprob_nextheader
-#define icp6s_oparamprob_option icp6s_outerrhist.icp6errs_paramprob_option
-#define icp6s_oredirect icp6s_outerrhist.icp6errs_redirect
-#define icp6s_ounknown icp6s_outerrhist.icp6errs_unknown
+   u_int64_t icp6s_odst_unreach_noroute;
+   u_int64_t icp6s_odst_unreach_admin;
+   u_int64_t icp6s_odst_unreach_beyondscope;
+   u_int64_t icp6s_odst_unreach_addr;
+   u_int64_t icp6s_odst_unreach_noport;
+   u_int64_t icp6s_opacket_too_big;
+   u_int64_t icp6s_otime_exceed_transit;
+   u_int64_t icp6s_otime_exceed_reassembly;
+   u_int64_t icp6s_oparamprob_header;
+   u_int64_t icp6s_oparamprob_nextheader;
+   u_int64_t icp6s_oparamprob_option;
+   u_int64_t icp6s_oredirect;  /* we regard redirect as an error here 
*/
+   u_int64_t icp6s_ounknown;
u_int64_t icp6s_pmtuchg;/* path MTU changes */
u_int64_t icp6s_nd_badopt;  /* bad ND options */
u_int64_t icp6s_badns;  /* bad neighbor solicitation */
@@ -590,6 +568,51 @@ struct icmp6stat {
 #define RTF_PROBEMTU   RTF_PROTO1
 
 #ifdef _KERNEL
+
+#include 
+
+enum icmp6stat_counters {
+   icp6s_error,
+   icp6s_canterror,
+   icp6s_toofreq,
+   icp6s_outhist,
+   icp6s_badcode = icp6s_outhist + 256,
+   icp6s_tooshort,
+   icp6s_checksum,
+   icp6s_badlen,
+   icp6s_reflect,
+   icp6s_inhist,
+   icp6s_nd_toomanyopt = icp6s_inhist + 256,
+   icp6s_odst_unreach_noroute,
+   icp6s_odst_unreach_admin,
+   icp6s_odst_unreach_beyondscope,
+   icp6s_odst_unreach_addr,
+   icp6s_odst_unreach_noport,
+   icp6s_opacket_too_big,
+   icp6s_otime_exceed_transit,
+   icp6s_otime_exceed_reassembly,
+   icp6s_oparamprob_header,
+   icp6s_oparamprob_nextheader,
+   icp6s_oparamprob_option,
+   icp6s_oredirect,
+   icp6s_ounknown,
+   icp6s_pmtuchg,
+   icp6s_nd_badopt,
+   icp6s_badns,
+   icp6s_badna,
+   icp6s_badrs,
+   icp6s_badra,
+   icp6s_badredirect,
+   icp6s_ncounters,
+};
+
+extern struct cpumem *icmp6counters;
+
+static inline void
+icmp6stat_inc(enum icmp6stat_counters c)
+{
+   counters_inc(icmp6counters, c);
+}
 
 struct rtentry;
 struct rttimer;
Index: netinet6/icmp6.c
===
RCS file: /d/cvs/src/sys/netinet6/icmp6.c,v
retrieving revision 1.199
diff -u -p -r1.199 icmp6.c
--- netinet6/icmp6.c5 Feb 2017 16:04:14 

Re: make sosetopt responsible for m_free

[Martin Pieuchot]

On 03/02/17(Fri) 11:02, David Hill wrote:
> On Fri, Feb 03, 2017 at 09:50:40AM +0100, Martin Pieuchot wrote:
> > On 02/02/17(Thu) 12:12, David Hill wrote:
> > > On Thu, Feb 02, 2017 at 09:34:07AM +0100, Martin Pieuchot wrote:
> > > > On 01/02/17(Wed) 19:27, David Hill wrote:
> > > > > Hello -
> > > > > 
> > > > > This diff makes sosetopt responsible for m_free which is much simpler.
> > > > > Requested by bluhm@ 
> > > > 
> > > > I'd suggest to move the m_free(9) calls to sys_setsockopt().  This
> > > > simplifies the existing code even more and will make it easier to use
> > > > the stack for this temporary storage.
> > > > 
> > > 
> > > New diff with mpi@'s suggestion. 
> > 
> > You forgot NFS and BFD that should now call m_free(9) after sosetopt(9). 
> >
> 
> Indeed!  Now with BFD and NFS...

You're introducing a use after-free in ip_pcbopts().  You need to
allocate/copy the mbuf there.

I must say I'm a bit afraid of this change because the amount of code it
touches.  There might be another use after free somewhere that I missed.

Maybe we should first split our huge *ctloutput functions.  

One easy move is to split setopt/getopt.

Then introduce more per-protocol functions instead of having everything
in ip{,6}_ctloutput().

For example move all the IPSEC craziness out of these functions.  Same
thing with ICMP6...  This might sound superfluous but it will help
if/when we decide to have a fine graining for different subsystems.

I'd also suggest to change the 'struct protosw' declaration to use C99
initializer.  So we can have:

.pr_ctloutput = ipsec_ctloutput

This would allow us to grep for "pr_ctloutput" (or pr_setopt) and know
directly which functions to review.

> Index: kern/uipc_socket.c
> ===
> RCS file: /cvs/src/sys/kern/uipc_socket.c,v
> retrieving revision 1.176
> diff -u -p -r1.176 uipc_socket.c
> --- kern/uipc_socket.c1 Feb 2017 20:59:47 -   1.176
> +++ kern/uipc_socket.c3 Feb 2017 16:00:26 -
> @@ -1551,16 +1551,15 @@ sowwakeup(struct socket *so)
>  }
>  
>  int
> -sosetopt(struct socket *so, int level, int optname, struct mbuf *m0)
> +sosetopt(struct socket *so, int level, int optname, struct mbuf *m)
>  {
>   int s, error = 0;
> - struct mbuf *m = m0;
>  
>   if (level != SOL_SOCKET) {
>   if (so->so_proto && so->so_proto->pr_ctloutput) {
>   NET_LOCK(s);
>   error = (*so->so_proto->pr_ctloutput)(PRCO_SETOPT, so,
> - level, optname, m0);
> + level, optname, m);
>   NET_UNLOCK(s);
>   return (error);
>   }
> @@ -1707,7 +1706,7 @@ sosetopt(struct socket *so, int level, i
>   level = dom->dom_protosw->pr_protocol;
>   NET_LOCK(s);
>   error = (*so->so_proto->pr_ctloutput)
> - (PRCO_SETOPT, so, level, optname, m0);
> + (PRCO_SETOPT, so, level, optname, m);
>   NET_UNLOCK(s);
>   return (error);
>   }
> @@ -1739,14 +1738,11 @@ sosetopt(struct socket *so, int level, i
>   if (error == 0 && so->so_proto && so->so_proto->pr_ctloutput) {
>   NET_LOCK(s);
>   (*so->so_proto->pr_ctloutput)(PRCO_SETOPT, so,
> - level, optname, m0);
> + level, optname, m);
>   NET_UNLOCK(s);
> - m = NULL;   /* freed by protocol */
>   }
>   }
>  bad:
> - if (m)
> - (void) m_free(m);
>   return (error);
>  }
>  
> Index: kern/uipc_syscalls.c
> ===
> RCS file: /cvs/src/sys/kern/uipc_syscalls.c,v
> retrieving revision 1.148
> diff -u -p -r1.148 uipc_syscalls.c
> --- kern/uipc_syscalls.c  26 Jan 2017 01:58:00 -  1.148
> +++ kern/uipc_syscalls.c  3 Feb 2017 16:00:26 -
> @@ -962,19 +962,13 @@ sys_setsockopt(struct proc *p, void *v, 
>   goto bad;
>   }
>   }
> - if (m == NULL) {
> - error = ENOBUFS;
> - goto bad;
> - }
>   error = copyin(SCARG(uap, val), mtod(m, caddr_t),
>   SCARG(uap, valsize));
> - if (error) {
> + if (error)
>   goto bad;
> - }
>   m->m_len = SCARG(uap, valsize);
>   }
>   error = sosetopt(fp->f_data, SCARG(uap, level), SCARG(uap, name), m);
> - m = NULL;
>  bad:
>   m_freem(m);
>   FRELE(fp, p);
> Index: net/bfd.c
> ===
> RCS file: /cvs/src/sys/net/bfd.c,v
> 

Re: Xorg vs wifi scanning

[Martin Pieuchot]

On 06/02/17(Mon) 12:59, Stefan Sperling wrote:
> On Mon, Feb 06, 2017 at 12:44:52PM +0100, Martin Pieuchot wrote:
> > On 06/02/17(Mon) 12:39, Theo Buehler wrote:
> > > This fixes the issue on my iwn0, but re-introduces the problem on my iwm0.
> > 
> > That means another ioctl(2) triggers the problem with iwm(4).  It would
> > help if you could figure out which one.
> 
> Probably SIOCS80211SCAN. Your diff doesn't touch ieee80211_ioctl().

My diff does touch ieee80211_ioctl().  Like that:

ifioctl() ->
  pr_usrreq(PRU_CONTROL ->
in_control( ->
  iwm_ioctl( ->
ieee80211_ioctl().

Or do you know another path to reach this ioctl handler?

Re: Xorg vs wifi scanning

[Stefan Sperling]

On Mon, Feb 06, 2017 at 12:44:52PM +0100, Martin Pieuchot wrote:
> On 06/02/17(Mon) 12:39, Theo Buehler wrote:
> > This fixes the issue on my iwn0, but re-introduces the problem on my iwm0.
> 
> That means another ioctl(2) triggers the problem with iwm(4).  It would
> help if you could figure out which one.

Probably SIOCS80211SCAN. Your diff doesn't touch ieee80211_ioctl().

Re: Xorg vs wifi scanning

[Martin Pieuchot]

On 06/02/17(Mon) 12:39, Theo Buehler wrote:
> On Mon, Feb 06, 2017 at 12:27:15PM +0100, Martin Pieuchot wrote:
> > Paul and Antoine reported that, since the NET_LOCK() went in, doing a
> > channel scan with iwm(4) and iwn(4) freezes X.
> > 
> > This is a deadlock due to the fact that wireless drivers sleep during
> > a long time holding the NET_LOCK() while the X server tries to
> > communicate with unix sockets, that still need the NET_LOCK().
> > 
> > The obvious solution to this problem is to not hold the NET_LOCK() for
> > unix socket related operations.  We're working on that.
> > 
> > However since hardware ioctl(2) can sleep there's no problem to release
> > the NET_LOCK() there.  This is true for all ioctls that are not
> > modifying any stack state (address, multicast group, etc).  Hence the
> > diff below that should fix the problem in a generic way.
> > 
> > ok?
> 
> This fixes the issue on my iwn0, but re-introduces the problem on my iwm0.

That means another ioctl(2) triggers the problem with iwm(4).  It would
help if you could figure out which one.

> Tested with
> 
> iwn0 at pci2 dev 0 function 0 "Intel Centrino Ultimate-N 6300" rev 0x35: msi, 
> MIMO 3T3R, MoW
> 
> and
> 
> iwm0 at pci2 dev 0 function 0 "Intel Dual Band Wireless AC 7265" rev 0x59, msi
> iwm0: hw rev 0x210, fw ver 16.242414.0
> 

Re: Xorg vs wifi scanning

[Theo Buehler]

On Mon, Feb 06, 2017 at 12:27:15PM +0100, Martin Pieuchot wrote:
> Paul and Antoine reported that, since the NET_LOCK() went in, doing a
> channel scan with iwm(4) and iwn(4) freezes X.
> 
> This is a deadlock due to the fact that wireless drivers sleep during
> a long time holding the NET_LOCK() while the X server tries to
> communicate with unix sockets, that still need the NET_LOCK().
> 
> The obvious solution to this problem is to not hold the NET_LOCK() for
> unix socket related operations.  We're working on that.
> 
> However since hardware ioctl(2) can sleep there's no problem to release
> the NET_LOCK() there.  This is true for all ioctls that are not
> modifying any stack state (address, multicast group, etc).  Hence the
> diff below that should fix the problem in a generic way.
> 
> ok?

This fixes the issue on my iwn0, but re-introduces the problem on my iwm0.

Tested with

iwn0 at pci2 dev 0 function 0 "Intel Centrino Ultimate-N 6300" rev 0x35: msi, 
MIMO 3T3R, MoW

and

iwm0 at pci2 dev 0 function 0 "Intel Dual Band Wireless AC 7265" rev 0x59, msi
iwm0: hw rev 0x210, fw ver 16.242414.0

Xorg vs wifi scanning

[Martin Pieuchot]

Paul and Antoine reported that, since the NET_LOCK() went in, doing a
channel scan with iwm(4) and iwn(4) freezes X.

This is a deadlock due to the fact that wireless drivers sleep during
a long time holding the NET_LOCK() while the X server tries to
communicate with unix sockets, that still need the NET_LOCK().

The obvious solution to this problem is to not hold the NET_LOCK() for
unix socket related operations.  We're working on that.

However since hardware ioctl(2) can sleep there's no problem to release
the NET_LOCK() there.  This is true for all ioctls that are not
modifying any stack state (address, multicast group, etc).  Hence the
diff below that should fix the problem in a generic way.

ok?

Index: netinet/in.c
===
RCS file: /cvs/src/sys/netinet/in.c,v
retrieving revision 1.133
diff -u -p -r1.133 in.c
--- netinet/in.c20 Dec 2016 12:35:38 -  1.133
+++ netinet/in.c6 Feb 2017 11:15:45 -
@@ -390,7 +390,11 @@ in_ioctl(u_long cmd, caddr_t data, struc
default:
if (ifp->if_ioctl == NULL)
return (EOPNOTSUPP);
-   return ((*ifp->if_ioctl)(ifp, cmd, data));
+   /* XXXSMP breaks atomicity */
+   rw_exit_write();
+   error = ((*ifp->if_ioctl)(ifp, cmd, data));
+   rw_enter_write();
+   return (error);
}
return (0);
 }
Index: netinet6/in6.c
===
RCS file: /cvs/src/sys/netinet6/in6.c,v
retrieving revision 1.197
diff -u -p -r1.197 in6.c
--- netinet6/in6.c  5 Feb 2017 16:04:14 -   1.197
+++ netinet6/in6.c  6 Feb 2017 11:16:09 -
@@ -190,6 +190,7 @@ in6_ioctl(u_long cmd, caddr_t data, stru
struct  in6_ifaddr *ia6 = NULL;
struct  in6_aliasreq *ifra = (struct in6_aliasreq *)data;
struct sockaddr_in6 *sa6;
+   int error;
 
if (ifp == NULL)
return (EOPNOTSUPP);
@@ -469,9 +470,13 @@ in6_ioctl(u_long cmd, caddr_t data, stru
break;
 
default:
-   if (ifp == NULL || ifp->if_ioctl == 0)
+   if (ifp->if_ioctl == NULL)
return (EOPNOTSUPP);
-   return ((*ifp->if_ioctl)(ifp, cmd, data));
+   /* XXXSMP breaks atomicity */
+   rw_exit_write();
+   error = ((*ifp->if_ioctl)(ifp, cmd, data));
+   rw_enter_write();
+   return (error);
}
 
return (0);
Index: dev/pci/if_iwm.c
===
RCS file: /cvs/src/sys/dev/pci/if_iwm.c,v
retrieving revision 1.162
diff -u -p -r1.162 if_iwm.c
--- dev/pci/if_iwm.c4 Feb 2017 19:20:59 -   1.162
+++ dev/pci/if_iwm.c6 Feb 2017 11:15:56 -
@@ -6133,18 +6133,13 @@ iwm_ioctl(struct ifnet *ifp, u_long cmd,
struct ifreq *ifr;
int s, err = 0;
 
-   /* XXXSMP breaks atomicity */
-   rw_exit_write();
-
/*
 * Prevent processes from entering this function while another
 * process is tsleep'ing in it.
 */
err = rw_enter(>ioctl_rwl, RW_WRITE | RW_INTR);
-   if (err) {
-   rw_enter_write();
+   if (err)
return err;
-   }
 
s = splnet();
 
@@ -6190,7 +6185,6 @@ iwm_ioctl(struct ifnet *ifp, u_long cmd,
 
splx(s);
rw_exit(>ioctl_rwl);
-   rw_enter_write();
 
return err;
 }

Re: Password corruption in adduser

[Theo Buehler]

On Mon, Feb 06, 2017 at 11:04:02AM +, Raf Czlonka wrote:
> Hi all,
> 
> How about doing it throughout the tree[0]?
> 
> [0] http://marc.info/?m=142689311221135

Thanks. Most of those are already fixed (or have been removed). I found
and fixed the one in smtpd/table.5 right after fixing this one and the
only $2a$ hash remaining is in ssh/auth.c, which I'll leave to others.
You may want to submit it as a separate patch.

Re: Password corruption in adduser

[Raf Czlonka]

Hi all,

How about doing it throughout the tree[0]?

[0] http://marc.info/?m=142689311221135

Cheers,

Raf

On Mon, Feb 06, 2017 at 05:53:22AM GMT, Theo Buehler wrote:
> On Sun, Feb 05, 2017 at 09:47:35PM -0800, Philip Guenther wrote:
> > On Sun, 5 Feb 2017, John McGuigan wrote:
> > > I've noticed something strange in adduser -- when attempting to add a 
> > > user completely though command line argument it seems to corrupt the 
> > > entry in /etc/master.passwd.
> > > 
> > > Example:
> > > 
> > > $ echo "HorseBatteryStaple" | encrypt
> > > $2b$09$ssZSLC6laHsTS7O2FwJ4Mufw6mSS/FGXw.9oNjr3BLTS7DJp5n4M2
> > > 
> > > # adduser -silent -noconfig -uid_start 5000 -group USER -shell ksh \
> > > -message no -batch some.user "" "Some User" \
> > > $2b$09$ssZSLC6laHsTS7O2FwJ4Mufw6mSS/FGXw.9oNjr3BLTS7DJp5n4M2
> > > Added user ``some.user''
> > ...
> > > some.user:b/bin/ksh9/9uoOrbTRaf//3ZprAb9k.hOpfe9vYVqjf1a:5000:5000:: \
> > > 0:0:Some User:/home/some.user:/bin/ksh
> > > 
> > > As you can see the password entry gets corrupted with a 'b/bin/ksh...'
> > 
> > Let's see what the adduser command is seeing by passing that all to 'echo' 
> > instead:
> > 
> > # echo \
> > > adduser -silent -noconfig -uid_start 5000 -group USER -shell ksh \
> > > -message no -batch some.user "" "Some User" \
> > > $2b$09$ssZSLC6laHsTS7O2FwJ4Mufw6mSS/FGXw.9oNjr3BLTS7DJp5n4M2
> > adduser -silent -noconfig -uid_start 5000 -group USER -shell ksh -message 
> > no -batch some.user  Some User b/bin/ksh9/FGXw.9oNjr3BLTS7DJp5n4M2
> > #
> > 
> > Ah, so the expansion is happening *outside* of adduser...in the shell.  
> > Yes, the shell does variable expansion even if the dollar-sign is in the 
> > middle of a word, so it's expanding the variables
> > $2  --> ""
> > $0  --> "/bin/ksh"
> > $ssZSLC6laHsTS7O2FwJ4Mufw6mSS   --> ""
> > 
> > 
> > > Behavior *is* present when hash is wrapped in "
> > 
> > Sure, because double-quotes only stop file-globbing and field splitting 
> > and not variable expansion.  You need single quotes for that:
> > 
> > # echo \
> > > adduser -silent -noconfig -uid_start 5000 -group USER -shell ksh \
> > > -message no -batch some.user "" "Some User" \
> > > '$2b$09$ssZSLC6laHsTS7O2FwJ4Mufw6mSS/FGXw.9oNjr3BLTS7DJp5n4M2'
> > adduser -silent -noconfig -uid_start 5000 -group USER -shell ksh -message 
> > no -batch some.user  Some User 
> > $2b$09$ssZSLC6laHsTS7O2FwJ4Mufw6mSS/FGXw.9oNjr3BLTS7DJp5n4M2
> > #
> 
> The adduser.8 manual page has an example with no quotes in it, so we
> should fix that.  Also, let's use a new hash using $2b$ instead of $2a$.
> 
> Index: adduser.8
> ===
> RCS file: /var/cvs/src/usr.sbin/adduser/adduser.8,v
> retrieving revision 1.44
> diff -u -p -r1.44 adduser.8
> --- adduser.8 24 Dec 2015 16:54:37 -  1.44
> +++ adduser.8 6 Feb 2017 05:49:00 -
> @@ -373,7 +373,7 @@ The password has been created using
>  .Xr encrypt 1 :
>  .Bd -literal -offset indent
>  # adduser -batch falken guest,staff,beer 'Prof. Falken' \e
> -$2a$06$1Sdjxjoxg4cNmT6zAxriGOLgdLXQ3HdJ2dKBbzEk68jSrO1EtLJ3C
> +'$2b$10$aOadQNznQ1YJFnqNaRRneOvYvZAEO7atYiTND3EsLf6afHT5t1UIK'
>  .Ed
>  .Pp
>  Create user
> 

Re: mount(8): some code shuffling to avoid a pledge problem

[Sebastian Benoit]

Theo Buehler(t...@math.ethz.ch) on 2017.02.06 08:17:43 +0100:
> ping
> 
> On Sat, Jan 28, 2017 at 03:25:53PM +0100, Theo Buehler wrote:
> > The problem:
> > 
> > $ mount /dev/tty /tmp
> > Abort trap (core dumped)
> > 
> > The relevant kdump snippet:
> > 
> >  45441 mountCALL  open(0x7f7eb580,0x1)
> >  45441 mountNAMI  "/dev/tty"
> >  45441 mountRET   open 3
> >  45441 mountCALL  ioctl(3,DIOCGDINFO,0x7f7eb350)
> >  45441 mountPLDG  ioctl, "tty", errno 1 Operation not permitted
> >  45441 mountPSIG  SIGABRT SIG_DFL
> > 
> > and the backtrace:
> > 
> > #0  0x1103967a72aa in ioctl () at :2
> > #1  0x110364ca97b9 in readlabelfs (device=0x7f7d0ada "/dev/tty", 
> > verbose=0) at /usr/src/lib/libutil/readlabel.c:128
> > #2  0x1100d4201210 in main () from /usr/obj/sbin/mount/mount
> > 
> > As usual, the fix consists of hoisting the problematic bit above the
> > pledge call.  This has the side effect that the string bashing functions
> > hasopt(), catopt() and maketypelist() are now called without pledge from
> > the getopt switch, but I don't think that's a problem.  It would be easy
> > to postpone the call to maketypelist() until after pledge, if anyone is
> > worried about it.

i think that tradeoff is fine here.

ok benno@

> > 
> > Index: mount.c
> > ===
> > RCS file: /var/cvs/src/sbin/mount/mount.c,v
> > retrieving revision 1.70
> > diff -u -p -r1.70 mount.c
> > --- mount.c 25 Jan 2017 02:33:25 -  1.70
> > +++ mount.c 28 Jan 2017 14:11:28 -
> > @@ -109,9 +109,6 @@ main(int argc, char * const argv[])
> > int all, ch, forceall, i, mntsize, rval, new;
> > char *options, mntpath[PATH_MAX];
> >  
> > -   if (pledge("stdio rpath disklabel proc exec", NULL) == -1)
> > -   err(1, "pledge");
> > -
> > all = forceall = 0;
> > options = NULL;
> > vfstype = "ffs";
> > @@ -168,6 +165,25 @@ main(int argc, char * const argv[])
> > argc -= optind;
> > argv += optind;
> >  
> > +   if (typelist == NULL && argc == 2) {
> > +   /*
> > +* If -t flag has not been specified, and spec contains either
> > +* a ':' or a '@' then assume that an NFS filesystem is being
> > +* specified ala Sun.  If not, check the disklabel for a
> > +* known filesystem type.
> > +*/
> > +   if (strpbrk(argv[0], ":@") != NULL)
> > +   vfstype = "nfs";
> > +   else {
> > +   char *labelfs = readlabelfs(argv[0], 0);
> > +   if (labelfs != NULL)
> > +   vfstype = labelfs;
> > +   }
> > +   }
> > +
> > +   if (pledge("stdio rpath disklabel proc exec", NULL) == -1)
> > +   err(1, "pledge");
> > +
> >  #defineBADTYPE(type)   
> > \
> > (strcmp(type, FSTAB_RO) &&  \
> > strcmp(type, FSTAB_RW) && strcmp(type, FSTAB_RQ))
> > @@ -261,23 +277,7 @@ main(int argc, char * const argv[])
> > mntonname, options, fs->fs_mntops, skip);
> > break;
> > case 2:
> > -   /*
> > -* If -t flag has not been specified, and spec contains either
> > -* a ':' or a '@' then assume that an NFS filesystem is being
> > -* specified ala Sun.  If not, check the disklabel for a
> > -* known filesystem type.
> > -*/
> > -   if (typelist == NULL) {
> > -   if (strpbrk(argv[0], ":@") != NULL)
> > -   vfstype = "nfs";
> > -   else {
> > -   char *labelfs = readlabelfs(argv[0], 0);
> > -   if (labelfs != NULL)
> > -   vfstype = labelfs;
> > -   }
> > -   }
> > -   rval = mountfs(vfstype,
> > -   argv[0], argv[1], options, NULL, 0);
> > +   rval = mountfs(vfstype, argv[0], argv[1], options, NULL, 0);
> > break;
> > default:
> > usage();
> > 
>