Re: Set prio when bypassing pf(4)

[Stuart Henderson]

On 2016/06/07 21:49, Vincent Gross wrote:
> 
> It's how henning@ set things up when integrating the new queuing mechanism.
> http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/kern/uipc_mbuf.c#rev1.160
> 
> > Is there any use for this apart for vlan(4) interfaces?
> 
> AFAICT, no. 

My understanding is that it is also meant to be used for interface
output queues. So you could use this to prioritize ARP over IP
traffic if you wanted. (Well..you could anyway, but you don't
have as many options - many switches map the priorities 0-7
onto 4 queues so there is only lower-priority than the default
'prio=3').

> > Should it
> > really be part of "struct ifnet" ?
> > 
> 
> sthen@ pointed out that struct if_data was heavily used by our ports, and
> that such a change would require a version bump. Now, I may have overlooked
> a better place for it.

I think it could go at the end of struct if_data without causing
trouble, though since it doesn't need to be exported to userland
isn't it better directly in ifnet rather than if_data? (if_data
is included in RTM_IFINFO's if_msghdr, whereas you need an ioctl
to see the non-if_data parts of struct ifnet from userland).

> I don't think we should make a special case for vlan(4), this kind of detail
> do not belong to the arp(4) or bpf(4) layer.

btw, since this is a perfect fit for the vlan priority for pppoe
control packets that I was looking at recently, here's a diff
to use it there.

Index: if_sppp.h
===
RCS file: /cvs/src/sys/net/if_sppp.h,v
retrieving revision 1.24
diff -u -p -r1.24 if_sppp.h
--- if_sppp.h   30 May 2016 23:30:11 -  1.24
+++ if_sppp.h   7 Jun 2016 20:53:28 -
@@ -56,7 +56,6 @@ enum ppp_phase {
 
 #define AUTHMAXLEN 256 /* including terminating '\0' */
 #define AUTHCHALEN 16  /* length of the challenge we send */
-#define SPPP_CTL_PRIO  7   /* priority to use for control packets */
 
 /*
  * Definitions to pass struct sppp data down into the kernel using the
Index: if_spppsubr.c
===
RCS file: /cvs/src/sys/net/if_spppsubr.c,v
retrieving revision 1.153
diff -u -p -r1.153 if_spppsubr.c
--- if_spppsubr.c   30 May 2016 23:30:10 -  1.153
+++ if_spppsubr.c   7 Jun 2016 20:53:28 -
@@ -914,7 +914,7 @@ sppp_cp_send(struct sppp *sp, u_short pr
return;
m->m_pkthdr.len = m->m_len = PKTHDRLEN + LCP_HEADER_LEN + len;
m->m_pkthdr.ph_ifidx = 0;
-   m->m_pkthdr.pf.prio = SPPP_CTL_PRIO;
+   m->m_pkthdr.pf.prio = sp->pp_if.if_llprio;
 
*mtod(m, u_int16_t *) = htons(proto);
lh = (struct lcp_header *)(mtod(m, u_int8_t *) + 2);
@@ -3992,7 +3992,7 @@ sppp_auth_send(const struct cp *cp, stru
if (! m)
return;
m->m_pkthdr.ph_ifidx = 0;
-   m->m_pkthdr.pf.prio = SPPP_CTL_PRIO;
+   m->m_pkthdr.pf.prio = sp->pp_if.if_llprio;
 
*mtod(m, u_int16_t *) = htons(cp->proto);
lh = (struct lcp_header *)(mtod(m, u_int8_t *) + 2);

Index: if_pppoe.c
===
RCS file: /cvs/src/sys/net/if_pppoe.c,v
retrieving revision 1.56
diff -u -p -r1.56 if_pppoe.c
--- if_pppoe.c  30 May 2016 23:30:11 -  1.56
+++ if_pppoe.c  7 Jun 2016 20:53:28 -
@@ -163,7 +163,7 @@ static void pppoe_timeout(void *);
 /* sending actual protocol control packets */
 static int pppoe_send_padi(struct pppoe_softc *);
 static int pppoe_send_padr(struct pppoe_softc *);
-static int pppoe_send_padt(unsigned int, u_int, const u_int8_t *);
+static int pppoe_send_padt(unsigned int, u_int, const u_int8_t *, u_int8_t);
 
 /* raw output */
 static int pppoe_output(struct pppoe_softc *, struct mbuf *);
@@ -696,7 +696,7 @@ pppoe_data_input(struct mbuf *m)
 #ifdef PPPOE_TERM_UNKNOWN_SESSIONS
printf("pppoe (data): input for unknown session 0x%x, sending 
PADT\n",
session);
-   pppoe_send_padt(m->m_pkthdr.ph_ifidx, session, shost);
+   pppoe_send_padt(m->m_pkthdr.ph_ifidx, session, shost, 0);
 #endif
goto drop;
}
@@ -1011,7 +1011,7 @@ pppoe_send_padi(struct pppoe_softc *sc)
m0 = pppoe_get_mbuf(len + PPPOE_HEADERLEN); /* header len + payload 
len */
if (m0 == NULL)
return (ENOBUFS);
-   m0->m_pkthdr.pf.prio = SPPP_CTL_PRIO;
+   m0->m_pkthdr.pf.prio = sc->sc_sppp.pp_if.if_llprio;
 
/* fill in pkt */
p = mtod(m0, u_int8_t *);
@@ -1170,7 +1170,8 @@ pppoe_disconnect(struct pppoe_softc *sc)
PPPOEDEBUG(("%s: disconnecting\n",
sc->sc_sppp.pp_if.if_xname));
err = pppoe_send_padt(sc->sc_eth_ifidx,
-   sc->sc_session, (const u_int8_t *)>sc_dest);
+   sc->sc_session, (const u_int8_t *)>sc_dest,
+   sc->sc_sppp.pp_if.if_llprio);
}
 
/* cleanup 

Re: Set prio when bypassing pf(4)

[Vincent Gross]

Le Tue, 7 Jun 2016 10:48:22 +0200,
Martin Pieuchot  a écrit :

> On 06/06/16(Mon) 23:52, Vincent Gross wrote:
> > On Mon, 6 Jun 2016 17:33:36 +0100
> > Stuart Henderson  wrote:
> >   
> > > On 2016/06/06 16:15, Vincent Gross wrote:  
> > > > When sending ARP requests, or when writing to a bpf handle (as
> > > > when sending DHCP Discover), we bypass pf(4) so we have no way
> > > > to define the priority (m->m_pkthdr.pf.prio) of the outgoing
> > > > packets.  
> > [...]  
> > > > 
> > > > This diff adds
> > > > 1) an if_llprio field to struct ifnet
> > > 
> > > struct if_data.. this is used by enough ports that changing the
> > > abi  
> > [...]  
> > >   
> > > > diff --git a/sbin/ifconfig/ifconfig.8
> > > > b/sbin/ifconfig/ifconfig.8
> > > 
> > > BTW. patch warns about offsets if you apply this to -current.
> > >   
> > [...]  
> > > 
> > > Other than these points, it seems a useful thing to do, pppoe
> > > could use it too.
> > > 
> > > I wonder what these broken ISP devices are that require the
> > > priority field in the vlan frame header to be 0 (aka "prio 1")...
> > >   
> > 
> > r2 below. I moved if_llprio from if_data to struct ifnet, and went
> > from u_char to u_int8_t. I also added a bound check in ifioctl().
> > 
> > Comments ? ok ?  
> 
> Could you explain me why our default prio is 3?
> 

It's how henning@ set things up when integrating the new queuing mechanism.
http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/kern/uipc_mbuf.c#rev1.160

> Is there any use for this apart for vlan(4) interfaces?

AFAICT, no. 

> Should it
> really be part of "struct ifnet" ?
> 

sthen@ pointed out that struct if_data was heavily used by our ports, and
that such a change would require a version bump. Now, I may have overlooked
a better place for it.

I don't think we should make a special case for vlan(4), this kind of detail
do not belong to the arp(4) or bpf(4) layer.

> I also find weird to see a field inside ``m_pkthdr.pf'' being used
> without pf(4).
> 
> > Index: sbin/ifconfig/ifconfig.8
> > ===
> > RCS file: /cvs/src/sbin/ifconfig/ifconfig.8,v
> > retrieving revision 1.267
> > diff -u -p -r1.267 ifconfig.8
> > --- sbin/ifconfig/ifconfig.86 Apr 2016 10:07:14
> > -   1.267 +++ sbin/ifconfig/ifconfig.8  6 Jun 2016
> > 21:43:46 - @@ -327,6 +327,10 @@ Disable special processing at
> > the link l Change the link layer address (MAC address) of the
> > interface. This should be specified as six colon-separated hex
> > values, or can be chosen randomly.
> > +.It Cm llprio Ar prio
> > +Set the priority for link layer communications
> > +.Pf ( Xr arp 4 ,
> > +.Xr bpf 4 ) .
> >  .It Cm media Op Ar type
> >  Set the media type of the interface to
> >  .Ar type .
> > Index: sbin/ifconfig/ifconfig.c
> > ===
> > RCS file: /cvs/src/sbin/ifconfig/ifconfig.c,v
> > retrieving revision 1.322
> > diff -u -p -r1.322 ifconfig.c
> > --- sbin/ifconfig/ifconfig.c3 May 2016 17:52:33
> > -   1.322 +++ sbin/ifconfig/ifconfig.c  6 Jun 2016
> > 21:43:46 - @@ -135,6 +135,7 @@ char name[IFNAMSIZ];
> >  intflags, xflags, setaddr, setipdst, doalias;
> >  u_long metric, mtu;
> >  intrdomainid;
> > +intllprio;
> >  intclearaddr, s;
> >  intnewaddr = 0;
> >  intaf = AF_INET;
> > @@ -157,6 +158,7 @@ voidaddaf(const char *, int);
> >  void   removeaf(const char *, int);
> >  void   setifbroadaddr(const char *, int);
> >  void   setifmtu(const char *, int);
> > +void   setifllprio(const char *, int);
> >  void   setifnwid(const char *, int);
> >  void   setifbssid(const char *, int);
> >  void   setifnwkey(const char *, int);
> > @@ -521,6 +523,7 @@ const structcmd {
> > { "instance",   NEXTARG,A_MEDIAINST,
> > setmediainst }, { "inst",   NEXTARG,
> > A_MEDIAINST,setmediainst }, { "lladdr",
> > NEXTARG,0,  setiflladdr },
> > +   { "llprio", NEXTARG,0,
> > setifllprio }, { NULL, /*src*/  0,
> > 0,  setifaddr }, { NULL, /*dst*/
> > 0,  0,  setifdstaddr },
> > { NULL, /*illegal*/0,   0,  NULL }, @@
> > -854,6 +857,11 @@ getinfo(struct ifreq *ifr, int create) else
> > rdomainid = ifr->ifr_rdomainid;
> >  #endif
> > +   if (ioctl(s, SIOCGIFLLPRIO, (caddr_t)ifr) < 0)
> > +   llprio = 0;
> > +   else
> > +   llprio = ifr->ifr_llprio;
> > +
> > return (0);
> >  }
> >  
> > @@ -1411,6 +1419,21 @@ setifmtu(const char *val, int d)
> >  
> >  /* ARGSUSED */
> >  void
> > +setifllprio(const char *val, int d)
> > +{
> > +   const char *errmsg = NULL;
> > +
> > +   (void) strlcpy(ifr.ifr_name, name, sizeof(ifr.ifr_name));
> > +
> > +   ifr.ifr_mtu = strtonum(val, 0, UCHAR_MAX, );
> > +   if (errmsg)
> > +   

Re: MBIM Patch (Round 2)

[Ingo Schwarze]

Hi Gerhard,

Gerhard Roth wrote on Tue, Jun 07, 2016 at 02:39:42PM +0200:
> On Tue, 7 Jun 2016 13:08:49 +0200 Martin Pieuchot  wrote:
>> On 07/06/16(Tue) 11:53, Gerhard Roth wrote:
>>> On Mon, 6 Jun 2016 10:30:11 +0200 Martin Pieuchot  wrote:

 Any reason for using a different license in documentation and code?

>>> Different in what sense?  Which paragraph do you mean?

>> This is a BSD 2-clauses the code is ISC.

>>> I just copied it from some other man page in share/man/man4.

Please do not copy random ancient licenses.

>>> Can't remember which one it was.  But they all say pretty much the
>>> same thing.

>> Well if you don't mind, please use /usr/share/misc/license.template :)

> Seriously, you don't mean that!

We always use /usr/share/misc/license.template
for new documentation, please consider doing that, too.
Of course, you have to adjust the commenting style
from C to roff(7) syntax.

The canonical example is:  /usr/src/share/man/man7/mdoc.7

> Did you take a look at that file?

What's wrong with using it for documentation as well as for code?

Yours,
  Ingo

Re: lockmgr() api removal

[Theo de Raadt]

> Martin Natano wrote:
> > It is time for the lockmgr() api to die. The api is only used by
> > filesystems, where it is a trivial change to use rrw locks instead. All
> > it needs is LK_* defines for the RW_* flags. (See the sys/lock.h hunk in
> > the diff below.)
> > 
> > The ffs regress tests display the same number of fail/ok results before
> > and after applying diff below, and I have done some manual testing with
> > various filesystems on amd64 and macppc.
> > 
> > Again, the purpose is to make filesystem code less scary and more
> > comprehensible.
> > 
> > Ok?
> 
> ok, i think it's time to commit this.

Whoa.

Did I miss a report about nfs server and client?  I know I have not tested
this diff.

Re: lockmgr() api removal

[Ted Unangst]

Martin Natano wrote:
> It is time for the lockmgr() api to die. The api is only used by
> filesystems, where it is a trivial change to use rrw locks instead. All
> it needs is LK_* defines for the RW_* flags. (See the sys/lock.h hunk in
> the diff below.)
> 
> The ffs regress tests display the same number of fail/ok results before
> and after applying diff below, and I have done some manual testing with
> various filesystems on amd64 and macppc.
> 
> Again, the purpose is to make filesystem code less scary and more
> comprehensible.
> 
> Ok?

ok, i think it's time to commit this.

Re: MBIM Patch (Round 2)

[Stuart Henderson]

On 2016/06/07 14:39, Gerhard Roth wrote:
> > > Now I get an IP address from my provider, I want something like this:
> > > 
> > >   inet 10.75.178.41 --> 10.75.178.42 netmask 0xfffc
> > > 
> > > But if I used SIOCAIFADDR I would get this instead:
> > > 
> > >   inet 0.0.0.1 --> 0.0.0.2 netmask 0xff00
> > >   inet 10.75.178.41 --> 10.75.178.42 netmask 0xfffc
> > > 
> > > And deleting the old one first seems more racy.
> > 
> > Why?
> 
> Because either there is a time without any IP address or there
> is one with two addresses. Neither seems right.

Is there a problem to have no address? That is what happens with
gif(4), tun(4) etc, even with ethernet interfaces, when they don't
have a particular address configured on them.

There doesn't seem a particular problem with dhclient removing
the old address before it adds a new one..

> See above. That's the current OpenBSD model for PPP.

There are several models :-)

The 0.0.0.1 thing is a specific hack for sppp/pppoe which has
"magic" values of 0.0.0.0 and 0.0.0.1 and a way to request a
specific hardcoded address. This is because *both* sides
propose addresses via IPCP (they are "equal" peers as far
as configuration is concerned), they have to agree and can
NAK each other.

It's not the only way though, even for PPP; ppp(4) doesn't use
this mechanism, it has separate configuration for pppd instead.
When we had ppp(8), that didn't, either.

As far as I understand MBIM addressing is driven from the "mobile
network" side, we just set the address they are telling us to use
without a way to propose our own address. If that's correct then
we don't need a way to set "our side's" address at all, or have
a way to tie down the remote side's address, so address
configuration can be a lot simpler than any PPP interface.

> > But when you lose the connection with the network or you manually set
> > the interface down, this IP address once given to you by the provider
> > is no longer yours and must not be used anymore.

The same can apply in other cases (wifi, wired ethernet etc) too,
but we don't do anything to remove it from those interfaces (though
it's easy enough to remove using ifstated if needed).

Re: MBIM Patch (Round 2)

[Gerhard Roth]

On Tue, 7 Jun 2016 13:08:49 +0200 Martin Pieuchot  wrote:
> On 07/06/16(Tue) 11:53, Gerhard Roth wrote:
> > On Mon, 6 Jun 2016 10:30:11 +0200 Martin Pieuchot  wrote:
> > > On 01/06/16(Wed) 17:20, Gerhard Roth wrote:
> > > Any reason for using a different license in documentation an code?
> > 
> > Different in what sense? Which paragraph do you mean?
> 
> This is a BSD 2-clauses the code is ISC.
> 
> > I just copied it from some other man page in share/man/man4. Can't
> > remember which one it was. But they all say pretty much the same thing.
> 
> Well if you don't mind, please use /usr/share/misc/license.template :)

Seriously, you don't mean that!
Did you take a look at that file?


> 
> > > > +/*
> > > > + * Some devices are picky about too frequent control messages.
> > > > + * Query device state not more often than every 0.5 secs.
> > > > + */
> > > > +struct timeval umbim_update_rate = { 0, 50 };
> > > ^
> > > This variable seems unused.  What it is for?
> > 
> > Some remnant from an earlier version.
> > 
> > 
> > > 
> > > > +int umbim_delay = 4000;
> > > 
> > > What is this supposed to be?
> > 
> > A value?
> > 
> > A litte delay in between the requests sent to the device.
> > Using a global variable helps to tune the value via DDB in the
> > development stage.
> 
> You're paraphrasing your code :)  My unclear question was just why
> do you need such delay? ;)

Well, ask the firmware writer. I don't know.


> 
> > > > +   break;
> > > > +   case SIOCSIFFLAGS:
> > > > +   usb_add_task(sc->sc_udev, >sc_umbim_task);
> > > 
> > > I guess you want to wait for the task to complete here.  You're not
> > > respecting  the API by returning directly.  But why using a task?  To
> > > serialize up/down operations?
> > 
> > No, waiting here is not an option. In order to get a complete configuration
> > of the device you need a mobile connection with the next base station.
> > Would you really want "ifconfig up" to block indefinitely when you're in
> > the basement without network access?
> > 
> > The task is there, because going "up" needs a lot of steps with
> > requests being sent to device and then waiting for the device to
> > respond to them.
> 
> I understand you point.  My concern is just that SIOCSIFFLAGS indicates
> that the interface is down/up once the ioclt(2) is finished.  It doesn't
> seem to be the case in your driver, I just hope this is not a problem.

How about WiFi? AFAIK it only starts scanning for an access point
but doesn't block until it finished association. So why should it
be a problem here?


> 
> > > > +   if (ifr->ifr_mtu > ifp->if_hardmtu) {
> > > > +   error = EINVAL;
> > > > +   break;
> > > > +   }
> > > > +   ifp->if_mtu = ifr->ifr_mtu;
> > > > +   break;
> > > > +   case SIOCGIFMTU:
> > > > +   ifr->ifr_mtu = ifp->if_mtu;
> > > > +   break;
> > > > +   case SIOCGIFHARDMTU:
> > > > +   ifr->ifr_hardmtu = ifp->if_hardmtu;
> > > > +   break;
> > > > +   case SIOCAIFADDR:
> > > 
> > > It is accepted that this ioctl(2) implicitly bring the interface up.
> > 
> > But this is a point-to-point interface and behaves a little bit
> > different from regular Ethernet interfaces. You're not supposed to
> > add IP destination addresses manually. Your provider will tell them to you.
> > And in case you do try to add them manually, that will not get a
> > working network interface.
> 
> I'm not saying it makes sense.  I'm just explaining how it is.
> 
> > > What about routing domains?  Did you try your device in a rdomain != 0?
> > 
> > No I didn't try them. But what could happen? This is a point-to-point
> > interface. No more routing decisions should be necessary on that level.
> 
> What could happen?  Good question.  I don't know.
> 
> > > > +void
> > > > +umbim_linkstate(struct umbim_softc *sc, int state)
> > > > +{
> > > > +   struct ifnet *ifp = GET_IFP(sc);
> > > > +   int  s;
> > > > +
> > > > +   s = splnet();
> > > > +   if (ifp->if_link_state != state) {
> > > > +   log(LOG_INFO, "%s: link state changed from %s to %s\n",
> > > > +   DEVNAM(sc),
> > > > +   LINK_STATE_IS_UP(ifp->if_link_state) ? "up" : 
> > > > "down",
> > > > +   LINK_STATE_IS_UP(state) ? "up" : "down");
> > > > +   ifp->if_link_state = state;
> > > > +   if_link_state_change(ifp);
> > > > +   if (!LINK_STATE_IS_UP(state)) {
> > > > +   memset(sc->sc_info.ipv4dns, 0,
> > > > +   sizeof (sc->sc_info.ipv4dns));
> > > > +   umbim_restore_ipv4addr(sc);
> > > 
> > > Why do you need to change the address based on the link state?
> > 
> > Well, once you're connected, your provider will give you an IP 

Re: MBIM Patch (Round 2)

[David Gwynne]

> On 2 Jun 2016, at 4:28 AM, Theo de Raadt  wrote:
> 
>> As I said, we could still change the name of the interface to 'ubm'
>> while keeping 'umbim' as the name of the driver.
> 
> No, I don't understand the proposal.  I think it should be ubm
> throughout, or I am threatening to rename ix(4) to a 8 character
> name.

intel10g(4) is far less ambiguous.

Re: MBIM Patch (Round 2)

[Martin Pieuchot]

On 07/06/16(Tue) 11:53, Gerhard Roth wrote:
> On Mon, 6 Jun 2016 10:30:11 +0200 Martin Pieuchot  wrote:
> > On 01/06/16(Wed) 17:20, Gerhard Roth wrote:
> > Any reason for using a different license in documentation an code?
> 
> Different in what sense? Which paragraph do you mean?

This is a BSD 2-clauses the code is ISC.

> I just copied it from some other man page in share/man/man4. Can't
> remember which one it was. But they all say pretty much the same thing.

Well if you don't mind, please use /usr/share/misc/license.template :)

> > > +/*
> > > + * Some devices are picky about too frequent control messages.
> > > + * Query device state not more often than every 0.5 secs.
> > > + */
> > > +struct timeval umbim_update_rate = { 0, 50 };
> >   ^
> > This variable seems unused.  What it is for?
> 
> Some remnant from an earlier version.
> 
> 
> > 
> > > +int umbim_delay = 4000;
> > 
> > What is this supposed to be?
> 
> A value?
> 
> A litte delay in between the requests sent to the device.
> Using a global variable helps to tune the value via DDB in the
> development stage.

You're paraphrasing your code :)  My unclear question was just why
do you need such delay? ;)

> > > + break;
> > > + case SIOCSIFFLAGS:
> > > + usb_add_task(sc->sc_udev, >sc_umbim_task);
> > 
> > I guess you want to wait for the task to complete here.  You're not
> > respecting  the API by returning directly.  But why using a task?  To
> > serialize up/down operations?
> 
> No, waiting here is not an option. In order to get a complete configuration
> of the device you need a mobile connection with the next base station.
> Would you really want "ifconfig up" to block indefinitely when you're in
> the basement without network access?
> 
> The task is there, because going "up" needs a lot of steps with
> requests being sent to device and then waiting for the device to
> respond to them.

I understand you point.  My concern is just that SIOCSIFFLAGS indicates
that the interface is down/up once the ioclt(2) is finished.  It doesn't
seem to be the case in your driver, I just hope this is not a problem.

> > > + if (ifr->ifr_mtu > ifp->if_hardmtu) {
> > > + error = EINVAL;
> > > + break;
> > > + }
> > > + ifp->if_mtu = ifr->ifr_mtu;
> > > + break;
> > > + case SIOCGIFMTU:
> > > + ifr->ifr_mtu = ifp->if_mtu;
> > > + break;
> > > + case SIOCGIFHARDMTU:
> > > + ifr->ifr_hardmtu = ifp->if_hardmtu;
> > > + break;
> > > + case SIOCAIFADDR:
> > 
> > It is accepted that this ioctl(2) implicitly bring the interface up.
> 
> But this is a point-to-point interface and behaves a little bit
> different from regular Ethernet interfaces. You're not supposed to
> add IP destination addresses manually. Your provider will tell them to you.
> And in case you do try to add them manually, that will not get a
> working network interface.

I'm not saying it makes sense.  I'm just explaining how it is.

> > What about routing domains?  Did you try your device in a rdomain != 0?
> 
> No I didn't try them. But what could happen? This is a point-to-point
> interface. No more routing decisions should be necessary on that level.

What could happen?  Good question.  I don't know.

> > > +void
> > > +umbim_linkstate(struct umbim_softc *sc, int state)
> > > +{
> > > + struct ifnet *ifp = GET_IFP(sc);
> > > + int  s;
> > > +
> > > + s = splnet();
> > > + if (ifp->if_link_state != state) {
> > > + log(LOG_INFO, "%s: link state changed from %s to %s\n",
> > > + DEVNAM(sc),
> > > + LINK_STATE_IS_UP(ifp->if_link_state) ? "up" : "down",
> > > + LINK_STATE_IS_UP(state) ? "up" : "down");
> > > + ifp->if_link_state = state;
> > > + if_link_state_change(ifp);
> > > + if (!LINK_STATE_IS_UP(state)) {
> > > + memset(sc->sc_info.ipv4dns, 0,
> > > + sizeof (sc->sc_info.ipv4dns));
> > > + umbim_restore_ipv4addr(sc);
> > 
> > Why do you need to change the address based on the link state?
> 
> Well, once you're connected, your provider will give you an IP address
> and the driver will put that address onto the network interface.
> 
> But when you lose the connection with the network or you manually set
> the interface down, this IP address once given to you by the provider
> is no longer yours and must not be used anymore.

So why restore?  Don't you want to forget the current address?  I don't
get it.


> > Anyway you current code is racy because if_link_state_change() is
> > asynchronous.
> 
> Right. It's better to update the IP address first and then call
> if_link_state_change() afterwards.

In your state machine you says that your link state is UP when you have
an IP address assigned, right?  I'm confused about that.

> > > +void
> > > +umbim_set_ipv4addr(struct ifnet *ifp, struct umbim_ifaddr *uif, 

httpd(8) fix incorrect comment

[Frank Schoep]

Came across an incorrect comment in httpd(8) explaining memory
allocation. Comment claims that 5 times the source memory needs to
be allocated if source consists solely of "<" and ">", but those
characters expand to four bytes ("&[g/l]t;"). "&" is the reason that
5 times the memory is required ("");


Index: httpd.c
===
RCS file: /cvs/src/usr.sbin/httpd/httpd.c,v
retrieving revision 1.55
diff -u -p -r1.55 httpd.c
--- httpd.c 22 May 2016 19:19:21 -  1.55
+++ httpd.c 7 Jun 2016 09:18:47 -
@@ -744,7 +744,10 @@ escape_html(const char* src)
 {
char*dp, *dst;
 
-   /* We need 5 times the memory if every letter is "<" or ">". */
+   /*
+* We need 5 times the memory if every source character is
+* "&" (escaped to "").
+*/
if ((dst = calloc(5, strlen(src) + 1)) == NULL)
return NULL;
 

httpd(8) fix incorrect comment

[Frank Schoep]

Came across an incorrect comment in httpd(8) explaining memory
allocation. Comment claims that 5 times the source memory needs to
be allocated if source consists solely of "<" and ">", but those
characters expand to four bytes ("&[g/l]t;"). "&" is the reason that
5 times the memory is required ("");


Index: httpd.c
===
RCS file: /cvs/src/usr.sbin/httpd/httpd.c,v
retrieving revision 1.55
diff -u -p -r1.55 httpd.c
--- httpd.c 22 May 2016 19:19:21 -  1.55
+++ httpd.c 7 Jun 2016 09:18:47 -
@@ -744,7 +744,10 @@ escape_html(const char* src)
{
char*dp, *dst;

-   /* We need 5 times the memory if every letter is "<" or ">". */
+   /*
+* We need 5 times the memory if every source character is
+* "&" (escaped to "").
+*/
if ((dst = calloc(5, strlen(src) + 1)) == NULL)
return NULL;

Re: MBIM Patch (Round 2)

[Gerhard Roth]

On Mon, 6 Jun 2016 10:30:11 +0200 Martin Pieuchot  wrote:
> On 01/06/16(Wed) 17:20, Gerhard Roth wrote:
> > [...] 
> > Thanks for all the feedback.
> 
> More comments inline.

Replies too.


> 
> > 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.c1 Jun 2016 14:32:18 -
> > @@ -107,6 +107,8 @@
> >  #include 
> >  
> >  #include "brconfig.h"
> > +#include 
> > +#include 
> 
> Does USB mobile broadband interfaces share a spec with non-USB devices?
> In other words would it make sense to move (some of) the defines in net/
> rather than usb/?

No, the MBIM spec is pure USB stuff that was schemed by usb.org.
 

> In any case these two include lines should be before the standard ones
> and under #ifndef SMALL.

That's right.


> [...]
> > Index: share/man/man4/umbim.4
> > ===
> > RCS file: share/man/man4/umbim.4
> > diff -N share/man/man4/umbim.4
> > --- /dev/null   1 Jan 1970 00:00:00 -
> > +++ share/man/man4/umbim.4  1 Jun 2016 14:32:18 -
> > @@ -0,0 +1,95 @@
> > +.\" Copyright (c) 2016 genua mbH
> > +.\" All rights reserved.
> > +.\"
> > +.\" Redistribution and use in source and binary forms, with or without
> > +.\" modification, are permitted provided that the following conditions
> > +.\" are met:
> > +.\"
> > +.\"- Redistributions of source code must retain the above copyright
> > +.\"  notice, this list of conditions and the following disclaimer.
> > +.\"- Redistributions in binary form must reproduce the above
> > +.\"  copyright notice, this list of conditions and the following
> > +.\"  disclaimer in the documentation and/or other materials provided
> > +.\"  with the distribution.
> > +.\"
> > +.\" THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> > +.\" "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> > +.\" LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
> > +.\" FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
> > +.\" COPYRIGHT HOLDERS OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
> > +.\" INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING,
> > +.\" BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
> > +.\" LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
> > +.\" CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
> > +.\" LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN
> > +.\" ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
> > +.\" POSSIBILITY OF SUCH DAMAGE.
> 
> Any reason for using a different license in documentation an code?

Different in what sense? Which paragraph do you mean?

I just copied it from some other man page in share/man/man4. Can't
remember which one it was. But they all say pretty much the same thing.


> 
> > +.Sh CAVEATS
> > +The
> > +.Nm
> > +driver currently does not support IPv6 addresses.
> > +.Pp
> > +Roaming can be prevented by the driver. This feature hasn't been tested.
> > +Please don't kill me in case your phone bills rack up sky high.
> 
> I wouldn't put the last sentence in this manual.  The license already
> say that.

Ok, it's gone.


> 
> > Index: sys/dev/usb/if_umbim.c
> > ===
> > RCS file: sys/dev/usb/if_umbim.c
> > diff -N sys/dev/usb/if_umbim.c
> > --- /dev/null   1 Jan 1970 00:00:00 -
> > +++ sys/dev/usb/if_umbim.c  1 Jun 2016 14:32:19 -
> > @@ -0,0 +1,2441 @@
> > +/* $OpenBSD$ */
> > +
> > +/*
> > + * Copyright (c) 2016 genua mbH
> > + * All rights reserved.
> > + *
> > + * Permission to use, copy, modify, and distribute this software for any
> > + * purpose with or without fee is hereby granted, provided that the above
> > + * copyright notice and this permission notice appear in all copies.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
> > + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
> > + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
> > + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
> > + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
> > + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
> > + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> > + */
> > +
> > +/*
> > + * Mobile Broadband Interface Model
> > + * http://www.usb.org/developers/docs/devclass_docs/MBIM-Compliance-1.0.pdf
> > + */
> > +#include "bpfilter.h"
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > 

Remove unused 'cpuprobe' from i386 bootblocks

[Tom Cosgrove]

Not used, not built, so can just be deleted.  Diff below.

Reduces the diff between i386 and amd64 bootblocks.

Thanks

Tom


Index: sys/arch/i386/stand/libsa/cpuprobe.c
===
RCS file: sys/arch/i386/stand/libsa/cpuprobe.c
diff -N sys/arch/i386/stand/libsa/cpuprobe.c
--- sys/arch/i386/stand/libsa/cpuprobe.c29 Mar 2014 18:09:29 -  
1.2
+++ /dev/null   1 Jan 1970 00:00:00 -
@@ -1,147 +0,0 @@
-/* $OpenBSD: cpuprobe.c,v 1.2 2014/03/29 18:09:29 guenther Exp $   */
-
-/*
- * Copyright (c) 2004 Tom Cosgrove 
- *
- * Permission to use, copy, modify, and distribute this software for any
- * purpose with or without fee is hereby granted, provided that the above
- * copyright notice and this permission notice appear in all copies.
- *
- * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
- * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
- * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
- * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
- * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
- * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
- * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
- */
-
-#include 
-#include 
-
-#include "libsa.h"
-
-int amd64_supported;
-static int cpu_family, cpu_model, cpu_stepping;
-static int psl_check;
-static u_int32_t feature_ecx, feature_edx, feature_amd;
-static char cpu_brandstr[48];  /* Includes term NUL byte */
-static char cpu_vendor[13];/* 12 chars plus NUL term */
-
-/*
- * cpuid instruction.  request in eax, result in eax, ebx, ecx, edx.
- * requires caller to provide u_int32_t regs[4] array.
- */
-u_int32_t
-cpuid(u_int32_t eax, u_int32_t *regs)
-{
-   __asm volatile(
-   "cpuid\n\t"
-   "movl   %%eax, 0(%2)\n\t"
-   "movl   %%ebx, 4(%2)\n\t"
-   "movl   %%ecx, 8(%2)\n\t"
-   "movl   %%edx, 12(%2)\n\t"
-   : "=a" (eax)
-   : "0" (eax), "S" (regs)
-   : "bx", "cx", "dx");
-
-   return eax;
-}
-
-void
-cpuprobe(void)
-{
-   u_int32_t cpuid_max, extended_max;
-   u_int32_t regs[4];
-
-   /*
-* The following is a simple check to see if cpuid is supported.
-* We try to toggle bit 21 (PSL_ID) in eflags.  If it works, then
-* cpuid is supported.  If not, there's no cpuid, and we don't
-* try it (don't want /boot to get an invalid opcode exception).
-*
-* XXX The NexGen Nx586 does not support this bit, so this is not
-* a good method to detect the presence of cpuid on this
-* processor.  That's fine: the purpose here is to detect the
-* absence of cpuid.  We don't mind if the instruction's not
-* there - this is not intended to determine exactly what
-* processor is there, just whether it's i386 or amd64.
-*
-* The only thing that would cause us grief is a processor which
-* does not support cpuid but which does allow the PSL_ID bit
-* in eflags to be toggled.
-*/
-   __asm volatile(
-   "pushfl\n\t"
-   "popl   %2\n\t"
-   "xorl   %2, %0\n\t"
-   "pushl  %0\n\t"
-   "popfl\n\t"
-   "pushfl\n\t"
-   "popl   %0\n\t"
-   "xorl   %2, %0\n\t" /* If %2 == %0, no cpuid */
-   : "=r" (psl_check)
-   : "0" (PSL_ID), "r" (0)
-   : "cc");
-
-   if (psl_check == PSL_ID) {  /* cpuid supported */
-   cpuid_max = cpuid(0, regs); /* Highest std call */
-
-   bcopy([1], cpu_vendor, sizeof(regs[1]));
-   bcopy([3], cpu_vendor + 4, sizeof(regs[3]));
-   bcopy([2], cpu_vendor + 8, sizeof(regs[2]));
-   cpu_vendor[sizeof(cpu_vendor) - 1] = '\0';
-
-   if (cpuid_max >= 1) {
-   u_int32_t id;
-
-   id = cpuid(1, regs);/* Get basic info */
-   cpu_stepping = id & 0x00f;
-   cpu_model = (id >> 4) & 0x000f;
-   cpu_family = (id >> 8) & 0x000f;
-
-   feature_ecx = regs[2];
-   feature_edx = regs[3];
-   }
-
-   extended_max = cpuid(0x8000, regs); /* Highest ext  */
-
-   if (extended_max >= 0x8001) {
-   cpuid(0x8001, regs);
-   feature_amd = regs[3];
-   if (feature_amd & CPUID_LONG)
-   amd64_supported = 1;
-   }
-
-   cpu_brandstr[0] = '\0';
-   if (extended_max >= 0x8004) {
-   

'continue' to appease style gods in i386,amd64 libsa

[Tom Cosgrove]

As per subject, a couple of empty loop bodies in the i396 and amd64 boot blocks.

Diff below.

Tom


Index: sys/arch/amd64/stand/libsa/biosdev.c
===
RCS file: /home/OpenBSD/cvs/src/sys/arch/amd64/stand/libsa/biosdev.c,v
retrieving revision 1.27
diff -u -p -u -r1.27 biosdev.c
--- sys/arch/amd64/stand/libsa/biosdev.c1 Oct 2015 20:28:12 -   
1.27
+++ sys/arch/amd64/stand/libsa/biosdev.c6 Jun 2016 21:04:07 -
@@ -671,7 +671,8 @@ biosdisk_err(u_int error)
register const u_char *p = bidos_errs;
 
while (*p && *p != error)
-   while (*p++);
+   while (*p++)
+   continue;
 
return ++p;
 }
@@ -703,7 +704,8 @@ biosdisk_errno(u_int error)
if (error == 0)
return 0;
 
-   for (p = tab; p->error && p->error != error; p++);
+   for (p = tab; p->error && p->error != error; p++)
+   continue;
 
return p->errno;
 }
Index: sys/arch/i386/stand/libsa/biosdev.c
===
RCS file: /home/OpenBSD/cvs/src/sys/arch/i386/stand/libsa/biosdev.c,v
retrieving revision 1.92
diff -u -p -u -r1.92 biosdev.c
--- sys/arch/i386/stand/libsa/biosdev.c 1 Oct 2015 20:28:12 -   1.92
+++ sys/arch/i386/stand/libsa/biosdev.c 6 Jun 2016 21:04:07 -
@@ -672,7 +672,8 @@ biosdisk_err(u_int error)
register const u_char *p = bidos_errs;
 
while (*p && *p != error)
-   while (*p++);
+   while (*p++)
+   continue;
 
return ++p;
 }
@@ -704,7 +705,8 @@ biosdisk_errno(u_int error)
if (error == 0)
return 0;
 
-   for (p = tab; p->error && p->error != error; p++);
+   for (p = tab; p->error && p->error != error; p++)
+   continue;
 
return p->errno;
 }

Re: using srp inside art

[Martin Pieuchot]

On 06/06/16(Mon) 17:14, Jonathan Matthew wrote:
> We've finally got srp and art to the point where we can use srp to manage the
> internal links in the art data structures.  This allows us to do route lookups
> without holding any locks, which is kind of nice.

It's awesome!

> As we're not doing unlocked insert, delete or walk (yet), those art functions
> use the locked variants of the srp functions.  Currently the lock that
> protects those operations is the kernel lock.
> 
> Callers to art_lookup and art_match now pass in a pointer to an srp_ref, which
> is always active on return, even if no route is found.  In some situations we
> use these functions while modifying the routing table, in which case the
> kernel lock already protects against concurrent modifications so the srp_ref
> is unnecessary, so we srp_leave it immediately.
> 
> I'd also like to draw attention to the comment in rtable_match.  Is it OK that
> we might choose a multipath route at random if the set of available routes
> changes while we're choosing?

I believe it's ok.  If for a short period of time the multi-path route
becomes single-path then the answer is obvious.

Now every time there's a change in a list of multipath route, it might
happen that for a given source, paths selected before and after the
change are different.  Simply because the order of the list mater and
there's no way to distinguish two routes with the same gateway and same
priority.

I have two nitpicks, but your diff is ok mpi@

> Index: art.h
> ===
> RCS file: /cvs/src/sys/net/art.h,v
> retrieving revision 1.13
> diff -u -p -u -p -r1.13 art.h
> --- art.h 3 Jun 2016 03:59:43 -   1.13
> +++ art.h 6 Jun 2016 00:32:31 -
> @@ -19,13 +19,15 @@
>  #ifndef _NET_ART_H_
>  #define _NET_ART_H_
>  
> +#include 

Could you keep the include in the .c file instead?

>  #define ART_MAXLVL   32  /* We currently use 32 levels for IPv6. */
>  
>  /*
>   * Root of the ART tables, equivalent to the radix head.
>   */
>  struct art_root {
> - struct art_table*ar_root;   /* First table */
> + struct srp   ar_root;   /* First table */
>   uint8_t  ar_bits[ART_MAXLVL];   /* Per level stride */
>   uint8_t  ar_nlvl;   /* Number of levels */
>   uint8_t  ar_alen;   /* Address length in bits */
>   while (1) {
> - /*
> -  * Rember the default route of this table in case
> -  * we do not find a better matching route.
> -  */
> - if (at->at_default != NULL)
> - dflt = at->at_default;
> -
>   /* Do a single level route lookup. */
>   j = art_findex(at, addr);
> + entry = srp_follow(nsr, >at_heap[j].node);
>  
> - /* If this is a leaf we're done. */
> - if (ISLEAF(at->at_heap[j]))
> + /* If this is a leaf (NULL is a leaf) we're done. */
> + if (ISLEAF(entry))
>   break;

I like the extended comments "(NULL is a leaf)", could you also put it
in art_lookup()?

Re: Set prio when bypassing pf(4)

[Martin Pieuchot]

On 06/06/16(Mon) 23:52, Vincent Gross wrote:
> On Mon, 6 Jun 2016 17:33:36 +0100
> Stuart Henderson  wrote:
> 
> > On 2016/06/06 16:15, Vincent Gross wrote:
> > > When sending ARP requests, or when writing to a bpf handle (as when
> > > sending DHCP Discover), we bypass pf(4) so we have no way to define
> > > the priority (m->m_pkthdr.pf.prio) of the outgoing packets.
> [...]
> > > 
> > > This diff adds
> > > 1) an if_llprio field to struct ifnet  
> > 
> > struct if_data.. this is used by enough ports that changing the abi
> [...]
> > 
> > > diff --git a/sbin/ifconfig/ifconfig.8 b/sbin/ifconfig/ifconfig.8  
> > 
> > BTW. patch warns about offsets if you apply this to -current.
> > 
> [...]
> > 
> > Other than these points, it seems a useful thing to do, pppoe could
> > use it too.
> > 
> > I wonder what these broken ISP devices are that require the
> > priority field in the vlan frame header to be 0 (aka "prio 1")...
> > 
> 
> r2 below. I moved if_llprio from if_data to struct ifnet, and went from
> u_char to u_int8_t. I also added a bound check in ifioctl().
> 
> Comments ? ok ?

Could you explain me why our default prio is 3?

Is there any use for this apart for vlan(4) interfaces?  Should it
really be part of "struct ifnet" ?

I also find weird to see a field inside ``m_pkthdr.pf'' being used
without pf(4).

> Index: sbin/ifconfig/ifconfig.8
> ===
> RCS file: /cvs/src/sbin/ifconfig/ifconfig.8,v
> retrieving revision 1.267
> diff -u -p -r1.267 ifconfig.8
> --- sbin/ifconfig/ifconfig.8  6 Apr 2016 10:07:14 -   1.267
> +++ sbin/ifconfig/ifconfig.8  6 Jun 2016 21:43:46 -
> @@ -327,6 +327,10 @@ Disable special processing at the link l
>  Change the link layer address (MAC address) of the interface.
>  This should be specified as six colon-separated hex values, or can
>  be chosen randomly.
> +.It Cm llprio Ar prio
> +Set the priority for link layer communications
> +.Pf ( Xr arp 4 ,
> +.Xr bpf 4 ) .
>  .It Cm media Op Ar type
>  Set the media type of the interface to
>  .Ar type .
> Index: sbin/ifconfig/ifconfig.c
> ===
> RCS file: /cvs/src/sbin/ifconfig/ifconfig.c,v
> retrieving revision 1.322
> diff -u -p -r1.322 ifconfig.c
> --- sbin/ifconfig/ifconfig.c  3 May 2016 17:52:33 -   1.322
> +++ sbin/ifconfig/ifconfig.c  6 Jun 2016 21:43:46 -
> @@ -135,6 +135,7 @@ char  name[IFNAMSIZ];
>  int  flags, xflags, setaddr, setipdst, doalias;
>  u_long   metric, mtu;
>  int  rdomainid;
> +int  llprio;
>  int  clearaddr, s;
>  int  newaddr = 0;
>  int  af = AF_INET;
> @@ -157,6 +158,7 @@ void  addaf(const char *, int);
>  void removeaf(const char *, int);
>  void setifbroadaddr(const char *, int);
>  void setifmtu(const char *, int);
> +void setifllprio(const char *, int);
>  void setifnwid(const char *, int);
>  void setifbssid(const char *, int);
>  void setifnwkey(const char *, int);
> @@ -521,6 +523,7 @@ const struct  cmd {
>   { "instance",   NEXTARG,A_MEDIAINST,setmediainst },
>   { "inst",   NEXTARG,A_MEDIAINST,setmediainst },
>   { "lladdr", NEXTARG,0,  setiflladdr },
> + { "llprio", NEXTARG,0,  setifllprio },
>   { NULL, /*src*/ 0,  0,  setifaddr },
>   { NULL, /*dst*/ 0,  0,  setifdstaddr },
>   { NULL, /*illegal*/0,   0,  NULL },
> @@ -854,6 +857,11 @@ getinfo(struct ifreq *ifr, int create)
>   else
>   rdomainid = ifr->ifr_rdomainid;
>  #endif
> + if (ioctl(s, SIOCGIFLLPRIO, (caddr_t)ifr) < 0)
> + llprio = 0;
> + else
> + llprio = ifr->ifr_llprio;
> +
>   return (0);
>  }
>  
> @@ -1411,6 +1419,21 @@ setifmtu(const char *val, int d)
>  
>  /* ARGSUSED */
>  void
> +setifllprio(const char *val, int d)
> +{
> + const char *errmsg = NULL;
> +
> + (void) strlcpy(ifr.ifr_name, name, sizeof(ifr.ifr_name));
> +
> + ifr.ifr_mtu = strtonum(val, 0, UCHAR_MAX, );
> + if (errmsg)
> + errx(1, "mtu %s: %s", val, errmsg);
> + if (ioctl(s, SIOCSIFLLPRIO, (caddr_t)) < 0)
> + warn("SIOCSIFLLPRIO");
> +}
> +
> +/* ARGSUSED */
> +void
>  setifgroup(const char *group_name, int dummy)
>  {
>   struct ifgroupreq ifgr;
> @@ -2894,6 +2917,7 @@ status(int link, struct sockaddr_dl *sdl
>   printf(" metric %lu", metric);
>   if (mtu)
>   printf(" mtu %lu", mtu);
> + printf(" llprio %lu", llprio);
>   putchar('\n');
>  #ifndef SMALL
>   if (showcapsflag)
> Index: sys/net/bpf.c
> ===
> RCS file: /cvs/src/sys/net/bpf.c,v
> retrieving revision 1.141
> diff -u -p -r1.141 bpf.c
> --- sys/net/bpf.c 18 May 2016 03:46:03 -  1.141
> +++ sys/net/bpf.c 6 Jun 2016 21:43:48 

typo in tcp_input.c

[Kapetanakis Giannis]

Just noticed this typo in tcp_input.c

G

Index: tcp_input.c
===
RCS file: /cvs/src/sys/netinet/tcp_input.c,v
retrieving revision 1.318
diff -u -p -u -p -r1.318 tcp_input.c
--- tcp_input.c 31 Mar 2016 13:11:14 -  1.318
+++ tcp_input.c 7 Jun 2016 08:36:39 -
@@ -3372,8 +3372,8 @@ syn_cache_insert(struct syn_cache *sc, s
 
 	/*

 * If there are no entries in the hash table, reinitialize
-* the hash secrets.  To avoid useless cache swaps and
-* and reinitialization, use it until the limit is reached.
+* the hash secrets. To avoid useless cache swaps and
+* reinitialization, use it until the limit is reached.
 */
if (set->scs_count == 0 && set->scs_use <= 0) {
arc4random_buf(set->scs_random, sizeof(set->scs_random));

Re: disklabel(8): refactor readlabel() for a better placed pledge

[Theo Buehler]

On Sun, May 29, 2016 at 10:55:48PM +0200, Theo Buehler wrote:
> The readlabel() function in disklabel() does two things: it reads the
> disklabel from the device using a ioctl() and then parses it into some
> strings.  We can't pledge beforehand since we have no way of knowing the
> file we process is actually a disk device.  However, once the ioctl()
> succeeds, we know that we deal with a disk and we can do all further
> processing of the untrusted data under pledge.
> 
> Thus, split up readlabel() into two functions and pledge between the
> two function calls.

Here's an updated version of this diff that also takes care of two
further things:

First, we need to call parse_autotable() before parselabel() because the
template will be used by the call to editor_allocspace() in parselabel().

Second, there still is a pledge problem:

$ ktrace disklabel -w /dev/tty floppy576
Abort trap (core dumped)
$ kdump | tail
 36196 disklabel RET   mmap 22796710084608/0x14bbc5ce8000
 36196 disklabel CALL  
mmap(0,0x1000,0x3,0x1002,-1,0)
 36196 disklabel RET   mmap 22795116298240/0x14bb66cf4000
 36196 disklabel CALL  pledge(0x14b92f938483,0)
 36196 disklabel STRU  pledge promise="stdio rpath wpath disklabel"
 36196 disklabel RET   pledge 0
 36196 disklabel CALL  ioctl(3,DIOCWDINFO,0x14b92fc4c180)
 36196 disklabel PLDG  ioctl, "ioctl", errno 1 Operation not permitted
 36196 disklabel PSIG  SIGABRT SIG_DFL
 36196 disklabel NAMI  "disklabel.core"

Since the DIOC{W,G}DINFO ioctls are called very late in the
(op == WRITE && (argc == 2 || argc == 3)) codepath, we either need to
drop pledge in that case or we can interpolate a dummy DIOCGDINFO call
that ensures the opened device is a disk device. We can then pledge
before parsing disktab(5). I went for the second option.

Index: disklabel.c
===
RCS file: /var/cvs/src/sbin/disklabel/disklabel.c,v
retrieving revision 1.220
diff -u -p -r1.220 disklabel.c
--- disklabel.c 1 Jun 2016 16:51:54 -   1.220
+++ disklabel.c 7 Jun 2016 06:15:13 -
@@ -206,23 +206,32 @@ main(int argc, char *argv[])
if (f < 0)
err(4, "%s", specname);
 
-   if (autotable != NULL)
-   parse_autotable(autotable);
-
-   if (op != WRITE || aflag || dflag)
+   if (op != WRITE || aflag || dflag) {
readlabel(f);
-   else if (argc == 2 || argc == 3)
-   makelabel(argv[1], argc == 3 ? argv[2] : NULL, );
-   else
-   usage();
 
-   if (op == EDIT || op == EDITOR || aflag) {
-   if (pledge("stdio rpath wpath cpath disklabel proc exec", NULL) 
== -1)
-   err(1, "pledge");
-   } else {
+   if (op == EDIT || op == EDITOR || aflag) {
+   if (pledge("stdio rpath wpath cpath disklabel proc "
+   "exec", NULL) == -1)
+   err(1, "pledge");
+   } else {
+   if (pledge("stdio rpath wpath disklabel", NULL) == -1)
+   err(1, "pledge");
+   }
+
+   if (autotable != NULL)
+   parse_autotable(autotable);
+   parselabel();
+   } else if (argc == 2 || argc == 3) {
+   /* Ensure f is a disk device before pledging. */
+   if (ioctl(f, DIOCGDINFO, ) < 0)
+   err(4, "ioctl DIOCGDINFO");
+
if (pledge("stdio rpath wpath disklabel", NULL) == -1)
err(1, "pledge");
-   }
+
+   makelabel(argv[1], argc == 3 ? argv[2] : NULL, );
+   } else
+   usage();
 
switch (op) {
case EDIT:
@@ -353,9 +362,6 @@ l_perror(char *s)
 void
 readlabel(int f)
 {
-   char *partname, *partduid;
-   struct fstab *fsent;
-   int i;
 
if (cflag && ioctl(f, DIOCRLDINFO) < 0)
err(4, "ioctl DIOCRLDINFO");
@@ -367,6 +373,14 @@ readlabel(int f)
if (ioctl(f, DIOCGDINFO, ) < 0)
err(4, "ioctl DIOCGDINFO");
}
+}
+
+void
+parselabel(void)
+{
+   char *partname, *partduid;
+   struct fstab *fsent;
+   int i;
 
i = asprintf(, "/dev/%s%c", dkname, 'a');
if (i == -1)
Index: extern.h
===
RCS file: /var/cvs/src/sbin/disklabel/extern.h,v
retrieving revision 1.27
diff -u -p -r1.27 extern.h
--- extern.h17 Oct 2015 13:27:08 -  1.27
+++ extern.h6 Jun 2016 19:16:37 -
@@ -28,6 +28,7 @@ void  display_partition(FILE *, struct di
 intduid_parse(struct disklabel *, char *);
 
 void   readlabel(int);
+void   parselabel(void);
 struct disklabel *makebootarea(char *, struct disklabel *);
 inteditor(int);
 void   editor_allocspace(struct disklabel *);

provide splraise on sparc64

[David Gwynne]

it would be nice to have splraise as an MI api within the kernel
so it could be used in some per cpu data structures. at the moment
the only way to use the ipl passed to such things is to use mutexes,
but then whats the point of having a per cpu data structure if
you're guaranteed to not content with other cpus (or be preempted)?

this is the first in a series of diffs i have to make splraise an
MI api, starting with sparc64.

most archs already have splraise in their MD code, or have it with
a weird name like _splraise or raiseipl or _cpu_intr_raise, but
sparc and sparc64 are special.

instead of aliasing splfoo() to splraise(IPL_FOO), sparc64 has
macros that create static inline functions for each spl function.
sparc64 mutexes are hand crafted in assembler so its never needed
an splraise function to call.

this refactors the sparc64 code so theres a single _splraise macro.
it is a macro because the code sequence is short, and so it can
take advantage of gcc constant detection which provides different
asm for immediate/constant ipl levels. the splfoo() things are then
defined to _splraise(IPL_FOO).

splraise itself is provided as a function that wraps the _splraise
macro, which gets the ipl level in register form of the asm.

this also removes the spl debug goo. spls have been solid for a
while, im pretty sure theyre ok.

ok?

Index: include/intr.h
===
RCS file: /cvs/src/sys/arch/sparc64/include/intr.h,v
retrieving revision 1.18
diff -u -p -r1.18 intr.h
--- include/intr.h  27 Sep 2015 11:29:20 -  1.18
+++ include/intr.h  7 Jun 2016 06:40:29 -
@@ -83,8 +83,29 @@ voidintr_establish(int, struct intrh
 #defineIPL_STATCLOCK   PIL_STATCLOCK   /* statclock */
 #defineIPL_HIGHPIL_HIGH/* everything */
 
+#define spl0() _spl(IPL_NONE)
+#define splsoftint()   _splraise(IPL_SOFTINT)
+#define splsoftclock() _splraise(IPL_SOFTCLOCK)
+#define splsoftnet()   _splraise(IPL_SOFTNET)
+#define splbio()   _splraise(IPL_BIO)
+#define splnet()   _splraise(IPL_NET)
+#define splsofttty()   _splraise(IPL_SOFTTTY)
+#define spltty()   _splraise(IPL_TTY)
+#define splvm()_splraise(IPL_VM)
+#define splaudio() _splraise(IPL_AUDIO)
+#define splclock() _splraise(IPL_CLOCK)
+#define splserial()_splraise(IPL_SERIAL)
+#define splsched() _splraise(IPL_SCHED)
+#define spllock()  _splraise(IPL_LOCK)
+#define splstatclock() _splraise(IPL_STATCLOCK)
+#define splhigh()  _splraise(IPL_HIGH)
+#define splx(_oldipl)  _splx(_oldipl)
+
+#define splzs()splserial()
+
 #defineIPL_MPSAFE  0x100
 
+int splraise(int);
 voidintr_barrier(void *);
 
 void   *softintr_establish(int, void (*)(void *), void *);
Index: include/psl.h
===
RCS file: /cvs/src/sys/arch/sparc64/include/psl.h,v
retrieving revision 1.30
diff -u -p -r1.30 psl.h
--- include/psl.h   7 Jun 2016 06:37:33 -   1.30
+++ include/psl.h   7 Jun 2016 06:40:29 -
@@ -317,156 +317,39 @@ stxa_sync(u_int64_t va, u_int64_t asi, u
intr_restore(s);
 }
 
-/*
- * GCC pseudo-functions for manipulating PIL
- */
+static inline int
+_spl(int newipl)
+{
+   int oldpil;
 
-#ifdef SPLDEBUG
-void prom_printf(const char *fmt, ...);
-extern int printspl;
-#define SPLPRINT(x)if(printspl) { int i=1000; prom_printf x ; 
while(i--); }
-#defineSPL(name, newpil)   
\
-extern __inline int name##X(const char *, int);
\
-extern __inline int name##X(const char *file, int line)
\
-{  \
-   u_int64_t oldpil = sparc_rdpr(pil); \
-   SPLPRINT(("{%s:%d %d=>%d}", file, line, oldpil, newpil));   \
-   sparc_wrpr(pil, newpil, 0); \
-   return (oldpil);\
-}
-/* A non-priority-decreasing version of SPL */
-#defineSPLHOLD(name, newpil) \
-extern __inline int name##X(const char *, int);
\
-extern __inline int name##X(const char * file, int line)   \
-{  \
-   int oldpil = sparc_rdpr(pil);   \
-   if (__predict_false((u_int64_t)newpil <= oldpil))   \
-   return (oldpil);\
-   SPLPRINT(("{%s:%d %d->!d}", file, line, oldpil, newpil));   \
-   sparc_wrpr(pil, newpil, 0); \
-   return (oldpil);\
-}
+   __asm volatile( "rdpr %%pil, %0 \n"
+   "wrpr %%g0, %1, %%pil   

Re: remove unused spls in sparc64

[Mark Kettenis]

> Date: Tue, 7 Jun 2016 12:57:16 +1000
> From: David Gwynne 
> 
> nothing using splsoftfd or splausoft on sparc64 so we can remove them.
> 
> ok?

ok kettenis@

> Index: dev/fd.c
> ===
> RCS file: /cvs/src/sys/arch/sparc64/dev/fd.c,v
> retrieving revision 1.45
> diff -u -p -r1.45 fd.c
> --- dev/fd.c  12 Jul 2014 18:44:43 -  1.45
> +++ dev/fd.c  7 Jun 2016 02:56:23 -
> @@ -542,7 +542,6 @@ fdcattach(fdc, pri)
>   return (-1);
>   }
>  
> - printf(" softpri %d", PIL_FDSOFT);
>   if (fdc->sc_flags & FDC_NOEJECT)
>   printf(": manual eject");
>   printf("\n");
> Index: include/psl.h
> ===
> RCS file: /cvs/src/sys/arch/sparc64/include/psl.h,v
> retrieving revision 1.29
> diff -u -p -r1.29 psl.h
> --- include/psl.h 7 Mar 2016 13:21:51 -   1.29
> +++ include/psl.h 7 Jun 2016 02:56:23 -
> @@ -46,8 +46,6 @@
>  
>  /* Interesting spl()s */
>  #define PIL_SCSI 3
> -#define PIL_FDSOFT   4
> -#define PIL_AUSOFT   4
>  #define PIL_BIO  5
>  #define PIL_VIDEO5
>  #define PIL_TTY  6
> @@ -402,12 +400,6 @@ SPLHOLD(splsoftint, 1)
>  #define  splsoftclocksplsoftint
>  #define  splsoftnet  splsoftint
>  
> -/* audio software interrupts are at software level 4 */
> -SPLHOLD(splausoft, PIL_AUSOFT)
> -
> -/* floppy software interrupts are at software level 4 too */
> -SPLHOLD(splfdsoft, PIL_FDSOFT)
> -
>  /* Block devices */
>  SPLHOLD(splbio, PIL_BIO)
>  
> @@ -447,8 +439,6 @@ SPLHOLD(splhigh, PIL_HIGH)
>  
>  #define  spl0()  spl0X(__FILE__, __LINE__)
>  #define  splsoftint()splsoftintX(__FILE__, __LINE__)
> -#define  splausoft() splausoftX(__FILE__, __LINE__)
> -#define  splfdsoft() splfdsoftX(__FILE__, __LINE__)
>  #define  splbio()splbioX(__FILE__, __LINE__)
>  #define  splnet()splnetX(__FILE__, __LINE__)
>  #define  spltty()splttyX(__FILE__, __LINE__)
> 
>