[PATCH] ksh: Fail gracefully if sethistsize() fails

[thomas]

This can be hard to debug, and could indeed prevent debugging, since
it can take out the ability to run /bin/sh, depending on how you've
set up your environment.

Before:
$ HISTSIZE=10 /bin/sh
/bin/sh: internal error: unable to allocate memory
  <--- no ksh

With this pa
$ HISTSIZE=10 ./ksh
./ksh: internal error: unable to allocate 16+88 bytes of memory
Unable to set the history size to 10
hostname$ ^D   <--- ksh running

diff --git a/bin/ksh/alloc.c b/bin/ksh/alloc.c
index 204e835890c..6e29e2eb3fe 100644
--- a/bin/ksh/alloc.c
+++ b/bin/ksh/alloc.c
@@ -97,7 +97,7 @@ aresize(void *ptr, size_t size, Area *ap)

l2 = realloc(l, sizeof(struct link) + size);
if (l2 == NULL)
-   internal_errorf("unable to allocate memory");
+   internal_errorf("unable to allocate %lu+%lu bytes of memory",
sizeof(struct link), size);
if (lprev)
lprev->next = l2;
else
diff --git a/bin/ksh/history.c b/bin/ksh/history.c
index a3a8b60cc76..3bd6444d7a9 100644
--- a/bin/ksh/history.c
+++ b/bin/ksh/history.c
@@ -549,10 +549,17 @@ sethistcontrol(const char *str)
 /*
  * set history
  * this means reallocating the dataspace
+ * if memory allocation fails then history size will remain unchanged
  */
 void
 sethistsize(int n)
 {
+   newenv(E_FUNC);
+   if (sigsetjmp(genv->jbuf, 0)) {
+   fprintf(stderr, "Unable to set the history size to %d\n", n);
+   quitenv(NULL);
+   return;
+   }
if (n > 0 && (uint32_t)n != histsize) {
int offset = histptr - history;

@@ -566,8 +573,8 @@ sethistsize(int n)
memmove(history, histptr - offset, n * sizeof(char *));
}

-   histsize = n;
histbase = areallocarray(histbase, n + 1, sizeof(char *), 
APERM);
+   histsize = n;
history = histbase + 1;
histptr = history + offset;
}

Re: ifconfig compiler warnings

[Sebastian Benoit]

Alexander Bluhm(alexander.bl...@gmx.net) on 2019.10.17 16:23:32 +0200:
> Hi,
> 
> I would like to fix some warings in ifconfig when compiled with
> WARNINGS=yes.
> 
> - Move all prototypes and variables used in multiple .c files into
>   common ifconfig.h.  Basically rename brconfig.h to ifconfig.h and
>   also use it for sff.c.
> - Fix missing prototypes.
> - Global variable s is a bad name as it shadows local variables.
>   Call it sock and use it everywhere.
> 
> There are more warnings, but the diff is long enough already.
> 
> ok?

ok

> 
> bluhm
> 
> Index: sbin/ifconfig/brconfig.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sbin/ifconfig/brconfig.c,v
> retrieving revision 1.22
> diff -u -p -r1.22 brconfig.c
> --- sbin/ifconfig/brconfig.c  28 Jun 2019 13:32:44 -  1.22
> +++ sbin/ifconfig/brconfig.c  17 Oct 2019 13:12:51 -
> @@ -47,7 +47,7 @@
>  #include 
>  #include 
> 
> -#include "brconfig.h"
> +#include "ifconfig.h"
> 
>  void bridge_ifsetflag(const char *, u_int32_t);
>  void bridge_ifclrflag(const char *, u_int32_t);
> @@ -202,7 +202,7 @@ addlocal(const char *ifsname, int d)
>   /* Add local */
>   strlcpy(breq.ifbr_name, name, sizeof(breq.ifbr_name));
>   strlcpy(breq.ifbr_ifsname, ifsname, sizeof(breq.ifbr_ifsname));
> - if (ioctl(s, SIOCBRDGADDL, (caddr_t)) == -1) {
> + if (ioctl(sock, SIOCBRDGADDL, (caddr_t)) == -1) {
>   if (errno == EEXIST)
>   return;
>   else
> @@ -217,12 +217,12 @@ bridge_ifsetflag(const char *ifsname, u_
> 
>   strlcpy(req.ifbr_name, name, sizeof(req.ifbr_name));
>   strlcpy(req.ifbr_ifsname, ifsname, sizeof(req.ifbr_ifsname));
> - if (ioctl(s, SIOCBRDGGIFFLGS, (caddr_t)) == -1)
> + if (ioctl(sock, SIOCBRDGGIFFLGS, (caddr_t)) == -1)
>   err(1, "%s: ioctl SIOCBRDGGIFFLGS %s", name, ifsname);
> 
>   req.ifbr_ifsflags |= flag & ~IFBIF_RO_MASK;
> 
> - if (ioctl(s, SIOCBRDGSIFFLGS, (caddr_t)) == -1)
> + if (ioctl(sock, SIOCBRDGSIFFLGS, (caddr_t)) == -1)
>   err(1, "%s: ioctl SIOCBRDGSIFFLGS %s", name, ifsname);
>  }
> 
> @@ -234,12 +234,12 @@ bridge_ifclrflag(const char *ifsname, u_
>   strlcpy(req.ifbr_name, name, sizeof(req.ifbr_name));
>   strlcpy(req.ifbr_ifsname, ifsname, sizeof(req.ifbr_ifsname));
> 
> - if (ioctl(s, SIOCBRDGGIFFLGS, (caddr_t)) == -1)
> + if (ioctl(sock, SIOCBRDGGIFFLGS, (caddr_t)) == -1)
>   err(1, "%s: ioctl SIOCBRDGGIFFLGS %s", name, ifsname);
> 
>   req.ifbr_ifsflags &= ~(flag | IFBIF_RO_MASK);
> 
> - if (ioctl(s, SIOCBRDGSIFFLGS, (caddr_t)) == -1)
> + if (ioctl(sock, SIOCBRDGSIFFLGS, (caddr_t)) == -1)
>   err(1, "%s: ioctl SIOCBRDGSIFFLGS %s", name, ifsname);
>  }
> 
> @@ -250,7 +250,7 @@ bridge_flushall(const char *val, int p)
> 
>   strlcpy(req.ifbr_name, name, sizeof(req.ifbr_name));
>   req.ifbr_ifsflags = IFBF_FLUSHALL;
> - if (ioctl(s, SIOCBRDGFLUSH, ) == -1)
> + if (ioctl(sock, SIOCBRDGFLUSH, ) == -1)
>   err(1, "%s", name);
>  }
> 
> @@ -261,7 +261,7 @@ bridge_flush(const char *val, int p)
> 
>   strlcpy(req.ifbr_name, name, sizeof(req.ifbr_name));
>   req.ifbr_ifsflags = IFBF_FLUSHDYN;
> - if (ioctl(s, SIOCBRDGFLUSH, ) == -1)
> + if (ioctl(sock, SIOCBRDGFLUSH, ) == -1)
>   err(1, "%s", name);
>  }
> 
> @@ -275,7 +275,7 @@ bridge_cfg(const char *delim)
>   u_int16_t bprio;
> 
>   strlcpy(ifbp.ifbop_name, name, sizeof(ifbp.ifbop_name));
> - if (ioctl(s, SIOCBRDGGPARAM, (caddr_t)) == -1) {
> + if (ioctl(sock, SIOCBRDGGPARAM, (caddr_t)) == -1) {
>   if (errno == ENOTTY)
>   return;
>   err(1, "%s SIOCBRDGGPARAM", name);
> @@ -323,7 +323,7 @@ bridge_list(char *delim)
>   err(1, "malloc");
>   bifc.ifbic_buf = inbuf = inb;
>   strlcpy(bifc.ifbic_name, name, sizeof(bifc.ifbic_name));
> - if (ioctl(s, SIOCBRDGIFS, ) == -1) {
> + if (ioctl(sock, SIOCBRDGIFS, ) == -1) {
>   if (errno == ENOTTY)
>   return;
>   err(1, "%s SIOCBRDGIFS", name);
> @@ -371,7 +371,7 @@ bridge_add(const char *ifn, int d)
> 
>   strlcpy(req.ifbr_name, name, sizeof(req.ifbr_name));
>   strlcpy(req.ifbr_ifsname, ifn, sizeof(req.ifbr_ifsname));
> - if (ioctl(s, SIOCBRDGADD, ) == -1) {
> + if (ioctl(sock, SIOCBRDGADD, ) == -1) {
>   if (errno == EEXIST)
>   return;
>   err(1, "%s: %s", name, ifn);
> @@ -385,7 +385,7 @@ bridge_delete(const char *ifn, int d)
> 
>   strlcpy(req.ifbr_name, name, sizeof(req.ifbr_name));
>   strlcpy(req.ifbr_ifsname, ifn, sizeof(req.ifbr_ifsname));
> - if (ioctl(s, SIOCBRDGDEL, ) == -1)
> + if (ioctl(sock, SIOCBRDGDEL, ) == -1)
>   err(1, "%s: %s", name, ifn);
>  }
> 
> 

Re: ifTypes, if_data->if_type, conflicts and too small

[Iain R. Learmonth]

Hi,

On 18/10/2019 17:19, Theo de Raadt wrote:
>> 3. Track down all the places that this might be used, and make sure they
>> are also changed to handle a u_int32_t.
> 
> Gigantic ABI breakage.
> 
> Hope you have fun upgrading bsd, ifconfig, and other tools through that
> change.

Yeah, I didn't say this would be fun.

>> The only place that I can think of that really might break is SNMP
>> clients,
> 
> If you change the offset of all the fields in if_data, and all the macros
> that access inside it, you'll break a lot more than the snmp tools.

I'm assuming that within OpenBSD base and ports everything can be fixed.
I mean SNMP clients like monitoring appliances, because these numbers
(maybe, I didn't check) appear on the wire.

> Some problems created by IANA cannot be solved.  In the coming decade I
> expect they'll come to realize they've allocated all the numbers in the
> protocols and services files, and start allocating out-of-range numbers
> there also.  They get into this situation reasons: they hand out numbers
> for things which don't actually need unique numbers, then don't delete
> ancient irrelevant things.
> 
> Should fix?  Very debatable.

I can also just say everything is fine and make up another "private"
assignment. If people don't feel the work is worth doing, then I won't
do it.

We have 31 numbers we didn't assign to something yet in if_types.h,
which is probably enough for some years yet.

Thanks,
Iain.

Re: ifTypes, if_data->if_type, conflicts and too small

[Theo de Raadt]

> This presents another problem because the if_type member of the if_data
> struct is defined as a u_char and so the type I've been assigned isn't
> even going to fit.

Riight.

> 3. Track down all the places that this might be used, and make sure they
> are also changed to handle a u_int32_t.

Gigantic ABI breakage.

Hope you have fun upgrading bsd, ifconfig, and other tools through that
change.

> The only place that I can think of that really might break is SNMP
> clients,

If you change the offset of all the fields in if_data, and all the macros
that access inside it, you'll break a lot more than the snmp tools.

 because I think that the ifType may appear in the SNMP tree.
> This is unfortunate, but there are probably more clients that are
> currently interpreting CARP interfaces as "Internal LAN on a bridge per
> IEEE 802.1ap" and MBIM as "Gigabit-capable passive optical networks
> (G-PON) as per ITU-T G.948". If we want to have standards compliance, we
> *should* fix this.

Some problems created by IANA cannot be solved.  In the coming decade I
expect they'll come to realize they've allocated all the numbers in the
protocols and services files, and start allocating out-of-range numbers
there also.  They get into this situation reasons: they hand out numbers
for things which don't actually need unique numbers, then don't delete
ancient irrelevant things.

Should fix?  Very debatable.

ifTypes, if_data->if_type, conflicts and too small

[Iain R. Learmonth]

Hi,

For context: I'm currently working on a patch set for OpenBSD that adds
support for AX.25 network interfaces.

Constants for interface types are defined in  based on
the IANA registry for ifTypes maintained as:

https://www.iana.org/assignments/ianaiftype-mib

As the bottom of this file, we see a comment:

/* private usage... how should we define these? */

Oh dear. There is no private use range defined in this registry, and as
far as I can tell once we'd made up our own assignments here no
questions were asked when we made up more. There are a total of 11
"private" interface types defined, all of which clash with types defined
in the IANA registry we claim to base these defines on.

Before I'd looked too far into this, I read about the IANA registry and
went and registered an interface type for AX.25, which is now allocated
as 298. The process for this was entirely painless and they responded
within days.

This presents another problem because the if_type member of the if_data
struct is defined as a u_char and so the type I've been assigned isn't
even going to fit.

Before I go any further here, I wanted to ask whether this has come up
before, whether anyone is looking at it, any ideas, etc?

If no one has yet looked at this and no one cares how it is fixed, I
would do the following:

1. Register each of the types we have as "private" allocations with IANA.
2. Increase the if_type member to be a u_int32_t.
3. Track down all the places that this might be used, and make sure they
are also changed to handle a u_int32_t.
4. See how badly things break during an upgrade, and if some
compatibility things need to be done (e.g. have two sets of ioctls for a
release period).
5. Sync if_types.h with the IANA registry again, which will mean that
the private use types change their numerical value.

The only place that I can think of that really might break is SNMP
clients, because I think that the ifType may appear in the SNMP tree.
This is unfortunate, but there are probably more clients that are
currently interpreting CARP interfaces as "Internal LAN on a bridge per
IEEE 802.1ap" and MBIM as "Gigabit-capable passive optical networks
(G-PON) as per ITU-T G.948". If we want to have standards compliance, we
*should* fix this.

Any comments and thoughts are welcome.

Thanks,
Iain.

Re: OpenBSD 6.x and wxallowed

[Theo de Raadt]

Nelson H. F. Beebe  wrote:

> If only a small number of packages need W^X capability, would it make
> sense to create a separate file tree for them, and let every other
> part of the filesystem enjoy W^X protection, along with additional
> security from addition of pledge() and veil() promises into software
> packages?

We did that.  They are all in /usr/local

But you went and disabled it.

Some people just cannot be helped.

No, we will not differentiate those binaries further via a symbolic
link far.

Re: OpenBSD 6.x and wxallowed

[Bryan Steele]

On Fri, Oct 18, 2019 at 07:39:26AM -0600, Nelson H. F. Beebe wrote:
> Because I dislike splitting disks into numerous partitions, each of
> whose sizes is a future show-stopper when they prove too small, I
> generally split disks into just root + swap.  Thus, I find on our
> currently 7 versions of OpenBSD 6.x in our test farm reports like
> this:
> 
>   # mount 
>   /dev/wd0a on / type ffs (local, wxallowed)


You are creating your own problem.

By default, /usr/local is mounted wxallowed. If you choose to create
a single root partition, you're responsible for maintaining your own
exploit mitigation countermeasures.

> The output of "man mount" says
> 
> wxallowed  Processes that ask for memory to be made writeable
>  plus executable using the mmap(2) and mprotect(2)
>  system calls are killed by default.  This option
>  allows those processes to continue operation.  It is
>  typically used on the /usr/local filesystem.
> 
> OpenBSD 3.3 introduced the W^X feature in 2004, and some other O/Ses
> have implemented it as well since then.
> 
> Has anyone looked into the problem of enumerating packages that are
> installed in the /usr/local tree that actually NEED simultaneous write
> and execute access?
> 
> If only a small number of packages need W^X capability, would it make
> sense to create a separate file tree for them, and let every other
> part of the filesystem enjoy W^X protection, along with additional
> security from addition of pledge() and veil() promises into software
> packages?
> 
> 
> ---
> - Nelson H. F. BeebeTel: +1 801 581 5254  
> -
> - University of UtahFAX: +1 801 581 4148  
> -
> - Department of Mathematics, 110 LCBInternet e-mail: be...@math.utah.edu  
> -
> - 155 S 1400 E RM 233   be...@acm.org  be...@computer.org 
> -
> - Salt Lake City, UT 84112-0090, USAURL: http://www.math.utah.edu/~beebe/ 
> -
> ---
> 
> 

Re: xhci(4): fix endless loop when transfer is aborted

[Martin Pieuchot]

On 18/10/19(Fri) 14:55, Stefan Sperling wrote:
> On Fri, Oct 18, 2019 at 12:54:06PM +0200, Stefan Sperling wrote:
> > On Fri, Oct 18, 2019 at 11:37:01AM +0200, Martin Pieuchot wrote:
> > > The question here is how an `xfer' with a status of NORMAL_COMPLETION
> > > can be found in the pipe's queue?
> > 
> > I don't know yet.
> 
> I have managed to track it down after realizing that the xfer which
> triggers the problem is allocated very early on, in ulpt_open().
> 
> It it always is one of these (from ulpt.c):
> 
>   sc->sc_in_xfer1 = usbd_alloc_xfer(sc->sc_udev);
>   sc->sc_in_xfer2 = usbd_alloc_xfer(sc->sc_udev);
> 
> Our friend which queues an xfer to the pipe after an error occured
> is this xfer callback which lacks any error checking whatsoever:
> 
> static void
> ulpt_input(struct usbd_xfer *xfer, void *priv, usbd_status status)
> {
>   struct ulpt_softc *sc = priv;
> 
>   DPRINTFN(2,("ulpt_input: got some data\n"));
>   /* Do it again. */
>   if (xfer == sc->sc_in_xfer1)
>   usbd_transfer(sc->sc_in_xfer2);
>   else
>   usbd_transfer(sc->sc_in_xfer1);
> }
> 
> 
> Proof that the problematic pipe is ulpt's sc->sc_in_pipe:
> 
> ulptwrite: transfer 16384 bytes
> ulpt_input: xfer=0xfd811f8da0f0 status=0
>   (at this point sc_in_xfer1 has completed, and sc_in_xfer2 is queued)
> xhci0: txerr? code 4
> ulpt_input: xfer=0xfd811f8da2d0 status=13
>  (at this point sc_in_xfer2 has failed, and sc_in_xfer1 is queued)
> usb0: usb_needs_explore
>  (the USB stack tries to detach the printer which has been turned off)
> usb_add_task: task=0x80093e80 state=0 type=1
> usb_explore: usb0
> usbd_do_request_flags
> usbd_do_request_flags: new xfer=0xfd811f8da690
> usbd_do_request_flags
> usbd_do_request_flags: new xfer=0xfd811f8da690
> ulpt_detach: sc=0x80e63600
> ulpt_detach: aborting pipe=0x80ddd000
> usbd_abort_pipe: pipe=0x80ddd000
> ulpt_detach: aborting pipe=0x80dbc000
>   (ulpt attempts to abort sc->sc_in_pipe which has sc_in_xfer1 queued)
> usbd_abort_pipe: pipe=0x80dbc000
> usbd_abort_pipe: pipe=0x80dbc000 xfer=0xfd811f8da0f0 
> (methods=0x81f41508)
> xhci_abort_xfer: xfer=0xfd811f8da0f0 status=NORMAL_COMPLETION 
> err=CANCELLED actlen=23 len=64 idx=-1
> xhci_abort_xfer: already done
>   (endless loop as discussed before)
> 
> So I would suggest this diff:

As soon as a transfer with an error condition is passed to
usb_transfer_complete(), usbd_start_next() won't be called.

If the callback of that transfer had already enqueue a new xfer, via
usbd_transfer(), it will stay on the queue until the pipe is aborted.

Currently all our drivers call usbd_transfer() without reinitializing
`xfer->status' and that confuses *hci's abort() methods.

Does the diff below help?

Index: usbdi.c
===
RCS file: /cvs/src/sys/dev/usb/usbdi.c,v
retrieving revision 1.101
diff -u -p -r1.101 usbdi.c
--- usbdi.c 6 Oct 2019 17:11:51 -   1.101
+++ usbdi.c 18 Oct 2019 15:11:29 -
@@ -294,6 +294,7 @@ usbd_transfer(struct usbd_xfer *xfer)
usbd_dump_queue(pipe);
 #endif
xfer->done = 0;
+   xfer->status = USBD_NOT_STARTED;
 
if (pipe->aborting)
return (USBD_CANCELLED);

Re: OpenBSD 6.x and wxallowed

[Florian Obser]

On Fri, Oct 18, 2019 at 07:39:26AM -0600, Nelson H. F. Beebe wrote:
> Because I dislike splitting disks into numerous partitions, each of
> whose sizes is a future show-stopper when they prove too small, I
> generally split disks into just root + swap.

> If only a small number of packages need W^X capability, would it make
> sense to create a separate file tree for them

OpenBSD 6.x and wxallowed

[Nelson H. F. Beebe]

Because I dislike splitting disks into numerous partitions, each of
whose sizes is a future show-stopper when they prove too small, I
generally split disks into just root + swap.  Thus, I find on our
currently 7 versions of OpenBSD 6.x in our test farm reports like
this:

# mount 
/dev/wd0a on / type ffs (local, wxallowed)

The output of "man mount" says

wxallowed  Processes that ask for memory to be made writeable
   plus executable using the mmap(2) and mprotect(2)
   system calls are killed by default.  This option
   allows those processes to continue operation.  It is
   typically used on the /usr/local filesystem.

OpenBSD 3.3 introduced the W^X feature in 2004, and some other O/Ses
have implemented it as well since then.

Has anyone looked into the problem of enumerating packages that are
installed in the /usr/local tree that actually NEED simultaneous write
and execute access?

If only a small number of packages need W^X capability, would it make
sense to create a separate file tree for them, and let every other
part of the filesystem enjoy W^X protection, along with additional
security from addition of pledge() and veil() promises into software
packages?


---
- Nelson H. F. BeebeTel: +1 801 581 5254  -
- University of UtahFAX: +1 801 581 4148  -
- Department of Mathematics, 110 LCBInternet e-mail: be...@math.utah.edu  -
- 155 S 1400 E RM 233   be...@acm.org  be...@computer.org -
- Salt Lake City, UT 84112-0090, USAURL: http://www.math.utah.edu/~beebe/ -
---

Re: mbuf limit atomic operation

[Alexander Bluhm]

On Fri, Oct 18, 2019 at 03:55:39PM +1000, David Gwynne wrote:
> why? is it significantly faster? page allocation should be in the slow path.

At least it was not slower.  Performance were slightly higher, but
changes are well below meassurement tolerance.

Usually it is a good idea to avoid locks and mutexes if possible.
Atomic operations cannot create deadlocks.

In this case the code becomes shorter.

I have put back the unprotected check in m_pool_alloc().  It avoids
possibly expensive atomic operations.  Do we want this?  I could
not measure any difference.

bluhm

Index: kern/uipc_mbuf.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/kern/uipc_mbuf.c,v
retrieving revision 1.272
diff -u -p -r1.272 uipc_mbuf.c
--- kern/uipc_mbuf.c19 Jul 2019 09:03:03 -  1.272
+++ kern/uipc_mbuf.c18 Oct 2019 12:33:43 -
@@ -133,7 +133,6 @@ struct  mutex m_extref_mtx = MUTEX_INITIA
 void   m_extfree(struct mbuf *);
 void   m_zero(struct mbuf *);

-struct mutex m_pool_mtx = MUTEX_INITIALIZER(IPL_NET);
 unsigned long mbuf_mem_limit;  /* how much memory can be allocated */
 unsigned long mbuf_mem_alloc;  /* how much memory has been allocated */

@@ -1473,30 +1472,23 @@ m_microtime(const struct mbuf *m, struct
 void *
 m_pool_alloc(struct pool *pp, int flags, int *slowdown)
 {
-   void *v = NULL;
-   int avail = 1;
+   void *v;
+   long alloc;

if (mbuf_mem_alloc + pp->pr_pgsize > mbuf_mem_limit)
return (NULL);

-   mtx_enter(_pool_mtx);
-   if (mbuf_mem_alloc + pp->pr_pgsize > mbuf_mem_limit)
-   avail = 0;
-   else
-   mbuf_mem_alloc += pp->pr_pgsize;
-   mtx_leave(_pool_mtx);
-
-   if (avail) {
-   v = (*pool_allocator_multi.pa_alloc)(pp, flags, slowdown);
+   alloc = atomic_add_long_nv(_mem_alloc, pp->pr_pgsize);
+   if (alloc > mbuf_mem_limit)
+   goto fail;

-   if (v == NULL) {
-   mtx_enter(_pool_mtx);
-   mbuf_mem_alloc -= pp->pr_pgsize;
-   mtx_leave(_pool_mtx);
-   }
-   }
+   v = (*pool_allocator_multi.pa_alloc)(pp, flags, slowdown);
+   if (v != NULL)
+   return (v);

-   return (v);
+ fail:
+   atomic_sub_long(_mem_alloc, pp->pr_pgsize);
+   return (NULL);
 }

 void
@@ -1504,9 +1496,7 @@ m_pool_free(struct pool *pp, void *v)
 {
(*pool_allocator_multi.pa_free)(pp, v);

-   mtx_enter(_pool_mtx);
-   mbuf_mem_alloc -= pp->pr_pgsize;
-   mtx_leave(_pool_mtx);
+   atomic_sub_long(_mem_alloc, pp->pr_pgsize);
 }

 void

Re: xhci(4): fix endless loop when transfer is aborted

[Stefan Sperling]

On Fri, Oct 18, 2019 at 12:54:06PM +0200, Stefan Sperling wrote:
> On Fri, Oct 18, 2019 at 11:37:01AM +0200, Martin Pieuchot wrote:
> > The question here is how an `xfer' with a status of NORMAL_COMPLETION
> > can be found in the pipe's queue?
> 
> I don't know yet.

I have managed to track it down after realizing that the xfer which
triggers the problem is allocated very early on, in ulpt_open().

It it always is one of these (from ulpt.c):

sc->sc_in_xfer1 = usbd_alloc_xfer(sc->sc_udev);
sc->sc_in_xfer2 = usbd_alloc_xfer(sc->sc_udev);

Our friend which queues an xfer to the pipe after an error occured
is this xfer callback which lacks any error checking whatsoever:

static void
ulpt_input(struct usbd_xfer *xfer, void *priv, usbd_status status)
{
struct ulpt_softc *sc = priv;

DPRINTFN(2,("ulpt_input: got some data\n"));
/* Do it again. */
if (xfer == sc->sc_in_xfer1)
usbd_transfer(sc->sc_in_xfer2);
else
usbd_transfer(sc->sc_in_xfer1);
}


Proof that the problematic pipe is ulpt's sc->sc_in_pipe:

ulptwrite: transfer 16384 bytes
ulpt_input: xfer=0xfd811f8da0f0 status=0
  (at this point sc_in_xfer1 has completed, and sc_in_xfer2 is queued)
xhci0: txerr? code 4
ulpt_input: xfer=0xfd811f8da2d0 status=13
 (at this point sc_in_xfer2 has failed, and sc_in_xfer1 is queued)
usb0: usb_needs_explore
 (the USB stack tries to detach the printer which has been turned off)
usb_add_task: task=0x80093e80 state=0 type=1
usb_explore: usb0
usbd_do_request_flags
usbd_do_request_flags: new xfer=0xfd811f8da690
usbd_do_request_flags
usbd_do_request_flags: new xfer=0xfd811f8da690
ulpt_detach: sc=0x80e63600
ulpt_detach: aborting pipe=0x80ddd000
usbd_abort_pipe: pipe=0x80ddd000
ulpt_detach: aborting pipe=0x80dbc000
  (ulpt attempts to abort sc->sc_in_pipe which has sc_in_xfer1 queued)
usbd_abort_pipe: pipe=0x80dbc000
usbd_abort_pipe: pipe=0x80dbc000 xfer=0xfd811f8da0f0 
(methods=0x81f41508)
xhci_abort_xfer: xfer=0xfd811f8da0f0 status=NORMAL_COMPLETION err=CANCELLED 
actlen=23 len=64 idx=-1
xhci_abort_xfer: already done
  (endless loop as discussed before)

So I would suggest this diff:

diff b8f72638b63831677f92fc0c98e5d2d00058e226 /usr/src (staged changes)
blob - 9a6befb293d733e157123a8d07be191206a99a4c
blob + b5ac024f334b203411243c9148b801a69e289b8a
--- sys/dev/usb/ulpt.c
+++ sys/dev/usb/ulpt.c
@@ -433,7 +433,11 @@ ulpt_input(struct usbd_xfer *xfer, void *priv, usbd_st
 {
struct ulpt_softc *sc = priv;
 
-   DPRINTFN(2,("ulpt_input: got some data\n"));
+   DPRINTFN(2,("ulpt_input: xfer=%p status=%d\n", xfer, status));
+
+   if (status != USBD_NORMAL_COMPLETION)
+   return;
+
/* Do it again. */
if (xfer == sc->sc_in_xfer1)
usbd_transfer(sc->sc_in_xfer2);


Which results in a clean ulpt detach:

ulptwrite: transfer 16384 bytes
ulptwrite
usbd_do_request_flags
ulpt_status: status=0x18 err=0
ulptwrite: transfer 16384 bytes
xhci0: txerr? code 4
ulpt_input: xfer=0xfd811f8da2d0 status=13
xhci0: txerr? code 4
usbd_clear_endpoint_stall
usbd_do_request_flags
xhci0: txerr? code 4
ulptwrite: error=13
usb0: usb_needs_explore
usb_add_task: task=0x80093e80 state=0 type=1
usb_explore: usb0
usbd_do_request_flags
usbd_do_request_flags
ulpt_detach: sc=0x80e63900
ulpt_detach: aborting pipe=0x80e73000
usbd_abort_pipe: pipe=0x80e73000
ulpt_detach: aborting pipe=0x80e8
usbd_abort_pipe: pipe=0x80e8
xhci0: xhci_cmd_configure_ep dev 10
usbd_abort_pipe: pipe=0x80e8
xhci0: xhci_cmd_configure_ep dev 10
ulptclose: closed
ulpt0 detached
usbd_abort_pipe: pipe=0x80ddd000
xhci0: xhci_cmd_configure_ep dev 10
xhci0: xhci_cmd_slot_control

Re: xhci(4): fix endless loop when transfer is aborted

[Stefan Sperling]

On Fri, Oct 18, 2019 at 11:37:01AM +0200, Martin Pieuchot wrote:
> The question here is how an `xfer' with a status of NORMAL_COMPLETION
> can be found in the pipe's queue?

I don't know yet.

Here is some output with usbdebug = 6.
The error handling starts at line "xhci0: txerr? code 4" and eventually
the code loops on the cancelled xfer as shown before.
Produced with the following diff and doing a manual write to usbdebug
with ddb before starting the print job (the system won't boot fast
enough with usbdebug set to 6).

Does this help?

diff b8f72638b63831677f92fc0c98e5d2d00058e226 /usr/src
blob - e2c8f1b97b22f57327768022bb12ae03f6c9cdc0
file + sys/dev/usb/usb.c
--- sys/dev/usb/usb.c
+++ sys/dev/usb/usb.c
@@ -66,10 +66,11 @@
 #include 
 #include 
 
+#define USB_DEBUG
 #ifdef USB_DEBUG
 #define DPRINTF(x) do { if (usbdebug) printf x; } while (0)
 #define DPRINTFN(n,x)  do { if (usbdebug>(n)) printf x; } while (0)
-intusbdebug = 0;
+intusbdebug = 5;
 #if defined(UHCI_DEBUG) && NUHCI > 0
 extern int uhcidebug;
 #endif
blob - e51b4b31947a37ac3127fddef935ba2b4c326778
file + sys/dev/usb/usbdi.c
--- sys/dev/usb/usbdi.c
+++ sys/dev/usb/usbdi.c
@@ -45,6 +45,7 @@
 #include 
 #include 
 
+#define USB_DEBUG
 #ifdef USB_DEBUG
 #define DPRINTF(x) do { if (usbdebug) printf x; } while (0)
 #define DPRINTFN(n,x)  do { if (usbdebug>(n)) printf x; } while (0)
blob - f9d7eb3a4018dd5b628f0d866630f5732cf53593
file + sys/dev/usb/xhci.c
--- sys/dev/usb/xhci.c
+++ sys/dev/usb/xhci.c
@@ -41,6 +41,7 @@ struct cfdriver xhci_cd = {
NULL, "xhci", DV_DULL
 };
 
+#define XHCI_DEBUG
 #ifdef XHCI_DEBUG
 #define DPRINTF(x) do { if (xhcidebug) printf x; } while(0)
 #define DPRINTFN(n,x)  do { if (xhcidebug>(n)) printf x; } while (0)
@@ -2176,6 +2177,7 @@ xhci_abort_xfer(struct usbd_xfer *xfer, usbd_status st
/* Transfer is already done. */
if (xfer->status != USBD_IN_PROGRESS) {
DPRINTF(("%s: already done \n", __func__));
+   usbd_dump_pipe(xfer->pipe);
return;
}
 


usbd_alloc_xfer() = 0xfd811f8da1e0
usbd_transfer: xfer=0xfd811f8da1e0, flags=2, pipe=0x80dbc000, 
running=0
usbd_dump_queue: pipe=0x80dbc000
usb_insert_transfer: pipe=0x80dbc000 running=0 timeout=5000
usb_transfer_complete: pipe=0x80dbc000 xfer=0xfd811f8da1e0 status=0 
actlen=1
usb_transfer_complete: repeat=0 new head=0x0
usbd_start_next: pipe=0x80dbc000, xfer=0x0
usbd_free_xfer: 0xfd811f8da1e0
usbd_transfer: xfer=0xfd811f8da2d0, flags=19, pipe=0x80e65000, 
running=0
usbd_dump_queue: pipe=0x80e65000
usb_insert_transfer: pipe=0x80e65000 running=0 timeout=0
usb_transfer_complete: pipe=0x80e65000 xfer=0xfd811f8da2d0 status=0 
actlen=16384
usb_transfer_complete: repeat=0 new head=0x0
usbd_start_next: pipe=0x80e65000, xfer=0x0
usbd_alloc_xfer() = 0xfd811f8da1e0
usbd_transfer: xfer=0xfd811f8da1e0, flags=2, pipe=0x80dbc000, 
running=0
usbd_dump_queue: pipe=0x80dbc000
usb_insert_transfer: pipe=0x80dbc000 running=0 timeout=5000
usb_transfer_complete: pipe=0x80dbc000 xfer=0xfd811f8da1e0 status=0 
actlen=1
usb_transfer_complete: repeat=0 new head=0x0
usbd_start_next: pipe=0x80dbc000, xfer=0x0
usbd_free_xfer: 0xfd811f8da1e0
usbd_transfer: xfer=0xfd811f8da2d0, flags=19, pipe=0x80e65000, 
running=0
usbd_dump_queue: pipe=0x80e65000
usb_insert_transfer: pipe=0x80e65000 running=0 timeout=0
usb_transfer_complete: pipe=0x80e65000 xfer=0xfd811f8da2d0 status=0 
actlen=16384
usb_transfer_complete: repeat=0 new head=0x0
usbd_start_next: pipe=0x80e65000, xfer=0x0
usbd_alloc_xfer() = 0xfd811f8da1e0
usbd_transfer: xfer=0xfd811f8da1e0, flags=2, pipe=0x80dbc000, 
running=0
usbd_dump_queue: pipe=0x80dbc000
usb_insert_transfer: pipe=0x80dbc000 running=0 timeout=5000
usb_transfer_complete: pipe=0x80dbc000 xfer=0xfd811f8da1e0 status=0 
actlen=1
usb_transfer_complete: repeat=0 new head=0x0
usbd_start_next: pipe=0x80dbc000, xfer=0x0
usbd_free_xfer: 0xfd811f8da1e0
usbd_transfer: xfer=0xfd811f8da2d0, flags=19, pipe=0x80e65000, 
running=0
usbd_dump_queue: pipe=0x80e65000
usb_insert_transfer: pipe=0x80e65000 running=0 timeout=0
usb_transfer_complete: pipe=0x80e65000 xfer=0xfd811f8da2d0 status=0 
actlen=16384
usb_transfer_complete: repeat=0 new head=0x0
usbd_start_next: pipe=0x80e65000, xfer=0x0
usbd_alloc_xfer() = 0xfd811f8da1e0
usbd_transfer: xfer=0xfd811f8da1e0, flags=2, pipe=0x80dbc000, 
running=0
usbd_dump_queue: pipe=0x80dbc000
usb_insert_transfer: pipe=0x80dbc000 running=0 timeout=5000
usb_transfer_complete: pipe=0x80dbc000 xfer=0xfd811f8da1e0 status=0 
actlen=1
usb_transfer_complete: repeat=0 new head=0x0
usbd_start_next: pipe=0x80dbc000, xfer=0x0

Re: netstart diff for aggr(4) handling

[Claudio Jeker]

On Thu, Oct 17, 2019 at 11:24:24PM +0200, Alexander Bluhm wrote:
> On Thu, Oct 17, 2019 at 04:47:27PM -0400, Brad Smith wrote:
> > Shouldn't aggr(4) be handled in the same manner as trunk(4)?
> 
> I guess so, OK bluhm@

+1 OK claudio
 
> > Index: netstart
> > ===
> > RCS file: /home/cvs/src/etc/netstart,v
> > retrieving revision 1.200
> > diff -u -p -u -p -r1.200 netstart
> > --- netstart29 Aug 2018 11:30:48 -  1.200
> > +++ netstart17 Oct 2019 20:45:29 -
> > @@ -300,12 +300,12 @@ vifscreate
> >
> >  # Configure all the non-loopback interfaces which we know about, but
> >  # do not start interfaces which must be delayed. Refer to hostname.if(5)
> > -ifmstart "" "trunk svlan vlan carp pppoe tun tap gif etherip gre egre 
> > mobileip pflow"
> > +ifmstart "" "aggr trunk svlan vlan carp pppoe tun tap gif etherip gre egre 
> > mobileip pflow"
> >
> >  # The trunk interfaces need to come up first in this list.
> >  # The (s)vlan interfaces need to come up after trunk.
> >  # Configure all the carp interfaces which we know about before default 
> > route.
> > -ifmstart "trunk svlan vlan carp pppoe"
> > +ifmstart "aggr trunk svlan vlan carp pppoe"
> >
> >  # Set default routes for IPv4 and IPv6.
> >  defaultroute
> 

-- 
:wq Claudio

Re: xhci(4): fix endless loop when transfer is aborted

[Martin Pieuchot]

On 17/10/19(Thu) 18:53, Stefan Sperling wrote:
> My USB scanner works again in 6.6 \o/
> Thanks to whoever fixed it. I have no idea when and how it happened but
> since a few releases back it has always been failed with some I/O error.
> But now it just works again as it once did (apart from a weird Xsane
> bug where it scans only part of the page which I will ignore for now;
> the 'scanimage' command scans as expected).
> 
> So I scanned an image and tried to print it.
> The default image format produced by scanimage is .pnm, and when I queued
> several megabytes of pnm data with lpr my printer started printing page
> after page of garbage.
> So I turned the printer off and was surprised to see that doing so
> made the whole machine hang hard. Nothing even on serial console.
> 
> I can reliably reproduce this. With an XHCI_DEBUG kernel I see the
> following messages scoll by very fast on the serial console:
> 
> xhci_abort_xfer: already done
> usbd_abort_pipe: pipe=0x80e84000 xfer=0xfd811f8da1e0 
> (methods=0x81efb9f8)
> xhci_abort_xfer: xfer=0xfd811f8da1e0 status=NORMAL_COMPLETION 
> err=CANCELLED actlen=23 len=64 idx=-1
> xhci_abort_xfer: already done
> usbd_abort_pipe: pipe=0x80e84000 xfer=0xfd811f8da1e0 
> (methods=0x81efb9f8)
> xhci_abort_xfer: xfer=0xfd811f8da1e0 status=NORMAL_COMPLETION 
> err=CANCELLED actlen=23 len=64 idx=-1
> xhci_abort_xfer: already done
> usbd_abort_pipe: pipe=0x80e84000 xfer=0xfd811f8da1e0 
> (methods=0x81efb9f8)
> xhci_abort_xfer: xfer=0xfd811f8da1e0 status=NORMAL_COMPLETION 
> err=CANCELLED actlen=23 len=64 idx=-1
> xhci_abort_xfer: already done
> usbd_abort_pipe: pipe=0x80e84000 xfer=0xfd811f8da1e0 
> (methods=0x81efb9f8)
> xhci_abort_xfer: xfer=0xfd811f8da1e0 status=NORMAL_COMPLETION 
> err=CANCELLED actlen=23 len=64 idx=-1
> xhci_abort_xfer: already done
> 
> Here is the loop we are in at that point, in usbdi.c's usbd_abort_pipe:
> 
>   while ((xfer = SIMPLEQ_FIRST(>queue)) != NULL) {
>   DPRINTFN(2,("%s: pipe=%p xfer=%p (methods=%p)\n", __func__,
>   pipe, xfer, pipe->methods));
>   /* Make the HC abort it (and invoke the callback). */
>   pipe->methods->abort(xfer);
>   /* XXX only for non-0 usbd_clear_endpoint_stall(pipe); */
>   }
> 
> 
> Note how this will keep looping forever if the xfer is never taken off
> the queue. I don't understand internal USB stack interface contracts
> (does anyone, really?) but assuming that the abort method is always
> expected to dequeue the transfer by calling the usb_transfer_complete()
> function, then the following patch allows this loop to exit when I turn
> off my printer while it is printing.

The question here is how an `xfer' with a status of NORMAL_COMPLETION
can be found in the pipe's queue?

The operations are like

- *hci driver is notified that a USB transfer is finish
- dequeue it and set `xter->status' to USBD_NORMAL_COMPLETION
- call usb_transfer_complete() that should dequeue the `xfer' from the
  pipe.

The fact that the whole USB stack use the USBD_* status value as return
code makes it complicated to understand 8)  But that's the question we
should answer.

Re: probable error in 66.html regarding rpki-client

[Theo de Raadt]

I'm not sure it matters.  The text is not invalid.  It was
imported into the tree.  It just didn't ship a binary.

> On Wed, Oct 16, 2019 at 09:34:26PM -0600, Theo de Raadt wrote:
> > Ross L Richardson  wrote:
> > 
> > > 66.html claims that rpki-client is included, but:
> > > -  gives no result.
> > > - The test system I just sysupgraded doesn't have it.
> > > 
> > > It's not actually linked to the build (in src/usr.sbin/Makefile), is it?
> > 
> > It isn't.  It wasn't ready.
> 
> Here's a patch, then...
> 
> Ross
> 
> 
> Index: 66.html
> ===
> RCS file: /cvs/www/66.html,v
> retrieving revision 1.72
> diff -u -p -r1.72 66.html
> --- 66.html   17 Oct 2019 13:49:28 -  1.72
> +++ 66.html   18 Oct 2019 06:48:48 -
> @@ -500,8 +500,6 @@ to 6.6.
>always check for namespace collisions on table
>commands. Introduced 'pfctl -FR' to reset pfctl(8) settings to
>defaults.
> -Imported Kristaps Dzonsons' RPKI
> -  validator,  href="https://man.openbsd.org/rpki-client.8;>rpki-client(8).
>   https://man.openbsd.org/relayd.8;>relayd(8) now 
> supports
>binary protocol health checking. See
>https://man.openbsd.org/relayd.conf.5;>relayd.conf(5).
> 

Re: probable error in 66.html regarding rpki-client

[Ross L Richardson]

On Wed, Oct 16, 2019 at 09:34:26PM -0600, Theo de Raadt wrote:
> Ross L Richardson  wrote:
> 
> > 66.html claims that rpki-client is included, but:
> > -  gives no result.
> > - The test system I just sysupgraded doesn't have it.
> > 
> > It's not actually linked to the build (in src/usr.sbin/Makefile), is it?
> 
> It isn't.  It wasn't ready.

Here's a patch, then...

Ross


Index: 66.html
===
RCS file: /cvs/www/66.html,v
retrieving revision 1.72
diff -u -p -r1.72 66.html
--- 66.html 17 Oct 2019 13:49:28 -  1.72
+++ 66.html 18 Oct 2019 06:48:48 -
@@ -500,8 +500,6 @@ to 6.6.
   always check for namespace collisions on table
   commands. Introduced 'pfctl -FR' to reset pfctl(8) settings to
   defaults.
-Imported Kristaps Dzonsons' RPKI
-  validator, https://man.openbsd.org/rpki-client.8;>rpki-client(8).
  https://man.openbsd.org/relayd.8;>relayd(8) now supports
   binary protocol health checking. See
   https://man.openbsd.org/relayd.conf.5;>relayd.conf(5).

[PATCH] www - 66.html - fix a typo - aggregation

[Raf Czlonka]

Hello,

... and a small typo.

Raf
Index: 66.html
===
RCS file: /cvs/www/66.html,v
retrieving revision 1.72
diff -u -p -r1.72 66.html
--- 66.html 17 Oct 2019 13:49:28 -  1.72
+++ 66.html 18 Oct 2019 06:18:22 -
@@ -345,7 +345,7 @@ to 6.6.
 Added https://man.openbsd.org/iavf.4;>iavf(4), a driver 
for
   Intel SR-IOV Virtual Functions of Intel 700 series Ethernet controllers.
 Added https://man.openbsd.org/aggr.4;>aggr(4), a
-  dedicated driver to implement 802.1AX link aggregration.
+  dedicated driver to implement 802.1AX link aggregation.
 Added port protection support
   to https://man.openbsd.org/switch.4;>switch(4). Domain
   membership is checked for unicast, flooded (broadcast) and local

[PATCH] www - 66.html - fix a broken link to relayd(8) man page

[Raf Czlonka]

Hi,

The URL to the relayd(8) manual page is incomplete.

Regards,

Raf

Index: 66.html
===
RCS file: /cvs/www/66.html,v
retrieving revision 1.72
diff -u -p -r1.72 66.html
--- 66.html 17 Oct 2019 13:49:28 -  1.72
+++ 66.html 18 Oct 2019 06:16:14 -
@@ -511,7 +511,7 @@ to 6.6.
   support for SNI with new 'tls keypair' option to load additional
   certificates.
 Added support for 'from/to address[/prefix]'
-  in https://man.openbsd.org/.8;>relayd(8) filter rules.
+  in https://man.openbsd.org/relayd.8;>relayd(8) filter rules.
 Implemented RFC 8555 "Automatic Certificate Management
   Environment (ACME)" to
   enable https://man.openbsd.org/acme-client.1;>acme-client(1)