Re: tftpd(8): diff for ip path rewrite

2017-10-21 Thread Sebastien Marie 
On Sat, Oct 21, 2017 at 10:10:39PM +0200, Jan Klemkow wrote:
> 
> Common files should be found in the default directory.  But, host
> specific files could be overwritten if they exist in the subdirectory.
> 
> The diff below should address all comments.
> 
> Index: tftpd.c
> ===
> RCS file: /mount/openbsd/cvs/src/usr.sbin/tftpd/tftpd.c,v
> retrieving revision 1.39
> diff -u -p -r1.39 tftpd.c
> --- tftpd.c   26 May 2017 17:38:46 -  1.39
> +++ tftpd.c   21 Oct 2017 19:49:04 -
> @@ -903,7 +911,20 @@ again:
>  
>   if (rwmap != NULL)
>   rewrite_map(client, filename);
> - else
> + else if (iflag) {
> + char nfilename[PATH_MAX];
> +
> + if (snprintf(nfilename, sizeof(nfilename), "%s/%s",
> + getip(>ss), filename) > PATH_MAX - 1) {
> + ecode = EBADOP;
> + goto error;
> + }

snprintf(3) could return -1 on error. the snprintf(3) man page
document the proper secure idiom as:

int ret = snprintf(nfilename, sizeof(nfilename), "%s/%s", 
getip(>ss), filename);
if (ret == -1 || ret >= sizeof(nfilename)) {
ecode = EBADOP;
goto error;
}

regarding the error EBADOP (Illegal TFTP operation), I am unsure if it
is the proper error code for this case.

ENOTFOUND (File not found) or EACCESS (Access violation) seems more
indicated to me. but I am not familiar with tftpd protocol and
expectations regarding these error codes.

thanks.
-- 
Sebastien Marie

Re: spamdb: allow keys to be specified in list mode

2017-10-21 Thread Todd C. Miller 
I have an OK on the man page bits.  Anyone else want to chime in?

 - todd

On Wed, 18 Oct 2017 10:22:00 -0600, "Todd C. Miller" wrote:

> I often want to query a specific key instead of dumping out the
> entire database.  This lets you do things like:
> 
> $ spamdb 180.124.41.143
> TRAPPED|180.124.41.143|1508373987
> 
> or:
> 
> $ spamdb 74.125.198.26 180.124.41.143 107.161.26.205
> WHITE|74.125.198.26|||1506223558|1506223558|1509333958|1|0
> TRAPPED|180.124.41.143|1508373987
> 
> No error is displayed for missing addresses but the exit value will
> be 1 instead of 0 in this case.
> 
>  - todd
> 
> Index: usr.sbin/spamdb/spamdb.8
> ===
> RCS file: /cvs/src/usr.sbin/spamdb/spamdb.8,v
> retrieving revision 1.19
> diff -u -p -u -r1.19 spamdb.8
> --- usr.sbin/spamdb/spamdb.8  11 Oct 2017 18:25:07 -  1.19
> +++ usr.sbin/spamdb/spamdb.8  18 Oct 2017 16:17:41 -
> @@ -22,10 +22,8 @@
>  .Nd spamd database tool
>  .Sh SYNOPSIS
>  .Nm spamdb
> -.Oo Oo Fl Tt Oc
> -.Fl a Ar keys Oc
> -.Oo Oo Fl GTt Oc
> -.Fl d Ar keys Oc
> +.Op Fl adGTt
> +.Op Ar keys ...
>  .Sh DESCRIPTION
>  .Nm
>  manipulates the spamd database in
> @@ -35,7 +33,7 @@ used for
>  .Pp
>  The options are as follows:
>  .Bl -tag -width Ds
> -.It Fl a Ar keys
> +.It Fl a
>  Add or update the entries for
>  .Ar keys .
>  This can be used to whitelist one or more IP addresses
> @@ -46,7 +44,7 @@ If any
>  specified match entries already in the spamd database,
>  .Nm
>  updates the entry's time last seen to now.
> -.It Fl d Ar keys
> +.It Fl d
>  Delete entries for
>  .Ar keys .
>  .It Fl G
> @@ -90,9 +88,12 @@ Otherwise
>  .Ar keys
>  must be numerical IP addresses.
>  .Ss DATABASE OUTPUT FORMAT
> -If invoked without any arguments,
> +If invoked without any options,
>  .Nm
>  lists the contents of the database in a text format.
> +If one or more
> +.Ar keys
> +are specified, only matching entries will be printed.
>  .Pp
>  For SPAMTRAP entries the format is:
>  .Pp
> Index: usr.sbin/spamdb/spamdb.c
> ===
> RCS file: /cvs/src/usr.sbin/spamdb/spamdb.c,v
> retrieving revision 1.33
> diff -u -p -u -r1.33 spamdb.c
> --- usr.sbin/spamdb/spamdb.c  12 Oct 2017 09:28:56 -  1.33
> +++ usr.sbin/spamdb/spamdb.c  18 Oct 2017 16:21:21 -
> @@ -186,10 +186,81 @@ dbupdate(DB *db, char *ip, int add, int 
>  }
>  
>  int
> +print_entry(DBT *dbk, DBT *dbd)
> +{
> + struct gdata gd;
> + char *a, *cp;
> +
> + if ((dbk->size < 1) || gdcopyin(dbd, ) == -1) {
> + warnx("bogus size db entry - bad db file?");
> + return (1);
> + }
> + a = malloc(dbk->size + 1);
> + if (a == NULL)
> + err(1, "malloc");
> + memcpy(a, dbk->data, dbk->size);
> + a[dbk->size]='\0';
> + cp = strchr(a, '\n');
> + if (cp == NULL) {
> + /* this is a non-greylist entry */
> + switch (gd.pcount) {
> + case -1: /* spamtrap hit, with expiry time */
> + printf("TRAPPED|%s|%lld\n", a,
> + (long long)gd.expire);
> + break;
> + case -2: /* spamtrap address */
> + printf("SPAMTRAP|%s\n", a);
> + break;
> + default: /* whitelist */
> + printf("WHITE|%s|||%lld|%lld|%lld|%d|%d\n", a,
> + (long long)gd.first, (long long)gd.pass,
> + (long long)gd.expire, gd.bcount,
> + gd.pcount);
> + break;
> + }
> + } else {
> + char *helo, *from, *to;
> +
> + /* greylist entry */
> + *cp = '\0';
> + helo = cp + 1;
> + from = strchr(helo, '\n');
> + if (from == NULL) {
> + warnx("No from part in grey key %s", a);
> + free(a);
> + return (1);
> + }
> + *from = '\0';
> + from++;
> + to = strchr(from, '\n');
> + if (to == NULL) {
> + /* probably old format - print it the
> +  * with an empty HELO field instead 
> +  * of erroring out.
> +  */   
> + printf("GREY|%s|%s|%s|%s|%lld|%lld|%lld|%d|%d\n",
> + a, "", helo, from, (long long)gd.first,
> + (long long)gd.pass, (long long)gd.expire,
> + gd.bcount, gd.pcount);
> + 
> + } else {
> + *to = '\0';
> + to++;
> + printf("GREY|%s|%s|%s|%s|%lld|%lld|%lld|%d|%d\n",
> + a, helo, from, to, (long long)gd.first,
> + (long long)gd.pass, (long long)gd.expire,
> +

Re: Httpd support for internal redirects.

2017-10-21 Thread Ori Bernstein 
On Sat, 21 Oct 2017 22:18:17 +0200, Anders Andersson  wrote:

> Maybe I don't fully understand the problem, but is it not better to
> fix your broken website generator than to add more code to httpd?
> 

It seemed like other people wanted the feature as well, and based
on the contents of the cited thread, it seemed like something that
might be accepted if implemented.

It was easy to implement, so I added it and sent a patch. If
it's not a desirable feature for httpd, no problem.

-- 
Ori Bernstein

Re: Please test: IPsec w/o KERNEL_LOCK()

2017-10-21 Thread Tim Stewart 

Stuart Henderson  writes:

> On 2017/10/21 14:52, Tim Stewart wrote:
>> Stuart Henderson  writes:
>>
>> > On 2017/10/21 12:04, Tim Stewart wrote:
>> >> *49727  296965  0  0  7 0x14200crynlk
>> >
>> > aha, it was that one. Try this diff on top.
>> >
>> > Index: fpu.c
>> > ===
>> > RCS file: /cvs/src/sys/arch/amd64/amd64/fpu.c,v
>> > retrieving revision 1.38
>> > diff -u -p -r1.38 fpu.c
>> > --- fpu.c  14 Oct 2017 04:44:43 -  1.38
>> > +++ fpu.c  21 Oct 2017 16:16:14 -
>> > @@ -347,7 +347,7 @@ void
>> >  fpu_kernel_enter(void)
>> >  {
>> >struct cpu_info *ci = curcpu();
>> > -  uint32_t cw;
>> > +  struct savefpu  *sfp;
>> >int  s;
>> >
>> >/*
>> > @@ -376,10 +376,11 @@ fpu_kernel_enter(void)
>> >
>> >/* Initialize the FPU */
>> >fninit();
>> > -  cw = __INITIAL_NPXCW__;
>> > -  fldcw();
>> > -  cw = __INITIAL_MXCSR__;
>> > -  ldmxcsr();
>> > +  sfp = _addr->u_pcb.pcb_savefpu;
>> > +  memset(>fp_fxsave, 0, sizeof(sfp->fp_fxsave));
>> > +  sfp->fp_fxsave.fx_fcw = __INITIAL_NPXCW__;
>> > +  sfp->fp_fxsave.fx_mxcsr = __INITIAL_MXCSR__;
>> > +  fxrstor(>fp_fxsave);
>> >  }
>> >
>> >  void
>>
>> I've been running with this additional patch for a couple of hours and
>> the hang has not reappeared.  I'll keep the system active and confirm
>> thta everything looks good tomorrow.
>>
>> I swear I've seen this patch before on a list but can't find the
>> original.  Can someone give me or point me at some context, so I know
>> what I've just done? :)
>
> Diff is from mikeb. It initializes the fpu more completely, we suspect
> something in the userland state wasn't getting cleared when entering the
> kernel. I saw some problems with aes-ni up after the "Correctly handle
> exceptions when restoring an invalid FPU context" commit. (aes-ni uses
> floating point registers).

I see.  So I'm guessing that an "unlocked" IPsec more likely to hit this
bug because it's using AES-NI outside of KERNEL_LOCK() now?  Am I close?

-TimS

--
Tim Stewart
---
Mail:   t...@stoo.org
Matrix: @tim:stoo.org

Re: Please test: IPsec w/o KERNEL_LOCK()

2017-10-21 Thread Stuart Henderson 
On 2017/10/21 14:52, Tim Stewart wrote:
> Stuart Henderson  writes:
> 
> > On 2017/10/21 12:04, Tim Stewart wrote:
> >> *49727  296965  0  0  7 0x14200crynlk
> >
> > aha, it was that one. Try this diff on top.
> >
> > Index: fpu.c
> > ===
> > RCS file: /cvs/src/sys/arch/amd64/amd64/fpu.c,v
> > retrieving revision 1.38
> > diff -u -p -r1.38 fpu.c
> > --- fpu.c   14 Oct 2017 04:44:43 -  1.38
> > +++ fpu.c   21 Oct 2017 16:16:14 -
> > @@ -347,7 +347,7 @@ void
> >  fpu_kernel_enter(void)
> >  {
> > struct cpu_info *ci = curcpu();
> > -   uint32_t cw;
> > +   struct savefpu  *sfp;
> > int  s;
> >
> > /*
> > @@ -376,10 +376,11 @@ fpu_kernel_enter(void)
> >
> > /* Initialize the FPU */
> > fninit();
> > -   cw = __INITIAL_NPXCW__;
> > -   fldcw();
> > -   cw = __INITIAL_MXCSR__;
> > -   ldmxcsr();
> > +   sfp = _addr->u_pcb.pcb_savefpu;
> > +   memset(>fp_fxsave, 0, sizeof(sfp->fp_fxsave));
> > +   sfp->fp_fxsave.fx_fcw = __INITIAL_NPXCW__;
> > +   sfp->fp_fxsave.fx_mxcsr = __INITIAL_MXCSR__;
> > +   fxrstor(>fp_fxsave);
> >  }
> >
> >  void
> 
> I've been running with this additional patch for a couple of hours and
> the hang has not reappeared.  I'll keep the system active and confirm
> thta everything looks good tomorrow.
> 
> I swear I've seen this patch before on a list but can't find the
> original.  Can someone give me or point me at some context, so I know
> what I've just done? :)

Diff is from mikeb. It initializes the fpu more completely, we suspect
something in the userland state wasn't getting cleared when entering the
kernel. I saw some problems with aes-ni up after the "Correctly handle
exceptions when restoring an invalid FPU context" commit. (aes-ni uses
floating point registers).

Re: Httpd support for internal redirects.

2017-10-21 Thread Anders Andersson 
On Wed, Oct 11, 2017 at 6:31 AM, Ori Bernstein  wrote:
> My website generator is a little stupid at times. It generates
> files with .html suffixes, but urls without them.
>
> I worked around this with some redirects, but it never felt
> quite right doing an extra round trip. Therefore, I added
> internal redirects, processing the rewrite before responding to
> an http request.

Maybe I don't fully understand the problem, but is it not better to
fix your broken website generator than to add more code to httpd?

Re: tftpd(8): diff for ip path rewrite

2017-10-21 Thread Jan Klemkow 
On Fri, Oct 20, 2017 at 12:04:41PM +, Jeremie Courreges-Anglas wrote:
> On Fri, Oct 20 2017, Sebastien Marie  wrote:
> > On Thu, Oct 19, 2017 at 08:58:12PM +0200, Jan Klemkow wrote:
> >> +  char nfilename[PATH_MAX];
> >> +
> >> +  snprintf(nfilename, sizeof nfilename, "%s/%s",
> >> +  getip(>ss), filename);
> >
> > - filename has PATH_MAX length
> > - getip(>ss) could have NI_MAXHOST length
> 
> INET6_ADDRSTRLEN since getip() calls getnameinfo(NI_NUMERICHOST), but
> your point stands.
> 
> > so nfilename could be larger than PATH_MAX (sizeof nfilename).
> >
> > I assume the return of snprintf() need to be checked. if truncation
> > occured, a warning should be issued and nfilename discarded (just
> > calling tftp_open(client, filename)) ?
> 
> I think we should refuse the request if truncated.

done
 
> >> +  if (access(nfilename, R_OK) == 0)
> >> +  tftp_open(client, nfilename);
> >> +  else
> >> +  tftp_open(client, filename);
> >> +  }
> 
> Here we look up the same file in both the client-specific subdirectory
> and the default directory.  Should we instead look only in the
> client-specific directory if the latter exist?

Common files should be found in the default directory.  But, host
specific files could be overwritten if they exist in the subdirectory.

The diff below should address all comments.

Thanks,
Jan

Index: tftpd.8
===
RCS file: /mount/openbsd/cvs/src/usr.sbin/tftpd/tftpd.8,v
retrieving revision 1.5
diff -u -p -r1.5 tftpd.8
--- tftpd.8 18 Jul 2015 05:32:56 -  1.5
+++ tftpd.8 21 Oct 2017 18:41:15 -
@@ -37,7 +37,7 @@
 .Nd DARPA Trivial File Transfer Protocol daemon
 .Sh SYNOPSIS
 .Nm tftpd
-.Op Fl 46cdv
+.Op Fl 46cdiv
 .Op Fl l Ar address
 .Op Fl p Ar port
 .Op Fl r Ar socket
@@ -100,6 +100,16 @@ If this option is specified,
 .Nm
 will run in the foreground and log
 the client IP, type of request, and filename to stderr.
+.It Fl i
+.Nm
+tries to rewrite the requested filename with a prefix of
+the client's IP address.
+If the rewritten path exists
+.Nm
+will serve this file.
+If not, it will serve the original filename.
+This option can not be combined with
+.Fl r .
 .It Fl l Ar address
 Listen on the specified address.
 By default
@@ -126,6 +136,8 @@ before the TFTP request will continue.
 By default
 .Nm
 does not use filename rewriting.
+This option can not be combined with
+.Fl i .
 .It Fl v
 Log the client IP, type of request, and filename.
 .It Ar directory
Index: tftpd.c
===
RCS file: /mount/openbsd/cvs/src/usr.sbin/tftpd/tftpd.c,v
retrieving revision 1.39
diff -u -p -r1.39 tftpd.c
--- tftpd.c 26 May 2017 17:38:46 -  1.39
+++ tftpd.c 21 Oct 2017 19:49:04 -
@@ -282,7 +282,7 @@ __dead void
 usage(void)
 {
extern char *__progname;
-   fprintf(stderr, "usage: %s [-46cdv] [-l address] [-p port] [-r socket]"
+   fprintf(stderr, "usage: %s [-46cdiv] [-l address] [-p port] [-r socket]"
" directory\n", __progname);
exit(1);
 }
@@ -290,6 +290,7 @@ usage(void)
 int  cancreate = 0;
 int  verbose = 0;
 int  debug = 0;
+int  iflag = 0;
 
 int
 main(int argc, char *argv[])
@@ -307,7 +308,7 @@ main(int argc, char *argv[])
int family = AF_UNSPEC;
int devnull = -1;
 
-   while ((c = getopt(argc, argv, "46cdl:p:r:v")) != -1) {
+   while ((c = getopt(argc, argv, "46cdil:p:r:v")) != -1) {
switch (c) {
case '4':
family = AF_INET;
@@ -321,6 +322,11 @@ main(int argc, char *argv[])
case 'd':
verbose = debug = 1;
break;
+   case 'i':
+   if (rewrite != NULL)
+   usage();
+   iflag = 1;
+   break;
case 'l':
addr = optarg;
break;
@@ -328,6 +334,8 @@ main(int argc, char *argv[])
port = optarg;
break;
case 'r':
+   if (iflag == 1)
+   usage();
rewrite = optarg;
break;
case 'v':
@@ -903,7 +911,20 @@ again:
 
if (rwmap != NULL)
rewrite_map(client, filename);
-   else
+   else if (iflag) {
+   char nfilename[PATH_MAX];
+
+   if (snprintf(nfilename, sizeof(nfilename), "%s/%s",
+   getip(>ss), filename) > PATH_MAX - 1) {
+   ecode = EBADOP;
+   goto error;
+   }
+
+   if (access(nfilename, R_OK) == 0)
+   tftp_open(client, nfilename);
+

iwm(4) command response buffering

2017-10-21 Thread Stefan Sperling 
The way iwm(4) handles command responses is rather clunky.

Storage for one response is provided in the softc.
This storage is "reserved" while a command which expects a response
is in progress. If such a command is already queued, further commands
wait in tsleep() until the other command is done, which defeats the
purpose of having a ring.

With the diff below, command response buffers are allocated
dynamically and tracked by command ring index.
Their size is also tracked for sanity checks and free().

We can now queue commands on the command ring without
worrying about other commands already in progress.
And we can remove one of the many tsleeps from the driver \o/

I have lightly tested this on 7260, 7265, and 8260 in GENERIC.MP
and on 7265 in bsd.rd. I don't see any regressions.

ok?

Index: if_iwm.c
===
RCS file: /cvs/src/sys/dev/pci/if_iwm.c,v
retrieving revision 1.214
diff -u -p -r1.214 if_iwm.c
--- if_iwm.c4 Oct 2017 18:00:12 -   1.214
+++ if_iwm.c21 Oct 2017 16:37:02 -
@@ -2204,12 +2204,13 @@ iwm_send_time_event_cmd(struct iwm_softc
 const struct iwm_time_event_cmd_v2 *cmd)
 {
struct iwm_time_event_cmd_v1 cmd_v1;
+   struct iwm_rx_packet *pkt;
+   struct iwm_time_event_resp *resp;
struct iwm_host_cmd hcmd = {
.id = IWM_TIME_EVENT_CMD,
-   .flags = IWM_CMD_WANT_SKB,
+   .flags = IWM_CMD_WANT_RESP,
+   .resp_pkt_len = sizeof(*pkt) + sizeof(*resp),
};
-   struct iwm_rx_packet *pkt;
-   struct iwm_time_event_resp *resp;
uint32_t resp_len;
int err;
 
@@ -2343,7 +2344,8 @@ iwm_nvm_read_chunk(struct iwm_softc *sc,
struct iwm_rx_packet *pkt;
struct iwm_host_cmd cmd = {
.id = IWM_NVM_ACCESS_CMD,
-   .flags = (IWM_CMD_WANT_SKB | IWM_CMD_SEND_IN_RFKILL),
+   .flags = (IWM_CMD_WANT_RESP | IWM_CMD_SEND_IN_RFKILL),
+   .resp_pkt_len = IWM_CMD_RESP_MAX,
.data = { _access_cmd, },
};
int err, offset_read;
@@ -2364,6 +2366,8 @@ iwm_nvm_read_chunk(struct iwm_softc *sc,
 
/* Extract NVM response */
nvm_resp = (void *)pkt->data;
+   if (nvm_resp == NULL)
+   return EIO;
 
err = le16toh(nvm_resp->status);
bytes_read = le16toh(nvm_resp->length);
@@ -3773,40 +3777,42 @@ iwm_send_cmd(struct iwm_softc *sc, struc
bus_addr_t paddr;
uint32_t addr_lo;
int err = 0, i, paylen, off, s;
-   int code;
-   int async, wantresp;
-   int group_id;
+   int idx, code, async, group_id;
size_t hdrlen, datasz;
uint8_t *data;
int generation = sc->sc_generation;
 
code = hcmd->id;
async = hcmd->flags & IWM_CMD_ASYNC;
-   wantresp = hcmd->flags & IWM_CMD_WANT_SKB;
+   idx = ring->cur;
 
for (i = 0, paylen = 0; i < nitems(hcmd->len); i++) {
paylen += hcmd->len[i];
}
 
-   /* if the command wants an answer, busy sc_cmd_resp */
-   if (wantresp) {
+   /* If this command waits for a response, allocate response buffer. */
+   hcmd->resp_pkt = NULL;
+   if (hcmd->flags & IWM_CMD_WANT_RESP) {
+   uint8_t *resp_buf;
KASSERT(!async);
-   while (sc->sc_wantresp != IWM_CMD_RESP_IDLE)
-   tsleep(>sc_wantresp, 0, "iwmcmdsl", 0);
-   sc->sc_wantresp = ring->qid << 16 | ring->cur;
+   KASSERT(hcmd->resp_pkt_len >= sizeof(struct iwm_rx_packet));
+   KASSERT(hcmd->resp_pkt_len <= IWM_CMD_RESP_MAX);
+   if (sc->sc_cmd_resp_pkt[idx] != NULL)
+   return ENOSPC;
+   resp_buf = malloc(hcmd->resp_pkt_len, M_DEVBUF,
+   M_NOWAIT | M_ZERO);
+   if (resp_buf == NULL)
+   return ENOMEM;
+   sc->sc_cmd_resp_pkt[idx] = resp_buf;
+   sc->sc_cmd_resp_len[idx] = hcmd->resp_pkt_len;
+   } else {
+   sc->sc_cmd_resp_pkt[idx] = NULL;
}
 
-   /*
-* Is the hardware still available?  (after e.g. above wait).
-*/
s = splnet();
-   if (generation != sc->sc_generation) {
-   err = ENXIO;
-   goto out;
-   }
 
-   desc = >desc[ring->cur];
-   txdata = >data[ring->cur];
+   desc = >desc[idx];
+   txdata = >data[idx];
 
group_id = iwm_cmd_groupid(code);
if (group_id != 0) {
@@ -3845,7 +3851,7 @@ iwm_send_cmd(struct iwm_softc *sc, struc
txdata->m = m; /* mbuf will be freed in iwm_cmd_done() */
paddr = txdata->map->dm_segs[0].ds_addr;
} else {
-   cmd = >cmd[ring->cur];
+   cmd = >cmd[idx];
paddr = txdata->cmd_paddr;
}
 
@@ -3853,7 +3859,7 @@ iwm_send_cmd(struct iwm_softc *sc, struc

Re: Please test: IPsec w/o KERNEL_LOCK()

2017-10-21 Thread Tim Stewart 
Stuart Henderson  writes:

> On 2017/10/21 12:04, Tim Stewart wrote:
>> *49727  296965  0  0  7 0x14200crynlk
>
> aha, it was that one. Try this diff on top.
>
> Index: fpu.c
> ===
> RCS file: /cvs/src/sys/arch/amd64/amd64/fpu.c,v
> retrieving revision 1.38
> diff -u -p -r1.38 fpu.c
> --- fpu.c 14 Oct 2017 04:44:43 -  1.38
> +++ fpu.c 21 Oct 2017 16:16:14 -
> @@ -347,7 +347,7 @@ void
>  fpu_kernel_enter(void)
>  {
>   struct cpu_info *ci = curcpu();
> - uint32_t cw;
> + struct savefpu  *sfp;
>   int  s;
>
>   /*
> @@ -376,10 +376,11 @@ fpu_kernel_enter(void)
>
>   /* Initialize the FPU */
>   fninit();
> - cw = __INITIAL_NPXCW__;
> - fldcw();
> - cw = __INITIAL_MXCSR__;
> - ldmxcsr();
> + sfp = _addr->u_pcb.pcb_savefpu;
> + memset(>fp_fxsave, 0, sizeof(sfp->fp_fxsave));
> + sfp->fp_fxsave.fx_fcw = __INITIAL_NPXCW__;
> + sfp->fp_fxsave.fx_mxcsr = __INITIAL_MXCSR__;
> + fxrstor(>fp_fxsave);
>  }
>
>  void

I've been running with this additional patch for a couple of hours and
the hang has not reappeared.  I'll keep the system active and confirm
thta everything looks good tomorrow.

I swear I've seen this patch before on a list but can't find the
original.  Can someone give me or point me at some context, so I know
what I've just done? :)

Thanks!

-TimS

-- 
Tim Stewart
---
Mail:   t...@stoo.org
Matrix: @tim:stoo.org

Re: Please test: IPsec w/o KERNEL_LOCK()

2017-10-21 Thread Stuart Henderson 
On 2017/10/21 12:06, Tim Stewart wrote:
> 
> Stuart Henderson  writes:
> 
> > On 2017/10/21 10:33, Tim Stewart wrote:
> >> I don't have much experience with capturing OpenBSD kernel panics.  I've
> >> set up screen on another system so that I'll have a log of serial
> >> console activity (this is an apu2c4) and have set ddb.console=1.  I will
> >> also reboot with bsd.gdb this time.
> >
> > Don't boot bsd.gdb - boot the normal kernel, but it can sometimes be useful
> > to have the debug kernel around for analysis post-crash.
> 
> I just sent another email with a reproduction and ddb output, but it was
> booted into the gdb kernel.  Do you think the results will be different
> if I boot with the non-GDB kernel?

I think it just means the kernel uses more RAM but don't think it will have
affected this result.

Re: Please test: IPsec w/o KERNEL_LOCK()

2017-10-21 Thread Stuart Henderson 
On 2017/10/21 12:04, Tim Stewart wrote:
> *49727  296965  0  0  7 0x14200crynlk

aha, it was that one. Try this diff on top.

Index: fpu.c
===
RCS file: /cvs/src/sys/arch/amd64/amd64/fpu.c,v
retrieving revision 1.38
diff -u -p -r1.38 fpu.c
--- fpu.c   14 Oct 2017 04:44:43 -  1.38
+++ fpu.c   21 Oct 2017 16:16:14 -
@@ -347,7 +347,7 @@ void
 fpu_kernel_enter(void)
 {
struct cpu_info *ci = curcpu();
-   uint32_t cw;
+   struct savefpu  *sfp;
int  s;
 
/*
@@ -376,10 +376,11 @@ fpu_kernel_enter(void)
 
/* Initialize the FPU */
fninit();
-   cw = __INITIAL_NPXCW__;
-   fldcw();
-   cw = __INITIAL_MXCSR__;
-   ldmxcsr();
+   sfp = _addr->u_pcb.pcb_savefpu;
+   memset(>fp_fxsave, 0, sizeof(sfp->fp_fxsave));
+   sfp->fp_fxsave.fx_fcw = __INITIAL_NPXCW__;
+   sfp->fp_fxsave.fx_mxcsr = __INITIAL_MXCSR__;
+   fxrstor(>fp_fxsave);
 }
 
 void

Re: Please test: IPsec w/o KERNEL_LOCK()

2017-10-21 Thread Tim Stewart 

Stuart Henderson  writes:

> On 2017/10/21 10:33, Tim Stewart wrote:
>> I don't have much experience with capturing OpenBSD kernel panics.  I've
>> set up screen on another system so that I'll have a log of serial
>> console activity (this is an apu2c4) and have set ddb.console=1.  I will
>> also reboot with bsd.gdb this time.
>
> Don't boot bsd.gdb - boot the normal kernel, but it can sometimes be useful
> to have the debug kernel around for analysis post-crash.

I just sent another email with a reproduction and ddb output, but it was
booted into the gdb kernel.  Do you think the results will be different
if I boot with the non-GDB kernel?

-TimS

--
Tim Stewart
---
Mail:   t...@stoo.org
Matrix: @tim:stoo.org

Re: Please test: IPsec w/o KERNEL_LOCK()

2017-10-21 Thread Tim Stewart 
Tim Stewart  writes:

> Martin Pieuchot  writes:
>
>> On 11/10/17(Wed) 17:01, Martin Pieuchot wrote:
>>> OpenBSD 6.2 includes nice performance and latency improvements due to
>>> the work done in the Network Stack in the previous years.  However as
>>> soon as IPsec is enabled, all network related processing are affected.
>>> In other words you cannot profit from the last MP work in the Network
>>> stack if you use IPsec.
>
> By "IPsec is enabled," do you mean as soon as an AS appears in the
> kernel?  Or is it some other condition?
>
>>> During the last 6 months I hoped that somebody else would look at the
>>> IPsec stack and tell us what needs to be done.  This didn't happen.
>>>
>>> Now that 6.2 is released, we cannot afford to continue to parallelize
>>> the Network Stack if some of our users and testers still run it under
>>> KERNEL_LOCK().
>>>
>>> So I did an audit of the IPsec stack and came with the small diff below.
>>> This diff doesn't remove the KERNEL_LOCK() (yet), but add some asserts
>>> to make sure that the global data structure are all accessed while
>>> holding the NET_LOCK().
>>
>> Here's the diff to stop grabbing the KERNEL_LOCK(), please test and
>> report back.
>
> I have several iked tunnels that are normally active.  I applied both
> patches last night against 6.2-stable, rebooted, and my router was hung
> when I woke up this morning.  Unfortunately I wasn't really set up to
> capture crash information, and there are no dumps in /var/crash/.
>
> I don't have much experience with capturing OpenBSD kernel panics.  I've
> set up screen on another system so that I'll have a log of serial
> console activity (this is an apu2c4) and have set ddb.console=1.  I will
> also reboot with bsd.gdb this time.  Is there anything else I should to
> increase my chances of getting a useful dump?

I reproduced the lockup again.  There was no output on the console.  The
router stopped forwarding traffic and I found that the root shell I had
open on the console was not respond.  I sent a BREAK and was dropped
into ddb.

Following is the output of `show panic' a `trace' for each CPU, and a
`ps':

#
# Stopped at  db_enter+0x5 
[/home/src-cvs/sys/arch/amd64/amd64/db_interface.c
ddb{0}> show panic
the kernel did not panic
ddb{0}> trace
db_enter(8000220f98f8,10,8000220f9888,302,8,8122bdd5) at 
db_enter+0x5 [/home/src-cvs/sys/arch/amd64/amd64/db_interface.c:402]
comintr(81ab42b0,800cf000,0,1,800ce480,81b225a8)
 at comintr+0x18f [/home/src-cvs/sys/dev/ic/com.c:1091]
intr_handler(8000220f9be0,800ce400,7,9,800ce480,8007af00)
 at intr_handler+0x5e [/home/src-cvs/sys/arch/amd64/amd64/intr.c:534]
Xintr_ioapic_edge4() at Xintr_ioapic_edge4+0xcc
--- interrupt ---
end of kernel
end trace frame: 0x7b91c905c700, count: -4
0x4c48246c894c5024:
ddb{0}> machine ddbcpu 1
Stopped at  x86_ipi_db+0x5 
[/home/src-cvs/sys/arch/amd64/amd64/db_interface.c:356]:popq%rbp
ddb{1}> trace
x86_ipi_db(8111dd22,10,8000220bf4a8,246,8,8122bdc5) at 
x86_ipi_db+0x5 [/home/src-cvs/sys/arch/amd64/amd64/db_interface.c:356]
x86_ipi_handler(800716b8,1388,80014300,80071000,0,0) at 
x86_ipi_handler+0x6b [/home/src-cvs/sys/arch/amd64/amd64/ipi.c:106]
Xresume_lapic_ipi() at Xresume_lapic_ipi+0x1f
--- interrupt ---
end of kernel
end trace frame: 0x7bbf6405c75250cc, count: -3
0x41cb8c419c524153:
ddb{1}> machine ddbcpu 2
Stopped at  x86_ipi_db+0x5 
[/home/src-cvs/sys/arch/amd64/amd64/db_interface.c:356]:popq%rbp
ddb{2}> trace
x86_ipi_db(8111dd22,10,8000220c57f8,246,8,8122bdc5) at 
x86_ipi_db+0x5 [/home/src-cvs/sys/arch/amd64/amd64/db_interface.c:356]
x86_ipi_handler(800726b8,1388,80014400,80072000,0,0) at 
x86_ipi_handler+0x6b [/home/src-cvs/sys/arch/amd64/amd64/ipi.c:106]
Xresume_lapic_ipi() at Xresume_lapic_ipi+0x1f
--- interrupt ---
end of kernel
end trace frame: 0x7bbf6405c75250cc, count: -3
0x41cb8c419c524153:
ddb{2}> machine ddbcpu 3
Stopped at  x86_ipi_db+0x5 
[/home/src-cvs/sys/arch/amd64/amd64/db_interface.c:356]:popq%rbp
ddb{3}> trace
x86_ipi_db(8000f4d0,10,8000f438,246,8,8122bdc5) at 
x86_ipi_db+0x5 [/home/src-cvs/sys/arch/amd64/amd64/db_interface.c:356]
x86_ipi_handler(80002221cd70,0,80074000,80002211e2f0,c,1080be) 
at x86_ipi_handler+0x6b [/home/src-cvs/sys/arch/amd64/amd64/ipi.c:106]
Xresume_lapic_ipi() at Xresume_lapic_ipi+0x1f
--- interrupt ---
end of kernel
end trace frame: 0x7bbf6405c75250cc, count: -3
0x41cb8c419c524153:
ddb{3}> machine ddbcpu 0
ddb{0}> ps
   PID TID   PPIDUID  S   FLAGS  WAIT  COMMAND
 37136  487145   7535  0  30x83  poll  htop
 39965  344785  95745  0  30x100083  ttyin ksh
 95745  351284  1  0  30x100089  selectmosh-server
  7535  104948  16008  0  3

Re: Please test: IPsec w/o KERNEL_LOCK()

2017-10-21 Thread Stuart Henderson 
On 2017/10/21 10:33, Tim Stewart wrote:
> I don't have much experience with capturing OpenBSD kernel panics.  I've
> set up screen on another system so that I'll have a log of serial
> console activity (this is an apu2c4) and have set ddb.console=1.  I will
> also reboot with bsd.gdb this time.

Don't boot bsd.gdb - boot the normal kernel, but it can sometimes be useful
to have the debug kernel around for analysis post-crash.

Re: Please test: IPsec w/o KERNEL_LOCK()

2017-10-21 Thread Tim Stewart 
Martin Pieuchot  writes:

> On 11/10/17(Wed) 17:01, Martin Pieuchot wrote:
>> OpenBSD 6.2 includes nice performance and latency improvements due to
>> the work done in the Network Stack in the previous years.  However as
>> soon as IPsec is enabled, all network related processing are affected.
>> In other words you cannot profit from the last MP work in the Network
>> stack if you use IPsec.

By "IPsec is enabled," do you mean as soon as an AS appears in the
kernel?  Or is it some other condition?

>> During the last 6 months I hoped that somebody else would look at the
>> IPsec stack and tell us what needs to be done.  This didn't happen.
>>
>> Now that 6.2 is released, we cannot afford to continue to parallelize
>> the Network Stack if some of our users and testers still run it under
>> KERNEL_LOCK().
>>
>> So I did an audit of the IPsec stack and came with the small diff below.
>> This diff doesn't remove the KERNEL_LOCK() (yet), but add some asserts
>> to make sure that the global data structure are all accessed while
>> holding the NET_LOCK().
>
> Here's the diff to stop grabbing the KERNEL_LOCK(), please test and
> report back.

I have several iked tunnels that are normally active.  I applied both
patches last night against 6.2-stable, rebooted, and my router was hung
when I woke up this morning.  Unfortunately I wasn't really set up to
capture crash information, and there are no dumps in /var/crash/.

I don't have much experience with capturing OpenBSD kernel panics.  I've
set up screen on another system so that I'll have a log of serial
console activity (this is an apu2c4) and have set ddb.console=1.  I will
also reboot with bsd.gdb this time.  Is there anything else I should to
increase my chances of getting a useful dump?

-TimS

--
Tim Stewart
---
Mail:   t...@stoo.org
Matrix: @tim:stoo.org

Re: Refactor TCP partial ACK handling

2017-10-21 Thread Mike Belopuhov 
On Fri, Oct 20, 2017 at 22:59 +0200, Klemens Nanni wrote:
> The comments for both void tcp_{sack,newreno}_partialack() still mention
> tp->snd_last and return value bits.
> 

Good eyes!  It made me spot a mistake I made by folding two lines
into an incorrect ifdef in tcp_sack_partialack.  I expected it to
say "ifdef TCP_FACK" while it says "ifNdef".  The adjusted comment
didn't make sense and I found the bug.

diff --git sys/netinet/tcp_input.c sys/netinet/tcp_input.c
index 45aafee0d05..d5de9cb2407 100644
--- sys/netinet/tcp_input.c
+++ sys/netinet/tcp_input.c
@@ -2690,13 +2690,13 @@ tcp_clean_sackreport(struct tcpcb *tp)
tp->sackblks[i].start = tp->sackblks[i].end=0;
 
 }
 
 /*
- * Checks for partial ack.  If partial ack arrives, turn off retransmission
- * timer, deflate the window, do not clear tp->t_dupacks, and return 1.
- * If the ack advances at least to tp->snd_last, return 0.
+ * Partial ack handling within a sack recovery episode.  When a partial ack
+ * arrives, turn off retransmission timer, deflate the window, do not clear
+ * tp->t_dupacks.
  */
 void
 tcp_sack_partialack(struct tcpcb *tp, struct tcphdr *th)
 {
/* Turn off retx. timer (will start again next segment) */
@@ -2711,16 +2711,16 @@ tcp_sack_partialack(struct tcpcb *tp, struct tcphdr *th)
if (tp->snd_cwnd > (th->th_ack - tp->snd_una)) {
tp->snd_cwnd -= th->th_ack - tp->snd_una;
tp->snd_cwnd += tp->t_maxseg;
} else
tp->snd_cwnd = tp->t_maxseg;
+   tp->snd_cwnd += tp->t_maxseg;
+   tp->t_flags |= TF_NEEDOUTPUT;
+#else
/* Force call to tcp_output */
if (tp->snd_awnd < tp->snd_cwnd)
tp->t_flags |= TF_NEEDOUTPUT;
-#else
-   tp->snd_cwnd += tp->t_maxseg;
-   tp->t_flags |= TF_NEEDOUTPUT;
 #endif
 }
 
 /*
  * Pull out of band byte out of a segment so
@@ -3078,14 +3078,14 @@ tcp_mss_update(struct tcpcb *tp)
}
 
 }
 
 /*
- * Checks for partial ack.  If partial ack arrives, force the retransmission
- * of the next unacknowledged segment, do not clear tp->t_dupacks, and return
- * 1.  By setting snd_nxt to ti_ack, this forces retransmission timer to
- * be started again.  If the ack advances at least to tp->snd_last, return 0.
+ * When a partial ack arrives, force the retransmission of the
+ * next unacknowledged segment.  Do not clear tp->t_dupacks.
+ * By setting snd_nxt to ti_ack, this forces retransmission timer
+ * to be started again.
  */
 void
 tcp_newreno_partialack(struct tcpcb *tp, struct tcphdr *th)
 {
/*