Re: broadcast simplex checksum

2021-02-05 Thread richard . n . procter 
Hi, 

Thanks for the background.

I'm ok with your latest diff as-is. I prefer a slightly different 
direction, see below, but not enough to object. 

On Mon, 1 Feb 2021, Alexander Bluhm wrote:

> On Mon, Feb 01, 2021 at 08:08:56AM +1300, Richard Procter wrote:
> > - Might the rule disabling checksum offload for broadcasts on IFF_SIMPLEX
> >   interfaces be weakened to disable checksum offload for all broadcast
> >   packets instead?
> 
> I just copied the condition from ether_resolve():
> 
> /* If broadcasting on a simplex interface, loopback a copy */
> if (ISSET(m->m_flags, M_BCAST) &&
> ISSET(ifp->if_flags, IFF_SIMPLEX) &&
> !m->m_pkthdr.pf.routed) {
> struct mbuf *mcopy;
> 
> /* XXX Should we input an unencrypted IPsec packet? */
> mcopy = m_copym(m, 0, M_COPYALL, M_NOWAIT);
> if (mcopy != NULL)
> if_input_local(ifp, mcopy, af);
> }
> 
> The bug is triggered when the packet is processed by if_input_local().
> 
> > This simplifies the logic, and shouldn't impact performance as
> 
> Yes, performance does not matter.  I just wanted to show that the
> code is necessary for simplex interfaces.

I think the log message and comment suffice to capture the rationale.

I see the following downside with the IFF_SIMPLEX term. The network layer 
offloads its checksum responsibility to the link layer, as an 
optimisation. The more the network layer accomodates the quirks of the 
link layer in making its decision to offload, the greater the layer 
violation.

Independently of this, there is good reason to compute /all/ broadcast 
checksums at the network layer: it is unprofitable to offload them. And 
avoiding unnecessary offload minimises what the link layer must handle on 
the network's behalf, its scope for going wrong.

So, I would rather eliminate the coupling than document it. And I would 
rather do that by eliminating unnecessary offload than disable it for each 
unhandled edge case.

> > - what motivates the new '!m->m_pkthdr.pf.routed??? term?
> 
> Just copied from ether_resolve().  It looks strange I don't know
> why it is there.  I can leave it out in my check if you think this
> is clearer.

It appears to eliminate a local broadcast storm when doing route-to 
the broadcast address:

> > if_ethersubr.c: revision 1.70
> > date: 2003/08/18 11:01:41;  author: dhartmei;  state: Exp;  lines: +3 
-2;
> > prevent looutput() feedback of broadcast/multicast packets if they are
> > pf routed. prevents a kernel lockup with some (non-sensical) route-to
> > r6ules. report and debugging by mpech@. ok itojun@, henning@, mpech@.

As software offload is always safer, it should be fine to omit the 
!m->m_pkthdr.pf.routed term.

So given the above, my preference would be to prune the unnecessary 
offload and weaken

+   if (ISSET(m->m_flags, M_BCAST) &&
+   ISSET(ifp->if_flags, IFF_SIMPLEX) &&
+   !m->m_pkthdr.pf.routed)
+   return (0);

to 

+   if (ISSET(m->m_flags, M_BCAST))
+   return (0);

I like the addition of in_ifcap_cksum() as it can encapsulate the offload 
policy.

cheers, 
Richard. 


> 
> bluhm
>

Re: wg(4): encapsulated transport checksums

2020-06-29 Thread richard . n . procter 
Hi Jason,

On 27/06/2020, at 10:09 PM, Jason A. Donenfeld  wrote:
> [...] I thought I'd lay out my understanding of the matter,
> and you can let me know whether or not this corresponds with what you  
> had in mind:
> [...]

My main concern is the end-to-end TCP or UDP transport checksum of the
tunneled (or inner, or encapsulated) packet. Your argument seems to
concern instead the per-hop IPv4 header checksum (which is also
interesting to look at, but first things first).

As I read it, wg_decap() tells the stack to ignore the transport checksum
of the tunneled packet, and I think this is incorrect for the following
reason: the packet may have been corrupted outside of the tunnel, because
the tunnel needn't cover the entire path the packet took through the
network.

So a tunneled packet may be corrupt, and one addressed to the tunnel
endpoint will be delivered with its end-to-end transport checksum
unverified. Higher layers may receive corrupt data as as result.[*]  

(Routers, as intermediate network elements, are under no obligation to
check end-to-end transport checksums. It isn't their job.)

One might argue (I do not) that this isn't a concern, because encryption
these days is ubiquitous and comes with far stronger integrity checks.
Nonetheless, even here transport checksums remain relevant for two
reasons. Firsly, because networks remain unreliable: I see non-zero
numbers of failed checksums on my systems. And, secondly, because higher
layers require a reliable transport: TLS for instance will drop the
connection when the MAC check fails[1].

Let's turn to the per-hop IPv4 checksum, which wg_decap() also tells the 
stack to ignore. I suspect the cost of verifying that checksum -- 20 bytes 
on a hot cache line -- is negligible both absolutely and relative to the 
cost of decrypting the packet. And as the optimisation appears nowhere 
else in the tree that I can find, removing it would make wg(4) no worse 
than the other tunnels. Do you have evidence that the cost is in fact 
significant?

Further, as Theo pointed out, the link layer has in any case no business 
with the IPv4 checksum, being part of the network layer. Layer violations 
are problematic for at least two reasons. Firstly, they constrict the 
design space. For instance, I suppose the IPv4 checksum needn't be 
per-hop. It might be updated incrementally, increasing its scope. But this 
would be nullified by any link layer that, acting on a false assumption 
about the layer above it, told the stack to ignore that layer's checksum. 
Secondly, layer violations, an optimisation, which almost by definition 
rely on additional premises, burden the correctness obligation and so 
trade less work for the computer in narrow circumstances for (often much) 
more work for the fallible human to show that the process remains sound.

On that balance I now think that skipping the IPv4 check a false economy,
too. Hopefully I have managed to make my reasons clearer this time around, 
please let me know if not, or if you disagree. 

See updated patch below, looking for oks to commit that.
 
cheers,
Richard.

[*] By way of background, I understand the transport checksum was added to
the early ARPANET primarily to guard against corruption inside its
routers[0], after a router fault which "[brought] the network to its
knees". In other words, end-to-end checks are necessary to catch faults in
the routers; link-level checks aren't enough.

More generally, transport checksums set a lower bound on the reliability
of the end-to-end transport, which decouples it from the reliability of  
the underlying network.

[0] Kleinrock "Queuing Systems, Volume 2: Computer Applications",
John Wiley and Sons (1976), p441

[1] RFC5246 TLS v1.2 (2008), section 7.2.2.

TLS's requirement is reasonable: it would otherwise be obliged to
reimplement for itself retransmission, which would drag with it other
transport layer mechanisms like acknowledgements and (for performance)
sequence numbers and sliding windows.

Index: net/if_wg.c
===
RCS file: /cvs/src/sys/net/if_wg.c,v
retrieving revision 1.7
diff -u -p -u -p -r1.7 if_wg.c
--- net/if_wg.c 23 Jun 2020 10:03:49 -  1.7
+++ net/if_wg.c 28 Jun 2020 20:17:07 -
@@ -1660,14 +1660,8 @@ wg_decap(struct wg_softc *sc, struct mbu
goto error;
}
 
-   /*
-* We can mark incoming packet csum OK. We mark all flags OK
-* irrespective to the packet type.
-*/
-   m->m_pkthdr.csum_flags |= (M_IPV4_CSUM_IN_OK | M_TCP_CSUM_IN_OK |
-   M_UDP_CSUM_IN_OK | M_ICMP_CSUM_IN_OK);
-   m->m_pkthdr.csum_flags &= ~(M_IPV4_CSUM_IN_BAD | M_TCP_CSUM_IN_BAD |
-   M_UDP_CSUM_IN_BAD | M_ICMP_CSUM_IN_BAD);
+   /* tunneled packet was not offloaded */
+   m->m_pkthdr.csum_flags = 0;
 
m->m_pkthdr.ph_ifidx = sc->sc_if.if_index;
m->m_pkthdr.ph_rtableid = sc->sc_if.if_rdomain;

wg(4): encapsulated transport checksums

2020-06-26 Thread richard . n . procter 
Hi, 

On its receive path, wg(4) uses the same mbuf for both the encrypted 
capsule and its encapsulated packet, which it passes up to the stack. We 
must therefore clear this mbuf's checksum status flags, as although the 
capsule may have been subject to hardware offload, its encapsulated packet 
was not.

This ensures that the transport checksums of packets bound for local 
delivery are verified. That is necessary because, although the tunnel 
provides stronger integrity checks, the tunnel endpoints and the 
transport endpoints needn't coincide.

However, as the network and tunnel endpoints _do_ conincide, it remains 
unncessary to check the per-hop IPv4 checksum.

ok? 

Index: net/if_wg.c
===
RCS file: /cvs/src/sys/net/if_wg.c,v
retrieving revision 1.7
diff -u -p -u -p -r1.7 if_wg.c
--- net/if_wg.c 23 Jun 2020 10:03:49 -  1.7
+++ net/if_wg.c 27 Jun 2020 02:48:37 -
@@ -1660,14 +1660,10 @@ wg_decap(struct wg_softc *sc, struct mbu
goto error;
}
 
-   /*
-* We can mark incoming packet csum OK. We mark all flags OK
-* irrespective to the packet type.
-*/
-   m->m_pkthdr.csum_flags |= (M_IPV4_CSUM_IN_OK | M_TCP_CSUM_IN_OK |
-   M_UDP_CSUM_IN_OK | M_ICMP_CSUM_IN_OK);
-   m->m_pkthdr.csum_flags &= ~(M_IPV4_CSUM_IN_BAD | M_TCP_CSUM_IN_BAD |
-   M_UDP_CSUM_IN_BAD | M_ICMP_CSUM_IN_BAD);
+   /* tunneled packet was not offloaded */
+   m->m_pkthdr.csum_flags = 0;
+   /* optimise: the tunnel provided a stronger integrity check */
+   m->m_pkthdr.csum_flags |= M_IPV4_CSUM_IN_OK;
 
m->m_pkthdr.ph_ifidx = sc->sc_if.if_index;
m->m_pkthdr.ph_rtableid = sc->sc_if.if_rdomain;

Re: [PATCH] add ping(1)-like stats to tcpbench(1)

2020-05-02 Thread richard . n . procter 



On Sat, 2 May 2020, j...@bitminer.ca wrote:

> A couple of further questions embedded:
> 
> A question on the std-dev -- is this for "n" measures as defined
> by -r interval?  "ping" reports an N packets transmitted.  Maybe
> this is obvious but your revised manpage doesn't say.  Could this
> be added to the output report?

Correct: the summary stats are computed over the periodic throughput 
statistics. I have tried to make this clearer in the man page.

Note however: for simplicity the summary duration covers the interval from 
first transfer to process exit. This though may differ significantly from 
the sum of sample durations only for the server, as only it may fall idle 
during the session. 

I did consider adding the number of samples and their duration to the 
summary but discarded it early on as possibly misleading for that reason 
(and, as a cosmetic thing, it required another line of output).

cheers, 
Richard. 

Index: tcpbench.1
===
RCS file: /cvs/src/usr.bin/tcpbench/tcpbench.1,v
retrieving revision 1.26
diff -u -p -u -p -r1.26 tcpbench.1
--- tcpbench.1  12 Feb 2020 14:46:36 -  1.26
+++ tcpbench.1  2 May 2020 20:49:02 -
@@ -65,14 +65,23 @@ of a listening server to connect to.
 .Pp
 Once connected, the client will send TCP or UDP traffic as fast as possible to
 the server.
-Both the client and server will periodically display throughput
-statistics along with any kernel variables the user has selected to
+Both the client and server will periodically compute and display throughput
+statistics.
+The server starts computing these for UDP on receipt of the first datagram,
+and stops for TCP when it has no connections.
+This display also includes any kernel variables the user has selected to
 sample (using the
 .Fl k
 option, which is only available in TCP mode).
 A list of available kernel variables may be obtained using the
 .Fl l
 option.
+.Pp
+A summary over the periodic throughput statistics is displayed on exit.
+Its accuracy may be increased by decreasing the
+.Ar interval .
+The summary bytes and duration cover the interval from transfer start
+to process exit.
 .Pp
 The options are as follows:
 .Bl -tag -width Ds

Re: [PATCH] add ping(1)-like stats to tcpbench(1)

2020-05-02 Thread richard . n . procter 



On Fri, 1 May 2020, j...@bitminer.ca wrote:
> > From: richard.n.procter () gmail ! com
> > This implements ping(1)-like summary statistics for tcpbench(1), e.g.
> > 
> > ^C
> > --- localhost tcpbench statistics ---
> > 1099642814 bytes sent over 4.126 seconds
> > bandwidth min/avg/max/std-dev = 1228.131/2117.309/2491.399/517.779 Mbps
> 
> Std-dev assumes a Gaussian distribution which is unlikely if there are issues.
> (I would agree that a working LAN link would have very low std-dev.)

Hi, thanks for the feedback. 

I believe the standard deviation is well-defined for any random variable 
that has a mean[0]; I'm happy to be corrected.

> Percentiles would be more informative and avoids the Gaussian assumption.

I agree percentiles would also be useful but that is a next-step. 
 
> TCP also benefits/suffers from slowstart, which would bias any short-term
> measures.  Why not suggest users just "tcpbench -r 200" and compute
> their own statistics?  (Note your example above is probably showing TCP
> slowstart behaviour).

Anyone remains free to compute their own stats (but I know from experience 
that this is tedious, hence this patch).

Regarding slow start: as tcpbench(1) benchmarks TCP, the stats ought to 
cover TCP connection performance. This includes slow start and the effects 
of congestion control more generally. Slow start can be effaced from the 
summary stats if desired and necessary by increasing the sample interval, 
or by increasing the benchmark duration (excepting the minimum).

> 
> > +   printf("bandwidth min/avg/max/std-dev = %.3Lf/%.3Lf/%.3Lf/%.3Lf
> > Mbps\n",
> > +   mainstats.floor_mbps, mainstats.mean_mbps, mainstats.peak_mbps,
> > +   std_dev);
> 
> More assumptions are implied here.  How do I measure a 2400 bps link with this
> report?  The precision is lost in "%.3Lf".

I highly doubt anyone needs to benchmark a 2400 bps link. The display is 
in any case precise to 1000 bps.

> Some nits:
> 
> > +   long double mbps, old_mean_mbps, slice_mbps = 0;
>
> "long double" is a waste of bits.  A double is sufficient.

Is there a cost? (Note, the type was established prior to this patch)

> > -   long double mbps, slice_mbps = 0;
> > +   long double mbps, old_mean_mbps, slice_mbps = 0;

> No diff for the manpage.

Good point; ping(1) mentions its stats display. See updated patch below. 

> A question: when does a TCP "std-dev" tell you more than a ping "std-dev"?

As their units differ, they serve distinct questions. 

cheers, 
Richard. 

[0] Leonard Kleinrock Queuing Systems: Volume 1: Theory John Wiley & Sons
1975; Appendix II, Probability Theory Refresher.

Index: tcpbench.1
===
RCS file: /cvs/src/usr.bin/tcpbench/tcpbench.1,v
retrieving revision 1.26
diff -u -p -u -p -r1.26 tcpbench.1
--- tcpbench.1  12 Feb 2020 14:46:36 -  1.26
+++ tcpbench.1  2 May 2020 11:32:13 -
@@ -65,14 +65,21 @@ of a listening server to connect to.
 .Pp
 Once connected, the client will send TCP or UDP traffic as fast as possible to
 the server.
-Both the client and server will periodically display throughput
-statistics along with any kernel variables the user has selected to
+Both the client and server will periodically compute and display throughput
+statistics.
+The server starts computing these for UDP on receipt of the first datagram,
+and stops for TCP when no connections are active.
+This display also includes any kernel variables the user has selected to
 sample (using the
 .Fl k
 option, which is only available in TCP mode).
 A list of available kernel variables may be obtained using the
 .Fl l
 option.
+.Pp
+A summary over all throughput statistics is displayed on exit.
+Its accuracy may be increased by decreasing the
+.Ar interval .
 .Pp
 The options are as follows:
 .Bl -tag -width Ds

Index: Makefile
===
RCS file: /cvs/src/usr.bin/tcpbench/Makefile,v
retrieving revision 1.8
diff -u -p -u -p -r1.8 Makefile
--- Makefile9 Jul 2017 21:23:19 -   1.8
+++ Makefile1 May 2020 09:57:26 -
@@ -1,7 +1,7 @@
 #  $OpenBSD: Makefile,v 1.8 2017/07/09 21:23:19 espie Exp $
 
 PROG=  tcpbench
-LDADD= -lkvm -levent
-DPADD= ${LIBKVM} ${LIBEVENT}
+LDADD= -lm -lkvm -levent
+DPADD= ${LIBM} ${LIBKVM} ${LIBEVENT}
 
 .include 
Index: tcpbench.c
===
RCS file: /cvs/src/usr.bin/tcpbench/tcpbench.c,v
retrieving revision 1.61
diff -u -p -u -p -r1.61 tcpbench.c
--- tcpbench.c  12 Feb 2020 14:46:36 -  1.61
+++ tcpbench.c  1 May 2020 09:57:26 -
@@ -50,6 +50,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -101,6 +102,8 @@ struct statctx {
u_long udp_slice_pkts;
 };
 
+struct statctx *udp_sc; /* singleton */
+
 static voidsignal_handler(int, short, void *);
 static voidsaddr_ntop(const struct sockaddr *, socklen_t, char *, size_t);
 static void

[PATCH] add ping(1)-like stats to tcpbench(1)

2020-05-01 Thread richard . n . procter 
Hi, 

This implements ping(1)-like summary statistics for tcpbench(1), e.g.

^C
--- localhost tcpbench statistics ---
1099642814 bytes sent over 4.126 seconds
bandwidth min/avg/max/std-dev = 1228.131/2117.309/2491.399/517.779 Mbps

The std-dev especially would have helped me catch a TCP performance 
problem sooner last year.

The patch is longish; I can split it up if wanted. Note:

- the udp struct statctx singleton is now a global; this lets
  udp_process_slice() drop an argument to match tcp_process_slice()

- adopt ping(1)'s output cosmetics on ^C 

- to make the client/server stats more comparable:
   - the udp server now computes stats from first read, not process start.
   - the tcp server now skips stats for slices that complete with
 no established connections. 

I'm no expert on the intricacies of floating point and I referred to 
[0][1][2] for the online/streaming algorithms. 

djm@ liked the direction; looking for oks on the code. 

cheers, 
Richard. 

[0] 
https://en.wikipedia.org/wiki/Algorithms_for_calculating_variance#Welford's_online_algorithm
[1] 
https://nestedsoftware.com/2018/03/20/calculating-a-moving-average-on-streaming-data-5a7k.22879.html
[2] 
https://nestedsoftware.com/2018/03/27/calculating-standard-deviation-on-streaming-data-253l.23919.html

Index: Makefile
===
RCS file: /cvs/src/usr.bin/tcpbench/Makefile,v
retrieving revision 1.8
diff -u -p -u -p -r1.8 Makefile
--- Makefile9 Jul 2017 21:23:19 -   1.8
+++ Makefile1 May 2020 09:57:26 -
@@ -1,7 +1,7 @@
 #  $OpenBSD: Makefile,v 1.8 2017/07/09 21:23:19 espie Exp $
 
 PROG=  tcpbench
-LDADD= -lkvm -levent
-DPADD= ${LIBKVM} ${LIBEVENT}
+LDADD= -lm -lkvm -levent
+DPADD= ${LIBM} ${LIBKVM} ${LIBEVENT}
 
 .include 
Index: tcpbench.c
===
RCS file: /cvs/src/usr.bin/tcpbench/tcpbench.c,v
retrieving revision 1.61
diff -u -p -u -p -r1.61 tcpbench.c
--- tcpbench.c  12 Feb 2020 14:46:36 -  1.61
+++ tcpbench.c  1 May 2020 09:57:26 -
@@ -50,6 +50,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -101,6 +102,8 @@ struct statctx {
u_long udp_slice_pkts;
 };
 
+struct statctx *udp_sc; /* singleton */
+
 static voidsignal_handler(int, short, void *);
 static voidsaddr_ntop(const struct sockaddr *, socklen_t, char *, size_t);
 static voiddrop_gid(void);
@@ -114,19 +117,22 @@ static void   list_kvars(void);
 static voidcheck_kvar(const char *);
 static char ** check_prepare_kvars(char *);
 static voidstats_prepare(struct statctx *);
+static voidsummary_display(void);
 static voidtcp_stats_display(unsigned long long, long double, float,
 struct statctx *, struct inpcb *, struct tcpcb *, struct socket *);
 static voidtcp_process_slice(int, short, void *);
 static voidtcp_server_handle_sc(int, short, void *);
 static voidtcp_server_accept(int, short, void *);
-static voidserver_init(struct addrinfo *, struct statctx *);
+static voidserver_init(struct addrinfo *);
 static voidclient_handle_sc(int, short, void *);
-static voidclient_init(struct addrinfo *, int, struct statctx *,
-struct addrinfo *);
+static voidclient_init(struct addrinfo *, int, struct addrinfo *);
 static int clock_gettime_tv(clockid_t, struct timeval *);
 static voidudp_server_handle_sc(int, short, void *);
 static voidudp_process_slice(int, short, void *);
 static int map_tos(char *, int *);
+static voidquit(int, short, void *);
+static voidwrapup(int);
+
 /*
  * We account the mainstats here, that is the stats
  * for all connections, all variables starting with slice
@@ -135,10 +141,17 @@ static intmap_tos(char *, int *);
  * between all slices so far.
  */
 static struct {
-   unsigned long long slice_bytes; /* bytes for last slice */
+   struct timeval t_first; /* first connect / packet */
+   unsigned long long total_bytes; /* bytes since t_first */
+   unsigned long long n_slices;/* slices since start */
+   unsigned long long slice_bytes; /* bytes since slice reset */
long double peak_mbps;  /* peak mbps so far */
+   long double floor_mbps; /* floor mbps so far */
+   long double mean_mbps;  /* mean mbps so far */
+   long double nvariance_mbps; /* for online std dev */
int nconns; /* connected clients */
struct event timer; /* process timer */
+   const char *host;   /* remote server for display */
 } mainstats;
 
 /* When adding variables, also add to tcp_stats_display() */
@@ -201,10 +214,13 @@ signal_handler(int sig, short event, voi
 */
switch (sig) {
case SIGINT:
+   printf("\n");
+   wrapup(0);
+   break;  /* NOTREACHED */
case SIGTERM:
case

Re: fix possible nexthop list corruption in bgpd

2020-01-24 Thread richard . n . procter 



On Sat, 25 Jan 2020, Claudio Jeker wrote:

> Adam Thompson reported a bgpd crash he sees in 6.6. Using
> kern.nosuidcoredump=3 he was able to get me a back trace of the crash.
> The RDE chokes on the TAILQ_REMOVE in nexthop_runner() which indicates
> that the nexthop_runners list gets corrupted.
> After staring at the code for a while I realized that it is possible that
> a nexthop is put on the runner list even though there are no objects to
> process. In this case if nexthop_unref() is called before the
> nexthop_runner() had a chance to run and remove that nexthop the queue
> will be corrupt because of a use-after-free of that element.
> Fix is simple, check before enqueuing a nexthop on the nexthop_runners
> queue that next_prefix is not NULL.
> 
> The 2nd hunk just adds a debug log in the case where a prefix removal
> actually completes the nexthop run.
> 
> OK?

If I understand right, the nexthop is vulnerable to being freed when its 
prefix_h list is empty; so don't queue these to the nexthop_runner(), 
which will be a no-op anyways (except for the debug message, which you've 
added).

ok procter@ 

> -- 
> :wq Claudio
> 
> Index: rde_rib.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/rde_rib.c,v
> retrieving revision 1.214
> diff -u -p -r1.214 rde_rib.c
> --- rde_rib.c 10 Jan 2020 14:52:57 -  1.214
> +++ rde_rib.c 23 Jan 2020 03:59:09 -
> @@ -1800,8 +1800,11 @@ nexthop_update(struct kroute_nexthop *ms
>   nh->nexthop_netlen = msg->netlen;
>  
>   nh->next_prefix = LIST_FIRST(>prefix_h);
> - TAILQ_INSERT_HEAD(_runners, nh, runner_l);
> - log_debug("nexthop %s update starting", log_addr(>exit_nexthop));
> + if (nh->next_prefix != NULL) {
> + TAILQ_INSERT_HEAD(_runners, nh, runner_l);
> + log_debug("nexthop %s update starting",
> + log_addr(>exit_nexthop));
> + }
>  }
>  
>  void
> @@ -1860,8 +1863,11 @@ nexthop_unlink(struct prefix *p)
>   if (p == p->nexthop->next_prefix) {
>   p->nexthop->next_prefix = LIST_NEXT(p, entry.list.nexthop);
>   /* remove nexthop from list if no prefixes left to update */
> - if (p->nexthop->next_prefix == NULL)
> + if (p->nexthop->next_prefix == NULL) {
>   TAILQ_REMOVE(_runners, p->nexthop, runner_l);
> + log_debug("nexthop %s update finished",
> + log_addr(>nexthop->exit_nexthop));
> + }
>   }
>  
>   p->flags &= ~PREFIX_NEXTHOP_LINKED;
> 
>

pf: remove 'one shot rules'

2020-01-24 Thread richard . n . procter 
Hi,

PF supports 'one shot rules'. Quoting pf.conf(5) "once - Creates a one 
shot rule that will remove itself from an active ruleset after the first 
match."

I'd like to simplify pf by removing them, unless there's a compelling 
reason not to.

Particularly as there is no 'first match' under concurrent ruleset 
evaluation (which we aim at). That obliges the code for one-shot rules to 
either serialise evaluation to some degree (hurting performance) or fail 
to do what it says on the tin in all cases (hurting correctness). Either 
way the code creates surprises and has to do a lot of hoop-jumping.

Thoughts?

cheers,
Richard. 


Index: sbin/pfctl/parse.y
===
RCS file: /cvs/src/sbin/pfctl/parse.y,v
retrieving revision 1.700
diff -u -p -u -p -r1.700 parse.y
--- sbin/pfctl/parse.y  15 Jan 2020 22:38:30 -  1.700
+++ sbin/pfctl/parse.y  24 Jan 2020 06:12:06 -
@@ -242,7 +242,6 @@ struct filter_opts {
 #define FOM_SETTOS 0x0100
 #define FOM_SCRUB_TCP  0x0200
 #define FOM_SETPRIO0x0400
-#define FOM_ONCE   0x1000
 #define FOM_PRIO   0x2000
 #define FOM_SETDELAY   0x4000
struct node_uid *uid;
@@ -487,7 +486,7 @@ int parseport(char *, struct range *r, i
 %token BITMASK RANDOM SOURCEHASH ROUNDROBIN LEASTSTATES STATICPORT PROBABILITY
 %token WEIGHT BANDWIDTH FLOWS QUANTUM
 %token QUEUE PRIORITY QLIMIT RTABLE RDOMAIN MINIMUM BURST PARENT
-%token LOAD RULESET_OPTIMIZATION RTABLE RDOMAIN PRIO ONCE DEFAULT DELAY
+%token LOAD RULESET_OPTIMIZATION RTABLE RDOMAIN PRIO DEFAULT DELAY
 %token STICKYADDRESS MAXSRCSTATES MAXSRCNODES SOURCETRACK GLOBAL RULE
 %token MAXSRCCONN MAXSRCCONNRATE OVERLOAD FLUSH SLOPPY PFLOW MAXPKTRATE
 %token TAGGED TAG IFBOUND FLOATING STATEPOLICY STATEDEFAULTS ROUTE
@@ -2163,9 +2162,6 @@ filter_opt: USER uids {
filter_opts.rcv = $3;
filter_opts.rcv->not = $1;
}
-   | ONCE {
-   filter_opts.marker |= FOM_ONCE;
-   }
| MAXPKTRATE NUMBER '/' NUMBER {
if ($2 < 0 || $2 > UINT_MAX ||
$4 < 0 || $4 > UINT_MAX) {
@@ -5086,7 +5082,6 @@ lookup(char *s)
{ "no-route",   NOROUTE},
{ "no-sync",NOSYNC},
{ "on", ON},
-   { "once",   ONCE},
{ "optimization",   OPTIMIZATION},
{ "os", OS},
{ "out",OUT},
@@ -5909,14 +5904,6 @@ rdomain_exists(u_int rdomain)
 int
 filteropts_to_rule(struct pf_rule *r, struct filter_opts *opts)
 {
-   if (opts->marker & FOM_ONCE) {
-   if ((r->action != PF_PASS && r->action != PF_DROP) || 
r->anchor) {
-   yyerror("'once' only applies to pass/block rules");
-   return (1);
-   }
-   r->rule_flag |= PFRULE_ONCE;
-   }
-
r->keep_state = opts->keep.action;
r->pktrate.limit = opts->pktrate.limit;
r->pktrate.seconds = opts->pktrate.seconds;
Index: sbin/pfctl/pfctl_parser.c
===
RCS file: /cvs/src/sbin/pfctl/pfctl_parser.c,v
retrieving revision 1.342
diff -u -p -u -p -r1.342 pfctl_parser.c
--- sbin/pfctl/pfctl_parser.c   17 Oct 2019 21:54:28 -  1.342
+++ sbin/pfctl/pfctl_parser.c   24 Jan 2020 06:12:06 -
@@ -1085,8 +1085,6 @@ print_rule(struct pf_rule *r, const char
printf(" allow-opts");
if (r->label[0])
printf(" label \"%s\"", r->label);
-   if (r->rule_flag & PFRULE_ONCE)
-   printf(" once");
if (r->tagname[0])
printf(" tag %s", r->tagname);
if (r->match_tagname[0]) {
Index: share/man/man5/pf.conf.5
===
RCS file: /cvs/src/share/man/man5/pf.conf.5,v
retrieving revision 1.583
diff -u -p -u -p -r1.583 pf.conf.5
--- share/man/man5/pf.conf.517 Jan 2020 09:07:35 -  1.583
+++ share/man/man5/pf.conf.524 Jan 2020 06:12:08 -
@@ -639,12 +639,6 @@ pass in proto icmp max-pkt-rate 100/10
 When the rate is exceeded, all ICMP is blocked until the rate falls below
 100 per 10 seconds again.
 .Pp
-.It Cm once
-Creates a one shot rule that will remove itself from an active ruleset after
-the first match.
-In case this is the only rule in the anchor, the anchor will be destroyed
-automatically after the rule is matched.
-.Pp
 .It Cm probability Ar number Ns %
 A probability attribute can be attached to a rule,
 with a value set between 0 and 100%,
Index: sys/net/pf.c
===
RCS file: /cvs/src/sys/net/pf.c,v
retrieving revision 1.1091
diff -u -p -u -p -r1.1091 pf.c
--- sys/net/pf.c17 Nov 2019 08:25:05 -  1.1091
+++

npppd(4) normalise checksum update method

2020-01-24 Thread richard . n . procter 
Hi, 

npppd(8) and pipex(4) can clamp TCP MSS independently of pf(4)
and so tweak the TCP checksum, too.

Substitute pf's algorithm to reduce the diversity of checksum-tweaking 
algorithms in the tree.

Compiled but untested. 

oks or test reports welcome (enable mss clamping by adding 'tcp-mss-adjust 
yes' to npppd.conf tunnel spec).

Richard.

Index: sys/net/pipex.c
===
RCS file: /cvs/src/sys/net/pipex.c,v
retrieving revision 1.107
diff -u -p -u -p -r1.107 pipex.c
--- sys/net/pipex.c 31 Jan 2019 18:01:14 -  1.107
+++ sys/net/pipex.c 24 Jan 2020 21:46:22 -
@@ -2691,24 +2691,14 @@ pipex_ccp_output(struct pipex_session *s
 #define TCP_OPTLEN_IN_SEGMENT  12  /* timestamp option and padding */
 #define MAXMSS(mtu) (mtu - sizeof(struct ip) - sizeof(struct tcphdr) - \
 TCP_OPTLEN_IN_SEGMENT)
-/*
- * The following macro is used to update an internet checksum.  "acc" is a
- * 32-bit accumulation of all the changes to the checksum (adding in old
- * 16-bit words and subtracting out new words), and "cksum" is the checksum
- * value to be updated.
- */
-#define ADJUST_CHECKSUM(acc, cksum) {  \
-   acc += cksum;   \
-   if (acc < 0) {  \
-   acc = -acc; \
-   acc = (acc >> 16) + (acc & 0x); \
-   acc += acc >> 16;   \
-   cksum = (u_short) ~acc; \
-   } else {\
-   acc = (acc >> 16) + (acc & 0x); \
-   acc += acc >> 16;   \
-   cksum = (u_short) acc;  \
-   }   \
+
+static inline void
+in_cksum_fixup(u_int16_t *cksum, u_int16_t was, u_int16_t now)
+{
+   u_int32_t x;
+   x = *cksum + was - now;
+   x = (x + (x >> 16)) & 0x; // see pf_cksum_fixup()
+   *cksum = (u_int16_t)(x);
 }
 
 /*
@@ -2719,7 +2709,7 @@ pipex_ccp_output(struct pipex_session *s
 Static struct mbuf *
 adjust_tcp_mss(struct mbuf *m0, int mtu)
 {
-   int opt, optlen, acc, mss, maxmss, lpktp;
+   int opt, optlen, mss, maxmss, lpktp;
struct ip *pip;
struct tcphdr *th;
u_char *pktp, *mssp;
@@ -2772,9 +2762,7 @@ adjust_tcp_mss(struct mbuf *m0, int mtu)
PIPEX_DBG((NULL, LOG_DEBUG,
"change tcp-mss %d => %d", mss, maxmss));
PUTSHORT(maxmss, mssp);
-   acc = htons(mss);
-   acc -= htons(maxmss);
-   ADJUST_CHECKSUM(acc, th->th_sum);
+   in_cksum_fixup(>th_sum, htons(mss), 
htons(maxmss));
}
goto handled;
/* NOTREACHED */
Index: usr.sbin/npppd/npppd/npppd_subr.c
===
RCS file: /cvs/src/usr.sbin/npppd/npppd/npppd_subr.c,v
retrieving revision 1.20
diff -u -p -u -p -r1.20 npppd_subr.c
--- usr.sbin/npppd/npppd/npppd_subr.c   10 May 2019 01:29:31 -  1.20
+++ usr.sbin/npppd/npppd/npppd_subr.c   24 Jan 2020 21:46:26 -
@@ -451,24 +451,13 @@ in_addr_range_delete_route(struct in_add
  * $FreeBSD: src/usr.sbin/ppp/tcpmss.c,v 1.1.4.3 2001/07/19 11:39:54 brian Exp 
$
  */
 
-/*
- * The following macro is used to update an internet checksum.  "acc" is a
- * 32-bit accumulation of all the changes to the checksum (adding in old
- * 16-bit words and subtracting out new words), and "cksum" is the checksum
- * value to be updated.
- */
-#define ADJUST_CHECKSUM(acc, cksum) {  \
-   acc += cksum;   \
-   if (acc < 0) {  \
-   acc = -acc; \
-   acc = (acc >> 16) + (acc & 0x); \
-   acc += acc >> 16;   \
-   cksum = (u_short) ~acc; \
-   } else {\
-   acc = (acc >> 16) + (acc & 0x); \
-   acc += acc >> 16;   \
-   cksum = (u_short) acc;  \
-   }   \
+static inline void
+in_cksum_fixup(u_int16_t *cksum, u_int16_t was, u_int16_t now)
+{
+   u_int32_t x;
+   x = *cksum + was - now;
+   x = (x + (x >> 16)) & 0x; // see pf_cksum_fixup()
+   *cksum = (u_int16_t)(x);
 }
 
 /**
@@ -481,7 +470,7 @@ in_addr_range_delete_route(struct in_add
 int
 adjust_tcp_mss(u_char *pktp, int lpktp, int mtu)
 {
-   int opt, optlen, acc, ip_off, mss, maxmss;
+   int opt, optlen, ip_off, mss, maxmss;
struct ip *pip;
struct

remove spurious checksumming attempts

2020-01-23 Thread richard . n . procter 
Hi, 

dlg@ pointed out what looks to be a spurious call in the bridge to 
in_proto_cksum_out(). I've checked the stack and found eight such calls 
I think can be safely removed.

in_proto_cksum_out() computes, for packets flagged as needing it, the 
transport checksum. It skips if the output interface is known and supports 
checksum offload.

The removed calls are either leftovers from when PF was opaque to 
checksums, or look to have been copied from these. (PF would modify 
packets leaving the invalidated the checksums to be recomputed later on, 
which induced recomputations on the input or forwarding paths.)

The correctness obligation: show for every call that it is not last on the 
only path that may emit un-checksummed packets, viz. the IP output path 
(i.e. non-forwarding, non-input).

if_bridge.c: the bridge operates at L2, so not on the IP output path.
ip_input.c: not in the IP output path
ip6_forward.c: not in the IP output path
ip6_output.c: ip6_output_ipsec_send() is called only by ip6_forward(), 
  so lies output the IP output path, too.

Testing: error symptoms would be bad transport checksums from 
non-offloading interfaces or in encapsulated packets.

Looking for OKs, or test reports, particularly for:
   - PF filtering the bridge
   - IP6 forwarding 

cheers, 
Richard. 

Index: net/if_bridge.c
===
RCS file: /cvs/src/sys/net/if_bridge.c,v
retrieving revision 1.338
diff -u -p -U12 -p -r1.338 if_bridge.c
--- net/if_bridge.c 6 Nov 2019 03:51:26 -   1.338
+++ net/if_bridge.c 24 Jan 2020 04:37:01 -
@@ -1561,32 +1561,25 @@ bridge_ipsec(struct ifnet *ifp, struct e
 * We don't need to do loop detection, the
 * bridge will do that for us.
 */
 #if NPF > 0
if ((encif = enc_getif(tdb->tdb_rdomain,
tdb->tdb_tap)) == NULL ||
pf_test(af, dir, encif, ) != PF_PASS) {
m_freem(m);
return (1);
}
if (m == NULL)
return (1);
-   else if (af == AF_INET)
-   in_proto_cksum_out(m, encif);
-#ifdef INET6
-   else if (af == AF_INET6)
-   in6_proto_cksum_out(m, encif);
-#endif /* INET6 */
 #endif /* NPF */
-
ip = mtod(m, struct ip *);
if ((af == AF_INET) &&
ip_mtudisc && (ip->ip_off & htons(IP_DF)) &&
tdb->tdb_mtu && ntohs(ip->ip_len) > tdb->tdb_mtu &&
tdb->tdb_mtutimeout > time_second)
bridge_send_icmp_err(ifp, eh, m,
hassnap, llc, tdb->tdb_mtu,
ICMP_UNREACH, ICMP_UNREACH_NEEDFRAG);
else
error = ipsp_process_packet(m, tdb, af, 0);
return (1);
} else
@@ -1715,25 +1708,24 @@ bridge_ip(struct ifnet *brifp, int dir, 
/* Finally, we get to filter the packet! */
if (pf_test(AF_INET, dir, ifp, ) != PF_PASS)
goto dropit;
if (m == NULL)
goto dropit;
 #endif /* NPF > 0 */
 
/* Rebuild the IP header */
if (m->m_len < hlen && ((m = m_pullup(m, hlen)) == NULL))
return (NULL);
if (m->m_len < sizeof(struct ip))
goto dropit;
-   in_proto_cksum_out(m, ifp);
ip = mtod(m, struct ip *);
ip->ip_sum = 0;
if (0 && (ifp->if_capabilities & IFCAP_CSUM_IPv4))
m->m_pkthdr.csum_flags |= M_IPV4_CSUM_OUT;
else {
ipstat_inc(ips_outswcsum);
ip->ip_sum = in_cksum(m, hlen);
}
 
break;
 
 #ifdef INET6
@@ -1761,26 +1753,24 @@ bridge_ip(struct ifnet *brifp, int dir, 
if ((brifp->if_flags & IFF_LINK2) == IFF_LINK2 &&
bridge_ipsec(ifp, eh, hassnap, , dir, AF_INET6, hlen,
m))
return (NULL);
 #endif /* IPSEC */
 
 #if NPF > 0
if (pf_test(AF_INET6, dir, ifp, ) != PF_PASS)
goto dropit;
if (m == NULL)
return (NULL);
 #endif /* NPF > 0 */
-   in6_proto_cksum_out(m, ifp);
-
break;
}
 #endif /* INET6 */
 
default:
goto dropit;
break;
}
 
/* Reattach SNAP header */
if (hassnap) {
M_PREPEND(m, LLC_SNAPFRAMELEN, M_DONTWAIT);

Re: once again: iwm(4) multi-frame rx + monitor mode

2019-09-04 Thread richard . n . procter 
On Fri, 30 Aug 2019, Stefan Sperling wrote:

> I would like to try this again: In iwm(4), process more than one frame
> per Rx interrupt, and enable monitor mode.
> 
> Monitor mode triggers "unhandled fimware response" errors without multi-Rx
> support. We have seen these infamous errors in other contexts as well.
> It is possible that the firmware decides to use multi-Rx in such cases
> which would of course confuse our driver.
> [...]
> Please test this on as many iwm devices as possible. Thanks!

I've tested this some. I used nc(1) to transfer a 500MB file between 
iwm(4) and my ral(4) AP once in each direction, both with and without the 
patch; diff'd the netstat -s output; and tried the one of the most 
demanding tests I know for an interface, which is:

AP# ping -q -s 10 -l 400 -c 400 -f iwm_machine

I've seen no problems or regressions. The patched transfer saw ~1/3 of the 
duplicate packets received but this could easily be noise. Every ping was
received, according to netstat. 

A big caveat is that my ral(4) AP is 11g and unlikely to be pushing iwm(4) 
hard. I forced ral(4) to OFDM54, and iwm(4) was seeing 73% signal strength 
from it.

I've not used monitor mode before. I was expecting to see frames from 
other nodes in tcpdump with monitor mode enabled, but didn't, whether 
iwm(4) was associated with a network or not. I didn't see any "unhandled 
firmware response" errors.

best, 
Richard.

iwm0 at pci1 dev 0 function 0 "Intel Dual Band Wireless AC 7265" rev 0x69, msi
iwm0: hw rev 0x210, fw ver 16.242414.0, address xx:xx:xx:xx:xx:xx

> 
> diff refs/heads/master refs/heads/iwm-multirx
> blob - 42cb00c62cc455d871da09560dcf82876db2f4c0
> blob + c9a4dc36c3a7cf09c9836141b6a966370ac94824
> --- sys/dev/pci/if_iwm.c
> +++ sys/dev/pci/if_iwm.c
> @@ -367,6 +367,7 @@ int   iwm_get_signal_strength(struct iwm_softc *, 
> struct
>  void iwm_rx_rx_phy_cmd(struct iwm_softc *, struct iwm_rx_packet *,
>   struct iwm_rx_data *);
>  int  iwm_get_noise(const struct iwm_statistics_rx_non_phy *);
> +int  iwm_rx_frame(struct iwm_softc *, struct mbuf *, uint32_t);
>  int  iwm_ccmp_decap(struct iwm_softc *, struct mbuf *,
>   struct ieee80211_node *);
>  void iwm_rx_rx_mpdu(struct iwm_softc *, struct iwm_rx_packet *,
> @@ -431,7 +432,7 @@ uint8_t   iwm_ridx2rate(struct ieee80211_rateset *, int)
>  int  iwm_rval2ridx(int);
>  void iwm_ack_rates(struct iwm_softc *, struct iwm_node *, int *, int *);
>  void iwm_mac_ctxt_cmd_common(struct iwm_softc *, struct iwm_node *,
> - struct iwm_mac_ctx_cmd *, uint32_t, int);
> + struct iwm_mac_ctx_cmd *, uint32_t);
>  void iwm_mac_ctxt_cmd_fill_sta(struct iwm_softc *, struct iwm_node *,
>   struct iwm_mac_data_sta *, int);
>  int  iwm_mac_ctxt_cmd(struct iwm_softc *, struct iwm_node *, uint32_t, int);
> @@ -476,6 +477,9 @@ const char *iwm_desc_lookup(uint32_t);
>  void iwm_nic_error(struct iwm_softc *);
>  void iwm_nic_umac_error(struct iwm_softc *);
>  #endif
> +void iwm_rx_mpdu(struct iwm_softc *, struct mbuf *, size_t);
> +int  iwm_rx_pkt_valid(struct iwm_rx_packet *);
> +void iwm_rx_pkt(struct iwm_softc *, struct iwm_rx_data *);
>  void iwm_notif_intr(struct iwm_softc *);
>  int  iwm_intr(void *);
>  int  iwm_match(struct device *, void *, void *);
> @@ -1729,7 +1733,6 @@ iwm_nic_rx_init(struct iwm_softc *sc)
>   IWM_FH_RCSR_RX_CONFIG_CHNL_EN_ENABLE_VAL|
>   IWM_FH_RCSR_CHNL0_RX_IGNORE_RXF_EMPTY   |  /* HW bug */
>   IWM_FH_RCSR_CHNL0_RX_CONFIG_IRQ_DEST_INT_HOST_VAL   |
> - IWM_FH_RCSR_CHNL0_RX_CONFIG_SINGLE_FRAME_MSK|
>   (IWM_RX_RB_TIMEOUT << IWM_FH_RCSR_RX_CONFIG_REG_IRQ_RBTH_POS) |
>   IWM_FH_RCSR_RX_CONFIG_REG_VAL_RB_SIZE_4K|
>   IWM_RX_QUEUE_SIZE_LOG << IWM_FH_RCSR_RX_CONFIG_RBDCB_SIZE_POS);
> @@ -3514,57 +3517,23 @@ iwm_ccmp_decap(struct iwm_softc *sc, struct mbuf *m, s
>   return 0;
>  }
>  
> -void
> -iwm_rx_rx_mpdu(struct iwm_softc *sc, struct iwm_rx_packet *pkt,
> -struct iwm_rx_data *data)
> +int
> +iwm_rx_frame(struct iwm_softc *sc, struct mbuf *m, uint32_t rx_pkt_status)
>  {
>   struct ieee80211com *ic = >sc_ic;
> - struct ifnet *ifp = IC2IFP(ic);
>   struct ieee80211_frame *wh;
>   struct ieee80211_node *ni;
>   struct ieee80211_rxinfo rxi;
>   struct ieee80211_channel *bss_chan;
> - struct mbuf *m;
>   struct iwm_rx_phy_info *phy_info;
> - struct iwm_rx_mpdu_res_start *rx_res;
>   int device_timestamp;
> - uint32_t len;
> - uint32_t rx_pkt_status;
>   int rssi, chanidx;
>   uint8_t saved_bssid[IEEE80211_ADDR_LEN] = { 0 };
>  
> - bus_dmamap_sync(sc->sc_dmat, data->map, 0, IWM_RBUF_SIZE,
> - BUS_DMASYNC_POSTREAD);
> -
>   phy_info = >sc_last_phy_info;
> - rx_res = (struct iwm_rx_mpdu_res_start *)pkt->data;
> - wh = (struct ieee80211_frame *)(pkt->data + sizeof(*rx_res));
> - len = le16toh(rx_res->byte_count);
> - if (len <

Re: iwm(4): fix ccmp decrypt edge cases

2019-08-16 Thread richard . n . procter 


On Thu, 15 Aug 2019, Stefan Sperling wrote:
> On Thu, Aug 15, 2019 at 03:47:02PM +1200, richard.n.proc...@gmail.com wrote:
> > > I agree we should handle a missing key but suggest an alternative 
> > > approach 
> > > below.
> 
> Hmm... your patch is surprisingly simple. I like it :)
> 
> I am still a bit worried about iwm firmware failing to install the key,
> which this patch would not handle. But that's a separate question.

See below for updated diff. Nice idea to add a panic on encrypt. 
Also I've followed the existing idiom of m_freem(m0); return NULL; 

I've checked that all assignments to k_flags are to fresh keys; the new 
flag is never clobbered.

The new check will be accounted against is_rx_wepfail ("input wep/wpa 
packets failed").

Lightly tested with a download and ifconfig up/down. 

ok?

best, 
Richard. 

Index: net80211/ieee80211_crypto.c
===
RCS file: /cvs/src/sys/net80211/ieee80211_crypto.c,v
retrieving revision 1.74
diff -u -p -u -p -r1.74 ieee80211_crypto.c
--- net80211/ieee80211_crypto.c 24 Sep 2018 20:14:59 -  1.74
+++ net80211/ieee80211_crypto.c 16 Aug 2019 09:50:47 -
@@ -157,6 +157,10 @@ ieee80211_set_key(struct ieee80211com *i
/* should not get there */
error = EINVAL;
}
+
+   if (error == 0)
+   k->k_flags |= IEEE80211_KEY_SWCRYPTO;
+
return error;
 }
 
@@ -209,6 +213,9 @@ struct mbuf *
 ieee80211_encrypt(struct ieee80211com *ic, struct mbuf *m0,
 struct ieee80211_key *k)
 {
+   if ((k->k_flags & IEEE80211_KEY_SWCRYPTO) == 0)
+   panic("%s: unset key %d", __func__, k->k_id);
+
switch (k->k_cipher) {
case IEEE80211_CIPHER_WEP40:
case IEEE80211_CIPHER_WEP104:
@@ -280,6 +287,12 @@ ieee80211_decrypt(struct ieee80211com *i
}
k = >ic_nw_keys[kid];
}
+
+   if ((k->k_flags & IEEE80211_KEY_SWCRYPTO) == 0) {
+   m_free(m0);
+   return NULL;
+   }
+
switch (k->k_cipher) {
case IEEE80211_CIPHER_WEP40:
case IEEE80211_CIPHER_WEP104:
Index: net80211/ieee80211_crypto.h
===
RCS file: /cvs/src/sys/net80211/ieee80211_crypto.h,v
retrieving revision 1.25
diff -u -p -u -p -r1.25 ieee80211_crypto.h
--- net80211/ieee80211_crypto.h 18 Aug 2017 17:30:12 -  1.25
+++ net80211/ieee80211_crypto.h 16 Aug 2019 09:50:47 -
@@ -78,6 +78,7 @@ struct ieee80211_key {
 #define IEEE80211_KEY_GROUP0x0001  /* group data key */
 #define IEEE80211_KEY_TX   0x0002  /* Tx+Rx */
 #define IEEE80211_KEY_IGTK 0x0004  /* integrity group key */
+#define IEEE80211_KEY_SWCRYPTO 0x0080  /* loaded for software crypto */
 
u_int   k_len;
u_int64_t   k_rsc[IEEE80211_NUM_TID];

Re: iwm(4): fix ccmp decrypt edge cases

2019-08-14 Thread richard . n . procter 



On Thu, 15 Aug 2019, richard.n.proc...@gmail.com wrote:

> Hi Stefan, 
> 
> On Wed, 14 Aug 2019, Stefan Sperling wrote:
> 
> > On Tue, Aug 13, 2019 at 10:47:24AM -0300, Martin Pieuchot wrote:
> > > if the crash is because of a missing key, put one :)
> > 
> > Yes, you've convinced me. With this patch we install the CCMP key
> > to both firmware and net80211. Until firmware confirms that the
> > key has been installed, we do software encrypt/decrypt.
> 
> I agree we should handle a missing key but suggest an alternative approach 
> below.

so:

m0 = NULL;
m_free(m0);

isn't so useful; fixed patch follows.

best, 
Richard. 

Index: net80211/ieee80211_crypto.c
===
RCS file: /cvs/src/sys/net80211/ieee80211_crypto.c,v
retrieving revision 1.74
diff -u -p -u -p -r1.74 ieee80211_crypto.c
--- net80211/ieee80211_crypto.c 24 Sep 2018 20:14:59 -  1.74
+++ net80211/ieee80211_crypto.c 15 Aug 2019 03:44:12 -
@@ -157,6 +157,10 @@ ieee80211_set_key(struct ieee80211com *i
/* should not get there */
error = EINVAL;
}
+
+   if (error == 0)
+   k->k_flags |= IEEE80211_KEY_SWCRYPTO;
+
return error;
 }
 
@@ -280,6 +284,11 @@ ieee80211_decrypt(struct ieee80211com *i
}
k = >ic_nw_keys[kid];
}
+
+   if ((k->k_flags & IEEE80211_KEY_SWCRYPTO) == 0) {
+   goto err;
+   }
+
switch (k->k_cipher) {
case IEEE80211_CIPHER_WEP40:
case IEEE80211_CIPHER_WEP104:
@@ -296,9 +305,15 @@ ieee80211_decrypt(struct ieee80211com *i
break;
default:
/* key not defined */
-   m_freem(m0);
-   m0 = NULL;
+   goto err;
}
+
+   return m0;
+
+err:
+   m_freem(m0);
+   m0 = NULL;
+
return m0;
 }
 
Index: net80211/ieee80211_crypto.h
===
RCS file: /cvs/src/sys/net80211/ieee80211_crypto.h,v
retrieving revision 1.25
diff -u -p -u -p -r1.25 ieee80211_crypto.h
--- net80211/ieee80211_crypto.h 18 Aug 2017 17:30:12 -  1.25
+++ net80211/ieee80211_crypto.h 15 Aug 2019 03:44:12 -
@@ -78,6 +78,7 @@ struct ieee80211_key {
 #define IEEE80211_KEY_GROUP0x0001  /* group data key */
 #define IEEE80211_KEY_TX   0x0002  /* Tx+Rx */
 #define IEEE80211_KEY_IGTK 0x0004  /* integrity group key */
+#define IEEE80211_KEY_SWCRYPTO 0x0080  /* loaded for software crypto */
 
u_int   k_len;
u_int64_t   k_rsc[IEEE80211_NUM_TID];



> > 
> > Should the firmware fail to install the key for some reason, we will
> > just keep using our software fallback until the interface is reset.
> > 
> > This also fixes the race with incoming frames during WPA handshake,
> > i.e. while (ni->ni_flags & IEEE80211_NODE_RXPROT) == 0.
> > Such frames can be passed to net80211 and will be dropped there.
> > 
> > Lightly tested on a 7260 device.
> > 
> > Ok?
> > 
> > diff refs/heads/master refs/heads/ccmp-crash2
> > blob - 038e6a63dfff113b525bb1e9a30c935996535569
> > blob + 8afcce065eb0d85f4469a3f378cacba1513bc215
> > --- sys/dev/pci/if_iwm.c
> > +++ sys/dev/pci/if_iwm.c
> > @@ -3595,10 +3595,17 @@ iwm_rx_rx_mpdu(struct iwm_softc *sc, struct 
> > iwm_rx_pac
> > rxi.rxi_rssi = rssi;
> > rxi.rxi_tstamp = device_timestamp;
> >  
> > -   /* Handle hardware decryption. */
> > +   /*
> > +* Handle hardware decryption.
> > +*
> > +* If the WPA handshake has completed but firmware is not yet ready
> > +* to decrypt frames, fall back to software decryption in net80211. 
> > +* net80211 will drop encrypted frames until the handshake completes.
> > +*/
> > if (((wh->i_fc[0] & IEEE80211_FC0_TYPE_MASK) != IEEE80211_FC0_TYPE_CTL)
> > && (wh->i_fc[1] & IEEE80211_FC1_PROTECTED) &&
> > !IEEE80211_IS_MULTICAST(wh->i_addr1) &&
> > +   (sc->sc_flags & IWM_FLAG_CCMP_READY) &&
> > (ni->ni_flags & IEEE80211_NODE_RXPROT) &&
> > ni->ni_pairwise_key.k_cipher == IEEE80211_CIPHER_CCMP) {
> > if ((rx_pkt_status & IWM_RX_MPDU_RES_STATUS_SEC_ENC_MSK) !=
> > @@ -4402,7 +4409,8 @@ iwm_tx(struct iwm_softc *sc, struct mbuf *m, struct ie
> >  
> > if (wh->i_fc[1] & IEEE80211_FC1_PROTECTED) {
> > k = ieee80211_get_txkey(ic, wh, ni);
> > -   if  (k->k_cipher != IEEE80211_CIPHER_CCMP) {
> > +   if  (k->k_cipher != IEEE80211_CIPHER_CCMP ||
> > +   (sc->sc_flags & IWM_FLAG_CCMP_READY) == 0) {
> > if ((m = ieee80211_encrypt(ic, m, k)) == NULL)
> > return ENOBUFS;
> >  
> > @@ -4418,7 +4426,8 @@ iwm_tx(struct iwm_softc *sc, struct mbuf *m, struct ie
> > /* Copy 802.11 header in TX command. */
> > memcpy(((uint8_t *)tx) + sizeof(*tx), wh, hdrlen);
> >  
> > -   if  (k != NULL && k->k_cipher

Re: iwm(4): fix ccmp decrypt edge cases

2019-08-14 Thread richard . n . procter 
Hi Stefan, 

On Wed, 14 Aug 2019, Stefan Sperling wrote:

> On Tue, Aug 13, 2019 at 10:47:24AM -0300, Martin Pieuchot wrote:
> > if the crash is because of a missing key, put one :)
> 
> Yes, you've convinced me. With this patch we install the CCMP key
> to both firmware and net80211. Until firmware confirms that the
> key has been installed, we do software encrypt/decrypt.

I agree we should handle a missing key but suggest an alternative approach 
below.

I'm unconvinced the root cause is a race on loading the key: I tried to 
induce an infinite race, by skipping the command to upload the key to 
hardware, but couldn't reproduce the crash. I suspect the guard for 
handling hardware decryption in iwm_rx_rx_mpdu() is too weak, and a frame 
governed by the offloaded key is slipping through.

The immediate cause of the crash is that, unless the driver sets 
IEEE80211_RXI_HWDEC on all frames governed by a key, the stack will 
attempt software decryption without the necessary context and crash. i.e. 
software decryption is the default and the stack assumes this is always 
possible. But it isn't: the stack has delegated decryption to the driver.

So, evade this class of errors and check that software decrypt can handle 
a key, i.e that ieee8021_set_key() has been called, and drop the frame if 
not. (see below; compiled but untested.)

Thoughts? 

best, 
Richard. 

iwm0 at pci1 dev 0 function 0 "Intel Dual Band Wireless AC 7265" rev 0x69, msi
iwm0: hw rev 0x210, fw ver 16.242414.0, address xx:xx:xx:xx:xx:xx 

Index: net80211/ieee80211_crypto.c
===
RCS file: /cvs/src/sys/net80211/ieee80211_crypto.c,v
retrieving revision 1.74
diff -u -p -u -p -r1.74 ieee80211_crypto.c
--- net80211/ieee80211_crypto.c 24 Sep 2018 20:14:59 -  1.74
+++ net80211/ieee80211_crypto.c 15 Aug 2019 01:12:28 -
@@ -157,6 +157,10 @@ ieee80211_set_key(struct ieee80211com *i
/* should not get there */
error = EINVAL;
}
+
+   if (error == 0)
+   k->k_flags |= IEEE80211_KEY_SWCRYPTO;
+
return error;
 }
 
@@ -280,6 +284,11 @@ ieee80211_decrypt(struct ieee80211com *i
}
k = >ic_nw_keys[kid];
}
+
+   if ((k->k_flags & IEEE80211_KEY_SWCRYPTO) == 0) {
+   goto err;
+   }
+
switch (k->k_cipher) {
case IEEE80211_CIPHER_WEP40:
case IEEE80211_CIPHER_WEP104:
@@ -296,9 +305,15 @@ ieee80211_decrypt(struct ieee80211com *i
break;
default:
/* key not defined */
-   m_freem(m0);
-   m0 = NULL;
+   goto err;
}
+
+   return m0;
+
+err:
+   m0 = NULL;
+   m_freem(m0);
+
return m0;
 }
 
Index: net80211/ieee80211_crypto.h
===
RCS file: /cvs/src/sys/net80211/ieee80211_crypto.h,v
retrieving revision 1.25
diff -u -p -u -p -r1.25 ieee80211_crypto.h
--- net80211/ieee80211_crypto.h 18 Aug 2017 17:30:12 -  1.25
+++ net80211/ieee80211_crypto.h 15 Aug 2019 01:12:28 -
@@ -78,6 +78,7 @@ struct ieee80211_key {
 #define IEEE80211_KEY_GROUP0x0001  /* group data key */
 #define IEEE80211_KEY_TX   0x0002  /* Tx+Rx */
 #define IEEE80211_KEY_IGTK 0x0004  /* integrity group key */
+#define IEEE80211_KEY_SWCRYPTO 0x0080  /* loaded for software crypto */
 
u_int   k_len;
u_int64_t   k_rsc[IEEE80211_NUM_TID];

---
illustration of disabling key upload below here
---


Index: dev/pci/if_iwm.c
===
RCS file: /cvs/src/sys/dev/pci/if_iwm.c,v
retrieving revision 1.244
diff -u -p -u -p -r1.244 if_iwm.c
--- dev/pci/if_iwm.c8 Aug 2019 13:56:56 -   1.244
+++ dev/pci/if_iwm.c14 Aug 2019 20:32:47 -
@@ -6088,7 +6088,7 @@ int
 iwm_set_key(struct ieee80211com *ic, struct ieee80211_node *ni,
 struct ieee80211_key *k)
 {
-   struct iwm_softc *sc = ic->ic_softc;
+   // struct iwm_softc *sc = ic->ic_softc;
struct iwm_add_sta_key_cmd cmd = { 0 };
 
if ((k->k_flags & IEEE80211_KEY_GROUP) ||
@@ -6108,8 +6108,10 @@ iwm_set_key(struct ieee80211com *ic, str
cmd.key_offset = 0;
cmd.sta_id = IWM_STATION_ID;
 
-   return iwm_send_cmd_pdu(sc, IWM_ADD_STA_KEY, IWM_CMD_ASYNC,
-   sizeof(cmd), );
+   // return iwm_send_cmd_pdu(sc, IWM_ADD_STA_KEY, IWM_CMD_ASYNC,
+   //sizeof(cmd), );
+
+   return 0;
 }
 
 void


> 
> Should the firmware fail to install the key for some reason, we will
> just keep using our software fallback until the interface is reset.
> 
> This also fixes the race with incoming frames during WPA handshake,
> i.e. while (ni->ni_flags & IEEE80211_NODE_RXPROT) == 0.
> Such frames can

PF: minor changes to TCP option parsing

2018-06-18 Thread richard . n . procter 
Hi, 

I have committed a patch to -current which refactors the six ways that PF 
finds TCP options into one new function.

I expect no side-effects, and the minor changes to finding MSS and WSCALE 
options that this involved were immaterial to the large sample of live 
traffic that I've examined.

However computer networks are good at confounding expectations.

If you do happen to notice problems related to MSS or WSCALE handling 
(used mostly by the syn{proxy,cookie} modes) please let me know. PF will 
now ignore these options when they fail to meet their mandatory length, as 
it already does the others.

best, 
Richard.

Re: witness report: vmmaplk, inode

2018-06-13 Thread richard . n . procter 


On Wed, 13 Jun 2018, richard.n.proc...@gmail.com wrote:
> I found this witness log on my computestick but not here.
> 
> OpenBSD 6.3-current (GENERIC.MP) #10: Mon Jun 11 14:02:36 NZST 2018
> procter@dill.internal:/usr/src/sys/arch/amd64/compile/GENERIC.MP
> 
> (CVS checkout on this date, clean but for a pf patch. NZST is 12 
> hours ahead of UTC.) 

For the record, I'm told there's a patch for this reversal: 
https://marc.info/?l=openbsd-bugs=152802593705776=2

, which was in posted in response to:
https://marc.info/?l=openbsd-tech=152796704214156=2 (Steele)

, and it also helped with:
https://marc.info/?l=openbsd-tech=152821522023626=2 (Popovski)

After patching my tree and noodling for a few more hours with 
emacs, firefox and chrome, the witness report hasn't reappeared.

cheers, 
Richard. 

> 
> lock order reversal:
>  1st 0xff0009fe22f8 vmmaplk (>lock) @ 
>   /usr/src/sys/uvm/uvm_map.c:4433
>  2nd 0xff00691ec0a0 inode (>i_lock) @ 
>   /usr/src/sys/ufs/ufs/ufs_vnops.c:1555
> lock order ">i_lock"(rrwlock) -> ">lock"(rwlock) first seen at:
> #0  witness_checkorder+0x4b4
> #1  _rw_enter_read+0x49
> #2  uvmfault_lookup+0x8d
> #3  uvm_fault+0x72
> #4  trap+0x516
> #5  recall_trap+0x8
> #6  copyout+0x48
> #7  ffs_read+0x1f0
> #8  VOP_READ+0x49
> #9  vn_read+0xca
> #10 dofilereadv+0x21c
> #11 sys_read+0x82
> #12 syscall+0x32a
> #13 Xsyscall_untramp+0xc0
> lock order ">lock"(rwlock) -> ">i_lock"(rrwlock) first seen at:
> #0  witness_checkorder+0x4b4
> #1  _rw_enter+0x68
> #2  _rrw_enter+0x3e
> #3  VOP_LOCK+0x3d
> #4  vn_lock+0x34
> #5  uvn_io+0x1b8
> #6  uvm_pager_put+0x109
> #7  uvn_flush+0x424
> #8  uvm_map_clean+0x3e7
> #9  syscall+0x32a
> #10 Xsyscall_untramp+0xc0
> 
> 
> OpenBSD 6.3-current (GENERIC.MP) #10: Mon Jun 11 14:02:36 NZST 2018
> procter@dill.internal:/usr/src/sys/arch/amd64/compile/GENERIC.MP
> real mem = 2056851456 (1961MB)
> avail mem = 1963495424 (1872MB)
> mpath0 at root
> scsibus0 at mpath0: 256 targets
> mainbus0 at root
> bios0 at mainbus0: SMBIOS rev. 3.0 @ 0x7b37e000 (51 entries)
> bios0: vendor Intel Corp. version "SCCHTAX5.86A.0024.2016.0408.1041" date 
> 04/08/2016
> bios0: Intel Corporation STK1AW32SC
> acpi0 at bios0: rev 2
> acpi0: sleep states S0 S4 S5
> acpi0: tables DSDT FACP APIC FPDT FIDT MCFG UEFI SSDT HPET SSDT SSDT SSDT 
> LPIT BCFG PRAM BGRT CSRT WDAT
> acpi0: wakeup devices
> acpitimer0 at acpi0: 3579545 Hz, 24 bits
> acpimadt0 at acpi0 addr 0xfee0: PC-AT compat
> cpu0 at mainbus0: apid 0 (boot processor)
> cpu0: Intel(R) Atom(TM) x5-Z8300 CPU @ 1.44GHz, 1440.34 MHz
> cpu0: 
> FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,EST,TM2,SSSE3,CX16,xTPR,PDCM,SSE4.1,SSE4.2,MOVBE,POPCNT,DEADLINE,AES,RDRAND,NXE,RDTSCP,LONG,LAHF,3DNOWP,PERF,ITSC,SMEP,ERMS,SENSOR,ARAT,MELTDOWN
> cpu0: 1MB 64b/line 16-way L2 cache
> cpu0: smt 0, core 0, package 0
> mtrr: Pentium Pro MTRR support, 8 var ranges, 88 fixed ranges
> cpu0: apic clock running at 79MHz
> cpu0: mwait min=64, max=64, C-substates=0.2.0.0.0.0.3.3, IBE
> cpu1 at mainbus0: apid 2 (application processor)
> cpu1: Intel(R) Atom(TM) x5-Z8300 CPU @ 1.44GHz, 1439.95 MHz
> cpu1: 
> FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,EST,TM2,SSSE3,CX16,xTPR,PDCM,SSE4.1,SSE4.2,MOVBE,POPCNT,DEADLINE,AES,RDRAND,NXE,RDTSCP,LONG,LAHF,3DNOWP,PERF,ITSC,SMEP,ERMS,SENSOR,ARAT,MELTDOWN
> cpu1: 1MB 64b/line 16-way L2 cache
> cpu1: smt 0, core 1, package 0
> cpu2 at mainbus0: apid 4 (application processor)
> cpu2: Intel(R) Atom(TM) x5-Z8300 CPU @ 1.44GHz, 1439.96 MHz
> cpu2: 
> FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,EST,TM2,SSSE3,CX16,xTPR,PDCM,SSE4.1,SSE4.2,MOVBE,POPCNT,DEADLINE,AES,RDRAND,NXE,RDTSCP,LONG,LAHF,3DNOWP,PERF,ITSC,SMEP,ERMS,SENSOR,ARAT,MELTDOWN
> cpu2: 1MB 64b/line 16-way L2 cache
> cpu2: smt 0, core 2, package 0
> cpu3 at mainbus0: apid 6 (application processor)
> cpu3: Intel(R) Atom(TM) x5-Z8300 CPU @ 1.44GHz, 1439.96 MHz
> cpu3: 
> FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,EST,TM2,SSSE3,CX16,xTPR,PDCM,SSE4.1,SSE4.2,MOVBE,POPCNT,DEADLINE,AES,RDRAND,NXE,RDTSCP,LONG,LAHF,3DNOWP,PERF,ITSC,SMEP,ERMS,SENSOR,ARAT,MELTDOWN
> cpu3: 1MB 64b/line 16-way L2 cache
> cpu3: smt 0, core 3, package 0
> ioapic0 at mainbus0: apid 1 pa 0xfec0, version 20, 115 pins
> acpimcfg0 at acpi0 addr 0xe000, bus 0-255
> acpihpet0 at acpi0: 14318179 Hz
> acpiprt0 at acpi0: bus 0 (PCI0)
> acpiprt1 at acpi0: bus 1 (RP01)
> acpiprt2 at acpi0: bus -1 (RP02)
> acpiprt3 at acpi0: bus -1 (RP03)
> acpiprt4 at acpi0: bus -1 (RP04)
> acpicpu0 at acpi0: C3(10@1000 mwait.1@0x64), C2(10@500 mwait.1@0x58), 
>

witness report: vmmaplk, inode

2018-06-12 Thread richard . n . procter 
Hi, 

I found this witness log on my computestick but not here.

I was doing little at the time besides using emacs and some vanilla 
chrome and possibly firefox. Hope it's of use.

cheers, 
Richard. 

OpenBSD 6.3-current (GENERIC.MP) #10: Mon Jun 11 14:02:36 NZST 2018
procter@dill.internal:/usr/src/sys/arch/amd64/compile/GENERIC.MP

(CVS checkout on this date, clean but for a pf patch. NZST is 12 
hours ahead of UTC.) 

lock order reversal:
 1st 0xff0009fe22f8 vmmaplk (>lock) @ 
  /usr/src/sys/uvm/uvm_map.c:4433
 2nd 0xff00691ec0a0 inode (>i_lock) @ 
  /usr/src/sys/ufs/ufs/ufs_vnops.c:1555
lock order ">i_lock"(rrwlock) -> ">lock"(rwlock) first seen at:
#0  witness_checkorder+0x4b4
#1  _rw_enter_read+0x49
#2  uvmfault_lookup+0x8d
#3  uvm_fault+0x72
#4  trap+0x516
#5  recall_trap+0x8
#6  copyout+0x48
#7  ffs_read+0x1f0
#8  VOP_READ+0x49
#9  vn_read+0xca
#10 dofilereadv+0x21c
#11 sys_read+0x82
#12 syscall+0x32a
#13 Xsyscall_untramp+0xc0
lock order ">lock"(rwlock) -> ">i_lock"(rrwlock) first seen at:
#0  witness_checkorder+0x4b4
#1  _rw_enter+0x68
#2  _rrw_enter+0x3e
#3  VOP_LOCK+0x3d
#4  vn_lock+0x34
#5  uvn_io+0x1b8
#6  uvm_pager_put+0x109
#7  uvn_flush+0x424
#8  uvm_map_clean+0x3e7
#9  syscall+0x32a
#10 Xsyscall_untramp+0xc0


OpenBSD 6.3-current (GENERIC.MP) #10: Mon Jun 11 14:02:36 NZST 2018
procter@dill.internal:/usr/src/sys/arch/amd64/compile/GENERIC.MP
real mem = 2056851456 (1961MB)
avail mem = 1963495424 (1872MB)
mpath0 at root
scsibus0 at mpath0: 256 targets
mainbus0 at root
bios0 at mainbus0: SMBIOS rev. 3.0 @ 0x7b37e000 (51 entries)
bios0: vendor Intel Corp. version "SCCHTAX5.86A.0024.2016.0408.1041" date 
04/08/2016
bios0: Intel Corporation STK1AW32SC
acpi0 at bios0: rev 2
acpi0: sleep states S0 S4 S5
acpi0: tables DSDT FACP APIC FPDT FIDT MCFG UEFI SSDT HPET SSDT SSDT SSDT LPIT 
BCFG PRAM BGRT CSRT WDAT
acpi0: wakeup devices
acpitimer0 at acpi0: 3579545 Hz, 24 bits
acpimadt0 at acpi0 addr 0xfee0: PC-AT compat
cpu0 at mainbus0: apid 0 (boot processor)
cpu0: Intel(R) Atom(TM) x5-Z8300 CPU @ 1.44GHz, 1440.34 MHz
cpu0: 
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,EST,TM2,SSSE3,CX16,xTPR,PDCM,SSE4.1,SSE4.2,MOVBE,POPCNT,DEADLINE,AES,RDRAND,NXE,RDTSCP,LONG,LAHF,3DNOWP,PERF,ITSC,SMEP,ERMS,SENSOR,ARAT,MELTDOWN
cpu0: 1MB 64b/line 16-way L2 cache
cpu0: smt 0, core 0, package 0
mtrr: Pentium Pro MTRR support, 8 var ranges, 88 fixed ranges
cpu0: apic clock running at 79MHz
cpu0: mwait min=64, max=64, C-substates=0.2.0.0.0.0.3.3, IBE
cpu1 at mainbus0: apid 2 (application processor)
cpu1: Intel(R) Atom(TM) x5-Z8300 CPU @ 1.44GHz, 1439.95 MHz
cpu1: 
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,EST,TM2,SSSE3,CX16,xTPR,PDCM,SSE4.1,SSE4.2,MOVBE,POPCNT,DEADLINE,AES,RDRAND,NXE,RDTSCP,LONG,LAHF,3DNOWP,PERF,ITSC,SMEP,ERMS,SENSOR,ARAT,MELTDOWN
cpu1: 1MB 64b/line 16-way L2 cache
cpu1: smt 0, core 1, package 0
cpu2 at mainbus0: apid 4 (application processor)
cpu2: Intel(R) Atom(TM) x5-Z8300 CPU @ 1.44GHz, 1439.96 MHz
cpu2: 
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,EST,TM2,SSSE3,CX16,xTPR,PDCM,SSE4.1,SSE4.2,MOVBE,POPCNT,DEADLINE,AES,RDRAND,NXE,RDTSCP,LONG,LAHF,3DNOWP,PERF,ITSC,SMEP,ERMS,SENSOR,ARAT,MELTDOWN
cpu2: 1MB 64b/line 16-way L2 cache
cpu2: smt 0, core 2, package 0
cpu3 at mainbus0: apid 6 (application processor)
cpu3: Intel(R) Atom(TM) x5-Z8300 CPU @ 1.44GHz, 1439.96 MHz
cpu3: 
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,EST,TM2,SSSE3,CX16,xTPR,PDCM,SSE4.1,SSE4.2,MOVBE,POPCNT,DEADLINE,AES,RDRAND,NXE,RDTSCP,LONG,LAHF,3DNOWP,PERF,ITSC,SMEP,ERMS,SENSOR,ARAT,MELTDOWN
cpu3: 1MB 64b/line 16-way L2 cache
cpu3: smt 0, core 3, package 0
ioapic0 at mainbus0: apid 1 pa 0xfec0, version 20, 115 pins
acpimcfg0 at acpi0 addr 0xe000, bus 0-255
acpihpet0 at acpi0: 14318179 Hz
acpiprt0 at acpi0: bus 0 (PCI0)
acpiprt1 at acpi0: bus 1 (RP01)
acpiprt2 at acpi0: bus -1 (RP02)
acpiprt3 at acpi0: bus -1 (RP03)
acpiprt4 at acpi0: bus -1 (RP04)
acpicpu0 at acpi0: C3(10@1000 mwait.1@0x64), C2(10@500 mwait.1@0x58), C1(1000@1 
mwait.1), PSS
acpicpu1 at acpi0: C3(10@1000 mwait.1@0x64), C2(10@500 mwait.1@0x58), C1(1000@1 
mwait.1), PSS
acpicpu2 at acpi0: C3(10@1000 mwait.1@0x64), C2(10@500 mwait.1@0x58), C1(1000@1 
mwait.1), PSS
acpicpu3 at acpi0: C3(10@1000 mwait.1@0x64), C2(10@500 mwait.1@0x58), C1(1000@1 
mwait.1), PSS
acpipwrres0 at acpi0: ID3C, resource for ISP3
acpipwrres1 at acpi0: WWPR, resource for HS03, MDM1
acpipwrres2 at acpi0: WWPR, resource for HS13, MDM1
acpipwrres3 at acpi0: WWPR, resource for SSC1, MDM3
acpipwrres4 at acpi0: WWPR, resource for SSCW, MDM3

Re: ipsec ah_massage_headers cleanup

2018-02-07 Thread richard . n . procter 


On Tue, 6 Feb 2018, Alexander Bluhm wrote:

> On Tue, Feb 06, 2018 at 11:04:51AM +1300, Richard Procter wrote:
> > > @@ -657,12 +667,13 @@ ah_input(struct mbuf *m, struct tdb *tdb
> > >   m_copyback(m, skip + rplen, ahx->authsize, ipseczeroes, M_NOWAIT);
> > >  
> > >   /* "Massage" the packet headers for crypto processing. */
> > > - if ((btsx = ah_massage_headers(, tdb->tdb_dst.sa.sa_family,
> > > - skip, ahx->type, 0)) != 0) {
> > > + error = ah_massage_headers(, tdb->tdb_dst.sa.sa_family, skip,
> > > + ahx->type, 0);
> > > + if (error) {
> > >   /* mbuf will be free'd by callee. */
> > 
> > This pre-existing comment muddled me. ah_massage_headers() has already 
> > freed it on error.
> 
> Although the code is correct, I also had to read the comment twice
> when I first saw it.  I think the tense is wrong.  The callee is
> ah_massage_headers, and there free has been called.
> 
> Does this clarify it?

It does, but the grammar is a little odd. (English tenses are obscure when 
I stop to think about them -- for instance, "he will be speckled" and "he 
has been bespeckled" works but "he has been be-freed" doesn't for some 
reason... I almost want to say "befree'n" but that sounds archaic.)

Anyways, I'll commit the simplification below. Having raised the issue, I 
should have provided it in the first place.

Index: netinet/ip_ah.c
===
RCS file: /cvs/src/sys/netinet/ip_ah.c,v
retrieving revision 1.135
diff -u -p -u -r1.135 ip_ah.c
--- netinet/ip_ah.c 6 Feb 2018 14:54:22 -   1.135
+++ netinet/ip_ah.c 6 Feb 2018 20:01:57 -
@@ -670,7 +670,7 @@ ah_input(struct mbuf *m, struct tdb *tdb
error = ah_massage_headers(, tdb->tdb_dst.sa.sa_family, skip,
ahx->type, 0);
if (error) {
-   /* mbuf will be free'd by callee. */
+   /* mbuf was freed by callee. */
free(tc, M_XDATA, 0);
crypto_freereq(crp);
return error;
@@ -1158,7 +1158,7 @@ ah_output(struct mbuf *m, struct tdb *td
error = ah_massage_headers(, tdb->tdb_dst.sa.sa_family, skip,
ahx->type, 1);
if (error) {
-   /* mbuf will be free'd by callee. */
+   /* mbuf was freed by callee. */
free(tc, M_XDATA, 0);
crypto_freereq(crp);
return error;





> bluhm
> 
> Index: netinet/ip_ah.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_ah.c,v
> retrieving revision 1.135
> diff -u -p -r1.135 ip_ah.c
> --- netinet/ip_ah.c   6 Feb 2018 14:54:22 -   1.135
> +++ netinet/ip_ah.c   6 Feb 2018 15:14:29 -
> @@ -670,7 +670,7 @@ ah_input(struct mbuf *m, struct tdb *tdb
>   error = ah_massage_headers(, tdb->tdb_dst.sa.sa_family, skip,
>   ahx->type, 0);
>   if (error) {
> - /* mbuf will be free'd by callee. */
> + /* mbuf has been be free'd by callee. */
>   free(tc, M_XDATA, 0);
>   crypto_freereq(crp);
>   return error;
> @@ -1158,7 +1158,7 @@ ah_output(struct mbuf *m, struct tdb *td
>   error = ah_massage_headers(, tdb->tdb_dst.sa.sa_family, skip,
>   ahx->type, 1);
>   if (error) {
> - /* mbuf will be free'd by callee. */
> + /* mbuf has been be free'd by callee. */
>   free(tc, M_XDATA, 0);
>   crypto_freereq(crp);
>   return error;
>

Re: ftp: don't close fin or s twice

2018-02-05 Thread richard . n . procter 


On Tue, 6 Feb 2018, Theo Buehler wrote:

> In cleanup_url_get, fin and s will be closed a second time, so mark them
> as invalid after closing them the first time.
> 
> Another option might be to remove the fclose/close calls, but since this
> happens right before the recursive call, I'm not sure whether this might
> run the risk of hitting limits.

I got curious about the 'else if': fin = fdopen(s) is possible, 
and closing both 'fin' and 's' will clobber errno.

How about the following further tweak to eliminate it?

I've also attached a follow-up patch that renames 's' -> 'fd'; searching 
for 's' is infeasible without the compiler's help.

Index: ftp/fetch.c
===
--- ftp.orig/fetch.c
+++ ftp/fetch.c
@@ -636,9 +636,11 @@ noslash:
}
} else {
fin = fdopen(s, "r+");
+   s = -1;
}
 #else /* !NOSSL */
fin = fdopen(s, "r+");
+   s = -1;
 #endif /* !NOSSL */
 
 #ifdef SMALL
@@ -912,10 +914,14 @@ noslash:
*loctail = '\0';
if (verbose)
fprintf(ttyout, "Redirected to %s\n", redirurl);
-   if (fin != NULL)
+   if (fin != NULL) {
fclose(fin);
-   else if (s != -1)
+   fin = NULL;
+   }
+   if (s != -1) {
close(s);
+   s = -1;
+   }
rval = url_get(redirurl, proxyenv, savefile, lastfile);
free(redirurl);
goto cleanup_url_get;
@@ -1039,10 +1045,14 @@ cleanup_url_get:
free(full_host);
free(sslhost);
 #endif /* !NOSSL */
-   if (fin != NULL)
+   if (fin != NULL) {
fclose(fin);
-   else if (s != -1)
+   fin = NULL;
+   }
+   if (s != -1) {
close(s);
+   s = -1;
+   }
if (out >= 0 && out != fileno(stdout))
close(out);
free(buf);


Index: ftp/fetch.c
===
--- ftp.orig/fetch.c
+++ ftp/fetch.c
@@ -189,7 +189,7 @@ url_get(const char *origline, const char
const char * volatile savefile;
char * volatile proxyurl = NULL;
char *credentials = NULL;
-   volatile int s = -1, out = -1;
+   volatile int fd = -1, out = -1;
volatile sig_t oldintr, oldinti;
FILE *fin = NULL;
off_t hashbytes;
@@ -364,13 +364,13 @@ noslash:
if (isfileurl) {
struct stat st;
 
-   s = open(path, O_RDONLY);
-   if (s == -1) {
+   fd = open(path, O_RDONLY);
+   if (fd == -1) {
warn("Can't open file %s", path);
goto cleanup_url_get;
}
 
-   if (fstat(s, ) == -1)
+   if (fstat(fd, ) == -1)
filesize = -1;
else
filesize = st.st_size;
@@ -399,7 +399,7 @@ noslash:
warn("Can't fstat %s", savefile);
goto cleanup_url_get;
}
-   if (lseek(s, st.st_size, SEEK_SET) == -1) {
+   if (lseek(fd, st.st_size, SEEK_SET) == -1) {
warn("Can't lseek %s", path);
goto cleanup_url_get;
}
@@ -429,7 +429,7 @@ noslash:
/* Finally, suck down the file. */
i = 0;
oldinti = signal(SIGINFO, psummary);
-   while ((len = read(s, buf, buflen)) > 0) {
+   while ((len = read(fd, buf, buflen)) > 0) {
bytes += len;
for (cp = buf; len > 0; len -= i, cp += i) {
if ((i = write(out, cp, len)) == -1) {
@@ -535,7 +535,7 @@ noslash:
if (verbose)
setvbuf(ttyout, NULL, _IOLBF, 0);
 
-   s = -1;
+   fd = -1;
for (res = res0; res; res = res->ai_next) {
if (getnameinfo(res->ai_addr, res->ai_addrlen, hbuf,
sizeof(hbuf), NULL, 0, NI_NUMERICHOST) != 0)
@@ -543,8 +543,8 @@ noslash:
if (verbose)
fprintf(ttyout, "Trying %s...\n", hbuf);
 
-   s = socket(res->ai_family, res->ai_socktype, res->ai_protocol);
-   if (s == -1) {
+   fd = socket(res->ai_family, res->ai_socktype, res->ai_protocol);
+   if (fd == -1) {
cause = "socket";
continue;
}
@@ -552,17 +552,17 @@ noslash:
 #ifndef SMALL
if (srcaddr) {
if

Re: add intel 6235 support to iwn(4)

2013-01-09 Thread Richard N 
Mark Kettenis mark.kettenis at xs4all.nl writes:

 
  Date: Tue, 13 Nov 2012 21:02:24 -0500
  From: James Turner james at calminferno.net
  
  I was actually able to add support myself with the following diff.
 
 Great!
 
 Committed an improved diff that also adds the other 6030 variant.
 
  Index: if_iwn.c
  ===
  RCS file: /cvs/src/sys/dev/pci/if_iwn.c,v
  retrieving revision 1.115
  diff -u -p if_iwn.c
  --- if_iwn.c11 Nov 2012 20:45:31 -  1.115
  +++ if_iwn.c14 Nov 2012 02:00:52 -
  @@ -639,7 +639,8 @@ iwn5000_attach(struct iwn_softc *sc, pci_product_id_t 
  break;
  case IWN_HW_REV_TYPE_6005:
  sc-limits = iwn6000_sensitivity_limits;
  -   if (pid == PCI_PRODUCT_INTEL_WL_6235_1) {
  +   if (pid == PCI_PRODUCT_INTEL_WL_6235_1 ||
  +   pid == PCI_PRODUCT_INTEL_WL_6030_2) {
  sc-fwname = iwn-6030;
   
  /* XXX: The 6235 generates a fatal firmware error when
 
 

--

Thankyou for all the above info
I am interested in purchasing a Laptop which will unfortunately have the Intel
Centrino Advanced-N 6235 + Bluetooth Combo Module

 I assume, in an earlier post, that someone disabled bluetooth (in their
BIOS), and then the iwn0 firmware/driver had worked flawlessly. ?

 I'm sorry for interrupting here, but I just wanted to confirm, that in OpenBSD
5.2 Official (Stable) Release , that this in fact is still the case. ?
 Either way, I do NOT plan on using Bluetooth on my laptop.

 Thanks for all your efforts as well.

Rick.