Re: cbfb: coreboot framebuffer console

[Joerg Jung]

Am 13.06.2016 um 12:35 schrieb Mark Kettenis :

>> Date: Sun, 12 Jun 2016 11:47:37 -0500
>> From: joshua stein 
>> 
>> Here's a new version with feedback from Miod and Ted.
>> 
>> It also fixes a bug on a Broadwell Chromebook (tested by Edward
>> Wandasiewicz) which has proper inteldrm but no vga, so cbfb was
>> getting chosen as the console early on but then not detaching when
>> inteldrm wanted to take over.  This detaches cbfb in the intel and
>> radeon drivers.
> 
> The addition of those hooks makes me even more convinced that efifb(4)
> and cbfb(4) should be the same driver.  The only thing that is really
> different is the data structure that is used to initialize the
> framebuffer parameters.

I tend to agree with Mark, these two should be merged.

> We should not get hung up too much about the driver name.  If it is
> felt that efifb(4) is inappropriate for these chromebooks with
> coreboot, we can always rename the driver.

Yes, renaming makes sense then.

>> Index: dev/wscons/wsconsio.h
>> ===
>> RCS file: /cvs/src/sys/dev/wscons/wsconsio.h,v
>> retrieving revision 1.74
>> diff -u -p -u -p -r1.74 wsconsio.h
>> --- dev/wscons/wsconsio.h30 Mar 2016 23:34:12 -1.74
>> +++ dev/wscons/wsconsio.h12 Jun 2016 16:40:46 -
>> @@ -348,6 +348,7 @@ struct wsmouse_calibcoords {
>> #defineWSDISPLAY_TYPE_INTELDRM69/* Intel KMS framebuffer */
>> #defineWSDISPLAY_TYPE_RADEONDRM 70/* ATI Radeon KMS framebuffer 
>> */
>> #defineWSDISPLAY_TYPE_EFIFB71/* EFI framebuffer */
>> +#defineWSDISPLAY_TYPE_CBFB72/* Coreboot framebuffer */
>> 
>> /* Basic display information.  Not applicable to all display types. */
>> struct wsdisplay_fbinfo {
>> Index: arch/amd64/amd64/cbfb.c
>> ===
>> RCS file: arch/amd64/amd64/cbfb.c
>> diff -N arch/amd64/amd64/cbfb.c
>> --- /dev/null1 Jan 1970 00:00:00 -
>> +++ arch/amd64/amd64/cbfb.c12 Jun 2016 16:40:46 -
>> @@ -0,0 +1,445 @@
>> +/*$OpenBSD$ */
>> +
>> +/*
>> + * Coreboot framebuffer console driver
>> + *
>> + * Copyright (c) 2016 joshua stein 
>> + * Copyright (c) 2015 YASUOKA Masahiko 
>> + *
>> + * 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 
>> +#include 
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#include 
>> +
>> +/* coreboot tables */
>> +
>> +struct cb_header {
>> +union {
>> +uint8_t signature[4]; /* "LBIO" */
>> +uint32_t signature32;
>> +};
>> +uint32_theader_bytes;
>> +uint32_theader_checksum;
>> +uint32_ttable_bytes;
>> +uint32_ttable_checksum;
>> +uint32_ttable_entries;
>> +};
>> +
>> +struct cb_framebuffer {
>> +uint64_tphysical_address;
>> +uint32_tx_resolution;
>> +uint32_ty_resolution;
>> +uint32_tbytes_per_line;
>> +uint8_tbits_per_pixel;
>> +uint8_tred_mask_pos;
>> +uint8_tred_mask_size;
>> +uint8_tgreen_mask_pos;
>> +uint8_tgreen_mask_size;
>> +uint8_tblue_mask_pos;
>> +uint8_tblue_mask_size;
>> +uint8_treserved_mask_pos;
>> +uint8_treserved_mask_size;
>> +};
>> +
>> +struct cb_entry {
>> +uint32_ttag;
>> +#define CB_TAG_VERSION  0x0004
>> +#define CB_TAG_FORWARD  0x0011
>> +#define CB_TAG_FRAMEBUFFER  0x0012
>> +uint32_tsize;
>> +union {
>> +charstring[0];
>> +uint64_t forward;
>> +struct cb_framebuffer fb;
>> +} u;
>> +};
>> +
>> +struct cbfb {
>> +struct rasops_info rinfo;
>> +int depth;
>> +paddr_t paddr;
>> +psize_t psize;
>> +
>> +struct cb_framebuffer table_cbfb;
>> +};
>> +
>> +struct cbfb_softc {
>> +struct device sc_dev;
>> +struct cbfb*sc_fb;
>> +};
>> +
>> +int cbfb_match(struct device *, void *, void *);
>> +void cbfb_attach(struct device *, struct device *, void *);
>> +void cbfb_rasops_preinit(struct cbfb *);
>> 

Re: IP_SENDSRCADDR [2/2] : add cmsg support

[Vincent Gross]

On Mon, 13 Jun 2016 19:57:15 +0200
Jeremie Courreges-Anglas  wrote:

> Vincent Gross  writes:
> 
> > Le Mon, 13 Jun 2016 07:35:16 +0200,
> > j...@wxcvbn.org (Jeremie Courreges-Anglas) a écrit :
> >  
> >> j...@wxcvbn.org (Jeremie Courreges-Anglas) writes:
> >>   
> >> > cc'ing sthen since he also has interest in IP_SENDSRCADDR
> >> >
> >> > Jeremie Courreges-Anglas  writes:
> >> >
> >> >> Vincent Gross  writes:
> >> >>
> >> >>> This diff adds support for IP_SENDSRCADDR cmsg on UDP sockets.
> >> >>> As for udp6_output(), we check that the source address+port is
> >> >>> available only if inp_laddr != *
> >> >>
> >> >> Your last IP_SENDSRCADDR diff didn't have that check, I think
> >> >> it is harmful.  If the socket is not bound then there is
> >> >> effectively no check performed by in_pcbaddrisavail(), thus I
> >> >> can use any random address. Other than this additional bypass
> >> >> check, your diff looks good to me.
> >> >>  
> > [...]  
> >> >>
> >> >> I haven't checked yet whether udp6_output is also affected.  If
> >> >> you folks already know that it isn't, please let me know.
> >> 
> >> The answer is "no", a few tests can't trigger the same problem.
> >> IIUC in6_selectsrc is responsible for rejection of non-local
> >> systems. Maybe we should take the same approach in netinet/, and
> >> extend in_selectsrc()?
> >> 
> >> --  
> >
> > While validating source address inside selection functions is the
> > right direction, I don't think it would be a good thing to extend
> > further in_selectsrc() prototype.  
> 
> I find it nice to have all the source address selection in one place.
> Or do you have another refactoring in mind?
> 

Uh, turns out I was operating on obsolete data. I would actually be
easy to shrink in_selectsrc() prototype to
(int)(struct in_addr **, struct sockaddr_in *, struct in_pcb *).
But this looks like a layering violation to me ... What do you think ?

$ grep -r in_selectsrc sys/net*
sys/netinet/in_pcb.c
sys/netinet/in_pcb.h
sys/netinet/udp_usrreq.c
$ cd sys/netinet
$ grep -A2 in_selectsrc
error = in_selectsrc(, sin, inp->inp_moptions,
>inp_route, >inp_laddr, inp->inp_rtableid);
if (error)
in_selectsrc(struct in_addr **insrc, struct sockaddr_in *sin,
struct ip_moptions *mopts, struct route *ro, struct in_addr *laddr,
u_int rtableid)
$ grep -A2 in_selectsrc udp_usrreq.c
error = in_selectsrc(, sin, inp->inp_moptions,
>inp_route, >inp_laddr, inp->inp_rtableid);
if (error)


> > However it is easy to add a check while
> > processing cmsg.
> >
> > rev2 below. Ok ?  
> 
> Nits below, looks fine otherwise.  The checks do detect addresses not
> configured on the system and overlaps of bound sockets.
> 
> >
> > diff --git a/share/man/man4/ip.4 b/share/man/man4/ip.4
> > index 111432b..154b0d1 100644
> > --- a/share/man/man4/ip.4
> > +++ b/share/man/man4/ip.4
> > @@ -290,6 +290,27 @@ cmsg_len = CMSG_LEN(sizeof(u_int))
> >  cmsg_level = IPPROTO_IP
> >  cmsg_type = IP_RECVRTABLE
> >  .Ed
> > +.Pp
> > +If the
> > +.Dv IP_SENDSRCADDR
> > +option is passed to a
> > +.Xr sendmsg 2
> > +call on a
> > +.Dv SOCK_DGRAM
> > +socket, the address passed along the
> > +.Vt cmsghdr
> > +structure will be used as the source of the outgoing
> > +.Tn UDP
> > +datagram.  The
> > +.Vt cmsghdr
> > +fields for
> > +.Xr sendmsg 2
> > +have the following values:  
> 
> I would have worded it "should have" here, since these are the values
> that the developer is supposed to pass.

Yes, I have to find a better wording for this part.

> 
> > +.Bd -literal -offset indent
> > +cmsg_len = CMSG_LEN(sizeof(struct in_addr))
> > +cmsg_level = IPPROTO_IP
> > +cmsg_type = IP_SENDSRCADDR
> > +.Ed
> >  .Ss "Multicast Options"
> >  .Tn IP
> >  multicasting is supported only on
> > diff --git a/sys/netinet/in.h b/sys/netinet/in.h
> > index adb1b30..bf8c95d 100644
> > --- a/sys/netinet/in.h
> > +++ b/sys/netinet/in.h
> > @@ -307,6 +307,7 @@ struct ip_opts {
> >  #define IP_RECVRTABLE  35   /* bool; receive rdomain
> > w/dgram */ #define IP_IPSECFLOWINFO 36   /* bool; IPsec flow
> > info for dgram */ #define IP_IPDEFTTL   37   /* int;
> > IP TTL system default */ +#define IP_SENDSRCADDR
> > 38   /* struct in_addr; source address to use */ 
> >  #define IP_RTABLE  0x1021  /* int; routing
> > table, see SO_RTABLE */ #define IP_DIVERTFL
> > 0x1022  /* int; divert direction flag opt */ diff --git
> > a/sys/netinet/udp_usrreq.c b/sys/netinet/udp_usrreq.c index
> > 1feea11..401ed7a 100644 --- a/sys/netinet/udp_usrreq.c
> > +++ b/sys/netinet/udp_usrreq.c
> > @@ -888,6 +888,7 @@ udp_output(struct inpcb *inp, struct mbuf *m,
> > struct mbuf *addr, struct sockaddr_in *sin = NULL;
> > struct udpiphdr *ui;
> > u_int32_t ipsecflowinfo = 0;
> > +   struct sockaddr_in src_sin;
> > int len = m->m_pkthdr.len;
> > struct 

Re: nfs_vnops: nfs_lookitup - uninitialized var

[David Hill]

Any thoughts on this?

On Mon, Jun 06, 2016 at 01:36:05PM -0400, David Hill wrote:
> Hello -
> 
> Clang reports a possible user of an uninitalized variable in
> nfs_vnops.c line 2605.
> 
> attrflag is uninitialized when calling nfsm_postop_attr(), which is a
> macro that only sets attrflag if (info.nmi_mrep != NULL).  I am not sure
> if that is possible, but maybe a KASSERT(info.nmi_mrep != NULL) before
> nfsm_postop_attr() would catch a bug...
> 
> Or just silence it with:
> 
> Index: nfs_vnops.c
> ===
> RCS file: /cvs/src/sys/nfs/nfs_vnops.c,v
> retrieving revision 1.169
> diff -u -p -r1.169 nfs_vnops.c
> --- nfs_vnops.c   29 Apr 2016 14:40:36 -  1.169
> +++ nfs_vnops.c   6 Jun 2016 17:34:24 -
> @@ -2562,7 +2562,7 @@ nfs_lookitup(struct vnode *dvp, char *na
>   struct vnode *newvp = NULL;
>   struct nfsnode *np, *dnp = VTONFS(dvp);
>   caddr_t cp2;
> - int error = 0, fhlen, attrflag;
> + int error = 0, fhlen, attrflag = 0;
>   nfsfh_t *nfhp;
>  
>   info.nmi_v3 = NFS_ISV3(dvp);
> 

Re: IP_SENDSRCADDR [1/2] : move cmsg handling code

[Jeremie Courreges-Anglas]

Vincent Gross  writes:

> On Sun, 12 Jun 2016 15:00:14 +0200
> Vincent Gross  wrote:
>
> Damn you autowrap ! get off my diff !
>
> (thanks jca@ for spotting)
>
>> This diff moves the cmsg handling code on top of udp_output(). I split
>> the whole IP_SENDSRCADDR thung in two chunks so that it's easier to
>> audit.
>> 
>> ok ?

Looks fine, ok jca@ with the nits below addressed.

> diff --git a/sys/netinet/udp_usrreq.c b/sys/netinet/udp_usrreq.c
> index 2db5998..1feea11 100644
> --- a/sys/netinet/udp_usrreq.c
> +++ b/sys/netinet/udp_usrreq.c
> @@ -906,6 +906,47 @@ udp_output(struct inpcb *inp, struct mbuf *m, struct 
> mbuf *addr,
>   goto release;
>   }
>  
> + if (control) {
> + u_int clen;
> + struct cmsghdr *cm;
> + caddr_t cmsgs;
> +
> + /*
> +  * XXX: Currently, we assume all the optional information is 
> stored

You only moved this comment line, but please keep it under 80 chars.

> +  * in a single mbuf.
> +  */
> + if (control->m_next) {
> + error = EINVAL;
> + goto release;
> + }
> +
> + clen = control->m_len;
> + cmsgs = mtod(control, caddr_t);
> + do {
> + if (clen < CMSG_LEN(0)) {
> + error = EINVAL;
> + goto release;
> + }
> + cm = (struct cmsghdr *)cmsgs;
> + if (cm->cmsg_len < CMSG_LEN(0) ||
> + CMSG_ALIGN(cm->cmsg_len) > clen) {
> + error = EINVAL;
> + goto release;
> + }
> +#ifdef IPSEC
> + if (ISSET(inp->inp_flags,INP_IPSECFLOWINFO) &&

I have no opinion about ISSET in general but this diff shouldn't
introduce it.

> + cm->cmsg_len == CMSG_LEN(sizeof(ipsecflowinfo)) &&
> + cm->cmsg_level == IPPROTO_IP &&
> + cm->cmsg_type == IP_IPSECFLOWINFO) {
> + ipsecflowinfo = *(u_int32_t *)CMSG_DATA(cm);
> + break;
> + }
> +#endif
> + clen -= CMSG_ALIGN(cm->cmsg_len);
> + cmsgs += CMSG_ALIGN(cm->cmsg_len);
> + } while (clen);
> + }
> +
>   if (addr) {
>   sin = mtod(addr, struct sockaddr_in *);
>  
> @@ -947,45 +988,6 @@ udp_output(struct inpcb *inp, struct mbuf *m, struct 
> mbuf *addr,
>   laddr = >inp_laddr;
>   }
>  
> -#ifdef IPSEC
> - if (control && (inp->inp_flags & INP_IPSECFLOWINFO) != 0) {
> - u_int clen;
> - struct cmsghdr *cm;
> - caddr_t cmsgs;
> -
> - /*
> -  * XXX: Currently, we assume all the optional information is 
> stored
> -  * in a single mbuf.
> -  */
> - if (control->m_next) {
> - error = EINVAL;
> - goto release;
> - }
> -
> - clen = control->m_len;
> - cmsgs = mtod(control, caddr_t);
> - do {
> - if (clen < CMSG_LEN(0)) {
> - error = EINVAL;
> - goto release;
> - }
> - cm = (struct cmsghdr *)cmsgs;
> - if (cm->cmsg_len < CMSG_LEN(0) ||
> - CMSG_ALIGN(cm->cmsg_len) > clen) {
> - error = EINVAL;
> - goto release;
> - }
> - if (cm->cmsg_len == CMSG_LEN(sizeof(ipsecflowinfo)) &&
> - cm->cmsg_level == IPPROTO_IP &&
> - cm->cmsg_type == IP_IPSECFLOWINFO) {
> - ipsecflowinfo = *(u_int32_t *)CMSG_DATA(cm);
> - break;
> - }
> - clen -= CMSG_ALIGN(cm->cmsg_len);
> - cmsgs += CMSG_ALIGN(cm->cmsg_len);
> - } while (clen);
> - }
> -#endif
>   /*
>* Calculate data length and get a mbuf
>* for UDP and IP headers.
>

-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Re: IP_SENDSRCADDR [2/2] : add cmsg support

[Jeremie Courreges-Anglas]

Vincent Gross  writes:

> Le Mon, 13 Jun 2016 07:35:16 +0200,
> j...@wxcvbn.org (Jeremie Courreges-Anglas) a écrit :
>
>> j...@wxcvbn.org (Jeremie Courreges-Anglas) writes:
>> 
>> > cc'ing sthen since he also has interest in IP_SENDSRCADDR
>> >
>> > Jeremie Courreges-Anglas  writes:
>> >  
>> >> Vincent Gross  writes:
>> >>  
>> >>> This diff adds support for IP_SENDSRCADDR cmsg on UDP sockets. As
>> >>> for udp6_output(), we check that the source address+port is
>> >>> available only if inp_laddr != *  
>> >>
>> >> Your last IP_SENDSRCADDR diff didn't have that check, I think it is
>> >> harmful.  If the socket is not bound then there is effectively no
>> >> check performed by in_pcbaddrisavail(), thus I can use any random
>> >> address. Other than this additional bypass check, your diff looks
>> >> good to me.
>> >>
> [...]
>> >>
>> >> I haven't checked yet whether udp6_output is also affected.  If you
>> >> folks already know that it isn't, please let me know.  
>> 
>> The answer is "no", a few tests can't trigger the same problem.  IIUC
>> in6_selectsrc is responsible for rejection of non-local systems.
>> Maybe we should take the same approach in netinet/, and extend
>> in_selectsrc()?
>> 
>> --
>
> While validating source address inside selection functions is the right
> direction, I don't think it would be a good thing to extend further
> in_selectsrc() prototype.

I find it nice to have all the source address selection in one place.
Or do you have another refactoring in mind?

> However it is easy to add a check while
> processing cmsg.
>
> rev2 below. Ok ?

Nits below, looks fine otherwise.  The checks do detect addresses not
configured on the system and overlaps of bound sockets.

>
> diff --git a/share/man/man4/ip.4 b/share/man/man4/ip.4
> index 111432b..154b0d1 100644
> --- a/share/man/man4/ip.4
> +++ b/share/man/man4/ip.4
> @@ -290,6 +290,27 @@ cmsg_len = CMSG_LEN(sizeof(u_int))
>  cmsg_level = IPPROTO_IP
>  cmsg_type = IP_RECVRTABLE
>  .Ed
> +.Pp
> +If the
> +.Dv IP_SENDSRCADDR
> +option is passed to a
> +.Xr sendmsg 2
> +call on a
> +.Dv SOCK_DGRAM
> +socket, the address passed along the
> +.Vt cmsghdr
> +structure will be used as the source of the outgoing
> +.Tn UDP
> +datagram.  The
> +.Vt cmsghdr
> +fields for
> +.Xr sendmsg 2
> +have the following values:

I would have worded it "should have" here, since these are the values
that the developer is supposed to pass.

> +.Bd -literal -offset indent
> +cmsg_len = CMSG_LEN(sizeof(struct in_addr))
> +cmsg_level = IPPROTO_IP
> +cmsg_type = IP_SENDSRCADDR
> +.Ed
>  .Ss "Multicast Options"
>  .Tn IP
>  multicasting is supported only on
> diff --git a/sys/netinet/in.h b/sys/netinet/in.h
> index adb1b30..bf8c95d 100644
> --- a/sys/netinet/in.h
> +++ b/sys/netinet/in.h
> @@ -307,6 +307,7 @@ struct ip_opts {
>  #define IP_RECVRTABLE35   /* bool; receive rdomain w/dgram */
>  #define IP_IPSECFLOWINFO 36   /* bool; IPsec flow info for dgram */
>  #define IP_IPDEFTTL  37   /* int; IP TTL system default */
> +#define IP_SENDSRCADDR   38   /* struct in_addr; source address 
> to use */
>  
>  #define IP_RTABLE0x1021  /* int; routing table, see SO_RTABLE */
>  #define IP_DIVERTFL  0x1022  /* int; divert direction flag opt */
> diff --git a/sys/netinet/udp_usrreq.c b/sys/netinet/udp_usrreq.c
> index 1feea11..401ed7a 100644
> --- a/sys/netinet/udp_usrreq.c
> +++ b/sys/netinet/udp_usrreq.c
> @@ -888,6 +888,7 @@ udp_output(struct inpcb *inp, struct mbuf *m, struct mbuf 
> *addr,
>   struct sockaddr_in *sin = NULL;
>   struct udpiphdr *ui;
>   u_int32_t ipsecflowinfo = 0;
> + struct sockaddr_in src_sin;
>   int len = m->m_pkthdr.len;
>   struct in_addr *laddr;
>   int error = 0;
> @@ -906,6 +907,8 @@ udp_output(struct inpcb *inp, struct mbuf *m, struct mbuf 
> *addr,
>   goto release;
>   }
>  
> + memset(_sin, 0, sizeof(src_sin));
> +
>   if (control) {
>   u_int clen;
>   struct cmsghdr *cm;
> @@ -939,9 +942,20 @@ udp_output(struct inpcb *inp, struct mbuf *m, struct 
> mbuf *addr,
>   cm->cmsg_level == IPPROTO_IP &&
>   cm->cmsg_type == IP_IPSECFLOWINFO) {
>   ipsecflowinfo = *(u_int32_t *)CMSG_DATA(cm);
> - break;
> - }
> + } else
>  #endif
> + if (cm->cmsg_len == CMSG_LEN(sizeof(struct in_addr)) &&
> + cm->cmsg_level == IPPROTO_IP &&
> + cm->cmsg_type == IP_SENDSRCADDR) {
> + bzero(_sin, sizeof(src_sin));

No need for bzero here since you unconditionally call memset above.

> + memcpy(_sin.sin_addr, CMSG_DATA(cm),
> + sizeof(struct in_addr));
> +   

Re: IP_SENDSRCADDR [2/2] : add cmsg support

[Vincent Gross]

Le Mon, 13 Jun 2016 07:35:16 +0200,
j...@wxcvbn.org (Jérémie Courrèges-Anglas) a écrit :

> j...@wxcvbn.org (Jeremie Courreges-Anglas) writes:
> 
> > cc'ing sthen since he also has interest in IP_SENDSRCADDR
> >
> > Jeremie Courreges-Anglas  writes:
> >  
> >> Vincent Gross  writes:
> >>  
> >>> This diff adds support for IP_SENDSRCADDR cmsg on UDP sockets. As
> >>> for udp6_output(), we check that the source address+port is
> >>> available only if inp_laddr != *  
> >>
> >> Your last IP_SENDSRCADDR diff didn't have that check, I think it is
> >> harmful.  If the socket is not bound then there is effectively no
> >> check performed by in_pcbaddrisavail(), thus I can use any random
> >> address. Other than this additional bypass check, your diff looks
> >> good to me.
> >>
[...]
> >>
> >> I haven't checked yet whether udp6_output is also affected.  If you
> >> folks already know that it isn't, please let me know.  
> 
> The answer is "no", a few tests can't trigger the same problem.  IIUC
> in6_selectsrc is responsible for rejection of non-local systems.
> Maybe we should take the same approach in netinet/, and extend
> in_selectsrc()?
> 
> --

While validating source address inside selection functions is the right
direction, I don't think it would be a good thing to extend further
in_selectsrc() prototype. However it is easy to add a check while
processing cmsg.

rev2 below. Ok ?


diff --git a/share/man/man4/ip.4 b/share/man/man4/ip.4
index 111432b..154b0d1 100644
--- a/share/man/man4/ip.4
+++ b/share/man/man4/ip.4
@@ -290,6 +290,27 @@ cmsg_len = CMSG_LEN(sizeof(u_int))
 cmsg_level = IPPROTO_IP
 cmsg_type = IP_RECVRTABLE
 .Ed
+.Pp
+If the
+.Dv IP_SENDSRCADDR
+option is passed to a
+.Xr sendmsg 2
+call on a
+.Dv SOCK_DGRAM
+socket, the address passed along the
+.Vt cmsghdr
+structure will be used as the source of the outgoing
+.Tn UDP
+datagram.  The
+.Vt cmsghdr
+fields for
+.Xr sendmsg 2
+have the following values:
+.Bd -literal -offset indent
+cmsg_len = CMSG_LEN(sizeof(struct in_addr))
+cmsg_level = IPPROTO_IP
+cmsg_type = IP_SENDSRCADDR
+.Ed
 .Ss "Multicast Options"
 .Tn IP
 multicasting is supported only on
diff --git a/sys/netinet/in.h b/sys/netinet/in.h
index adb1b30..bf8c95d 100644
--- a/sys/netinet/in.h
+++ b/sys/netinet/in.h
@@ -307,6 +307,7 @@ struct ip_opts {
 #define IP_RECVRTABLE  35   /* bool; receive rdomain w/dgram */
 #define IP_IPSECFLOWINFO   36   /* bool; IPsec flow info for dgram */
 #define IP_IPDEFTTL37   /* int; IP TTL system default */
+#define IP_SENDSRCADDR 38   /* struct in_addr; source address to use */
 
 #define IP_RTABLE  0x1021  /* int; routing table, see SO_RTABLE */
 #define IP_DIVERTFL0x1022  /* int; divert direction flag opt */
diff --git a/sys/netinet/udp_usrreq.c b/sys/netinet/udp_usrreq.c
index 1feea11..401ed7a 100644
--- a/sys/netinet/udp_usrreq.c
+++ b/sys/netinet/udp_usrreq.c
@@ -888,6 +888,7 @@ udp_output(struct inpcb *inp, struct mbuf *m, struct mbuf 
*addr,
struct sockaddr_in *sin = NULL;
struct udpiphdr *ui;
u_int32_t ipsecflowinfo = 0;
+   struct sockaddr_in src_sin;
int len = m->m_pkthdr.len;
struct in_addr *laddr;
int error = 0;
@@ -906,6 +907,8 @@ udp_output(struct inpcb *inp, struct mbuf *m, struct mbuf 
*addr,
goto release;
}
 
+   memset(_sin, 0, sizeof(src_sin));
+
if (control) {
u_int clen;
struct cmsghdr *cm;
@@ -939,9 +942,20 @@ udp_output(struct inpcb *inp, struct mbuf *m, struct mbuf 
*addr,
cm->cmsg_level == IPPROTO_IP &&
cm->cmsg_type == IP_IPSECFLOWINFO) {
ipsecflowinfo = *(u_int32_t *)CMSG_DATA(cm);
-   break;
-   }
+   } else
 #endif
+   if (cm->cmsg_len == CMSG_LEN(sizeof(struct in_addr)) &&
+   cm->cmsg_level == IPPROTO_IP &&
+   cm->cmsg_type == IP_SENDSRCADDR) {
+   bzero(_sin, sizeof(src_sin));
+   memcpy(_sin.sin_addr, CMSG_DATA(cm),
+   sizeof(struct in_addr));
+   src_sin.sin_family = AF_INET;
+   src_sin.sin_len = sizeof(src_sin);
+   /* no check on reuse done when sin->sin_port == 
0 */
+   if ((error = in_pcbaddrisavail(inp, _sin, 
0, curproc)))
+   goto release;
+   }
clen -= CMSG_ALIGN(cm->cmsg_len);
cmsgs += CMSG_ALIGN(cm->cmsg_len);
} while (clen);
@@ -980,6 +994,17 @@ udp_output(struct inpcb *inp, struct mbuf *m, struct mbuf 
*addr,
if (error)
   

Fix return value of OF_getprop() when name has a '@' in it

[Tom Cosgrove]

In OF_getprop(), if the "name" property doesn't exist it is synthesised
from the unit name.  If that synthesised property has an "@" in it, we
truncate the name just before the "@", but we currently continue to return
the full length of the unit name.

This diff returns the length of the truncated name.

Thanks

Tom


Index: sys/dev/ofw/fdt.c
===
RCS file: /home/OpenBSD/cvs/src/sys/dev/ofw/fdt.c,v
retrieving revision 1.12
diff -u -p -u -r1.12 fdt.c
--- sys/dev/ofw/fdt.c   12 Jun 2016 12:55:42 -  1.12
+++ sys/dev/ofw/fdt.c   12 Jun 2016 20:59:31 -
@@ -782,8 +782,10 @@ OF_getprop(int handle, char *prop, void 
if (data) {
len = strlcpy(buf, data, buflen);
data = strchr(buf, '@');
-   if (data)
+   if (data) {
*data = 0;
+   return data - (char *)buf + 1;
+   }
return len + 1;
}
}

fdt_node_property() fix

[Mark Kettenis]

Currently it is not possible to figure out whether a property isn't
there, or whether it has a length of zero.  Zero-length properties do
exist (both in the FDT world and the OFW world), so we need to fix
this.  The most logical way to do this is to make fdt_node_property()
return -1 if a property could not be found.

ok?


Index: arch/armv7/armv7/armv7_machdep.c
===
RCS file: /cvs/src/sys/arch/armv7/armv7/armv7_machdep.c,v
retrieving revision 1.30
diff -u -p -r1.30 armv7_machdep.c
--- arch/armv7/armv7/armv7_machdep.c12 Jun 2016 01:01:12 -  1.30
+++ arch/armv7/armv7/armv7_machdep.c13 Jun 2016 12:45:57 -
@@ -912,7 +912,7 @@ fdt_find_cons(const char *name)
/* First check if "stdout-path" is set. */
node = fdt_find_node("/chosen");
if (node) {
-   if (fdt_node_property(node, "stdout-path", )) {
+   if (fdt_node_property(node, "stdout-path", ) > 0) {
if (strchr(stdout, ':') != NULL) {
strlcpy(buf, stdout, sizeof(buf));
if ((p = strchr(buf, ':')) != NULL)
Index: dev/ofw/fdt.c
===
RCS file: /cvs/src/sys/dev/ofw/fdt.c,v
retrieving revision 1.12
diff -u -p -r1.12 fdt.c
--- dev/ofw/fdt.c   12 Jun 2016 12:55:42 -  1.12
+++ dev/ofw/fdt.c   13 Jun 2016 12:45:58 -
@@ -171,12 +171,12 @@ fdt_node_property(void *node, char *name
char *tmp;

if (!tree_inited)
-   return 0;
+   return -1;
 
ptr = (u_int32_t *)node;
 
if (betoh32(*ptr) != FDT_NODE_BEGIN)
-   return 0;
+   return -1;
 
ptr = skip_node_name(ptr + 1);
 
@@ -189,7 +189,7 @@ fdt_node_property(void *node, char *name
}
ptr = skip_property(ptr);
}
-   return 0;
+   return -1;
 }
 
 /*
@@ -750,7 +750,7 @@ OF_getproplen(int handle, char *prop)
 * flattened device tree specification, so we synthesize one
 * from the unit name of the node if it is missing.
 */
-   if (len == 0 && strcmp(prop, "name") == 0) {
+   if (len < 0 && strcmp(prop, "name") == 0) {
name = fdt_node_name(node);
data = strchr(name, '@');
if (data)
@@ -777,7 +777,7 @@ OF_getprop(int handle, char *prop, void 
 * flattened device tree specification, so we synthesize one
 * from the unit name of the node if it is missing.
 */
-   if (len == 0 && strcmp(prop, "name") == 0) {
+   if (len < 0 && strcmp(prop, "name") == 0) {
data = fdt_node_name(node);
if (data) {
len = strlcpy(buf, data, buflen);
@@ -813,7 +813,7 @@ OF_getpropintarray(int handle, char *pro
int i;
 
len = OF_getprop(handle, prop, buf, buflen);
-   if (len <0 || (len % sizeof(uint32_t)))
+   if (len < 0 || (len % sizeof(uint32_t)))
return -1;
 
for (i = 0; i < len / sizeof(uint32_t); i++)

Re: armv7 / efi failure on imx6q SabreLite

[Mark Kettenis]

> Date: Sun, 12 Jun 2016 19:48:24 +0200
> From: Matthieu Herrb 
> 
> Hi,
> 
> I'm trying to convert my SabreLite board to boot via efiboot, but
> without success so far.
> 
> I'm using the nitrogen6q u-boot from ports
> 
> the boot loader is loaded and runs but it
> fails to find the sata disk to load the kernel:
> 
> => fatload sata 0:1 0x1080 efi/boot/bootarm.efi
> reading efi/boot/bootarm.efi
> 65556 bytes read in 9 ms (6.9 MiB/s)
> => fatload sata 0:1 0x1300 imx6q-sabrelite.dtb
> reading imx6q-sabrelite.dtb
> 36834 bytes read in 8 ms (4.4 MiB/s)
> =>  bootefi 0x1080 0x1300
> ## Starting EFI application at 0x1080 ...
> Scanning disks on sata...
> Scanning disks on usb...
> Scanning disks on mmc...
> MMC: no card present
> MMC Device 2 not found
> MMC Device 3 not found
> Found 2 disks

The firmware found 2 disks.  Perhaps there is something wrong with
efiboot boot device selection code.  And unfortunately efiboot doesn't
currently support selecting alternative boot devices.

> >> OpenBSD/armv7 BOOTARM 0.1
> boot> 
> cannot open sd0a:/etc/random.seed: Device not configured
> booting sd0a:/bsd: open sd0a:/bsd: Device not configured
>  failed(6). will try /bsd
> boot>

This suggests that we actually didn't find the boot device.  So the
problem might be that the boot device selection code in efiboot
doesn't work for SATA devices.

Re: MBIM Patch (Round 3)

[Martin Pieuchot]

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.

Re: cbfb: coreboot framebuffer console

[Mark Kettenis]

> Date: Sun, 12 Jun 2016 11:47:37 -0500
> From: joshua stein 
> 
> Here's a new version with feedback from Miod and Ted.
> 
> It also fixes a bug on a Broadwell Chromebook (tested by Edward
> Wandasiewicz) which has proper inteldrm but no vga, so cbfb was
> getting chosen as the console early on but then not detaching when
> inteldrm wanted to take over.  This detaches cbfb in the intel and
> radeon drivers.

The addition of those hooks makes me even more convinced that efifb(4)
and cbfb(4) should be the same driver.  The only thing that is really
different is the data structure that is used to initialize the
framebuffer parameters.

We should not get hung up too much about the driver name.  If it is
felt that efifb(4) is inappropriate for these chromebooks with
coreboot, we can always rename the driver.

> Index: dev/wscons/wsconsio.h
> ===
> RCS file: /cvs/src/sys/dev/wscons/wsconsio.h,v
> retrieving revision 1.74
> diff -u -p -u -p -r1.74 wsconsio.h
> --- dev/wscons/wsconsio.h 30 Mar 2016 23:34:12 -  1.74
> +++ dev/wscons/wsconsio.h 12 Jun 2016 16:40:46 -
> @@ -348,6 +348,7 @@ struct wsmouse_calibcoords {
>  #define  WSDISPLAY_TYPE_INTELDRM 69  /* Intel KMS 
> framebuffer */
>  #define  WSDISPLAY_TYPE_RADEONDRM 70 /* ATI Radeon KMS 
> framebuffer */
>  #define  WSDISPLAY_TYPE_EFIFB71  /* EFI framebuffer */
> +#define  WSDISPLAY_TYPE_CBFB 72  /* Coreboot framebuffer 
> */
>  
>  /* Basic display information.  Not applicable to all display types. */
>  struct wsdisplay_fbinfo {
> Index: arch/amd64/amd64/cbfb.c
> ===
> RCS file: arch/amd64/amd64/cbfb.c
> diff -N arch/amd64/amd64/cbfb.c
> --- /dev/null 1 Jan 1970 00:00:00 -
> +++ arch/amd64/amd64/cbfb.c   12 Jun 2016 16:40:46 -
> @@ -0,0 +1,445 @@
> +/*   $OpenBSD$ */
> +
> +/*
> + * Coreboot framebuffer console driver
> + *
> + * Copyright (c) 2016 joshua stein 
> + * Copyright (c) 2015 YASUOKA Masahiko 
> + *
> + * 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 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +/* coreboot tables */
> +
> +struct cb_header {
> + union {
> + uint8_t signature[4]; /* "LBIO" */
> + uint32_t signature32;
> + };
> + uint32_theader_bytes;
> + uint32_theader_checksum;
> + uint32_ttable_bytes;
> + uint32_ttable_checksum;
> + uint32_ttable_entries;
> +};
> +
> +struct cb_framebuffer {
> + uint64_tphysical_address;
> + uint32_tx_resolution;
> + uint32_ty_resolution;
> + uint32_tbytes_per_line;
> + uint8_t bits_per_pixel;
> + uint8_t red_mask_pos;
> + uint8_t red_mask_size;
> + uint8_t green_mask_pos;
> + uint8_t green_mask_size;
> + uint8_t blue_mask_pos;
> + uint8_t blue_mask_size;
> + uint8_t reserved_mask_pos;
> + uint8_t reserved_mask_size;
> +};
> +
> +struct cb_entry {
> + uint32_ttag;
> +#define CB_TAG_VERSION  0x0004
> +#define CB_TAG_FORWARD  0x0011
> +#define CB_TAG_FRAMEBUFFER  0x0012
> + uint32_tsize;
> + union {
> + charstring[0];
> + uint64_t forward;
> + struct cb_framebuffer fb;
> + } u;
> +};
> +
> +struct cbfb {
> + struct rasops_info rinfo;
> + int  depth;
> + paddr_t  paddr;
> + psize_t  psize;
> +
> + struct cb_framebuffertable_cbfb;
> +};
> +
> +struct cbfb_softc {
> + struct devicesc_dev;
> + struct cbfb *sc_fb;
> +};
> +
> +int   cbfb_match(struct device *, void *, void *);
> +void  cbfb_attach(struct device *, struct device *, void *);
> +void  cbfb_rasops_preinit(struct cbfb *);
> +int   cbfb_ioctl(void *, u_long, caddr_t, int, struct proc *);
> +paddr_t   cbfb_mmap(void *, off_t, int);
> +int   cbfb_alloc_screen(void *, const struct 

Re: pool_setipl for src/sys/uvm

[Mark Kettenis]

> Date: Mon, 13 Jun 2016 14:27:53 +1000
> From: David Gwynne 
> 
> this adds pool_setipl to the pools in src/sys/uvm.
> 
> this assumes that the various things are only allocated and freed
> from a process context. if any of these are released from an interrupt
> we should probably set them to IPL_VM.
> 
> ok?

The IPL_VM for uvm_map_kmem_entry_pool is definitely not necessary.
That pool is only used for non-interrupt-safe maps, which means the
pool should never be used in interrupt-context.

That also means the existing IPL_VM for uvm_map_entry_pool shouldn't
be necessary either.  That would need some testing though, so it's
fine if you leave it like that for now (and perhaps add an XXX).

> Index: uvm_amap.c
> ===
> RCS file: /cvs/src/sys/uvm/uvm_amap.c,v
> retrieving revision 1.71
> diff -u -p -r1.71 uvm_amap.c
> --- uvm_amap.c26 May 2016 13:37:26 -  1.71
> +++ uvm_amap.c13 Jun 2016 04:26:23 -
> @@ -227,6 +227,7 @@ amap_init(void)
>   /* Initialize the vm_amap pool. */
>   pool_init(_amap_pool, sizeof(struct vm_amap), 0, 0, PR_WAITOK,
>   "amappl", NULL);
> + pool_setipl(_amap_pool, IPL_NONE);
>   pool_sethiwat(_amap_pool, 4096);
>  
>   /* initialize small amap pools */
> @@ -237,12 +238,14 @@ amap_init(void)
>   (i + 1) * sizeof(struct vm_anon *);
>   pool_init(_small_amap_pool[i], size, 0, 0, 0,
>   amap_small_pool_names[i], NULL);
> + pool_setipl(_small_amap_pool[i], IPL_NONE);
>   }
>  
>   pool_init(_amap_chunk_pool,
>   sizeof(struct vm_amap_chunk) +
>   UVM_AMAP_CHUNK * sizeof(struct vm_anon *), 0, 0, 0,
>   "amapchunkpl", NULL);
> + pool_setipl(_amap_chunk_pool, IPL_NONE);
>   pool_sethiwat(_amap_chunk_pool, 4096);
>  }
>  
> Index: uvm_anon.c
> ===
> RCS file: /cvs/src/sys/uvm/uvm_anon.c,v
> retrieving revision 1.46
> diff -u -p -r1.46 uvm_anon.c
> --- uvm_anon.c8 May 2016 11:52:32 -   1.46
> +++ uvm_anon.c13 Jun 2016 04:26:23 -
> @@ -50,6 +50,7 @@ uvm_anon_init(void)
>  {
>   pool_init(_anon_pool, sizeof(struct vm_anon), 0, 0,
>   PR_WAITOK, "anonpl", NULL);
> + pool_setipl(_anon_pool, IPL_NONE);
>   pool_sethiwat(_anon_pool, uvmexp.free / 16);
>  }
>  
> Index: uvm_aobj.c
> ===
> RCS file: /cvs/src/sys/uvm/uvm_aobj.c,v
> retrieving revision 1.80
> diff -u -p -r1.80 uvm_aobj.c
> --- uvm_aobj.c21 Aug 2015 16:04:35 -  1.80
> +++ uvm_aobj.c13 Jun 2016 04:26:23 -
> @@ -799,9 +799,11 @@ uao_init(void)
>*/
>   pool_init(_swhash_elt_pool, sizeof(struct uao_swhash_elt),
>   0, 0, PR_WAITOK, "uaoeltpl", NULL);
> + pool_setipl(_swhash_elt_pool, IPL_NONE);
>  
>   pool_init(_aobj_pool, sizeof(struct uvm_aobj), 0, 0, PR_WAITOK,
>   "aobjpl", NULL);
> + pool_setipl(_aobj_pool, IPL_NONE);
>  }
>  
>  /*
> Index: uvm_map.c
> ===
> RCS file: /cvs/src/sys/uvm/uvm_map.c,v
> retrieving revision 1.215
> diff -u -p -r1.215 uvm_map.c
> --- uvm_map.c 5 Jun 2016 08:35:57 -   1.215
> +++ uvm_map.c 13 Jun 2016 04:26:23 -
> @@ -2787,11 +2787,13 @@ uvm_map_init(void)
>   /* initialize the map-related pools. */
>   pool_init(_vmspace_pool, sizeof(struct vmspace),
>   0, 0, PR_WAITOK, "vmsppl", NULL);
> + pool_setipl(_vmspace_pool, IPL_NONE);
>   pool_init(_map_entry_pool, sizeof(struct vm_map_entry),
>   0, 0, PR_WAITOK, "vmmpepl", NULL);
>   pool_setipl(_map_entry_pool, IPL_VM);
>   pool_init(_map_entry_kmem_pool, sizeof(struct vm_map_entry),
>   0, 0, 0, "vmmpekpl", NULL);
> + pool_setipl(_map_entry_kmem_pool, IPL_VM);
>   pool_sethiwat(_map_entry_pool, 8192);
>  
>   uvm_addr_init();
> 
>