Re: umb(4) doesn't need a custom network input function

[Gerhard Roth]

On Fri, 2023-08-25 at 20:40 +1000, David Gwynne wrote:
> umb(4) is a hardware p2p driver, it just has ip coming in, so we can do
> the same thing we do for the address family and input processing as
> other p2p interfaces.
> 
> the short packet check that umb_input does is already done by the ip
> stacks, so we're not losing anything.
> 
> i havent got a umb(4), so i need someone to test this. oks are nice too.

Tested download of https://ftp.hostserver.de/pub/OpenBSD/ftplist with
its v4 and v6 address and it's ok. tcpdump also fine.

OK gerhard@


> 
> Index: if_umb.c
> ===
> RCS file: /cvs/src/sys/dev/usb/if_umb.c,v
> retrieving revision 1.51
> diff -u -p -r1.51 if_umb.c
> --- if_umb.c18 Apr 2023 22:01:23 -  1.51
> +++ if_umb.c25 Aug 2023 10:36:19 -
> @@ -138,7 +138,6 @@ void umb_close_bulkpipes(struct umb_so
>  int umb_ioctl(struct ifnet *, u_long, caddr_t);
>  int umb_output(struct ifnet *, struct mbuf *, struct sockaddr *,
>     struct rtentry *);
> -void    umb_input(struct ifnet *, struct mbuf *);
>  void    umb_start(struct ifnet *);
>  void    umb_rtrequest(struct ifnet *, int, struct rtentry *);
>  void    umb_watchdog(struct ifnet *);
> @@ -610,7 +609,8 @@ umb_attach(struct device *parent, struct
>     sizeof (struct ncm_pointer16);
> ifp->if_mtu = 1500; /* use a common default */
> ifp->if_hardmtu = sc->sc_maxpktlen;
> -   ifp->if_input = umb_input;
> +   ifp->if_bpf_mtap = p2p_bpf_mtap;
> +   ifp->if_input = p2p_input;
> ifp->if_output = umb_output;
> if_attach(ifp);
> if_alloc_sadl(ifp);
> @@ -910,48 +910,6 @@ umb_output(struct ifnet *ifp, struct mbu
> return if_enqueue(ifp, m);
>  }
>  
> -void
> -umb_input(struct ifnet *ifp, struct mbuf *m)
> -{
> -   uint32_t af;
> -
> -   if ((ifp->if_flags & IFF_UP) == 0) {
> -   m_freem(m);
> -   return;
> -   }
> -   if (m->m_pkthdr.len < sizeof (struct ip) + sizeof(af)) {
> -   ifp->if_ierrors++;
> -   DPRINTFN(4, "%s: dropping short packet (len %d)\n", __func__,
> -   m->m_pkthdr.len);
> -   m_freem(m);
> -   return;
> -   }
> -   m->m_pkthdr.ph_rtableid = ifp->if_rdomain;
> -
> -   /* pop off DLT_LOOP header, no longer needed */
> -   af = *mtod(m, uint32_t *);
> -   m_adj(m, sizeof (af));
> -   af = ntohl(af);
> -
> -   ifp->if_ibytes += m->m_pkthdr.len;
> -   switch (af) {
> -   case AF_INET:
> -   ipv4_input(ifp, m);
> -   return;
> -#ifdef INET6
> -   case AF_INET6:
> -   ipv6_input(ifp, m);
> -   return;
> -#endif /* INET6 */
> -   default:
> -   ifp->if_ierrors++;
> -   DPRINTFN(4, "%s: dropping packet with bad IP version (af 
> %d)\n",
> -   __func__, af);
> -   m_freem(m);
> -   return;
> -   }
> -}
> -
>  static inline int
>  umb_align(size_t bufsz, int offs, int alignment, int remainder)
>  {
> @@ -2376,7 +2334,7 @@ umb_decap(struct umb_softc *sc, struct u
> struct ifnet *ifp = GET_IFP(sc);
> int  s;
> void*buf;
> -   uint32_t len, af = 0;
> +   uint32_t len;
> char*dp;
> struct ncm_header16 *hdr16;
> struct ncm_header32 *hdr32;
> @@ -2499,20 +2457,14 @@ umb_decap(struct umb_softc *sc, struct u
> ifp->if_iqdrops++;
> continue;
> }
> -   m = m_prepend(m, sizeof(uint32_t), M_DONTWAIT);
> -   if (m == NULL) {
> -   ifp->if_iqdrops++;
> -   continue;
> -   }
> switch (*dp & 0xf0) {
> case 4 << 4:
> -   af = htonl(AF_INET);
> +   m->m_pkthdr.ph_family = AF_INET;
> break;
> case 6 << 4:
> -   af = htonl(AF_INET6);
> +   m->m_pkthdr.ph_family = AF_INET6;
> break;
> }
> -   *mtod(m, uint32_t *) = af;
> ml_enqueue(, m);
> }
>  done:
> 



smime.p7s
Description: S/MIME cryptographic signature

Re: ober_scanf_elements() and empty sequences

[Gerhard Roth]

On Tue, 2023-08-22 at 13:27 +0200, Martijn van Duren wrote:
> On Tue, 2023-08-22 at 10:16 +0000, Gerhard Roth wrote:
> > On Tue, 2023-08-22 at 11:16 +0200, Martijn van Duren wrote:
> > > On Mon, 2023-08-21 at 07:35 +, Gerhard Roth wrote:
> > > > Hi Martijn,
> > > > 
> > > > last November you fixed ber.c so that sequences won't generate
> > > > an uninitialized subelement.
> > > > 
> > > > This revealed another bug in ober_scanf_elements(): it couldn't
> > > > process sequences with an empty list of subelements. The following
> > > > code failed in ober_scanf_elements():
> > > > 
> > > > struct ber_element  *root;
> > > > struct ber_element  *sub;
> > > > 
> > > > if ((root = ober_add_sequence(NULL)) == NULL)
> > > > err(1, "ober_add_sequence() failed");
> > > > 
> > > > errno = 0;
> > > > if (ober_scanf_elements(root, "{e", ))
> > > > err(1, "ober_scanf_elements() failed");
> > > > 
> > > > printf("sub = %p\n", sub);
> > > > 
> > > > 
> > > > The patch below fixes that.
> > > > 
> > > > Gerhard
> > > > 
> > > > 
> > > > Index: lib/libutil/ber.c
> > > > ===
> > > > RCS file: /cvs/src/lib/libutil/ber.c,v
> > > > retrieving revision 1.24
> > > > diff -u -p -u -p -r1.24 ber.c
> > > > --- lib/libutil/ber.c   3 Nov 2022 17:58:10 -   1.24
> > > > +++ lib/libutil/ber.c   21 Aug 2023 07:24:21 -
> > > > @@ -700,7 +700,8 @@ ober_scanf_elements(struct ber_element *
> > > >  
> > > > va_start(ap, fmt);
> > > > while (*fmt) {
> > > > -   if (ber == NULL && *fmt != '$' && *fmt != '}' && *fmt 
> > > > != ')')
> > > > +   if (ber == NULL && *fmt != '$' && *fmt != '}' && *fmt 
> > > > != ')' &&
> > > > +   *fmt != 'e')
> > > > goto fail;
> > > 
> > > I'm not sure about this part. An ober_scanf_elements of "{}e" on your
> > > example above also fails. The 'e' element might not increment the ber
> > > pointer, but I do think it should fail if an expected element is
> > > missing.
> > 
> > You're right. And digging into it, only makes it look worse.
> > 
> > I think, the 'e' format should behave differently.
> > 
> > 1) in case of '{e' it is the first element of a sequence.
> > Empty sequences are allowed and we should not fail but
> > set the argument to NULL instead.
> > 
> > 2) in all other cases, 'e' is the next element in the list.
> > And here we should fail if there are less elements in the
> > list than expected.
> > 
> > Below is an updated diff that fixes the problem by remembering
> > whether the last traversal was via 'be_sub' (fsub == 1) or 'be_next'
> > and then behaves differently. Not very elegant although.
> 
> I'm not sure yet. Do you have a particular usecase for this?
> Usually you would just do something like
> ober_scanf_elements(elm, "e{", seq);
> if (seq->be_sub != NULL) ...

In my use-case the format contained more elements that followed.
I wanted to scan them all with a single ober_scanf_elements().
But I can just as well split that up into two separate call.

So let's forget about this patch.

Many thanks for looking into this!

Gerhard


> 
> > 
> > > 
> > > > switch (*fmt++) {
> > > > case '$':
> > > > @@ -797,7 +798,7 @@ ober_scanf_elements(struct ber_element *
> > > > if (ber->be_encoding != BER_TYPE_SEQUENCE &&
> > > >     ber->be_encoding != BER_TYPE_SET)
> > > > goto fail;
> > > > -   if (ber->be_sub == NULL || level >= _MAX_SEQ-1)
> > > > +   if (level >= _MAX_SEQ-1)
> > > 
> > > This part is OK martijn@
> > > 
> > > > goto fail;
> > > > parent[++level] = ber;
> > > >   

Re: ober_scanf_elements() and empty sequences

[Gerhard Roth]

On Tue, 2023-08-22 at 11:16 +0200, Martijn van Duren wrote:
> On Mon, 2023-08-21 at 07:35 +0000, Gerhard Roth wrote:
> > Hi Martijn,
> > 
> > last November you fixed ber.c so that sequences won't generate
> > an uninitialized subelement.
> > 
> > This revealed another bug in ober_scanf_elements(): it couldn't
> > process sequences with an empty list of subelements. The following
> > code failed in ober_scanf_elements():
> > 
> > struct ber_element  *root;
> > struct ber_element  *sub;
> > 
> > if ((root = ober_add_sequence(NULL)) == NULL)
> > err(1, "ober_add_sequence() failed");
> > 
> > errno = 0;
> > if (ober_scanf_elements(root, "{e", ))
> > err(1, "ober_scanf_elements() failed");
> > 
> > printf("sub = %p\n", sub);
> > 
> > 
> > The patch below fixes that.
> > 
> > Gerhard
> > 
> > 
> > Index: lib/libutil/ber.c
> > ===
> > RCS file: /cvs/src/lib/libutil/ber.c,v
> > retrieving revision 1.24
> > diff -u -p -u -p -r1.24 ber.c
> > --- lib/libutil/ber.c   3 Nov 2022 17:58:10 -   1.24
> > +++ lib/libutil/ber.c   21 Aug 2023 07:24:21 -
> > @@ -700,7 +700,8 @@ ober_scanf_elements(struct ber_element *
> >  
> > va_start(ap, fmt);
> > while (*fmt) {
> > -   if (ber == NULL && *fmt != '$' && *fmt != '}' && *fmt != 
> > ')')
> > +   if (ber == NULL && *fmt != '$' && *fmt != '}' && *fmt != 
> > ')' &&
> > +   *fmt != 'e')
> > goto fail;
> 
> I'm not sure about this part. An ober_scanf_elements of "{}e" on your
> example above also fails. The 'e' element might not increment the ber
> pointer, but I do think it should fail if an expected element is
> missing.

You're right. And digging into it, only makes it look worse.

I think, the 'e' format should behave differently.

1) in case of '{e' it is the first element of a sequence.
Empty sequences are allowed and we should not fail but
set the argument to NULL instead.

2) in all other cases, 'e' is the next element in the list.
And here we should fail if there are less elements in the
list than expected.

Below is an updated diff that fixes the problem by remembering
whether the last traversal was via 'be_sub' (fsub == 1) or 'be_next'
and then behaves differently. Not very elegant although.


> 
> > switch (*fmt++) {
> > case '$':
> > @@ -797,7 +798,7 @@ ober_scanf_elements(struct ber_element *
> > if (ber->be_encoding != BER_TYPE_SEQUENCE &&
> >     ber->be_encoding != BER_TYPE_SET)
> > goto fail;
> > -   if (ber->be_sub == NULL || level >= _MAX_SEQ-1)
> > +   if (level >= _MAX_SEQ-1)
> 
> This part is OK martijn@
> 
> > goto fail;
> > parent[++level] = ber;
> > ber = ber->be_sub;
> > 
> 

Index: lib/libutil/ber.c
===
RCS file: /cvs/src/lib/libutil/ber.c,v
retrieving revision 1.24
diff -u -p -u -p -r1.24 ber.c
--- lib/libutil/ber.c   3 Nov 2022 17:58:10 -   1.24
+++ lib/libutil/ber.c   22 Aug 2023 10:10:43 -
@@ -695,12 +695,14 @@ ober_scanf_elements(struct ber_element *
off_t   *pos;
struct ber_oid  *o;
struct ber_element  *parent[_MAX_SEQ], **e;
+   int  fsub = 0;
 
memset(parent, 0, sizeof(struct ber_element *) * _MAX_SEQ);
 
va_start(ap, fmt);
while (*fmt) {
-   if (ber == NULL && *fmt != '$' && *fmt != '}' && *fmt != ')')
+   if (ber == NULL && *fmt != '$' && *fmt != '}' && *fmt != ')' &&
+   *fmt != 'e')
goto fail;
switch (*fmt++) {
case '$':
@@ -732,6 +734,8 @@ ober_scanf_elements(struct ber_element *
case 'e':
e = va_arg(ap, struct ber_element **);
*e = ber;
+   if (fsub)
+   goto fail;
ret++;
continue;
case 'E':
@@ -797,10 +801,11 @@ ober_scanf_elements(struct ber_

ober_scanf_elements() and empty sequences

[Gerhard Roth]

Hi Martijn,

last November you fixed ber.c so that sequences won't generate
an uninitialized subelement.

This revealed another bug in ober_scanf_elements(): it couldn't
process sequences with an empty list of subelements. The following
code failed in ober_scanf_elements():

struct ber_element  *root;
struct ber_element  *sub;

if ((root = ober_add_sequence(NULL)) == NULL)
err(1, "ober_add_sequence() failed");

errno = 0;
if (ober_scanf_elements(root, "{e", ))
err(1, "ober_scanf_elements() failed");

printf("sub = %p\n", sub);


The patch below fixes that.

Gerhard


Index: lib/libutil/ber.c
===
RCS file: /cvs/src/lib/libutil/ber.c,v
retrieving revision 1.24
diff -u -p -u -p -r1.24 ber.c
--- lib/libutil/ber.c   3 Nov 2022 17:58:10 -   1.24
+++ lib/libutil/ber.c   21 Aug 2023 07:24:21 -
@@ -700,7 +700,8 @@ ober_scanf_elements(struct ber_element *
 
va_start(ap, fmt);
while (*fmt) {
-   if (ber == NULL && *fmt != '$' && *fmt != '}' && *fmt != ')')
+   if (ber == NULL && *fmt != '$' && *fmt != '}' && *fmt != ')' &&
+   *fmt != 'e')
goto fail;
switch (*fmt++) {
case '$':
@@ -797,7 +798,7 @@ ober_scanf_elements(struct ber_element *
if (ber->be_encoding != BER_TYPE_SEQUENCE &&
ber->be_encoding != BER_TYPE_SET)
goto fail;
-   if (ber->be_sub == NULL || level >= _MAX_SEQ-1)
+   if (level >= _MAX_SEQ-1)
goto fail;
parent[++level] = ber;
ber = ber->be_sub;



smime.p7s
Description: S/MIME cryptographic signature

Fix dhcrelay6 on carp

[Gerhard Roth]

This patch fixes dhcrelay on carp. Without it, the AF_LINK entry
(the only one containing the interface index and rdomain of the
carp interface) of carp interfaces was ignored.

When doing the IPV6_JOIN_GROUP, ip6_setmoptions() would see an
zero interface index and picked an arbitrary, "appropriate one"
instead of the carp interface itself.

Similary code is found in the IPv4 version of dhcrelay.


Index: usr.sbin/dhcrelay6/dispatch.c
===
RCS file: /cvs/src/usr.sbin/dhcrelay6/dispatch.c,v
retrieving revision 1.2
diff -u -p -u -p -r1.2 dispatch.c
--- usr.sbin/dhcrelay6/dispatch.c   17 Jan 2021 13:41:24 -  1.2
+++ usr.sbin/dhcrelay6/dispatch.c   13 Jul 2023 13:38:38 -
@@ -168,7 +168,8 @@ setup_iflist(void)
 
/* Skip non ethernet interfaces. */
if (ifi->ifi_type != IFT_ETHER &&
-   ifi->ifi_type != IFT_ENC) {
+   ifi->ifi_type != IFT_ENC &&
+   ifi->ifi_type != IFT_CARP) {
TAILQ_REMOVE(, intf, entry);
free(intf);
continue;



smime.p7s
Description: S/MIME cryptographic signature

iked processes are orphans

[Gerhard Roth]

Hi Tobi,

a recent change to iked.c moved the call to daemon() behind proc_init().
Now iked forks all its children and afterwards daemonizes itself into
background leaving the kids behind orphaned.

The patch below restores the parent/child relationship. With it, the
parent calls daemon() first. And by putting the daemon() call into
proc_init() we make sure that any re-execed child won't call daemon()
again.

Gerhard


Index: sbin/iked/iked.c
===
RCS file: /cvs/src/sbin/iked/iked.c,v
retrieving revision 1.65
diff -u -p -u -p -r1.65 iked.c
--- sbin/iked/iked.c25 Jun 2023 08:07:04 -  1.65
+++ sbin/iked/iked.c28 Jun 2023 08:30:28 -
@@ -203,8 +203,6 @@ main(int argc, char *argv[])
 
setproctitle("parent");
log_procinit("parent");
-   if (!debug && daemon(0, 0) == -1)
-   err(1, "failed to daemonize");
 
event_init();
 
Index: sbin/iked/proc.c
===
RCS file: /cvs/src/sbin/iked/proc.c,v
retrieving revision 1.38
diff -u -p -u -p -r1.38 proc.c
--- sbin/iked/proc.c5 Mar 2023 22:17:22 -   1.38
+++ sbin/iked/proc.c28 Jun 2023 08:30:28 -
@@ -205,6 +205,8 @@ proc_init(struct privsep *ps, struct pri
 
if (proc_id == PROC_PARENT) {
privsep_process = PROC_PARENT;
+   if (!debug && daemon(0, 0) == -1)
+   fatal("failed to daemonize");
proc_setup(ps, procs, nproc);
 
/*



smime.p7s
Description: S/MIME cryptographic signature

Fix possible mem-leak in snmpd/usm.c

[Gerhard Roth]

Rev 1.25 introduced a mem-leak:


Index: usr.sbin/snmpd/usm.c
===
RCS file: /cvs/src/usr.sbin/snmpd/usm.c,v
retrieving revision 1.25
diff -u -p -r1.25 usm.c
--- usr.sbin/snmpd/usm.c20 Dec 2022 20:01:25 -  1.25
+++ usr.sbin/snmpd/usm.c8 May 2023 12:12:16 -
@@ -629,8 +629,10 @@ usm_decrypt(struct snmp_message *msg, st
return NULL;
 
scoped_pdu_len = usm_crypt(msg, privstr, (int)privlen, buf, 0);
-   if (scoped_pdu_len < 0)
+   if (scoped_pdu_len < 0) {
+   free(buf);
return NULL;
+   }
 
bzero(, sizeof(ber));
ober_set_application(, smi_application);



smime.p7s
Description: S/MIME cryptographic signature

Re: DIOCGETRULE is slow for large rulesets (2/3)

[Gerhard Roth]

On Wed, 2023-04-26 at 13:47 +0200, Alexandr Nedvedicky wrote:
> Hello,
> 
> 
> On Wed, Apr 26, 2023 at 11:37:58AM +1000, David Gwynne wrote:
> > >  fail:
> > > -   if (flags & FWRITE)
> > > -   rw_exit_write(_rw);
> > > -   else
> > > -   rw_exit_read(_rw);
> > > +   rw_exit_write(_rw);
> > 
> > i dont think having the open mode flags affect whether you take a shared
> > or exclusive lock makes sense anyway, so im happy with these bits.
> 
>     while updating my diff I've noticed pfclose() needs
>     same change: dropping 'flags & FWRITE' test. This is
>     fixed i new diff below.
> 
> 
> > 
> > int unit = minor(dev);
> > 
> > if (unit & ((1 << CLONE_SHIFT) - 1))
> > return (ENXIO);
> > 
> > this has some ties into the second issue, which is that we shouldn't
> > use the pid as an identifier for state across syscalls like this
> > in the kernel. the reason is that userland could be weird or buggy
> > (or hostile) and fork in between creating a transaction and ending
> > a transaction, but it will still have the fd it from from open(/dev/pf).
> > instead, we should be using the whole dev_t or the minor number,
> > as long as it includes the bits that the cloning infrastructure
> > uses, as the identifier.
> 
>     in new diff I'm using a minor(dev) to identify transaction owner.
> > 
> > i would also check that the process^Wminor number that created a
> > transaction is the same when looking up a transaction in pf_find_trans.
> 
>     the pf_find_trans() is a dead code in diff below as it is
>     not being used there. Just to make sure I got things right
>     so I can update '3/3' diff in stack accordingly. Let's assume
>     snippet below comes from pfioctl() function
> 
> int   
>   
>   
>     
> pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, struct proc *p) {
> ...
> t = pf_find_trans(ticket);
> if (t == NULL)
> return (ENXIO);
> 
> KASSERT(t->pft_unit == minor(dev));
> 
>     is that kind of check in KASSET() something you have on your mind?
>     perhaps I can trade KASSERT() to regular code:
> 
> if (t->pft_unit != minor(dev))
> return (EPERM);
> 
> 
> thank you for pointers to bpf.c updated diff is below.
> 
> regards
> sashan
> 
> 8<---8<---8<--8<
> diff --git a/sys/net/pf_ioctl.c b/sys/net/pf_ioctl.c
> index 1141069dcf6..b9904c545c5 100644
> --- a/sys/net/pf_ioctl.c
> +++ b/sys/net/pf_ioctl.c
> @@ -117,6 +117,11 @@ void    pf_qid_unref(u_int16_t);
>  int pf_states_clr(struct pfioc_state_kill *);
>  int pf_states_get(struct pfioc_states *);
>  
> +struct pf_trans*pf_open_trans(uint32_t);
> +struct pf_trans*pf_find_trans(uint64_t);
> +void    pf_free_trans(struct pf_trans *);
> +void    pf_rollback_trans(struct pf_trans *);
> +
>  struct pf_rule  pf_default_rule, pf_default_rule_new;
>  
>  struct {
> @@ -168,6 +173,8 @@ int  pf_rtlabel_add(struct pf_addr_wrap 
> *);
>  void    pf_rtlabel_remove(struct pf_addr_wrap *);
>  void    pf_rtlabel_copyout(struct pf_addr_wrap *);
>  
> +uint64_t trans_ticket = 1;
> +LIST_HEAD(, pf_trans)  pf_ioctl_trans = LIST_HEAD_INITIALIZER(pf_trans);
>  
>  void
>  pfattach(int num)
> @@ -293,6 +300,28 @@ pfopen(dev_t dev, int flags, int fmt, struct proc *p)
>  int
>  pfclose(dev_t dev, int flags, int fmt, struct proc *p)
>  {
> +   struct pf_trans *w, *s;
> +   LIST_HEAD(, pf_trans)   tmp_list;
> +   uint32_t unit = minor(dev);
> +
> +   if (minor(dev) >= 1)
> +   return (ENXIO);

If you want to use the minor dev-id to release the transaction,
the above two lines should go away.


> +
> +   LIST_INIT(_list);
> +   rw_enter_write(_rw);
> +   LIST_FOREACH_SAFE(w, _ioctl_trans, pft_entry, s) {
> +   if (w->pft_unit == unit) {
> +   LIST_REMOVE(w, pft_entry);
> +   LIST_INSERT_HEAD(_list, w, pft_entry);
> +   }
> +   }
> +   rw_exit_write(_rw);
> +
> +   while ((w = LIST_FIRST(_list)) != NULL) {
> +   LIST_REMOVE(w, pft_entry);
> +   pf_free_trans(w);
> +   }
> +
> return (0);
>  }
>  
> @@ -1142,10 +1171,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int 
> flags, struct proc *p)
> return (EACCES);
> }
>  
> -   if (flags & FWRITE)
> -   rw_enter_write(_rw);
> -   else
> -   rw_enter_read(_rw);
> +   rw_enter_write(_rw);
>  
> switch (cmd) {
>  
> @@ 

Re: DIOCGETRULE is slow for large rulesets (2/3)

[Gerhard Roth]

On Wed, 2023-04-26 at 19:42 +1000, David Gwynne wrote:
> On Wed, Apr 26, 2023 at 07:48:18AM +0000, Gerhard Roth wrote:
> > On Wed, 2023-04-26 at 00:39 +0200, Alexandr Nedvedicky wrote:
> > > Hello,
> > > 
> > > This is the second diff. It introduces a transaction (pf_trans).
> > > It's more or less diff with dead code.
> > > 
> > > It's still worth to note those two chunks in this diff:
> > > 
> > > @@ -1142,10 +1172,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int 
> > > flags, struct proc *p)
> > > return (EACCES);
> > > }
> > > ??
> > > -??if (flags & FWRITE)
> > > -??rw_enter_write(_rw);
> > > -??else
> > > -??rw_enter_read(_rw);
> > > +??rw_enter_write(_rw);
> > > ??
> > > switch (cmd) {
> > > ??
> > > @@ -3022,10 +3049,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int 
> > > flags, struct proc *p)
> > > break;
> > > }
> > > ??fail:
> > > -??if (flags & FWRITE)
> > > -??rw_exit_write(_rw);
> > > -??else
> > > -??rw_exit_read(_rw);
> > > +??rw_exit_write(_rw);
> > > ??
> > > `pfioctl_rw` serializes processes which perform ioctl(2) to pf(4).
> > > I'd like to also this lock to serialize access to table of transactions.
> > > To keep things simple for now I'd like to make every process to perform
> > > ioctl(2) on pf(4) exclusively. I plan to revisit this later when I'll
> > > be pushing operation on pfioctl_rw further down to individual ioctl
> > > operations.
> > > 
> > > the operations pf_open_trans(), pf_find_trans(), pf_rollback_trans()
> > > introduced in this diff are unused. They will be brought alive in
> > > the 3rd diff.
> > > 
> > > thanks and
> > > regards
> > > sashan
> > > 
> > > 8<---8<---8<--8<
> > > diff --git a/sys/net/pf_ioctl.c b/sys/net/pf_ioctl.c
> > > index 7ea22050506..7e4c7d2a2ab 100644
> > > --- a/sys/net/pf_ioctl.c
> > > +++ b/sys/net/pf_ioctl.c
> > > @@ -117,6 +117,11 @@ void?? 
> > > pf_qid_unref(u_int16_t);
> > > ??int pf_states_clr(struct 
> > > pfioc_state_kill *);
> > > ??int pf_states_get(struct 
> > > pfioc_states *);
> > > ??
> > > +struct pf_trans*pf_open_trans(pid_t);
> > > +struct pf_trans*pf_find_trans(uint64_t);
> > > +void?? pf_free_trans(struct pf_trans 
> > > *);
> > > +void?? pf_rollback_trans(struct 
> > > pf_trans *);
> > > +
> > > ??struct pf_rule?? pf_default_rule, pf_default_rule_new;
> > > ??
> > > ??struct {
> > > @@ -168,6 +173,8 @@ int?? 
> > > pf_rtlabel_add(struct pf_addr_wrap *);
> > > ??void?? pf_rtlabel_remove(struct 
> > > pf_addr_wrap *);
> > > ??void?? pf_rtlabel_copyout(struct 
> > > pf_addr_wrap *);
> > > ??
> > > +uint64_t trans_ticket = 1;
> > > +LIST_HEAD(, pf_trans)pf_ioctl_trans = 
> > > LIST_HEAD_INITIALIZER(pf_trans);
> > > ??
> > > ??void
> > > ??pfattach(int num)
> > > @@ -293,6 +300,29 @@ pfopen(dev_t dev, int flags, int fmt, struct proc *p)
> > > ??int
> > > ??pfclose(dev_t dev, int flags, int fmt, struct proc *p)
> > > ??{
> > > +??struct pf_trans *w, *s;
> > > +??LIST_HEAD(, pf_trans)??tmp_list;
> > > +
> > > +??if (minor(dev) >= 1)
> > > +??return (ENXIO);
> > > +
> > > +??if (flags & FWRITE) {
> > > +??LIST_INIT(_list);
> > > +??rw_enter_write(_rw);
> > > +??LIST_FOREACH_SAFE(w, _ioctl_trans, 
> > > pft_entry, s) {

Re: DIOCGETRULE is slow for large rulesets (2/3)

[Gerhard Roth]

On Wed, 2023-04-26 at 00:39 +0200, Alexandr Nedvedicky wrote:
> Hello,
> 
> This is the second diff. It introduces a transaction (pf_trans).
> It's more or less diff with dead code.
> 
> It's still worth to note those two chunks in this diff:
> 
> @@ -1142,10 +1172,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int 
> flags, struct proc *p)
> return (EACCES);
> }
>  
> -   if (flags & FWRITE)
> -   rw_enter_write(_rw);
> -   else
> -   rw_enter_read(_rw);
> +   rw_enter_write(_rw);
>  
> switch (cmd) {
>  
> @@ -3022,10 +3049,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int 
> flags, struct proc *p)
> break;
> }
>  fail:
> -   if (flags & FWRITE)
> -   rw_exit_write(_rw);
> -   else
> -   rw_exit_read(_rw);
> +   rw_exit_write(_rw);
>  
> `pfioctl_rw` serializes processes which perform ioctl(2) to pf(4).
> I'd like to also this lock to serialize access to table of transactions.
> To keep things simple for now I'd like to make every process to perform
> ioctl(2) on pf(4) exclusively. I plan to revisit this later when I'll
> be pushing operation on pfioctl_rw further down to individual ioctl
> operations.
> 
> the operations pf_open_trans(), pf_find_trans(), pf_rollback_trans()
> introduced in this diff are unused. They will be brought alive in
> the 3rd diff.
> 
> thanks and
> regards
> sashan
> 
> 8<---8<---8<--8<
> diff --git a/sys/net/pf_ioctl.c b/sys/net/pf_ioctl.c
> index 7ea22050506..7e4c7d2a2ab 100644
> --- a/sys/net/pf_ioctl.c
> +++ b/sys/net/pf_ioctl.c
> @@ -117,6 +117,11 @@ void    pf_qid_unref(u_int16_t);
>  int pf_states_clr(struct pfioc_state_kill *);
>  int pf_states_get(struct pfioc_states *);
>  
> +struct pf_trans*pf_open_trans(pid_t);
> +struct pf_trans*pf_find_trans(uint64_t);
> +void    pf_free_trans(struct pf_trans *);
> +void    pf_rollback_trans(struct pf_trans *);
> +
>  struct pf_rule  pf_default_rule, pf_default_rule_new;
>  
>  struct {
> @@ -168,6 +173,8 @@ int  pf_rtlabel_add(struct pf_addr_wrap 
> *);
>  void    pf_rtlabel_remove(struct pf_addr_wrap *);
>  void    pf_rtlabel_copyout(struct pf_addr_wrap *);
>  
> +uint64_t trans_ticket = 1;
> +LIST_HEAD(, pf_trans)  pf_ioctl_trans = LIST_HEAD_INITIALIZER(pf_trans);
>  
>  void
>  pfattach(int num)
> @@ -293,6 +300,29 @@ pfopen(dev_t dev, int flags, int fmt, struct proc *p)
>  int
>  pfclose(dev_t dev, int flags, int fmt, struct proc *p)
>  {
> +   struct pf_trans *w, *s;
> +   LIST_HEAD(, pf_trans)   tmp_list;
> +
> +   if (minor(dev) >= 1)
> +   return (ENXIO);
> +
> +   if (flags & FWRITE) {
> +   LIST_INIT(_list);
> +   rw_enter_write(_rw);
> +   LIST_FOREACH_SAFE(w, _ioctl_trans, pft_entry, s) {
> +   if (w->pft_pid == p->p_p->ps_pid) {
> +   LIST_REMOVE(w, pft_entry);
> +   LIST_INSERT_HEAD(_list, w, pft_entry);
> +   }
> +   }
> +   rw_exit_write(_rw);
> +
> +   while ((w = LIST_FIRST(_list)) != NULL) {
> +   LIST_REMOVE(w, pft_entry);
> +   pf_free_trans(w);
> +   }
> +   }
> +
> return (0);
>  }

Kernel close() routines don't work the same way as user-land close() ones do.
pfclose() is called only once when the last reference to /dev/pf goes away.
There is no way you can keep track of your transactions like that.


>  
> @@ -1142,10 +1172,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int 
> flags, struct proc *p)
> return (EACCES);
> }
>  
> -   if (flags & FWRITE)
> -   rw_enter_write(_rw);
> -   else
> -   rw_enter_read(_rw);
> +   rw_enter_write(_rw);
>  
> switch (cmd) {
>  
> @@ -3022,10 +3049,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int 
> flags, struct proc *p)
> break;
> }
>  fail:
> -   if (flags & FWRITE)
> -   rw_exit_write(_rw);
> -   else
> -   rw_exit_read(_rw);
> +   rw_exit_write(_rw);
>  
> return (error);
>  }
> @@ -3244,3 +3268,52 @@ pf_sysctl(void *oldp, size_t *oldlenp, void *newp, 
> size_t newlen)
>  
> return sysctl_rdstruct(oldp, oldlenp, newp, , sizeof(pfs));
>  }
> +
> +struct pf_trans *
> +pf_open_trans(pid_t pid)
> +{
> +   static uint64_t ticket = 1;
> +   struct pf_trans *t;
> +
> +   rw_assert_wrlock(_rw);
> +
> +   t = malloc(sizeof(*t), M_TEMP, M_WAITOK);
> +   memset(t, 0, sizeof(struct pf_trans));
> +   t->pft_pid = pid;
> +   t->pft_ticket = ticket++;

TLS 1.3 ClientHello and Windows 11

[Gerhard Roth]

I stumbled upon a problem that xfreerdp couldn't connect to Windows 11
servers with NLA and TLS 1.3. This can also be reproduced with

# openssl -tls1_3 -connect :3389

Here openssl will fail with a "tlsv1 alert internal error" instead
of blocking in "read R BLOCK".

So I played around with the advertized TLS extensions in our ClientHello
message an found that Windows is pleased if we add a
psk_key_exchange_modes extension.

That aligns with RFC 8446, Sect. 4.2.9. "Pre-Shared Key Extension Modes":

In order to use PSKs, clients MUST also send a
"psk_key_exchange_modes" extension.

...

A client MUST provide a "psk_key_exchange_modes" extension if it
offers a "pre_shared_key" extension.  If clients offer
"pre_shared_key" without a "psk_key_exchange_modes" extension,
servers MUST abort the handshake.


My patch adds this extension depening on the offer of a "key_share"
extension. But maybe this should be done unconditionally?


Gerhard


Index: lib/libssl/ssl_tlsext.c
===
RCS file: /cvs/src/lib/libssl/ssl_tlsext.c,v
retrieving revision 1.131
diff -u -p -r1.131 ssl_tlsext.c
--- lib/libssl/ssl_tlsext.c 26 Nov 2022 16:08:56 -  1.131
+++ lib/libssl/ssl_tlsext.c 27 Mar 2023 08:51:01 -
@@ -1434,6 +1434,8 @@ tlsext_keyshare_client_build(SSL *s, uin
if (!tls_key_share_public(s->s3->hs.key_share, _exchange))
return 0;
 
+   s->s3->hs.tls13.use_psk_dhe_ke = 1;
+
if (!CBB_flush(cbb))
return 0;
 



smime.p7s
Description: S/MIME cryptographic signature

Re: 7.2: snmp mibtree command broken

[Gerhard Roth]

On Tue, 2022-12-20 at 11:34 +0100, Martijn van Duren wrote:
> On Tue, 2022-12-20 at 10:28 +0000, Gerhard Roth wrote:
> > Hi Martijn,
> > 
> > On Tue, 2022-12-20 at 11:10 +0100, Martijn van Duren wrote:
> > > On Tue, 2022-12-20 at 10:21 +0100, Matthias Pitzl wrote:
> > > > Hi,
> > > > 
> > > > Since the release of OpenBSD 7.2, snmp mibtree is broken:
> > > > > root@host:~# snmp mibtree
> > > > > snmp: No securityName specified
> > > > 
> > > > Greetings,
> > > > Matthias
> > > 
> > > Apparently no one has used this one in a long time (including me).
> > > This got broken in snmpc.c r1.37 on 2021/08/11 when changing snmp(1)'s
> > > default to v3.
> > > 
> > > This may not the prettiest solution, but this is the shortest I can come
> > > up with.
> > 
> > no sub-command that hasn't 'snmp_app->usecommonopt' set can have any of
> > the SNMPv2 or SNMPv3 credentials. Sure, currently 'mibtree' is the only
> > one. But for future updates, wouldn't it be better to check
> > 
> > if (!snmp_app->usecommonopt)
> > 
> > instead of
> > 
> > if (strcmp(snmp_app->name, "mibtree") == 0)
> > 
> > Greetings,
> > 
> > Gerhard
> > 
> > 
> You're right, that's better.

ok gerhard@

> 
> Index: snmpc.c
> ===
> RCS file: /cvs/src/usr.bin/snmp/snmpc.c,v
> retrieving revision 1.38
> diff -u -p -r1.38 snmpc.c
> --- snmpc.c 21 Oct 2021 08:17:34 -  1.38
> +++ snmpc.c 20 Dec 2022 10:33:58 -
> @@ -468,7 +468,9 @@ main(int argc, char *argv[])
> argc -= optind;
> argv += optind;
>  
> -   if (version == SNMP_V1 || version == SNMP_V2C) {
> +   if (!snmp_app->usecommonopt) {
> +   /* No SNMP protocol settings */
> +   } else if (version == SNMP_V1 || version == SNMP_V2C) {
> if (community == NULL || community[0] == '\0')
> errx(1, "No community name specified.");
> } else if (version == SNMP_V3) {
> 



smime.p7s
Description: S/MIME cryptographic signature

Re: 7.2: snmp mibtree command broken

[Gerhard Roth]

Hi Martijn,

On Tue, 2022-12-20 at 11:10 +0100, Martijn van Duren wrote:
> On Tue, 2022-12-20 at 10:21 +0100, Matthias Pitzl wrote:
> > Hi,
> > 
> > Since the release of OpenBSD 7.2, snmp mibtree is broken:
> > > root@host:~# snmp mibtree
> > > snmp: No securityName specified
> > 
> > Greetings,
> > Matthias
> 
> Apparently no one has used this one in a long time (including me).
> This got broken in snmpc.c r1.37 on 2021/08/11 when changing snmp(1)'s
> default to v3.
> 
> This may not the prettiest solution, but this is the shortest I can come
> up with.

no sub-command that hasn't 'snmp_app->usecommonopt' set can have any of
the SNMPv2 or SNMPv3 credentials. Sure, currently 'mibtree' is the only
one. But for future updates, wouldn't it be better to check

if (!snmp_app->usecommonopt)

instead of

if (strcmp(snmp_app->name, "mibtree") == 0)

Greetings,

Gerhard

> 
> martijn@
> 
> Index: snmpc.c
> ===
> RCS file: /cvs/src/usr.bin/snmp/snmpc.c,v
> retrieving revision 1.38
> diff -u -p -r1.38 snmpc.c
> --- snmpc.c 21 Oct 2021 08:17:34 -  1.38
> +++ snmpc.c 20 Dec 2022 10:08:29 -
> @@ -468,7 +468,9 @@ main(int argc, char *argv[])
> argc -= optind;
> argv += optind;
>  
> -   if (version == SNMP_V1 || version == SNMP_V2C) {
> +   if (strcmp(snmp_app->name, "mibtree") == 0) {
> +   /* No SNMP protocol settings */
> +   } else if (version == SNMP_V1 || version == SNMP_V2C) {
> if (community == NULL || community[0] == '\0')
> errx(1, "No community name specified.");
> } else if (version == SNMP_V3) {
> 



smime.p7s
Description: S/MIME cryptographic signature

Possible segfault in iked

[Gerhard Roth]

Hi,

since there's a 'sa_free(sa)' followed by a 'continue' a few lines down
from the RB_FOREACH(), we must use RB_FOREACH_SAFE() instead.

Gerhard


Index: sbin/iked/ikev2.c
===
RCS file: /cvs/src/sbin/iked/ikev2.c,v
retrieving revision 1.346
diff -u -p -C6 -u -p -r1.346 ikev2.c
--- sbin/iked/ikev2.c   14 Mar 2022 12:58:55 -  1.346
+++ sbin/iked/ikev2.c   28 May 2022 13:08:29 -
@@ -223,13 +223,13 @@ ikev2_shutdown(struct privsep_proc *p)
 }
 
 int
 ikev2_dispatch_parent(int fd, struct privsep_proc *p, struct imsg *imsg)
 {
struct iked *env = p->p_env;
-   struct iked_sa  *sa;
+   struct iked_sa  *sa, *satmp;
struct iked_policy  *pol, *old;
 
switch (imsg->hdr.type) {
case IMSG_CTL_RESET:
return (config_getreset(env, imsg));
case IMSG_CTL_COUPLE:
@@ -242,13 +242,13 @@ ikev2_dispatch_parent(int fd, struct pri
timer_del(env, >sc_inittmr);
TAILQ_FOREACH(pol, >sc_policies, pol_entry) {
if (policy_generate_ts(pol) == -1)
fatalx("%s: too many traffic selectors", 
__func__);
}
/* Find new policies for dangling SAs */
-   RB_FOREACH(sa, iked_sas, >sc_sas) {
+   RB_FOREACH_SAFE(sa, iked_sas, >sc_sas, satmp) {
if (sa->sa_state != IKEV2_STATE_ESTABLISHED) {
sa_state(env, sa, IKEV2_STATE_CLOSING);
ikev2_ike_sa_setreason(sa, "reload");
sa_free(env, sa);
continue;
}



smime.p7s
Description: S/MIME cryptographic signature

Re: ure(4): add support for RTL8156B

[Gerhard Roth]

On 4/1/22 07:41, Kevin Lo wrote:
> On Fri, Apr 01, 2022 at 12:06:02PM +1000, Jonathan Matthew wrote:
>>
>> On Thu, Mar 31, 2022 at 09:41:09PM +0800, Kevin Lo wrote:
>>> Hi,
>>> 
>>>  
>>> This diff adds preliminary support for RTL8156B to ure(4) and
>>> bug fixes for RTL8153/RTL8156.
>>>
>>> Tested:
>>> ure0 at uhub0 port 12 configuration 1 interface 0 "Realtek USB 
>>> 10/100/1G/2.5G LAN" rev 3.20/31.00 addr 3
>>> ure0: RTL8156B (0x7410), address 00:e0:4c:xx:xx:xx
>>
>> Works OK here:
>>
>> ure0 at uhub0 port 2 configuration 1 interface 0 "Realtek USB 10/100 LAN" 
>> rev 2.10/20.00 addr 2
>> ure0: RTL8152 (0x4c00), address 00:e0:4c:xx:xx:xx
>> rlphy0 at ure0 phy 0: RTL8201E 10/100 PHY, rev. 2
>>
>> Regarding this part:
>>
>>> @@ -1914,7 +2026,7 @@ ure_rxeof(struct usbd_xfer *xfer, void *
>>> total_len -= roundup(pktlen, URE_RX_BUF_ALIGN);
>>> buf += sizeof(rxhdr);
>>>   
>>> -   m = m_devget(buf, pktlen, ETHER_ALIGN);
>>> +   m = m_devget(buf, pktlen - ETHER_CRC_LEN, ETHER_ALIGN);
>>> if (m == NULL) {
>>> DPRINTF(("unable to allocate mbuf for next packet\n"));
>>> ifp->if_ierrors++;
>>
>> We tried this earlier (r1.22 of if_ure.c) and had to back it out because it
>> didn't work on some devices.  Have we worked out what the problem was there?
> 
> First off, thanks for testing!  I recall this problem.  I used speedtest-cli
> to test the connection speed at home, significant drop in download speed with
> r1.22.
> 
> With my latest diff, no regressions seen on so far:
> 
> ure0: RTL8153 (0x5c10), address 00:e0:4c:xx:xx:xx
> rgephy1 at ure0 phy 0: RTL8251 PHY, rev. 0
> 
> ure0: RTL8153B (0x6010), address f4:28:53:xx:xx:xx
> rgephy1 at ure0 phy 0: RTL8251 PHY, rev. 0
> 
> ure0 at uhub0 port 4 configuration 1 interface 0 "Planex USB 10/100/1G/2.5G 
> LAN" rev 2.10/30.00 addr 5
> ure0: RTL8156 (0x7030), address 1c:c0:35:xx:xx:xx
> 


You can add another one to the list that works just fine with your
patch:

ure0 at uhub1 port 1 configuration 1 interface 0 "Realtek USB 10/100/1000 LAN" 
rev 3.00/30.00 addr 2
ure0: RTL8153 (0x5c30), address 00:13:3b:xx:xx:xx
rgephy0 at ure0 phy 0: RTL8251 PHY, rev. 0


Gerhard



smime.p7s
Description: S/MIME cryptographic signature

Re: [patch] urndis: uncomment watchdog

[Gerhard Roth]

On 12/8/21 15:13, Mikhail wrote:

On Wed, Dec 08, 2021 at 02:43:04PM +0100, Gerhard Roth wrote:

Well, the RNDIS device doesn't respond to REMOTE_NDIS_KEEPALIVE_MSG
messages anymore, but now you hope that it'll still process the
REMOTE_NDIS_RESET_MSG we are sending? Sounds like wishful thinking.
I'd say a usbd_reset_port() might be more effective.



BTW: I was wrong about the 5 seconds. In fact its 10 seconds since the
same timeout applies to the reset message.



I think if the device don't ack the keepalive message the driver will
just output an error to the log and return, there should be no second 5
sec timeout:

  748 rval = urndis_ctrl_send(sc, keep, sizeof(*keep));
  749 free(keep, M_TEMP, sizeof *keep);
  750
  751 if (rval != RNDIS_STATUS_SUCCESS) {
  752 printf("%s: keepalive failed\n", DEVNAME(sc));
  753 return rval;
  754 }
  755
  756 if ((hdr = urndis_ctrl_recv(sc)) == NULL) {
  757 printf("%s: unable to get keepalive response\n", 
DEVNAME(sc));
  758 return RNDIS_STATUS_FAILURE;
  759 }



As you see it calls urndis_ctrl_recv() which in turn calls
urndis_ctrl_msg() which in turn calls usbd_do_request().

usbd_do_request() calls usbd_do_request_flags() with a timeout
of USBD_DEFAULT_TIMEOUT (which is 5 seconds). And
usbd_do_request_flags() sets up an xfer with the USBD_SYNCHRONOUS
flag. Hence usbd_transfer() will wait for the completion up to
USBD_DEFAULT_TIMEOUT seconds.

It may be that the transfer fails with USBD_IOERROR. In that case
the I/O completes faster.


  
I don't want to fight for this diff, if you think that it's too naive to

expect proper reset from unresponsive device - that's fine, I'm ready to
drop the diff, but who knows how those devices are engineered and trade
of of not being able to run other watchdogs comparing to possible
network recovery does look reasonable to me.



I don't blame the idea of revitializing urndis_watchdog(). But that
code has been disabled for more than 10 years. And its quite different
for what all the other watchdog routines of USB network interface
drivers do. Maybe the code needs rethinking.


smime.p7s
Description: S/MIME cryptographic signature

Re: [patch] urndis: uncomment watchdog

[Gerhard Roth]

On 12/8/21 14:31, Mikhail wrote:

On Wed, Dec 08, 2021 at 02:10:49PM +0100, Gerhard Roth wrote:

Well, there's only one watchdog thread for all of the
network interfaces. If it is blocked, not other watchdogs
can run.


I don't think this is a big loss. On one side - no other watchdogs can
run for 5 secs, but on other side - watchdog can potentially recover the
network service with automatic reset of urndis device and return network
connectivity.

Isn't it a fair trade of?



Well, the RNDIS device doesn't respond to REMOTE_NDIS_KEEPALIVE_MSG
messages anymore, but now you hope that it'll still process the
REMOTE_NDIS_RESET_MSG we are sending? Sounds like wishful thinking.
I'd say a usbd_reset_port() might be more effective.

BTW: I was wrong about the 5 seconds. In fact its 10 seconds since the
same timeout applies to the reset message.



smime.p7s
Description: S/MIME cryptographic signature

Re: [patch] urndis: uncomment watchdog

[Gerhard Roth]

On 12/8/21 14:08, Mikhail wrote:

On Wed, Dec 08, 2021 at 10:39:15AM +0100, Gerhard Roth wrote:

urndis_watchdog() calls urndis_ctrl_keepalive() which sends an RNDIS
keepalive msg and then waits for the reply with USBD_DEFAULT_TIMEOUT.
That means if the device stopped responding, the if_watchdog_thread
will be blocked for 5 seconds. Not sure if that's a good idea.


Hello, I think this behavior is like it is supposed to work.

What are drawbacks of having this keepalive thread being blocked while
waiting the answer for keepalive? - in case of timeout we will have
error message in the logs and user will be able to handle the problem
manually.



Well, there's only one watchdog thread for all of the
network interfaces. If it is blocked, not other watchdogs
can run.


smime.p7s
Description: S/MIME cryptographic signature

Re: [patch] urndis: uncomment watchdog

[Gerhard Roth]

On 12/8/21 10:26, Mikhail wrote:

On Tue, Nov 30, 2021 at 09:40:35PM +0300, Mikhail wrote:

Currently watchdog functionality for urndis driver is disabled
(commented), I've tested it and it works properly - reset messages are
correctly sent and cmplt packets are received according to RNDIS spec (I
patched the driver to forcedly send reset message and act on it with
device reset every 5 seconds). I suggest to uncomment it.

The hardware is Megafon 4G МM200-1:

urndis0 at uhub3 port 2 configuration 1 interface 0 "Qualcomm MM200-1" rev 
2.00/3.18 addr 5

Tests, comments or objections?


Ping.

Commenting was introduced by mk@ in 61cec9357e4f with the following
note:


At some point we probably want to use the watchdog functionality but the
current code is completely untested so disable it entirely rather than
enabling it this close to release.


I've tested it and it works, maybe there will be a committer who can
enable the watchdog? - the functionality is widely used in other drivers
and seem to be useful in general.



Hi,

urndis_watchdog() calls urndis_ctrl_keepalive() which sends an RNDIS
keepalive msg and then waits for the reply with USBD_DEFAULT_TIMEOUT.
That means if the device stopped responding, the if_watchdog_thread
will be blocked for 5 seconds. Not sure if that's a good idea.

Gerhard


smime.p7s
Description: S/MIME cryptographic signature

Re: urndis0: IOERROR

[Gerhard Roth]

On 11/22/21 10:47, Mikhail wrote:

On Mon, Nov 22, 2021 at 12:32:30PM +0300, Mikhail wrote:

On Mon, Nov 22, 2021 at 09:31:59AM +0100, Gerhard Roth wrote:

On 11/20/21 17:12, Mikhail wrote:

Comparing Windows and OpenBSD tcpdumps I noticed some differences:

1) In REMOTE_NDIS_INITIALIZE_MSG (I patched the kernel to send it before
getting MAC address and with proper minor version) bInterfaceClass on
OpenBSD is set to Unknown (0x), on Windows it's properly set to
Wireless Controller (0xe0).

2) The control transfer stage for this message on OpenBSD is set to
Data, on Windows it is Setup.

3) The answer for the message on Windows is with USBD_STATUS_SUCCESS
(0x), on OpenBSD it's Unknown (0x000d).

4) The answer for the message on Windows contains "Control transfer
stage: Complete" message, on OpenBSD it's set to Status.

Maybe someone can prompt me:

1) Does those differences matter at all?

2) Where to look regarding changing bInterfaceClass for outgoing
messages? I can see it's properly set in urndis driver (line 133 of
if_urndis.c), but not passed anywhere down to USB stack.


You're getting something wrong here. It is the USB function that sets
bInterfaceClass in its device descriptor, not the host.


It's what I see in wireshark - bInterfaceClass is set for outgoing
packets, for windows it is in frame 27, in patched openbsd it is in fram
172.


Oh, I think I understand what's happening  - this line about
bInterfaceClass in the capture is in square brackets, and according to
the docs it is generated by wireshark:

https://www.wireshark.org/docs/wsug_html_chunked/AppMessages.html

Wireshark provides you with additional information generated out of the
plain packet data or it may need to indicate dissection problems.
Messages generated by Wireshark are usually placed in square brackets
(“[]”).

But it is still a difference between windows and openbsd, not sure how
critical it is.



Your OpenBSD pcap contains not "URB_CONTROL in" packets. Don't know 
where they got lost but since you cannot see any data sent by the 
function on the control pipe, it is almost impossible to do any 
debugging here. You only see what the host sends to the function by miss 
the replies.




smime.p7s
Description: S/MIME cryptographic signature

Re: urndis0: IOERROR

[Gerhard Roth]

On 11/20/21 17:12, Mikhail wrote:

Comparing Windows and OpenBSD tcpdumps I noticed some differences:

1) In REMOTE_NDIS_INITIALIZE_MSG (I patched the kernel to send it before
getting MAC address and with proper minor version) bInterfaceClass on
OpenBSD is set to Unknown (0x), on Windows it's properly set to
Wireless Controller (0xe0).

2) The control transfer stage for this message on OpenBSD is set to
Data, on Windows it is Setup.

3) The answer for the message on Windows is with USBD_STATUS_SUCCESS
(0x), on OpenBSD it's Unknown (0x000d).

4) The answer for the message on Windows contains "Control transfer
stage: Complete" message, on OpenBSD it's set to Status.

Maybe someone can prompt me:

1) Does those differences matter at all?

2) Where to look regarding changing bInterfaceClass for outgoing
messages? I can see it's properly set in urndis driver (line 133 of
if_urndis.c), but not passed anywhere down to USB stack.


You're getting something wrong here. It is the USB function that sets 
bInterfaceClass in its device descriptor, not the host.





On Sun, Nov 14, 2021 at 02:39:33PM +0300, Mikhail wrote:

On Sat, Nov 13, 2021 at 08:27:21PM +0300, Mikhail wrote:

Hello, I get aforesaid error when trying to plug in my 4G usb modem, it
works well on another laptop with windows 10.

I enabled debug info, but seem the failure somewhere deeper in usb stack
and I wasn't able to catch it, can someone advice me on further
debugging efforts?


I did some further investigation with wireshark - in attachment two
captures - from windows (modem is working) and from openbsd (not
working).

I also found some differences in behavior of the device attachment and
processing. According to RNDIS specs[1]:

1) MinorVersion of REMOTE_NDIS_INITIALIZE_MSG must be set to 0x
(paragraph 2.2.2), but in our code it's set to 1:

sys/dev/usb/if_urndis.c:
  459 msg->rm_ver_minor = htole32(1);

2) The REMOTE_NDIS_INITIALIZE_MSG message must come first, but in
OpenBSD first message is getting hardware address (same file):

1462 if (urndis_ctrl_query(sc, OID_802_3_PERMANENT_ADDRESS, NULL, 0,
1463 , ) != RNDIS_STATUS_SUCCESS) {
1464 printf(": unable to get hardware address\n");
1465 splx(s);
1466 return;
1467 }

In yota-windows.pcapng the REMOTE_NDIS_INITIALIZE_MSG is in frame 27.

In yota-openbsd.pcapng OID_802_3_PERMANENT_ADDRESS is in fram 290.

Maybe someone with USB protocol understanding can take a look at the
captures and note the problems in device responses?

I also tried latest snapshot, the problem still persist.

[1] - 
https://winprotocoldoc.blob.core.windows.net/productionwindowsarchives/WinArchive/%5BMS-RNDIS%5D.pdf






smime.p7s
Description: S/MIME cryptographic signature

Missing semicolon in snmpd/parse.y

[Gerhard Roth]

Hi,

the rule for 'listen_udptcp' is missing a semicolon at its end.

I have no idea what yacc does to the following 'port' rule without
that semicolon.

Gerhard


Index: usr.sbin/snmpd/parse.y
===
RCS file: /cvs/src/usr.sbin/snmpd/parse.y,v
retrieving revision 1.70
diff -u -p -u -p -r1.70 parse.y
--- usr.sbin/snmpd/parse.y  15 Oct 2021 15:01:29 -  1.70
+++ usr.sbin/snmpd/parse.y  20 Oct 2021 11:45:29 -
@@ -350,6 +350,7 @@ listen_udptcp   : listenproto STRING port 
free($2);
free($3);
}
+   ;
 
 port   : /* empty */   {
$$ = NULL;


smime.p7s
Description: S/MIME cryptographic signature

Re: date -j and seconds since the Epoch

[Gerhard Roth]

Hello Ingo,

thanks for looking into this.

On 8/6/21 8:13 PM, Ingo Schwarze wrote:

Hi Gerhard and Bryan,

Gerhard Roth wrote on Mon, Aug 02, 2021 at 10:36:05AM +0200:


Bryan Vyhmeister found a strange behavior in date(1):

# date -f %s -j 1627519989
Thu Jul 29 01:53:09 PDT 2021
# date -u -f %s -j 1627519989
Thu Jul 29 00:53:09 UTC 2021

Looks like PDT is GMT-1, which of course is wrong.

The problem arises from the -f option. The argument of date(1) is passed
to strptime(3). Normally, this will return a broken down time in the
local timezone.


This claim confused me somewhat at first.  I think a more accurate
statement would be that strptime(3) does not use the TZ at all.
It merely parses the string and fills the fields in struct tm.
The question whether the caller had any particular time zone in
mind does not even arise here.

Since date(1) says:

   ENVIRONMENT
  TZ   The time zone to use when parsing or displaying dates.  [...]

the command

$ date -f %H:%M -j 9:00

is correct to essentially just echo "09:00" back at you
because date(1) is specified to use the same time zone
for parsing and printing.


But the '%s' format makes an exception and returns a
date in UTC.


That is indeed true.

So for %s the time zone used for parsing is necessarily different,
while the time zone used for printing is still the $TZ specified
by the user (or the /etc/localtime specified by the sysadmin).

So i think your approach of using timegm(3) for %s and mktime(3)
otherwise is essentially correct.

However, a format string can contain more characters than just
a single conversion specification.  For example, somebody might
wish to parse an input file containing a line

   SSE=1627519989

and legitimately say

   $ date -f SSE=%s -j $(grep SSE= input_file)

which still yields the wrong result even with your patch.


You're right. I was thinking about this too, but couldn't come up with a 
sensible example of how to combine '%s' with something else.


Doing a strstr(3) instead of strcmp(3) is surely the better solution.



Even worse, what are the admittedly weird commands

   $ date -f %s:%H -j 1627519989:15
   $ date -f %H:%s -j 15:1627519989
   $ date -f %H:%s:%m -j 15:1627519989:03

supposed to do?  Apparently, data from later conversions is supposed
to override data from earlier ones, so should the last conversion
that is related to the the time zone win, i.e. %s:%H use mktime(3)
but %H:%s and %H:%s:%m gmtime(3)?  I would argue that is excessive
complexity for little benefit, if any.

One might also argue that %s:%H 1627519989:09 is more usefully
interpreted as "9 o'clock UTC on the day containing 1627519989"
than "9 o'clock in the local TZ on the day containing 1627519989".
In addition to using a consistent input time zone, the former also
avoids fun with crossing the date line that the latter might cause.
The former is also easier to implement, see the patch below.

What do people think?


ok gerhard@




The patch below isn't very beautiful, but fixes the problem:

# date -f %s -j 1627519989
Wed Jul 28 17:53:09 PDT 2021


P.S.
Your patch was mangled (one missing blank line and spurious spaces
inserted before tabs on some lines).  Lately, it is becoming annoying
that even very experienced developers no longer seem able to reliably
send email containing patches that actually apply...  :-(


Sorry about that. I will write "I shouldn't use thunderbird to submit 
patches" a hundred times ;)


Gerhard





Index: date.c
===
RCS file: /cvs/src/bin/date/date.c,v
retrieving revision 1.56
diff -u -p -r1.56 date.c
--- date.c  8 Aug 2019 02:17:51 -   1.56
+++ date.c  6 Aug 2021 17:48:01 -
@@ -219,7 +219,11 @@ setthetime(char *p, const char *pformat)
}
  
  	/* convert broken-down time to UTC clock time */

-   if ((tval = mktime(lt)) == -1)
+   if (pformat != NULL && strstr(pformat, "%s") != NULL)
+   tval = timegm(lt);
+   else
+   tval = mktime(lt);
+   if (tval == -1)
errx(1, "specified date is outside allowed range");
  
  	if (jflag)

date -j and seconds since the Epoch

[Gerhard Roth]

Hi,

Bryan Vyhmeister found a strange behavior in date(1):

# date -f %s -j 1627519989
Thu Jul 29 01:53:09 PDT 2021
# date -u -f %s -j 1627519989
Thu Jul 29 00:53:09 UTC 2021

Looks like PDT is GMT-1, which of course is wrong.

The problem arises from the -f option. The argument of date(1) is passed
to strptime(3). Normally, this will return a broken down time in the
local timezone. But the '%s' format makes an exception and returns a
date in UTC.

The patch below isn't very beautiful, but fixes the problem:

# date -f %s -j 1627519989
Wed Jul 28 17:53:09 PDT 2021


Gerhard


Index: bin/date/date.c
===
RCS file: /cvs/src/bin/date/date.c,v
retrieving revision 1.56
diff -u -p -r1.56 date.c
--- bin/date/date.c 8 Aug 2019 02:17:51 -   1.56
+++ bin/date/date.c 2 Aug 2021 07:56:15 -
@@ -219,7 +219,11 @@ setthetime(char *p, const char *pformat)
}
/* convert broken-down time to UTC clock time */
-   if ((tval = mktime(lt)) == -1)
+   if (pformat && strcmp(pformat, "%s") == 0)
+   tval = timegm(lt);
+   else
+   tval = mktime(lt);
+   if (tval == -1)
errx(1, "specified date is outside allowed range");
if (jflag)

Re: Change umb(4) devclass from DV_DULL to DV_IFNET

[Gerhard Roth]

On 4/20/21 7:28 PM, Patrick Wildt wrote:

Am Mon, Apr 19, 2021 at 10:25:39AM +0200 schrieb Tilo Stritzky:

On 10/04/21 22:56  Tilo Stritzky wrote:

umb interfaces advertise themselves as generic devices.
Network makes a lot more sense, I think.
tested on amd64.


Having seen no response on this one, I'ld like to expand a little
further.

This value defines how a device is identified in hotplug events,
eventually showing up in userland as argv to /etc/hotplug/attach.
It doesn't seem to be used by the kernel itself.

Currently umb(4) mobile broadband interfaces identify as ``generic''.
This is not exactly wrong, but there is a ``network interface'' class
which is a much tighter match. All other USB network interfaces set
it to DV_IFNET.

The change lets me group umb related stuff together with other
hotplugged network devices in /etc/hotplug/attach.

Alas, this might break some existing hotplugd setups.

tilo


That does indeed make sense.  umb(4) is some kind of a network device,
and especially in hotplug I think it makes a lot more sense there as
well.  So, I'm in favour.

Objections?  ok?


ok gerhard@




Index: if_umb.c
===
RCS file: /cvs/src/sys/dev/usb/if_umb.c,v
retrieving revision 1.43
diff -u -p -r1.43 if_umb.c
--- if_umb.c1 Apr 2021 08:39:52 -   1.43
+++ if_umb.c10 Apr 2021 20:14:59 -
@@ -212,7 +212,7 @@ uint8_t  umb_uuid_qmi_mbim[] = MBIM_UUI
  uint32_t   umb_session_id = 0;

  struct cfdriver umb_cd = {
-   NULL, "umb", DV_DULL
+   NULL, "umb", DV_IFNET
  };

  const struct cfattach umb_ca = {




umb0 at uhub0 port 1 configuration 1 interface 0 "MediaTek Inc Product" rev 
2.00/3.00 addr 2
umsm0 at uhub0 port 1 configuration 1 interface 2 "MediaTek Inc Product" rev 
2.00/3.00 addr 2
ucom0 at umsm0
umsm1 at uhub0 port 1 configuration 1 interface 3 "MediaTek Inc Product" rev 
2.00/3.00 addr 2
ucom1 at umsm1
umsm2 at uhub0 port 1 configuration 1 interface 4 "MediaTek Inc Product" rev 
2.00/3.00 addr 2
ucom2 at umsm2
umsm3 at uhub0 port 1 configuration 1 interface 5 "MediaTek Inc Product" rev 
2.00/3.00 addr 2
ucom3 at umsm3
umass0 at uhub0 port 1 configuration 1 interface 6 "MediaTek Inc Product" rev 
2.00/3.00 addr 2
umass0: using SCSI over Bulk-Only
scsibus2 at umass0: 2 targets, initiator 0
sd1 at scsibus2 targ 1 lun 0:  removable

Re: Huawei ME906s-158 LTE, cdce(4) vs umb(4)

[Gerhard Roth]

On 3/28/21 3:16 PM, Stuart Henderson wrote:

On 2021/03/28 13:40, Patrick Wildt wrote:

Am Sun, Mar 28, 2021 at 10:53:53AM +0100 schrieb Stuart Henderson:

On 2021/03/25 00:14, Stuart Henderson wrote:

On 2021/03/25 00:30, Patrick Wildt wrote:

Without having looked at anything, it might be worth looking at the most
recent mail in this thread:

'Re: [PATCH] umb(4) fix for X20 (DW5821e) in Dell Latitude 7300'



oh, usb runs through all drivers looking for a VID/PID match before
then running through all looking for a class match? I didn't realise
(thought it would try driver-by-driver with first VID/PID then class)
but that does make sense.

Updated diff below adding my pid/vid and tweaking some comments. My card
attaches to umb with this. I've only added it commented-out to the manual
for now until I see it actually pass traffic.

So this fixes Bryan's, at least improves mine (will look for another
sim / sim-adapter tomorrow), and seems targetted enough to not risk
fallout with existing working devices.

I think this makes sense to commit. OK with me if you'd like to commit
it Gerhard (I think it was your diff originally).


So this isn't enough for the Huawei yet but it doesn't make things
any worse, and helps for DW5821e.

If Gerhard isn't around, can I commit this bit so I'm not wrangling
things which should be separate commits? OK?


Yes, please do.  ok patrick@


Thanks, done.

Since it looks like we're going to need additional quirks to get Huawei
to work and having three separate vid/pid tables seems silly, here's a
diff to change to using a single pid/vid table covering the matches and
the existing fccauth quirk.

Tested with EM7455 (fcc auth required; works as before) and Huawei
(attaches to the correct config descriptor; packet transfer not working
but this is not unexpected).


Hello Stuart,

this is fine with me. ok gerhard@





Index: if_umb.c
===
RCS file: /cvs/src/sys/dev/usb/if_umb.c,v
retrieving revision 1.38
diff -u -p -r1.38 if_umb.c
--- if_umb.c28 Mar 2021 12:08:58 -  1.38
+++ if_umb.c28 Mar 2021 13:10:56 -
@@ -224,34 +224,34 @@ const struct cfattach umb_ca = {
  
  int umb_delay = 4000;
  
-/*

- * Normally, MBIM devices are detected by their interface class and subclass.
- * But for some models that have multiple configurations, it is better to
- * match by vendor and product id so that we can select the desired
- * configuration ourselves, e.g. to override a class-based match to another
- * driver.
- *
- * OTOH, some devices identify themselves as an MBIM device but fail to speak
- * the MBIM protocol.
- */
-struct umb_products {
+struct umb_quirk {
struct usb_devno dev;
-   int  confno;
+   u_int32_tumb_flags;
+   int  umb_confno;
+   int  umb_match;
  };
-const struct umb_products umb_devs[] = {
-   { { USB_VENDOR_DELL, USB_PRODUCT_DELL_DW5821E }, 2 },
-   { { USB_VENDOR_HUAWEI, USB_PRODUCT_HUAWEI_ME906S }, 3 },
+const struct umb_quirk umb_quirks[] = {
+   { { USB_VENDOR_DELL, USB_PRODUCT_DELL_DW5821E },
+ 0,
+ 2,
+ UMATCH_VENDOR_PRODUCT
+   },
+
+   { { USB_VENDOR_HUAWEI, USB_PRODUCT_HUAWEI_ME906S },
+ 0,
+ 3,
+ UMATCH_VENDOR_PRODUCT
+   },
+
+   { { USB_VENDOR_SIERRA, USB_PRODUCT_SIERRA_EM7455 },
+ UMBFLG_FCC_AUTH_REQUIRED,
+ 0,
+ 0
+   },
  };
  
  #define umb_lookup(vid, pid)		\

-   ((const struct umb_products *)usb_lookup(umb_devs, vid, pid))
-
-/*
- * These devices require an "FCC Authentication" command.
- */
-const struct usb_devno umb_fccauth_devs[] = {
-   { USB_VENDOR_SIERRA, USB_PRODUCT_SIERRA_EM7455 },
-};
+   ((const struct umb_quirk *)usb_lookup(umb_quirks, vid, pid))
  
  uint8_t umb_qmi_alloc_cid[] = {

0x01,
@@ -283,10 +283,12 @@ int
  umb_match(struct device *parent, void *match, void *aux)
  {
struct usb_attach_arg *uaa = aux;
+   const struct umb_quirk *quirk;
usb_interface_descriptor_t *id;
  
-	if (umb_lookup(uaa->vendor, uaa->product) != NULL)

-   return UMATCH_VENDOR_PRODUCT;
+   quirk = umb_lookup(uaa->vendor, uaa->product);
+   if (quirk != NULL && quirk->umb_match)
+   return (quirk->umb_match);
if (!uaa->iface)
return UMATCH_NONE;
if ((id = usbd_get_interface_descriptor(uaa->iface)) == NULL)
@@ -317,6 +319,7 @@ umb_attach(struct device *parent, struct
  {
struct umb_softc *sc = (struct umb_softc *)self;
struct usb_attach_arg *uaa = aux;
+   const struct umb_quirk *quirk;
usbd_status status;
struct usbd_desc_iter iter;
const usb_descriptor_t *desc;
@@ -339,13 +342,27 @@ umb_attach(struct device *parent, struct
sc->sc_ctrl_ifaceno = uaa->ifaceno;
ml_init(>sc_tx_ml);
  
+	quirk = 

Fix ix(4) link status

[Gerhard Roth]

ix(4) relies on link-state change interrupts the update the link state
via if_link_state_change(). However, after ixgbe_stop() all interrupts
for the device are disabled and there won't be any IXGBE_EICR_LSC
interrupt.

Simple solution: manually update link state after ixgbe_stop().

Gerhard


--- sys/dev/pci/if_ix.c 2020/07/18 07:18:22 1.172
+++ sys/dev/pci/if_ix.c 2020/10/12 09:13:59
@@ -1606,6 +1606,8 @@ ixgbe_stop(void *arg)
/* Should we really clear all structures on stop? */
ixgbe_free_transmit_structures(sc);
ixgbe_free_receive_structures(sc);
+
+   ixgbe_update_link_status(sc);
 }
 
 

Bad definition of SIOCG80211JOIN

[Gerhard Roth]

The current definition of SIOCG80211JOIN uses 256 for the command,
but the _IOC() macro only allows 8 bit for the command value.
Using 256 would set the lowermost bit of the ioctl group.
Fortunately, 'i' (0x69) already has the lowermost bit set. Otherwise
SIOCG80211JOIN would never reach ifioctl().

The patch below is compatible with the current definition and
results in no binary change, it just reflects reality better.

Gerhard


--- sys/net80211/ieee80211_ioctl.h  2020/04/29 13:13:30 1.40
+++ sys/net80211/ieee80211_ioctl.h  2020/10/05 11:24:06
@@ -275,11 +275,11 @@ struct ieee80211_keyrun {
 
 #define SIOCS80211SCAN  _IOW('i', 210, struct ifreq)
 
 #defineSIOCG80211JOINALL   _IOWR('i', 218, struct 
ieee80211_joinreq_all)
 #defineSIOCS80211JOIN  _IOWR('i', 255, struct ifreq)
-#defineSIOCG80211JOIN  _IOWR('i', 256, struct ifreq)
+#defineSIOCG80211JOIN  _IOWR('i', 0, struct ifreq)
 
 /* join is pointed at by ifr.ifr_data */
 struct ieee80211_join {
u_int8_ti_len;  /* length of i_nwid */
u_int8_ti_nwid[IEEE80211_NWID_LEN];

Re: usbd_abort_pipe(); usbd_close_pipe; dance

[Gerhard Roth]

Hi Marcus,

On 2020-07-31 11:22, Marcus Glocker wrote:

Maybe I'm missing something here.

But is there any specific reason why the most of our USB drivers are
calling usbd_abort_pipe() right before usbd_close_pipe()?  Since
usbd_close_pipe() already will call usbd_abort_pipe() if the pipe isn't
empty, as documented in the man page:

DESCRIPTION
  The usbd_abort_pipe() function aborts any transfers queued on pipe.

  The usbd_close_pipe() function aborts any transfers queued on pipe
  then deletes it.

In case this happened because of an inherited copy/paste chain, can we
nuke the superfluous usbd_abort_pipe() calls?


I was asking myself the same question before ;)

Apparently, the call to usbd_abort_pipe() inside of usbd_close_pipe()
wasn't there right from the start. It was added by mpi@ with rev 1.57 of
usbdi.c abort 7 years ago. Hence drivers written before that needed the
the usbd_abort_pipe(). But that is no longer the case.

ok gerhard@







Index: if_atu.c
===
RCS file: /cvs/src/sys/dev/usb/if_atu.c,v
retrieving revision 1.131
diff -u -p -u -p -r1.131 if_atu.c
--- if_atu.c10 Jul 2020 13:26:40 -  1.131
+++ if_atu.c31 Jul 2020 08:26:24 -
@@ -2252,7 +2252,6 @@ atu_stop(struct ifnet *ifp, int disable)
  
  	/* Stop transfers. */

if (sc->atu_ep[ATU_ENDPT_RX] != NULL) {
-   usbd_abort_pipe(sc->atu_ep[ATU_ENDPT_RX]);
err = usbd_close_pipe(sc->atu_ep[ATU_ENDPT_RX]);
if (err) {
DPRINTF(("%s: close rx pipe failed: %s\n",
@@ -2262,7 +2261,6 @@ atu_stop(struct ifnet *ifp, int disable)
}
  
  	if (sc->atu_ep[ATU_ENDPT_TX] != NULL) {

-   usbd_abort_pipe(sc->atu_ep[ATU_ENDPT_TX]);
err = usbd_close_pipe(sc->atu_ep[ATU_ENDPT_TX]);
if (err) {
DPRINTF(("%s: close tx pipe failed: %s\n",
Index: if_aue.c
===
RCS file: /cvs/src/sys/dev/usb/if_aue.c,v
retrieving revision 1.110
diff -u -p -u -p -r1.110 if_aue.c
--- if_aue.c10 Jul 2020 13:26:40 -  1.110
+++ if_aue.c31 Jul 2020 08:26:25 -
@@ -1518,7 +1518,6 @@ aue_stop(struct aue_softc *sc)
  
  	/* Stop transfers. */

if (sc->aue_ep[AUE_ENDPT_RX] != NULL) {
-   usbd_abort_pipe(sc->aue_ep[AUE_ENDPT_RX]);
err = usbd_close_pipe(sc->aue_ep[AUE_ENDPT_RX]);
if (err) {
printf("%s: close rx pipe failed: %s\n",
@@ -1528,7 +1527,6 @@ aue_stop(struct aue_softc *sc)
}
  
  	if (sc->aue_ep[AUE_ENDPT_TX] != NULL) {

-   usbd_abort_pipe(sc->aue_ep[AUE_ENDPT_TX]);
err = usbd_close_pipe(sc->aue_ep[AUE_ENDPT_TX]);
if (err) {
printf("%s: close tx pipe failed: %s\n",
@@ -1538,7 +1536,6 @@ aue_stop(struct aue_softc *sc)
}
  
  	if (sc->aue_ep[AUE_ENDPT_INTR] != NULL) {

-   usbd_abort_pipe(sc->aue_ep[AUE_ENDPT_INTR]);
err = usbd_close_pipe(sc->aue_ep[AUE_ENDPT_INTR]);
if (err) {
printf("%s: close intr pipe failed: %s\n",
Index: if_axe.c
===
RCS file: /cvs/src/sys/dev/usb/if_axe.c,v
retrieving revision 1.141
diff -u -p -u -p -r1.141 if_axe.c
--- if_axe.c10 Jul 2020 13:26:40 -  1.141
+++ if_axe.c31 Jul 2020 08:26:25 -
@@ -1473,7 +1473,6 @@ axe_stop(struct axe_softc *sc)
  
  	/* Stop transfers. */

if (sc->axe_ep[AXE_ENDPT_RX] != NULL) {
-   usbd_abort_pipe(sc->axe_ep[AXE_ENDPT_RX]);
err = usbd_close_pipe(sc->axe_ep[AXE_ENDPT_RX]);
if (err) {
printf("axe%d: close rx pipe failed: %s\n",
@@ -1483,7 +1482,6 @@ axe_stop(struct axe_softc *sc)
}
  
  	if (sc->axe_ep[AXE_ENDPT_TX] != NULL) {

-   usbd_abort_pipe(sc->axe_ep[AXE_ENDPT_TX]);
err = usbd_close_pipe(sc->axe_ep[AXE_ENDPT_TX]);
if (err) {
printf("axe%d: close tx pipe failed: %s\n",
@@ -1493,7 +1491,6 @@ axe_stop(struct axe_softc *sc)
}
  
  	if (sc->axe_ep[AXE_ENDPT_INTR] != NULL) {

-   usbd_abort_pipe(sc->axe_ep[AXE_ENDPT_INTR]);
err = usbd_close_pipe(sc->axe_ep[AXE_ENDPT_INTR]);
if (err) {
printf("axe%d: close intr pipe failed: %s\n",
Index: if_axen.c
===
RCS file: /cvs/src/sys/dev/usb/if_axen.c,v
retrieving revision 1.29
diff -u -p -u -p -r1.29 if_axen.c
--- if_axen.c   10 Jul 2020 13:26:40 -  1.29
+++ if_axen.c   31 Jul 2020 08:26:25 -
@@ -1426,7 +1426,6 @@ axen_stop(struct axen_softc *sc)
  
  	/* Stop transfers. */

if (sc->axen_ep[AXEN_ENDPT_RX] != NULL) {
-   

Avoid realloc

[Gerhard Roth]

Recently a stat(2) call was added to load_server_config() of ssh to
avoid reallocs. However, a buffer of 'st_size' length might be too
short to hold the null terminator of the string.

Add one more byte to the size, if it is sure that we can't overflow.


Gerhard



Index: usr.bin/ssh/servconf.c
===
RCS file: /cvs/src/usr.bin/ssh/servconf.c,v
retrieving revision 1.367
diff -u -p -u -p -r1.367 servconf.c
--- usr.bin/ssh/servconf.c  5 Jul 2020 23:59:45 -   1.367
+++ usr.bin/ssh/servconf.c  17 Jul 2020 09:27:08 -
@@ -2339,7 +2339,8 @@ load_server_config(const char *filename,
sshbuf_reset(conf);
/* grow buffer, so realloc is avoided for large config files */
if (fstat(fileno(f), ) == 0 && st.st_size > 0 &&
-(r = sshbuf_allocate(conf, st.st_size)) != 0)
+   st.st_size < LONG_MAX &&
+   (r = sshbuf_allocate(conf, st.st_size + 1)) != 0)
fatal("%s: allocate failed: %s", __func__, ssh_err(r));
while (getline(, , f) != -1) {
lineno++;

tmpfs bug in reclaim

[Gerhard Roth]

tmpfs_reclaim() has to make sure that the VFS cache has no more
locks held for the vnode. Else vclean() could panic because v_holdcnt
is non-zero.

I know that tmpfs is disabled by default, but it would be nice
to have this fix in the code base anyway.

Gerhard


Index: sys/tmpfs/tmpfs_vnops.c
===
RCS file: /cvs/src/sys/tmpfs/tmpfs_vnops.c,v
retrieving revision 1.42
diff -u -p -u -p -r1.42 tmpfs_vnops.c
--- sys/tmpfs/tmpfs_vnops.c 11 Jun 2020 09:18:43 -  1.42
+++ sys/tmpfs/tmpfs_vnops.c 13 Jul 2020 09:48:37 -
@@ -1080,6 +1080,8 @@ tmpfs_reclaim(void *v)
racing = TMPFS_NODE_RECLAIMING(node);
rw_exit_write(>tn_nlock);
 
+   cache_purge(vp);
+
/*
 * If inode is not referenced, i.e. no links, then destroy it.
 * Note: if racing - inode is about to get a new vnode, leave it.

Re: IPv6 Support for umb(4)

[Gerhard Roth]

On 4/30/20 11:07 PM, Stuart Henderson wrote:

On 2020/04/30 20:32, Gerhard Roth wrote:

Hi Theo,

is umb really working that differently for a P2P interface? I think it is
very similar to ppp(4) and IPv6. The standard way is to obtain the IP
address via PPP protocol. Just like this, umb(4) obtains the IP address via
MBIM protocol.


PPP is quite different, it only negotiates a link-local v6 address. If you
want a routable address you must use slaac, dhcpv6, or static config.


That's what is implemented with this and former umb(4) patches. With the
current patch you can just do "ifconfig umb0 inet eui64" or "ifconfig umb0
-inet6" anytime, whether the interface is up or not.

But then there seem to be strange ISPs in Japan as detected by job@. They
don't provide any IPv6 address via MBIM protocol but rather use the standard
IPv6 SLAAC protocol. It strange; just as if ppp(4) would need DHCP to obtain
its IPv4 address. But still, it seems to exist and users can still work with
umb(4) if they do "ifconfig umb0 inet6 autoconf".


What happens if an ISP provides a v6 address via MBIM-config and the interface
is set to "inet6 autoconf", does it still work OK? That's what most people
will try. Since we disabled IPv6 by default, IPv6 users already know how to
use "inet6 autoconf".


I just can't tell. All providers here in Germany will give me an IPv6 
address via MBIM. There's no way I could test this case. Especially not 
in times of Corona.


Gerhard





I also feel noone is going to read the manual page, find this piece
of text, and understand it.  Honestly, I don't understand this piece
of text.  I'm not going to set the AUTOCONF6 flag.  How does one even
set it?

ifconfig: AUTOCONF6: bad value

Of course not, but I am ironically trying to show this documentation
chunk doesn't help at all.  People can't act upon it properly.

I still argue umb's inet6 should work absolutely as much like regular
interfaces, or it is useless.  People are not going to treat this
interface differently and then gain successful inet6.  If inet6 can't
work naturally and easily, but instead is a special snowflake, that
is just plain dumb.


Unfortunately mobile data is a special snowflake because you get some
providers who say "to conserve licenses with the network vendor you can
have either an IPv4 address or an IPv6 address but not both at the same
time" so you need a way to do something which isn't required on normal
ethernet.


+.Pp
+To use IPv6, configure a link-local address.
+If the device is able to connect to the ISP's network but doesn't
+show an IPv6 address, setting the
+.Sy AUTOCONF6
+flag on the interface before bringing it up may help.
+.Ed


Showing the actual command to type will help a lot here.

On 2020/04/30 20:52, Gerhard Roth wrote:

It it too much to expect users to read the ifconfig man page?


Printed, it is 28 pages of A4.

Compare with the wifi drivers, you have to look at ifconfig(8) if
you want all the details, but EXAMPLES in iwm(4) (and I think all the other
drivers) is enough for a quick bare-bones config. That seems a reasonable
model.

Re: IPv6 Support for umb(4)

[Gerhard Roth]

On 4/30/20 8:04 PM, Theo de Raadt wrote:

I also feel noone is going to read the manual page, find this piece
of text, and understand it.  Honestly, I don't understand this piece
of text.  I'm not going to set the AUTOCONF6 flag.  How does one even
set it?

ifconfig: AUTOCONF6: bad value

Of course not, but I am ironically trying to show this documentation
chunk doesn't help at all.  People can't act upon it properly.


Well then, we should fix the ifconfig(8) man page first, because it 
talks about the AUTOCONF6 flag, too.


It it too much to expect users to read the ifconfig man page?

Re: IPv6 Support for umb(4)

[Gerhard Roth]

Hi Theo,

is umb really working that differently for a P2P interface? I think it 
is very similar to ppp(4) and IPv6. The standard way is to obtain the IP 
address via PPP protocol. Just like this, umb(4) obtains the IP address 
via MBIM protocol.


That's what is implemented with this and former umb(4) patches. With the 
current patch you can just do "ifconfig umb0 inet eui64" or "ifconfig 
umb0 -inet6" anytime, whether the interface is up or not.


But then there seem to be strange ISPs in Japan as detected by job@. 
They don't provide any IPv6 address via MBIM protocol but rather use the 
standard IPv6 SLAAC protocol. It strange; just as if ppp(4) would need 
DHCP to obtain its IPv4 address. But still, it seems to exist and users 
can still work with umb(4) if they do "ifconfig umb0 inet6 autoconf".


I don't see any way, how this different behavior could be "unified" in 
umb(4). It's a problem with the ISP, not with the driver.



Gerhard


On 4/30/20 8:04 PM, Theo de Raadt wrote:

I don't know the answers.

But if umb works differently it will suck.

I also feel noone is going to read the manual page, find this piece
of text, and understand it.  Honestly, I don't understand this piece
of text.  I'm not going to set the AUTOCONF6 flag.  How does one even
set it?

ifconfig: AUTOCONF6: bad value

Of course not, but I am ironically trying to show this documentation
chunk doesn't help at all.  People can't act upon it properly.

I still argue umb's inet6 should work absolutely as much like regular
interfaces, or it is useless.  People are not going to treat this
interface differently and then gain successful inet6.  If inet6 can't
work naturally and easily, but instead is a special snowflake, that
is just plain dumb.

Gerhard Roth  wrote:


On 4/30/20 4:03 PM, Theo de Raadt wrote:

Is that still the true behaviour?  I think it isn't, the "before up"
aspect is gone isn't it?


That's right for IP configuration via MBIM and I deleted the "before
up" from the first sentence.

But wasn't sure for the SLAAC case. Will autoconf work if I set the
flag after the interface is up? Should I delete the "before up" here,
too?

Gerhard




+.Pp
+To use IPv6, configure a link-local address.
+If the device is able to connect to the ISP's network but doesn't
+show an IPv6 address, setting the
+.Sy AUTOCONF6
+flag on the interface before bringing it up may help.
+.Ed

Re: IPv6 Support for umb(4)

[Gerhard Roth]

On 4/30/20 4:03 PM, Theo de Raadt wrote:

Is that still the true behaviour?  I think it isn't, the "before up"
aspect is gone isn't it?


That's right for IP configuration via MBIM and I deleted the "before up" 
from the first sentence.


But wasn't sure for the SLAAC case. Will autoconf work if I set the flag 
after the interface is up? Should I delete the "before up" here, too?


Gerhard




+.Pp
+To use IPv6, configure a link-local address.
+If the device is able to connect to the ISP's network but doesn't
+show an IPv6 address, setting the
+.Sy AUTOCONF6
+flag on the interface before bringing it up may help.
+.Ed

Re: IPv6 Support for umb(4)

[Gerhard Roth]

On Mon, 27 Apr 2020 16:59:22 +0200
Gerhard Roth  wrote:

> On 4/27/20 4:53 PM, Theo de Raadt wrote:
> > Gerhard Roth  wrote:
> >   
> >> Hi Theo,
> >>
> >> On 4/27/20 4:39 PM, Theo de Raadt wrote:  
> >>> Is this code in umb_decode_ip_configuration() reached again, if
> >>> you do a late ifconfig (don't set inet6 at up time, but set it
> >>> later)  
> >>
> >> no, seting inet6 later doesn't work. On MBIM level I have to tell the
> >> device *before* the CONNECT whether I want IPv6 support or not. And
> >> there is no way to change that selection later on while we are
> >> connected.
> >>
> >> The only way to do this transparently would be an implicit disconnect
> >> followed by a reconnect in if_umb.c. But then I would need some trigger
> >> that is called every time someone does 'ifconfig umb0 inet6 eui64' or
> >> 'ifconfig umb0 -inet6'. And I'm not sure whether the implicit
> >> temporary link loss is appreciated by the user.  
> > 
> > Then this diff seems incorrect.
> > 
> > The alternative of disconnecting, is not nice.
> > 
> > Perhaps umb can always request inet6 at startup, but not actually expose
> > it to the network stack.  Then enabling the software inet6 flag can expose
> > inet6 to the network layer, disabling the inet6 flag can remove exposure
> > to the network layer, but the actual address and support is always attempted
> > by the driver?
> >   
> 
> 
> That would work. Will try this.


So here is an updated diff that behaves in the proposed way.
umb_decode_ip_configuration() just stores the IP addresses in softc
and the real configuration is now done in umb_configure().
The later we also call, when our address-change hook is called.

Gerhard



Index: share/man/man4/umb.4
===
RCS file: /cvs/src/share/man/man4/umb.4,v
retrieving revision 1.10
diff -u -p -u -p -r1.10 umb.4
--- share/man/man4/umb.418 Feb 2020 08:09:37 -  1.10
+++ share/man/man4/umb.430 Apr 2020 11:47:55 -
@@ -40,6 +40,13 @@ will remain in this state until the MBIM
 In case the device is connected to an "always-on" USB port,
 it may be possible to connect to a provider without entering the
 PIN again even if the system was rebooted.
+.Pp
+To use IPv6, configure a link-local address.
+If the device is able to connect to the ISP's network but doesn't
+show an IPv6 address, setting the
+.Sy AUTOCONF6
+flag on the interface before bringing it up may help.
+.Ed
 .Sh HARDWARE
 The following devices should work:
 .Pp
Index: sys/dev/usb/if_umb.c
===
RCS file: /cvs/src/sys/dev/usb/if_umb.c,v
retrieving revision 1.33
diff -u -p -u -p -r1.33 if_umb.c
--- sys/dev/usb/if_umb.c27 Apr 2020 11:16:51 -  1.33
+++ sys/dev/usb/if_umb.c30 Apr 2020 11:18:54 -
@@ -147,6 +147,9 @@ void umb_newstate(struct umb_softc *, 
 voidumb_state_task(void *);
 voidumb_up(struct umb_softc *);
 voidumb_down(struct umb_softc *, int);
+#ifdef INET6
+voidumb_addr6_change(void *);
+#endif
 
 voidumb_get_response_task(void *);
 
@@ -164,11 +167,10 @@ intumb_decode_packet_service(struct u
 int umb_decode_signal_state(struct umb_softc *, void *, int);
 int umb_decode_connect_info(struct umb_softc *, void *, int);
 voidumb_clear_addr(struct umb_softc *);
-int umb_add_inet_config(struct umb_softc *, struct in_addr, u_int,
-   struct in_addr);
-int umb_add_inet6_config(struct umb_softc *, struct in6_addr *,
-   u_int, struct in6_addr *);
+int umb_add_inet_config(struct umb_softc *);
+int umb_add_inet6_config(struct umb_softc *);
 voidumb_send_inet_proposal(struct umb_softc *, int);
+int umb_configure(struct umb_softc *);
 int umb_decode_ip_configuration(struct umb_softc *, void *, int);
 voidumb_rx(struct umb_softc *);
 voidumb_rxeof(struct usbd_xfer *, void *, usbd_status);
@@ -477,6 +479,11 @@ umb_attach(struct device *parent, struct
USB_TASK_TYPE_GENERIC);
timeout_set(>sc_statechg_timer, umb_statechg_timeout, sc);
 
+#ifdef INET6
+   task_set(>sc_atask, umb_addr6_change, sc);
+   task_set(>sc_ctask, (void (*)(void *))umb_configure, sc);
+#endif
+
if (usbd_open_pipe_intr(uaa->iface, ctrl_ep, USBD_SHORT_XFER_OK,
>sc_ctrl_pipe, sc, >sc_intr_msg, sizeof (sc->sc_intr_msg),
umb_intr, USBD_DEFAULT_INTERVAL)) {
@@ -530,6 +537,10 @@ umb_attach(struct device *parent, struct
 #if NBPFILT

Re: IPv6 Support for umb(4)

[Gerhard Roth]

On 4/27/20 4:53 PM, Theo de Raadt wrote:

Gerhard Roth  wrote:


Hi Theo,

On 4/27/20 4:39 PM, Theo de Raadt wrote:

Is this code in umb_decode_ip_configuration() reached again, if
you do a late ifconfig (don't set inet6 at up time, but set it
later)


no, seting inet6 later doesn't work. On MBIM level I have to tell the
device *before* the CONNECT whether I want IPv6 support or not. And
there is no way to change that selection later on while we are
connected.

The only way to do this transparently would be an implicit disconnect
followed by a reconnect in if_umb.c. But then I would need some trigger
that is called every time someone does 'ifconfig umb0 inet6 eui64' or
'ifconfig umb0 -inet6'. And I'm not sure whether the implicit
temporary link loss is appreciated by the user.


Then this diff seems incorrect.

The alternative of disconnecting, is not nice.

Perhaps umb can always request inet6 at startup, but not actually expose
it to the network stack.  Then enabling the software inet6 flag can expose
inet6 to the network layer, disabling the inet6 flag can remove exposure
to the network layer, but the actual address and support is always attempted
by the driver?




That would work. Will try this.

Re: IPv6 Support for umb(4)

[Gerhard Roth]

Hi Theo,

On 4/27/20 4:39 PM, Theo de Raadt wrote:

Is this code in umb_decode_ip_configuration() reached again, if
you do a late ifconfig (don't set inet6 at up time, but set it
later)


no, seting inet6 later doesn't work. On MBIM level I have to tell the 
device *before* the CONNECT whether I want IPv6 support or not. And 
there is no way to change that selection later on while we are connected.


The only way to do this transparently would be an implicit disconnect
followed by a reconnect in if_umb.c. But then I would need some trigger
that is called every time someone does 'ifconfig umb0 inet6 eui64' or 
'ifconfig umb0 -inet6'. And I'm not sure whether the implicit temporary 
link loss is appreciated by the user.


Gerhard




That is how other network interfaces work.  I'm trying to make
sure this behaviour isn't too weird (ie. requiring a down, then up).

Gerhard Roth  wrote:


And since IPv6 is now optional for umb(4), we can just skip
evaluation of the IPv6 part of the IP configuration, if it
wasn't enabled.

Gerhard


Index: sys/dev/usb/if_umb.c
===
RCS file: /cvs/src/sys/dev/usb/if_umb.c,v
retrieving revision 1.33
diff -u -p -u -p -r1.33 if_umb.c
--- sys/dev/usb/if_umb.c27 Apr 2020 11:16:51 -  1.33
+++ sys/dev/usb/if_umb.c27 Apr 2020 13:56:09 -
@@ -1937,6 +1937,10 @@ tryv6:;
/*
 * IPv6 configuation
 */
+   if ((sc->sc_flags & UMBFLG_NO_INET6) ||
+   in6ifa_ifpforlinklocal(GET_IFP(sc), 0) == NULL)
+   goto done;
+
avail_v6 = letoh32(ic->ipv6_available);
if (avail_v6 == 0) {
if (ifp->if_flags & IFF_DEBUG)

Re: IPv6 Support for umb(4)

[Gerhard Roth]

And since IPv6 is now optional for umb(4), we can just skip
evaluation of the IPv6 part of the IP configuration, if it
wasn't enabled.

Gerhard


Index: sys/dev/usb/if_umb.c
===
RCS file: /cvs/src/sys/dev/usb/if_umb.c,v
retrieving revision 1.33
diff -u -p -u -p -r1.33 if_umb.c
--- sys/dev/usb/if_umb.c27 Apr 2020 11:16:51 -  1.33
+++ sys/dev/usb/if_umb.c27 Apr 2020 13:56:09 -
@@ -1937,6 +1937,10 @@ tryv6:;
/*
 * IPv6 configuation
 */
+   if ((sc->sc_flags & UMBFLG_NO_INET6) ||
+   in6ifa_ifpforlinklocal(GET_IFP(sc), 0) == NULL)
+   goto done;
+
avail_v6 = letoh32(ic->ipv6_available);
if (avail_v6 == 0) {
if (ifp->if_flags & IFF_DEBUG)

Re: IPv6 Support for umb(4)

[Gerhard Roth]

Hello Claudio,

On Mon, 27 Apr 2020 11:51:50 +0200 Claudio Jeker  
wrote:
> On Mon, Apr 27, 2020 at 10:26:01AM +0200, Gerhard Roth wrote:
> > Should we change umb(4) so that it only grabs an IPv6 address
> > in case somebody does a "ifconfig umb0 inet6 eui64" first?
> > 
> > Anyone willing to ok the patch below?  
> 
> see below
>  
> > On 2/19/20 9:19 AM, Gerhard Roth wrote:  
> > > On Wed, 19 Feb 2020 08:45:39 +0100 Claudio Jeker 
> > >  wrote:  
> > > > On Tue, Feb 18, 2020 at 11:16:54PM +, Stuart Henderson wrote:  
> > > > > On 2020/02/18 13:40, Gerhard Roth wrote:  
> > > > > > > > Yes, I tried MBIM_CONTEXT_IPTYPE_IPV4ANDV6 myself first but to 
> > > > > > > > no
> > > > > > > > avail. The switched to MBIM_CONTEXT_IPTYPE_IPV4V6 and everything
> > > > > > > > was fine.  
> > > > > > > 
> > > > > > > Obviously it needs to switch based on INET6, but with the current
> > > > > > > mechanism used for handling v6 in OpenBSD, shouldn't it disable v6
> > > > > > > unless the interface has a link-local configured? (So the usual 
> > > > > > > method
> > > > > > > to enable v6 and bring an interface up would be "inet6 eui64" and 
> > > > > > > "up").
> > > > > > > That is how pppoe works.  
> > > > > > 
> > > > > > 
> > > > > > Hi Stuart,
> > > > > > 
> > > > > > you mean like that?  
> > > > > 
> > > > > Yes, that looks right - sorry I don't have a working umb to test 
> > > > > though!  
> > > > 
> > > > I guess we should then also adjust the manpage to make sure people know
> > > > how to enable IPv6 in hostname.umb0  
> > > 
> > > 
> > > Took a look at pppoe.4 and tried to extract what is needed for umb.4.
> > > Updated diff below.
> > > 
> > > Gerhard
> > > 
> > > 
> > > Index: share/man/man4/umb.4
> > > ===
> > > RCS file: /cvs/src/share/man/man4/umb.4,v
> > > retrieving revision 1.10
> > > diff -u -p -u -p -r1.10 umb.4
> > > --- share/man/man4/umb.4  18 Feb 2020 08:09:37 -  1.10
> > > +++ share/man/man4/umb.4  19 Feb 2020 08:14:01 -
> > > @@ -40,6 +40,19 @@ will remain in this state until the MBIM
> > >   In case the device is connected to an "always-on" USB port,
> > >   it may be possible to connect to a provider without entering the
> > >   PIN again even if the system was rebooted.
> > > +.Pp
> > > +To use IPv6, configure a link-local address before bringing
> > > +the interface up.
> > > +Some devices require the
> > > +.Sy AUTOCONF6
> > > +flag on the interface.  
> 
> I think I asked this already, how does one know if autoconf is needed or
> not. Is that only device specific or also provider dependent?
> Adding autoconf will enable slaacd(8) on the device. What kind of troubles
> could result from this on a device that do not need it?
> In general this paragraph does not really help me to understand the
> situation better.

Basically, the MBIM protocol allows us to obtain the IPv6
address the ISP has given to us with the MBIM_CID_IP_CONFIGURATION
query. And all the provider I've met have done so, so far.

Yet job@ had a Japanese ISP that didn't give any IP address
via the MBIM protocol but wanted us to use regular IPv6
protocols to obtain the IP address.

Now this is a little bit too much detail for a man page.
And there is no technical way to find out what is required apart
from trial and error.

So I rephrased the sentence to:

If the device is able to connect to the ISP's network
but doesn't show an IPv6 address, setting the
AUTOCONF6 on the interface before bringing it up may help.

(see updated diff below)
Does that sound better?

> 
> > > +.Pp
> > > +A typical
> > > +.Pa /etc/hostname.umb0
> > > +looks like this:
> > > +.Bd -literal -offset indent
> > > +pin 1234 apn ISP-APN-Name inet6 eui64 autoconf -roaming up
> > > +.Ed  
> 
> In my opinion this is a bad example, it mixes a lot of different options
> which are not explained to the user in that man page. Also I think it is
> bad practice to put everything on one line. ifconfig(8) is rather
> sensitive when it comes to multiple commands in a single invocation.
>

Re: IPv6 Support for umb(4)

[Gerhard Roth]

Should we change umb(4) so that it only grabs an IPv6 address
in case somebody does a "ifconfig umb0 inet6 eui64" first?

Anyone willing to ok the patch below?



On 2/19/20 9:19 AM, Gerhard Roth wrote:

On Wed, 19 Feb 2020 08:45:39 +0100 Claudio Jeker  
wrote:

On Tue, Feb 18, 2020 at 11:16:54PM +, Stuart Henderson wrote:

On 2020/02/18 13:40, Gerhard Roth wrote:

Yes, I tried MBIM_CONTEXT_IPTYPE_IPV4ANDV6 myself first but to no
avail. The switched to MBIM_CONTEXT_IPTYPE_IPV4V6 and everything
was fine.


Obviously it needs to switch based on INET6, but with the current
mechanism used for handling v6 in OpenBSD, shouldn't it disable v6
unless the interface has a link-local configured? (So the usual method
to enable v6 and bring an interface up would be "inet6 eui64" and "up").
That is how pppoe works.



Hi Stuart,

you mean like that?


Yes, that looks right - sorry I don't have a working umb to test though!


I guess we should then also adjust the manpage to make sure people know
how to enable IPv6 in hostname.umb0



Took a look at pppoe.4 and tried to extract what is needed for umb.4.
Updated diff below.

Gerhard


Index: share/man/man4/umb.4
===
RCS file: /cvs/src/share/man/man4/umb.4,v
retrieving revision 1.10
diff -u -p -u -p -r1.10 umb.4
--- share/man/man4/umb.418 Feb 2020 08:09:37 -  1.10
+++ share/man/man4/umb.419 Feb 2020 08:14:01 -
@@ -40,6 +40,19 @@ will remain in this state until the MBIM
  In case the device is connected to an "always-on" USB port,
  it may be possible to connect to a provider without entering the
  PIN again even if the system was rebooted.
+.Pp
+To use IPv6, configure a link-local address before bringing
+the interface up.
+Some devices require the
+.Sy AUTOCONF6
+flag on the interface.
+.Pp
+A typical
+.Pa /etc/hostname.umb0
+looks like this:
+.Bd -literal -offset indent
+pin 1234 apn ISP-APN-Name inet6 eui64 autoconf -roaming up
+.Ed
  .Sh HARDWARE
  The following devices should work:
  .Pp
Index: sys/dev/usb/if_umb.c
===
RCS file: /cvs/src/sys/dev/usb/if_umb.c,v
retrieving revision 1.32
diff -u -p -u -p -r1.32 if_umb.c
--- sys/dev/usb/if_umb.c18 Feb 2020 08:09:37 -  1.32
+++ sys/dev/usb/if_umb.c18 Feb 2020 12:35:45 -
@@ -2583,7 +2583,8 @@ umb_send_connect(struct umb_softc *sc, i
c->iptype = htole32(MBIM_CONTEXT_IPTYPE_IPV4);
  #ifdef INET6
/* XXX FIXME: support IPv6-only mode, too */
-   if ((sc->sc_flags & UMBFLG_NO_INET6) == 0)
+   if ((sc->sc_flags & UMBFLG_NO_INET6) == 0 &&
+   in6ifa_ifpforlinklocal(GET_IFP(sc), 0) != NULL)
c->iptype = htole32(MBIM_CONTEXT_IPTYPE_IPV4V6);
  #endif
memcpy(c->context, umb_uuid_context_internet, sizeof (c->context));

Re: IPv6 Support for umb(4)

[Gerhard Roth]

On Wed, 19 Feb 2020 08:45:39 +0100 Claudio Jeker  
wrote:
> On Tue, Feb 18, 2020 at 11:16:54PM +, Stuart Henderson wrote:
> > On 2020/02/18 13:40, Gerhard Roth wrote:  
> > > > > Yes, I tried MBIM_CONTEXT_IPTYPE_IPV4ANDV6 myself first but to no
> > > > > avail. The switched to MBIM_CONTEXT_IPTYPE_IPV4V6 and everything
> > > > > was fine.
> > > > 
> > > > Obviously it needs to switch based on INET6, but with the current
> > > > mechanism used for handling v6 in OpenBSD, shouldn't it disable v6
> > > > unless the interface has a link-local configured? (So the usual method
> > > > to enable v6 and bring an interface up would be "inet6 eui64" and "up").
> > > > That is how pppoe works.  
> > > 
> > > 
> > > Hi Stuart,
> > > 
> > > you mean like that?  
> > 
> > Yes, that looks right - sorry I don't have a working umb to test though!  
> 
> I guess we should then also adjust the manpage to make sure people know
> how to enable IPv6 in hostname.umb0


Took a look at pppoe.4 and tried to extract what is needed for umb.4.
Updated diff below.

Gerhard


Index: share/man/man4/umb.4
===
RCS file: /cvs/src/share/man/man4/umb.4,v
retrieving revision 1.10
diff -u -p -u -p -r1.10 umb.4
--- share/man/man4/umb.418 Feb 2020 08:09:37 -  1.10
+++ share/man/man4/umb.419 Feb 2020 08:14:01 -
@@ -40,6 +40,19 @@ will remain in this state until the MBIM
 In case the device is connected to an "always-on" USB port,
 it may be possible to connect to a provider without entering the
 PIN again even if the system was rebooted.
+.Pp
+To use IPv6, configure a link-local address before bringing
+the interface up.
+Some devices require the
+.Sy AUTOCONF6
+flag on the interface.
+.Pp
+A typical
+.Pa /etc/hostname.umb0
+looks like this:
+.Bd -literal -offset indent
+pin 1234 apn ISP-APN-Name inet6 eui64 autoconf -roaming up
+.Ed
 .Sh HARDWARE
 The following devices should work:
 .Pp
Index: sys/dev/usb/if_umb.c
===
RCS file: /cvs/src/sys/dev/usb/if_umb.c,v
retrieving revision 1.32
diff -u -p -u -p -r1.32 if_umb.c
--- sys/dev/usb/if_umb.c18 Feb 2020 08:09:37 -  1.32
+++ sys/dev/usb/if_umb.c18 Feb 2020 12:35:45 -
@@ -2583,7 +2583,8 @@ umb_send_connect(struct umb_softc *sc, i
c->iptype = htole32(MBIM_CONTEXT_IPTYPE_IPV4);
 #ifdef INET6
/* XXX FIXME: support IPv6-only mode, too */
-   if ((sc->sc_flags & UMBFLG_NO_INET6) == 0)
+   if ((sc->sc_flags & UMBFLG_NO_INET6) == 0 &&
+   in6ifa_ifpforlinklocal(GET_IFP(sc), 0) != NULL)
c->iptype = htole32(MBIM_CONTEXT_IPTYPE_IPV4V6);
 #endif
memcpy(c->context, umb_uuid_context_internet, sizeof (c->context));

Re: IPv6 Support for umb(4)

[Gerhard Roth]

On Tue, 18 Feb 2020 12:11:05 + Stuart Henderson  
wrote:
> On 2020/02/18 08:25, Gerhard Roth wrote:
> > > > @@ -2393,6 +2581,11 @@ umb_send_connect(struct umb_softc *sc, i
> > > > c->authprot = htole32(MBIM_AUTHPROT_NONE);
> > > > c->compression = htole32(MBIM_COMPRESSION_NONE);
> > > > c->iptype = htole32(MBIM_CONTEXT_IPTYPE_IPV4);
> > > > +#ifdef INET6
> > > > +   /* XXX FIXME: support IPv6-only mode, too */
> > > > +   if ((sc->sc_flags & UMBFLG_NO_INET6) == 0)
> > > > +   c->iptype = htole32(MBIM_CONTEXT_IPTYPE_IPV4V6);
> > > 
> > > Is there a reason why MBIM_CONTEXT_IPTYPE_IPV4V6 is used and not
> > > MBIM_CONTEXT_IPTYPE_IPV4ANDV6?  
> > 
> > 
> > Yes, I tried MBIM_CONTEXT_IPTYPE_IPV4ANDV6 myself first but to no
> > avail. The switched to MBIM_CONTEXT_IPTYPE_IPV4V6 and everything
> > was fine.  
> 
> Obviously it needs to switch based on INET6, but with the current
> mechanism used for handling v6 in OpenBSD, shouldn't it disable v6
> unless the interface has a link-local configured? (So the usual method
> to enable v6 and bring an interface up would be "inet6 eui64" and "up").
> That is how pppoe works.


Hi Stuart,

you mean like that?

Gerhard


Index: sys/dev/usb/if_umb.c
===
RCS file: /cvs/src/sys/dev/usb/if_umb.c,v
retrieving revision 1.32
diff -u -p -u -p -r1.32 if_umb.c
--- sys/dev/usb/if_umb.c18 Feb 2020 08:09:37 -  1.32
+++ sys/dev/usb/if_umb.c18 Feb 2020 12:35:45 -
@@ -2583,7 +2583,8 @@ umb_send_connect(struct umb_softc *sc, i
c->iptype = htole32(MBIM_CONTEXT_IPTYPE_IPV4);
 #ifdef INET6
/* XXX FIXME: support IPv6-only mode, too */
-   if ((sc->sc_flags & UMBFLG_NO_INET6) == 0)
+   if ((sc->sc_flags & UMBFLG_NO_INET6) == 0 &&
+   in6ifa_ifpforlinklocal(GET_IFP(sc), 0) != NULL)
c->iptype = htole32(MBIM_CONTEXT_IPTYPE_IPV4V6);
 #endif
memcpy(c->context, umb_uuid_context_internet, sizeof (c->context));

Re: IPv6 Support for umb(4)

[Gerhard Roth]

Hi Claudio,

thanks for looking at it.

For your questions find my replies below.


On Mon, 17 Feb 2020 17:30:03 +0100 Claudio Jeker  
wrote:
> On Tue, Feb 04, 2020 at 09:16:34AM +0100, Gerhard Roth wrote:
> > Hi Alex,
> > 
> > thanks for looking into it.
> > 
> > 
> > On Tue, 4 Feb 2020 00:20:42 +0100 Alexander Bluhm  
> > wrote:  
> > > On Tue, Jan 28, 2020 at 03:03:47PM +0100, Gerhard Roth wrote:  
> > > > this patch adds IPv6 support to umb(4).
> > > 
> > > It breaks my IPv4 setup.
> > > 
> > > umb0 at uhub0 port 4 configuration 1 interface 6 "Lenovo H5321 gw" rev 
> > > 2.00/0.00 addr 2
> > > provider Vodafone.de
> > > firmware CXP 901 8700/1 - R3C18
> > > 
> > > When I apply the diff, my umb device does not get an IPv4 address.
> > > 
> > > umb0: state going up from 'open' to 'radio on'
> > > umb0: none state unlocked (-1 attempts left)
> > > umb0: set/qry MBIM_CID_SUBSCRIBER_READY_STATUS failed: BUSY
> > > umb0: packet service changed from detached to attaching, class none, 
> > > speed: 0 up / 0 down
> > > umb0: SIM initialized
> > > umb0: state going up from 'radio on' to 'SIM is ready'
> > > umb0: packet service changed from attaching to attached, class HSPA, 
> > > speed: 576 up / 1440 down
> > > umb0: state going up from 'SIM is ready' to 'attached'
> > > umb0: connecting ...
> > > umb0: set/qry MBIM_CID_CONNECT failed: NO_DEVICE_SUPPORT  
> > 
> > That looks like the firmware of this Lenovo H5321 doesn't support IPv6.
> > Well at least it gives a decent error code.
> > 
> >   
> > > umb0: state change timeout
> > > umb0: connecting ...
> > > umb0: set/qry MBIM_CID_CONNECT failed: NO_DEVICE_SUPPORT
> > > umb0: state change timeout
> > > umb0: connecting ...
> > > umb0: set/qry MBIM_CID_CONNECT failed: NO_DEVICE_SUPPORT
> > > umb0: state change timeout
> > > ...
> > > 
> > > A few comments inline.
> > >   
> > > > +#ifdef INET6
> > > > +int umb_add_inet6_config(struct umb_softc *, struct 
> > > > in6_addr *,
> > > > +   u_int, struct in6_addr *);
> > > > +#endif
> > > 
> > > Usually I avoid #ifdef for prototypes.  It does not matter whether
> > > the compiler reads them and without #ifdef the code is nicer.  
> > 
> > Removed it.
> > 
> >   
> > >   
> > > > +tryv6:;
> > > 
> > > The ; is wrong.  
> > 
> > Not really, because the label 'tryv6' is immediately followed by
> > an "#ifdef INET6". So looking just at this label I cannot directly tell
> > whether there is code that follows or not. And a label with no code
> > following is a syntax error in C.
> > 
> > I just followed a old "C Style and Coding Standards for SunOS" paper
> > by Bill Shannon from 1993 that says:
> > 
> > If a label is not followed by a program statement (e.g.
> > if the next token is a closing brace( } )) a NULL
> > statement ( ; ) must follow the label.
> > 
> >   
> > >   
> > > > +   if (n == 0 || off + sizeof (ipv6elem) > len)
> > > > +   goto done;
> > > > +   if (n != 1 && ifp->if_flags & IFF_DEBUG)
> > > > +   log(LOG_INFO, "%s: more than one IPv6 addr: 
> > > > %d\n",
> > > > +   DEVNAM(ifp->if_softc), n);
> > > > +
> > > > +   /* Only pick the first one */
> > > > +   memcpy(, data + off, sizeof (ipv6elem));
> > > > +   memcpy(, ipv6elem.addr, sizeof (addr6));
> > > > +
> > > > +   off = letoh32(ic->ipv6_gwoffs);
> > > > +   memcpy(, data + off, sizeof (gw6));
> > > 
> > > I think we should check the data length like above.
> > > 
> > >   if (off + sizeof (gw6) > len)
> > >   goto done;
> > > 
> > > And IPv4 should get the same check.  
> > 
> > Thanks for spotting that. Added check to both cases.
> > 
> >   
> > >   
> > > > @@ -380,6 +381,6 @@ struct umb_softc {
> > > >
> > > >  #define sc_state   sc_info.state
> > > >  #define sc_roaming sc_info.enable

Re: snmpd(8) reduce generic errors

[Gerhard Roth]

On 2/14/20 3:42 PM, Martijn van Duren wrote:

Apparently not many people check the error count in their snmp stats.
This appears to been here since day 1.

OK?

martijn@

Index: snmpe.c
===
RCS file: /cvs/src/usr.sbin/snmpd/snmpe.c,v
retrieving revision 1.60
diff -u -p -r1.60 snmpe.c
--- snmpe.c 24 Oct 2019 12:39:27 -  1.60
+++ snmpe.c 14 Feb 2020 14:41:44 -
@@ -766,6 +766,8 @@ snmpe_response(struct snmp_message *msg)
msg->sm_varbindresp = ober_unlink_elements(msg->sm_pduend);
  
  	switch (msg->sm_error) {

+   case SNMP_ERROR_NONE:
+   break;
case SNMP_ERROR_TOOBIG:
stats->snmp_intoobigs++;
break;



ok gerhard@

Re: IPv6 Support for umb(4)

[Gerhard Roth]

Hi Alex,

thanks for looking into it.


On Tue, 4 Feb 2020 00:20:42 +0100 Alexander Bluhm  
wrote:
> On Tue, Jan 28, 2020 at 03:03:47PM +0100, Gerhard Roth wrote:
> > this patch adds IPv6 support to umb(4).  
> 
> It breaks my IPv4 setup.
> 
> umb0 at uhub0 port 4 configuration 1 interface 6 "Lenovo H5321 gw" rev 
> 2.00/0.00 addr 2
> provider Vodafone.de
> firmware CXP 901 8700/1 - R3C18
> 
> When I apply the diff, my umb device does not get an IPv4 address.
> 
> umb0: state going up from 'open' to 'radio on'
> umb0: none state unlocked (-1 attempts left)
> umb0: set/qry MBIM_CID_SUBSCRIBER_READY_STATUS failed: BUSY
> umb0: packet service changed from detached to attaching, class none, speed: 0 
> up / 0 down
> umb0: SIM initialized
> umb0: state going up from 'radio on' to 'SIM is ready'
> umb0: packet service changed from attaching to attached, class HSPA, speed: 
> 576 up / 1440 down
> umb0: state going up from 'SIM is ready' to 'attached'
> umb0: connecting ...
> umb0: set/qry MBIM_CID_CONNECT failed: NO_DEVICE_SUPPORT

That looks like the firmware of this Lenovo H5321 doesn't support IPv6.
Well at least it gives a decent error code.


> umb0: state change timeout
> umb0: connecting ...
> umb0: set/qry MBIM_CID_CONNECT failed: NO_DEVICE_SUPPORT
> umb0: state change timeout
> umb0: connecting ...
> umb0: set/qry MBIM_CID_CONNECT failed: NO_DEVICE_SUPPORT
> umb0: state change timeout
> ...
> 
> A few comments inline.
> 
> > +#ifdef INET6
> > +int umb_add_inet6_config(struct umb_softc *, struct 
> > in6_addr *,
> > +   u_int, struct in6_addr *);
> > +#endif  
> 
> Usually I avoid #ifdef for prototypes.  It does not matter whether
> the compiler reads them and without #ifdef the code is nicer.

Removed it.


> 
> > +tryv6:;  
> 
> The ; is wrong.

Not really, because the label 'tryv6' is immediately followed by
an "#ifdef INET6". So looking just at this label I cannot directly tell
whether there is code that follows or not. And a label with no code
following is a syntax error in C.

I just followed a old "C Style and Coding Standards for SunOS" paper
by Bill Shannon from 1993 that says:

If a label is not followed by a program statement (e.g.
if the next token is a closing brace( } )) a NULL
statement ( ; ) must follow the label.


> 
> > +   if (n == 0 || off + sizeof (ipv6elem) > len)
> > +   goto done;
> > +   if (n != 1 && ifp->if_flags & IFF_DEBUG)
> > +   log(LOG_INFO, "%s: more than one IPv6 addr: %d\n",
> > +   DEVNAM(ifp->if_softc), n);
> > +
> > +   /* Only pick the first one */
> > +   memcpy(, data + off, sizeof (ipv6elem));
> > +   memcpy(, ipv6elem.addr, sizeof (addr6));
> > +
> > +   off = letoh32(ic->ipv6_gwoffs);
> > +   memcpy(, data + off, sizeof (gw6));  
> 
> I think we should check the data length like above.
> 
>   if (off + sizeof (gw6) > len)
>   goto done;
> 
> And IPv4 should get the same check.

Thanks for spotting that. Added check to both cases.


> 
> > @@ -380,6 +381,6 @@ struct umb_softc {
> >
> >  #define sc_state   sc_info.state
> >  #define sc_roaming sc_info.enable_roaming
> > -   struct umb_info sc_info;
> > +   struct umb_info  sc_info;
> >  };
> >  #endif /* _KERNEL */  
> 
> This whitespace chunk is wrong.

I think it was wrong before. It's common idiom to add an extra space
to non-pointer members to keep the member names aligned, e.g.

struct foo {
int *abc;
int  def;
int *bar;
};

Please correct me if I'm wrong.

> 
> bluhm


The updated patch below introduces a UMBFLG_NO_INET6 which is set on
receipt of a MBIM_STATUS_NO_DEVICE_SUPPORT in response to a
MBIM_CID_CONNECT. The code will then retry the connect operation in
IPv4-only mode.

That won't give you any IPv6 support, but at least it won't break
your setup.


Gerhard


Index: sbin/ifconfig/ifconfig.c
===
RCS file: /cvs/src/sbin/ifconfig/ifconfig.c,v
retrieving revision 1.417
diff -u -p -u -p -r1.417 ifconfig.c
--- sbin/ifconfig/ifconfig.c27 Dec 2019 14:34:46 -  1.417
+++ sbin/ifconfig/ifconfig.c28 Jan 2020 12:16:23 -
@@ -5666,6 +5666,7 @@ umb_status(void)
char apn[UMB_APN_MAXLEN+1];
char pn[UMB_PHONENR_MAXLEN+1];
int  i, n;
+   char astr[INET6_ADDRSTRLEN];
 
memset((char *), 0, sizeof(mi))

Re: IPv6 Support for umb(4)

[Gerhard Roth]

So far I've got only one ok from job@ and he'd like me to commit this.

So I asking if there are any objections against this going into the base.


Gerhard


On Tue, 28 Jan 2020 15:03:47 +0100 Gerhard Roth  wrote:
> Hi,
> 
> this patch adds IPv6 support to umb(4).
> 
> It will try to obtain a IPv6 address if the kernel is compiled with INET6.
> Currently there is no option to disable IPv6 on such a kernel (other than
> manually calling "ifconfig umb0 -inet6"). Nor is there a IPv6-only mode which
> refrains from obtaining an IPv4 address from the kernel.
> 
> To get an IPv6 address, your provider has to offer one. But more importantly
> the firmware of your umb(4) device has to have IPv6 support. I stumbled
> across two older Sierra Wireless modules (EM8805 and MC3805) that refused
> to provide an IPv6 address.
> 
> Have fun,
> 
> Gerhard
> 
> 
> Index: sbin/ifconfig/ifconfig.c
> ===
> RCS file: /cvs/src/sbin/ifconfig/ifconfig.c,v
> retrieving revision 1.417
> diff -u -p -u -p -r1.417 ifconfig.c
> --- sbin/ifconfig/ifconfig.c  27 Dec 2019 14:34:46 -  1.417
> +++ sbin/ifconfig/ifconfig.c  23 Jan 2020 09:24:38 -
> @@ -5666,6 +5666,7 @@ umb_status(void)
>   char apn[UMB_APN_MAXLEN+1];
>   char pn[UMB_PHONENR_MAXLEN+1];
>   int  i, n;
> + char astr[INET6_ADDRSTRLEN];
>  
>   memset((char *), 0, sizeof(mi));
>   ifr.ifr_data = (caddr_t)
> @@ -5830,7 +5831,15 @@ umb_status(void)
>   for (i = 0, n = 0; i < UMB_MAX_DNSSRV; i++) {
>   if (mi.ipv4dns[i].s_addr == INADDR_ANY)
>   break;
> - printf("%s %s", n++ ? "" : "\tdns", inet_ntoa(mi.ipv4dns[i]));
> + printf("%s %s", n++ ? "" : "\tdns",
> + inet_ntop(AF_INET, [i], astr, sizeof (astr)));
> + }
> + for (i = 0; i < UMB_MAX_DNSSRV; i++) {
> + if (memcmp([i], _any,
> + sizeof (mi.ipv6dns[i])) == 0)
> + break;
> + printf("%s %s", n++ ? "" : "\tdns",
> + inet_ntop(AF_INET6, [i], astr, sizeof (astr)));
>   }
>   if (n)
>   printf("\n");
> Index: share/man/man4/umb.4
> ===
> RCS file: /cvs/src/share/man/man4/umb.4,v
> retrieving revision 1.9
> diff -u -p -u -p -r1.9 umb.4
> --- share/man/man4/umb.4  23 Nov 2017 20:47:26 -  1.9
> +++ share/man/man4/umb.4  28 Jan 2020 09:04:20 -
> @@ -40,6 +40,11 @@ will remain in this state until the MBIM
>  In case the device is connected to an "always-on" USB port,
>  it may be possible to connect to a provider without entering the
>  PIN again even if the system was rebooted.
> +.Pp
> +If the kernel has been compiled with INET6, the driver will try to
> +obtain an IPv6 address from the provider. To succeed with the IPv6
> +configuration, both the ISP and the MBIM device have to offer IPv6
> +support.
>  .Sh HARDWARE
>  The following devices should work:
>  .Pp
> @@ -64,10 +69,6 @@ The following devices should work:
>  .%U http://www.usb.org/developers/docs/devclass_docs/MBIM10Errata1_073013.zip
>  .Re
>  .Sh CAVEATS
> -The
> -.Nm
> -driver does not support IPv6.
> -.Pp
>  Devices which fail to provide a conforming MBIM implementation will
>  probably be attached as some other driver, such as
>  .Xr umsm 4 .
> Index: sys/dev/usb/if_umb.c
> ===
> RCS file: /cvs/src/sys/dev/usb/if_umb.c,v
> retrieving revision 1.31
> diff -u -p -u -p -r1.31 if_umb.c
> --- sys/dev/usb/if_umb.c  26 Nov 2019 23:04:28 -  1.31
> +++ sys/dev/usb/if_umb.c  28 Jan 2020 09:08:16 -
> @@ -43,6 +43,14 @@
>  #include 
>  #include 
>  
> +#ifdef INET6
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#endif
> +
>  #include 
>  
>  #include 
> @@ -158,7 +166,11 @@ int   umb_decode_connect_info(struct umb
>  void  umb_clear_addr(struct umb_softc *);
>  int   umb_add_inet_config(struct umb_softc *, struct in_addr, u_int,
>   struct in_addr);
> -void  umb_send_inet_proposal(struct umb_softc *);
> +#ifdef INET6
> +int   umb_add_inet6_config(struct umb_softc *, struct in6_addr *,
> + u_int, struct in6_addr *);
> +#endif
> +void  umb_send_inet_proposal(struct umb_softc *, int);
>  int   umb_decode_ip_configuration(struct umb_softc *, void *, int);
>  

IPv6 Support for umb(4)

[Gerhard Roth]

Hi,

this patch adds IPv6 support to umb(4).

It will try to obtain a IPv6 address if the kernel is compiled with INET6.
Currently there is no option to disable IPv6 on such a kernel (other than
manually calling "ifconfig umb0 -inet6"). Nor is there a IPv6-only mode which
refrains from obtaining an IPv4 address from the kernel.

To get an IPv6 address, your provider has to offer one. But more importantly
the firmware of your umb(4) device has to have IPv6 support. I stumbled
across two older Sierra Wireless modules (EM8805 and MC3805) that refused
to provide an IPv6 address.

Have fun,

Gerhard


Index: sbin/ifconfig/ifconfig.c
===
RCS file: /cvs/src/sbin/ifconfig/ifconfig.c,v
retrieving revision 1.417
diff -u -p -u -p -r1.417 ifconfig.c
--- sbin/ifconfig/ifconfig.c27 Dec 2019 14:34:46 -  1.417
+++ sbin/ifconfig/ifconfig.c23 Jan 2020 09:24:38 -
@@ -5666,6 +5666,7 @@ umb_status(void)
char apn[UMB_APN_MAXLEN+1];
char pn[UMB_PHONENR_MAXLEN+1];
int  i, n;
+   char astr[INET6_ADDRSTRLEN];
 
memset((char *), 0, sizeof(mi));
ifr.ifr_data = (caddr_t)
@@ -5830,7 +5831,15 @@ umb_status(void)
for (i = 0, n = 0; i < UMB_MAX_DNSSRV; i++) {
if (mi.ipv4dns[i].s_addr == INADDR_ANY)
break;
-   printf("%s %s", n++ ? "" : "\tdns", inet_ntoa(mi.ipv4dns[i]));
+   printf("%s %s", n++ ? "" : "\tdns",
+   inet_ntop(AF_INET, [i], astr, sizeof (astr)));
+   }
+   for (i = 0; i < UMB_MAX_DNSSRV; i++) {
+   if (memcmp([i], _any,
+   sizeof (mi.ipv6dns[i])) == 0)
+   break;
+   printf("%s %s", n++ ? "" : "\tdns",
+   inet_ntop(AF_INET6, [i], astr, sizeof (astr)));
}
if (n)
printf("\n");
Index: share/man/man4/umb.4
===
RCS file: /cvs/src/share/man/man4/umb.4,v
retrieving revision 1.9
diff -u -p -u -p -r1.9 umb.4
--- share/man/man4/umb.423 Nov 2017 20:47:26 -  1.9
+++ share/man/man4/umb.428 Jan 2020 09:04:20 -
@@ -40,6 +40,11 @@ will remain in this state until the MBIM
 In case the device is connected to an "always-on" USB port,
 it may be possible to connect to a provider without entering the
 PIN again even if the system was rebooted.
+.Pp
+If the kernel has been compiled with INET6, the driver will try to
+obtain an IPv6 address from the provider. To succeed with the IPv6
+configuration, both the ISP and the MBIM device have to offer IPv6
+support.
 .Sh HARDWARE
 The following devices should work:
 .Pp
@@ -64,10 +69,6 @@ The following devices should work:
 .%U http://www.usb.org/developers/docs/devclass_docs/MBIM10Errata1_073013.zip
 .Re
 .Sh CAVEATS
-The
-.Nm
-driver does not support IPv6.
-.Pp
 Devices which fail to provide a conforming MBIM implementation will
 probably be attached as some other driver, such as
 .Xr umsm 4 .
Index: sys/dev/usb/if_umb.c
===
RCS file: /cvs/src/sys/dev/usb/if_umb.c,v
retrieving revision 1.31
diff -u -p -u -p -r1.31 if_umb.c
--- sys/dev/usb/if_umb.c26 Nov 2019 23:04:28 -  1.31
+++ sys/dev/usb/if_umb.c28 Jan 2020 09:08:16 -
@@ -43,6 +43,14 @@
 #include 
 #include 
 
+#ifdef INET6
+#include 
+#include 
+#include 
+#include 
+#include 
+#endif
+
 #include 
 
 #include 
@@ -158,7 +166,11 @@ int umb_decode_connect_info(struct umb
 voidumb_clear_addr(struct umb_softc *);
 int umb_add_inet_config(struct umb_softc *, struct in_addr, u_int,
struct in_addr);
-voidumb_send_inet_proposal(struct umb_softc *);
+#ifdef INET6
+int umb_add_inet6_config(struct umb_softc *, struct in6_addr *,
+   u_int, struct in6_addr *);
+#endif
+voidumb_send_inet_proposal(struct umb_softc *, int);
 int umb_decode_ip_configuration(struct umb_softc *, void *, int);
 voidumb_rx(struct umb_softc *);
 voidumb_rxeof(struct usbd_xfer *, void *, usbd_status);
@@ -800,8 +812,8 @@ umb_input(struct ifnet *ifp, struct mbuf
 #endif /* INET6 */
default:
ifp->if_ierrors++;
-   DPRINTFN(4, "%s: dropping packet with bad IP version (%d)\n",
-   __func__, ipv);
+   DPRINTFN(4, "%s: dropping packet with bad IP version (af %d)\n",
+   __func__, af);
m_freem(m);
return 1;
}
@@ -902,7 +914,10 @@ umb_rtrequest(struct ifnet *ifp, int req
struct umb_softc *sc = ifp->if_softc;
 
if (req == RTM_PROPOSAL) {
-   umb_send_inet_proposal(sc);
+   umb_send_inet_proposal(sc, AF_INET);
+#ifdef INET6
+   umb_send_inet_proposal(sc, 

legacy sending of traps in snmpd

[Gerhard Roth]

Hi,

any initialization of the form

struct ber_oid trapoid = OID(MIB_snmpTrapOID);

requires a smi_scalar_oidlen() afterwards to set 'bo_n' to the correct
length.

The old ber_oid_cmp() from usr.sbin/snmpd/ber.c used to iterate over
all elements of 'bo_id' and not just the first 'bo_n' ones. So calling
smi_scalar_oidlen() wasn't a requirement here. However, with the new
ober_oid_cmp() it is, since this version only iterates up to 'bo_n'
array elements.

Gerhard



Index: usr.sbin/snmpd/trap.c
===
RCS file: /cvs/src/usr.sbin/snmpd/trap.c,v
retrieving revision 1.33
diff -u -p -u -p -r1.33 trap.c
--- usr.sbin/snmpd/trap.c   24 Oct 2019 12:39:27 -  1.33
+++ usr.sbin/snmpd/trap.c   9 Dec 2019 13:32:21 -
@@ -83,6 +83,8 @@ trap_agentx(struct agentx_handle *h, str
goto done;
}
 
+   smi_scalar_oidlen();
+   smi_scalar_oidlen();
while (pdu->datalen > sizeof(struct agentx_hdr)) {
x++;
 

fix xhci 'actlen' calculation

[Gerhard Roth]

Hi,

xhci's calculation of 'xfer->actlen' is wrong if the xfer was split into
multiple TRBs. That's because the code just looks at the remainder
reported by the status TRB. However, this remainder only refers to the
total size of this single TRB; not to the total size of the xfer.

Example: assume a 16 kb xfer is split into two TRBs for 8 kb each.
Then we get a short read of 6 kb. The status TRB will refer
to the first TRB with remainder set to 2 kb. Currently xhci
would assume that actlen is 'xfer->length' (16 kb) minus the
remainder (2 kb). That yields a wrong actlen of 14 kb instead
of 6 kb.

The patch below fixes this by adding up all the remainders of all the
transfer TRBs up to the current one.

Gerhard


Index: sys/dev/usb/xhci.c
===
RCS file: /cvs/src/sys/dev/usb/xhci.c,v
retrieving revision 1.106
diff -u -p -u -p -r1.106 xhci.c
--- sys/dev/usb/xhci.c  6 Oct 2019 17:30:00 -   1.106
+++ sys/dev/usb/xhci.c  12 Nov 2019 09:29:47 -
@@ -810,6 +810,28 @@ xhci_event_xfer(struct xhci_softc *sc, u
xhci_xfer_done(xfer);
 }
 
+uint32_t
+xhci_xfer_length_generic(struct xhci_xfer *xx, struct xhci_pipe *xp,
+int trb_idx)
+{
+   int  trb0_idx;
+   uint32_t len = 0;
+
+   KASSERT(xx->index >= 0);
+   trb0_idx =
+   ((xx->index + xp->ring.ntrb) - xx->ntrb) % (xp->ring.ntrb - 1);
+
+   while (1) {
+   len +=
+   le32toh(XHCI_TRB_LEN(xp->ring.trbs[trb0_idx].trb_status));
+   if (trb0_idx == trb_idx)
+   break;
+   if (++trb0_idx == xp->ring.ntrb)
+   trb0_idx = 0;
+   }
+   return len;
+}
+
 int
 xhci_event_xfer_generic(struct xhci_softc *sc, struct usbd_xfer *xfer,
 struct xhci_pipe *xp, uint32_t remain, int trb_idx,
@@ -823,12 +845,22 @@ xhci_event_xfer_generic(struct xhci_soft
 * This might be the last TRB of a TD that ended up
 * with a Short Transfer condition, see below.
 */
-   if (xfer->actlen == 0)
-   xfer->actlen = xfer->length - remain;
+   if (xfer->actlen == 0) {
+   if (remain)
+   xfer->actlen =
+   xhci_xfer_length_generic(xx, xp, trb_idx) -
+   remain;
+   else
+   xfer->actlen = xfer->length;
+   }
xfer->status = USBD_NORMAL_COMPLETION;
break;
case XHCI_CODE_SHORT_XFER:
-   xfer->actlen = xfer->length - remain;
+   /*
+* Use values from the transfer TRB instead of the status TRB.
+*/
+   xfer->actlen = xhci_xfer_length_generic(xx, xp, trb_idx) -
+   remain;
/*
 * If this is not the last TRB of a transfer, we should
 * theoretically clear the IOC at the end of the chain

Re: HID devices without numbered reports

[Gerhard Roth]

On Mon, 28 Oct 2019 15:41:34 +0100 Patrick Wildt  wrote:
> On Mon, Oct 28, 2019 at 03:14:22PM +1100, Damien Miller wrote:
> > On Mon, 28 Oct 2019, Damien Miller wrote:
> >   
> > > BTW, the token still becomes unresponsive after the first transaction,
> > > but looking at a sniff (using an OpenViszla), it seems we're getting the
> > > DATA0/DATA1 flipping incorrect on the wire.
> > > 
> > > On OpenBSD, this is the last rx of the transaction with the card:
> > > 
> > > []  12.992349 d=  0.001951 [154.0 +  3.500] [  3] IN   : 8.4
> > > []  12.992352 d=  0.03 [154.0 +  6.667] [ 67] DATA0: 00 10 00 
> > > 01 0e 1b 4a 78 ec 87 06 bd 47 d4 a0 49 d9 c7 2d 89 d9 7e 2c c5 62 87 53 
> > > 92 9b 90 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> > > 00 00 00 00 00 00 00 00 00 00 00 00 00 3a e8
> > > 
> > > and this is the first tx of the first packet of the subsequent transaction
> > > that hangs:
> > > 
> > > []  22.201067 d=  0.001998 [146.0 +  4.333] [  3] OUT  : 8.4
> > > []  22.201070 d=  0.03 [146.0 +  7.583] [ 67] DATA0: ff ff ff 
> > > ff 86 00 08 c0 65 eb 53 9a 48 04 7d 00 00 00 00 00 00 00 00 00 00 00 00 
> > > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> > > 00 00 00 00 00 00 00 00 00 00 00 00 00 d6 1d  
> > 
> > Since I don't really understand how USB sequence numbers work, it just
> > occurred to me to check the sequence bit on the last sent packet.
> > It too is a DATA0, so the sequence is definitiely getting lost across
> > close+open.
> > 
> > Here's an annotated trace - starting with the last command sent to
> > a Yk5 token during key enrollment:
> > 
> > []   9.212345 d=  0.001992 [  2   +  4.333] [  3] OUT  : 10.4 
> > []   9.212349 d=  0.03 [  2   +  7.583] [ 67] DATA1: 00 13 00 
> > 01 83 00 47 00 01 00 00 00 00 40 a9 dc 95 51 0e ec 44 6d 7a b1 14 88 d1 84 
> > 93 b4 aa d2 e5 10 11 db 9c fa b4 b3 0b 89 2c ea 3f b9 e3 06 10 e8 a1 62 11 
> > 59 60 fe 1e c2 23 e6 52 9c 9f 4b 8a c8 
> > []   9.212395 d=  0.46 [  2   + 53.750] [  1] ACK 
> > []   9.212397 d=  0.02 [  2   + 55.667] [  3] IN   : 10.4 
> > []   9.212400 d=  0.03 [  2   + 58.833] [  1] NAK 
> > []   9.214346 d=  0.001946 [  4   +  4.500] [  3] OUT  : 10.4 
> > []   9.214349 d=  0.03 [  4   +  7.750] [ 67] DATA0: 00 13 00 
> > 01 00 6e 80 20 0d cb 5e 5c 32 1c 8a f1 e2 b1 bf 00 00 00 00 00 00 00 00 00 
> > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> > 00 00 00 00 00 00 00 00 00 00 00 11 7c 
> > []   9.214395 d=  0.46 [  4   + 53.750] [  1] ACK 
> > []   9.214397 d=  0.02 [  4   + 55.667] [  3] IN   : 10.4 
> > []   9.214400 d=  0.03 [  4   + 58.833] [  1] NAK 
> > []   9.216345 d=  0.001945 [  6   +  4.000] [  3] IN   : 10.4 
> > []   9.216349 d=  0.03 [  6   +  7.167] [  1] NAK 
> > 
> > Note that the last OUT was a DATA0.
> > 
> > The token replies with a bunch of data:
> > 
> > []   9.350355 d=  0.001997 [140   +  3.250] [  3] IN   : 10.4 
> > []   9.350358 d=  0.03 [140   +  6.417] [ 67] DATA1: 00 13 00 
> > 01 83 03 8d 05 04 f6 80 b1 df 4d 8e fe 30 cd a0 2c 42 e1 a1 46 52 9f 8a 06 
> > 80 21 78 7a 73 71 e3 f3 0a e6 f6 e0 74 54 ef df 0c 74 ed be 3f 35 1d dd 35 
> > cf 33 14 fc 33 6e b6 45 11 bd f5 79 99 
> > []   9.350404 d=  0.46 [140   + 52.417] [  1] ACK 
> > []   9.352356 d=  0.001951 [142   +  3.750] [  3] IN   : 10.4 
> > []   9.352359 d=  0.03 [142   +  6.917] [ 67] DATA0: 00 13 00 
> > 01 00 61 98 66 2c 14 5f 65 e5 4c 40 df 01 56 e8 d8 2b e8 f7 a8 ff 00 96 a6 
> > f3 95 00 d9 93 87 cb c8 b1 02 23 ec 8f 38 37 b8 cf da 73 62 18 90 ff 8b 01 
> > cf a1 78 61 3e cc 48 ac 7b 45 4f b0 6b 
> > []   9.352405 d=  0.46 [142   + 53.167] [  1] ACK 
> > []   9.354356 d=  0.001950 [144   +  3.500] [  3] IN   : 10.4 
> > []   9.354359 d=  0.03 [144   +  6.667] [ 67] DATA1: 00 13 00 
> > 01 01 4a 0d 3a 6d d7 ca fc 00 e1 ad 7e 78 b1 9d 88 30 82 02 bd 30 82 01 a5 
> > a0 03 02 01 02 02 04 18 ac 46 c0 30 0d 06 09 2a 86 48 86 f7 0d 01 01 0b 05 
> > 00 30 2e 31 2c 30 2a 06 03 55 04 83 af 
> > []   9.354405 d=  0.46 [144   + 52.833] [  1] ACK 
> > []   9.356356 d=  0.001951 [146   +  3.583] [  3] IN   : 10.4 
> > []   9.356359 d=  0.03 [146   +  6.750] [ 67] DATA0: 00 13 00 
> > 01 02 03 13 23 59 75 62 69 63 6f 20 55 32 46 20 52 6f 6f 74 20 43 41 20 53 
> > 65 72 69 61 6c 20 34 35 37 32 30 30 36 33 31 30 20 17 0d 31 34 30 38 30 31 
> > 30 30 30 30 30 30 5a 18 0f 32 30 98 68 
> > []   9.356405 d=  0.46 [146   + 52.667] [  1] ACK 
> > []   9.358356 d=  0.001951 [148   +  3.583] [  3] IN   : 10.4 
> > []   9.358359 d=  0.03 [148   +  6.750] [ 67] DATA1: 00 13 00 
> > 01 03 35 30 30 39 30 34 30 30 30 30 30 30 5a 30 6e 31 0b 30 09 06 03 55 04 
> > 06 13 02 53 

Re: Add SHA-2 support to snmpd [2/2] SHA-2/RFC7860

[Gerhard Roth]

On 6/7/19 10:45 AM, Martijn van Duren wrote:
> On 6/7/19 10:41 AM, Gerhard Roth wrote:
>> On 6/7/19 9:52 AM, Martijn van Duren wrote:
>>> On 6/7/19 9:50 AM, Martijn van Duren wrote:
>>>> Hello tech@,
>>>>
>>>> I managed to get SHA-2 support working for snmpd, based on RFC7860 and  
>>>> tested with net-snmp commandline tools.
>>>>
>>>> I split the diff up in 2 steps for readability.
>>> Step 2: Implement the SHA-2 values.
>>>>
>>>> OK?
>>
>> Great stuff!
>> ok gerhard@, but please update snmpd.conf.5, too. Otherwise nobody knows
>> how to use it.
>>
>>
> Of course.

ok gerhard@


> 
> diff --git a/snmpd.conf.5 b/snmpd.conf.5
> index 70ad72c..2eeb11e 100644
> --- a/snmpd.conf.5
> +++ b/snmpd.conf.5
> @@ -241,9 +241,13 @@ for this user account.
>  Optionally the HMAC algorithm used for authentication can be specified.
>  .Ar hmac
>  must be either
> -.Ic hmac-md5
> +.Ic hmac-md5 ,
> +.Ic hmac-sha1 ,
> +.Ic hmac-sha224 ,
> +.Ic hmac-sha256 ,
> +.Ic hmac-sha384 ,
>  or
> -.Ic hmac-sha1 .
> +.Ic hmac-sha512 .
>  If omitted the default is
>  .Ic hmac-sha1 .
>  .Pp
> 

Re: Add SHA-2 support to snmpd [2/2] SHA-2/RFC7860

[Gerhard Roth]

On 6/7/19 9:52 AM, Martijn van Duren wrote:
> On 6/7/19 9:50 AM, Martijn van Duren wrote:
>> Hello tech@,
>>
>> I managed to get SHA-2 support working for snmpd, based on RFC7860 and  
>> tested with net-snmp commandline tools.
>>
>> I split the diff up in 2 steps for readability.
> Step 2: Implement the SHA-2 values.
>>
>> OK?

Great stuff!
ok gerhard@, but please update snmpd.conf.5, too. Otherwise nobody knows
how to use it.


>>
>> martijn@
> 
> diff --git a/parse.y b/parse.y
> index 419dea5..cc719ea 100644
> --- a/parse.y
> +++ b/parse.y
> @@ -500,6 +500,18 @@ auth : STRING{
>   else if (strcasecmp($1, "hmac-sha1") == 0 ||
>strcasecmp($1, "hmac-sha1-96") == 0)
>   $$ = AUTH_SHA1;
> + else if (strcasecmp($1, "hmac-sha224") == 0 ||
> + strcasecmp($1, "usmHMAC128SHA224AuthProtocol") == 0)
> + $$ = AUTH_SHA224;
> + else if (strcasecmp($1, "hmac-sha256") == 0 ||
> + strcasecmp($1, "usmHMAC192SHA256AuthProtocol") == 0)
> + $$ = AUTH_SHA256;
> + else if (strcasecmp($1, "hmac-sha384") == 0 ||
> + strcasecmp($1, "usmHMAC256SHA384AuthProtocol") == 0)
> + $$ = AUTH_SHA384;
> + else if (strcasecmp($1, "hmac-sha512") == 0 ||
> + strcasecmp($1, "usmHMAC384SHA512AuthProtocol") == 0)
> + $$ = AUTH_SHA512;
>   else {
>   yyerror("syntax error, bad auth hmac");
>   free($1);
> diff --git a/snmpd.h b/snmpd.h
> index 0f7cf70..6fdb919 100644
> --- a/snmpd.h
> +++ b/snmpd.h
> @@ -59,7 +59,7 @@
>  #define SNMPD_MAXUSERNAMELEN 32
>  #define SNMPD_MAXCONTEXNAMELEN   32
>  
> -#define SNMP_USM_MAXDIGESTLEN12
> +#define SNMP_USM_MAXDIGESTLEN48
>  #define SNMP_USM_SALTLEN 8
>  #define SNMP_USM_KEYLEN  64
>  #define SNMP_CIPHER_KEYLEN   16
> @@ -534,7 +534,11 @@ TAILQ_HEAD(socklist, listen_sock);
>  enum usmauth {
>   AUTH_NONE = 0,
>   AUTH_MD5,   /* HMAC-MD5-96, RFC3414 */
> - AUTH_SHA1   /* HMAC-SHA-96, RFC3414 */
> + AUTH_SHA1,  /* HMAC-SHA-96, RFC3414 */
> + AUTH_SHA224,/* usmHMAC128SHA224AuthProtocol. RFC7860 */
> + AUTH_SHA256,/* usmHMAC192SHA256AuthProtocol. RFC7860 */
> + AUTH_SHA384,/* usmHMAC256SHA384AuthProtocol. RFC7860 */
> + AUTH_SHA512 /* usmHMAC384SHA512AuthProtocol. RFC7860 */
>  };
>  
>  #define AUTH_DEFAULT AUTH_SHA1   /* Default digest */
> diff --git a/usm.c b/usm.c
> index 80229f3..4f37e78 100644
> --- a/usm.c
> +++ b/usm.c
> @@ -97,6 +97,14 @@ usm_get_md(enum usmauth ua)
>   return EVP_md5();
>   case AUTH_SHA1:
>   return EVP_sha1();
> + case AUTH_SHA224:
> + return EVP_sha224();
> + case AUTH_SHA256:
> + return EVP_sha256();
> + case AUTH_SHA384:
> + return EVP_sha384();
> + case AUTH_SHA512:
> + return EVP_sha512();
>   case AUTH_NONE:
>   default:
>   return NULL;
> @@ -110,6 +118,14 @@ usm_get_digestlen(enum usmauth ua)
>   case AUTH_MD5:
>   case AUTH_SHA1:
>   return 12;
> + case AUTH_SHA224:
> + return 16;
> + case AUTH_SHA256:
> + return 24;
> + case AUTH_SHA384:
> + return 32;
> + case AUTH_SHA512:
> + return 48;
>   case AUTH_NONE:
>   default:
>   return 0;
> @@ -136,6 +152,10 @@ usm_valid_digestlen(size_t digestlen)
>   switch (digestlen) {
>   case 0:
>   case 12:
> + case 16:
> + case 24:
> + case 32:
> + case 48:
>   return 1;
>   default:
>   return 0;
> @@ -204,6 +224,18 @@ usm_checkuser(struct usmuser *up, const char **errp)
>   up->uu_seclevel |= SNMP_MSGFLAG_AUTH;
>   auth = "HMAC-SHA1-96";
>   break;
> + case AUTH_SHA224:
> + up->uu_seclevel |= SNMP_MSGFLAG_AUTH;
> + auth = "usmHMAC128SHA224AuthProtocol";
> + case AUTH_SHA256:
> + up->uu_seclevel |= SNMP_MSGFLAG_AUTH;
> + auth = "usmHMAC192SHA256AuthProtocol";
> + case AUTH_SHA384:
> + up->uu_seclevel |= SNMP_MSGFLAG_AUTH;
> + auth = "usmHMAC256SHA384AuthProtocol";
> + case AUTH_SHA512:
> + up->uu_seclevel |= SNMP_MSGFLAG_AUTH;
> + auth = "usmHMAC384SHA512AuthProtocol";
>   }
>  
>   switch (up->uu_priv) {
> 

Re: Add SHA-2 support to snmpd [1/2] Digest length is not always 12 bytes

[Gerhard Roth]

On 6/7/19 9:50 AM, Martijn van Duren wrote:
> Hello tech@,
> 
> I managed to get SHA-2 support working for snmpd, based on RFC7860 and  
> tested with net-snmp commandline tools.
> 
> I split the diff up in 2 steps for readability.
> Step 1: Don't assume the digestlength is always 12 bytes. This is only
> true for MD5 and SHA-1.
> 
> OK?

ok gerhard@


> 
> martijn@
> 
> diff --git a/snmpd.h b/snmpd.h
> index 5d5adfd..0f7cf70 100644
> --- a/snmpd.h
> +++ b/snmpd.h
> @@ -59,7 +59,7 @@
>  #define SNMPD_MAXUSERNAMELEN 32
>  #define SNMPD_MAXCONTEXNAMELEN   32
>  
> -#define SNMP_USM_DIGESTLEN   12
> +#define SNMP_USM_MAXDIGESTLEN12
>  #define SNMP_USM_SALTLEN 8
>  #define SNMP_USM_KEYLEN  64
>  #define SNMP_CIPHER_KEYLEN   16
> diff --git a/usm.c b/usm.c
> index 811235c..80229f3 100644
> --- a/usm.c
> +++ b/usm.c
> @@ -45,7 +45,9 @@
>  SLIST_HEAD(, usmuser)usmuserlist;
>  
>  const EVP_MD *usm_get_md(enum usmauth);
> +size_tusm_get_digestlen(enum usmauth);
>  const EVP_CIPHER *usm_get_cipher(enum usmpriv);
> +int   usm_valid_digestlen(size_t digestlen);
>  void  usm_cb_digest(void *, size_t);
>  int   usm_valid_digest(struct snmp_message *, off_t, char *,
>   size_t);
> @@ -101,6 +103,19 @@ usm_get_md(enum usmauth ua)
>   }
>  }
>  
> +size_t
> +usm_get_digestlen(enum usmauth ua)
> +{
> + switch (ua) {
> + case AUTH_MD5:
> + case AUTH_SHA1:
> + return 12;
> + case AUTH_NONE:
> + default:
> + return 0;
> + }
> +}
> +
>  const EVP_CIPHER *
>  usm_get_cipher(enum usmpriv up)
>  {
> @@ -115,6 +130,18 @@ usm_get_cipher(enum usmpriv up)
>   }
>  }
>  
> +int
> +usm_valid_digestlen(size_t digestlen)
> +{
> + switch (digestlen) {
> + case 0:
> + case 12:
> + return 1;
> + default:
> + return 0;
> + }
> +}
> +
>  struct usmuser *
>  usm_newuser(char *name, const char **errp)
>  {
> @@ -257,7 +284,7 @@ usm_decode(struct snmp_message *msg, struct ber_element 
> *elm, const char **errp)
>  
>   if (enginelen > SNMPD_MAXENGINEIDLEN ||
>   userlen > SNMPD_MAXUSERNAMELEN ||
> - (digestlen != (MSG_HAS_AUTH(msg) ? SNMP_USM_DIGESTLEN : 0)) ||
> + !usm_valid_digestlen(digestlen) ||
>   (saltlen != (MSG_HAS_PRIV(msg) ? SNMP_USM_SALTLEN : 0))) {
>   *errp = "bad field length";
>   msg->sm_flags &= SNMP_MSGFLAG_REPORT;
> @@ -343,7 +370,7 @@ usm_encode(struct snmp_message *msg, struct ber_element 
> *e)
>   struct ber   ber;
>   struct ber_element  *usm, *a, *res = NULL;
>   void*ptr;
> - char digest[SNMP_USM_DIGESTLEN];
> + char digest[SNMP_USM_MAXDIGESTLEN];
>   size_t   digestlen, saltlen;
>   ssize_t  len;
>  
> @@ -362,7 +389,7 @@ usm_encode(struct snmp_message *msg, struct ber_element 
> *e)
>   assert(msg->sm_user != NULL);
>  #endif
>   bzero(digest, sizeof(digest));
> - digestlen = sizeof(digest);
> + digestlen = usm_get_digestlen(msg->sm_user->uu_auth);
>   } else
>   digestlen = 0;
>  
> @@ -456,6 +483,7 @@ usm_finalize_digest(struct snmp_message *msg, char *buf, 
> ssize_t len)
>  {
>   const EVP_MD*md;
>   u_char   digest[EVP_MAX_MD_SIZE];
> + size_t   digestlen;
>   unsigned hlen;
>  
>   if (msg->sm_resp == NULL ||
> @@ -464,10 +492,13 @@ usm_finalize_digest(struct snmp_message *msg, char 
> *buf, ssize_t len)
>   msg->sm_digest_offs == 0 ||
>   len <= 0)
>   return;
> - bzero(digest, SNMP_USM_DIGESTLEN);
> +
> + if ((digestlen = usm_get_digestlen(msg->sm_user->uu_auth)) == 0)
> + return;
> + bzero(digest, digestlen);
>  #ifdef DEBUG
> - assert(msg->sm_digest_offs + SNMP_USM_DIGESTLEN <= (size_t)len);
> - assert(!memcmp(buf + msg->sm_digest_offs, digest, SNMP_USM_DIGESTLEN));
> + assert(msg->sm_digest_offs + digestlen <= (size_t)len);
> + assert(!memcmp(buf + msg->sm_digest_offs, digest, digestlen));
>  #endif
>  
>   if ((md = usm_get_md(msg->sm_user->uu_auth)) == NULL)
> @@ -476,7 +507,7 @@ usm_finalize_digest(struct snmp_message *msg, char *buf, 
> ssize_t len)
>   HMAC(md, msg->sm_user->uu_authkey, (int)msg->sm_user->uu_authkeylen,
>   (u_char*)buf, (size_t)len, digest, );
>  
> - memcpy(buf + msg->sm_digest_offs, digest, SNMP_USM_DIGESTLEN);
> + memcpy(buf + msg->sm_digest_offs, digest, digestlen);
>   return;
>  }
>  
> @@ -506,7 +537,7 @@ usm_valid_digest(struct snmp_message *msg, off_t offs,
>   if (!MSG_HAS_AUTH(msg))
>   return 1;
>  
> - if (digestlen != SNMP_USM_DIGESTLEN)
> + if (digestlen != usm_get_digestlen(msg->sm_user->uu_auth))
>  

Re: if_umb.c: typos

[Gerhard Roth]

Hello Ingo,

I must apologize for my sticky fingers and lots of copy & pasting :/
Thanks for finding.

ok gerhard@


On Mon, 14 Jan 2019 14:21:37 +0100 Ingo Feinerer  wrote:
> A few messsage -> message fixes.
> 
> Index: if_umb.c
> ===
> RCS file: /cvs/src/sys/dev/usb/if_umb.c,v
> retrieving revision 1.21
> diff -u -p -r1.21 if_umb.c
> --- if_umb.c  2 Oct 2018 19:49:10 -   1.21
> +++ if_umb.c  14 Jan 2019 13:16:50 -
> @@ -1197,7 +1197,7 @@ umb_decode_response(struct umb_softc *sc
>   umb_command_done(sc, response, len);
>   break;
>   default:
> - DPRINTF("%s: discard messsage %s\n", DEVNAM(sc),
> + DPRINTF("%s: discard message %s\n", DEVNAM(sc),
>   umb_request2str(type));
>   break;
>   }
> @@ -1211,19 +1211,19 @@ umb_handle_indicate_status_msg(struct um
>   uint32_t cid;
>  
>   if (len < sizeof (*m)) {
> - DPRINTF("%s: discard short %s messsage\n", DEVNAM(sc),
> + DPRINTF("%s: discard short %s message\n", DEVNAM(sc),
>   umb_request2str(letoh32(m->hdr.type)));
>   return;
>   }
>   if (memcmp(m->devid, umb_uuid_basic_connect, sizeof (m->devid))) {
> - DPRINTF("%s: discard %s messsage for other UUID '%s'\n",
> + DPRINTF("%s: discard %s message for other UUID '%s'\n",
>   DEVNAM(sc), umb_request2str(letoh32(m->hdr.type)),
>   umb_uuid2str(m->devid));
>   return;
>   }
>   infolen = letoh32(m->infolen);
>   if (len < sizeof (*m) + infolen) {
> - DPRINTF("%s: discard truncated %s messsage (want %d, got %d)\n",
> + DPRINTF("%s: discard truncated %s message (want %d, got %d)\n",
>   DEVNAM(sc), umb_request2str(letoh32(m->hdr.type)),
>   (int)sizeof (*m) + infolen, len);
>   return;
> @@ -2333,7 +2333,7 @@ umb_command_done(struct umb_softc *sc, v
>   int  qmimsg = 0;
>  
>   if (len < sizeof (*cmd)) {
> - DPRINTF("%s: discard short %s messsage\n", DEVNAM(sc),
> + DPRINTF("%s: discard short %s message\n", DEVNAM(sc),
>   umb_request2str(letoh32(cmd->hdr.type)));
>   return;
>   }
> @@ -2341,7 +2341,7 @@ umb_command_done(struct umb_softc *sc, v
>   if (memcmp(cmd->devid, umb_uuid_basic_connect, sizeof (cmd->devid))) {
>   if (memcmp(cmd->devid, umb_uuid_qmi_mbim,
>   sizeof (cmd->devid))) {
> - DPRINTF("%s: discard %s messsage for other UUID '%s'\n",
> + DPRINTF("%s: discard %s message for other UUID '%s'\n",
>   DEVNAM(sc), umb_request2str(letoh32(cmd->hdr.type)),
>   umb_uuid2str(cmd->devid));
>   return;
> @@ -2370,7 +2370,7 @@ umb_command_done(struct umb_softc *sc, v
>  
>   infolen = letoh32(cmd->infolen);
>   if (len < sizeof (*cmd) + infolen) {
> - DPRINTF("%s: discard truncated %s messsage (want %d, got %d)\n",
> + DPRINTF("%s: discard truncated %s message (want %d, got %d)\n",
>   DEVNAM(sc), umb_cid2str(cid),
>   (int)sizeof (*cmd) + infolen, len);
>   return;

Bogus assertwaitok() in usb_block_allocmem()

[Gerhard Roth]

usb_block_allocmem() does not sleep and is careful to always use the
BUS_DMA_NOWAIT flag. So why the assertwaitok()?


Gerhard


Index: sys/dev/usb/usb_mem.c
===
RCS file: /cvs/src/sys/dev/usb/usb_mem.c,v
retrieving revision 1.31
diff -u -p -u -p -r1.31 usb_mem.c
--- sys/dev/usb/usb_mem.c   18 Nov 2018 16:33:26 -  1.31
+++ sys/dev/usb/usb_mem.c   5 Dec 2018 10:53:39 -
@@ -108,8 +108,6 @@ usb_block_allocmem(bus_dma_tag_t tag, si
}
splx(s);
 
-   assertwaitok();
-
DPRINTFN(6, ("usb_block_allocmem: no free\n"));
p = malloc(sizeof *p, M_USB, M_NOWAIT);
if (p == NULL)

Re: umsm(4) and umb(4) separate loading for the same composite USB modem device

[Gerhard Roth]

On Thu, 16 Aug 2018 13:56:13 +0300 Denis  wrote:
> I can change AT!UDUSBCOMP modes for MC7304 and MC7455 I have in production.
> 
> But how to make full dump of all the USB device descriptors for each
> UDUSBCOMP mode? Can I make it by usbdevs - or how?

Hi Denis,

no that won't work. You could switch the module to each one of the supported
modes and query the descriptors. Unfortunately, some of the modes are one
way streets, i.e. for the offered APIs of the mode there is no known method
to change it back again to some different mode (although I'm quite sure that
Sierra Wireless knows how to do it).

So to get that information, it's much easier to read the documentation:
https://source.sierrawireless.com/resources/airprime/minicard/airprime_mc73xx_usb_driver_developers_guide/#
 (registration required).
In chapter 3.1 "AirPrime MC73xx USB Interfaces" you'll find what you're
looking for.

Gerhard

> 
> Denis
> 
> On 8/15/2018 5:41 PM, Mark Kettenis wrote:
> >> Date: Wed, 15 Aug 2018 09:56:50 +0100
> >> From: Stuart Henderson 
> >>
> >> On 2018/08/14 18:43, Bryan Vyhmeister wrote:  
> >>> On Tue, Aug 14, 2018 at 05:53:43PM +0300, Denis wrote:  
>  Most of modern modems have serial discipline ports and USB Mobile
>  Broadband Interface Model (MBIM) interface in some port compositions
>  simultaneously. It seems very useful to have different disciplines
>  supported by umsm(4) and umb(4) drivers on the same device.
>   
> >>>   
> 
>  Does it possible to have simultaneously operated AT + NMEA ports by
>  umsm(4)driver and MBIM interface by umb(4) driver on the same MC7304
>  device in 6.3?  
> >>>
> >>> What is the advantage in having a device attach to both umsm(4) and
> >>> umb(4)? What are you trying to accomplish? The EM7455 worked perfectly
> >>> with umb(4) until your previous umsm(4) diff and now it only attaches as
> >>> umsm(4). Are you wanting to send SMS messages or something like that
> >>> with your devices?
> >>>
> >>> Bryan
> >>>  
> >>
> >> Denis has a good point because umsm is needed for GPS and as you
> >> suggest SMS.
> >>
> >> What determines which driver attaches when a device is supported by
> >> multiple drivers? Perhaps the simplest option without more complex work
> >> to support different interfaces on different drivers would be to have
> >> the device attach to umb by default but attach to umsm instead if umb is
> >> disabled in the kernel. Then at least a standard kernel could be used
> >> with "disable umb" from boot config.  
> > 
> > The return value from the "match" function determines which driver
> > attaches.  The driver that returns the highest value wins.
> > 
> > However, matching for USB devices is complicated.  Drivers already use
> > different return values (the UMATCH_* constants).  On top of that
> > drivers can claim a whole device or claim just a particular interface
> > of a device.  This requires some careful though to make sure the right
> > driver attaches.
> > 
> > What we really need is a full dump of the usb device descriptors,
> > preferably in all the different UDUSBCOMP modes.
> >   

Re: [patch] Add kvm_close in mib_hrsystemprocs function

[Gerhard Roth]

On Thu, 31 May 2018 17:40:36 +0800 Nan Xiao  wrote:
> Hi Gerhard,
> 
> Thanks for your reply!
> 
> Yes, if no "kvm_close(kd);", there will be resource (memory, file
> descriptor) leak. So hope you can commit it, thanks!
> 
> 
> On 5/30/2018 4:49 PM, Gerhard Roth wrote:
> > On Wed, 30 May 2018 16:25:55 +0800 Nan Xiao  wrote:  
> >> Hi tech@,
> >>
> >> Maybe kvm_close is needed if kvm_getprocs returns NULL here? Sorry if I
> >> am wrong, thanks!
> >>
> >> Index: mib.c
> >> ===
> >> RCS file: /cvs/src/usr.sbin/snmpd/mib.c,v
> >> retrieving revision 1.87
> >> diff -u -p -r1.87 mib.c
> >> --- mib.c  25 May 2018 08:23:15 -  1.87
> >> +++ mib.c  30 May 2018 08:15:19 -
> >> @@ -516,8 +516,10 @@ mib_hrsystemprocs(struct oid *oid, struc
> >>return (-1);
> >>
> >>if (kvm_getprocs(kd, KERN_PROC_ALL, 0,
> >> -  sizeof(struct kinfo_proc), ) == NULL)
> >> +  sizeof(struct kinfo_proc), ) == NULL) {
> >> +  kvm_close(kd);
> >>return (-1);
> >> +  }
> >>
> >>*elm = ber_add_integer(*elm, val);
> >>ber_set_header(*elm, BER_CLASS_APPLICATION, SNMP_T_GAUGE32);
> >>  
> > 
> > 
> > Looks reasonable.
> >   
> 


Reluctant to commit code with my own ok. Anybody else willing to
give an ok?

Gerhard

Re: [patch] Add kvm_close in mib_hrsystemprocs function

[Gerhard Roth]

On Wed, 30 May 2018 16:25:55 +0800 Nan Xiao  wrote:
> Hi tech@,
> 
> Maybe kvm_close is needed if kvm_getprocs returns NULL here? Sorry if I
> am wrong, thanks!
> 
> Index: mib.c
> ===
> RCS file: /cvs/src/usr.sbin/snmpd/mib.c,v
> retrieving revision 1.87
> diff -u -p -r1.87 mib.c
> --- mib.c 25 May 2018 08:23:15 -  1.87
> +++ mib.c 30 May 2018 08:15:19 -
> @@ -516,8 +516,10 @@ mib_hrsystemprocs(struct oid *oid, struc
>   return (-1);
> 
>   if (kvm_getprocs(kd, KERN_PROC_ALL, 0,
> - sizeof(struct kinfo_proc), ) == NULL)
> + sizeof(struct kinfo_proc), ) == NULL) {
> + kvm_close(kd);
>   return (-1);
> + }
> 
>   *elm = ber_add_integer(*elm, val);
>   ber_set_header(*elm, BER_CLASS_APPLICATION, SNMP_T_GAUGE32);
> 


Looks reasonable.

Re: Speed up snmpwalk

[Gerhard Roth]

On Tue, 22 May 2018 11:05:48 +0200 Claudio Jeker <cje...@diehard.n-r-g.com> 
wrote:
> On Tue, May 22, 2018 at 10:26:30AM +0200, Gerhard Roth wrote:
> > Hi,
> > 
> > a snmpwalk of HOST-RESOURCES-MIB::hrSWRunTable doesn't scale very well
> > with an increasing number of running processes.
> > 
> > For every process and each of the 7 elements of the table, mib_hrswrun()
> > would call kinfo_proc() which queried all the processes running on the
> > system and sort them by pid.
> > 
> > The patch below keeps the results cached and updates the list of processes
> > at maximum once every 5 seconds.
> > 
> > Gerhard
> >   
> 
> Agreed, two minor nits inline
> 
> > 
> > Index: usr.sbin/snmpd/mib.c
> > ===
> > RCS file: /cvs/src/usr.sbin/snmpd/mib.c,v
> > retrieving revision 1.86
> > diff -u -p -u -p -r1.86 mib.c
> > --- usr.sbin/snmpd/mib.c9 May 2018 13:56:46 -   1.86
> > +++ usr.sbin/snmpd/mib.c22 May 2018 08:17:16 -
> > @@ -861,11 +861,18 @@ int
> >  kinfo_proc(u_int32_t idx, struct kinfo_proc **kinfo)
> >  {
> > static struct kinfo_proc *kp = NULL;
> > -   static size_tnkp = 0;
> > +   static struct kinfo_proc **klist = NULL;
> > +   static size_tnkp = 0, nklist = 0;
> > +   static time_ttinfo = 0;
> > int  mib[] = { CTL_KERN, KERN_PROC,
> > KERN_PROC_ALL, 0, sizeof(*kp), 0 };
> > -   struct kinfo_proc   **klist;
> > +   struct kinfo_proc**knew;
> > size_t   size, count, i;
> > +   time_t   now;
> > +
> > +   (void)time();  
> 
> I think using clock_gettime(CLOCK_MONOTONIC, ...) would be prefered here.
> Just in case the clock jumps after snmpd was started.
> 
> > +   if (now - tinfo < 5 && kp != NULL && klist != NULL)
> > +   goto cached;
> >  
> > for (;;) {
> > size = nkp * sizeof(*kp);
> > @@ -892,23 +899,26 @@ kinfo_proc(u_int32_t idx, struct kinfo_p
> > }
> > nkp = count;
> > }
> > +   tinfo = now;
> >  
> > -   klist = calloc(count, sizeof(*klist));
> > -   if (klist == NULL)
> > +   knew = recallocarray(klist, nklist, count, sizeof(*klist));
> > +   if (knew == NULL)
> > return (-1);
> > +   klist = knew;
> > +   nklist = count;
> >  
> > -   for (i = 0; i < count; i++)
> > +   for (i = 0; i < nklist; i++)
> > klist[i] = [i];
> > -   qsort(klist, count, sizeof(*klist), kinfo_proc_comp);
> > +   qsort(klist, nklist, sizeof(*klist), kinfo_proc_comp);
> >  
> > +cached:
> > *kinfo = NULL;
> > -   for (i = 0; i < count; i++) {
> > +   for (i = 0; i < nklist; i++) {
> > if (klist[i]->p_pid >= (int32_t)idx) {
> > *kinfo = klist[i];
> > break;
> > }
> > }
> > -   free(klist);  
> 
> IIRC snmpd is also doing a big cleanup round on exit like some other
> daemons, this will result in a "leak" there. IMO this is fine. Just wanted
> to point out. I think a cleaner approach would be to run a timer, that
> frees klist after 5 seconds and the lookup code would then just skip the
> fetching if the pointer is != NULL. Also has the benefit that large
> process tables are not cached for a long time if there are no requests
> anymore.
> 
> > return (0);
> >  }
> >   
> 



So here's an updated diff that uses a timer to purge the cache after
5 seconds:


Index: usr.sbin/snmpd/mib.c
===
RCS file: /cvs/src/usr.sbin/snmpd/mib.c,v
retrieving revision 1.86
diff -u -p -u -p -r1.86 mib.c
--- usr.sbin/snmpd/mib.c9 May 2018 13:56:46 -   1.86
+++ usr.sbin/snmpd/mib.c22 May 2018 09:57:57 -
@@ -414,6 +414,7 @@ int  mib_hrswrun(struct oid *, struct be
 
 int kinfo_proc_comp(const void *, const void *);
 int kinfo_proc(u_int32_t, struct kinfo_proc **);
+voidkinfo_proc_free(void);
 int kinfo_args(struct kinfo_proc *, char **);
 
 static struct oid hr_mib[] = {
@@ -857,24 +858,29 @@ kinfo_proc_comp(const void *a, const voi
return (((*k1)->p_pid > (*k2)->p_pid) ? 1 : -1);
 }
 
+static struct event  kinfo_timer;
+static struct kinfo_proc *kp = NULL;
+static struct kinfo_proc **klist = NULL;
+static size_tnkp = 0, nklist = 0;
+
 int
 kinfo_proc(u_int32_t idx, struc

Speed up snmpwalk

[Gerhard Roth]

Hi,

a snmpwalk of HOST-RESOURCES-MIB::hrSWRunTable doesn't scale very well
with an increasing number of running processes.

For every process and each of the 7 elements of the table, mib_hrswrun()
would call kinfo_proc() which queried all the processes running on the
system and sort them by pid.

The patch below keeps the results cached and updates the list of processes
at maximum once every 5 seconds.

Gerhard


Index: usr.sbin/snmpd/mib.c
===
RCS file: /cvs/src/usr.sbin/snmpd/mib.c,v
retrieving revision 1.86
diff -u -p -u -p -r1.86 mib.c
--- usr.sbin/snmpd/mib.c9 May 2018 13:56:46 -   1.86
+++ usr.sbin/snmpd/mib.c22 May 2018 08:17:16 -
@@ -861,11 +861,18 @@ int
 kinfo_proc(u_int32_t idx, struct kinfo_proc **kinfo)
 {
static struct kinfo_proc *kp = NULL;
-   static size_tnkp = 0;
+   static struct kinfo_proc **klist = NULL;
+   static size_tnkp = 0, nklist = 0;
+   static time_ttinfo = 0;
int  mib[] = { CTL_KERN, KERN_PROC,
KERN_PROC_ALL, 0, sizeof(*kp), 0 };
-   struct kinfo_proc   **klist;
+   struct kinfo_proc**knew;
size_t   size, count, i;
+   time_t   now;
+
+   (void)time();
+   if (now - tinfo < 5 && kp != NULL && klist != NULL)
+   goto cached;
 
for (;;) {
size = nkp * sizeof(*kp);
@@ -892,23 +899,26 @@ kinfo_proc(u_int32_t idx, struct kinfo_p
}
nkp = count;
}
+   tinfo = now;
 
-   klist = calloc(count, sizeof(*klist));
-   if (klist == NULL)
+   knew = recallocarray(klist, nklist, count, sizeof(*klist));
+   if (knew == NULL)
return (-1);
+   klist = knew;
+   nklist = count;
 
-   for (i = 0; i < count; i++)
+   for (i = 0; i < nklist; i++)
klist[i] = [i];
-   qsort(klist, count, sizeof(*klist), kinfo_proc_comp);
+   qsort(klist, nklist, sizeof(*klist), kinfo_proc_comp);
 
+cached:
*kinfo = NULL;
-   for (i = 0; i < count; i++) {
+   for (i = 0; i < nklist; i++) {
if (klist[i]->p_pid >= (int32_t)idx) {
*kinfo = klist[i];
break;
}
}
-   free(klist);
 
return (0);
 }

Re: close filedescriptors of children

[Gerhard Roth]

If proc_init() knows about debug mode, we can move the call to daemon(3)
into proc_init(). Then only the parent calls daemon(3). The children will
inherit stdin/out/err from the parent and don't have to do anything.
And since the children don't call daemon(3) themselves anymore, there
won't be any zombies.

Below is an updated, more simpler patch.

The only problem here is that half of the daemons set the 'nochdir'
flag of daemon(3), while the other half doesn't. Hence the proc.c files
differ in the arguments passed to daemon(3). If this is a problem, I
can make this a parameter passed to proc_init().


Gerhard


Index: usr.sbin/httpd/httpd.c
===
RCS file: /cvs/src/usr.sbin/httpd/httpd.c,v
retrieving revision 1.67
diff -u -p -u -p -r1.67 httpd.c
--- usr.sbin/httpd/httpd.c  28 May 2017 10:37:26 -  1.67
+++ usr.sbin/httpd/httpd.c  8 Mar 2018 09:17:07 -
@@ -215,11 +215,9 @@ main(int argc, char *argv[])
}
 
/* only the parent returns */
-   proc_init(ps, procs, nitems(procs), argc0, argv, proc_id);
+   proc_init(ps, procs, nitems(procs), argc0, argv, proc_id, debug);
 
log_procinit("parent");
-   if (!debug && daemon(1, 0) == -1)
-   err(1, "failed to daemonize");
 
if (ps->ps_noaction == 0)
log_info("startup");
Index: usr.sbin/httpd/httpd.h
===
RCS file: /cvs/src/usr.sbin/httpd/httpd.h,v
retrieving revision 1.135
diff -u -p -u -p -r1.135 httpd.h
--- usr.sbin/httpd/httpd.h  7 Feb 2018 03:28:05 -   1.135
+++ usr.sbin/httpd/httpd.h  7 Mar 2018 15:49:30 -
@@ -761,7 +761,7 @@ __dead void fatalx(const char *, ...)
 enum privsep_procid
proc_getid(struct privsep_proc *, unsigned int, const char *);
 voidproc_init(struct privsep *, struct privsep_proc *, unsigned int,
-   int, char **, enum privsep_procid);
+   int, char **, enum privsep_procid, int);
 voidproc_kill(struct privsep *);
 voidproc_connect(struct privsep *);
 voidproc_dispatch(int, short event, void *);
Index: usr.sbin/httpd/proc.c
===
RCS file: /cvs/src/usr.sbin/httpd/proc.c,v
retrieving revision 1.37
diff -u -p -u -p -r1.37 proc.c
--- usr.sbin/httpd/proc.c   28 May 2017 10:37:26 -  1.37
+++ usr.sbin/httpd/proc.c   8 Mar 2018 09:17:46 -
@@ -191,7 +191,7 @@ proc_connect(struct privsep *ps)
 
 void
 proc_init(struct privsep *ps, struct privsep_proc *procs, unsigned int nproc,
-int argc, char **argv, enum privsep_procid proc_id)
+int argc, char **argv, enum privsep_procid proc_id, int debug)
 {
struct privsep_proc *p = NULL;
struct privsep_pipes*pa, *pb;
@@ -205,6 +205,8 @@ proc_init(struct privsep *ps, struct pri
 
if (proc_id == PROC_PARENT) {
privsep_process = PROC_PARENT;
+   if (!debug && daemon(1, 0) == -1)
+   fatal("failed to daemonize");
proc_setup(ps, procs, nproc);
 
/*
Index: usr.sbin/relayd/proc.c
===
RCS file: /cvs/src/usr.sbin/relayd/proc.c,v
retrieving revision 1.39
diff -u -p -u -p -r1.39 proc.c
--- usr.sbin/relayd/proc.c  28 May 2017 10:39:15 -  1.39
+++ usr.sbin/relayd/proc.c  8 Mar 2018 09:19:41 -
@@ -191,7 +191,7 @@ proc_connect(struct privsep *ps)
 
 void
 proc_init(struct privsep *ps, struct privsep_proc *procs, unsigned int nproc,
-int argc, char **argv, enum privsep_procid proc_id)
+int argc, char **argv, enum privsep_procid proc_id, int debug)
 {
struct privsep_proc *p = NULL;
struct privsep_pipes*pa, *pb;
@@ -205,6 +205,8 @@ proc_init(struct privsep *ps, struct pri
 
if (proc_id == PROC_PARENT) {
privsep_process = PROC_PARENT;
+   if (!debug && daemon(1, 0) == -1)
+   fatal("failed to daemonize");
proc_setup(ps, procs, nproc);
 
/*
Index: usr.sbin/relayd/relayd.c
===
RCS file: /cvs/src/usr.sbin/relayd/relayd.c,v
retrieving revision 1.171
diff -u -p -u -p -r1.171 relayd.c
--- usr.sbin/relayd/relayd.c29 Nov 2017 15:24:50 -  1.171
+++ usr.sbin/relayd/relayd.c8 Mar 2018 09:18:21 -
@@ -212,11 +212,9 @@ main(int argc, char *argv[])
ps->ps_title[proc_id] = title;
 
/* only the parent returns */
-   proc_init(ps, procs, nitems(procs), argc0, argv, proc_id);
+   proc_init(ps, procs, nitems(procs), argc0, argv, proc_id, debug);
 
log_procinit("parent");
-   if (!debug && daemon(1, 0) == -1)
-   err(1, "failed to daemonize");
 
if (ps->ps_noaction == 0)
log_info("startup");
Index: 

Re: close filedescriptors of children

[Gerhard Roth]

On Wed, 7 Mar 2018 17:20:06 +0100 Mike Belopuhov <m...@belopuhov.com> wrote:
> On 7 March 2018 at 17:01, Gerhard Roth <gerhard_r...@genua.de> wrote:
> >
> > Hi Benno,
> >
> > thanks for your reply.
> >
> > On Wed, 7 Mar 2018 15:22:28 +0100 Sebastian Benoit <be...@openbsd.org>  
> wrote:
> > > Hi,
> > >
> > > switchd and vmd use the same proc.c,and should stay in sync.  
> >
> > Ack. I missed them.
> >  
> 
> iked also uses proc.c. I think you've got all the others,
> but perhaps you should run a find?
> 
> Cheers,
> Mike

Hi Mike,

but iked still uses an older version of proc.c that just forks off
the children but does not execve() the own binary.

Also, iked is the only one that daemon(3)-izes before calling
proc_init(). So here stdout, stdin, and stderr is already remapped
to /dev/null before forking the kids.

Gerhard

Re: close filedescriptors of children

[Gerhard Roth]

Hi Benno,

thanks for your reply.

On Wed, 7 Mar 2018 15:22:28 +0100 Sebastian Benoit  wrote:
> Hi,
> 
> switchd and vmd use the same proc.c,and should stay in sync.

Ack. I missed them.


> 
> Also, this breaks -dvv (i.e. debug output when running inthe foreground),
> at least for relayd.

Stupid me, indeed.

> 
> /Benno
> 

Below is an updated patch that includes proc.c of switchd and vmd.
It also passes the 'debug' flag to proc_init() so that it won't touch
std* in that case.

I was wandering if this would be a good idea to move the daemon(3)
call into the PROC_PARENT case of proc_init(), too?


Gerhard


Index: usr.sbin/httpd/httpd.c
===
RCS file: /cvs/src/usr.sbin/httpd/httpd.c,v
retrieving revision 1.67
diff -u -p -u -p -r1.67 httpd.c
--- usr.sbin/httpd/httpd.c  28 May 2017 10:37:26 -  1.67
+++ usr.sbin/httpd/httpd.c  7 Mar 2018 15:49:47 -
@@ -215,7 +215,7 @@ main(int argc, char *argv[])
}
 
/* only the parent returns */
-   proc_init(ps, procs, nitems(procs), argc0, argv, proc_id);
+   proc_init(ps, procs, nitems(procs), argc0, argv, proc_id, debug);
 
log_procinit("parent");
if (!debug && daemon(1, 0) == -1)
Index: usr.sbin/httpd/httpd.h
===
RCS file: /cvs/src/usr.sbin/httpd/httpd.h,v
retrieving revision 1.135
diff -u -p -u -p -r1.135 httpd.h
--- usr.sbin/httpd/httpd.h  7 Feb 2018 03:28:05 -   1.135
+++ usr.sbin/httpd/httpd.h  7 Mar 2018 15:49:30 -
@@ -761,7 +761,7 @@ __dead void fatalx(const char *, ...)
 enum privsep_procid
proc_getid(struct privsep_proc *, unsigned int, const char *);
 voidproc_init(struct privsep *, struct privsep_proc *, unsigned int,
-   int, char **, enum privsep_procid);
+   int, char **, enum privsep_procid, int);
 voidproc_kill(struct privsep *);
 voidproc_connect(struct privsep *);
 voidproc_dispatch(int, short event, void *);
Index: usr.sbin/httpd/proc.c
===
RCS file: /cvs/src/usr.sbin/httpd/proc.c,v
retrieving revision 1.37
diff -u -p -u -p -r1.37 proc.c
--- usr.sbin/httpd/proc.c   28 May 2017 10:37:26 -  1.37
+++ usr.sbin/httpd/proc.c   7 Mar 2018 15:50:06 -
@@ -27,6 +27,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -191,13 +192,14 @@ proc_connect(struct privsep *ps)
 
 void
 proc_init(struct privsep *ps, struct privsep_proc *procs, unsigned int nproc,
-int argc, char **argv, enum privsep_procid proc_id)
+int argc, char **argv, enum privsep_procid proc_id, int debug)
 {
struct privsep_proc *p = NULL;
struct privsep_pipes*pa, *pb;
unsigned int proc;
unsigned int dst;
int  fds[2];
+   int  fd;
 
/* Don't initiate anything if we are not really going to run. */
if (ps->ps_noaction)
@@ -246,6 +248,13 @@ proc_init(struct privsep *ps, struct pri
fatalx("%s: process %d missing process initialization",
__func__, proc_id);
 
+   if (!debug && (fd = open(_PATH_DEVNULL, O_RDWR, 0)) != -1) {
+   dup2(fd, STDIN_FILENO);
+   dup2(fd, STDOUT_FILENO);
+   dup2(fd, STDERR_FILENO);
+   if (fd > 2)
+   close(fd);
+   }
p->p_init(ps, p);
 
fatalx("failed to initiate child process");
Index: usr.sbin/relayd/proc.c
===
RCS file: /cvs/src/usr.sbin/relayd/proc.c,v
retrieving revision 1.39
diff -u -p -u -p -r1.39 proc.c
--- usr.sbin/relayd/proc.c  28 May 2017 10:39:15 -  1.39
+++ usr.sbin/relayd/proc.c  7 Mar 2018 15:43:03 -
@@ -27,6 +27,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -191,13 +192,14 @@ proc_connect(struct privsep *ps)
 
 void
 proc_init(struct privsep *ps, struct privsep_proc *procs, unsigned int nproc,
-int argc, char **argv, enum privsep_procid proc_id)
+int argc, char **argv, enum privsep_procid proc_id, int debug)
 {
struct privsep_proc *p = NULL;
struct privsep_pipes*pa, *pb;
unsigned int proc;
unsigned int dst;
int  fds[2];
+   int  fd;
 
/* Don't initiate anything if we are not really going to run. */
if (ps->ps_noaction)
@@ -246,6 +248,13 @@ proc_init(struct privsep *ps, struct pri
fatalx("%s: process %d missing process initialization",
__func__, proc_id);
 
+   if (!debug && (fd = open(_PATH_DEVNULL, O_RDWR, 0)) != -1) {
+   dup2(fd, STDIN_FILENO);
+   dup2(fd, STDOUT_FILENO);
+   

close filedescriptors of children

[Gerhard Roth]

Hi,

proc_init() is done before daemon() and for the child processes of httpd,
relayd and snmpd() this function never returns. That means that the
children inherit stdin, stdout, and stderr of the caller and never close
them.

This fix this, proc_init() should map these filedes to /dev/null for a
child. The code is simpled and copied from deamon(3), without the lintish
(void) casts.

Gerhard


Index: usr.sbin/httpd/proc.c
===
RCS file: /cvs/src/usr.sbin/httpd/proc.c,v
retrieving revision 1.37
diff -u -p -u -p -r1.37 proc.c
--- usr.sbin/httpd/proc.c   28 May 2017 10:37:26 -  1.37
+++ usr.sbin/httpd/proc.c   7 Mar 2018 12:31:11 -
@@ -27,6 +27,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -198,6 +199,7 @@ proc_init(struct privsep *ps, struct pri
unsigned int proc;
unsigned int dst;
int  fds[2];
+   int  fd;
 
/* Don't initiate anything if we are not really going to run. */
if (ps->ps_noaction)
@@ -246,6 +248,13 @@ proc_init(struct privsep *ps, struct pri
fatalx("%s: process %d missing process initialization",
__func__, proc_id);
 
+   if ((fd = open(_PATH_DEVNULL, O_RDWR, 0)) != -1) {
+   dup2(fd, STDIN_FILENO);
+   dup2(fd, STDOUT_FILENO);
+   dup2(fd, STDERR_FILENO);
+   if (fd > 2)
+   close(fd);
+   }
p->p_init(ps, p);
 
fatalx("failed to initiate child process");
Index: usr.sbin/relayd/proc.c
===
RCS file: /cvs/src/usr.sbin/relayd/proc.c,v
retrieving revision 1.39
diff -u -p -u -p -r1.39 proc.c
--- usr.sbin/relayd/proc.c  28 May 2017 10:39:15 -  1.39
+++ usr.sbin/relayd/proc.c  7 Mar 2018 12:32:28 -
@@ -27,6 +27,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -198,6 +199,7 @@ proc_init(struct privsep *ps, struct pri
unsigned int proc;
unsigned int dst;
int  fds[2];
+   int  fd;
 
/* Don't initiate anything if we are not really going to run. */
if (ps->ps_noaction)
@@ -246,6 +248,13 @@ proc_init(struct privsep *ps, struct pri
fatalx("%s: process %d missing process initialization",
__func__, proc_id);
 
+   if ((fd = open(_PATH_DEVNULL, O_RDWR, 0)) != -1) {
+   dup2(fd, STDIN_FILENO);
+   dup2(fd, STDOUT_FILENO);
+   dup2(fd, STDERR_FILENO);
+   if (fd > 2)
+   close(fd);
+   }
p->p_init(ps, p);
 
fatalx("failed to initiate child process");
Index: usr.sbin/snmpd/proc.c
===
RCS file: /cvs/src/usr.sbin/snmpd/proc.c,v
retrieving revision 1.24
diff -u -p -u -p -r1.24 proc.c
--- usr.sbin/snmpd/proc.c   29 May 2017 12:56:26 -  1.24
+++ usr.sbin/snmpd/proc.c   7 Mar 2018 12:34:02 -
@@ -27,6 +27,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -198,6 +199,7 @@ proc_init(struct privsep *ps, struct pri
unsigned int proc;
unsigned int dst;
int  fds[2];
+   int  fd;
 
/* Don't initiate anything if we are not really going to run. */
if (ps->ps_noaction)
@@ -246,6 +248,13 @@ proc_init(struct privsep *ps, struct pri
fatalx("%s: process %d missing process initialization",
__func__, proc_id);
 
+   if ((fd = open(_PATH_DEVNULL, O_RDWR, 0)) != -1) {
+   dup2(fd, STDIN_FILENO);
+   dup2(fd, STDOUT_FILENO);
+   dup2(fd, STDERR_FILENO);
+   if (fd > 2)
+   close(fd);
+   }
p->p_init(ps, p);
 
fatalx("failed to initiate child process");

Re: [patch] snmpd hrStorageSize negative values

[Gerhard Roth]

On Sat, 25 Nov 2017 11:42:07 -0700 Joel Knight  wrote:
> On Thu, Mar 9, 2017 at 10:02 PM, Joel Knight  wrote:
> > Hi.
> >
> > snmpd(8) uses unsigned ints internally to represent the size and used
> > space of a file system. The HOST-RESOURCES-MIB defines the valid
> > values for those OIDs as 0..2147483647. With sufficiently large file
> > systems, this can cause negative numbers to be returned for the size
> > and used space OIDs.
> >
> > .1.3.6.1.2.1.25.2.3.1.5.36=-1573167768  
> 
> Hi. Just wanted to bump this again and see if anyone that cares about
> snmp could take a look? Looking for oks and someone who wouldn't mind
> committing it.
> 
> 
> > At sthen's suggestion, do what net-snmp does and fiddle with the
> > values to prevent wrapping. Yes this mucks with the actual values of
> > size, used space, and block size, but it allows snmpd to convey the
> > proper size and used space of the file system which is what most
> > everybody is really interested in.
> >
> > In case gmail hoses this diff, it's also here:
> > https://www.packetmischief.ca/files/patches/snmpd.hrstorage2.diff  


Hi Joel,

I think this won't work unless you also change the type of 'size' and
'used' to u_int64_t.

Gerhard

umb(4): recover after temporary loss of packet service

[Gerhard Roth]

In case we have a temporary loss of connection in umb(4), the USB xfers
may time-out. umb_txeof() should always check whether there are further
mbufs in the if_snd queue; not only after successful transmits.

Also, aborting the xfer in case the watchdog timer triggers, can help
to resume from hanging transmits.


To test this fix, flood ping via umb(4) and then move somewhere where
there's no reception. Ping will start spitting out "No buffer space
available" errors after some time. Now go back to where there's
reception. Without this fix, the ping errors will continue (because
umb_start() isn't called anymore). With this fix, they'll go away.


Gerhard


Index: if_umb.c
===
RCS file: /cvs/src/sys/dev/usb/if_umb.c,v
retrieving revision 1.16
diff -u -p -r1.16 if_umb.c
--- if_umb.c20 Oct 2017 09:35:09 -  1.16
+++ if_umb.c23 Oct 2017 08:28:21 -
@@ -896,7 +896,7 @@ umb_watchdog(struct ifnet *ifp)
 
ifp->if_oerrors++;
printf("%s: watchdog timeout\n", DEVNAM(sc));
-   /* XXX FIXME: re-initialize device */
+   usbd_abort_pipe(sc->sc_tx_pipe);
return;
 }
 
@@ -1845,10 +1845,9 @@ umb_txeof(struct usbd_xfer *xfer, void *
if (status == USBD_STALLED)
usbd_clear_endpoint_stall_async(sc->sc_tx_pipe);
}
-   } else {
-   if (IFQ_IS_EMPTY(>if_snd) == 0)
-   umb_start(ifp);
}
+   if (IFQ_IS_EMPTY(>if_snd) == 0)
+   umb_start(ifp);
 
splx(s);
 }

Re: Routing table update on link state change

[Gerhard Roth]

On Tue, 5 Sep 2017 10:47:11 +0200 Martin Pieuchot <m...@openbsd.org> wrote:
> On 04/09/17(Mon) 14:53, Gerhard Roth wrote:
> > Hi Martin,
> > 
> > 
> > On Mon, 4 Sep 2017 14:18:50 +0200 Martin Pieuchot <m...@openbsd.org> wrote:
> > > On 04/09/17(Mon) 13:10, Gerhard Roth wrote:
> > > > Hi,
> > > > 
> > > > I noticed a problem with the routing table that is easy to reproduce: 
> > > > put
> > > > multiple IPs on the same carp interface:
> > > 
> > > Great bug analysis, however returning EAGAIN for every route update is a
> > > no go if you have a big routing table like BGP full feeds.
> > > 
> > > We should return EAGAIN only if the multipath list contains multiple
> > > elements.
> > > 
> > > But since we're now returning EAGAIN much often we want to skip route
> > > entries that have already been borough UP/taken down.
> > > 
> > > Diff below does that, does it work for you?  Do you mind adding a
> > > regression test?
> > 
> > I can confirm that your version works for me. Thanks for the improvement.
> > 
> > Regarding the regression test: gimme some time ;)
> 
> I wrote this test but I can't reproduce your behavior on -current.
> 
> Since I don't see the 'P' flag in your route output I believe you're not
> running current, right?

No, it's current I'm running.


> 
> Can you trigger the bug with this test?  Or can you adjust it to trigger
> the bug?

Jupp. The problem is that the UP flag on the carp routes does not
appear immediately. And then you put the underlying inferface down
before the carp routes had the RTF_UP set.

Put a short sleep before the "ifconfig vether0 down" and you should be
able to spot the bug.

Gerhard


> 
> Index: Makefile
> ===
> RCS file: /cvs/src/regress/sbin/route/Makefile,v
> retrieving revision 1.19
> diff -u -p -r1.19 Makefile
> --- Makefile  10 Aug 2017 13:08:39 -  1.19
> +++ Makefile  5 Sep 2017 08:41:30 -
> @@ -329,6 +329,23 @@ RTTEST_TARGETS+:=rttest${n}
>  rttest${n}:
>   ! ${RCMD} change -expire 30 192.0.2.1 192.0.2.2
>  
> +
> +#
> +n=   30
> +RTTEST_TARGETS+:=rttest${n}
> +rttest${n}:
> + ${SUDO} ifconfig vether0 rdomain ${RDOMAIN} up
> + ${SUDO} ifconfig carp0 rdomain ${RDOMAIN} carpdev vether0 vhid 3 up
> + ${SUDO} ifconfig carp0 inet alias 10.1.255.1/14
> + ${SUDO} ifconfig carp0 inet alias 10.1.255.2/14
> + ${SUDO} ifconfig vether0 10.1.254.56/14
> + ${SUDO} ifconfig carp0 inet alias 10.1.255.3/14
> + ${SUDO} ifconfig vether0 down
> + ${RCMD} -n show -inet
> + ${SUDO} ifconfig carp0 destroy
> + ${SUDO} ifconfig vether0 destroy
> +
> +
>  REGRESS_TARGETS=netmask ${RTTEST_TARGETS}
>  REGRESS_ROOT_TARGETS=${REGRESS_TARGETS}
>  .PHONY: ${REGRESS_TARGETS}

Re: Routing table update on link state change

[Gerhard Roth]

Hi Martin,


On Mon, 4 Sep 2017 14:18:50 +0200 Martin Pieuchot <m...@openbsd.org> wrote:
> On 04/09/17(Mon) 13:10, Gerhard Roth wrote:
> > Hi,
> > 
> > I noticed a problem with the routing table that is easy to reproduce: put
> > multiple IPs on the same carp interface:
> 
> Great bug analysis, however returning EAGAIN for every route update is a
> no go if you have a big routing table like BGP full feeds.
> 
> We should return EAGAIN only if the multipath list contains multiple
> elements.
> 
> But since we're now returning EAGAIN much often we want to skip route
> entries that have already been borough UP/taken down.
> 
> Diff below does that, does it work for you?  Do you mind adding a
> regression test?

I can confirm that your version works for me. Thanks for the improvement.

Regarding the regression test: gimme some time ;)

Gerhard


> 
> Index: net/route.c
> ===
> RCS file: /cvs/src/sys/net/route.c,v
> retrieving revision 1.366
> diff -u -p -r1.366 route.c
> --- net/route.c   11 Aug 2017 21:24:19 -  1.366
> +++ net/route.c   4 Sep 2017 12:08:16 -
> @@ -1550,6 +1550,7 @@ rt_if_linkstate_change(struct rtentry *r
>  {
>   struct ifnet *ifp = arg;
>   struct sockaddr_in6 sa_mask;
> + int error;
>  
>   if (rt->rt_ifidx != ifp->if_index)
>   return (0);
> @@ -1561,38 +1562,37 @@ rt_if_linkstate_change(struct rtentry *r
>   }
>  
>   if (LINK_STATE_IS_UP(ifp->if_link_state) && ifp->if_flags & IFF_UP) {
> - if (!(rt->rt_flags & RTF_UP)) {
> - /* bring route up */
> - rt->rt_flags |= RTF_UP;
> - rtable_mpath_reprio(id, rt_key(rt),
> - rt_plen2mask(rt, _mask),
> - rt->rt_priority & RTP_MASK, rt);
> - }
> + if (ISSET(rt->rt_flags, RTF_UP))
> + return (0);
> +
> + /* bring route up */
> + rt->rt_flags |= RTF_UP;
> + error = rtable_mpath_reprio(id, rt_key(rt),
> + rt_plen2mask(rt, _mask), rt->rt_priority & RTP_MASK, rt);
>   } else {
> - if (rt->rt_flags & RTF_UP) {
> - /*
> -  * Remove redirected and cloned routes (mainly ARP)
> -  * from down interfaces so we have a chance to get
> -  * new routes from a better source.
> -  */
> - if (ISSET(rt->rt_flags, RTF_CLONED|RTF_DYNAMIC) &&
> - !ISSET(rt->rt_flags, RTF_CACHED|RTF_BFD)) {
> - int error;
> -
> - if ((error = rtdeletemsg(rt, ifp, id)))
> - return (error);
> - return (EAGAIN);
> - }
> - /* take route down */
> - rt->rt_flags &= ~RTF_UP;
> - rtable_mpath_reprio(id, rt_key(rt),
> - rt_plen2mask(rt, _mask),
> - rt->rt_priority | RTP_DOWN, rt);
> + /*
> +  * Remove redirected and cloned routes (mainly ARP)
> +  * from down interfaces so we have a chance to get
> +  * new routes from a better source.
> +  */
> + if (ISSET(rt->rt_flags, RTF_CLONED|RTF_DYNAMIC) &&
> + !ISSET(rt->rt_flags, RTF_CACHED|RTF_BFD)) {
> + if ((error = rtdeletemsg(rt, ifp, id)))
> + return (error);
> + return (EAGAIN);
>   }
> +
> + if (!ISSET(rt->rt_flags, RTF_UP))
> + return (0);
> +
> + /* take route down */
> + rt->rt_flags &= ~RTF_UP;
> + error = rtable_mpath_reprio(id, rt_key(rt),
> + rt_plen2mask(rt, _mask), rt->rt_priority | RTP_DOWN, rt);
>   }
>   if_group_routechange(rt_key(rt), rt_plen2mask(rt, _mask));
>  
> - return (0);
> + return (error);
>  }
>  
>  struct sockaddr *
> Index: net/rtable.c
> ===
> RCS file: /cvs/src/sys/net/rtable.c,v
> retrieving revision 1.61
> diff -u -p -r1.61 rtable.c
> --- net/rtable.c  30 Jul 2017 18:18:08 -  1.61
> +++ net/rtable.c  4 Sep 2017 12:02:00 -
> @@ -751,6 +751,7 @@ rtable_mpath_reprio(unsigned int rtablei
>   rt->rt_priority = prio;
>   rtable_mpath_insert(an, rt);
>   rtfree(rt);
> + error = EAGAIN;
>   }
>   rw_exit_write(>ar_lock);
>  

Routing table update on link state change

[Gerhard Roth]

Hi,

I noticed a problem with the routing table that is easy to reproduce: put
multiple IPs on the same carp interface:

# ifconfig em0
em0: flags=8843 mtu 1500
lladdr 00:0a:e4:31:9d:6e
index 1 priority 0 llprio 3
groups: egress
media: Ethernet autoselect (100baseTX full-duplex,rxpause,txpause)
status: active
inet 10.1.254.56 netmask 0xfffc broadcast 10.3.255.255
# ifconfig carp0 up carpdev em0 vhid 3
# ifconfig carp0 inet alias 10.1.255.1 netmask 0xfffc
# ifconfig carp0 inet alias 10.1.255.2 netmask 0xfffc
# ifconfig carp0 inet alias 10.1.255.3 netmask 0xfffc
# route -n show -inet
Routing tables

Internet:
DestinationGatewayFlags   Refs  Use   Mtu  Prio Iface
...
10.0/1410.1.254.56UCn143573 - 4 em0  
10.0/1410.1.255.1 UCn00 -19 carp0
10.0/1410.1.255.3 UCn00 -19 carp0
10.0/1410.1.255.2 UCn00 -19 carp0
...

Now pull the cable on em0 and see what happens:

# route -n show -inet
Routing tables

Internet:
DestinationGatewayFlags   Refs  Use   Mtu  Prio Iface
...
10.0/1410.1.255.3 UCn00 -19 carp0
10.0/1410.1.255.2 UCn00 -19 carp0
10.0/1410.1.254.56Cn 143763 - 4 em0  
10.0/1410.1.255.1 Cn 00 -19 carp0
...


Some of the routes to 10.0/14 on carp0 still have the up ('U') flag set,
but not all of them.

The reason is that rt_if_linkstate_change() calls rtable_mpath_reprio()
and that function reorders the routing table while we're iterating over
it in rt_if_track().

Returning EAGAIN in case rtable_mpath_reprio() was called fixes the
problem.


Gerhard


Index: sys/net/route.c
===
RCS file: /cvs/src/sys/net/route.c,v
retrieving revision 1.366
diff -u -p -u -p -r1.366 route.c
--- sys/net/route.c 11 Aug 2017 21:24:19 -  1.366
+++ sys/net/route.c 4 Sep 2017 08:25:05 -
@@ -1550,14 +1550,15 @@ rt_if_linkstate_change(struct rtentry *r
 {
struct ifnet *ifp = arg;
struct sockaddr_in6 sa_mask;
+   int ret = 0;
 
if (rt->rt_ifidx != ifp->if_index)
-   return (0);
+   return (ret);
 
/* Local routes are always usable. */
if (rt->rt_flags & RTF_LOCAL) {
rt->rt_flags |= RTF_UP;
-   return (0);
+   return (ret);
}
 
if (LINK_STATE_IS_UP(ifp->if_link_state) && ifp->if_flags & IFF_UP) {
@@ -1567,6 +1568,7 @@ rt_if_linkstate_change(struct rtentry *r
rtable_mpath_reprio(id, rt_key(rt),
rt_plen2mask(rt, _mask),
rt->rt_priority & RTP_MASK, rt);
+   ret = EAGAIN;
}
} else {
if (rt->rt_flags & RTF_UP) {
@@ -1588,11 +1590,12 @@ rt_if_linkstate_change(struct rtentry *r
rtable_mpath_reprio(id, rt_key(rt),
rt_plen2mask(rt, _mask),
rt->rt_priority | RTP_DOWN, rt);
+   ret = EAGAIN;
}
}
if_group_routechange(rt_key(rt), rt_plen2mask(rt, _mask));
 
-   return (0);
+   return (ret);
 }
 
 struct sockaddr *




















Script started on Mon Sep  4 10:18:22 2017
bash: warning: setlocale: LC_TIME: cannot change locale (en_US.UTF-8)
bash: warning: setlocale: LC_NUMERIC: cannot change locale (en_US.UTF-8)
]0;gerhard@null: ~gerhard@null> ifconfig em0
em0: flags=8843 mtu 1500
lladdr 00:0a:e4:31:9d:6e
index 1 priority 0 llprio 3
groups: egress
media: Ethernet autoselect (100baseTX full-duplex,rxpause,txpause)
status: active
inet 10.1.254.56 netmask 0xfffc broadcast 10.3.255.255
]0;gerhard@null: ~gerhard@null> ifconfig carp0 up carpdev em0 vhid 3
ifconfig: SIOCGIFFLAGS: Device not configured
]0;gerhard@null: ~gerhard@null> ifconfig carp0 up carpdev em0 vhid 
3[1@s[1@u[1@d[1@^C
]0;gerhard@null: ~gerhard@null> sudo bash
bash-4.4# ifconfig carp0 up carpdev em0 vhid 3
bash-4.4# ifconfig carp0 inet alias 10.1.255.1 netmask 0xfffc
bash-4.4# ifconfig carp0 inet alias 10.1.255.[1@2
bash-4.4# ifconfig carp0 inet alias 10.1.255.2 netmask 
0xfffc[[1@3
bash-4.4# route sho[[[n [[-n show -inet
Routing tables

Internet:
MPATH.before lines 1-23/71 

Re: snmpd: format string for yyerror

[Gerhard Roth]

On Fri, 28 Jul 2017 12:36:22 + Florian Obser  wrote:
> not really a problem, errstr are just various static strings, but still...
> 
> pointed out by clang, OK?
> 
> diff --git snmpd/parse.y snmpd/parse.y
> index efd1159c3ab..cc3d4194556 100644
> --- snmpd/parse.y
> +++ snmpd/parse.y
> @@ -273,14 +273,14 @@ main: LISTEN ON STRING  {
>   const char *errstr;
>   user = usm_newuser($2, );
>   if (user == NULL) {
> - yyerror(errstr);
> + yyerror("%s", errstr);
>   free($2);
>   YYERROR;
>   }
>   } userspecs {
>   const char *errstr;
>   if (usm_checkuser(user, ) < 0) {
> - yyerror(errstr);
> + yyerror("%s", errstr);
>   YYERROR;
>   }
>   user = NULL;
> 
> 


Definitely an improvement! ok gerhard@

Re: snmpd: engine id is just a binary string?

[Gerhard Roth]

On Fri, 28 Jul 2017 12:39:48 + Florian Obser  wrote:
> Not sure about this one, a quick glance at RFC 3411 suggests this
> is just a binary string, so uint8_t is more appropriate.
> 
> Any snmp nerds around?
> 
> clang complained about this:
> 
> /usr/src/usr.sbin/snmpd/snmpd.c:349:47: warning: implicit conversion from 
> 'int' to 'char' changes value from 128 to -128 [-Wconstant-conversion]
> env->sc_engineid[(env->sc_engineid_len)++] = SNMP_ENGINEID_FMT_EID;
>~ ^
> /usr/src/usr.sbin/snmpd/snmpd.h:80:31: note: expanded from macro 
> 'SNMP_ENGINEID_FMT_EID'
> ^~~
> 
> diff --git snmpd/snmpd.h snmpd/snmpd.h
> index 91186f23e42..ce1902bdc03 100644
> --- snmpd/snmpd.h
> +++ snmpd/snmpd.h
> @@ -438,7 +438,7 @@ struct snmp_message {
>   long longsm_secmodel;
>   u_int32_tsm_engine_boots;
>   u_int32_tsm_engine_time;
> - char sm_ctxengineid[SNMPD_MAXENGINEIDLEN];
> + uint8_t  sm_ctxengineid[SNMPD_MAXENGINEIDLEN];
>   size_t   sm_ctxengineid_len;
>   char sm_ctxname[SNMPD_MAXCONTEXNAMELEN+1];
>  
> @@ -574,7 +574,7 @@ struct snmpd {
>   char sc_rwcommunity[SNMPD_MAXCOMMUNITYLEN];
>   char sc_trcommunity[SNMPD_MAXCOMMUNITYLEN];
>  
> - char sc_engineid[SNMPD_MAXENGINEIDLEN];
> + uint8_t  sc_engineid[SNMPD_MAXENGINEIDLEN];
>   size_t   sc_engineid_len;
>  
>   struct snmp_statssc_stats;
> 

Looks good, ok gerhard@

Re: snmpd getbulk replies

[Gerhard Roth]

On Thu, 27 Jul 2017 12:46:23 +0100 Stuart Henderson <s...@spacehopper.org> 
wrote:
> On 2017/07/27 10:58, Gerhard Roth wrote:
> > Hi,
> > 
> > snmpd uses the same storage for sm_error and sm_nonrepeaters. Same applies
> > to sm_errorindex and sm_maxrepetitions. If we produce a response PDU to
> > a getbulk request, sm_error will carry the number of non-repeaters from
> > the request and sm_errorindex the max. number of repetitions. This is
> > wrong. We should clears those fields in case of success.
> 
> This is definitely an improvement. There still seems to be something not
> quite right though, non-repeaters are repeating - I wouldn't expect to see
> sysObjectID, sysUpTime, sysContact, ipDefaultTTL, ipInRecives, ipInHdrErrors
> here:
> 
> $ snmpbulkget -Cn2 -Cr4 -v2c -c public 127.0.0.1 SNMPv2-MIB::sysDescr \
>IP-MIB::ipForwarding IF-MIB::ifName IF-MIB::ifInOctets
> SNMPv2-MIB::sysDescr.0 = STRING: sym
> SNMPv2-MIB::sysObjectID.0 = OID: OPENBSD-BASE-MIB::openBSDDefaultObjectID
> SNMPv2-MIB::sysUpTime.0 = Timeticks: (18115) 0:03:01.15
> SNMPv2-MIB::sysContact.0 = STRING: root@some.email.address.example
> IP-MIB::ipForwarding.0 = INTEGER: forwarding(1)
> IP-MIB::ipDefaultTTL.0 = INTEGER: 64
> IP-MIB::ipInReceives.0 = Counter32: 26379088
> IP-MIB::ipInHdrErrors.0 = Counter32: 0
> IF-MIB::ifName.1 = STRING: em0
> IF-MIB::ifName.2 = STRING: em1
> IF-MIB::ifName.3 = STRING: enc0
> IF-MIB::ifName.4 = STRING: lo0
> IF-MIB::ifInOctets.1 = Counter32: 2586116638
> IF-MIB::ifInOctets.2 = Counter32: 0
> IF-MIB::ifInOctets.3 = Counter32: 0
> IF-MIB::ifInOctets.4 = Counter32: 508223137


Hi Steuart,

could you please try the updated patch below. I guess the non-repeater
parameters was completely ignored before.

Gerhard


Index: usr.sbin/snmpd/snmpe.c
===
RCS file: /cvs/src/usr.sbin/snmpd/snmpe.c,v
retrieving revision 1.47
diff -u -p -u -p -r1.47 snmpe.c
--- usr.sbin/snmpd/snmpe.c  21 Apr 2017 13:50:23 -  1.47
+++ usr.sbin/snmpd/snmpe.c  27 Jul 2017 11:58:52 -
@@ -439,7 +439,8 @@ snmpe_parsevarbinds(struct snmp_message 
case SNMP_C_GETBULKREQ:
ret = mps_getbulkreq(msg, >sm_c,
>sm_end, ,
-   msg->sm_maxrepetitions);
+   (msg->sm_i < msg->sm_nonrepeaters) ?
+   1 : msg->sm_maxrepetitions);
if (ret == 0 || ret == 1)
break;
msg->sm_error = SNMP_ERROR_NOSUCHNAME;
@@ -467,6 +468,8 @@ snmpe_parsevarbinds(struct snmp_message 
}
 
msg->sm_errstr = "none";
+   msg->sm_error = 0;
+   msg->sm_errorindex = 0;
 
return (ret);
  varfail:

snmpd getbulk replies

[Gerhard Roth]

Hi,

snmpd uses the same storage for sm_error and sm_nonrepeaters. Same applies
to sm_errorindex and sm_maxrepetitions. If we produce a response PDU to
a getbulk request, sm_error will carry the number of non-repeaters from
the request and sm_errorindex the max. number of repetitions. This is
wrong. We should clears those fields in case of success.

Gerhard


Index: usr.sbin/snmpd/snmpe.c
===
RCS file: /cvs/src/usr.sbin/snmpd/snmpe.c,v
retrieving revision 1.47
diff -u -p -u -p -r1.47 snmpe.c
--- usr.sbin/snmpd/snmpe.c  21 Apr 2017 13:50:23 -  1.47
+++ usr.sbin/snmpd/snmpe.c  27 Jul 2017 08:49:31 -
@@ -467,6 +467,8 @@ snmpe_parsevarbinds(struct snmp_message 
}
 
msg->sm_errstr = "none";
+   msg->sm_error = 0;
+   msg->sm_errorindex = 0;
 
return (ret);
  varfail:

Re: Standard conformance of strtol(3)

[Gerhard Roth]

On 06.07.2017 17:53, Todd C. Miller wrote:

On Thu, 06 Jul 2017 07:37:19 -0600, "Todd C. Miller" wrote:

glibc strtol() behavior:
 AIX
 FreeBSD
 GNU/Linux
 Solaris
 macOS


SunOS 4.1.3 has the same behavior as Solaris.  That's as far back
as I care to go.

  - todd



FWIW: AT's SVR4 from 1988 had the same behaviour ;)


Gerhard

Fix possible fault in sysctl_file()

[Gerhard Roth]

Hi,

file pointer may be incompletely initialized after falloc(). For example,
sys_socket() initializes 'f_flag', 'f_type', and 'f_ops' but may sleep
then in socreate() before assigning 'f_data'.

That is why there is the FIF_LARVAL flag, that is check by the macro
FILE_IS_USABLE(). Of the three different operations sysctl_file()
implements, two of them (namely KERN_FILE_BYPID and KERN_FILE_BYUID)
use the FILE_IS_USABLE() to keep hand off incomplete file pointers.
Yet the third operation (KERN_FILE_BYFILE) doesn't. That can yield
a fault when dereferencing fp->f_data.

The fix is rather straightforward.

Gerhard


Index: sys/kern/kern_sysctl.c
===
RCS file: /cvs/src/sys/kern/kern_sysctl.c,v
retrieving revision 1.328
diff -u -p -u -p -r1.328 kern_sysctl.c
--- sys/kern/kern_sysctl.c  14 Jun 2017 03:00:40 -  1.328
+++ sys/kern/kern_sysctl.c  20 Jun 2017 11:31:40 -
@@ -1327,6 +1327,7 @@ sysctl_file(int *name, u_int namelen, ch
FREF(fp);
do {
if (fp->f_count > 1 && /* 0, +1 for our FREF() */
+   FILE_IS_USABLE(fp) &&
(arg == 0 || fp->f_type == arg)) {
int af, skip = 0;
if (arg == DTYPE_SOCKET && fp->f_type == arg) {

Fix umb(4) on big-endian machines

[Gerhard Roth]

Hi,

all MBIM values are in litte-endian encoding but somewhere in the fine
print it reads that "the addresses will be in network byte order".

So applying letoh32() on addresses is just plain wrong. On little-endian
machines, we didn't notice since letoh32() is a no-op there. But one
big-endian ones, the driver used IP addresses in the wrong byte order.

Cheers,

Gerhard



Index: sys/dev/usb/if_umb.c
===
RCS file: /cvs/src/sys/dev/usb/if_umb.c,v
retrieving revision 1.11
diff -u -p -u -p -r1.11 if_umb.c
--- sys/dev/usb/if_umb.c18 Apr 2017 13:27:55 -  1.11
+++ sys/dev/usb/if_umb.c2 May 2017 14:41:32 -
@@ -1646,7 +1646,6 @@ umb_decode_ip_configuration(struct umb_s
 
/* Only pick the first one */
memcpy(, data + off, sizeof (ipv4elem));
-   ipv4elem.addr = letoh32(ipv4elem.addr);
ipv4elem.prefixlen = letoh32(ipv4elem.prefixlen);
 
memset(, 0, sizeof (ifra));
@@ -1660,8 +1659,7 @@ umb_decode_ip_configuration(struct umb_s
sin->sin_len = sizeof (ifra.ifra_dstaddr);
if (avail & MBIM_IPCONF_HAS_GWINFO) {
off = letoh32(ic->ipv4_gwoffs);
-   sin->sin_addr.s_addr =
-   letoh32(*((uint32_t *)(data + off)));
+   sin->sin_addr.s_addr = *((uint32_t *)(data + off));
}
 
sin = (struct sockaddr_in *)_mask;
@@ -1690,7 +1688,7 @@ umb_decode_ip_configuration(struct umb_s
while (n-- > 0) {
if (off + sizeof (uint32_t) > len)
break;
-   val = letoh32(*((uint32_t *)(data + off)));
+   val = *((uint32_t *)(data + off));
if (i < UMB_MAX_DNSSRV)
sc->sc_info.ipv4dns[i++] = val;
off += sizeof (uint32_t);

Re: umb: aggregate packets on tx

[Gerhard Roth]

On Mon, 12 Dec 2016 14:50:50 +0100 Gerhard Roth <gerhard_r...@genua.de> wrote:
> The current umb(4) implementation needs one USB transfer for every packet
> that is sent. With the following patch, we can now aggregate several
> packets from the ifq into one single USB transfer.
> 
> This may speed up the tx path. And even if it doesn't, at least it
> reduces the number of transfers required.
> 
> 
> Gerhard
> 

Ping.

Anyone willing to ok this?

(Patch below updated to match current).


Gerhard


Index: sys/dev/usb/if_umb.c
===
RCS file: /cvs/src/sys/dev/usb/if_umb.c,v
retrieving revision 1.9
diff -u -p -u -p -r1.9 if_umb.c
--- sys/dev/usb/if_umb.c22 Jan 2017 10:17:39 -  1.9
+++ sys/dev/usb/if_umb.c20 Feb 2017 07:44:40 -
@@ -156,7 +156,7 @@ int  umb_decode_connect_info(struct umb
 int umb_decode_ip_configuration(struct umb_softc *, void *, int);
 voidumb_rx(struct umb_softc *);
 voidumb_rxeof(struct usbd_xfer *, void *, usbd_status);
-int umb_encap(struct umb_softc *, struct mbuf *);
+int umb_encap(struct umb_softc *);
 voidumb_txeof(struct usbd_xfer *, void *, usbd_status);
 voidumb_decap(struct umb_softc *, struct usbd_xfer *);
 
@@ -299,6 +299,7 @@ umb_attach(struct device *parent, struct
 
sc->sc_udev = uaa->device;
sc->sc_ctrl_ifaceno = uaa->ifaceno;
+   ml_init(>sc_tx_ml);
 
/*
 * Some MBIM hardware does not provide the mandatory CDC Union
@@ -583,8 +584,25 @@ umb_ncm_setup(struct umb_softc *sc)
UGETW(np.wLength) == sizeof (np)) {
sc->sc_rx_bufsz = UGETDW(np.dwNtbInMaxSize);
sc->sc_tx_bufsz = UGETDW(np.dwNtbOutMaxSize);
-   } else
+   sc->sc_maxdgram = UGETW(np.wNtbOutMaxDatagrams);
+   sc->sc_align = UGETW(np.wNdpOutAlignment);
+   sc->sc_ndp_div = UGETW(np.wNdpOutDivisor);
+   sc->sc_ndp_remainder = UGETW(np.wNdpOutPayloadRemainder);
+   /* Validate values */
+   if (!powerof2(sc->sc_align) || sc->sc_align == 0 ||
+   sc->sc_align >= sc->sc_tx_bufsz)
+   sc->sc_align = sizeof (uint32_t);
+   if (!powerof2(sc->sc_ndp_div) || sc->sc_ndp_div == 0 ||
+   sc->sc_ndp_div >= sc->sc_tx_bufsz)
+   sc->sc_ndp_div = sizeof (uint32_t);
+   if (sc->sc_ndp_remainder >= sc->sc_ndp_div)
+   sc->sc_ndp_remainder = 0;
+   } else {
sc->sc_rx_bufsz = sc->sc_tx_bufsz = 8 * 1024;
+   sc->sc_maxdgram = 0;
+   sc->sc_align = sc->sc_ndp_div = sizeof (uint32_t);
+   sc->sc_ndp_remainder = 0;
+   }
 }
 
 int
@@ -593,12 +611,12 @@ umb_alloc_xfers(struct umb_softc *sc)
if (!sc->sc_rx_xfer) {
if ((sc->sc_rx_xfer = usbd_alloc_xfer(sc->sc_udev)) != NULL)
sc->sc_rx_buf = usbd_alloc_buffer(sc->sc_rx_xfer,
-   sc->sc_rx_bufsz + MBIM_HDR32_LEN);
+   sc->sc_rx_bufsz);
}
if (!sc->sc_tx_xfer) {
if ((sc->sc_tx_xfer = usbd_alloc_xfer(sc->sc_udev)) != NULL)
sc->sc_tx_buf = usbd_alloc_buffer(sc->sc_tx_xfer,
-   sc->sc_tx_bufsz + MBIM_HDR16_LEN);
+   sc->sc_tx_bufsz);
}
return (sc->sc_rx_buf && sc->sc_tx_buf) ? 1 : 0;
 }
@@ -617,10 +635,7 @@ umb_free_xfers(struct umb_softc *sc)
sc->sc_tx_xfer = NULL;
sc->sc_tx_buf = NULL;
}
-   if (sc->sc_tx_m) {
-   m_freem(sc->sc_tx_m);
-   sc->sc_tx_m = NULL;
-   }
+   ml_purge(>sc_tx_ml);
 }
 
 int
@@ -792,35 +807,91 @@ umb_input(struct ifnet *ifp, struct mbuf
return 1;
 }
 
+static inline int
+umb_align(size_t bufsz, int offs, int alignment, int remainder)
+{
+   size_t   m = alignment - 1;
+   int  align;
+
+   align = (((size_t)offs + m) & ~m) - alignment + remainder;
+   if (align < offs)
+   align += alignment;
+   if (align > bufsz)
+   align = bufsz;
+   return align - offs;
+}
+
+static inline int
+umb_padding(void *buf, size_t bufsz, int offs, int alignment, int remainder)
+{
+   int  nb;
+
+   nb = umb_align(bufsz, offs, alignment, remainder);
+   if (nb > 0)
+   memset(buf + offs, 0, nb);
+   return nb;
+}
+
 void
 umb_start(struct ifnet *ifp)
 {
struct umb_softc *sc = ifp->if_softc;
-   struct mbuf *m_head = NULL;
+   struct mbuf *m = NULL;
+   int  ndg

umb: aggregate packets on tx

[Gerhard Roth]

The current umb(4) implementation needs one USB transfer for every packet
that is sent. With the following patch, we can now aggregate several
packets from the ifq into one single USB transfer.

This may speed up the tx path. And even if it doesn't, at least it
reduces the number of transfers required.


Gerhard


Index: sys/dev/usb/if_umb.c
===
RCS file: /cvs/src/sys/dev/usb/if_umb.c,v
retrieving revision 1.8
diff -u -p -u -p -r1.8 if_umb.c
--- sys/dev/usb/if_umb.c25 Nov 2016 12:43:26 -  1.8
+++ sys/dev/usb/if_umb.c12 Dec 2016 13:38:34 -
@@ -156,7 +156,7 @@ int  umb_decode_connect_info(struct umb
 int umb_decode_ip_configuration(struct umb_softc *, void *, int);
 voidumb_rx(struct umb_softc *);
 voidumb_rxeof(struct usbd_xfer *, void *, usbd_status);
-int umb_encap(struct umb_softc *, struct mbuf *);
+int umb_encap(struct umb_softc *);
 voidumb_txeof(struct usbd_xfer *, void *, usbd_status);
 voidumb_decap(struct umb_softc *, struct usbd_xfer *);
 
@@ -299,6 +299,7 @@ umb_attach(struct device *parent, struct
 
sc->sc_udev = uaa->device;
sc->sc_ctrl_ifaceno = uaa->ifaceno;
+   ml_init(>sc_tx_ml);
 
/*
 * Some MBIM hardware does not provide the mandatory CDC Union
@@ -583,8 +584,25 @@ umb_ncm_setup(struct umb_softc *sc)
UGETW(np.wLength) == sizeof (np)) {
sc->sc_rx_bufsz = UGETDW(np.dwNtbInMaxSize);
sc->sc_tx_bufsz = UGETDW(np.dwNtbOutMaxSize);
-   } else
+   sc->sc_maxdgram = UGETW(np.wNtbOutMaxDatagrams);
+   sc->sc_align = UGETW(np.wNdpOutAlignment);
+   sc->sc_ndp_div = UGETW(np.wNdpOutDivisor);
+   sc->sc_ndp_remainder = UGETW(np.wNdpOutPayloadRemainder);
+   /* Validate values */
+   if (!powerof2(sc->sc_align) || sc->sc_align == 0 ||
+   sc->sc_align >= sc->sc_tx_bufsz)
+   sc->sc_align = sizeof (uint32_t);
+   if (!powerof2(sc->sc_ndp_div) || sc->sc_ndp_div == 0 ||
+   sc->sc_ndp_div >= sc->sc_tx_bufsz)
+   sc->sc_ndp_div = sizeof (uint32_t);
+   if (sc->sc_ndp_remainder >= sc->sc_ndp_div)
+   sc->sc_ndp_remainder = 0;
+   } else {
sc->sc_rx_bufsz = sc->sc_tx_bufsz = 8 * 1024;
+   sc->sc_maxdgram = 0;
+   sc->sc_align = sc->sc_ndp_div = sizeof (uint32_t);
+   sc->sc_ndp_remainder = 0;
+   }
 }
 
 int
@@ -593,12 +611,12 @@ umb_alloc_xfers(struct umb_softc *sc)
if (!sc->sc_rx_xfer) {
if ((sc->sc_rx_xfer = usbd_alloc_xfer(sc->sc_udev)) != NULL)
sc->sc_rx_buf = usbd_alloc_buffer(sc->sc_rx_xfer,
-   sc->sc_rx_bufsz + MBIM_HDR32_LEN);
+   sc->sc_rx_bufsz);
}
if (!sc->sc_tx_xfer) {
if ((sc->sc_tx_xfer = usbd_alloc_xfer(sc->sc_udev)) != NULL)
sc->sc_tx_buf = usbd_alloc_buffer(sc->sc_tx_xfer,
-   sc->sc_tx_bufsz + MBIM_HDR16_LEN);
+   sc->sc_tx_bufsz);
}
return (sc->sc_rx_buf && sc->sc_tx_buf) ? 1 : 0;
 }
@@ -617,10 +635,7 @@ umb_free_xfers(struct umb_softc *sc)
sc->sc_tx_xfer = NULL;
sc->sc_tx_buf = NULL;
}
-   if (sc->sc_tx_m) {
-   m_freem(sc->sc_tx_m);
-   sc->sc_tx_m = NULL;
-   }
+   ml_purge(>sc_tx_ml);
 }
 
 int
@@ -792,35 +807,91 @@ umb_input(struct ifnet *ifp, struct mbuf
return 1;
 }
 
+static inline int
+umb_align(size_t bufsz, int offs, int alignment, int remainder)
+{
+   size_t   m = alignment - 1;
+   int  align;
+
+   align = (((size_t)offs + m) & ~m) - alignment + remainder;
+   if (align < offs)
+   align += alignment;
+   if (align > bufsz)
+   align = bufsz;
+   return align - offs;
+}
+
+static inline int
+umb_padding(void *buf, size_t bufsz, int offs, int alignment, int remainder)
+{
+   int  nb;
+
+   nb = umb_align(bufsz, offs, alignment, remainder);
+   if (nb > 0)
+   memset(buf + offs, 0, nb);
+   return nb;
+}
+
 void
 umb_start(struct ifnet *ifp)
 {
struct umb_softc *sc = ifp->if_softc;
-   struct mbuf *m_head = NULL;
+   struct mbuf *m = NULL;
+   int  ndgram = 0;
+   int  offs, plen, len, mlen;
+   int  maxalign;
 
if (usbd_is_dying(sc->sc_udev) ||
!(ifp->if_flags & IFF_RUNNING) ||
ifq_is_oactive(>if_snd))
return;
 
-   m_head = ifq_deq_begin(>if_snd);
-   if (m_head == NULL)
-   return;
+   KASSERT(ml_empty(>sc_tx_ml));
 
-   if (!umb_encap(sc, m_head)) {
-   

umb string padding

[Gerhard Roth]

This patch fixes a bug in the padding of umb strings. Instead of
padding the right position, umb_padding() would always zero padding
bytes at the beginning of the buffer.

For the two callers of umb_addstr(), this won't hurt in
umb_send_connect() since the first value in the buffer is the
session id, which is zero anyway. But for umb_setpin() we might try to
change the wrong type of PIN, iff the length of the PIN is not a
multiple of 4.

Gerhard


Index: sys/dev/usb/if_umb.c
===
RCS file: /cvs/src/sys/dev/usb/if_umb.c,v
retrieving revision 1.7
diff -u -p -u -p -r1.7 if_umb.c
--- sys/dev/usb/if_umb.c21 Nov 2016 08:19:36 -  1.7
+++ sys/dev/usb/if_umb.c22 Nov 2016 11:41:49 -
@@ -1211,7 +1211,7 @@ umb_padding(void *data, int len, size_t 
int  np = 0;
 
while (len < sz && (len % 4) != 0) {
-   *p++ = '\0';
+   *(p + len) = '\0';
len++;
np++;
}

FCC Auth patch for umb(4)

[Gerhard Roth]

Some MBIM devices need a FCC Authentication before they're willing to
turn on the radio. This has to be done by sending a QMI command inside
an MBIM message.

This patch is based on an earlier patch by Stuart Henderson. One
crucial thing was missing in sthen@'s patch: first a client-id (CID)
has to be allocated and this CID must then be patched into the
right field of the FCC-Auth.

Sending the FCC-Auth is limited to a list of devices known to require
this. Currently, this is only the Sierra Wireless EM7455.

This patch was possible thanks to a lot of testing by Bryan Vyhmeister.


Gerhard


Index: sys/dev/usb/if_umb.c
===
RCS file: /cvs/src/sys/dev/usb/if_umb.c,v
retrieving revision 1.6
diff -u -p -u -p -r1.6 if_umb.c
--- sys/dev/usb/if_umb.c14 Nov 2016 12:55:56 -  1.6
+++ sys/dev/usb/if_umb.c16 Nov 2016 08:42:44 -
@@ -170,6 +170,8 @@ int  umb_setpin(struct umb_softc *, int
int);
 voidumb_setdataclass(struct umb_softc *);
 voidumb_radio(struct umb_softc *, int);
+voidumb_allocate_cid(struct umb_softc *);
+voidumb_send_fcc_auth(struct umb_softc *);
 voidumb_packet_service(struct umb_softc *, int);
 voidumb_connect(struct umb_softc *);
 voidumb_disconnect(struct umb_softc *);
@@ -177,8 +179,10 @@ voidumb_send_connect(struct umb_softc
 
 voidumb_qry_ipconfig(struct umb_softc *);
 voidumb_cmd(struct umb_softc *, int, int, void *, int);
+voidumb_cmd1(struct umb_softc *, int, int, void *, int, uint8_t *);
 voidumb_command_done(struct umb_softc *, void *, int);
 voidumb_decode_cid(struct umb_softc *, uint32_t, void *, int);
+voidumb_decode_qmi(struct umb_softc *, uint8_t *, int);
 
 voidumb_intr(struct usbd_xfer *, void *, usbd_status);
 
@@ -188,6 +192,7 @@ int  umb_xfer_tout = USBD_DEFAULT_TIMEO
 
 uint8_t umb_uuid_basic_connect[] = MBIM_UUID_BASIC_CONNECT;
 uint8_t umb_uuid_context_internet[] = 
MBIM_UUID_CONTEXT_INTERNET;
+uint8_t umb_uuid_qmi_mbim[] = MBIM_UUID_QMI_MBIM;
 uint32_tumb_session_id = 0;
 
 struct cfdriver umb_cd = {
@@ -204,6 +209,39 @@ const struct cfattach umb_ca = {
 
 int umb_delay = 4000;
 
+/*
+ * These devices require an "FCC Authentication" command.
+ */
+const struct usb_devno umb_fccauth_devs[] = {
+   { USB_VENDOR_SIERRA, USB_PRODUCT_SIERRA_EM7455 },
+};
+
+uint8_t umb_qmi_alloc_cid[] = {
+   0x01,
+   0x0f, 0x00, /* len */
+   0x00,   /* QMUX flags */
+   0x00,   /* service "ctl" */
+   0x00,   /* CID */
+   0x00,   /* QMI flags */
+   0x01,   /* transaction */
+   0x22, 0x00, /* msg "Allocate CID" */
+   0x04, 0x00, /* TLV len */
+   0x01, 0x01, 0x00, 0x02  /* TLV */
+};
+
+uint8_t umb_qmi_fcc_auth[] = {
+   0x01,
+   0x0c, 0x00, /* len */
+   0x00,   /* QMUX flags */
+   0x02,   /* service "dms" */
+#define UMB_QMI_CID_OFFS   5
+   0x00,   /* CID (filled in later) */
+   0x00,   /* QMI flags */
+   0x01, 0x00, /* transaction */
+   0x5f, 0x55, /* msg "Send FCC Authentication" */
+   0x00, 0x00  /* TLV len */
+};
+
 int
 umb_match(struct device *parent, void *match, void *aux)
 {
@@ -328,6 +366,10 @@ umb_attach(struct device *parent, struct
printf("%s: missing MBIM descriptor\n", DEVNAM(sc));
goto fail;
}
+   if (usb_lookup(umb_fccauth_devs, uaa->vendor, uaa->product)) {
+   sc->sc_flags |= UMBFLG_FCC_AUTH_REQUIRED;
+   sc->sc_cid = -1;
+   }
 
for (i = 0; i < uaa->nifaces; i++) {
if (usbd_iface_claimed(sc->sc_udev, i))
@@ -783,7 +825,14 @@ umb_statechg_timeout(void *arg)
 {
struct umb_softc *sc = arg;
 
-   printf("%s: state change timeout\n",DEVNAM(sc));
+   if (sc->sc_info.regstate == MBIM_REGSTATE_ROAMING && !sc->sc_roaming) {
+   /*
+* Query the registration state until we're with the home
+* network again.
+*/
+   umb_cmd(sc, MBIM_CID_REGISTER_STATE, MBIM_CMDOP_QRY, NULL, 0);
+   } else
+   printf("%s: state change timeout\n",DEVNAM(sc));
usb_add_task(sc->sc_udev, >sc_umb_task);
 }
 
@@ -863,8 +912,23 @@ umb_up(struct umb_softc *sc)
umb_open(sc);
break;
case UMB_S_OPEN:
-   DPRINTF("%s: init: turning radio on ...\n", DEVNAM(sc));
-   umb_radio(sc, 1);
+   if (sc->sc_flags & UMBFLG_FCC_AUTH_REQUIRED) {
+   if 

Re: [PATCH] usbdevs for Sierra Wireless EM7455

[Gerhard Roth]

On Tue, 15 Nov 2016 08:11:01 -0800 Bryan Vyhmeister  
wrote:
> This patch adds the Sierra Wireless EM7455 umb(4) device to usbdevs in
> preparation for another patch to if_umb.c which adds full support for
> the EM7455.
> 
> Bryan
> 
> 
> Index: sys/dev/usb/usbdevs
> ===
> RCS file: /cvs/src/sys/dev/usb/usbdevs,v
> retrieving revision 1.670
> diff -u -p -r1.670 usbdevs
> --- sys/dev/usb/usbdevs   23 Sep 2016 08:18:00 -  1.670
> +++ sys/dev/usb/usbdevs   10 Nov 2016 17:13:34 -
> @@ -3840,6 +3840,7 @@ product SIERRA MC8355   0x9013  MC8355
>  product SIERRA AIRCARD_340U  0x9051  Aircard 340U
>  product SIERRA AIRCARD_770S  0x9053  Aircard 770S
>  product SIERRA MC74550x9071  MC7455
> +product SIERRA EM74550x9079  EM7455
>  
>  /* Sigmatel products */
>  product SIGMATEL IRDA0x4200  IrDA


Thanks, committed.

Gerhard

umb: NCM datagram pointer entries

[Gerhard Roth]

Hi,

according to the NCM spec, the list of datagram pointer entries has to
be terminated with an entry where wDatagramIndex and wDatagramLen are
zero. Not all implementations seem to follow that rule: otto@ had one
that only sets the index to zero while using an arbitrary length value.

The patch below fixes the parsing to stop if any of those values is
zero. It was successfully tested by otto@

Gerhard


Index: if_umb.c
===
RCS file: /cvs/src/sys/dev/usb/if_umb.c,v
retrieving revision 1.5
diff -u -p -u -p -r1.5 if_umb.c
--- if_umb.c10 Nov 2016 14:45:43 -  1.5
+++ if_umb.c14 Nov 2016 09:34:29 -
@@ -1815,7 +1815,7 @@ umb_decap(struct umb_softc *sc, struct u
}
 
/* Terminating zero entry */
-   if (dlen == 0 && doff == 0)
+   if (dlen == 0 || doff == 0)
break;
if (len < dlen + doff) {
/* Skip giant datagram but continue processing */

Re: [PATCH] umb(4) fixes for ifconfig

[Gerhard Roth]

On Thu, 10 Nov 2016 13:56:00 +0100 Otto Moerbeek <o...@drijf.net> wrote:
> On Wed, Nov 09, 2016 at 08:51:04AM -0800, Bryan Vyhmeister wrote:
> 
> > This patch accomplishes two things for umb(4) connections. One, some SIM
> > cards have a plus at the beginning of the phone number while others do
> > not. In my case, an AT Wireless SIM card in the US has the plus where
> > a T-Mobile SIM card in Germany does not. The output of ifconfig(8)
> > originally added a plus regardless which caused two plus signs to appear
> > for me as below. With this patch, ifconfig(8) now adds the plus if it
> > is not there and does not add the plus if it is already programmed into
> > the SIM card.
> > 
> > umb0: flags=8951<UP,POINTOPOINT,RUNNING,PROMISC,SIMPLEX,MULTICAST> mtu 1430
> > index 5 priority 0 llprio 3
> > roaming disabled registration home network
> > state #7 cell-class LTE rssi -79dBm speed 47.7Mps up 286Mps down
> > SIM initialized PIN valid (3 attempts left)
> > subscriber-id 31041088 ICC-id 8901410427 provider AT
> > device EM7455 IMEI 014582000 firmware SWI9X30C_02.08.
> > phone# ++1213555123 APN broadband
> > dns 172.26.38.1
> > groups: egress
> > status: active
> > inet 10.45.181.18 --> 10.45.181.17 netmask 0xfffc
> > 
> > 
> > The second part of the patch also fixes the last digit of the phone
> > number and mulitple digits of the subscriber-id, ICC-id, and IMEI being
> > cut off. Both fixes were discussed with Gerhard Roth. I sent him an
> > original patch but it only partially solved the issues and after
> > discussion with him, he came up with the patch below which solves and
> > fully accounts for the issues and possible scenarios. With this patch
> > the output looks like this.
> > 
> > umb0: flags=8851<UP,POINTOPOINT,RUNNING,SIMPLEX,MULTICAST> mtu 1430
> > index 5 priority 0 llprio 3
> > roaming disabled registration home network
> > state #7 cell-class LTE rssi -103dBm speed 47.7Mps up 286Mps down
> > SIM initialized PIN valid (3 attempts left)
> > subscriber-id 310410812345678 ICC-id 89014104278812345678 provider AT
> > device EM7455 IMEI 014582012345678 firmware SWI9X30C_02.08.02.00
> > phone# +12135551234 APN broadband
> > dns 172.26.38.1
> > status: active
> > inet 10.84.107.51 --> 10.84.107.52 netmask 0xfff8
> > 
> > 
> > Bryan
> > 
> > 
> > Index: sbin/ifconfig/ifconfig.c
> > ===
> > RCS file: /cvs/src/sbin/ifconfig/ifconfig.c,v
> > retrieving revision 1.332
> > diff -u -p -r1.332 ifconfig.c
> > --- sbin/ifconfig/ifconfig.c8 Nov 2016 14:37:20 -   1.332
> > +++ sbin/ifconfig/ifconfig.c9 Nov 2016 16:34:26 -
> > @@ -5153,7 +5153,7 @@ umb_status(void)
> > printf("\t");
> > n = 0;
> > if (pn[0])
> > -   printf("%sphone# +%s", n++ ? " " : "", pn);
> > +   printf("%sphone# +%s", n++ ? " " : "", pn[0] == '+' ? 
> > [1] : pn);
> > if (apn[0])
> > printf("%sAPN %s", n++ ? " " : "", apn);
> > printf("\n");
> > @@ -5323,7 +5323,7 @@ done:
> > }
> > *out++ = isascii(c) ? (char)c : '?';
> > in++;
> > -   inlen -= sizeof (*in);
> > +   inlen--;
> > }
> >  }
> 
> Id's say just print the number as-is, no special casing for plus etc.
> 
> The inlen chunk is ok.
> 
> By some coincidence I was looking at the H5321gw card in my thinkpad.
> I'm making some progress; before no valid frames were received at all,
> now at least I get some. There seems to be some problem decoding the
> header, and that ends up getting offsets wrong (on my card the data
> does not follow the header immediately (at offset 0x0c), but at offset
> 0x20). More to follow later on when I find some more time
> investigating this.
> 
>   -Otto
> 
> 


Hi Otto,

that is probably due to my mistake. I assumed the the NCM pointer
structure will always immediately follow the NCM header because
all devices I had for testing did so. But according to the NCM
spec this is not necessary. And some devices apparently format
the buffer like this:

  ++
  |  

Re: umb(4) man page tweaks

[Gerhard Roth]

On Mon, 20 Jun 2016 08:50:05 +0200 Gerhard Roth <gerhard_r...@genua.de> wrote:
> Hallo Stefan,
> 
> danke, dass Du Dich darum kuemmerst.
> 
> ok gerhard@

Sorry for the German. This mail was intended to go to Stefan only and not
the mailing list. My mistake.

Gerhard

> 
> 
> On Sun, 19 Jun 2016 15:03:13 +0200 Stefan Sperling <s...@stsp.name> wrote:
> > Some information in the umb(4) man page seems to be outdated (IPV4 gateway
> > handling), or doesn't really belong in a man page ("please hack the driver
> > to add a device to a blacklist").
> > 
> > Also try to shorten the page a bit and move information about troublesome
> > devices to CAVEATS.
> > 
> > Index: umb.4
> > ===
> > RCS file: /cvs/src/share/man/man4/umb.4,v
> > retrieving revision 1.1
> > diff -u -p -r1.1 umb.4
> > --- umb.4   15 Jun 2016 19:39:34 -  1.1
> > +++ umb.4   19 Jun 2016 12:58:24 -
> > @@ -26,44 +26,27 @@
> >  The
> >  .Nm
> >  driver provides support for USB MBIM devices.
> > -Those devices establish connections via celluar networks such as
> > -GPRS, UMTS, and LTE.
> >  .Pp
> > -The
> > -.Nm
> > -device appears as a regular point-to-point network interface,
> > +MBIM devices establish connections via cellular networks such as
> > +GPRS, UMTS, and LTE.
> > +They appear as a regular point-to-point network interface,
> >  transporting raw IP frames.
> >  .Pp
> >  Required configuration parameters like PIN and APN have to be set
> > -via
> > +with
> >  .Xr ifconfig 8 .
> >  Once the SIM card has been unlocked with the correct PIN, it
> > -will remain in this state until the device is power-cycled.
> > +will remain in this state until the MBIM device is power-cycled.
> >  In case the device is connected to an "always-on" USB port,
> > -it is possible to connect to a provider without entering the
> > -PIN again even afer a reboot of the system.
> > -.Pp
> > -If a default gateway route is configured for the
> > -.Nm
> > -network interface, the driver will modify the destination IP address
> > -dynamically, according to the information sent by the network provider.
> > +it may be possible to connect to a provider without entering the
> > +PIN again even if the system was rebooted.
> >  .Sh HARDWARE
> > -The following devices are known to be supported by the
> > -.Nm
> > -driver:
> > +The following devices should work:
> >  .Pp
> >  .Bl -tag -width Ds -offset indent -compact
> >  .It Tn Sierra Wireless EM8805
> >  .It Tn Sierra Wireless MC8305
> >  .El
> > -.Pp
> > -There are probably a lot more devices that also work flawlessly.
> > -If some devices fail to provide a confirming MBIM implementation,
> > -their vendor and product IDs should be added to the driver's blacklist
> > -manually.
> > -Since most devices offer multiple interfaces, blacklisted ones will
> > -probably be attached by some other driver, such as
> > -.Xr umsm 4 .
> >  .Sh SEE ALSO
> >  .Xr intro 4 ,
> >  .Xr netintro 4 ,
> > @@ -78,4 +61,8 @@ probably be attached by some other drive
> >  .Sh CAVEATS
> >  The
> >  .Nm
> > -driver currently does not support IPv6 addresses.
> > +driver does not support IPv6.
> > +.Pp
> > +Devices which fail to provide a confirming MBIM implementation will
> > +probably be attached by some other driver, such as
> > +.Xr umsm 4 .

Re: umb(4) man page tweaks

[Gerhard Roth]

Hallo Stefan,

danke, dass Du Dich darum kuemmerst.

ok gerhard@


On Sun, 19 Jun 2016 15:03:13 +0200 Stefan Sperling  wrote:
> Some information in the umb(4) man page seems to be outdated (IPV4 gateway
> handling), or doesn't really belong in a man page ("please hack the driver
> to add a device to a blacklist").
> 
> Also try to shorten the page a bit and move information about troublesome
> devices to CAVEATS.
> 
> Index: umb.4
> ===
> RCS file: /cvs/src/share/man/man4/umb.4,v
> retrieving revision 1.1
> diff -u -p -r1.1 umb.4
> --- umb.4 15 Jun 2016 19:39:34 -  1.1
> +++ umb.4 19 Jun 2016 12:58:24 -
> @@ -26,44 +26,27 @@
>  The
>  .Nm
>  driver provides support for USB MBIM devices.
> -Those devices establish connections via celluar networks such as
> -GPRS, UMTS, and LTE.
>  .Pp
> -The
> -.Nm
> -device appears as a regular point-to-point network interface,
> +MBIM devices establish connections via cellular networks such as
> +GPRS, UMTS, and LTE.
> +They appear as a regular point-to-point network interface,
>  transporting raw IP frames.
>  .Pp
>  Required configuration parameters like PIN and APN have to be set
> -via
> +with
>  .Xr ifconfig 8 .
>  Once the SIM card has been unlocked with the correct PIN, it
> -will remain in this state until the device is power-cycled.
> +will remain in this state until the MBIM device is power-cycled.
>  In case the device is connected to an "always-on" USB port,
> -it is possible to connect to a provider without entering the
> -PIN again even afer a reboot of the system.
> -.Pp
> -If a default gateway route is configured for the
> -.Nm
> -network interface, the driver will modify the destination IP address
> -dynamically, according to the information sent by the network provider.
> +it may be possible to connect to a provider without entering the
> +PIN again even if the system was rebooted.
>  .Sh HARDWARE
> -The following devices are known to be supported by the
> -.Nm
> -driver:
> +The following devices should work:
>  .Pp
>  .Bl -tag -width Ds -offset indent -compact
>  .It Tn Sierra Wireless EM8805
>  .It Tn Sierra Wireless MC8305
>  .El
> -.Pp
> -There are probably a lot more devices that also work flawlessly.
> -If some devices fail to provide a confirming MBIM implementation,
> -their vendor and product IDs should be added to the driver's blacklist
> -manually.
> -Since most devices offer multiple interfaces, blacklisted ones will
> -probably be attached by some other driver, such as
> -.Xr umsm 4 .
>  .Sh SEE ALSO
>  .Xr intro 4 ,
>  .Xr netintro 4 ,
> @@ -78,4 +61,8 @@ probably be attached by some other drive
>  .Sh CAVEATS
>  The
>  .Nm
> -driver currently does not support IPv6 addresses.
> +driver does not support IPv6.
> +.Pp
> +Devices which fail to provide a confirming MBIM implementation will
> +probably be attached by some other driver, such as
> +.Xr umsm 4 .

Re: MBIM Patch (Round 3)

[Gerhard Roth]

On 13.06.2016 12:52, Martin Pieuchot wrote:

On 10/06/16(Fri) 21:09, Mark Kettenis wrote:

Date: Fri, 10 Jun 2016 17:20:18 +0100
From: Stuart Henderson 

On 2016/06/10 16:05, Mark Kettenis wrote:

In any case this is something we can figure out once the code hits the
tree.  Unless mpi@ is still unhappy with the way the driver performs
ioctls, I think we should get the driver bits into the tree such that
we can work on fixing the remaining issues with it.


Agreed.

Did people notice the uhub.c part? Any concerns with that?


Yes, that is a separate issue as well.  And should therefore be a
separate commit.  It doesn't seem unreasonable to me to retry once
before giving up.  But this is another area where mpi@ should have the
final say.

The printf doesn't trigger on my Sierra Wireless EM7345 (which really
is an Intel XMM 7160 in disguise).


I'm worried that this hack alone won't help us improve the currently
outdated code to initialize USB devices as it will work around other
bugs.

That said I think it's a good starting point to improve things,  so I
gave Gerhard my ok.



Thanks for the ok, the umb(4) driver has been committed now.

But because of the controversy, I did exclude the uhub.c part.
I would suggest to wait and see what other users of umb(4) experience.
If I'm the only one seeing this strange behavior, we might just forget
about it. But if others suffer from it, too, then I'll submit a separate
patch for it.

Gerhard

Re: MBIM Patch (Round 3)

[Gerhard Roth]

On 10.06.2016 14:41, Bryan Vyhmeister wrote:

On Fri, Jun 10, 2016 at 09:43:36AM +0200, Gerhard Roth wrote:

Hmm, I don't see the missing break. It is still stuck in the same
state trying to turn on the radio and always getting non-confirmative
resonses.

If the break before "case UMB_S_RADIO:" is missing, I would expect
to see a debug printf "umb0: init: checking SIM state ...".
Could you please check once again?



From a previous email you said to comment out the break after "case

UMB_S_RADIO" which did not have any effect. Commenting out the break
after "case UMB_S_OPEN" as I believe you are saying above did have an
effect.


case UMB_S_OPEN:
DPRINTF("%s: init: turning radio on ...\n", DEVNAM(sc));
umb_radio(sc, 1);
// break;
case UMB_S_RADIO:
DPRINTF("%s: init: checking SIM state ...\n", DEVNAM(sc));
umb_cmd(sc, MBIM_CID_SUBSCRIBER_READY_STATUS, MBIM_CMDOP_QRY,
NULL, 0);
break;
case UMB_S_SIMREADY:
DPRINTF("%s: init: attaching ...\n", DEVNAM(sc));
umb_packet_service(sc, 1);
break;


The error code is rather generic and gives us no help to
determine why exactly the device refuses to turn on the
radio. But it reports that the hw-state is on, which means
we can at least exclude any rfkill switch ;)

So why does the firmware refuse to turn it on? Could still
be a problem of the PIN-less SIM card you're using.


With the break commented out above I now see this output from ifconfig
umb0 (sensitive info with XXX):

umb0: flags=8811<UP,POINTOPOINT,SIMPLEX,MULTICAST> mtu 1500
index 5 priority 0
roaming disabled registration not registered
state SIM is ready cell-class none
SIM initialized PIN valid (3 attempts left)
subscriber-id  ICC-id XX
device EM7455 IMEI 014582000 firmware SWI9X30C_02.08.
phone# ++XX APN broadband
status: down

I have not seen the first ten digits of the SIM card's phone number, the
subscriber-id, or the ICC-id in ifconfig output before so that seems
promising. Could this be something like needing the FCC auth? The debug
output follows. Thank you!


Well, the device still refuses to turn on the radio and without it,
there's no chance to register with the provider.

That you see more information now is just due to the fact, that the
state machine now continues beyond the "turn radio on" step.

You might try the FCC auth patch, but I'm not very confident about
it in your situation. Your device already speaks MBIM, so why should
we need to enable it.

I haven't got the slightest clue, why it refuses to turn on the radio.
It might be related to the type of SIM card. But honestly, I don't know.




Bryan



OpenBSD 6.0-beta (GENERIC.MP) #1: Fri Jun 10 04:25:48 PDT 2016
r...@x.brycv.com:/usr/src/sys/arch/amd64/compile/GENERIC.MP
real mem = 17024274432 (16235MB)
avail mem = 16503758848 (15739MB)
mpath0 at root
scsibus0 at mpath0: 256 targets
mainbus0 at root
bios0 at mainbus0: SMBIOS rev. 2.8 @ 0xb7c01000 (66 entries)
bios0: vendor LENOVO version "R02ET44W (1.17 )" date 01/25/2016
bios0: LENOVO 20F6CTO1WW
acpi0 at bios0: rev 2
acpi0: sleep states S0 S3 S4 S5
acpi0: tables DSDT FACP UEFI SSDT SSDT ECDT HPET APIC MCFG SSDT SSDT DBGP DBG2 
BOOT BATB SSDT SSDT MSDM DMAR ASF! FPDT UEFI
acpi0: wakeup devices LID_(S4) SLPB(S3) IGBE(S4) PXSX(S4) PXSX(S4) PXSX(S4) 
PXSX(S4) EXP8(S4) PXSX(S4) XHCI(S3)
acpitimer0 at acpi0: 3579545 Hz, 24 bits
acpiec0 at acpi0
acpihpet0 at acpi0: 2399 Hz
acpimadt0 at acpi0 addr 0xfee0: PC-AT compat
cpu0 at mainbus0: apid 0 (boot processor)
cpu0: Intel(R) Core(TM) i7-6600U CPU @ 2.60GHz, 2485.32 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,SMX,EST,TM2,SSSE3,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,LONG,LAHF,ABM,3DNOWP,PERF,ITSC,FSGSBASE,SGX,BMI1,HLE,AVX2,SMEP,BMI2,ERMS,INVPCID,RTM,MPX,RDSEED,ADX,SMAP,CLFLUSHOPT,PT,SENSOR,ARAT
cpu0: 256KB 64b/line 8-way L2 cache
cpu0: smt 0, core 0, package 0
mtrr: Pentium Pro MTRR support, 10 var ranges, 88 fixed ranges
cpu0: apic clock running at 24MHz
cpu0: mwait min=64, max=64, C-substates=0.2.1.2.4.1.1.1, IBE
cpu1 at mainbus0: apid 2 (application processor)
cpu1: Intel(R) Core(TM) i7-6600U CPU @ 2.60GHz, 2494.21 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,SMX,EST,TM2,SSSE3,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,LONG,LAHF,ABM,3DNOWP,PERF,ITSC,FSGSBASE,SGX,BMI1,HLE,AVX2,SMEP,BMI2,ERMS,IN

Re: MBIM Patch (Round 3)

[Gerhard Roth]

On 10.06.2016 05:09, Bryan Vyhmeister wrote:

On Thu, Jun 09, 2016 at 10:31:58PM +0200, Gerhard Roth wrote:

If that doesn't help, please set UMB_DEBUG and set umb_debug to 5.


I left that break commented out which perhaps I shouldn't have but below
is the output when I set an apn and bring umb0 up.


Hmm, I don't see the missing break. It is still stuck in the same
state trying to turn on the radio and always getting non-confirmative
resonses.

If the break before "case UMB_S_RADIO:" is missing, I would expect
to see a debug printf "umb0: init: checking SIM state ...".
Could you please check once again?



[...]

umb0: init: turning radio on ...
umb0: set radio on
umb0: -> set MBIM_CID_RADIO_STATE (tid 7)
umb0: sent MBIM_COMMAND_MSG (tid 7)
   0:   03 00 00 00 34 00 00 00 07 00 00 00 01 00 00 00
  16:   00 00 00 00 a2 89 cc 33 bc bb 8b 4f b6 b0 13 3e
  32:   c2 aa e6 df 03 00 00 00 01 00 00 00 04 00 00 00
  48:   01 00 00 00
umb0: umb_intr: response available
umb0: got response: len 56
   0:   03 00 00 80 38 00 00 00 07 00 00 00 01 00 00 00
  16:   00 00 00 00 a2 89 cc 33 bc bb 8b 4f b6 b0 13 3e
  32:   c2 aa e6 df 03 00 00 00 02 00 00 00 08 00 00 00

  ^^^
  status == MBIM_STATUS_FAILURE

  48:   01 00 00 00 00 00 00 00

  ^^^ 
  hw_state on sw_state off

umb0: <- rcv MBIM_COMMAND_DONE (tid 7)
umb0: set/qry MBIM_CID_RADIO_STATE failed: FAILURE



The error code is rather generic and gives us no help to
determine why exactly the device refuses to turn on the
radio. But it reports that the hw-state is on, which means
we can at least exclude any rfkill switch ;)

So why does the firmware refuse to turn it on? Could still
be a problem of the PIN-less SIM card you're using.

Re: MBIM Patch (Round 3)

[Gerhard Roth]

On 10.06.2016 00:22, Stuart Henderson wrote:

On 2016/06/10 00:10, Mark Kettenis wrote:

From: Gerhard Roth <gerhard_r...@genua.de>
Date: Thu, 9 Jun 2016 23:48:23 +0200

On 09.06.2016 23:42, Mark Kettenis wrote:

Date: Thu, 9 Jun 2016 22:59:28 +0200 (CEST)
From: Mark Kettenis <mark.kette...@xs4all.nl>


Date: Wed, 8 Jun 2016 15:08:52 +0200
From: Gerhard Roth <gerhard_r...@genua.de>

I would be glad to hear from some people trying this with a real MBIM
device.


Sierra Wireless EM7345 4G LTE here.  This devices currently attached
as umodem(4).  But I did add its vendor id and device id to umb_devs,
which makes it partially attach:

umb0 at uhub0 port 4 "Sierra Wireless Inc. Sierra Wireless EM7345 4G LTE" rev 
2.00/17.29 addr 2
umb0: switching to config #1
umb0: ignoring invalid segment size 1500
umb0: ctrl_len=512, maxpktlen=8192, cap=0x4
umb0: no control interface found

(this is with UMB_DEBUG enabled)

It seems this device needs some additional poking to select alternate
interface settings.


With the appropriate alternate settings for the communication
interface and data interface (1 and 2) hardcoded in the driver, this
works!


Great!

Although another example of a device violating the MBIM spec which
clearly states that alternate settings 0 and 1 have to be used.


Which version of the spec are you looking at?  Because my copy
(Revision 1.0 Errata-1) clearly status alternate settings 1 and 2 have
to be used ;).


There are different sections, 3.2 for dual-mode (NCM1.0 / MBIM)
devices talks about various alternate settings for NCM1.0 and
for MBIM, then 6.6 which is only dealing with MBIM just talks
about 0 and 1 ..



That's how I looked at it, too. But it seems that some vendors look
at it differently ;)

So we should find a solution where we don't need to use the
currently hard-coded MBIM_INTERFACE_ALTSETTING (== 1) anymore.

Re: MBIM Patch (Round 3)

[Gerhard Roth]

On 09.06.2016 23:42, Mark Kettenis wrote:

Date: Thu, 9 Jun 2016 22:59:28 +0200 (CEST)
From: Mark Kettenis <mark.kette...@xs4all.nl>


Date: Wed, 8 Jun 2016 15:08:52 +0200
From: Gerhard Roth <gerhard_r...@genua.de>

I would be glad to hear from some people trying this with a real MBIM
device.


Sierra Wireless EM7345 4G LTE here.  This devices currently attached
as umodem(4).  But I did add its vendor id and device id to umb_devs,
which makes it partially attach:

umb0 at uhub0 port 4 "Sierra Wireless Inc. Sierra Wireless EM7345 4G LTE" rev 
2.00/17.29 addr 2
umb0: switching to config #1
umb0: ignoring invalid segment size 1500
umb0: ctrl_len=512, maxpktlen=8192, cap=0x4
umb0: no control interface found

(this is with UMB_DEBUG enabled)

It seems this device needs some additional poking to select alternate
interface settings.


With the appropriate alternate settings for the communication
interface and data interface (1 and 2) hardcoded in the driver, this
works!


Great!

Although another example of a device violating the MBIM spec which
clearly states that alternate settings 0 and 1 have to be used.




umb0: flags=8851<UP,POINTOPOINT,RUNNING,SIMPLEX,MULTICAST> mtu 1500
index 5 priority 0
roaming disabled registration home network
state up cell-class LTE rssi -79dBm speed 47.7Mps up 95.4Mps down
SIM initialized PIN valid (3 attempts left)
subscriber-id  ICC-id YY provider NL KPN
device XMM7160_V1.2_MB IMEI Z firmware FIH7160_V1.2_WW
APN umts.xs4all.nl
groups: egress
status: active
inet 83.161.163.248 --> 83.161.163.1 netmask 0xff00

Re: MBIM Patch (Round 3)

[Gerhard Roth]

On 09.06.2016 21:35, Bryan Vyhmeister wrote:

On Wed, Jun 08, 2016 at 03:08:52PM +0200, Gerhard Roth wrote:

I would be glad to hear from some people trying this with a real MBIM
device.


I have a Sierra Wireless EM7455 MBIM device that I purchased with my
ThinkPad X260. I am very excited for this driver to make it into
OpenBSD. I am a little bit unclear as to how to connect to AT wireless
in the United States thus far but I want to rule out an error in how I
am using the driver. Perhaps I have a similar issue to what sthen@ has.
I have been watching the driver discussion on the list and applied the
most recent complete patch and then did the following sequence:

ifconfig umb0 pin 1234 apn broadband
ifconfig umb0 inet 0.0.0.1 0.0.0.2
route add -ifp umb0 default 0.0.0.2
ifconfig umb0 up


If you're using the latest version of the driver, the 'ifconfig ubm0 
inet ...' isn't required anymore.


But you probably have to set the default route after the interface is
up.




I don't have a PIN set on this SIM card which seems to be needed? I'm
not sure if it's different elsewhere but I've never had a SIM card with
a PIN set before here. The output of ifconfig umb0:

umb0: flags=8811<UP,POINTOPOINT,SIMPLEX,MULTICAST> mtu 1500
index 4 priority 0
roaming disabled registration not registered
state open cell-class none
SIM not initialized PIN valid (3 attempts left)
device EM7455 IMEI 014582000 firmware SWI9X30C_02.08.
APN broadband
groups: egress
status: down
inet 0.0.0.1 --> 0.0.0.2 netmask 0xff00


Hmm, around here apart from some special company bulk contracts,
almost all SIM cards require PINs. But since yours says "PIN valid"
it clearly is content without one. But the "SIM not initialized"
is a bit strange.





From the console:


umb0: state going up from 'down' to 'open'
umb0: PIN2 state locked (3 attempts left)
umb0: SIM not initialized (PIN missing)
umb0: SIM not initialized (PIN missing)
umb0: set/qry MBIM_CID_RADIO_STATE failed: FAILURE
umb0: state change time out
umb0: set/qry MBIM_CID_RADIO_STATE failed: FAILURE
umb0: state change time out
umb0: set/qry MBIM_CID_RADIO_STATE failed: FAILURE
umb0: state change time out
umb0: set/qry MBIM_CID_RADIO_STATE failed: FAILURE
umb0: state change time out
umb0: set/qry MBIM_CID_RADIO_STATE failed: FAILURE
umb0: state change time out
umb0: set/qry MBIM_CID_RADIO_STATE failed: FAILURE
umb0: state change time out
umb0: set/qry MBIM_CID_RADIO_STATE failed: FAILURE
umb0: state change time out
umb0: set/qry MBIM_CID_RADIO_STATE failed: FAILURE

I'm not totally clear as to whether I have the right firmware by default. I
haven't booted up Windows on this system at all and there is different firmware
for some carriers (AT, Verizon, Spring) listed from Sierra Wireless for this
model. Perhaps I need to try with Verizon and see what happens.

I also tried with several other apn values that work in some circumstances 
(wap.cingular, phone) with identical results. Any ideas? Thank you!


No, I don't think that you have problems with the correct APN at this
stage. The driver is trying to turn on the radio and doesn't get a
response on the radio state.

Are you sure you haven't turned it off with the rfkill switch?

Or maybe this device refuses to accept radio commands and wants to
auto-control the radio. You could try this by commenting out the
"break" statement in umb_ub() in the UBM_S_RADIO case and see
what happens:

case UMB_S_RADIO:
umb_cmd(sc, MBIM_CID_SUBSCRIBER_READY_STATUS,
MBIM_CMDOP_QRY, NULL, 0);
// break;
case UMB_S_SIMREADY:
umb_packet_service(sc, 1);
break;

This way, it will send a command to turn the radio on, but also
continue to send the next command (register packet service) which
would otherwise be delayed until the device confirms that the radio
is on.

If that doesn't help, please set UMB_DEBUG and set umb_debug to 5.



Bryan



OpenBSD 6.0-beta (GENERIC.MP) #1: Wed Jun  8 08:11:28 PDT 2016
r...@x.example.com:/usr/src/sys/arch/amd64/compile/GENERIC.MP
real mem = 17024274432 (16235MB)
avail mem = 16503767040 (15739MB)
mpath0 at root
scsibus0 at mpath0: 256 targets
mainbus0 at root
bios0 at mainbus0: SMBIOS rev. 2.8 @ 0xb7c01000 (66 entries)
bios0: vendor LENOVO version "R02ET44W (1.17 )" date 01/25/2016
bios0: LENOVO 20F6CTO1WW
acpi0 at bios0: rev 2
acpi0: sleep states S0 S3 S4 S5
acpi0: tables DSDT FACP UEFI SSDT SSDT ECDT HPET APIC MCFG SSDT SSDT DBGP DBG2 
BOOT BATB SSDT SSDT MSDM DMAR ASF! FPDT UEFI
acpi0: wakeup devices LID_(S4) SLPB(S3) IGBE(S4) PXSX(S4) PXSX(S4) PXSX(S4) 
PXSX(S4) EXP8(S4) PXSX(S4) XHCI(S3)
acpitimer0 at acpi0: 3579545 Hz, 24 bits
acpiec0 at acpi0
acpihpet0 at acpi0: 2399 Hz
acpimadt0 at acpi0 addr 0xfee0: PC-AT compat
cpu0 at mainbus0: apid 0 (boot processor)
cpu0: Intel(R) Core(TM) i7-6600U CPU

Re: MBIM Patch (Round 3)

[Gerhard Roth]

On 09.06.2016 19:04, Ingo Schwarze wrote:

Hi Gerhard,

Gerhard Roth wrote on Wed, Jun 08, 2016 at 03:08:52PM +0200:


+.\" Copyright (c) 2016 genua mbH
+ * Copyright (c) 2016 genua mbH


These kinds of Copyright notices without the name of the actual author
are misleading.  The purpose of a Copyright notice is to inform the
reader who enjoys rights with respect to the Works; while they are
not legally required to establish Copyright and purely of advisory
nature, it is hard in practice, often almost impossible, to find out
who holds rights without them.  So putting incorrect or incomplete
information in Copyright notices defeats the very purpose of these
notices.  Please never do that.

According to international law, specifically Article 6bis of the
Berne Convention (1886, last amended 1979), even when transferring
all the economic rights, the original author of a Works always
retains the Moral Rights, including the following:

  "Independently of the author's economic rights, and even after
  the transfer of the said rights, the author shall have the right
  to claim authorship of the work and to object to any distortion,
  mutilation or other modification of, or other derogatory action
  in relation to, the said work, which would be prejudicial to his
  honor or reputation."

So not naming the author in the Copyright notice effectively
subverts the author's most fundamental inalienable right:
Being known as the author - without which the other moral rights
against derogatory action etc. lose most of their power.

At the very least, the name of the author must be included,
for example as follows:

  Copyright (c) 2016 genua mbH
  This software was written by Gerhard Roth.

But actually, company names on ISC software licences are silly.

The ISC license is specifically designed to grant all rights under
Copyright that can legally be granted except one:  To relicense.
But relicensing never has any effect since that ISC license already
grants all rights; relicensing under a different license could only
grant less rights, which would have no legal effect but might confuse
people unaware of the original grant of the ISC license.  The ISC
license only explicitly reserves one right:  To be known as the
author.  And that cannot ever be given away (see Article 6bis above).

So technically, if genua mbH insists on "(C) genua mbH", what they
are actually saying is this:  "Look, in the future, we might wish
to decide to attempt to deceive people into believing that this
software is less free than it actually is, so we reserve the right
to relicense under a less free license; or we mistrust the author
and fear that he himself might attempt this deception, so we legally
bar the author from re-releasing his own code."  In my book, both
would make them look somewhat silly.

So please get their OK for:

  Copyright (c) 2016 Gerhard Roth.

If they want acknowledgement for supporting the development, which
would only be fair if they did support it, that acknowledgement
does not belong inside, but after the license, for example:

  * Copyright (c) 2016 Gerhard Roth.
  *
  * Permission to use, copy, modify, and distribute this software for any
  * [...]
  * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
  *
  * Development of this software was supported by genua mbH.

Of course, if you have a working contract with them, they may be
allowed to insist on the silly line "Copyright (c) 2016 genua mbH"
if they want to.  But even if they try to forbid you from adding

  This software was written by Gerhard Roth.

they cannot prevent that.  Even if your working contract would say
that you transfer all your rights including the Moral Rights, that
part of it would be null and void.


Note that the form you used might be considered legal in the U.S.
because the U.S. still doesn't fully implement the Berne Convention,
after all those 130 years.  Last time i checked, the U.S. still
allowed companies to strip authors of rights that are inalienable
by international law.  But in most other countries, in particular
those that respect international law, and specifically in Germany,
your version of the Copyright notice seriously misrepresents the
legal situation.  And none of my proposed versions is illegal in
the U.S., by the way.

Yours,
  Ingo




Please don't tell me how to licence the code. As you are not the author
and as you don't have any rights on it, this is none of your business.

Either OpenBSD accepts it this way or it rejects it. But the copyright
notice won't change. EOT

Re: MBIM Patch (Round 3)

[Gerhard Roth]

On 09.06.2016 17:52, Gerhard Roth wrote:

But: this payload is only 13 bytes but the length field says 14 bytes (0x0d).


Studid me. Can't even read single digit hex values anymore :(

Re: MBIM Patch (Round 3)

[Gerhard Roth]

On Thu, 9 Jun 2016 17:37:54 +0200 Gerhard Roth <gerhard_r...@genua.de> wrote:
> On Thu, 9 Jun 2016 16:19:14 +0100 Stuart Henderson <s...@spacehopper.org> 
> wrote:
> > On 2016/06/09 15:29, Stuart Henderson wrote:
> > > On 2016/06/08 15:08, Gerhard Roth wrote:
> > > > I would be glad to hear from some people trying this with a real MBIM
> > > > device.
> > > 
> > > So I have a Dell-branded Sierra MC8805, but I don't seem able to
> > > get it to recognise my SIM card (which I can see from my Huawei
> > > umsm).
> > 
> > aha, it needs a command (and same for some HP-branded ones)... 
> > https://sigquit.wordpress.com/2015/02/
> 
> Hmm, so you need somthing similar to umb_cmd() where you have to
> use UUID d1a30bc2-f97a-6e43-bf65-c7e24fb0f0d3 instead of
> 'umb_uuid_basic_connect'. It is clear that 'op' is MBIM_CMDOP_SET.
> 
> But what value to use for 'cid' (command-id)? Somewhere it says '1',
> but '1' is MBIM_CID_DEVICE_CAPS. Well, could be anyway.
> 
> And what's inside the payload ('data', 'len')? I'm not sure.


Decoding the bytes from 
https://lists.freedesktop.org/archives/libmbim-devel/2015-August/000626.html

03 00 00 00 3D 00 00 00 07 00 00 00 01 00 00 00 00 00 00 00 D1 A3 0B C2
^type   ^len^tid^nfrag  ^currfrag   ^UUID
F9 7A 6E 43 BF 65 C7 E2 4F B0 F0 D3 01 00 00 00 01 00 00 00 0D 00 00 00
^cid^op == SET  ^infolen
01 0C 00 00 02 14 00 01 00 5F 55 00 00
^info


So:

- CID is in fact 1
- payload is { 0x01, 0x0c, 0x00, 0x00, 0x02, 0x14, 0x00, 0x01, 0x00, 0x5f, 
0x55, 0x00, 0x00 }

But: this payload is only 13 bytes but the length field says 14 bytes (0x0d).
So maybe the guy missed to paste the last byte of the payload. Appending
another null byte would be a good start :)

Re: MBIM Patch (Round 3)

[Gerhard Roth]

On Thu, 9 Jun 2016 16:19:14 +0100 Stuart Henderson <s...@spacehopper.org> wrote:
> On 2016/06/09 15:29, Stuart Henderson wrote:
> > On 2016/06/08 15:08, Gerhard Roth wrote:
> > > I would be glad to hear from some people trying this with a real MBIM
> > > device.
> > 
> > So I have a Dell-branded Sierra MC8805, but I don't seem able to
> > get it to recognise my SIM card (which I can see from my Huawei
> > umsm).
> 
> aha, it needs a command (and same for some HP-branded ones)... 
> https://sigquit.wordpress.com/2015/02/

Hmm, so you need somthing similar to umb_cmd() where you have to
use UUID d1a30bc2-f97a-6e43-bf65-c7e24fb0f0d3 instead of
'umb_uuid_basic_connect'. It is clear that 'op' is MBIM_CMDOP_SET.

But what value to use for 'cid' (command-id)? Somewhere it says '1',
but '1' is MBIM_CID_DEVICE_CAPS. Well, could be anyway.

And what's inside the payload ('data', 'len')? I'm not sure.

Re: MBIM Patch (Round 3)

[Gerhard Roth]

On Thu, 9 Jun 2016 15:29:34 +0100 Stuart Henderson <s...@spacehopper.org> wrote:
> On 2016/06/08 15:08, Gerhard Roth wrote:
> > I would be glad to hear from some people trying this with a real MBIM
> > device.
> 
> So I have a Dell-branded Sierra MC8805, but I don't seem able to
> get it to recognise my SIM card (which I can see from my Huawei
> umsm).

You're not even getting anywhere near SIM card information. See below.


> 
> # ifconfig umb0 pin  apn x
> # ifconfig umb0  
> umb0: flags=8810<POINTOPOINT,SIMPLEX,MULTICAST> mtu 1500
>   index 19 priority 0
>   roaming disabled registration unknown
>   state down cell-class none
>   SIM not initialized PIN required
>   APN x
>   status: down
> 
> Any suggestions of where I can poke?
> 
> Jun  9 15:22:31 zoo apmd: system resumed from sleep
> Jun  9 15:22:31 zoo /bsd: uvideo0 at uhub4 port 5 configuration 1 interface 0 
> "Sonix Technology Co., Ltd. USB 2.0 Camera" rev 2.00/1.00 addr 2
> Jun  9 15:22:31 zoo /bsd: video0 at uvideo0
> Jun  9 15:22:32 zoo /bsd: usbd_fill_iface_data: bad max packet size
> Jun  9 15:22:32 zoo /bsd: usbd_fill_iface_data: bad max packet size
> Jun  9 15:22:32 zoo /bsd: ugen0 at uhub4 port 6 "Sierra Wireless, 
> Incorporated Dell Wireless 5570 HSPA+ (42Mbps) Mobile Broadband Card" rev 
> 2.00/0.00 addr 3
> Jun  9 15:22:32 zoo /bsd: ugen0: setting configuration index 0 failed
> Jun  9 15:22:32 zoo /bsd: ugen0 detached
> Jun  9 15:22:43 zoo /bsd: umb0 at uhub4 port 6 configuration 2 interface 12 
> "Sierra Wireless, Incorporated Dell Wireless 5570 HSPA+ (42Mbps) Mobile 
> Broadband Card" rev 2.00/0.06 addr 3
> Jun  9 15:22:43 zoo /bsd: umb0: ctrl_len=4096, maxpktlen=4064, cap=0x20
> Jun  9 15:22:43 zoo /bsd: umb0: ctrl-ifno#12: ep-ctrl=2, data-ifno#13: 
> ep-rx=1, ep-tx=1
> Jun  9 15:22:43 zoo /bsd: umb0: -> snd MBIM_OPEN_MSG (tid 1)
> Jun  9 15:22:43 zoo /bsd: umb0: sent MBIM_OPEN_MSG (tid 1)
> Jun  9 15:22:43 zoo /bsd:0:   01 00 00 00 10 00 00 00 01 00 00 00 00 10 
> 00 00
> Jun  9 15:22:43 zoo /bsd: umb0: vers 1.0

This is the first MBIM message sent the the device.


> Jun  9 15:22:44 zoo /bsd: umb0: notification error: IOERROR
> Jun  9 15:22:51 zoo last message repeated 305 times

Normally, the device would reply with a MBIM_OPEN_DONE message on the
control pipe. And it should inform us that this reply is ready for
fetching by a UCDC_N_RESPONSE_AVAILABLE message on the interrupt pipe.

Apparently it tries to send something on the interrupt pipe, but
we fail to receive it (306 IOERRORs within 7 seconds).

I have no idea what makes the interrupt pipe fail.


> Jun  9 15:22:51 zoo /bsd: umb0: notification error: CANCELLED
> Jun  9 15:22:51 zoo /bsd: umb0 detached
> Jun  9 15:23:02 zoo /bsd: umb0 at uhub4 port 6 configuration 2 interface 12 
> "Sierra Wireless, Incorporated Dell Wireless 5570 HSPA+ (42Mbps) Mobile 
> Broadband Card" rev 2.00/0.06 addr 3
> Jun  9 15:23:02 zoo /bsd: umb0: ctrl_len=4096, maxpktlen=4064, cap=0x20
> Jun  9 15:23:02 zoo /bsd: umb0: ctrl-ifno#12: ep-ctrl=2, data-ifno#13: 
> ep-rx=1, ep-tx=1
> Jun  9 15:23:02 zoo /bsd: umb0: -> snd MBIM_OPEN_MSG (tid 1)
> Jun  9 15:23:02 zoo /bsd: umb0: sent MBIM_OPEN_MSG (tid 1)
> Jun  9 15:23:02 zoo /bsd:0:   01 00 00 00 10 00 00 00 01 00 00 00 00 10 
> 00 00
> Jun  9 15:23:02 zoo /bsd: umb0: vers 1.0
> Jun  9 15:24:06 zoo /bsd: umb0: -> set MBIM_CID_PIN (tid 2)
> Jun  9 15:24:06 zoo /bsd: umb0: sent MBIM_COMMAND_MSG (tid 2)
> Jun  9 15:24:06 zoo /bsd:0:   03 00 00 00 50 00 00 00 02 00 00 00 01 00 
> 00 00
> Jun  9 15:24:06 zoo /bsd:   16:   00 00 00 00 a2 89 cc 33 bc bb 8b 4f b6 b0 
> 13 3e
> Jun  9 15:24:06 zoo /bsd:   32:   c2 aa e6 df 04 00 00 00 01 00 00 00 20 00 
> 00 00
> Jun  9 15:24:06 zoo /bsd:   48:   02 00 00 00 00 00 00 00 18 00 00 00 08 00 
> 00 00
> Jun  9 15:24:06 zoo /bsd:   64:   00 00 00 00 00 00 00 00 30 00 30 00 30 00 
> 30 00

MBIM Patch (Round 3)

[Gerhard Roth]

Here comes the next version of the MBIM driver.

Changes since last version:

- incorporated suggestions from mpi@

- renamed to "umb"
Only file "mbim.h" which contains MBIM protocol related stuff
continues to use "mbim" as prefix.

- No longer takes fake addresses nor does it try to restore them


I would be glad to hear from some people trying this with a real MBIM
device.


Gerhard



Index: sbin/ifconfig/ifconfig.8
===
RCS file: /cvs/src/sbin/ifconfig/ifconfig.8,v
retrieving revision 1.267
diff -u -p -u -p -r1.267 ifconfig.8
--- sbin/ifconfig/ifconfig.86 Apr 2016 10:07:14 -   1.267
+++ sbin/ifconfig/ifconfig.88 Jun 2016 12:52:59 -
@@ -519,6 +519,8 @@ tunnel
 .Xr vxlan 4 )
 .It
 .Xr vlan 4
+.It
+.Xr umb 4
 .El
 .\" BRIDGE
 .Sh BRIDGE
@@ -1645,6 +1647,67 @@ will be assigned 802.1Q tag 5.
 Disassociate from the parent interface.
 This breaks the link between the vlan interface and its parent,
 clears its vlan tag, flags, and link address, and shuts the interface down.
+.El
+.\" UMB
+.Sh UMB
+.nr nS 1
+.Bk -words
+.Nm ifconfig
+.Ar umb-interface
+.Op Cm pin Ar pin
+.Op Cm chgpin Ar oldpin Ar newpin
+.Op Cm puk Ar puk Ar newpin
+.Op Oo Fl Oc Ns Cm apn Ar apn
+.Op Oo Fl Oc Ns Cm class Ar class,class,...
+.Op Oo Fl Oc Ns Cm roaming
+.Ek
+.nr nS 0
+.Pp
+The following options are available for an
+.Xr umb 4
+interface:
+.Bl -tag -width Ds
+.It Cm pin Ar pin
+Enter the PIN required to unlock the SIM card. Most SIM cards will not
+allow to establish a network association without providing a PIN.
+.It Cm chgpin Ar oldpin Ar newpin
+Permanently changes the PIN of the SIM card from the current value
+.Ar oldpin
+to
+.Ar newpin .
+.It Cm puk Ar puk Ar newpin
+Sets the PIN of the SIM card to
+.Ar newpin
+using the PUK
+.Ar puk
+to validate the request.
+.It Cm apn Ar apn
+Set the "Access Point Name" required by your network provider.
+.It Fl apn
+Clear the current "Access Point Name" value.
+.It Cm class
+List all available cell classes.
+.It Cm class Ar class,class,...
+Set the preferred cell classes. Apart from those listed by
+.Nm Cm class
+the following aliases can be used:
+.Ar 4G,
+.Ar 3G,
+and
+.Ar 2G.
+.It Fl class
+Clear any cell class preferences.
+.It Cm roaming
+Enable data roaming.
+.It Fl roaming
+Disable data roaming.
+.It Cm up
+As soon as the interface is marked as "up", the
+.Xr umb 4
+device will try to establish a data connection with the service provider.
+.It Cm down
+Marking the interface as "down" will terminate any existing data connection
+and deregister with the service provider.
 .El
 .Sh EXAMPLES
 Assign the
Index: sbin/ifconfig/ifconfig.c
===
RCS file: /cvs/src/sbin/ifconfig/ifconfig.c,v
retrieving revision 1.322
diff -u -p -u -p -r1.322 ifconfig.c
--- sbin/ifconfig/ifconfig.c3 May 2016 17:52:33 -   1.322
+++ sbin/ifconfig/ifconfig.c8 Jun 2016 12:52:59 -
@@ -107,6 +107,10 @@
 #include 
 
 #include "brconfig.h"
+#ifndef SMALL
+#include 
+#include 
+#endif /* SMALL */
 
 #define MINIMUM(a, b)  (((a) < (b)) ? (a) : (b))
 #define MAXIMUM(a, b)  (((a) > (b)) ? (a) : (b))
@@ -145,6 +149,7 @@ int showmediaflag;
 intshowcapsflag;
 intshownet80211chans;
 intshownet80211nodes;
+intshowclasses;
 
 void   notealias(const char *, int);
 void   setifaddr(const char *, int);
@@ -275,6 +280,18 @@ void   unsetifdesc(const char *, int);
 void   printifhwfeatures(const char *, int);
 void   setpair(const char *, int);
 void   unsetpair(const char *, int);
+void   umb_status(void);
+void   umb_printclasses(char *, int);
+intumb_parse_classes(const char *);
+void   umb_setpin(const char *, int);
+void   umb_chgpin(const char *, const char *);
+void   umb_puk(const char *, const char *);
+void   umb_pinop(int, int, const char *, const char *);
+void   umb_apn(const char *, int);
+void   umb_setclass(const char *, int);
+void   umb_roaming(const char *, int);
+void   utf16_to_char(uint16_t *, int, char *, size_t);
+intchar_to_utf16(const char *, uint16_t *, size_t);
 #else
 void   setignore(const char *, int);
 #endif
@@ -486,6 +503,15 @@ const struct   cmd {
{ "-descr", 1,  0,  unsetifdesc },
{ "wol",IFXF_WOL,   0,  setifxflags },
{ "-wol",   -IFXF_WOL,  0,  setifxflags },
+   { "pin",NEXTARG,0,  umb_setpin },
+   { "chgpin", NEXTARG2,   0,  NULL, umb_chgpin },
+   { "puk",NEXTARG2,   0,  NULL, umb_puk },
+   { "apn",NEXTARG,0,  umb_apn },
+   { "-apn",   -1, 0,  umb_apn },
+   { "class",  NEXTARG0,   0,  umb_setclass },
+   { "-class", -1, 0,  umb_setclass },
+   { "roaming",1,  0,  

Re: MBIM Patch (Round 2)

[Gerhard Roth]

On Wed, 8 Jun 2016 11:31:41 +0100 Stuart Henderson <s...@spacehopper.org> wrote:
> On 2016/06/08 11:59, Gerhard Roth wrote:
> > On Wed, 8 Jun 2016 10:54:00 +0100 Stuart Henderson <s...@spacehopper.org> 
> > wrote:
> > > On 2016/06/08 11:48, Gerhard Roth wrote:
> > > > 
> > > > Currently I do this to get the interface up and running as my default
> > > > route:
> > > > 
> > > > # ifconfig umb0 pin  apn 
> > > > # ifconfig umb0 inet 0.0.0.1 0.0.0.2
> > > > # route delete default
> > > > # route add -ifp umb0 default 0.0.0.2
> > > > # ifconfig umb0 up
> > > > 
> > > > Yes it seem strange that the 'route add -ifp ubm0' needs an IP address,
> > > > but that's the way it is. That's why I need the fake IP address on
> > > > the interface right from the beginning. Otherwise the 'route add'
> > > > would fail.
> > > 
> > > Ah yes this is a known strangeness - the address in the "route add"
> > > can be anything at all though, it doesn't need to be configured on
> > > the interface.
> > 
> > Well, to me it looks as if we need to put it on the interface:
> > 
> > # route add -ifp umb0 default 0.0.0.2
> > route: writing to routing socket: Network is unreachable
> > add net default: gateway 0.0.0.2: Network is unreachable
> > # ifconfig ubm0 inet 0.0.0.1 0.0.0.2
> > # route add -ifp umb0 default 0.0.0.2
> > add net default: gateway 0.0.0.2
> > 
> 
> hmm... I wonder if that check in route addition can be relaxed,
> at least for the case where the -ifp interface is point-to-point.
> 
> I think you'll find that you *can* use an address other than
> the one configured on the interface though.
> 
> $ ifconfig pppoe1 
>  
> pppoe1: flags=48851<UP,POINTOPOINT,RUNNING,SIMPLEX,MULTICAST,INET6_NOPRIVACY> 
> mtu 1500 llprio 3
> index 18 priority 0
> dev: em1 state: session
> sid: 0x1378 PADI retries: 1 PADR retries: 0 time: 1d 12:05:38
> sppp: phase network authproto chap 
> groups: pppoe zen egress
> status: active
> inet6 fe80::20d:b9ff:fe41:7e48%pppoe1 ->  prefixlen 64 scopeid 0x12
> inet6 2a02:8011:d00e:3:225:90ff:fec0:77b5 ->  prefixlen 64
> inet 82.68.199.142 --> 62.3.84.27 netmask 0x
> 
> # route delete default# make sure it's cleared:
> route: writing to routing socket: No such process
> delete net default: not in table
> 
> # route add default -ifp pppoe1 1.2.3.4
> add net default: gateway 1.2.3.4

Ah, I can reproduce that: using an arbitrary IP address here succeeds if
the interface is UP but fails if it is down.


> 
> # route -n get 8.8.8.8
>route to: 8.8.8.8
> destination: default
>mask: default
> gateway: 1.2.3.4
>   interface: pppoe1
>  if address: 82.68.199.142
>priority: 8 (static)
>   flags: <UP,GATEWAY,DONE,STATIC>
>  use   mtuexpire
>  430 0 0 
> 
> # ping -c1 8.8.8.8
> PING 8.8.8.8 (8.8.8.8): 56 data bytes
> 64 bytes from 8.8.8.8: icmp_seq=0 ttl=58 time=14.027 ms
> --- 8.8.8.8 ping statistics ---
> 1 packets transmitted, 1 packets received, 0.0% packet loss
> round-trip min/avg/max/std-dev = 14.027/14.027/14.027/0.000 ms

Re: MBIM Patch (Round 2)

[Gerhard Roth]

On Wed, 8 Jun 2016 10:54:00 +0100 Stuart Henderson <s...@spacehopper.org> wrote:
> On 2016/06/08 11:48, Gerhard Roth wrote:
> > 
> > Currently I do this to get the interface up and running as my default
> > route:
> > 
> > # ifconfig umb0 pin  apn 
> > # ifconfig umb0 inet 0.0.0.1 0.0.0.2
> > # route delete default
> > # route add -ifp umb0 default 0.0.0.2
> > # ifconfig umb0 up
> > 
> > Yes it seem strange that the 'route add -ifp ubm0' needs an IP address,
> > but that's the way it is. That's why I need the fake IP address on
> > the interface right from the beginning. Otherwise the 'route add'
> > would fail.
> 
> Ah yes this is a known strangeness - the address in the "route add"
> can be anything at all though, it doesn't need to be configured on
> the interface.

Well, to me it looks as if we need to put it on the interface:

# route add -ifp umb0 default 0.0.0.2
route: writing to routing socket: Network is unreachable
add net default: gateway 0.0.0.2: Network is unreachable
# ifconfig ubm0 inet 0.0.0.1 0.0.0.2
# route add -ifp umb0 default 0.0.0.2
add net default: gateway 0.0.0.2